10:18 a.m.
Hi,
for <https://bugzilla.redhat.com/show_bug.cgi?id=1658126>;, I'll soon
post four short patch sets in response to this email (I'm sending this
email basically as a common cover letter for those sets, for better
threading).
The main difference (for libguestfs & co) between LUKS-on-LVM and
LVM-on-LUKS is that in the latter, the names of the decrypted LUKS block
devices don't matter. However we name the decrypted devices via
inspect_do_decrypt() -> make_mapname() in "common/options/decrypt.c" is
irrelevant, as LVM will scan for PVs on all block devices, regardless of
their names. The resultant LVs (hosting the filesystems) are what
"/etc/fstab" refers to, and we can deal with those names -- the VG and
LV names -- well, because (a) there is an "lvs" command (and API) for
enumeration (see "lv_canonical" in "daemon/lvm_utils.ml", and
lv_canonical() in "daemon/lvm.c" -- see also commit c37c8e770d664), and
(b) the VG and LV names are stored in the LVM metadata, so libguestfs
doesn't have to "invent" names.
This is not the case with LUKS-on-LVM. The names of the decrypted LUKS
devices is relevant, as they directly host filesystems, and "/etc/fstab"
likely refers to them. Furthermore, there is no enumeration / scanning
inside the guest, only a file called "/etc/crypttab". Unfortunately, the
root filesystem containing "/etc/crypttab" tends to exist on a LUKS
device as well, so we can't consult it for determining the decrypted
device names when we're about to decrypt those same devices in
inspect_do_decrypt(). (This circular dependency is circumvented during
normal guest boot by having a plaintext initrd containing a copy of
"/etc/crypttab", and LUKS-related parameters on the a kernel command
line in the bootloader config.)
In fact, since at least RHEL6, when the user selects either LVM-on-LUKS
or LUKS-on-LVM in Anaconda, the installer never asks for LUKS device
names (while it does let the user set VG and LV names). Instead, for a
given LUKS header containing UUID <UUID>, the decrypted block device
will automatically be named /dev/mapper/luks-<UUID>. (Anaconda sets up
"/etc/crypttab" and "/etc/fstab" like that.)
The patches I'm about to post follow the same logic -- I don't attempt
intricate UUID-based (re)mappings through "/etc/crypttab", in
"check_fstab_entry" -> "resolve_fstab_device", in
"daemon/inspect_fs_unix_fstab.ml".
(Assume that "/etc/fstab" refers to "/dev/mapper/hunter2" as a
plaintext
block device to mount.
Assume that "/etc/crypttab" associates the "hunter2" device name with
a
block device containing a LUKS header with UUID
28372dec-afca-412a-a089-736437fd03af. (For us to realize this, we'd need
to parse "/etc/crypttab" with Augeas in the first place.)
Then we'd have to recall, by UUID, that we had mapped that particular
LUKS device as "/dev/mapper/crypt<WHATEVER>" in inspect_do_decrypt().
Then either close and reopen the LUKS device as "hunter2" (there is no
rename operation!), which could get messy as the dependent filesystem
would be in use, or replace "hunter2" with "crypt<WHATEVER>" in
both
"/etc/fstab" and "/etc/crypttab".)
Instead, accept the "/dev/mapper/luks-<UUID>" naming scheme as the
standard one. Thus, inspect_do_decrypt() will name the decrypted block
device "/dev/mapper/luks-<UUID>" at once (rather than
"/dev/mapper/crypt<WHATEVER>"). And that name is expected to match the
references in "/etc/fstab" and "/etc/crypttab".
I've tested this with RHEL-6 and RHEL-7 guests successfully (using
"guestfish -i", "virt-inspector", and virt-v2v). Example
"lsblk"
outputs, with "lsblk" executed in the converted domains:
RHEL6:
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sr0 11:0 1 1024M 0 rom
vda 252:0 0 20G 0 disk
├─vda1 252:1 0 500M 0 part /boot
└─vda2 252:2 0 19.5G 0 part
├─vg_rhel6luks-root (dm-0) 253:0 0 14.7G 0 lvm
│ └─luks-0d619854-ccd5-43b1-8883-991fec5ef713 (dm-2)
253:2 0 14.7G 0 crypt /
└─vg_rhel6luks-swap (dm-1) 253:1 0 4.9G 0 lvm
└─luks-4e9e7a6f-a68c-42fd-92b4-8f4f2579a389 (dm-3)
253:3 0 4.9G 0 crypt [SWAP]
RHEL7:
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
vda 252:0 0 20G 0 disk
├─vda1 252:1 0 1G 0 part /boot
└─vda2 252:2 0 19G 0 part
├─rhel_rhel7luks-root 253:0 0 10G 0 lvm
│ └─luks-74558d73-1ef3-4327-878b-f11b266a20fc 253:3 0 10G 0 crypt /
├─rhel_rhel7luks-swap 253:1 0 3.9G 0 lvm
│ └─luks-621270fd-6758-4001-8335-492cf5e32a45 253:2 0 3.9G 0 crypt [SWAP]
└─rhel_rhel7luks-home 253:4 0 5.1G 0 lvm
└─luks-0433f315-afd7-41a0-bd94-3f742114035c 253:5 0 5.1G 0 crypt /home
Thanks,
Laszlo
10:19 a.m.
New subject: [libguestfs-common PATCH 0/2] options: decrypt LUKS-on-LV devices
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Laszlo Ersek (2):
options: extract & refactor decryption of mountables (partitions)
options: decrypt LUKS-on-LV devices
options/decrypt.c | 132 +++++++++++++++++++++++++++++-----------------
1 file changed, 83 insertions(+), 49 deletions(-)
base-commit: 41126802097f0a864cab8679ae8672b45914d54b
--
2.19.1.3.g30247aa5d201
10:19 a.m.
New subject: [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)
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(a)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
+ } 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'.
+ */
+ 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)
--
2.19.1.3.g30247aa5d201
4:35 a.m.
New subject: [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)
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(a)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.
+ } 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.
+ 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.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
5:44 a.m.
New subject: [libguestfs-common PATCH 1/2] options: extract & refactor decryption of mountables (partitions)
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(a)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
10:19 a.m.
New subject: [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)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);
/* 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);
}
--
2.19.1.3.g30247aa5d201
4:43 a.m.
New subject: [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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>
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)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.
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.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
6:12 a.m.
New subject: [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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>
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
> Signed-off-by: Laszlo Ersek <lersek(a)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.
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.
4:59 a.m.
New subject: [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>
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
>> Signed-off-by: Laszlo Ersek <lersek(a)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.
>
3 a.m.
New subject: [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>
>>>
>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
>>> Signed-off-by: Laszlo Ersek <lersek(a)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
5:58 a.m.
New subject: [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
On 02/28/22 10:00, Richard W.M. Jones wrote:
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>
>>>>
>>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
>>>> Signed-off-by: Laszlo Ersek <lersek(a)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.
Thanks!
Commit range 41126802097f..2d8c0f8d40b5 (for now).
Thanks,
Laszlo
10:21 a.m.
New subject: [libguestfs PATCH 0/3] tests: add LUKS-on-LVM test
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Laszlo Ersek (3):
tests: rename "luks" to "lvm-on-luks"
tests: add LUKS-on-LVM test
TODO: remove "Better support for encrypted devices"
.gitignore | 3
+-
TODO | 10
--
test-data/phony-guests/Makefile.am | 15
++-
test-data/phony-guests/guests.xml.in | 22
++++-
test-data/phony-guests/make-fedora-img.pl | 62
+++++++++++-
tests/Makefile.am | 6
+-
tests/luks/test-key-option-inspect-luks-on-lvm.sh | 103
++++++++++++++++++++
tests/luks/{test-key-option-inspect.sh => test-key-option-inspect-lvm-on-luks.sh} |
4 +-
8 files changed, 200 insertions(+), 25 deletions(-)
create mode 100755 tests/luks/test-key-option-inspect-luks-on-lvm.sh
rename tests/luks/{test-key-option-inspect.sh =>
test-key-option-inspect-lvm-on-luks.sh} (94%)
--
2.19.1.3.g30247aa5d201
10:21 a.m.
New subject: [libguestfs PATCH 1/3] tests: rename "luks" to "lvm-on-luks"
Clarify that our current usage of "luks" stands for "lvm-on-luks"
(IOW,
that the decrypted LUKS devices are Physical Volumes for LVM).
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
test-data/phony-guests/Makefile.am | 8
++++----
tests/Makefile.am | 4
++--
.gitignore | 2 +-
test-data/phony-guests/guests.xml.in | 4
++--
test-data/phony-guests/make-fedora-img.pl | 8
++++----
tests/luks/{test-key-option-inspect.sh => test-key-option-inspect-lvm-on-luks.sh} | 4
++--
6 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am
index 60313548af32..0114d10bb170 100644
--- a/test-data/phony-guests/Makefile.am
+++ b/test-data/phony-guests/Makefile.am
@@ -49,7 +49,7 @@ disk_images = \
fedora-md1.img \
fedora-md2.img \
fedora-btrfs.img \
- fedora-luks.img \
+ fedora-lvm-on-luks.img \
ubuntu.img \
archlinux.img \
coreos.img \
@@ -96,11 +96,11 @@ fedora-btrfs.img: make-fedora-img.pl \
fedora.db
SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$<
-# Make a (dummy) Fedora image with LVM encrypted with LUKS.
-fedora-luks.img: make-fedora-img.pl \
+# Make a (dummy) Fedora image with LVM-on-LUKS.
+fedora-lvm-on-luks.img: make-fedora-img.pl \
fedora-journal.tar.xz \
fedora.db
- SRCDIR=$(srcdir) LAYOUT=lvm-luks $(top_builddir)/run --test ./$<
+ SRCDIR=$(srcdir) LAYOUT=lvm-on-luks $(top_builddir)/run --test ./$<
# Make a (dummy) Debian image.
debian.img: make-debian-img.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 690e09b5e603..9112726750a7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -464,12 +464,12 @@ TESTS += \
luks/test-luks.sh \
luks/test-luks-list.sh \
luks/test-key-option.sh \
- luks/test-key-option-inspect.sh
+ luks/test-key-option-inspect-lvm-on-luks.sh
EXTRA_DIST += \
luks/test-luks.sh \
luks/test-luks-list.sh \
luks/test-key-option.sh \
- luks/test-key-option-inspect.sh
+ luks/test-key-option-inspect-lvm-on-luks.sh
TESTS += \
lvm/test-lvm-filtering.sh \
diff --git a/.gitignore b/.gitignore
index ebb67850c6c6..8fb61aa7bb66 100644
--- a/.gitignore
+++ b/.gitignore
@@ -452,7 +452,7 @@ Makefile.in
/test-data/phony-guests/debian.img
/test-data/phony-guests/fedora.img
/test-data/phony-guests/fedora-btrfs.img
-/test-data/phony-guests/fedora-luks.img
+/test-data/phony-guests/fedora-lvm-on-luks.img
/test-data/phony-guests/fedora-md1.img
/test-data/phony-guests/fedora-md2.img
/test-data/phony-guests/fedora.db
diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in
index 4139d04f689f..3af6b27af073 100644
--- a/test-data/phony-guests/guests.xml.in
+++ b/test-data/phony-guests/guests.xml.in
@@ -185,7 +185,7 @@
<!-- LUKS password is 'FEDORA' -->
<domain type='test'>
- <name>fedora-luks</name>
+ <name>fedora-lvm-on-luks</name>
<memory>1048576</memory>
<os>
<type>hvm</type>
@@ -194,7 +194,7 @@
<devices>
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
- <source file='@abs_builddir(a)/fedora-luks.img'/>
+ <source file='@abs_builddir(a)/fedora-lvm-on-luks.img'/>
<target dev='vda' bus='virtio'/>
</disk>
</devices>
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index 27b154f9d71d..4cd6ef9575eb 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -171,8 +171,8 @@ EOF
}
}
-elsif ($ENV{LAYOUT} eq 'lvm-luks') {
- push (@images, "fedora-luks.img-t");
+elsif ($ENV{LAYOUT} eq 'lvm-on-luks') {
+ push (@images, "fedora-lvm-on-luks.img-t");
open (my $fstab, '>', "fedora.fstab") or die;
print $fstab <<EOF;
@@ -183,9 +183,9 @@ EOF
$bootdev = '/dev/sda1';
- $g->disk_create ("fedora-luks.img-t", "raw", $IMAGE_SIZE);
+ $g->disk_create ("fedora-lvm-on-luks.img-t", "raw",
$IMAGE_SIZE);
- $g->add_drive ("fedora-luks.img-t", format => "raw");
+ $g->add_drive ("fedora-lvm-on-luks.img-t", format =>
"raw");
$g->launch ();
$g->part_init ('/dev/sda', 'mbr');
diff --git a/tests/luks/test-key-option-inspect.sh
b/tests/luks/test-key-option-inspect-lvm-on-luks.sh
similarity index 94%
rename from tests/luks/test-key-option-inspect.sh
rename to tests/luks/test-key-option-inspect-lvm-on-luks.sh
index 78128db84b52..3d2bb9e6c843 100755
--- a/tests/luks/test-key-option-inspect.sh
+++ b/tests/luks/test-key-option-inspect-lvm-on-luks.sh
@@ -24,9 +24,9 @@ set -e
$TEST_FUNCTIONS
skip_if_skipped
skip_unless_feature_available luks
-skip_unless_phony_guest fedora-luks.img
+skip_unless_phony_guest fedora-lvm-on-luks.img
-disk=../test-data/phony-guests/fedora-luks.img
+disk=../test-data/phony-guests/fedora-lvm-on-luks.img
device=/dev/sda2
# Get the UUID of the LUKS device.
--
2.19.1.3.g30247aa5d201
10:21 a.m.
New subject: [libguestfs PATCH 2/3] tests: add LUKS-on-LVM test
Create a new (fake) Fedora disk image with two partitions. /dev/sda1 is
the boot partition as usual, /dev/sda2 is used as an LVM PV. The VG has
four LVs, Root and LV1 through LV3.
Each LV holds a LUKS device (with a different key). Each decrypted LUKS
device holds an ext2 filesystem, with "/dev/mapper/Root-luks" holding the
root filesystem.
Each filesystem has a dedicated label (ROOT, LV1, LV2, LV3).
In the test case, run guestfish in inspector mode, twice.
In the first invocation, provide the LUKS passphrases by LV name. Also
specific to the first invocation, fetch the LUKS UUIDs by LV name.
In the second invocation, provide the LUKS passphrases by UUID.
In both invocations, after decryption, check the filesystem labels, the
/dev/mapper/* names generated for the decrypted LUKS block devices, and
the existence of "/etc/fedora-release" on the root filesystem.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
test-data/phony-guests/Makefile.am | 7 ++
tests/Makefile.am | 2 +
.gitignore | 1 +
test-data/phony-guests/guests.xml.in | 18 ++++
test-data/phony-guests/make-fedora-img.pl | 54 ++++++++++
tests/luks/test-key-option-inspect-luks-on-lvm.sh | 103 ++++++++++++++++++++
6 files changed, 185 insertions(+)
diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am
index 0114d10bb170..0a41f168bdfc 100644
--- a/test-data/phony-guests/Makefile.am
+++ b/test-data/phony-guests/Makefile.am
@@ -49,6 +49,7 @@ disk_images = \
fedora-md1.img \
fedora-md2.img \
fedora-btrfs.img \
+ fedora-luks-on-lvm.img \
fedora-lvm-on-luks.img \
ubuntu.img \
archlinux.img \
@@ -96,6 +97,12 @@ fedora-btrfs.img: make-fedora-img.pl \
fedora.db
SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$<
+# Make a (dummy) Fedora image with LUKS-on-LVM.
+fedora-luks-on-lvm.img: make-fedora-img.pl \
+ fedora-journal.tar.xz \
+ fedora.db
+ SRCDIR=$(srcdir) LAYOUT=luks-on-lvm $(top_builddir)/run --test ./$<
+
# Make a (dummy) Fedora image with LVM-on-LUKS.
fedora-lvm-on-luks.img: make-fedora-img.pl \
fedora-journal.tar.xz \
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9112726750a7..9a2a2aab59a5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -464,11 +464,13 @@ TESTS += \
luks/test-luks.sh \
luks/test-luks-list.sh \
luks/test-key-option.sh \
+ luks/test-key-option-inspect-luks-on-lvm.sh \
luks/test-key-option-inspect-lvm-on-luks.sh
EXTRA_DIST += \
luks/test-luks.sh \
luks/test-luks-list.sh \
luks/test-key-option.sh \
+ luks/test-key-option-inspect-luks-on-lvm.sh \
luks/test-key-option-inspect-lvm-on-luks.sh
TESTS += \
diff --git a/.gitignore b/.gitignore
index 8fb61aa7bb66..4c7656fccd6e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -452,6 +452,7 @@ Makefile.in
/test-data/phony-guests/debian.img
/test-data/phony-guests/fedora.img
/test-data/phony-guests/fedora-btrfs.img
+/test-data/phony-guests/fedora-luks-on-lvm.img
/test-data/phony-guests/fedora-lvm-on-luks.img
/test-data/phony-guests/fedora-md1.img
/test-data/phony-guests/fedora-md2.img
diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in
index 3af6b27af073..dfcccc3d4cff 100644
--- a/test-data/phony-guests/guests.xml.in
+++ b/test-data/phony-guests/guests.xml.in
@@ -183,6 +183,24 @@
</devices>
</domain>
+ <!-- LUKS passwords are 'FEDORA-Root', 'FEDORA-LV1',
'FEDORA-LV2',
+ 'FEDORA-LV3' -->
+ <domain type='test'>
+ <name>fedora-luks-on-lvm</name>
+ <memory>1048576</memory>
+ <os>
+ <type>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <devices>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source file='@abs_builddir(a)/fedora-luks-on-lvm.img'/>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ </devices>
+ </domain>
+
<!-- LUKS password is 'FEDORA' -->
<domain type='test'>
<name>fedora-lvm-on-luks</name>
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index 4cd6ef9575eb..84d4742e9000 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -200,6 +200,60 @@ EOF
init_lvm_root ('/dev/mapper/luks');
}
+elsif ($ENV{LAYOUT} eq 'luks-on-lvm') {
+ push (@images, "fedora-luks-on-lvm.img-t");
+
+ open (my $fstab, '>', "fedora.fstab") or die;
+ print $fstab <<EOF;
+LABEL=BOOT /boot ext2 default 0 0
+LABEL=ROOT / ext2 default 0 0
+EOF
+ close ($fstab) or die;
+
+ $bootdev = '/dev/sda1';
+
+ $g->disk_create ("fedora-luks-on-lvm.img-t", "raw",
$IMAGE_SIZE);
+
+ $g->add_drive ("fedora-luks-on-lvm.img-t", format =>
"raw");
+ $g->launch ();
+
+ $g->part_init ('/dev/sda', 'mbr');
+ foreach my $p (@PARTITIONS) {
+ $g->part_add('/dev/sda', @$p);
+ }
+
+ # Create the Volume Group on /dev/sda2.
+ $g->pvcreate ('/dev/sda2');
+ $g->vgcreate ('VG', ['/dev/sda2']);
+ $g->lvcreate ('Root', 'VG', 32);
+ $g->lvcreate ('LV1', 'VG', 32);
+ $g->lvcreate ('LV2', 'VG', 32);
+ $g->lvcreate ('LV3', 'VG', 64);
+
+ # Format each Logical Group as a LUKS device, with a different password.
+ $g->luks_format ('/dev/VG/Root', 'FEDORA-Root', 0);
+ $g->luks_format ('/dev/VG/LV1', 'FEDORA-LV1', 0);
+ $g->luks_format ('/dev/VG/LV2', 'FEDORA-LV2', 0);
+ $g->luks_format ('/dev/VG/LV3', 'FEDORA-LV3', 0);
+
+ # Open the LUKS devices. This creates nodes like /dev/mapper/*-luks.
+ $g->cryptsetup_open ('/dev/VG/Root', 'FEDORA-Root',
'Root-luks');
+ $g->cryptsetup_open ('/dev/VG/LV1', 'FEDORA-LV1',
'LV1-luks');
+ $g->cryptsetup_open ('/dev/VG/LV2', 'FEDORA-LV2',
'LV2-luks');
+ $g->cryptsetup_open ('/dev/VG/LV3', 'FEDORA-LV3',
'LV3-luks');
+
+ # Phony root filesystem.
+ $g->mkfs ('ext2', '/dev/mapper/Root-luks', blocksize => 4096,
label => 'ROOT');
+ $g->set_uuid ('/dev/mapper/Root-luks',
'01234567-0123-0123-0123-012345678902');
+
+ # Other filesystems, just for testing findfs-label.
+ $g->mkfs ('ext2', '/dev/mapper/LV1-luks', blocksize => 4096,
label => 'LV1');
+ $g->mkfs ('ext2', '/dev/mapper/LV2-luks', blocksize => 1024,
label => 'LV2');
+ $g->mkfs ('ext2', '/dev/mapper/LV3-luks', blocksize => 2048,
label => 'LV3');
+
+ $g->mount ('/dev/mapper/Root-luks', '/');
+}
+
else {
print STDERR "$0: Unknown LAYOUT: ",$ENV{LAYOUT},"\n";
exit 1;
diff --git a/tests/luks/test-key-option-inspect-luks-on-lvm.sh
b/tests/luks/test-key-option-inspect-luks-on-lvm.sh
new file mode 100755
index 000000000000..bf8aa3b1ee6e
--- /dev/null
+++ b/tests/luks/test-key-option-inspect-luks-on-lvm.sh
@@ -0,0 +1,103 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2019-2022 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test the --key option. It is handled by common code so we only need
+# to test one tool (guestfish).
+
+set -e
+
+$TEST_FUNCTIONS
+skip_if_skipped
+skip_unless_feature_available luks
+skip_unless_phony_guest fedora-luks-on-lvm.img
+
+# Start a background guestfish instance to check key specification by Logical
+# Volume names.
+guestfish=(guestfish --listen --ro --inspector
+ --add ../test-data/phony-guests/fedora-luks-on-lvm.img)
+keys_by_lvname=(--key /dev/VG/Root:key:FEDORA-Root
+ --key /dev/VG/LV1:key:FEDORA-LV1
+ --key /dev/VG/LV2:key:FEDORA-LV2
+ --key /dev/VG/LV3:key:FEDORA-LV3)
+
+# The variable assignment below will fail, and abort the script, if guestfish
+# refuses to start up.
+fish_ref=$("${guestfish[@]}" "${keys_by_lvname[@]}")
+
+# Set GUESTFISH_PID as necessary for the remote access.
+eval "$fish_ref"
+
+# From this point on, if any remote guestfish command fails, that will cause
+# *both* the guestfish server *and* this script to exit, with an error.
+# However, we also want the background guestfish process to exit if (a) this
+# script exits cleanly, or (b) this script exits with a failure due to a reason
+# that's different from a failed remote command.
+function cleanup_guestfish
+{
+ if [ -n "$GUESTFISH_PID" ]; then
+ guestfish --remote -- exit >/dev/null 2>&1 || :
+ fi
+}
+trap cleanup_guestfish EXIT
+
+# Get the UUIDs of the LUKS devices.
+uuid_root=$(guestfish --remote -- luks-uuid /dev/VG/Root)
+uuid_lv1=$( guestfish --remote -- luks-uuid /dev/VG/LV1)
+uuid_lv2=$( guestfish --remote -- luks-uuid /dev/VG/LV2)
+uuid_lv3=$( guestfish --remote -- luks-uuid /dev/VG/LV3)
+
+# The actual test.
+function check_filesystems
+{
+ local decrypted_root decrypted_lv1 decrypted_lv2 decrypted_lv3 exists
+
+ # Get the names of the decrypted LUKS block devices that host the filesystems
+ # with the labels listed below.
+ decrypted_root=$(guestfish --remote -- findfs-label ROOT)
+ decrypted_lv1=$( guestfish --remote -- findfs-label LV1)
+ decrypted_lv2=$( guestfish --remote -- findfs-label LV2)
+ decrypted_lv3=$( guestfish --remote -- findfs-label LV3)
+
+ # Verify the device names. These come from decrypt_mountables() in
+ # "common/options/decrypt.c".
+ test /dev/mapper/luks-"$uuid_root" = "$decrypted_root"
+ test /dev/mapper/luks-"$uuid_lv1" = "$decrypted_lv1"
+ test /dev/mapper/luks-"$uuid_lv2" = "$decrypted_lv2"
+ test /dev/mapper/luks-"$uuid_lv3" = "$decrypted_lv3"
+
+ # Check "/etc/fedora-release" on the root fs.
+ exists=$(guestfish --remote -- exists /etc/fedora-release)
+ test true = "$exists"
+}
+check_filesystems
+
+# Exit the current guestfish background process.
+guestfish --remote -- exit
+GUESTFISH_PID=
+
+# Start up a similar guestfish background process, but specify the keys by
+# UUID.
+keys_by_uuid=(--key "$uuid_root":key:FEDORA-Root
+ --key "$uuid_lv1":key:FEDORA-LV1
+ --key "$uuid_lv2":key:FEDORA-LV2
+ --key "$uuid_lv3":key:FEDORA-LV3)
+fish_ref=$("${guestfish[@]}" "${keys_by_uuid[@]}")
+eval "$fish_ref"
+
+# Repeat the test.
+check_filesystems
--
2.19.1.3.g30247aa5d201
10:21 a.m.
New subject: [libguestfs PATCH 3/3] TODO: remove "Better support for encrypted devices"
LUKS support used to work best if the LUKS device resided on a partition,
and contained a Physical Volume for an LVM Volume Group. This scheme, also
called LVM-on-LUKS, is commonly created by installers of various Linux
distributions. (See RHBZ#1451665.)
Libguestfs now also supports the scheme wherein the LUKS device resides on
an LVM Logical Volume, and contains a filesystem. This is called
LUKS-on-LVM, it is the inverse of the above scheme, and is created by
installers of other Linux distributions. (See RHBZ#1658126.)
Both schemes are now decrypted by libguestfs-based utilities when
inspection is enabled (such as in "guestfish -i", virt-inspector,
virt-v2v), through the inspect_mount() function in utilities written in C,
and through the "inspect_decrypt" function in ones written in OCaml.
We don't seem to need an API like "list-luks-devices", as
"list-dm-devices" returns decrypted (i.e., opened) LUKS devices too; for
example, in the LUKS-on-LVM case:
><fs> list-dm-devices
/dev/mapper/luks-0d619854-ccd5-43b1-8883-991fec5ef713
/dev/mapper/luks-4e9e7a6f-a68c-42fd-92b4-8f4f2579a389
Thus, the subject TODO section is now out of date, and it's unclear what
remains "to do" there; let's just remove the section.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
TODO | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/TODO b/TODO
index 064386ac2dce..6fb11d5ecdd4 100644
--- a/TODO
+++ b/TODO
@@ -161,16 +161,6 @@ Could we make guestfish interactive if commands are used without
params?
Image name? disk.img
Size of image? 10M
-Better support for encrypted devices
-------------------------------------
-
-Currently LUKS support only works if the device contains volume
-groups. If it contains, eg., partitions, you cannot access them.
-We would like to add:
-
- - Direct access to the /dev/mapper device (eg. if it contains
- anything apart from VGs).
-
Display image as PS
-------------------
--
2.19.1.3.g30247aa5d201
4:46 a.m.
New subject: [libguestfs PATCH 3/3] TODO: remove "Better support for encrypted devices"
Series looks good, ACK
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
6:17 a.m.
New subject: [libguestfs PATCH 3/3] TODO: remove "Better support for encrypted devices"
On 02/24/22 11:46, Richard W.M. Jones wrote:
Series looks good, ACK
1 ed7e3462a252 Update common submodule
2 39a5bb6fda4d tests: rename "luks" to "lvm-on-luks"
3 322113314055 tests: add LUKS-on-LVM test
4 b6ef56187f6d TODO: remove "Better support for encrypted devices"
Thanks!
Laszlo
10:22 a.m.
New subject: [guestfs-tools PATCH 0/3] inspector: add LUKS-on-LVM test
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Laszlo Ersek (2):
inspector: rename "luks" to "lvm-on-luks"
inspector: add LUKS-on-LVM test
Richard W.M. Jones (1):
test-data: Replace deprecated luks_open with cryptsetup_open.
.gitignore | 3 +-
inspector/Makefile.am | 11
++--
inspector/{expected-fedora-luks.img.xml => expected-fedora-lvm-on-luks.img.xml} | 0
inspector/{expected-fedora.img.xml => expected-fedora-luks-on-lvm.img.xml} | 6
+-
inspector/{test-virt-inspector-luks.sh => test-virt-inspector-luks-on-lvm.sh} | 15
+++--
inspector/{test-virt-inspector-luks.sh => test-virt-inspector-lvm-on-luks.sh} | 2
+-
test-data/phony-guests/Makefile.am | 15
+++--
test-data/phony-guests/guests.xml.in | 22
++++++-
test-data/phony-guests/make-fedora-img.pl | 64
++++++++++++++++++--
9 files changed, 113 insertions(+), 25 deletions(-)
copy inspector/{expected-fedora.img.xml => expected-fedora-luks-on-lvm.img.xml} (98%)
copy inspector/{test-virt-inspector-luks.sh => test-virt-inspector-luks-on-lvm.sh}
(73%)
rename inspector/{expected-fedora-luks.img.xml => expected-fedora-lvm-on-luks.img.xml}
(100%)
rename inspector/{test-virt-inspector-luks.sh => test-virt-inspector-lvm-on-luks.sh}
(96%)
--
2.19.1.3.g30247aa5d201
10:22 a.m.
New subject: [guestfs-tools PATCH 1/3] test-data: Replace deprecated luks_open with cryptsetup_open.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
The two calls are identical, so this simply avoids a deprecation
warning.
(Picked from libguestfs commit 0b223a287711 ("test-data: Replace
deprecated luks_open with cryptsetup_open.", 2021-05-27), synchronizing
"make-fedora-img.pl" in guestfs-tools with the same file in libguestfs @
commit 0b223a287711.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
test-data/phony-guests/make-fedora-img.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index 90492b814e2b..27b154f9d71d 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -195,7 +195,7 @@ EOF
# Put LUKS on the second partition.
$g->luks_format ('/dev/sda2', 'FEDORA', 0);
- $g->luks_open ('/dev/sda2', 'FEDORA', 'luks');
+ $g->cryptsetup_open ('/dev/sda2', 'FEDORA', 'luks');
init_lvm_root ('/dev/mapper/luks');
}
--
2.19.1.3.g30247aa5d201
10:22 a.m.
New subject: [guestfs-tools PATCH 2/3] inspector: rename "luks" to "lvm-on-luks"
Clarify that our current usage of "luks" stands for "lvm-on-luks"
(IOW,
that the decrypted LUKS devices are Physical Volumes for LVM).
(This is a port of libguestfs patch 'tests: rename "luks" to
"lvm-on-luks"'.)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
inspector/Makefile.am | 8
++++----
test-data/phony-guests/Makefile.am | 8
++++----
.gitignore | 2 +-
inspector/{expected-fedora-luks.img.xml => expected-fedora-lvm-on-luks.img.xml} | 0
inspector/{test-virt-inspector-luks.sh => test-virt-inspector-lvm-on-luks.sh} | 2
+-
test-data/phony-guests/guests.xml.in | 4 ++--
test-data/phony-guests/make-fedora-img.pl | 8
++++----
7 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/inspector/Makefile.am b/inspector/Makefile.am
index 3c7cc17c98e3..961498e81788 100644
--- a/inspector/Makefile.am
+++ b/inspector/Makefile.am
@@ -27,13 +27,13 @@ example_xml = \
EXTRA_DIST = \
expected-debian.img.xml \
expected-fedora.img.xml \
- expected-fedora-luks.img.xml \
+ expected-fedora-lvm-on-luks.img.xml \
expected-ubuntu.img.xml \
expected-archlinux.img.xml \
expected-coreos.img.xml \
expected-windows.img.xml \
test-virt-inspector.sh \
- test-virt-inspector-luks.sh \
+ test-virt-inspector-lvm-on-luks.sh \
test-virt-inspector-docs.sh \
test-xmllint.sh \
virt-inspector.pod
@@ -95,14 +95,14 @@ TESTS_ENVIRONMENT = $(top_builddir)/run --test
TESTS = \
test-virt-inspector-docs.sh \
test-virt-inspector.sh \
- test-virt-inspector-luks.sh
+ test-virt-inspector-lvm-on-luks.sh
if HAVE_XMLLINT
TESTS += test-xmllint.sh
endif
check-valgrind:
- $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-luks.sh"
VG="@VG@" check
+ $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-lvm-on-luks.sh"
VG="@VG@" check
check-valgrind-local-guests:
for g in $(GUESTS); do \
diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am
index 60313548af32..0114d10bb170 100644
--- a/test-data/phony-guests/Makefile.am
+++ b/test-data/phony-guests/Makefile.am
@@ -49,7 +49,7 @@ disk_images = \
fedora-md1.img \
fedora-md2.img \
fedora-btrfs.img \
- fedora-luks.img \
+ fedora-lvm-on-luks.img \
ubuntu.img \
archlinux.img \
coreos.img \
@@ -96,11 +96,11 @@ fedora-btrfs.img: make-fedora-img.pl \
fedora.db
SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$<
-# Make a (dummy) Fedora image with LVM encrypted with LUKS.
-fedora-luks.img: make-fedora-img.pl \
+# Make a (dummy) Fedora image with LVM-on-LUKS.
+fedora-lvm-on-luks.img: make-fedora-img.pl \
fedora-journal.tar.xz \
fedora.db
- SRCDIR=$(srcdir) LAYOUT=lvm-luks $(top_builddir)/run --test ./$<
+ SRCDIR=$(srcdir) LAYOUT=lvm-on-luks $(top_builddir)/run --test ./$<
# Make a (dummy) Debian image.
debian.img: make-debian-img.sh
diff --git a/.gitignore b/.gitignore
index 6bc509faba32..5489c0cd2a5d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -151,7 +151,7 @@ Makefile.in
/test-data/phony-guests/debian.img
/test-data/phony-guests/fedora.img
/test-data/phony-guests/fedora-btrfs.img
-/test-data/phony-guests/fedora-luks.img
+/test-data/phony-guests/fedora-lvm-on-luks.img
/test-data/phony-guests/fedora-md1.img
/test-data/phony-guests/fedora-md2.img
/test-data/phony-guests/fedora.db
diff --git a/inspector/expected-fedora-luks.img.xml
b/inspector/expected-fedora-lvm-on-luks.img.xml
similarity index 100%
rename from inspector/expected-fedora-luks.img.xml
rename to inspector/expected-fedora-lvm-on-luks.img.xml
diff --git a/inspector/test-virt-inspector-luks.sh
b/inspector/test-virt-inspector-lvm-on-luks.sh
similarity index 96%
rename from inspector/test-virt-inspector-luks.sh
rename to inspector/test-virt-inspector-lvm-on-luks.sh
index 79adb1a9619f..955ac6316946 100755
--- a/inspector/test-virt-inspector-luks.sh
+++ b/inspector/test-virt-inspector-lvm-on-luks.sh
@@ -27,7 +27,7 @@ set -x
$TEST_FUNCTIONS
skip_if_skipped
-f=../test-data/phony-guests/fedora-luks.img
+f=../test-data/phony-guests/fedora-lvm-on-luks.img
# Ignore zero-sized file.
if [ -s "$f" ]; then
diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in
index 4139d04f689f..3af6b27af073 100644
--- a/test-data/phony-guests/guests.xml.in
+++ b/test-data/phony-guests/guests.xml.in
@@ -185,7 +185,7 @@
<!-- LUKS password is 'FEDORA' -->
<domain type='test'>
- <name>fedora-luks</name>
+ <name>fedora-lvm-on-luks</name>
<memory>1048576</memory>
<os>
<type>hvm</type>
@@ -194,7 +194,7 @@
<devices>
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
- <source file='@abs_builddir(a)/fedora-luks.img'/>
+ <source file='@abs_builddir(a)/fedora-lvm-on-luks.img'/>
<target dev='vda' bus='virtio'/>
</disk>
</devices>
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index 27b154f9d71d..4cd6ef9575eb 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -171,8 +171,8 @@ EOF
}
}
-elsif ($ENV{LAYOUT} eq 'lvm-luks') {
- push (@images, "fedora-luks.img-t");
+elsif ($ENV{LAYOUT} eq 'lvm-on-luks') {
+ push (@images, "fedora-lvm-on-luks.img-t");
open (my $fstab, '>', "fedora.fstab") or die;
print $fstab <<EOF;
@@ -183,9 +183,9 @@ EOF
$bootdev = '/dev/sda1';
- $g->disk_create ("fedora-luks.img-t", "raw", $IMAGE_SIZE);
+ $g->disk_create ("fedora-lvm-on-luks.img-t", "raw",
$IMAGE_SIZE);
- $g->add_drive ("fedora-luks.img-t", format => "raw");
+ $g->add_drive ("fedora-lvm-on-luks.img-t", format =>
"raw");
$g->launch ();
$g->part_init ('/dev/sda', 'mbr');
--
2.19.1.3.g30247aa5d201
10:22 a.m.
New subject: [guestfs-tools PATCH 3/3] inspector: add LUKS-on-LVM test
Port libguestfs patch 'tests: add LUKS-on-LVM test' to virt-inspector.
(This patch is best formatted with "--find-copies-harder".)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
inspector/Makefile.am | 5
+-
test-data/phony-guests/Makefile.am | 7
+++
.gitignore | 1
+
inspector/{expected-fedora.img.xml => expected-fedora-luks-on-lvm.img.xml} |
6 +--
inspector/{test-virt-inspector-lvm-on-luks.sh => test-virt-inspector-luks-on-lvm.sh} |
15 ++++--
test-data/phony-guests/guests.xml.in | 18
+++++++
test-data/phony-guests/make-fedora-img.pl | 54
++++++++++++++++++++
7 files changed, 97 insertions(+), 9 deletions(-)
diff --git a/inspector/Makefile.am b/inspector/Makefile.am
index 961498e81788..297887bae3a4 100644
--- a/inspector/Makefile.am
+++ b/inspector/Makefile.am
@@ -27,12 +27,14 @@ example_xml = \
EXTRA_DIST = \
expected-debian.img.xml \
expected-fedora.img.xml \
+ expected-fedora-luks-on-lvm.img.xml \
expected-fedora-lvm-on-luks.img.xml \
expected-ubuntu.img.xml \
expected-archlinux.img.xml \
expected-coreos.img.xml \
expected-windows.img.xml \
test-virt-inspector.sh \
+ test-virt-inspector-luks-on-lvm.sh \
test-virt-inspector-lvm-on-luks.sh \
test-virt-inspector-docs.sh \
test-xmllint.sh \
@@ -95,6 +97,7 @@ TESTS_ENVIRONMENT = $(top_builddir)/run --test
TESTS = \
test-virt-inspector-docs.sh \
test-virt-inspector.sh \
+ test-virt-inspector-luks-on-lvm.sh \
test-virt-inspector-lvm-on-luks.sh
if HAVE_XMLLINT
@@ -102,7 +105,7 @@ TESTS += test-xmllint.sh
endif
check-valgrind:
- $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-lvm-on-luks.sh"
VG="@VG@" check
+ $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-luks-on-lvm.sh
test-virt-inspector-lvm-on-luks.sh" VG="@VG@" check
check-valgrind-local-guests:
for g in $(GUESTS); do \
diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am
index 0114d10bb170..0a41f168bdfc 100644
--- a/test-data/phony-guests/Makefile.am
+++ b/test-data/phony-guests/Makefile.am
@@ -49,6 +49,7 @@ disk_images = \
fedora-md1.img \
fedora-md2.img \
fedora-btrfs.img \
+ fedora-luks-on-lvm.img \
fedora-lvm-on-luks.img \
ubuntu.img \
archlinux.img \
@@ -96,6 +97,12 @@ fedora-btrfs.img: make-fedora-img.pl \
fedora.db
SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$<
+# Make a (dummy) Fedora image with LUKS-on-LVM.
+fedora-luks-on-lvm.img: make-fedora-img.pl \
+ fedora-journal.tar.xz \
+ fedora.db
+ SRCDIR=$(srcdir) LAYOUT=luks-on-lvm $(top_builddir)/run --test ./$<
+
# Make a (dummy) Fedora image with LVM-on-LUKS.
fedora-lvm-on-luks.img: make-fedora-img.pl \
fedora-journal.tar.xz \
diff --git a/.gitignore b/.gitignore
index 5489c0cd2a5d..87040a727a7c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -151,6 +151,7 @@ Makefile.in
/test-data/phony-guests/debian.img
/test-data/phony-guests/fedora.img
/test-data/phony-guests/fedora-btrfs.img
+/test-data/phony-guests/fedora-luks-on-lvm.img
/test-data/phony-guests/fedora-lvm-on-luks.img
/test-data/phony-guests/fedora-md1.img
/test-data/phony-guests/fedora-md2.img
diff --git a/inspector/expected-fedora.img.xml
b/inspector/expected-fedora-luks-on-lvm.img.xml
similarity index 98%
copy from inspector/expected-fedora.img.xml
copy to inspector/expected-fedora-luks-on-lvm.img.xml
index 72cddaf8816d..6e21591d8066 100644
--- a/inspector/expected-fedora.img.xml
+++ b/inspector/expected-fedora-luks-on-lvm.img.xml
@@ -1,7 +1,7 @@
<?xml version="1.0"?>
<operatingsystems>
<operatingsystem>
- <root>/dev/VG/Root</root>
+ <root>/dev/mapper/luks-ROOTUUID</root>
<name>linux</name>
<arch>x86_64</arch>
<distro>fedora</distro>
@@ -13,11 +13,11 @@
<hostname>fedora.invalid</hostname>
<osinfo>fedora14</osinfo>
<mountpoints>
- <mountpoint dev="/dev/VG/Root">/</mountpoint>
+ <mountpoint dev="/dev/mapper/luks-ROOTUUID">/</mountpoint>
<mountpoint dev="/dev/sda1">/boot</mountpoint>
</mountpoints>
<filesystems>
- <filesystem dev="/dev/VG/Root">
+ <filesystem dev="/dev/mapper/luks-ROOTUUID">
<type>ext2</type>
<label>ROOT</label>
<uuid>01234567-0123-0123-0123-012345678902</uuid>
diff --git a/inspector/test-virt-inspector-lvm-on-luks.sh
b/inspector/test-virt-inspector-luks-on-lvm.sh
similarity index 73%
copy from inspector/test-virt-inspector-lvm-on-luks.sh
copy to inspector/test-virt-inspector-luks-on-lvm.sh
index 955ac6316946..b2e10232c05d 100755
--- a/inspector/test-virt-inspector-lvm-on-luks.sh
+++ b/inspector/test-virt-inspector-luks-on-lvm.sh
@@ -19,7 +19,7 @@
# Test that virt-inspector can work on encrypted images when the
# right password is supplied.
#
-# Regression test for https://bugzilla.redhat.com/show_bug.cgi?id=1602353
+# Regression test for https://bugzilla.redhat.com/show_bug.cgi?id=1658126
set -e
set -x
@@ -27,16 +27,21 @@ set -x
$TEST_FUNCTIONS
skip_if_skipped
-f=../test-data/phony-guests/fedora-lvm-on-luks.img
+f=../test-data/phony-guests/fedora-luks-on-lvm.img
+keys=(--key /dev/VG/Root:key:FEDORA-Root
+ --key /dev/VG/LV1:key:FEDORA-LV1
+ --key /dev/VG/LV2:key:FEDORA-LV2
+ --key /dev/VG/LV3:key:FEDORA-LV3)
# Ignore zero-sized file.
if [ -s "$f" ]; then
+ uuid_root=$(guestfish --ro -i -a "$f" "${keys[@]}" luks-uuid
/dev/VG/Root)
b=$(basename "$f")
- echo FEDORA |
- $VG virt-inspector --keys-from-stdin --format=raw -a "$f" >
"actual-$b.xml"
+ $VG virt-inspector "${keys[@]}" --format=raw -a "$f" >
"actual-$b.xml"
# Check the generated output validate the schema.
$XMLLINT --noout --relaxng "$srcdir/virt-inspector.rng"
"actual-$b.xml"
# This 'diff' command will fail (because of -e option) if there
# are any differences.
- diff -ur $diff_ignore "$srcdir/expected-$b.xml" "actual-$b.xml"
+ sed -e "s/ROOTUUID/$uuid_root/" < "$srcdir/expected-$b.xml" \
+ | diff -u - "actual-$b.xml"
fi
diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in
index 3af6b27af073..dfcccc3d4cff 100644
--- a/test-data/phony-guests/guests.xml.in
+++ b/test-data/phony-guests/guests.xml.in
@@ -183,6 +183,24 @@
</devices>
</domain>
+ <!-- LUKS passwords are 'FEDORA-Root', 'FEDORA-LV1',
'FEDORA-LV2',
+ 'FEDORA-LV3' -->
+ <domain type='test'>
+ <name>fedora-luks-on-lvm</name>
+ <memory>1048576</memory>
+ <os>
+ <type>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <devices>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source file='@abs_builddir(a)/fedora-luks-on-lvm.img'/>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ </devices>
+ </domain>
+
<!-- LUKS password is 'FEDORA' -->
<domain type='test'>
<name>fedora-lvm-on-luks</name>
diff --git a/test-data/phony-guests/make-fedora-img.pl
b/test-data/phony-guests/make-fedora-img.pl
index 4cd6ef9575eb..84d4742e9000 100755
--- a/test-data/phony-guests/make-fedora-img.pl
+++ b/test-data/phony-guests/make-fedora-img.pl
@@ -200,6 +200,60 @@ EOF
init_lvm_root ('/dev/mapper/luks');
}
+elsif ($ENV{LAYOUT} eq 'luks-on-lvm') {
+ push (@images, "fedora-luks-on-lvm.img-t");
+
+ open (my $fstab, '>', "fedora.fstab") or die;
+ print $fstab <<EOF;
+LABEL=BOOT /boot ext2 default 0 0
+LABEL=ROOT / ext2 default 0 0
+EOF
+ close ($fstab) or die;
+
+ $bootdev = '/dev/sda1';
+
+ $g->disk_create ("fedora-luks-on-lvm.img-t", "raw",
$IMAGE_SIZE);
+
+ $g->add_drive ("fedora-luks-on-lvm.img-t", format =>
"raw");
+ $g->launch ();
+
+ $g->part_init ('/dev/sda', 'mbr');
+ foreach my $p (@PARTITIONS) {
+ $g->part_add('/dev/sda', @$p);
+ }
+
+ # Create the Volume Group on /dev/sda2.
+ $g->pvcreate ('/dev/sda2');
+ $g->vgcreate ('VG', ['/dev/sda2']);
+ $g->lvcreate ('Root', 'VG', 32);
+ $g->lvcreate ('LV1', 'VG', 32);
+ $g->lvcreate ('LV2', 'VG', 32);
+ $g->lvcreate ('LV3', 'VG', 64);
+
+ # Format each Logical Group as a LUKS device, with a different password.
+ $g->luks_format ('/dev/VG/Root', 'FEDORA-Root', 0);
+ $g->luks_format ('/dev/VG/LV1', 'FEDORA-LV1', 0);
+ $g->luks_format ('/dev/VG/LV2', 'FEDORA-LV2', 0);
+ $g->luks_format ('/dev/VG/LV3', 'FEDORA-LV3', 0);
+
+ # Open the LUKS devices. This creates nodes like /dev/mapper/*-luks.
+ $g->cryptsetup_open ('/dev/VG/Root', 'FEDORA-Root',
'Root-luks');
+ $g->cryptsetup_open ('/dev/VG/LV1', 'FEDORA-LV1',
'LV1-luks');
+ $g->cryptsetup_open ('/dev/VG/LV2', 'FEDORA-LV2',
'LV2-luks');
+ $g->cryptsetup_open ('/dev/VG/LV3', 'FEDORA-LV3',
'LV3-luks');
+
+ # Phony root filesystem.
+ $g->mkfs ('ext2', '/dev/mapper/Root-luks', blocksize => 4096,
label => 'ROOT');
+ $g->set_uuid ('/dev/mapper/Root-luks',
'01234567-0123-0123-0123-012345678902');
+
+ # Other filesystems, just for testing findfs-label.
+ $g->mkfs ('ext2', '/dev/mapper/LV1-luks', blocksize => 4096,
label => 'LV1');
+ $g->mkfs ('ext2', '/dev/mapper/LV2-luks', blocksize => 1024,
label => 'LV2');
+ $g->mkfs ('ext2', '/dev/mapper/LV3-luks', blocksize => 2048,
label => 'LV3');
+
+ $g->mount ('/dev/mapper/Root-luks', '/');
+}
+
else {
print STDERR "$0: Unknown LAYOUT: ",$ENV{LAYOUT},"\n";
exit 1;
--
2.19.1.3.g30247aa5d201
4:47 a.m.
New subject: [guestfs-tools PATCH 3/3] inspector: add LUKS-on-LVM test
Also this series looks good, ACK
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
6:35 a.m.
New subject: [guestfs-tools PATCH 3/3] inspector: add LUKS-on-LVM test
On 02/24/22 11:47, Richard W.M. Jones wrote:
Also this series looks good, ACK
1 63e157f18793 Update common submodule
2 9a27f19269f5 test-data: Replace deprecated luks_open with cryptsetup_open.
3 8f2bbc3d50d8 inspector: rename "luks" to "lvm-on-luks"
4 27da4b0c4991 inspector: add LUKS-on-LVM test
Thanks!
Laszlo
10:23 a.m.
New subject: [v2v PATCH] convert_linux: include the "xts" module in the initrd
The conversion may change the CPU model of the domain, which in turn can
lead to the disappearance of various CPU flags/features, such as "aes"
(the AES-NI instruction set).
In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on
the "xts" module, whereas the built-in, non-accelerated AES module does
depend on the (external) "xts" module.
As a result of the conversion, the target domain may therefore become
dependent on the "xts" module, for decrypting LUKS block devices that
employ the "aes-xts-plain64" cipher (which is the default LUKS cipher
nowadays). When dracut runs in the appliance (in my case:
dracut-055-6.fc35.x86_64), it does not seem to notice this dependency.
Thus we need to explicitly include "xts" in the rebuilt initrd, if the
original kernel provides that module.
https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
convert/convert_linux.ml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 45ce069ad9bf..116d9e06f792 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
| None -> ()
| Some initrd ->
(* Enable the basic virtio modules in the kernel. *)
+ (* Also forcibly include the "xts" module; see RHBZ#1658126. *)
let modules =
let modules =
(* The order of modules here is deliberately the same as the
@@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
*)
List.filter (fun m -> List.mem m kernel.ki_modules)
[ "virtio"; "virtio_ring";
"virtio_blk";
- "virtio_scsi"; "virtio_net";
"virtio_pci" ] in
+ "virtio_scsi"; "virtio_net";
"virtio_pci"; "xts" ] in
if modules <> [] then modules
else
(* Fallback copied from old virt-v2v. XXX Why not "ide"? *)
--
2.19.1.3.g30247aa5d201
4:48 a.m.
New subject: [v2v PATCH] convert_linux: include the "xts" module in the initrd
On Wed, Feb 23, 2022 at 05:23:08PM +0100, Laszlo Ersek wrote:
The conversion may change the CPU model of the domain, which in turn
can
lead to the disappearance of various CPU flags/features, such as "aes"
(the AES-NI instruction set).
In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on
the "xts" module, whereas the built-in, non-accelerated AES module does
depend on the (external) "xts" module.
As a result of the conversion, the target domain may therefore become
dependent on the "xts" module, for decrypting LUKS block devices that
employ the "aes-xts-plain64" cipher (which is the default LUKS cipher
nowadays). When dracut runs in the appliance (in my case:
dracut-055-6.fc35.x86_64), it does not seem to notice this dependency.
Thus we need to explicitly include "xts" in the rebuilt initrd, if the
original kernel provides that module.
https://bugzilla.redhat.com/show_bug.cgi?id=1658126
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
convert/convert_linux.ml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 45ce069ad9bf..116d9e06f792 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
| None -> ()
| Some initrd ->
(* Enable the basic virtio modules in the kernel. *)
+ (* Also forcibly include the "xts" module; see RHBZ#1658126. *)
let modules =
let modules =
(* The order of modules here is deliberately the same as the
@@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
*)
List.filter (fun m -> List.mem m kernel.ki_modules)
[ "virtio"; "virtio_ring";
"virtio_blk";
- "virtio_scsi"; "virtio_net";
"virtio_pci" ] in
+ "virtio_scsi"; "virtio_net";
"virtio_pci"; "xts" ] in
if modules <> [] then modules
else
(* Fallback copied from old virt-v2v. XXX Why not "ide"? *)
--
2.19.1.3.g30247aa5d201
ACK (-ed already)
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
6:13 a.m.
New subject: [v2v PATCH] convert_linux: include the "xts" module in the initrd
On 02/24/22 11:48, Richard W.M. Jones wrote:
On Wed, Feb 23, 2022 at 05:23:08PM +0100, Laszlo Ersek wrote:
> The conversion may change the CPU model of the domain, which in turn can
> lead to the disappearance of various CPU flags/features, such as "aes"
> (the AES-NI instruction set).
>
> In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on
> the "xts" module, whereas the built-in, non-accelerated AES module does
> depend on the (external) "xts" module.
>
> As a result of the conversion, the target domain may therefore become
> dependent on the "xts" module, for decrypting LUKS block devices that
> employ the "aes-xts-plain64" cipher (which is the default LUKS cipher
> nowadays). When dracut runs in the appliance (in my case:
> dracut-055-6.fc35.x86_64), it does not seem to notice this dependency.
> Thus we need to explicitly include "xts" in the rebuilt initrd, if the
> original kernel provides that module.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1658126
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
> convert/convert_linux.ml | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index 45ce069ad9bf..116d9e06f792 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
> | None -> ()
> | Some initrd ->
> (* Enable the basic virtio modules in the kernel. *)
> + (* Also forcibly include the "xts" module; see RHBZ#1658126. *)
> let modules =
> let modules =
> (* The order of modules here is deliberately the same as the
> @@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
> *)
> List.filter (fun m -> List.mem m kernel.ki_modules)
> [ "virtio"; "virtio_ring";
"virtio_blk";
> - "virtio_scsi"; "virtio_net";
"virtio_pci" ] in
> + "virtio_scsi"; "virtio_net";
"virtio_pci"; "xts" ] in
> if modules <> [] then modules
> else
> (* Fallback copied from old virt-v2v. XXX Why not "ide"? *)
> --
> 2.19.1.3.g30247aa5d201
ACK (-ed already)
Yes (thanks!), I just didn't want to carry that forward from a bugzilla
comment :)
Thanks,
Laszlo
Rich.
6:43 a.m.
New subject: [v2v PATCH] convert_linux: include the "xts" module in the initrd
On 02/24/22 11:48, Richard W.M. Jones wrote:
On Wed, Feb 23, 2022 at 05:23:08PM +0100, Laszlo Ersek wrote:
> The conversion may change the CPU model of the domain, which in turn can
> lead to the disappearance of various CPU flags/features, such as "aes"
> (the AES-NI instruction set).
>
> In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on
> the "xts" module, whereas the built-in, non-accelerated AES module does
> depend on the (external) "xts" module.
>
> As a result of the conversion, the target domain may therefore become
> dependent on the "xts" module, for decrypting LUKS block devices that
> employ the "aes-xts-plain64" cipher (which is the default LUKS cipher
> nowadays). When dracut runs in the appliance (in my case:
> dracut-055-6.fc35.x86_64), it does not seem to notice this dependency.
> Thus we need to explicitly include "xts" in the rebuilt initrd, if the
> original kernel provides that module.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1658126
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
> convert/convert_linux.ml | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index 45ce069ad9bf..116d9e06f792 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
> | None -> ()
> | Some initrd ->
> (* Enable the basic virtio modules in the kernel. *)
> + (* Also forcibly include the "xts" module; see RHBZ#1658126. *)
> let modules =
> let modules =
> (* The order of modules here is deliberately the same as the
> @@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
> *)
> List.filter (fun m -> List.mem m kernel.ki_modules)
> [ "virtio"; "virtio_ring";
"virtio_blk";
> - "virtio_scsi"; "virtio_net";
"virtio_pci" ] in
> + "virtio_scsi"; "virtio_net";
"virtio_pci"; "xts" ] in
> if modules <> [] then modules
> else
> (* Fallback copied from old virt-v2v. XXX Why not "ide"? *)
> --
> 2.19.1.3.g30247aa5d201
ACK (-ed already)
1 8ab4493faab8 Update common submodule
2 6711106145f9 convert_linux: include the "xts" module in the initrd
Thanks!
Laszlo
998
days inactive
1003
days old
26 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones