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

[Laszlo Ersek]

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"

?

Thanks!
Laszlo