Hi Guilhem, Am 12.12.2015 um 20:38 schrieb Guilhem Moulin: > Here is a simple patch that adds keyfile support for non-root devices. > Ideally we would also warn the user if the key file is stored on an > unencrypted device, but that requires some code refactoring in the hook > file so it'll come in a later patch.
I like your idea to check whether the keyfile is on the rootfs for resume and other non-rootfs devices that are processed during initramfs. I've some comments and questions regarding your patch though. Have you tested the patch already? > 0001-Add-keyfile-support-for-non-root-devices.patch > > > From efcd427201f7c0b6835e8bdedc559bd5623bc87e Mon Sep 17 00:00:00 2001 > From: Guilhem Moulin <[email protected]> > Date: Sat, 12 Dec 2015 20:04:56 +0100 > Subject: [PATCH] Add keyfile support for non-root devices. > > --- > debian/changelog | 3 +++ > debian/initramfs/cryptroot-hook | 31 ++++++++++++++++++++----------- > debian/initramfs/cryptroot-script | 3 +++ > 3 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/debian/changelog b/debian/changelog > index ea1f2c4..fa3c2c1 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -50,6 +50,9 @@ cryptsetup (2:1.7.0-1~mejo2) mejo-unstable; urgency=medium > storing keyfiles directly in the initrd. (closes: #786578) > * debian/initramfs/cryptroot-hook: display a warning for invalid source > devices (closes: #720515, #781955, #784435) > + * debian/initramfs/cryptroot-{hook,script}: add keyfile support for > non-root > + devices. A warning is printed if the keyfile itself in stored in the > root > + partition. (closes: #774647) > > -- Jonas Meurer <[email protected]> Thu, 10 Dec 2015 13:30:03 +0100 > > diff --git a/debian/initramfs/cryptroot-hook b/debian/initramfs/cryptroot-hook > index 4e42086..f1bda44 100644 > --- a/debian/initramfs/cryptroot-hook > +++ b/debian/initramfs/cryptroot-hook > @@ -232,11 +232,12 @@ get_lvm_deps() { > } > > get_device_opts() { > - local target source link extraopts rootopts opt key > + local target source link extraopts rootopts opt key isrootdev > target="$1" > extraopts="$2" > + isrootdev="$3" What's the reason for introducing the isrootdev variable? Why not use the "rootdev" option that's already there? Honestly, I don't get it. Probably I miss something, though ;) */ You set isrootdev=n at the beginning of add_device(). If the device is an underlying device for rootfs , then rootdev is added to $opts (old) and now, isrootdev=y is set additionally (new). The value of isrootdev is given to get_device_opts() as $3. */ At the start of get_device_opts(), isrootdev is set to the value of $3. If the opt "rootdev" is set, then isrootdev=y is set. */ Up to now, the variable $isrootdev is only set to 'y' in exactly the same situations as that the opt "rootdev" is set as well. If rootdev is not set in $opts, then $isroodev=n. So in my eyes, the opt "rootdev" and the variable $isrootdev are consistent. */ Later in get_device_opts() when the keyfiles are processed, you check for $isrootdev and warn if it $isrootdev!=n. Why not simply search for "rootdev" in $OPTIONS instead? See below for a suggestion. > KEYSCRIPT="" > - KEYFILE="" > + KEYFILE="" # key file to copy in the initramfs image > CRYPTHEADER="" > OPTIONS="" > > @@ -340,6 +341,7 @@ get_device_opts() { > OPTIONS="$OPTIONS,$opt" > ;; > rootdev) > + isrootdev=y See above. > OPTIONS="$OPTIONS,$opt" > ;; > *) > @@ -371,8 +373,17 @@ get_device_opts() { > key="/cryptroot-keyfiles/${target}.key" > ;; > *) > - echo "cryptsetup: WARNING: target $target uses > a key file, skipped" >&2 > - return 1 > + if [ "$isrootdev" != n ]; then See above. I think, that the following would be sufficient: if echo "$OPTIONS" | grep -q "\brootdev\b"; then > + echo "cryptsetup: WARNING: root target > $target uses a key file, skipped" >&2 > + return 1; > + elif [ "$(stat -Lc %m -- "$key" 2>/dev/null)" > != / ]; then Would need to stat against the canonical file (e.g. after readlink) here. Otherwise, the keyfile path can be a link on the rootfs that points to another partition. Should be as easy as adding: keylink=$(readlink -e "$dev") just before the case selection and running stat against $keylink. > + echo "cryptsetup: WARNING: $target's > key file $key is not on the root FS, skipped" >&2 > + return 1; > + else > + > OPTIONS="$OPTIONS,keyscript=cat-rootmnt" # prefix $rootmnt in local-block > + # TODO: warn if the keyfile is not > stored on an encrypted device > + fi No need for the TODO in my eyes. We only get here, if the key is stored on the rootfs. In this case, we don't fiddle around with the keyfile at all. All we do, is change the path to the keyfile in cryptroot-script, so the keyfile itself is not touched by us at all. Sure, it would be nice to warn the user if she stores the keyfile on an unencrypted root fs, but then this is just one more corner case where a user implements an uncommon custom setup in an unsecure fashion, and we cannot check against all those corner cases anyway. > + key=$(readlink -f "$key") Would be key="$keylink" then. > ;; > esac > fi > @@ -455,10 +466,11 @@ canonical_device() { > } > > add_device() { > - local node nodes opts lastopts i count > + local node nodes opts lastopts i count isrootdev > nodes="$1" > opts="" # Applied to all nodes > lastopts="" # Applied to last node > + isrootdev=n See above. > > if [ -z "$nodes" ]; then > return 0 > @@ -466,11 +478,8 @@ add_device() { > > # Flag root device > if echo "$rootdevs" | grep -q "\b$nodes\b"; then > - if [ -z "$opts" ]; then > - opts="rootdev" > - else > - opts="$opts,rootdev" > - fi > + opts="${opts:+$opts,}rootdev" I like :) > + isrootdev=y See above. > fi > > # Check that it is a node under /dev/mapper/ > @@ -509,7 +518,7 @@ add_device() { > fi > > # Get crypttab root options > - if ! get_device_opts "$node" "$opts"; then > + if ! get_device_opts "$node" "$opts" "$isrootdev"; then See above. > continue > fi > echo "$OPTIONS" >>"$DESTDIR/conf/conf.d/cryptroot" > diff --git a/debian/initramfs/cryptroot-script > b/debian/initramfs/cryptroot-script > index e450580..877161a 100644 > --- a/debian/initramfs/cryptroot-script > +++ b/debian/initramfs/cryptroot-script > @@ -304,6 +304,9 @@ setup_mapping() > cryptkeyscript="/lib/cryptsetup/askpass" > cryptkey="Please unlock disk $diskname: " > fi > + elif [ "$cryptkeyscript" = "cat-rootmnt" ]; then > + cryptkeyscript=cat > + cryptkey="${rootmnt}${cryptkey}" > fi > > > -- 2.6.4 Cheers jonas
signature.asc
Description: OpenPGP digital signature
