[PATCH options v2 0/3] options: Allow multiple and default --key parameters.
Refactor this code to use guestfs_int_split_string function which slightly simplifies it. This should have no effect. --- options/keys.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/options/keys.c b/options/keys.c index f783066..74b5497 100644 --- a/options/keys.c +++ b/options/keys.c @@ -153,49 +153,42 @@ get_key (struct key_store *ks, const char *device) } struct key_store * -key_store_add_from_selector (struct key_store *ks, const char *selector_orig) +key_store_add_from_selector (struct key_store *ks, const char *selector) { - CLEANUP_FREE char *selector = strdup (selector_orig); - const char *elem; - char *saveptr; + CLEANUP_FREE_STRING_LIST char **fields = + guestfs_int_split_string (':', selector); struct key_store_key key; - if (!selector) - error (EXIT_FAILURE, errno, "strdup"); + if (!fields) + error (EXIT_FAILURE, errno, "guestfs_int_split_string"); - /* 1: device */ - elem = strtok_r (selector, ":", &saveptr); - if (!elem) { + if (guestfs_int_count_strings (fields) != 3) { invalid_selector: - error (EXIT_FAILURE, 0, "invalid selector for --key: %s", selector_orig); + error (EXIT_FAILURE, 0, "invalid selector for --key: %s", selector); } - key.device = strdup (elem); + + /* 1: device */ + key.device = strdup (fields[0]); if (!key.device) error (EXIT_FAILURE, errno, "strdup"); /* 2: key type */ - elem = strtok_r (NULL, ":", &saveptr); - if (!elem) - goto invalid_selector; - else if (STREQ (elem, "key")) + if (STREQ (fields[1], "key")) key.type = key_string; - else if (STREQ (elem, "file")) + else if (STREQ (fields[1], "file")) key.type = key_file; else goto invalid_selector; /* 3: actual key */ - elem = strtok_r (NULL, ":", &saveptr); - if (!elem) - goto invalid_selector; switch (key.type) { case key_string: - key.string.s = strdup (elem); + key.string.s = strdup (fields[2]); if (!key.string.s) error (EXIT_FAILURE, errno, "strdup"); break; case key_file: - key.file.name = strdup (elem); + key.file.name = strdup (fields[2]); if (!key.file.name) error (EXIT_FAILURE, errno, "strdup"); break; -- 2.23.0
On Tuesday, 26 November 2019 17:40:46 CET Richard W.M. Jones wrote:
Refactor this code to use guestfs_int_split_string function which slightly simplifies it. This should have no effect. --- options/keys.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-)
LGTM. Thanks, -- Pino Toscano
This allows multiple --key parameters on the command line to match a single device. For example: tool --key /dev/sda1:key:trykey1 --key /dev/sda1:key:trykey2 would try "trykey1" and "trykey2" against /dev/sda1. --- options/decrypt.c | 41 ++++++++++++++++++++++++++++++++--------- options/keys.c | 45 +++++++++++++++++++++++++++++++++++---------- options/options.h | 6 ++++-- 3 files changed, 71 insertions(+), 21 deletions(-) diff --git a/options/decrypt.c b/options/decrypt.c index 234163d..131e79c 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -26,6 +26,8 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <libintl.h> +#include <error.h> #include "c-ctype.h" @@ -74,21 +76,42 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) if (partitions == NULL) exit (EXIT_FAILURE); - int need_rescan = 0; - size_t i; + int need_rescan = 0, r; + size_t i, j; + for (i = 0; partitions[i] != NULL; ++i) { CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]); if (type && STREQ (type, "crypto_LUKS")) { char mapname[32]; make_mapname (partitions[i], mapname, sizeof mapname); - CLEANUP_FREE char *key = get_key (ks, partitions[i]); - /* XXX Should we call guestfs_luks_open_ro if readonly flag - * is set? This might break 'mount_ro'. - */ - if (guestfs_luks_open (g, partitions[i], key, mapname) == -1) - exit (EXIT_FAILURE); - + CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i]); + + if (guestfs_int_count_strings (keys) == 0) + error (EXIT_FAILURE, 0, + _("no key was provided to open LUKS encrypted %s, " + "try using --key on the command line"), + partitions[i]); + + /* Try each key in turn. */ + for (j = 0; keys[j] != NULL; ++j) { + /* XXX Should we call guestfs_luks_open_ro if readonly flag + * is set? This might break 'mount_ro'. + */ + guestfs_push_error_handler (g, NULL, NULL); + r = guestfs_luks_open (g, partitions[i], keys[j], mapname); + guestfs_pop_error_handler (g); + if (r == 0) + goto opened; + } + error (EXIT_FAILURE, 0, + _("could not find key to open LUKS encrypted %s.\n\n" + "Try using --key on the command line.\n\n" + "Original error: %s (%d)"), + partitions[i], guestfs_last_error (g), + guestfs_last_errno (g)); + + opened: need_rescan = 1; } } diff --git a/options/keys.c b/options/keys.c index 74b5497..782bdb6 100644 --- a/options/keys.c +++ b/options/keys.c @@ -121,15 +121,32 @@ read_first_line_from_file (const char *filename) return ret; } -char * -get_key (struct key_store *ks, const char *device) +/* Return the key(s) matching this particular device from the + * keystore. There may be multiple. If none are read from the + * keystore, ask the user. + */ +char ** +get_keys (struct key_store *ks, const char *device) { - size_t i; + size_t i, j, len; + char **r; + char *s; + + /* We know the returned list must have at least one element and not + * more than ks->nr_keys. + */ + len = 1; + if (ks) + len = MIN (1, ks->nr_keys); + r = calloc (len+1, sizeof (char *)); + if (r == NULL) + error (EXIT_FAILURE, errno, "calloc"); + + j = 0; if (ks) { for (i = 0; i < ks->nr_keys; ++i) { struct key_store_key *key = &ks->keys[i]; - char *s; if (STRNEQ (key->device, device)) continue; @@ -139,17 +156,25 @@ get_key (struct key_store *ks, const char *device) s = strdup (key->string.s); if (!s) error (EXIT_FAILURE, errno, "strdup"); - return s; + r[j++] = s; + break; case key_file: - return read_first_line_from_file (key->file.name); + s = read_first_line_from_file (key->file.name); + r[j++] = s; + break; } - - /* Key not found in the key store, ask the user for it. */ - break; } } - return read_key (device); + if (j == 0) { + /* Key not found in the key store, ask the user for it. */ + s = read_key (device); + if (!s) + error (EXIT_FAILURE, 0, _("could not read key from user")); + r[0] = s; + } + + return r; } struct key_store * diff --git a/options/options.h b/options/options.h index 6fadf1e..510e8a8 100644 --- a/options/options.h +++ b/options/options.h @@ -104,7 +104,9 @@ struct mp { /* A key in the key store. */ struct key_store_key { - /* The device this key refers to. */ + /* The device this key refers to. There may be multiple matching + * devices in the list. + */ char *device; enum { @@ -146,7 +148,7 @@ extern void print_inspect_prompt (void); /* in key.c */ extern char *read_key (const char *param); -extern char *get_key (struct key_store *ks, const char *device); +extern char **get_keys (struct key_store *ks, const char *device); extern struct key_store *key_store_add_from_selector (struct key_store *ks, const char *selector); extern struct key_store *key_store_import_key (struct key_store *ks, const struct key_store_key *key); extern void free_key_store (struct key_store *ks); -- 2.23.0
On Tuesday, 26 November 2019 17:40:47 CET Richard W.M. Jones wrote:
This allows multiple --key parameters on the command line to match a single device. For example:
tool --key /dev/sda1:key:trykey1 --key /dev/sda1:key:trykey2
would try "trykey1" and "trykey2" against /dev/sda1. ---
Mostly LGTM, just one note/question below.
+ CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i]); + + if (guestfs_int_count_strings (keys) == 0) + error (EXIT_FAILURE, 0, + _("no key was provided to open LUKS encrypted %s, " + "try using --key on the command line"), + partitions[i]);
Is this check ever going to be true? get_keys() calls read_key() to ask the user for a key if none were provided, and on read_key() failure error() is called directly. In case this might be dead code, I'd place an assert instead or a non-translatable internal error. -- Pino Toscano
On Wed, Nov 27, 2019 at 06:45:37PM +0100, Pino Toscano wrote:
On Tuesday, 26 November 2019 17:40:47 CET Richard W.M. Jones wrote:
This allows multiple --key parameters on the command line to match a single device. For example:
tool --key /dev/sda1:key:trykey1 --key /dev/sda1:key:trykey2
would try "trykey1" and "trykey2" against /dev/sda1. ---
Mostly LGTM, just one note/question below.
+ CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i]); + + if (guestfs_int_count_strings (keys) == 0) + error (EXIT_FAILURE, 0, + _("no key was provided to open LUKS encrypted %s, " + "try using --key on the command line"), + partitions[i]);
Is this check ever going to be true? get_keys() calls read_key() to ask the user for a key if none were provided, and on read_key() failure error() is called directly. In case this might be dead code, I'd place an assert instead or a non-translatable internal error.
It looks like this shouldn't ever be true because of: s = read_key (device); if (!s) error (EXIT_FAILURE, 0, _("could not read key from user")); in get_keys, so I'll replace this with an internal error of some sort. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
You can specify default keys which are tried against each device (after more specific keys fail), eg: tool --key :key:defaultkey1 --key :key:defaultkey2 which would try "defaultkey1" and "defaultkey2" against all devices in the guest. --- options/keys.c | 3 ++- options/options.h | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/options/keys.c b/options/keys.c index 782bdb6..817508b 100644 --- a/options/keys.c +++ b/options/keys.c @@ -148,7 +148,8 @@ get_keys (struct key_store *ks, const char *device) for (i = 0; i < ks->nr_keys; ++i) { struct key_store_key *key = &ks->keys[i]; - if (STRNEQ (key->device, device)) + if (STRNEQ (key->device, "") && + STRNEQ (key->device, device)) continue; switch (key->type) { diff --git a/options/options.h b/options/options.h index 510e8a8..2f6bc5c 100644 --- a/options/options.h +++ b/options/options.h @@ -105,7 +105,10 @@ struct mp { /* A key in the key store. */ struct key_store_key { /* The device this key refers to. There may be multiple matching - * devices in the list. + * devices in the list. It is never NULL but it may also be "" + * which is interpreted as a default key which is tried after any + * device-specific keys (there may be multiple defaults in the + * list). */ char *device; -- 2.23.0
Hi Rich and Pino, Commenting after a test. I've installed a RHEL 7 virtual machine with 2 disks, using the graphical installer. During the installation, I selected the 2 disks as well as encryption checkbox. It asked me for only one password. After the installation, when the machine boots, it asks for the password (showing a device UUID) only once. When connected as root, I can see that there are indeed 2 encrypted partitions: /dev/sda2 and /dev/sdb1, which are used as LVM PVs. They both use the same encryption key, but the initramfs only prompts once, which is the behavior proposed by Rich. So, I pushed the test a little more and added 2 disks to the virtual machine and manually configured LUKS (luksFormat, etc...), with the same passphrase, but different from the one provided during the installation. I added the disks to /etc/crypttab and at boot I'm asked to provide 3 passphrases: 1 for the initial devices and 1 per additional disk. This is similar to Pino's fully deterministic approach. I then realized that I had encrypted the whole device, while the installation had created partitions. So, I added 2 other disks and partitioned them and encrypted them with the same passphrase, but a 3rd one. This time, I'm asked for 5 passphrases, confirming that it doesn't try the passphrase against more than one device. But that doesn't explain why it asks for only one passphrase for the initial devices. The LVM VG is configured with 2 PVs: /dev/sda2 and /dev/sdb1. Maybe it's considered as a single unit. I would need to dig deeper, but it's late. So, the conclusion is that in the real world, we find both cases: 1 key for multiple devices with a single prompt, and 1 identical key for multiple devices with N prompts. @Richard W.M. Jones <rjones@redhat.com>, do you think it's possible to add the ability to provide the UUID instead of /dev/sdxN ? We could document that the list of devices and UUIDs can be retrieved from lsblk and blkid. My 2 cents. On Tue, Nov 26, 2019 at 5:44 PM Richard W.M. Jones <rjones@redhat.com> wrote:
You can specify default keys which are tried against each device (after more specific keys fail), eg:
tool --key :key:defaultkey1 --key :key:defaultkey2
which would try "defaultkey1" and "defaultkey2" against all devices in the guest. --- options/keys.c | 3 ++- options/options.h | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/options/keys.c b/options/keys.c index 782bdb6..817508b 100644 --- a/options/keys.c +++ b/options/keys.c @@ -148,7 +148,8 @@ get_keys (struct key_store *ks, const char *device) for (i = 0; i < ks->nr_keys; ++i) { struct key_store_key *key = &ks->keys[i];
- if (STRNEQ (key->device, device)) + if (STRNEQ (key->device, "") && + STRNEQ (key->device, device)) continue;
switch (key->type) { diff --git a/options/options.h b/options/options.h index 510e8a8..2f6bc5c 100644 --- a/options/options.h +++ b/options/options.h @@ -105,7 +105,10 @@ struct mp { /* A key in the key store. */ struct key_store_key { /* The device this key refers to. There may be multiple matching - * devices in the list. + * devices in the list. It is never NULL but it may also be "" + * which is interpreted as a default key which is tried after any + * device-specific keys (there may be multiple defaults in the + * list). */ char *device;
-- 2.23.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Fabien Dupont, RHCA Principal Software Engineer Red Hat - Migration Engineering <https://www.redhat.com> <https://red.ht/sig> <https://redhat.com/summit>
On Tue, Nov 26, 2019 at 11:09:01PM +0100, Fabien Dupont wrote:
Hi Rich and Pino,
Commenting after a test. I've installed a RHEL 7 virtual machine with 2 disks, using the graphical installer. During the installation, I selected the 2 disks as well as encryption checkbox. It asked me for only one password. After the installation, when the machine boots, it asks for the password (showing a device UUID) only once. When connected as root, I can see that there are indeed 2 encrypted partitions: /dev/sda2 and /dev/sdb1, which are used as LVM PVs. They both use the same encryption key, but the initramfs only prompts once, which is the behavior proposed by Rich.
So, I pushed the test a little more and added 2 disks to the virtual machine and manually configured LUKS (luksFormat, etc...), with the same passphrase, but different from the one provided during the installation. I added the disks to /etc/crypttab and at boot I'm asked to provide 3 passphrases: 1 for the initial devices and 1 per additional disk. This is similar to Pino's fully deterministic approach. I then realized that I had encrypted the whole device, while the installation had created partitions. So, I added 2 other disks and partitioned them and encrypted them with the same passphrase, but a 3rd one. This time, I'm asked for 5 passphrases, confirming that it doesn't try the passphrase against more than one device.
You can change this behaviour by configuring a module called decrypt_keyctl, at least on Debian hosts: https://unix.stackexchange.com/questions/392284/using-a-single-passphrase-to...
But that doesn't explain why it asks for only one passphrase for the initial devices. The LVM VG is configured with 2 PVs: /dev/sda2 and /dev/sdb1. Maybe it's considered as a single unit. I would need to dig deeper, but it's late. So, the conclusion is that in the real world, we find both cases: 1 key for multiple devices with a single prompt, and 1 identical key for multiple devices with N prompts.
@Richard W.M. Jones <rjones@redhat.com>, do you think it's possible to add the ability to provide the UUID instead of /dev/sdxN ? We could document that the list of devices and UUIDs can be retrieved from lsblk and blkid.
Pino & I discussed this already and that's why the other thread about SSA. It requires some deeper changes, but Pino is looking into it. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (3)
-
Fabien Dupont -
Pino Toscano -
Richard W.M. Jones