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

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

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(a)redhat.com&gt;, 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(a)redhat.com&gt; wrote: -- Fabien Dupont, RHCA Principal Software Engineer Red Hat - Migration Engineering <https://www.redhat.com> <https://red.ht/sig> <https://redhat.com/summit>