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

[Guilhem Moulin]

Hi,

On Thu, 17 Dec 2015 at 10:55:06 +0100, Jonas Meurer wrote:
> I've some comments and questions regarding your patch though. Have you
> tested the patch already?

Yes, but only on a resume device (and without LVM).  (But I don't see
how the same wouldn't work for other devices and/or LVM.)
 
> 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 ;)

I only wanted to avoid parsing the $OPTIONS string :-P  But indeed it's
easier to grep through $OPTIONS.  And not more error prone if we assume
that crypttab fields don't contain blanks or commas.

> See above. I think, that the following would be sufficient:
> 
> if echo "$OPTIONS" | grep -q "\brootdev\b"; then

As pointed out on IRC, word delimiters are error prone since
‘key=/path/to/rootdev.key’ would match, for instance.  But

    if echo "$OPTIONS" | grep -q "^(.*,)?rootdev(,.*)?$"; then

looks fine.

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

Oh, right, stat's -L flag deferences symlink, but doesn't take the
canonical path, indeed.

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

Makes sense, fixed and new patch enclosed.

Cheers,
-- 
Guilhem.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-keyfile-support-for-non-root-devices.patch
Type: text/x-diff
Size: 3114 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-cryptsetup-devel/attachments/20151219/c5412cee/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-cryptsetup-devel/attachments/20151219/c5412cee/attachment.sig>