Libguestfs - Jun 2022 - [libguestfs-common PATCH v2 00/12] LUKS decryption with Clevis+Tang

v1:       https://listman.redhat.com/archives/libguestfs/2022-June/029277.html
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453

Please see the Notes section on each patch for the updates in this
version. (The CVE fix has been merged: 35467027f657.)

Thanks,
Laszlo

Laszlo Ersek (12):
  options: fix UUID comparison logic bug in get_keys()
  mltools/tools_utils: remove unused function "key_store_to_cli"
  mltools/tools_utils: allow multiple "--key" options for OCaml tools
    too
  options: replace NULL-termination with number-of-elements in
    get_keys()
  options: wrap each passphrase from get_keys() into a struct
  options: add back-end for LUKS decryption with Clevis+Tang
  options: introduce selector type "key_clevis"
  options: generalize "--key" selector parsing for C-language
utilities
  mltools/tools_utils-c: handle internal type error with abort()
  mltools/tools_utils: generalize "--key" selector parsing for OCaml
    utils
  options, mltools/tools_utils: parse "--key ID:clevis" options
  options, mltools/tools_utils: add helper for network dependency

 mltools/tools_utils-c.c |  43 ++++---
 mltools/tools_utils.ml  |  51 ++++----
 mltools/tools_utils.mli |  12 +-
 options/decrypt.c       |  30 +++--
 options/key-option.pod  |   9 ++
 options/keys.c          | 124 ++++++++++++++------
 options/options.h       |  19 ++-
 7 files changed, 191 insertions(+), 97 deletions(-)

-- 
2.19.1.3.g30247aa5d201

When we iterate over the keys in the keystore, we intend to skip a key
(i.e., we do not append it to the result array) if:

  (key_id differs from device name) && (key_id differs from device UUID)

The problem is how we currently express the second condition:

  uuid && STRNEQ (key->id, uuid)

This is wrong: when the LUKS UUID is missing (e.g. because
guestfs_luks_uuid() failed or we're looking up the keys for a BitLocker
device), this expression evaluates to 0. Which states that the key_id does
*not* differ from the device UUID -- in other words, that we have a match.

Invert the result for when "uuid" is NULL:

  !uuid || STRNEQ (key->id, uuid)

(

- Equivalently: what we currently have for "differs" is:

    uuid ? STRNEQ (key->id, uuid) : false

  but we need:

    uuid ? STRNEQ (key->id, uuid) : true

- Yet another way to express

    key_id differs from device UUID

  correctly is with the "implication operator":

    uuid ? STRNEQ (key->id, uuid)

  The crucial feature is that, from a false premise, everything follows;
  therefore, if the UUID is missing, we get "true" (which we want for
  "differs"). In C, we don't have an "implication
operator", but

    A ? B

  is equivalent to

    ?A ? B

  (compare the truth tables!), which we *can* express in C, as

    !A || B

)

In practice the bug has been masked for the following reasons:

- It's pretty rare that "uuid" is NULL.

- Even when "uuid" is NULL, and therefore we incorrectly include a key
in
  the result list, the consequence is only bad performance. Namely, we
  attempt unlocking a LUKS or BitLocker volume with a key that the user
  never intended for that -- so the attempt will fail, and
  decrypt_mountables() [options/decrypt.c] will just advance to the next
  key in the result list. Put differently, we cannot "mistakenly"
unlock
  an encrypted device.

However, because unlocking disk encryption requires plenty of computation
by design (in order to resist brute forcing), this waste may not be
trivial.

Fixes: bb4a2dc17a78b53437896d4215ae82df8e11b788
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 options/keys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/options/keys.c b/options/keys.c
index d27a7123e67e..8713372a305e 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -152,11 +152,11 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid)
 
   if (ks) {
     for (i = 0; i < ks->nr_keys; ++i) {
       struct key_store_key *key = &ks->keys[i];
 
-      if (STRNEQ (key->id, device) && (uuid && STRNEQ
(key->id, uuid)))
+      if (STRNEQ (key->id, device) && (!uuid || STRNEQ (key->id,
uuid)))
         continue;
 
       switch (key->type) {
       case key_string:
         s = strdup (key->string.s);
-- 
2.19.1.3.g30247aa5d201

Function "key_store_to_cli" was introduced in commit 310ffe7e507e
("mltools: Add key_store_to_cli", 2021-09-21), and put to use in
virt-v2v
commit cff4514927b3 ("v2v: Pass --key parameters through to the input
helper", 2021-09-21).

However, the function has not been in use since virt-v2v commit
724ecb5e887e ("input: Turn helper into an OCaml module", 2021-12-02);
remove it now.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 mltools/tools_utils.mli |  8 --------
 mltools/tools_utils.ml  | 10 ----------
 2 files changed, 18 deletions(-)

diff --git a/mltools/tools_utils.mli b/mltools/tools_utils.mli
index 501830065038..8d9af7a0a661 100644
--- a/mltools/tools_utils.mli
+++ b/mltools/tools_utils.mli
@@ -101,18 +101,10 @@ val create_standard_options : Getopt.speclist ->
?anon_fun:Getopt.anon_fun -> ?k
     which allows another tool to run this tool and change the program
     name used in error messages.
 
     Returns a new {!cmdline_options} structure. *)
 
-val key_store_to_cli : key_store -> string list
-(** Convert a {!key_store} object back to a list of command line
-    options, essentially undoing the effect of Getopt parsing.
-    This is used in virt-v2v to pass the keystore to helpers.
-    It is not particularly secure, especially if you use the
-    [:key:] selector, although not any less secure than passing
-    them via the command line in the first place. *)
-
 val external_command : ?echo_cmd:bool -> string -> string list
 (** Run an external command, slurp up the output as a list of lines.
 
     [echo_cmd] specifies whether to output the full command on verbose
     mode, and it's on by default. *)
diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index 695fda7e548c..8508534e16ee 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -418,20 +418,10 @@ let create_standard_options argspec ?anon_fun ?(key_opts =
false)
   let argspec = !argspec in
 
   let getopt = Getopt.create argspec ?anon_fun usage_msg in
   { getopt; ks; debug_gc }
 
-let key_store_to_cli { keys } -  Hashtbl.fold (
-    fun k v acc ->
-      let arg -        match v with
-        | KeyString s -> sprintf "%s:key:%s" k s
-        | KeyFileName f -> sprintf "%s:file:%s" k f in
-      "--key" :: arg :: acc
-  ) keys []
-
 (* Run an external command, slurp up the output as a list of lines. *)
 let external_command ?(echo_cmd = true) cmd    if echo_cmd then
     debug "%s" cmd;
   let chan = Unix.open_process_in cmd in
-- 
2.19.1.3.g30247aa5d201

Commit c10c8baedb88 ("options: Allow multiple --key parameters.",
2019-11-28) enabled multi-key support for the C-language tools only.
C-language tools parse the "--key" option as follows:

  OPTION_key                      [options/options.h]
    key_store_add_from_selector() [options/keys.c]
      key_store_import_key()      [options/keys.c]

And the last function above simply appends the imported key to the
keystore.

However, commit c10c8baedb88 was ineffective (already at the time of
writing) for OCaml-language tools. From commit f84d95474ccf ("Introduce a
--key option in tools that accept keys", 2018-09-21), OCaml tools first
de-duplicate the "--key" options (using the device identifier as a
hash
table key), and only (distinct) elements of the flattened hash table are
passed to key_store_import_key():

  parse_key_selector                    [mltools/tools_utils.ml]
    Hashtbl.replace

  inspect_decrypt                       [mltools/tools_utils.ml]
    Hashtbl.fold
    c_inspect_decrypt                   [mltools/tools_utils.ml]
      guestfs_int_mllib_inspect_decrypt [mltools/tools_utils-c.c]
        key_store_import_key()          [options/keys.c]

This problem can be demonstrated by passing two keys, a good one and a
wrong one, for the same device identifier, to a C-language guestfs tool
(such as virt-cat), and to an OCaml-language one (such as
virt-get-kernel). The latter is sensitive to the order of "--key"
options:

$ virt-cat \
    --key /dev/sda2:key:good-key \
    --key /dev/sda2:key:wrong-key \
    -d DOMAIN \
    /no-such-file
> libguestfs: error: download: /no-such-file: No such file or directory
Here the wrong key (passed as the 2nd "--key" option) does not
invalidate
the first (good) key.

$ virt-get-kernel \
    --key /dev/sda2:key:good-key \
    --key /dev/sda2:key:wrong-key \
    -d DOMAIN \
    -o /tmp
> virt-get-kernel: could not find key to open LUKS encrypted /dev/sda2.
>
> Try using --key on the command line.
>
> Original error: cryptsetup_open: cryptsetup exited with status 2: No key
> available with this passphrase. (0)
Here the wrong key replaces the good key.

$ virt-get-kernel \
    --key /dev/sda2:key:wrong-key \
    --key /dev/sda2:key:good-key \
    -d DOMAIN \
    -o /tmp
> download: /boot/vmlinuz-[...] -> /tmp/vmlinuz-[...]
> download: /boot/initramfs-[...].img -> /tmp/initramfs-[...].img
Reversing the order of "--key" options for the OCaml-language tool
allows
the good key to prevail (and the wrong to be overwritten).

Fix this discrepancy by replacing the hash table with a plain list
(reference).

(Side comment: the hash table-based deduplication didn't do its job
entirely, either. We could still pass two keys for the same LUKS block
device: once by pathname, and another time by LUKS UUID.)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 mltools/tools_utils.ml | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index 8508534e16ee..6006ab7e4f6c 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -27,11 +27,11 @@ open Getopt.OptionName
  * messages.
  *)
 let prog = ref prog
 
 type key_store = {
-  keys : (string, key_store_key) Hashtbl.t;
+  keys : (string * key_store_key) list ref;
 }
 and key_store_key    | KeyString of string
   | KeyFileName of string
 
@@ -374,11 +374,11 @@ let create_standard_options argspec ?anon_fun ?(key_opts =
false)
       | n ->
         error (f_"invalid output for --machine-readable: %s") fmt
       )
   in
   let ks = {
-    keys = Hashtbl.create 13;
+    keys = ref [];
   } in
   let argspec = ref argspec in
   let add_argspec = List.push_back argspec in
 
   add_argspec ([ S 'V'; L"version" ], Getopt.Unit
print_version_and_exit, s_"Display version and exit");
@@ -393,13 +393,13 @@ let create_standard_options argspec ?anon_fun ?(key_opts =
false)
   if key_opts then (
     let parse_key_selector arg        let parts = String.nsplit ~max:3
":" arg in
       match parts with
       | [ device; "key"; key ] ->
-         Hashtbl.replace ks.keys device (KeyString key)
+         List.push_back ks.keys (device, KeyString key)
       | [ device; "file"; file ] ->
-         Hashtbl.replace ks.keys device (KeyFileName file)
+         List.push_back ks.keys (device, KeyFileName file)
       | _ ->
          error (f_"invalid selector string for --key: %s") arg
     in
 
     add_argspec ([ L"echo-keys" ],       Getopt.Unit c_set_echo_keys,
s_"Don?t turn off echo for passphrases");
@@ -680,24 +680,17 @@ let is_btrfs_subvolume g fs    with Guestfs.Error msg as
exn ->
     if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
     else raise exn
 
 let inspect_decrypt g ks -  (* Turn the keys in the key_store into a simpler
struct, so it is possible
-   * to read it using the C API.
-   *)
-  let keys_as_list = Hashtbl.fold (
-    fun k v acc ->
-      (k, v) :: acc
-  ) ks.keys [] in
   (* Note we pass original 'g' even though it is not used by the
    * callee.  This is so that 'g' is kept as a root on the stack, and
    * so cannot be garbage collected while we are in the c_inspect_decrypt
    * function.
    *)
   c_inspect_decrypt g#ocaml_handle (Guestfs.c_pointer g#ocaml_handle)
-    keys_as_list
+    !(ks.keys)
 
 let with_timeout op timeout ?(sleep = 2) fn    let start_t = Unix.gettimeofday
() in
   let rec loop ()      if Unix.gettimeofday () -. start_t > float_of_int
timeout then
-- 
2.19.1.3.g30247aa5d201

Currently, get_keys() returns a NULL-terminated array of (char*) elements.
In a later patch in this series, we'll want to change the element type to
a structure type, at which point the NULL-termination would become
awkward. Replace the NULL-termination scheme with the number-of-elements
scheme. No behavioral changes.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 options/options.h |  4 +++-
 options/decrypt.c | 17 +++++++-------
 options/keys.c    | 24 ++++++++++++++------
 3 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/options/options.h b/options/options.h
index 80df91a85f98..d7a3aeff6f41 100644
--- a/options/options.h
+++ b/options/options.h
@@ -149,11 +149,13 @@ extern void inspect_mount_root (guestfs_h *g, const char
*root);
 #define inspect_mount() inspect_mount_handle (g, ks)
 extern void print_inspect_prompt (void);
 
 /* in key.c */
 extern char *read_key (const char *param);
-extern char **get_keys (struct key_store *ks, const char *device, const char
*uuid);
+extern char **get_keys (struct key_store *ks, const char *device, const char
*uuid,
+                        size_t *nr_matches);
+extern void free_keys (char **keys, size_t nr_matches);
 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);
 
 /* in options.c */
diff --git a/options/decrypt.c b/options/decrypt.c
index 1cd7b627e264..c25b5888d8b4 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -122,14 +122,14 @@ decrypt_mountables (guestfs_h *g, const char * const
*mountables,
   const char *mountable;
 
   while ((mountable = *mnt_scan++) != NULL) {
     CLEANUP_FREE char *type = NULL;
     CLEANUP_FREE char *uuid = NULL;
-    CLEANUP_FREE_STRING_LIST char **keys = NULL;
+    char **keys;
+    size_t nr_matches;
     CLEANUP_FREE char *mapname = NULL;
-    const char * const *key_scan;
-    const char *key;
+    size_t scan;
 
     type = guestfs_vfs_type (g, mountable);
     if (type == NULL)
       continue;
 
@@ -142,37 +142,38 @@ decrypt_mountables (guestfs_h *g, const char * const
*mountables,
       continue;
 
     /* Grab the keys that we should try with this device, based on device name,
      * or UUID (if any).
      */
-    keys = get_keys (ks, mountable, uuid);
-    assert (keys[0] != NULL);
+    keys = get_keys (ks, mountable, uuid, &nr_matches);
+    assert (nr_matches > 0);
 
     /* Generate a node name for the plaintext (decrypted) device node. */
     if (uuid == NULL || asprintf (&mapname, "luks-%s", uuid) ==
-1)
       mapname = make_mapname (mountable);
 
     /* Try each key in turn. */
-    key_scan = (const char * const *)keys;
-    while ((key = *key_scan++) != NULL) {
+    for (scan = 0; scan < nr_matches; ++scan) {
+      const char *key = keys[scan];
       int r;
 
       guestfs_push_error_handler (g, NULL, NULL);
       r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1);
       guestfs_pop_error_handler (g);
 
       if (r == 0)
         break;
     }
 
-    if (key == NULL)
+    if (scan == nr_matches)
       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)"),
              mountable, guestfs_last_error (g), guestfs_last_errno (g));
 
+    free_keys (keys, nr_matches);
     decrypted_some = true;
   }
 
   return decrypted_some;
 }
diff --git a/options/keys.c b/options/keys.c
index 8713372a305e..1d97d980a460 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -124,11 +124,12 @@ read_first_line_from_file (const char *filename)
 /* 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, const char *uuid)
+get_keys (struct key_store *ks, const char *device, const char *uuid,
+          size_t *nr_matches)
 {
   size_t i, j, nmemb;
   char **r;
   char *s;
 
@@ -137,18 +138,16 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid)
    */
   nmemb = 1;
   if (ks && ks->nr_keys > nmemb)
     nmemb = ks->nr_keys;
 
-  /* make room for the terminating NULL */
-  if (nmemb == (size_t)-1)
+  if (nmemb > (size_t)-1 / sizeof *r)
     error (EXIT_FAILURE, 0, _("size_t overflow"));
-  nmemb++;
 
-  r = calloc (nmemb, sizeof (char *));
+  r = malloc (nmemb * sizeof *r);
   if (r == NULL)
-    error (EXIT_FAILURE, errno, "calloc");
+    error (EXIT_FAILURE, errno, "malloc");
 
   j = 0;
 
   if (ks) {
     for (i = 0; i < ks->nr_keys; ++i) {
@@ -175,16 +174,27 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid)
   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;
+    r[j++] = s;
   }
 
+  *nr_matches = j;
   return r;
 }
 
+void
+free_keys (char **keys, size_t nr_matches)
+{
+  size_t i;
+
+  for (i = 0; i < nr_matches; ++i)
+    free (keys[i]);
+  free (keys);
+}
+
 struct key_store *
 key_store_add_from_selector (struct key_store *ks, const char *selector)
 {
   CLEANUP_FREE_STRING_LIST char **fields      guestfs_int_split_string
(':', selector);
-- 
2.19.1.3.g30247aa5d201

For adding Clevis support later in this patch set, replace the (char *)
element type of the get_keys() result array with a new structure: "struct
matching_key". No behavioral changes.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 options/options.h | 13 +++++++--
 options/decrypt.c |  6 ++--
 options/keys.c    | 30 ++++++++++++--------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/options/options.h b/options/options.h
index d7a3aeff6f41..9bd812525d8a 100644
--- a/options/options.h
+++ b/options/options.h
@@ -132,10 +132,17 @@ struct key_store_key {
 struct key_store {
   struct key_store_key *keys;
   size_t nr_keys;
 };
 
+/* A key matching a particular ID (pathname of the libguestfs device node that
+ * stands for the encrypted block device, or LUKS UUID).
+ */
+struct matching_key {
+  char *passphrase;
+};
+
 /* in config.c */
 extern void parse_config (void);
 
 /* in decrypt.c */
 extern void inspect_do_decrypt (guestfs_h *g, struct key_store *ks);
@@ -149,13 +156,13 @@ extern void inspect_mount_root (guestfs_h *g, const char
*root);
 #define inspect_mount() inspect_mount_handle (g, ks)
 extern void print_inspect_prompt (void);
 
 /* in key.c */
 extern char *read_key (const char *param);
-extern char **get_keys (struct key_store *ks, const char *device, const char
*uuid,
-                        size_t *nr_matches);
-extern void free_keys (char **keys, size_t nr_matches);
+extern struct matching_key *get_keys (struct key_store *ks, const char *device,
+                                      const char *uuid, size_t *nr_matches);
+extern void free_keys (struct matching_key *keys, size_t nr_matches);
 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);
 
 /* in options.c */
diff --git a/options/decrypt.c b/options/decrypt.c
index c25b5888d8b4..421a38c2a11f 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -122,11 +122,11 @@ decrypt_mountables (guestfs_h *g, const char * const
*mountables,
   const char *mountable;
 
   while ((mountable = *mnt_scan++) != NULL) {
     CLEANUP_FREE char *type = NULL;
     CLEANUP_FREE char *uuid = NULL;
-    char **keys;
+    struct matching_key *keys;
     size_t nr_matches;
     CLEANUP_FREE char *mapname = NULL;
     size_t scan;
 
     type = guestfs_vfs_type (g, mountable);
@@ -151,15 +151,15 @@ decrypt_mountables (guestfs_h *g, const char * const
*mountables,
     if (uuid == NULL || asprintf (&mapname, "luks-%s", uuid) ==
-1)
       mapname = make_mapname (mountable);
 
     /* Try each key in turn. */
     for (scan = 0; scan < nr_matches; ++scan) {
-      const char *key = keys[scan];
+      struct matching_key *key = keys + scan;
       int r;
 
       guestfs_push_error_handler (g, NULL, NULL);
-      r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1);
+      r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname,
-1);
       guestfs_pop_error_handler (g);
 
       if (r == 0)
         break;
     }
diff --git a/options/keys.c b/options/keys.c
index 1d97d980a460..56fca17a94b5 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -123,16 +123,16 @@ read_first_line_from_file (const char *filename)
 
 /* 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 **
+struct matching_key *
 get_keys (struct key_store *ks, const char *device, const char *uuid,
           size_t *nr_matches)
 {
-  size_t i, j, nmemb;
-  char **r;
+  size_t i, nmemb;
+  struct matching_key *r, *match;
   char *s;
 
   /* We know the returned list must have at least one element and not
    * more than ks->nr_keys.
    */
@@ -145,11 +145,11 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid,
 
   r = malloc (nmemb * sizeof *r);
   if (r == NULL)
     error (EXIT_FAILURE, errno, "malloc");
 
-  j = 0;
+  match = r;
 
   if (ks) {
     for (i = 0; i < ks->nr_keys; ++i) {
       struct key_store_key *key = &ks->keys[i];
 
@@ -159,39 +159,45 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid,
       switch (key->type) {
       case key_string:
         s = strdup (key->string.s);
         if (!s)
           error (EXIT_FAILURE, errno, "strdup");
-        r[j++] = s;
+        match->passphrase = s;
+        ++match;
         break;
       case key_file:
         s = read_first_line_from_file (key->file.name);
-        r[j++] = s;
+        match->passphrase = s;
+        ++match;
         break;
       }
     }
   }
 
-  if (j == 0) {
+  if (match == r) {
     /* 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[j++] = s;
+    match->passphrase = s;
+    ++match;
   }
 
-  *nr_matches = j;
+  *nr_matches = (size_t)(match - r);
   return r;
 }
 
 void
-free_keys (char **keys, size_t nr_matches)
+free_keys (struct matching_key *keys, size_t nr_matches)
 {
   size_t i;
 
-  for (i = 0; i < nr_matches; ++i)
-    free (keys[i]);
+  for (i = 0; i < nr_matches; ++i) {
+    struct matching_key *key = keys + i;
+
+    free (key->passphrase);
+  }
   free (keys);
 }
 
 struct key_store *
 key_store_add_from_selector (struct key_store *ks, const char *selector)
-- 
2.19.1.3.g30247aa5d201

Extend the "matching_key" structure with a new boolean field,
"clevis".
"clevis" is mutually exclusive with a non-NULL "passphrase"
field. If
"clevis" is set, open the LUKS device with
guestfs_clevis_luks_unlock() --
which requires no explicit passphrase --; otherwise, continue calling
guestfs_cryptsetup_open().

This patch introduces no change in observable behavior; there is no user
interface yet for setting the "clevis" field to "true".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v2:
    
    - check GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK; error out if unavailable [Rich]
    
    v1 notes were:
    
    This patch introduces a call to guestfs_clevis_luks_unlock(), which is a
    new libguestfs API (introduced in the sibling libguestfs series).
    
    Assuming we still care about building guestfs-tools and virt-v2v against
    an independent libguestfs (library and appliance), we need to do one of
    the following things:
    
    (1) Make the call dependent on the GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK
    feature test macro. If it is missing, the library is too old, just set
    "r = -1", and (possibly) print a warning to stderr.
    
    (2) Cut a new libguestfs release, and bump the minimum version in
    "m4/guestfs-libraries.m4" (in both guestfs-tools and virt-v2v) to
the
    new version.
    
    Both of these have drawbacks.
    
    Disadvantages of (1):
    
    - Printing a raw warning to stderr is OK for the C-language tools, but
      the code is used by OCaml tools as well, which have pretty-printed
      warnings.
    
    - Simply skipping the guestfs_clevis_luks_unlock() call -- i.e.,
      pretending it fails -- is not user-friendly. In particular, once we
      add the new selector type for "--key" later in this series, and
      document it in "options/key-option.pod", all the tools' docs
will pick
      it up at once, even if the library is too old to provide the
      interface.
    
    Disadvantages of (2):
    
    - We may not want to leap over a large version distance in
      "m4/guestfs-libraries.m4" (in both guestfs-tools and virt-v2v),
for
      whatever reason.
    
    - Even if we make the guestfs_clevis_luks_unlock() API mandatory in the
      library, the daemon may not implement it -- it's ultimately appliance
      (host distro) dependent, which is why the API belongs to a new option
      group called "clevisluks". That is, the actual behavior of
      guestfs_clevis_luks_unlock() may still differ from what the user
      expects based on "options/key-option.pod".
    
      This drawback is mitigated however: the documentation of the new
      selector in "options/key-option.pod" references "ENCRYPTED
DISKS" in
      guestfs(3), which in turn references
"guestfs_clevis_luks_unlock",
      which does highlight the "clevisluks" option group.
    
    I vote (2) -- cut a new libguestfs release, then bump the
    "m4/guestfs-libraries.m4" requirements. I'm not doing that
just yet in
    those projects (in the sibling series here): if I did that, those series
    would not compile; the libguestfs version number would have to be
    increased first! And I don't know if we want *something else* in the new
    libguestfs release, additionally.

 options/options.h |  6 ++++++
 options/decrypt.c | 13 ++++++++++++-
 options/keys.c    |  7 ++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/options/options.h b/options/options.h
index 9bd812525d8a..61a385da13ae 100644
--- a/options/options.h
+++ b/options/options.h
@@ -136,10 +136,16 @@ struct key_store {
 
 /* A key matching a particular ID (pathname of the libguestfs device node that
  * stands for the encrypted block device, or LUKS UUID).
  */
 struct matching_key {
+  /* True iff the passphrase should be reconstructed using Clevis, talking to
+   * Tang servers over the network.
+   */
+  bool clevis;
+
+  /* Explicit passphrase, otherwise. */
   char *passphrase;
 };
 
 /* in config.c */
 extern void parse_config (void);
diff --git a/options/decrypt.c b/options/decrypt.c
index 421a38c2a11f..97c8b88d16aa 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -155,11 +155,22 @@ decrypt_mountables (guestfs_h *g, const char * const
*mountables,
     for (scan = 0; scan < nr_matches; ++scan) {
       struct matching_key *key = keys + scan;
       int r;
 
       guestfs_push_error_handler (g, NULL, NULL);
-      r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname,
-1);
+      assert (key->clevis == (key->passphrase == NULL));
+      if (key->clevis)
+#ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK
+        r = guestfs_clevis_luks_unlock (g, mountable, mapname);
+#else
+        error (EXIT_FAILURE, 0,
+               _("'clevis_luks_unlock', needed for decrypting %s,
is "
+                 "unavailable in this libguestfs version"),
mountable);
+#endif
+      else
+        r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname,
+                                     -1);
       guestfs_pop_error_handler (g);
 
       if (r == 0)
         break;
     }
diff --git a/options/keys.c b/options/keys.c
index 56fca17a94b5..75c659561c52 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -159,15 +159,17 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid,
       switch (key->type) {
       case key_string:
         s = strdup (key->string.s);
         if (!s)
           error (EXIT_FAILURE, errno, "strdup");
+        match->clevis = false;
         match->passphrase = s;
         ++match;
         break;
       case key_file:
         s = read_first_line_from_file (key->file.name);
+        match->clevis = false;
         match->passphrase = s;
         ++match;
         break;
       }
     }
@@ -176,10 +178,11 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid,
   if (match == r) {
     /* 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"));
+    match->clevis = false;
     match->passphrase = s;
     ++match;
   }
 
   *nr_matches = (size_t)(match - r);
@@ -192,11 +195,13 @@ free_keys (struct matching_key *keys, size_t nr_matches)
   size_t i;
 
   for (i = 0; i < nr_matches; ++i) {
     struct matching_key *key = keys + i;
 
-    free (key->passphrase);
+    assert (key->clevis == (key->passphrase == NULL));
+    if (!key->clevis)
+      free (key->passphrase);
   }
   free (keys);
 }
 
 struct key_store *
-- 
2.19.1.3.g30247aa5d201

From an earlier patch in this series, we can now represent LUKS decryption
with Clevis+Tang in those matching keys that we distill for a particular
guestfs device or LUKS UUID. Now extend the keystore (composed from
command line options), i.e. the store that is filtered into matching keys,
with a selector type that stands for Clevis+Tang.

Again, this patch introduces no change in observable behavior; there is
still no user interface for placing a selector of the new type into the
keystore.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    
    - fix the "tpe" typo in the patch subject
    - pick up Rich's R-b

 options/options.h | 1 +
 options/keys.c    | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/options/options.h b/options/options.h
index 61a385da13ae..e7a0364cc926 100644
--- a/options/options.h
+++ b/options/options.h
@@ -113,10 +113,11 @@ struct key_store_key {
   char *id;
 
   enum {
     key_string,             /* key specified as string */
     key_file,               /* key stored in a file */
+    key_clevis,             /* key reconstructed with Clevis+Tang */
   } type;
   union {
     struct {
       char *s;              /* string of the key */
     } string;
diff --git a/options/keys.c b/options/keys.c
index 75c659561c52..7729fe79c99b 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -169,10 +169,15 @@ get_keys (struct key_store *ks, const char *device, const
char *uuid,
         s = read_first_line_from_file (key->file.name);
         match->clevis = false;
         match->passphrase = s;
         ++match;
         break;
+      case key_clevis:
+        match->clevis = true;
+        match->passphrase = NULL;
+        ++match;
+        break;
       }
     }
   }
 
   if (match == r) {
@@ -289,9 +294,12 @@ free_key_store (struct key_store *ks)
       free (key->string.s);
       break;
     case key_file:
       free (key->file.name);
       break;
+    case key_clevis:
+      /* nothing */
+      break;
     }
     free (key->id);
   }
 }
-- 
2.19.1.3.g30247aa5d201

The key_store_add_from_selector() function parses the argument of the
"--key" option in C-language tools:

  OPTION_key                      [options/options.h]
    key_store_add_from_selector() [options/keys.c]
      key_store_import_key()      [options/keys.c]

The function currently (a) uses a horrible "goto", (b) is not flexible
enough for selector types that do not take precisely 1 type-specific
parameter. Rewrite the function with more informative error messages and
an easier to follow structure, plus make the type-specific parameter count
a function of the type.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    
    - NULL-init the "fields" variable in
key_store_add_from_selector(), for
      more obvious CLEANUP_FREE_STRING_LIST hygiene [Rich]
    
    - pick up Rich's R-b

 options/keys.c | 46 ++++++++++----------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/options/keys.c b/options/keys.c
index 7729fe79c99b..37e1648fc969 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -210,48 +210,50 @@ free_keys (struct matching_key *keys, size_t nr_matches)
 }
 
 struct key_store *
 key_store_add_from_selector (struct key_store *ks, const char *selector)
 {
-  CLEANUP_FREE_STRING_LIST char **fields -    guestfs_int_split_string
(':', selector);
+  CLEANUP_FREE_STRING_LIST char **fields = NULL;
+  size_t field_count;
   struct key_store_key key;
 
+  fields = guestfs_int_split_string (':', selector);
   if (!fields)
     error (EXIT_FAILURE, errno, "guestfs_int_split_string");
+  field_count = guestfs_int_count_strings (fields);
 
-  if (guestfs_int_count_strings (fields) != 3) {
-   invalid_selector:
-    error (EXIT_FAILURE, 0, "invalid selector for --key: %s",
selector);
-  }
-
-  /* 1: device */
+  /* field#0: ID */
+  if (field_count < 1)
+    error (EXIT_FAILURE, 0, _("selector '%s': missing ID"),
selector);
   key.id = strdup (fields[0]);
   if (!key.id)
     error (EXIT_FAILURE, errno, "strdup");
 
-  /* 2: key type */
-  if (STREQ (fields[1], "key"))
+  /* field#1...: TYPE, and TYPE-specific properties */
+  if (field_count < 2)
+    error (EXIT_FAILURE, 0, _("selector '%s': missing TYPE"),
selector);
+
+  if (STREQ (fields[1], "key")) {
     key.type = key_string;
-  else if (STREQ (fields[1], "file"))
-    key.type = key_file;
-  else
-    goto invalid_selector;
-
-  /* 3: actual key */
-  switch (key.type) {
-  case key_string:
+    if (field_count != 3)
+      error (EXIT_FAILURE, 0,
+             _("selector '%s': missing KEY_STRING, or too many
fields"),
+             selector);
     key.string.s = strdup (fields[2]);
     if (!key.string.s)
       error (EXIT_FAILURE, errno, "strdup");
-    break;
-  case key_file:
+  } else if (STREQ (fields[1], "file")) {
+    key.type = key_file;
+    if (field_count != 3)
+      error (EXIT_FAILURE, 0,
+             _("selector '%s': missing FILENAME, or too many
fields"),
+             selector);
     key.file.name = strdup (fields[2]);
     if (!key.file.name)
       error (EXIT_FAILURE, errno, "strdup");
-    break;
-  }
+  } else
+    error (EXIT_FAILURE, 0, _("selector '%s': invalid TYPE"),
selector);
 
   return key_store_import_key (ks, &key);
 }
 
 struct key_store *
-- 
2.19.1.3.g30247aa5d201

If the "default" label is taken in
guestfs_int_mllib_inspect_decrypt(),
that means the C code and the OCaml code disagree on the data constructors
of the "key_store_key" OCaml type. Such internal errors are best
reported
with abort() -- not entirely unlike a failed assertion.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Suggested-by: Richard W.M. Jones <rjones at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v2:
    
    - new patch [Rich]

 mltools/tools_utils-c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c
index 081466776666..c703998d4838 100644
--- a/mltools/tools_utils-c.c
+++ b/mltools/tools_utils-c.c
@@ -74,13 +74,11 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv,
value keysv)
       key.file.name = strdup (String_val (Field (v, 0)));
       if (!key.file.name)
         caml_raise_out_of_memory ();
       break;
     default:
-      error (EXIT_FAILURE, 0,
-             "internal error: unhandled Tag_val (v) = %d",
-             Tag_val (v));
+      abort ();
     }
 
     ks = key_store_import_key (ks, &key);
 
     keysv = Field (keysv, 1);
-- 
2.19.1.3.g30247aa5d201

In another patch in this series, we generalize the "--key" selector
parsing for C-language utilities. Adapt the OCaml-language "--key"
parser:

- Incorporate the new (more informative) error messages for consistency.

- Prepare for selector types that do not take any type-specific
  parameters. These will be represented with constant constructors of the
  "key_store_key" type, and such values are not blocks, but unboxed
  integers:
  <https://v2.ocaml.org/manual/intfc.html#ss:c-concrete-datatypes>.

(This patch is best shown with "git show -b" for review.)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v2:
    
    - fix typo in commit message (s/langauge/language/) [Rich]
    
    - handle internal type error with abort() [Rich]

 mltools/tools_utils.ml  | 14 ++++++--
 mltools/tools_utils-c.c | 38 +++++++++++---------
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index 6006ab7e4f6c..e534cbead47a 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -390,18 +390,28 @@ let create_standard_options argspec ?anon_fun ?(key_opts =
false)
                  L"colour"; L"colours" ], Getopt.Unit
set_colours, s_"Use ANSI colour sequences even if not tty");
   add_argspec ([ L"wrap" ],           Getopt.Unit set_wrap,    
s_"Wrap log messages even if not tty");
 
   if key_opts then (
     let parse_key_selector arg -      let parts = String.nsplit ~max:3
":" arg in
+      let parts = String.nsplit ":" arg in
       match parts with
+      | [] ->
+        error (f_"selector '%s': missing ID") arg
+      | [ _ ] ->
+        error (f_"selector '%s': missing TYPE") arg
+      | [ _; "key" ]
+      |  _ :: "key" :: _ :: _ :: _ ->
+        error (f_"selector '%s': missing KEY_STRING, or too many
fields") arg
       | [ device; "key"; key ] ->
          List.push_back ks.keys (device, KeyString key)
+      | [ _; "file" ]
+      |  _ :: "file" :: _ :: _ :: _ ->
+        error (f_"selector '%s': missing FILENAME, or too many
fields") arg
       | [ device; "file"; file ] ->
          List.push_back ks.keys (device, KeyFileName file)
       | _ ->
-         error (f_"invalid selector string for --key: %s") arg
+         error (f_"selector '%s': invalid TYPE") arg
     in
 
     add_argspec ([ L"echo-keys" ],       Getopt.Unit c_set_echo_keys,
s_"Don?t turn off echo for passphrases");
     add_argspec ([ L"keys-from-stdin" ], Getopt.Unit
c_set_keys_from_stdin, s_"Read passphrases from stdin");
     add_argspec ([ L"key" ], Getopt.String (s_"SELECTOR",
parse_key_selector), s_"Specify a LUKS key");
diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c
index c703998d4838..d22a5e65b0c2 100644
--- a/mltools/tools_utils-c.c
+++ b/mltools/tools_utils-c.c
@@ -60,26 +60,32 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv,
value keysv)
     key.id = strdup (String_val (Field (elemv, 0)));
     if (!key.id)
       caml_raise_out_of_memory ();
 
     v = Field (elemv, 1);
-    switch (Tag_val (v)) {
-    case 0:  /* KeyString of string */
-      key.type = key_string;
-      key.string.s = strdup (String_val (Field (v, 0)));
-      if (!key.string.s)
-        caml_raise_out_of_memory ();
-      break;
-    case 1:  /* KeyFileName of string */
-      key.type = key_file;
-      key.file.name = strdup (String_val (Field (v, 0)));
-      if (!key.file.name)
-        caml_raise_out_of_memory ();
-      break;
-    default:
-      abort ();
-    }
+    if (Is_block (v))
+      switch (Tag_val (v)) {
+      case 0:  /* KeyString of string */
+        key.type = key_string;
+        key.string.s = strdup (String_val (Field (v, 0)));
+        if (!key.string.s)
+          caml_raise_out_of_memory ();
+        break;
+      case 1:  /* KeyFileName of string */
+        key.type = key_file;
+        key.file.name = strdup (String_val (Field (v, 0)));
+        if (!key.file.name)
+          caml_raise_out_of_memory ();
+        break;
+      default:
+        abort ();
+      }
+    else
+      switch (Int_val (v)) {
+      default:
+        abort ();
+      }
 
     ks = key_store_import_key (ks, &key);
 
     keysv = Field (keysv, 1);
   }
-- 
2.19.1.3.g30247aa5d201

Provide the user interface (in both the C and the OCaml tools) for
selecting network-based, passphrase-less decryption. This is the front-end
exposing the previously added back-end.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 mltools/tools_utils.ml  | 5 +++++
 mltools/tools_utils-c.c | 3 +++
 options/keys.c          | 4 ++++
 options/key-option.pod  | 6 ++++++
 4 files changed, 18 insertions(+)

diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index e534cbead47a..1da5850340d4 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -32,10 +32,11 @@ type key_store = {
   keys : (string * key_store_key) list ref;
 }
 and key_store_key    | KeyString of string
   | KeyFileName of string
+  | KeyClevis
 
 external c_inspect_decrypt : Guestfs.t -> int64 -> (string *
key_store_key) list -> unit = "guestfs_int_mllib_inspect_decrypt"
 external c_set_echo_keys : unit -> unit =
"guestfs_int_mllib_set_echo_keys" [@@noalloc]
 external c_set_keys_from_stdin : unit -> unit =
"guestfs_int_mllib_set_keys_from_stdin" [@@noalloc]
 external c_rfc3339_date_time_string : unit -> string =
"guestfs_int_mllib_rfc3339_date_time_string"
@@ -406,10 +407,14 @@ let create_standard_options argspec ?anon_fun ?(key_opts =
false)
       | [ _; "file" ]
       |  _ :: "file" :: _ :: _ :: _ ->
         error (f_"selector '%s': missing FILENAME, or too many
fields") arg
       | [ device; "file"; file ] ->
          List.push_back ks.keys (device, KeyFileName file)
+      |  _ :: "clevis" :: _ :: _ ->
+        error (f_"selector '%s': too many fields") arg
+      | [ device; "clevis" ] ->
+         List.push_back ks.keys (device, KeyClevis)
       | _ ->
          error (f_"selector '%s': invalid TYPE") arg
     in
 
     add_argspec ([ L"echo-keys" ],       Getopt.Unit c_set_echo_keys,
s_"Don?t turn off echo for passphrases");
diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c
index d22a5e65b0c2..4ff42e5d5f7f 100644
--- a/mltools/tools_utils-c.c
+++ b/mltools/tools_utils-c.c
@@ -79,10 +79,13 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv,
value keysv)
       default:
         abort ();
       }
     else
       switch (Int_val (v)) {
+      case 0:  /* KeyClevis */
+        key.type = key_clevis;
+        break;
       default:
         abort ();
       }
 
     ks = key_store_import_key (ks, &key);
diff --git a/options/keys.c b/options/keys.c
index 37e1648fc969..e7c550f4796b 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -248,10 +248,14 @@ key_store_add_from_selector (struct key_store *ks, const
char *selector)
              _("selector '%s': missing FILENAME, or too many
fields"),
              selector);
     key.file.name = strdup (fields[2]);
     if (!key.file.name)
       error (EXIT_FAILURE, errno, "strdup");
+  } else if (STREQ (fields[1], "clevis")) {
+    key.type = key_clevis;
+    if (field_count != 2)
+      error (EXIT_FAILURE, 0, _("selector '%s': too many
fields"), selector);
   } else
     error (EXIT_FAILURE, 0, _("selector '%s': invalid TYPE"),
selector);
 
   return key_store_import_key (ks, &key);
 }
diff --git a/options/key-option.pod b/options/key-option.pod
index 90a3b15c57a2..34229ce9cbb2 100644
--- a/options/key-option.pod
+++ b/options/key-option.pod
@@ -12,6 +12,12 @@ Use the specified C<KEY_STRING> as passphrase.
 
 =item B<--key> C<ID>:file:FILENAME
 
 Read the passphrase from F<FILENAME>.
 
+=item B<--key> C<ID>:clevis
+
+Attempt passphrase-less unlocking for C<ID> with Clevis, over the
+network.  Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more
+information on network-bound disk encryption (NBDE).
+
 =back
-- 
2.19.1.3.g30247aa5d201

For "--key ID:clevis" to actually work, the appliance needs network
connectivity. Some tools allow the user to control that (for example,
guestfish takes "--network"); those tools can already use "--key
ID:clevis", provided they enable inspection and networking.

However, other tools have had no use for networking thus far. Introduce a
helper function for the OCaml utils, and another for the C utils, so that
the utils can query if there's at least one clevis selector, and if so,
enable networking for the appliance, before launching the appliance.

Note that we need to implement both helpers separately from each other;
that is, the OCaml one is not based on the C one. The reason is that
"struct key_store" (from "options/options.h"), upon which we
could
logically base a common helper, exists in the OCaml tools only
ephemerally, in guestfs_int_mllib_inspect_decrypt():

  inspect_decrypt                       [mltools/tools_utils.ml]
    c_inspect_decrypt                   [mltools/tools_utils.ml]
      guestfs_int_mllib_inspect_decrypt [mltools/tools_utils-c.c]
        key_store_import_key()          [options/keys.c]
        inspect_do_decrypt()            [options/decrypt.c]
        free_key_store()                [options/keys.c]

At that time, it's too late for enabling networking for the appliance.
Therefore, in the OCaml tools, search the "earlier" data structure
called
"cmdline_options.ks". *That* "key_store" type comes from
"mltools/tools_utils.ml".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

Notes:
    v2:
    - pick up Rich's R-b

 options/options.h       |  1 +
 mltools/tools_utils.mli |  4 ++++
 mltools/tools_utils.ml  |  5 +++++
 options/keys.c          | 15 +++++++++++++++
 options/key-option.pod  |  3 +++
 5 files changed, 28 insertions(+)

diff --git a/options/options.h b/options/options.h
index e7a0364cc926..60d5d8064113 100644
--- a/options/options.h
+++ b/options/options.h
@@ -168,10 +168,11 @@ extern char *read_key (const char *param);
 extern struct matching_key *get_keys (struct key_store *ks, const char *device,
                                       const char *uuid, size_t *nr_matches);
 extern void free_keys (struct matching_key *keys, size_t nr_matches);
 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 bool key_store_requires_network (const struct key_store *ks);
 extern void free_key_store (struct key_store *ks);
 
 /* in options.c */
 extern void option_a (const char *arg, const char *format, int blocksize,
struct drv **drvsp);
 extern void option_d (const char *arg, struct drv **drvsp);
diff --git a/mltools/tools_utils.mli b/mltools/tools_utils.mli
index 8d9af7a0a661..ec900e6389bc 100644
--- a/mltools/tools_utils.mli
+++ b/mltools/tools_utils.mli
@@ -194,10 +194,14 @@ val inspect_mount_root_ro : Guestfs.guestfs -> string
-> unit
     read-only. *)
 
 val is_btrfs_subvolume : Guestfs.guestfs -> string -> bool
 (** Checks if a filesystem is a btrfs subvolume. *)
 
+val key_store_requires_network : key_store -> bool
+(** [key_store_requires_network ks] returns [true] iff [ks] contains at least
+    one "ID:clevis" selector. *)
+
 val inspect_decrypt : Guestfs.guestfs -> key_store -> unit
 (** Simple implementation of decryption: look for any encrypted
     partitions and decrypt them, then rescan for VGs. *)
 
 val with_timeout : string -> int -> ?sleep:int -> (unit -> 'a
option) -> 'a
diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index 1da5850340d4..562bfadc18a8 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -694,10 +694,15 @@ let is_btrfs_subvolume g fs      ignore
(g#mountable_subvolume fs); true
   with Guestfs.Error msg as exn ->
     if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
     else raise exn
 
+let key_store_requires_network ks +  List.exists (function
+               | _, KeyClevis -> true
+               | _ -> false) !(ks.keys)
+
 let inspect_decrypt g ks    (* Note we pass original 'g' even though it
is not used by the
    * callee.  This is so that 'g' is kept as a root on the stack, and
    * so cannot be garbage collected while we are in the c_inspect_decrypt
    * function.
diff --git a/options/keys.c b/options/keys.c
index e7c550f4796b..d987ae56198c 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -282,10 +282,25 @@ key_store_import_key (struct key_store *ks, const struct
key_store_key *key)
   ++ks->nr_keys;
 
   return ks;
 }
 
+bool
+key_store_requires_network (const struct key_store *ks)
+{
+  size_t i;
+
+  if (ks == NULL)
+    return false;
+
+  for (i = 0; i < ks->nr_keys; ++i)
+    if (ks->keys[i].type == key_clevis)
+      return true;
+
+  return false;
+}
+
 void
 free_key_store (struct key_store *ks)
 {
   size_t i;
 
diff --git a/options/key-option.pod b/options/key-option.pod
index 34229ce9cbb2..6bc04df177b1 100644
--- a/options/key-option.pod
+++ b/options/key-option.pod
@@ -18,6 +18,9 @@ Read the passphrase from F<FILENAME>.
 
 Attempt passphrase-less unlocking for C<ID> with Clevis, over the
 network.  Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more
 information on network-bound disk encryption (NBDE).
 
+Note that if any such option is present on the command line, QEMU user
+networking will be automatically enabled for the libguestfs appliance.
+
 =back
-- 
2.19.1.3.g30247aa5d201

On 06/30/22 14:20, Laszlo Ersek wrote:
> v1:      
https://listman.redhat.com/archives/libguestfs/2022-June/029277.html
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453
> 
> Please see the Notes section on each patch for the updates in this
> version. (The CVE fix has been merged: 35467027f657.)
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (12):
>   options: fix UUID comparison logic bug in get_keys()
>   mltools/tools_utils: remove unused function "key_store_to_cli"
>   mltools/tools_utils: allow multiple "--key" options for OCaml
tools
>     too
>   options: replace NULL-termination with number-of-elements in
>     get_keys()
>   options: wrap each passphrase from get_keys() into a struct
>   options: add back-end for LUKS decryption with Clevis+Tang
>   options: introduce selector type "key_clevis"
>   options: generalize "--key" selector parsing for C-language
utilities
>   mltools/tools_utils-c: handle internal type error with abort()
>   mltools/tools_utils: generalize "--key" selector parsing for
OCaml
>     utils
>   options, mltools/tools_utils: parse "--key ID:clevis" options
>   options, mltools/tools_utils: add helper for network dependency
> 
>  mltools/tools_utils-c.c |  43 ++++---
>  mltools/tools_utils.ml  |  51 ++++----
>  mltools/tools_utils.mli |  12 +-
>  options/decrypt.c       |  30 +++--
>  options/key-option.pod  |   9 ++
>  options/keys.c          | 124 ++++++++++++++------
>  options/options.h       |  19 ++-
>  7 files changed, 191 insertions(+), 97 deletions(-)
> 
Commit range 35467027f657..af6cb55bc58a.

Thanks!
Laszlo