-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
a number of cryptsetup fixes and additions #15637
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
Conversation
46dcf1f to
66a9014
Compare
66a9014 to
4b274a6
Compare
|
Force pushed new, rebased version. PTAL! |
keszybz
left a comment
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.
Looks reasonable. Some minor comments on the user-visible parts.
|
|
||
| } else if ((val = startswith(option, "key-slot="))) { | ||
| } else if ((val = startswith(option, "key-slot=")) || | ||
| (val = startswith(option, "keyslot="))) { |
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.
Beautiful ;(
| * is still linked by someone else and we'll leave it to them to remove it securely | ||
| * eventually! */ | ||
|
|
||
| random_bytes(buffer, sizeof(buffer)); |
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.
Do we care about the quality, i.e. why not use pseudo_random_bytes() here? This should jsut not be easily compressible and such, so anything even slightly random is more than good enough.
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.
we don't care about the quality much. that's why we use random_bytes() instead of genuine_random_bytes()... random_bytes() after all tries to get data from kernel if it can and doesn't mean blocking and uses genuine_random_bytes() otherwise.
But you are right we could no bother with the kernel random buffer at all here, and do entirely pseudo-random stuff here. But it appeared to me this would be unnecessarily sloppy. I mean, we do this to flush out crypto keys, and if it's no real difference to us it appears to still be better to overwrite with actual random stuff if we can get it cheaply than guessable pseudo-random stuff always.
I mean, in a way this is supposed to be as close to what shred(1) does is we can, except it should be graceful and quick and not be needlessly paranoid. shred() overwrites everything 3 times or so with random data generated from /dev/urandom. Now I am not sure we want to spend the 3 times thing, we want to be quick after all, but I'd still do the /dev/urandom thing if it's cheap and easy to do. And it is for us, it's just a slightly different function we have to call... Hope that makes some sense?
|
|
||
| int load_key_file( | ||
| const char *key_file, | ||
| char **search_path, |
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.
The usual comment for the future: it's much easier to review stuff when the move is done in a separate commit from the additions...
…for erasing With that it becomes useful for deleting password files and such.
Support some aliases Debian added, and drop some options that Debian dropped from our list of unsupported options.
This is useful when the key file is acquired dynamically in some form and should be erased after use. Note that this code tries to be robust, and removes the key file both on success and on failure.
…ath logic Let's do some rearrangements, so that we can later on use this to automatically search for a key file.
Let's make loading of keys a bit more automatic and define a common place where key files can be placed. Specifically, whenever a volume of name "foo" is attempted, search for a key file in /etc/cryptsetup-keys.d/foo.key and /run/cryptsetup-keys.d/foo.key, unless a key file is declared explicitly. With this scheme we have a simple discovery in place that should make it more straightfoward wher to place keys, and requires no explicit configuration to be used.
Only then we'll try again to ask the user for a password. Fixes: systemd#12152
… file system This adds a new switch try-empty-password. If set and none of PKCS#11 or key files work, it is attempted to unlock the volume with an empty password, before the user is asked for a password. Usecase: an installer generates an OS image on one system, which is the booted up for the first time in a possibly different system. The image is encrypted using a random volume key, but an empty password. A tool that runs on first boot then queries the user for a password to set or enrols the volume in the TPM, removing the empty password. (Of course, in such a scenario it is important to never reuse the installer image on multiple systems as they all will have the same volume key, but that's a different question.)
4b274a6 to
6e41f4d
Compare
|
Thanks a million for the review. And I might even better my English this was. Double win! ;-) Force pushed new version. Only changes are the suggested ones. Given those were all in docs/comments taking liberty to set green label. |
|
LGTM. |
No description provided.