[Libguestfs] [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices

[Laszlo Ersek]

On 02/24/22 13:12, Laszlo Ersek wrote:
> On 02/24/22 11:43, Richard W.M. Jones wrote:
>> On Wed, Feb 23, 2022 at 05:19:15PM +0100, Laszlo Ersek wrote:
>>> Using the previously extracted function decrypt_mountables(), look for
>>> LUKS devices on logical volumes (LVs) too.
>>>
>>> In the LVM-on-LUKS scheme, the names of the plaintext (decrypted) block
>>> devices don't matter, as these devices host Physical Volumes, and LVM
>>> enumerates PVs automatically -- there are no references to these decrypted
>>> block devices in "/etc/fstab", for example. For naming such decrypted
>>> devices, continue calling make_mapname().
>>>
>>> In the LUKS-on-LVM scheme however, the decrypted devices are supposed to
>>> hold filesystems, and "/etc/fstab" may refer to them. Such decrypted
>>> devices are commonly called /dev/mapper/luks-<UUID>, where <UUID> is the
>>> UUID inside the LUKS header. Employ this naming when decrypting Logical
>>> Volumes. Reuse make_mapname() as a fallback in this case.
>>>
>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>>
>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
>>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>>> ---
>>>  options/decrypt.c | 24 ++++++++++++++++++------
>>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/options/decrypt.c b/options/decrypt.c
>>> index 9141cf5193ad..f7c1d876b3ed 100644
>>> --- a/options/decrypt.c
>>> +++ b/options/decrypt.c
>>> @@ -38,8 +38,11 @@
>>>  #include "options.h"
>>>  
>>>  /**
>>> - * Make a LUKS map name from the partition name,
>>> - * eg. C<"/dev/vda2" =E<gt> "cryptvda2">
>>> + * Make a LUKS map name from the partition or logical volume name, eg.
>>> + * C<"/dev/vda2" =E<gt> "cryptvda2">, or C<"/dev/vg-ssd/lv-root7" =E<gt>
>>> + * "cryptvgssdlvroot7">.  Note that, in logical volume device names,
>>> + * c_isalnum() eliminates the "/" separator from between the VG and the LV, so
>>> + * this mapping is not unique; but for our purposes, it will do.
>>>   */
>>>  static void
>>>  make_mapname (const char *device, char *mapname, size_t len)
>>> @@ -67,7 +70,7 @@ make_mapname (const char *device, char *mapname, size_t len)
>>>  
>>>  static bool
>>>  decrypt_mountables (guestfs_h *g, const char * const *mountables,
>>> -                    struct key_store *ks)
>>> +                    struct key_store *ks, bool name_decrypted_by_uuid)
>>>  {
>>>    bool decrypted_some = false;
>>>    const char * const *mnt_scan = mountables;
>>> @@ -77,7 +80,7 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables,
>>>      CLEANUP_FREE char *type = NULL;
>>>      CLEANUP_FREE char *uuid = NULL;
>>>      CLEANUP_FREE_STRING_LIST char **keys = NULL;
>>> -    char mapname[32];
>>> +    char mapname[512];
>>>      const char * const *key_scan;
>>>      const char *key;
>>>  
>>> @@ -102,7 +105,9 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables,
>>>      assert (keys[0] != NULL);
>>>  
>>>      /* Generate a node name for the plaintext (decrypted) device node. */
>>> -    make_mapname (mountable, mapname, sizeof mapname);
>>> +    if (!name_decrypted_by_uuid || uuid == NULL ||
>>> +        snprintf (mapname, sizeof mapname, "luks-%s", uuid) < 0)
>>> +      make_mapname (mountable, mapname, sizeof mapname);
>>
>> This should all really be using asprintf, and not stack-allocating
>> large arrays.  Although it's kind of not related to this change, this
>> change does make the stack frame larger.
> 
> asprintf() was my first idea, but reworking make_mapname() would then
> require measuring the input string lenght ;)
> 
> OK, I'll insert a patch between #1 and #2 for converting make_mapname()
> to asprintf(), then I'll redo this patch with asprintf() too.

On a second thought, if it's OK with you:

I'd prefer merging this series as-is, and then posting additional
patches on top:

- assume GUESTFS_HAVE_LUKS_UUID
- assume GUESTFS_HAVE_CRYPTSETUP_OPEN
- bump libguestfs version requirement in
virt-v2v.git/m4/guestfs-libraries.m4 to 1.44
- remove "XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY"
- allocate "mapname" with asprintf() on all paths.

The reason I'd like to do these afterwards is that none of these warts
are introduced/created by this series, and I had put lots of testing
into the series (not just at the end, but mid-series too). If I rebased
and needed to resolved conflicts, all that testing would have to be
redone. I'd really like to avoid that.

Thanks!
Laszlo





> 
> Thanks
> Laszlo
> 
>>
>> Saying that, when removing gnulib (libguestfs commit 0f54df53d26), I
>> dropped the warning about large stack frames:
>>
>>   -dnl Warn about large stack frames, including estimates for alloca
>>   -dnl and variable length arrays.
>>   -gl_WARN_ADD([-Wstack-usage=10000])
>>
>> I really need to add that back ...
>>
>>>      /* Try each key in turn. */
>>>      key_scan = (const char * const *)keys;
>>> @@ -145,15 +150,22 @@ void
>>>  inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
>>>  {
>>>    CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g);
>>> +  CLEANUP_FREE_STRING_LIST char **lvs = NULL;
>>>    bool need_rescan;
>>>  
>>>    if (partitions == NULL)
>>>      exit (EXIT_FAILURE);
>>>  
>>> -  need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks);
>>> +  need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks,
>>> +                                    false);
>>>  
>>>    if (need_rescan) {
>>>      if (guestfs_lvm_scan (g, 1) == -1)
>>>        exit (EXIT_FAILURE);
>>>    }
>>> +
>>> +  lvs = guestfs_lvs (g);
>>> +  if (lvs == NULL)
>>> +    exit (EXIT_FAILURE);
>>> +  decrypt_mountables (g, (const char * const *)lvs, ks, true);
>>
>> ACK
>>
>> Rich.
>>
>