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

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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.

Thanks!
Laszlo