Re: [Libguestfs] [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.

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.