[Libguestfs] [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)
Richard W.M. Jones
rjones at redhat.com
Thu Feb 24 10:35:23 UTC 2022
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);
> +#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.
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/
More information about the Libguestfs
mailing list