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

Thu Feb 24 11:44:13 UTC 2022

On 02/24/22 11:35, Richard W.M. Jones wrote:
> 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.

Thanks, those #ifdefs did look stale to me. The cleanups should go into
a separate patch; I'll look into that.

> 
>> +    } 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.

Will do (separate commit indeed).

> 
>> +      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.

Thanks!
Laszlo