Re: [Libguestfs] [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)

On Wed, Feb 23, 2022 at 05:19:14PM +0100, Laszlo Ersek wrote: > The inspect_do_decrypt() function interates over the list of partitions. > For each partition, it fetches the potentially matching keys, and tries > each key in turn. If no key unlocks the partition, the program exits with > an error message. If at least one partition is unlocked, then LVM is > rescanned. > > Decouple "partition" from the above logic, replacing it with "mountable". > Call the new function decrypt_mountables(). As a part of the extraction, > clean up the following readability warts: > > - CLEANUP_FREE* and other variable declarations intermixed with code, > > - a "goto" statement that is not used on an error path, > > - needless conditions on a one-shot "is_bitlocker" helper variable, > > - nondescript array indices, > > - guestfs_int_count_strings() called for counting the length of the full > string list, only to check if the string list is non-empty, > > - a comment about GUESTFS_CRYPTSETUP_OPEN_READONLY placed outside of > GUESTFS_HAVE_CRYPTSETUP_OPEN. > > Regression-checked with: > - libguestfs/tests/luks/test-key-option-inspect.sh > - guestfs-tools/inspector/test-virt-inspector-luks.sh > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > options/decrypt.c | 128 +++++++++++++++++++++++++++------------------- > 1 file changed, 75 insertions(+), 53 deletions(-) > > diff --git a/options/decrypt.c b/options/decrypt.c > index 434b7d58c2a7..9141cf5193ad 100644 > --- a/options/decrypt.c > +++ b/options/decrypt.c > @@ -65,6 +65,78 @@ make_mapname (const char *device, char *mapname, size_t len) > *mapname = '\0'; > } > > +static bool > +decrypt_mountables (guestfs_h *g, const char * const *mountables, > + struct key_store *ks) > +{ > + bool decrypted_some = false; > + const char * const *mnt_scan = mountables; > + const char *mountable; > + > + while ((mountable = *mnt_scan++) != NULL) { > + CLEANUP_FREE char *type = NULL; > + CLEANUP_FREE char *uuid = NULL; > + CLEANUP_FREE_STRING_LIST char **keys = NULL; > + char mapname[32]; > + const char * const *key_scan; > + const char *key; > + > + type = guestfs_vfs_type (g, mountable); > + if (type == NULL) > + continue; > + > + /* "cryptsetup luksUUID" cannot read a UUID on Windows BitLocker disks > + * (unclear if this is a limitation of the format or cryptsetup). > + */ > + if (STREQ (type, "crypto_LUKS")) { > +#ifdef GUESTFS_HAVE_LUKS_UUID > + uuid = guestfs_luks_uuid (g, mountable); > +#endif As a separate patch after this one, I don't mind getting rid of this #ifdef conditional, and also the GUESTFS_HAVE_CRYPTSETUP_OPEN conditional below. guestfs_luks_uuid was added in libguestfs 1.42 (released March 2020). guestfs_cryptsetup_open was added in libguestfs 1.44 (Jan 2021). guestfs-tools already requires libguestfs >= 1.44 (see guestfs-tools.git/m4/guestfs-libraries.m4). So that's OK. virt-v2v in theory requires only libguestfs >= 1.40, but I don't think there's any problem with requiring 1.44. It already requires a very recent libnbd. virt-v2v.git/m4/guestfs-libraries.m4 would need to be changed.

> + } else if (STRNEQ (type, "BitLocker")) > + continue; > + > + /* Grab the keys that we should try with this device, based on device name, > + * or UUID (if any). > + */ > + keys = get_keys (ks, mountable, uuid); > + assert (keys[0] != NULL); > + > + /* Generate a node name for the plaintext (decrypted) device node. */ > + make_mapname (mountable, mapname, sizeof mapname); > + > + /* Try each key in turn. */ > + key_scan = (const char * const *)keys; > + while ((key = *key_scan++) != NULL) { > + int r; > + > + guestfs_push_error_handler (g, NULL, NULL); > +#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN > + /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly is > + * set? This might break 'mount_ro'. > + */ This XXX comment (copied from the old code) is bogus. It would definitely be wrong to use OPEN_READONLY. In r/o mode we defend against modifying the underlying devices using a qcow2 overlay (lib/drives.c:create_overlay). Writable guest devices are required to mount filesystems with journals, even when mounting with -o ro. So you might drop it, maybe as a separate commit.

> + r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1); > +#else > + r = guestfs_luks_open (g, mountable, key, mapname); > +#endif > + guestfs_pop_error_handler (g); > + > + if (r == 0) > + break; > + } > + > + if (key == NULL) > + error (EXIT_FAILURE, 0, > + _("could not find key to open LUKS encrypted %s.\n\n" > + "Try using --key on the command line.\n\n" > + "Original error: %s (%d)"), > + mountable, guestfs_last_error (g), guestfs_last_errno (g)); > + > + decrypted_some = true; > + } > + > + return decrypted_some; > +} > + > /** > * Simple implementation of decryption: look for any encrypted > * partitions and decrypt them, then rescan for VGs. > @@ -73,62 +145,12 @@ void > inspect_do_decrypt (guestfs_h *g, struct key_store *ks) > { > CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g); > + bool need_rescan; > + > if (partitions == NULL) > exit (EXIT_FAILURE); > > - int need_rescan = 0, r; > - size_t i, j; > - > - for (i = 0; partitions[i] != NULL; ++i) { > - CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]); > - if (type && > - (STREQ (type, "crypto_LUKS") || STREQ (type, "BitLocker"))) { > - bool is_bitlocker = STREQ (type, "BitLocker"); > - char mapname[32]; > - make_mapname (partitions[i], mapname, sizeof mapname); > - > -#ifdef GUESTFS_HAVE_LUKS_UUID > - CLEANUP_FREE char *uuid = NULL; > - > - /* This fails for Windows BitLocker disks because cryptsetup > - * luksUUID cannot read a UUID (unclear if this is a limitation > - * of the format or cryptsetup). > - */ > - if (!is_bitlocker) > - uuid = guestfs_luks_uuid (g, partitions[i]); > -#else > - const char *uuid = NULL; > -#endif > - > - CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i], uuid); > - assert (guestfs_int_count_strings (keys) > 0); > - > - /* Try each key in turn. */ > - for (j = 0; keys[j] != NULL; ++j) { > - /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly > - * is set? This might break 'mount_ro'. > - */ > - guestfs_push_error_handler (g, NULL, NULL); > -#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN > - r = guestfs_cryptsetup_open (g, partitions[i], keys[j], mapname, -1); > -#else > - r = guestfs_luks_open (g, partitions[i], keys[j], mapname); > -#endif > - guestfs_pop_error_handler (g); > - if (r == 0) > - goto opened; > - } > - error (EXIT_FAILURE, 0, > - _("could not find key to open LUKS encrypted %s.\n\n" > - "Try using --key on the command line.\n\n" > - "Original error: %s (%d)"), > - partitions[i], guestfs_last_error (g), > - guestfs_last_errno (g)); > - > - opened: > - need_rescan = 1; > - } > - } > + need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks); > > if (need_rescan) { > if (guestfs_lvm_scan (g, 1) == -1) Patch looks fine, ACK.