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

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(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. > > 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.