Laszlo Ersek
2022-Feb-23 16:19 UTC
[Libguestfs] [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)
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 at redhat.com> --- 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 + } 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'. + */ + 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) -- 2.19.1.3.g30247aa5d201
Richard W.M. Jones
2022-Feb-24 10:35 UTC
[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 at redhat.com> > --- > 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); > +#endifAs 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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/