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

[Richard W.M. Jones]

On Fri, Feb 25, 2022 at 11:59:37AM +0100, Laszlo Ersek wrote:
> 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.

That's fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v