Bug 66396

Created attachment 103783 [details] [review]
systemd-cryptsetup patch

I created patch. Please, can you review this?

/etc/crypttab example:

mycryptdisk   /dev/sdc  /root/luks.key  luks,header=/root/secure/luksheader.img

Comment on attachment 103783 [details] [review]
systemd-cryptsetup patch

Review of attachment 103783 [details] [review]:
-----------------------------------------------------------------

::: src/cryptsetup/cryptsetup.c
@@ +140,5 @@
> +        } else if (startswith(option, "header=")) {
> +                arg_type = CRYPT_LUKS1;
> +
> +                if(!path_is_absolute(option+7))
> +                        log_error("Header path '%s' is not absolute. Ignoring.", option+7);

That seems dangerous. The whole --header business is pretty dangerous, so any misconfiguration should be treated as fatal.

@@ +143,5 @@
> +                if(!path_is_absolute(option+7))
> +                        log_error("Header path '%s' is not absolute. Ignoring.", option+7);
> +
> +                free(arg_header);
> +                arg_header = strdup(option+7);

Let's make this fatal too. If there are two header= options (i.e. arg_header was already set), refuse to continue.

@@ +405,4 @@
>                  r = crypt_load(cd, CRYPT_LUKS1, NULL);
>  
> +                if (data_device)
> +                        r = crypt_set_data_device(cd, data_device);

This assignment to r would overwrite the previous value, which hasn't been checked yet.

@@ +578,5 @@
>                  k = crypt_init(&cd, argv[3]);
> +
> +                if(arg_header) {
> +                        log_info("LUKS header: %s", arg_header);
> +                        k = crypt_init(&cd, arg_header);

Please make this log_debug.

@@ +634,4 @@
>                          if (streq_ptr(arg_type, CRYPT_TCRYPT))
>                                  k = attach_tcrypt(cd, argv[2], key_file, passwords, flags);
>                          else
> +                                k = attach_luks_or_plain(cd, argv[2], key_file, ( arg_header ? argv[3] : NULL), passwords, flags);

I don't grok this. Why argv[3] is used if arg_header is set?

I made some notes on the implementation. Looks reasonable in general, but please include changes to cryptsetup.xml and other man pages as necessary.

Created attachment 111105 [details] [review]
systemd-cryptsetup patch

Created attachment 111106 [details] [review]
crypttab.xml patch

(In reply to Zbigniew Jedrzejewski-Szmek from comment #3)

> @@ +634,4 @@
> >                          if (streq_ptr(arg_type, CRYPT_TCRYPT))
> >                                  k = attach_tcrypt(cd, argv[2], key_file, passwords, flags);
> >                          else
> > +                                k = attach_luks_or_plain(cd, argv[2], key_file, ( arg_header ? argv[3] : NULL), passwords, flags);
> 
> I don't grok this. Why argv[3] is used if arg_header is set?

argv[3] contains data_device if (and only if) arg_header is set.

> +                if (data_device)
> +                        r = crypt_set_data_device(cd, data_device);

From official docs:

> int crypt_set_data_device (struct crypt_device * cd, const char * device) 	
> Set data device For LUKS it is encrypted data device when LUKS header is separated.

See also http://wiki.cryptsetup.googlecode.com/git/API/libcryptsetup_8h.html#aff3afff2a83a64f80962afa446eb9dca (

If arg_header is not set then argv[3] contains regular LUKS-device.

Applied in http://cgit.freedesktop.org/systemd/systemd/commit/?id=7376e83528.