Re: [Libguestfs] [libguestfs-common PATCH v2 06/12] options: add back-end for LUKS decryption with Clevis+Tang

On Thu, Jun 30, 2022 at 02:20:22PM +0200, Laszlo Ersek wrote: > 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(a)redhat.com&gt; > --- > > 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); To be clear on why this is correct, it's because this code is only used during option parsing in command line tools, where we want to exit when an error happens.

> +#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 * Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt;