Libguestfs - Nov 2019 - [PATCH common v2 1/3] options: Simplify selector parsing for --key options.

v1:
https://www.redhat.com/archives/libguestfs/2019-November/msg00036.html

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

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 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

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