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

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