[Libguestfs] [libguestfs-common PATCH 07/12] options: add back-end for LUKS decryption with Clevis+Tang
Richard W.M. Jones
rjones at redhat.com
Wed Jun 29 13:49:36 UTC 2022
On Wed, Jun 29, 2022 at 02:43:46PM +0200, Laszlo Ersek wrote:
> On 06/28/22 16:33, Richard W.M. Jones wrote:
> > 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 at redhat.com>
> >> ---
> >>
> >> 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.
>
> How about this:
>
> - In this patch, if GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK is not defined, but
> "key->clevis" is set, then abort(). The idea being, the utilities should
> never permit (or cause) "key->clevis" to be set when the API is missing.
> The utilities need to catch the unsatisfiable request for clevis
> earlier. (In this patch, it is too late for reporting that kind of error!)
>
> - At the end of this series, rename "key_store_requires_network" to
> "key_store_contains_clevis".
>
> - Append another patch: introduce a C function that checks both
> GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK and guestfs_available("clevisluks"), and
> returns a bool. Add an OCaml wrapper too. (OCaml has g#available, but
> cannot check the GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK feature test macro!)
>
> - In the tools, don't just call key_store_contains_clevis before
> launching the appliance(s), for enabling networking. After launching the
> appliance, check the new function too (if "key_store_contains_clevis"),
> and exit then if the functionality is missing.
It sounds over-complicated. Why wouldn't this work:
#ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK
clevis_luks_unlock (g, blah);
#else
error (g, f_"libguestfs was not compiled with clevis/tang functionality");
return -1;
#endif
(and nothing else). I wouldn't overthink this, we just want an
actionable error message and for downstream packagers not to have to
upgrade libguestfs + guestfs-tools + virt-v2v all at the same time.
Rich.
> Thanks,
> Laszlo
>
>
> >
> >> 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.
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
More information about the Libguestfs
mailing list