Bug 66396 - RFE: implement --header option in systemd-cryptsetup/crypttab
Summary: RFE: implement --header option in systemd-cryptsetup/crypttab
Status: RESOLVED FIXED
Alias: None
Product: systemd
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: systemd-bugs
QA Contact: systemd-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-30 12:54 UTC by andr
Modified: 2015-01-08 21:34 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
systemd-cryptsetup patch (3.11 KB, patch)
2014-08-01 04:49 UTC, Chaser
Details | Splinter Review
systemd-cryptsetup patch (3.42 KB, patch)
2014-12-21 12:16 UTC, Chaser
Details | Splinter Review
crypttab.xml patch (1.07 KB, patch)
2014-12-21 12:17 UTC, Chaser
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description andr 2013-06-30 12:54:59 UTC
Original cryptsetup has an --header option:

man cryptsetup:
"--header <device or file storing the LUKS header>
Use  a  detached  (separated)  metadata  device or file where the LUKS header is stored. This options allows to store ciphertext and LUKS header on different
devices."

But there are no such option in systemd-cryptsetup. I cannot use it in crypttab too. What about of it's implementation?

It can be used for creation "hidden" volumes, which are not contains any metadata about chipper end encryption type.
From my point of view, it will be useful feature.
Comment 1 Chaser 2014-08-01 04:49:28 UTC
Created attachment 103783 [details] [review]
systemd-cryptsetup patch

I created patch. Please, can you review this?
Comment 2 Chaser 2014-08-01 04:52:24 UTC
/etc/crypttab example:

mycryptdisk   /dev/sdc  /root/luks.key  luks,header=/root/secure/luksheader.img
Comment 3 Zbigniew Jedrzejewski-Szmek 2014-08-03 05:04:56 UTC
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?
Comment 4 Zbigniew Jedrzejewski-Szmek 2014-08-03 05:05:58 UTC
I made some notes on the implementation. Looks reasonable in general, but please include changes to cryptsetup.xml and other man pages as necessary.
Comment 5 Chaser 2014-12-21 12:16:35 UTC
Created attachment 111105 [details] [review]
systemd-cryptsetup patch
Comment 6 Chaser 2014-12-21 12:17:29 UTC
Created attachment 111106 [details] [review]
crypttab.xml patch
Comment 7 Chaser 2014-12-21 12:27:08 UTC
(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 (
Comment 8 Chaser 2014-12-21 12:31:45 UTC
If arg_header is not set then argv[3] contains regular LUKS-device.
Comment 9 Zbigniew Jedrzejewski-Szmek 2015-01-08 21:34:55 UTC
Applied in http://cgit.freedesktop.org/systemd/systemd/commit/?id=7376e83528.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.