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

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!