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

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

- For the OCaml language tools, extend "parse_key_selector" with the macro check. For that, I'll add a very small helper function.

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.

I'll post v3, hopefully soon.