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

Fri Jul 1 12:36:28 UTC 2022

On 07/01/22 14:33, Richard W.M. Jones wrote:
> On Fri, Jul 01, 2022 at 02:31:32PM +0200, Laszlo Ersek wrote:
>> On 07/01/22 12:25, Richard W.M. Jones wrote:
>>> On Fri, Jul 01, 2022 at 12:08:51PM +0200, Laszlo Ersek wrote:
>>>> On 07/01/22 11:56, Laszlo Ersek wrote:
>>>>
>>>>> This code runs (in virt-v2v, for example) on the following call path:
>>>>>
>>>>>   main                                       [v2v/v2v.ml]
>>>>>     create_standard_options                  [common/mltools/tools_utils.ml]
>>>>>     Getopt.parse
>>>>>       parse_key_selector                     [common/mltools/tools_utils.ml]
>>>>>         List.push_back
>>>>>
>>>>>     (* at this point, cmdline_options.ks
>>>>>      * has been populated
>>>>>      *)
>>>>>
>>>>>     convert                                  [convert/convert.ml]
>>>>>       (* skipped in virt-v2v, because it
>>>>>        * enables networking anyway:
>>>>>        *)
>>>>>       key_store_requires_network             [common/mltools/tools_utils.ml]
>>>>>
>>>>>       g#set_network
>>>>>       g#launch
>>>>>
>>>>>       inspect_decrypt                        [common/mltools/tools_utils.ml]
>>>>>         c_inspect_decrypt                    [common/mltools/tools_utils.ml]
>>>>>           guestfs_int_mllib_inspect_decrypt  [common/mltools/tools_utils-c.c]
>>>>>
>>>>>             loop over cmdline_options.ks:
>>>>>               key_store_import_key           [common/options/keys.c]
>>>>>
>>>>>             inspect_do_decrypt               [common/options/decrypt.c]
>>>>>               decrypt_mountables             [common/options/decrypt.c]
>>>>>                 MACRO CHECK HERE
>>>>>                   guestfs_clevis_luks_unlock
>>>>>             free_key_store                   [common/options/keys.c]
>>>>>
>>>>> This is why I said "it's too late to report this error in
>>>>> decrypt_mountables()"!
>>>>>
>>>>> In comparison, here's how the C tools work (such as virt-cat):
>>>>>
>>>>>   main                                   [cat/cat.c]
>>>>>     OPTION_key                           [common/options/options.h]
>>>>>       key_store_add_from_selector        [common/options/keys.c]
>>>>>         key_store_import_key             [common/options/keys.c]
>>>>>
>>>>>     /* "ks" has been populated */
>>>>>
>>>>>     key_store_requires_network           [common/options/keys.c]
>>>>>     guestfs_set_network
>>>>>     guestfs_launch
>>>>>     inspect_mount                        [common/options/options.h]
>>>>>       inspect_mount_handle               [common/options/inspect.c]
>>>>>         inspect_do_decrypt               [common/options/decrypt.c]
>>>>>           decrypt_mountables             [common/options/decrypt.c]
>>>>>             MACRO CHECK HERE
>>>>>               guestfs_clevis_luks_unlock
>>>>>     free_key_store                       [common/options/keys.c]
>>>>>
>>>>> Note the following important facts:
>>>>>
>>>>> (1) The macro check is reached, in both kinds of tools, way after the
>>>>> options have been parsed, and quite a bit after the appliance has been
>>>>> launched.
>>>>>
>>>>> (2) The OCaml and C *option parsers* share *nothing* related to keys.
>>>>> Consider:
>>>>>
>>>>> - OCaml does not call key_store_add_from_selector() at all,
>>>>>
>>>>> - while OCaml does call key_store_import_key(), it does so *outside* of
>>>>> option parsing. Namely, "parse_key_selector" records the command line
>>>>> options in an OCaml-only data structure (cmdline_options.ks). That is
>>>>> then translated to an *ephemeral* C-language keystore much later, with
>>>>> key_store_import_key() -- only in guestfs_int_mllib_inspect_decrypt()!
>>>>> That is, after the appliance is running.
>>>>>
>>>>> - Correspondingly: in the C tools, the C-language keystore is freed
>>>>> *after* inspection completes (that is, free_key_store() is called
>>>>> *after* inspect_mount() returns), but in OCaml, the ephemeral keystore
>>>>> is released (correctly) *inside* guestfs_int_mllib_inspect_decrypt() --
>>>>> that is, internally to the outer function "inspect_decrypt".
>>>>>
>>>>>
>>>>> If we want to catch the lack of GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK during
>>>>> option parsing, then I need to implement a brand new C function, and
>>>>> wrap it for OCaml too (OCaml cannot check the C-language macro
>>>>> GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK).
>>>>>
>>>>> For this: because we already have "key_store_requires_network" (same
>>>>> name, but totally separate impl in C and OCaml), I'd have to rename that
>>>>> function (both impls) to "key_store_contains_clevis", and use it for two
>>>>> purposes:
>>>>>
>>>>> - right after option parsing, if the key store contains clevis, but the
>>>>> feature test macro is missing, bail.
>>>>>
>>>>> - just before launching the appliance, if the key store contains clevis,
>>>>> enable networking.
>>>>
>>>> There could be an easier solution for this BTW. Namely:
>>>>
>>>> - For the C language tools, extend key_store_add_from_selector() with
>>>> the GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK macro check. (Review the call tree
>>>> to see what that's right.) This would be a small modification to
>>>
>>> OK, I see, this sounds like a better / earlier check (don't even add a
>>> clevis key if we know it's going t fail later).
>>>
>>>> - For the OCaml language tools, extend "parse_key_selector" with the
>>>> macro check. For that, I'll add a very small helper function.
>>>
>>> And same here.
>>>
>>>> Both of these changes only affect patch "options, mltools/tools_utils:
>>>> parse "--key ID:clevis" options".
>>>>
>>>> Regarding the present patch, I'll keep the macro check, but replace the
>>>> error message with abort() -- as it should never be reached.
>>>
>>> Yup.
>>>
>>>> I'll post v3, hopefully soon.
>>>
>>> OK, this seems better, although TBH the late check in virt-v2v wasn't
>>> really that bad.
>>
>> I'm totally happy to merge this version (v2) as well!
>>
>> It's just that you wrote
>>
>>> To be clear on why this is correct, it's because this code is only
>>> used during option parsing in command line tools, where we want to
>>> exit when an error happens.
>>
>> and letting that "slide" un-answered would have been dishonest from me,
>> knowing that the error wouldn't be found during option parsing.
>>
>> That is: I don't insist on v3 at all. I made the proposal for v3 in
>> order to satisfy the (perceived) requirement from you.
>>
>> If you are OK with the v2 behavior however, so am I :) Please let me
>> know which way you prefer.
> 
> TBH I'm fine with v2, but if you've done significant work on
> v3, then we could do that.  You decide!

OK, let's go with v2 then -- I wanted to be sure of your opinion of v3's
necessity before starting it, so I've not written one line of code for
it yet :)

I'll merge all four series hopefully today.

Thanks,
Laszlo