[Libguestfs] [libguestfs-common PATCH v2 06/12] options: add back-end for LUKS decryption with Clevis+Tang
Richard W.M. Jones
rjones at redhat.com
Fri Jul 1 12:33:38 UTC 2022
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!
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
More information about the Libguestfs
mailing list