Skip to content

Conversation

@poettering
Copy link
Member

No description provided.

@poettering poettering force-pushed the cryptsetup-literal branch 2 times, most recently from 46dcf1f to 66a9014 Compare April 29, 2020 21:45
@poettering
Copy link
Member Author

Force pushed new, rebased version. PTAL!

Copy link
Member

@keszybz keszybz left a 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="))) {
Copy link
Member

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));
Copy link
Member

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.

Copy link
Member Author

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,
Copy link
Member

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...

@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 19, 2020
poettering added 11 commits May 19, 2020 17:27
…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.)
@poettering poettering force-pushed the cryptsetup-literal branch from 4b274a6 to 6e41f4d Compare May 19, 2020 15:34
@poettering
Copy link
Member Author

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.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 19, 2020
@poettering poettering linked an issue May 19, 2020 that may be closed by this pull request
@keszybz
Copy link
Member

keszybz commented May 19, 2020

LGTM.

@poettering poettering merged commit d31dda5 into systemd:master May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptsetup good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed util-lib

Development

Successfully merging this pull request may close these issues.

truecrypt: tries= not honored in crypttab

2 participants