[pkg-cryptsetup-devel] Bug#774647: Bug#774647: cryptsetup on initramfs does not support key files (resume swap on LVM)

Thu Dec 17 09:55:06 UTC 2015

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 <guilhem at guilhem.org>
> 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 <mejo at debian.org>  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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-cryptsetup-devel/attachments/20151217/7130ad18/attachment.sig>