Add better failure handling in case CephFS with fscrypt fails - #5115
Add better failure handling in case CephFS with fscrypt fails#5115nixpanic wants to merge 2 commits into
Conversation
Signed-off-by: Niels de Vos <ndevos@ibm.com>
Madhu-1
left a comment
Madhu-1
left a comment
There was a problem hiding this comment.
32 limit is required for SourceType_raw_key and it going to be same requirement for SourceType_custom_passphrase because in cephcsi both are supported, The current change might affect both? i have checked the complete code but just mentioned so that we consider it as well.
@nixpanic in CI we have encryption disabled due to dependency, do we still need to keep it disabled or we can enable it as its good time to do it (if possible)
|
|
||
| protector, err := fscryptactions.CreateProtector(fscryptContext, protectorName, keyFn, owner) | ||
| if err != nil { | ||
| if err != nil && protector != nil { |
There was a problem hiding this comment.
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 protector.Revert() as well
| // 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) { |
There was a problem hiding this comment.
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.
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.
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?
| passphrase, err = resizePassphrase(passphrase, keySize) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
can we also check wont this change break the change added in #4464?
There was a problem hiding this comment.
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)
It seems that fscrypt expects a key with exactly 32 bytes. In order to use a random length key from a KMS, either repeat the key until the length is reached, or trim the key when needed. See: https://github.com/google/fscrypt/tree/v0.3.4#using-a-raw-key-protector Signed-off-by: Niels de Vos <ndevos@ibm.com>
d9d3858 to
e4dc6d6
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically closed due to inactivity. Please re-open if these changes are still required. |
2 participants
It seems that fscrypt expects a key with exactly 32 bytes. In order to
use a random length key from a KMS, either repeat the key until the
length is reached, or trim the key when needed.
See: https://github.com/google/fscrypt/tree/v0.3.4#using-a-raw-key-protector
Test-container-image:
quay.io/nixpanic/cephcsi:rhstor-6834Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)