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

[Richard W.M. Jones]

On Wed, Jun 29, 2022 at 04:14:55PM +0200, Laszlo Ersek wrote:
> On 06/29/22 15:49, Richard W.M. Jones wrote:
> > 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.
> 
> Good point. The caller of clevis_luks_unlock() is decrypt_mountables()
> [options/decrypt.c], and that function *already* has:
> 
>       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));
> 
> I can certainly add one more of that.
> 
> Can I use this error message instead:
> 
> "'clevis_luks_unlock' is unavailable in this libguestfs version"

Looks fine, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org