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

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.