-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add better failure handling in case CephFS with fscrypt fails #5115
base: devel
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,8 @@ var policyV2Support = []util.KernelVersion{ | |
|
||
// error values | ||
var ( | ||
ErrBadAuth = errors.New("key authentication check failed") | ||
ErrBadAuth = errors.New("key authentication check failed") | ||
ErrEmptyPassphrase = errors.New("empty passphrase given") | ||
) | ||
|
||
func AppendEncyptedSubdirectory(dir string) string { | ||
|
@@ -94,14 +95,35 @@ func getPassphrase(ctx context.Context, encryption util.VolumeEncryption, volID | |
return passphrase, nil | ||
} | ||
|
||
// resizePassphrase makes sure that the given passphrase will be of [size] | ||
// bytes. In case the passphrase is shorter, it will be repeated as many times | ||
// as needed. When a passphrase is (or becomes) longer than the requested | ||
// [size], the passphrase in truncated. | ||
func resizePassphrase(passphrase string, size int) (string, error) { | ||
if passphrase == "" || size <= 0 { | ||
return "", ErrEmptyPassphrase | ||
} | ||
|
||
for len(passphrase) < size { | ||
passphrase += passphrase | ||
} | ||
if len(passphrase) > size { | ||
passphrase = string([]byte(passphrase)[:size]) | ||
} | ||
|
||
return passphrase, nil | ||
} | ||
|
||
// createKeyFuncFromVolumeEncryption returns an fscrypt key function returning | ||
// encryption keys from a VolumeEncryption struct. | ||
func createKeyFuncFromVolumeEncryption( | ||
ctx context.Context, | ||
encryption util.VolumeEncryption, | ||
volID string, | ||
keySize int, | ||
) (func(fscryptactions.ProtectorInfo, bool) (*fscryptcrypto.Key, error), error) { | ||
// keys must be 32 bytes, see https://github.com/google/fscrypt?tab=readme-ov-file#using-a-raw-key-protector | ||
keySize := encryptionPassphraseSize / 2 | ||
|
||
keyFunc := func(info fscryptactions.ProtectorInfo, retry bool) (*fscryptcrypto.Key, error) { | ||
if retry { | ||
return nil, ErrBadAuth | ||
|
@@ -112,9 +134,11 @@ func createKeyFuncFromVolumeEncryption( | |
return nil, err | ||
} | ||
|
||
if keySize < 0 { | ||
keySize = len(passphrase) | ||
passphrase, err = resizePassphrase(passphrase, keySize) | ||
if err != nil { | ||
return nil, err | ||
Comment on lines
+137
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also check wont this change break the change added in #4464? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I still need to figure out how the different protector is created, and then only do the password resizing for the raw key protector(s) |
||
} | ||
|
||
key, err := fscryptcrypto.NewBlankKey(keySize) | ||
copy(key.Data(), passphrase) | ||
|
||
|
@@ -171,7 +195,7 @@ func unlockExisting( | |
errMsg := fmt.Sprintf("fscrypt: unlock with protector error: %v", err) | ||
log.ErrorLog(ctx, "%s, retry using a null padded passphrase", errMsg) | ||
|
||
keyFn, err = createKeyFuncFromVolumeEncryption(ctx, *volEncryption, volID, encryptionPassphraseSize/2) | ||
keyFn, err = createKeyFuncFromVolumeEncryption(ctx, *volEncryption, volID) | ||
if err != nil { | ||
log.ErrorLog(ctx, "fscrypt: could not create key function: %v", err) | ||
|
||
|
@@ -217,12 +241,14 @@ func initializeAndUnlock( | |
} | ||
|
||
protector, err := fscryptactions.CreateProtector(fscryptContext, protectorName, keyFn, owner) | ||
if err != nil { | ||
if err != nil && protector != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i looked at the CreateProtector it will always return the protector as nil when there is an error, we don't need a check for the protector if there is an error as nothing is to be reverted for that protector we can remove below code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good, thanks! |
||
log.ErrorLog(ctx, "fscrypt: protector name=%s create failed: %v. reverting.", protectorName, err) | ||
if revertErr := protector.Revert(); revertErr != nil { | ||
return revertErr | ||
} | ||
|
||
return err | ||
} else if err != nil { | ||
return err | ||
} | ||
|
||
|
@@ -374,7 +400,7 @@ func Unlock( | |
stagingTargetPath string, volID string, | ||
) error { | ||
// Fetches keys from KMS. Do this first to catch KMS errors before setting up anything. | ||
keyFn, err := createKeyFuncFromVolumeEncryption(ctx, *volEncryption, volID, -1) | ||
keyFn, err := createKeyFuncFromVolumeEncryption(ctx, *volEncryption, volID) | ||
if err != nil { | ||
log.ErrorLog(ctx, "fscrypt: could not create key function: %v", err) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
/* | ||
Copyright 2025 The Ceph-CSI Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
package fscrypt | ||
|
||
import ( | ||
"errors" | ||
"testing" | ||
) | ||
|
||
func TestResizePassphrase(t *testing.T) { | ||
t.Parallel() | ||
tests := []struct { | ||
name string | ||
passphrase string | ||
size int | ||
ret string | ||
err error | ||
}{ | ||
{ | ||
"matching passphrase size", | ||
"secret", | ||
6, | ||
"secret", | ||
nil, | ||
}, | ||
{ | ||
"short passphrase", | ||
"secret", | ||
64, | ||
"secretsecretsecretsecretsecretsecretsecretsecretsecretsecretsecr", | ||
nil, | ||
}, | ||
{ | ||
"long passphrase", | ||
"secret", | ||
2, | ||
"se", | ||
nil, | ||
}, | ||
{ | ||
"half a passphrase", | ||
"secret", | ||
3, | ||
"sec", | ||
nil, | ||
}, | ||
{ | ||
"a little too shot passphrase", | ||
"secret", | ||
7, | ||
"secrets", | ||
nil, | ||
}, | ||
{ | ||
"empty passphrase", | ||
"", | ||
16, | ||
"", | ||
ErrEmptyPassphrase, | ||
}, | ||
{ | ||
"zero length requested", | ||
"secret", | ||
0, | ||
"", | ||
ErrEmptyPassphrase, | ||
}, | ||
{ | ||
"negative length requested", | ||
"secret", | ||
-32, | ||
"", | ||
ErrEmptyPassphrase, | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
t.Parallel() | ||
|
||
ret, err := resizePassphrase(tt.passphrase, tt.size) | ||
if ret != tt.ret { | ||
t.Errorf("resizePassphrase() returned %q of %d bytes, expected %q of %d bytes", tt.ret, len(tt.ret), ret, len(ret)) | ||
} | ||
if !errors.Is(err, tt.err) { | ||
t.Errorf("resizePassphrase() returned %v as error, expected %v", err, tt.err) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume this function is added to make sure we always get the size of 32 which is required what will happen if the passphrase size is 8 or something even if its added it will not become 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the added unit test
TestResizePassphrase
, I think it works as described, no matter what size of password is used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a case where the passphrase length is 3 and the size is 7 and validate returned passphrase length is matching the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added them now.