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

On Tue, Jun 28, 2022 at 01:49:10PM +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: > 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. ^ Yes, we need to do this one. > (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. I'm not sure I see the problem here. If we don't have the interface at compile time [or the feature at runtime - but that's extra complexity], _and_ the user uses the new option, we should fail with an error (using the normal mechanism for errors, don't print stuff on stderr). It's an error that we need to report to the user if they're expecting a clevis selector to work and it cannot.

> 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. The trouble with (2) is it pushes the complexity to distros. We now need to synchronise releases across 3 packages (and we have no choice about it), for what is a relatively minor new feature in the big picture. > options/options.h | 6 ++++++ > options/decrypt.c | 7 ++++++- > options/keys.c | 7 ++++++- > 3 files changed, 18 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..820fbc5629cd 100644 > --- a/options/decrypt.c > +++ b/options/decrypt.c > @@ -155,11 +155,16 @@ 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) > + r = guestfs_clevis_luks_unlock (g, mountable, mapname); > + 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 * The patch itself looks fine, apart from use of the new API. Rich.