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

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(a)redhat.com&gt; >>>> >>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 >>>> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >>>> --- >>>> 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.