[arch-projects] [RFC][PATCH][initscripts] cryptsetup: add keyfile-size= support
This is useful e.g. if the keyfile is a raw device, where only parts of it should be read. It is typically used whenever the keyfile-offset= option is specified. --- This patch is compile-tested only, and just meant for gathering feedback. Do you guys agree that this is necessary and sufficient to cover the use-case brought up by Heiko? Cheers, Tom man/crypttab.xml | 11 +++++++++++ src/cryptsetup/cryptsetup.c | 22 +++++++++++++++------- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/man/crypttab.xml b/man/crypttab.xml index 2fa8e89..ba3f750 100644 --- a/man/crypttab.xml +++ b/man/crypttab.xml @@ -131,6 +131,17 @@ <varlistentry> + <term><varname>keyfile-size=</varname></term> + + <listitem><para>Specifies the maximum number + of bytes to read from the keyfile; see + <citerefentry><refentrytitle>cryptsetup</refentrytitle><manvolnum>8</manvolnum></citerefentry> + for possible values and the default + value of this option.</para></listitem> + </varlistentry> + + + <varlistentry> <term><varname>keyfile-offset=</varname></term> <listitem><para>Specifies the number diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index cc30e50..1035ac4 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -37,6 +37,7 @@ static const char *opt_type = NULL; /* LUKS1 or PLAIN */ static char *opt_cipher = NULL; static unsigned opt_key_size = 0; +static unsigned opt_keyfile_size = 0; static unsigned opt_keyfile_offset = 0; static char *opt_hash = NULL; static unsigned opt_tries = 0; @@ -80,6 +81,13 @@ static int parse_one_option(const char *option) { return 0; } + } else if (startswith(option, "keyfile-size=")) { + + if (safe_atou(option+13, &opt_keyfile_offset) < 0) { + log_error("keyfile-size= parse failure, ignoring."); + return 0; + } + } else if (startswith(option, "keyfile-offset=")) { if (safe_atou(option+15, &opt_keyfile_offset) < 0) { @@ -238,7 +246,6 @@ int main(int argc, char *argv[]) { char **passwords = NULL, *truncated_cipher = NULL; const char *cipher = NULL, *cipher_mode = NULL, *hash = NULL, *name = NULL; char *description = NULL, *name_buffer = NULL, *mount_point = NULL; - unsigned keyfile_size = 0; if (argc <= 1) { help(); @@ -437,6 +444,11 @@ int main(int argc, char *argv[]) { zero(params); params.hash = hash; + /* for CRYPT_PLAIN limit reads + * from keyfile to key length, and + * ignore keyfile-size */ + opt_keyfile_size = opt_key_size / 8; + /* In contrast to what the name * crypt_setup() might suggest this * doesn't actually format anything, @@ -448,14 +460,10 @@ int main(int argc, char *argv[]) { cipher_mode, NULL, NULL, - opt_key_size / 8, + opt_keyfile_size, ¶ms); pass_volume_key = streq(hash, "plain"); - - /* for CRYPT_PLAIN limit reads - * from keyfile to key length */ - keyfile_size = opt_key_size / 8; } if (k < 0) { @@ -470,7 +478,7 @@ int main(int argc, char *argv[]) { argv[3]); if (key_file) - k = crypt_activate_by_keyfile_offset(cd, argv[2], CRYPT_ANY_SLOT, key_file, keyfile_size, + k = crypt_activate_by_keyfile_offset(cd, argv[2], CRYPT_ANY_SLOT, key_file, opt_keyfile_size, opt_keyfile_offset, flags); else { char **p; -- 1.7.11.3
On Sun, Jul 29, 2012 at 2:15 AM, Tom Gundersen <teg@jklm.no> wrote:
This is useful e.g. if the keyfile is a raw device, where only parts of it should be read. It is typically used whenever the keyfile-offset= option is specified. ---
This patch is compile-tested only, and just meant for gathering feedback. Do you guys agree that this is necessary and sufficient to cover the use-case brought up by Heiko?
If anyone wants to try this out and don't want to compile systemd, I threw up a binary here: <https://dev.archlinux.org/~tomegun/systemd-cryptsetup>. Please test this only on un-important data. I have not tested it myself yet. -t
On 07/28/2012 06:15 PM, Tom Gundersen wrote:
This is useful e.g. if the keyfile is a raw device, where only parts of it should be read. It is typically used whenever the keyfile-offset= option is specified. ---
This patch is compile-tested only, and just meant for gathering feedback. Do you guys agree that this is necessary and sufficient to cover the use-case brought up by Heiko?
Cheers,
Tom
man/crypttab.xml | 11 +++++++++++ src/cryptsetup/cryptsetup.c | 22 +++++++++++++++------- 2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/man/crypttab.xml b/man/crypttab.xml index 2fa8e89..ba3f750 100644 --- a/man/crypttab.xml +++ b/man/crypttab.xml @@ -131,6 +131,17 @@
Sorry to if I restate obvious things but part of the problem is all of the similarly named flags. This patch removes the double-duty for size= corresponding to --key-size and --keyfile-size. We are left with size= just meaning --key-size and no ability to pass the --size option. I *think* this leaves us with how the cryptsetup frontend handles the two options. HOWEVER, there are still problems with systemd-cryptsetup. The most obvious thing is that sysd uses crypt_activate_by_volume_key(), but cryptsetup itself only uses this in conjunction with the --master-key-file option. The two might be equivalent for PLAIN with no hashing because the data read from the keyfile is the "master key," but I'm worried that their are some subtleties that won't be supported. In the end I think it would be best to support ALL options from cryptsetup that are used for "cryptsetup luksOpen" and/or "cryptsetup create" regardless of whether we think they're only useful for ad hoc mappings.
On Sun, Jul 29, 2012 at 6:34 PM, Matthew Monaco <dgbaley27@0x01b.net> wrote:
On 07/28/2012 06:15 PM, Tom Gundersen wrote:
This is useful e.g. if the keyfile is a raw device, where only parts of it should be read. It is typically used whenever the keyfile-offset= option is specified. ---
This patch is compile-tested only, and just meant for gathering feedback. Do you guys agree that this is necessary and sufficient to cover the use-case brought up by Heiko?
Cheers,
Tom
man/crypttab.xml | 11 +++++++++++ src/cryptsetup/cryptsetup.c | 22 +++++++++++++++------- 2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/man/crypttab.xml b/man/crypttab.xml index 2fa8e89..ba3f750 100644 --- a/man/crypttab.xml +++ b/man/crypttab.xml @@ -131,6 +131,17 @@
Sorry to if I restate obvious things but part of the problem is all of the similarly named flags. This patch removes the double-duty for size= corresponding to --key-size and --keyfile-size. We are left with size= just meaning --key-size and no ability to pass the --size option.
Well, not exactly. If keyfile-size is unset, or set at 0, then this patch does not make a difference at all. Moreover, when using PLAIN encryption, keyfile-size is always ignored, and calculated from key-size as before. I'll add a note about this to the manpage. I'll also fix a typo I just noticed in the patch ;-)
I *think* this leaves us with how the cryptsetup frontend handles the two options.
HOWEVER, there are still problems with systemd-cryptsetup. The most obvious thing is that sysd uses crypt_activate_by_volume_key(), but cryptsetup itself only uses this in conjunction with the --master-key-file option. The two might be equivalent for PLAIN with no hashing because the data read from the keyfile is the "master key," but I'm worried that their are some subtleties that won't be supported.
Hm, this seems like a separate issue. Worth looking into though.
In the end I think it would be best to support ALL options from cryptsetup that are used for "cryptsetup luksOpen" and/or "cryptsetup create" regardless of whether we think they're only useful for ad hoc mappings.
I think the best approach would be to submit one patch per feature (and make sure to find a use-case for it). -t
participants (2)
-
Matthew Monaco
-
Tom Gundersen