[PATCH common v2 0/4] Windows BitLocker support.

[PATCH v3 0/8] Windows BitLocker...

[PATCH libnbd] info: Write output...

Richard W.M. Jones

Monday, 7 September 2020 Mon, 7 Sep '20

4:41 a.m.

For links to the original patch series, see: https://bugzilla.redhat.com/show_bug.cgi?id=1808977#c8 The original feedback was that ignoring errors from guestfs_luks_uuid would ignore legitimate errors from non-BitLocker disks, so I have modified this series so that errors are only ignored in the BitLocker case. As noted in the 4th patch there is no actual error in the BitLocker case, cryptsetup luksUUID simply exits without printing anything. Rich.

Show replies by date

Richard W.M. Jones

Monday, 7 September Mon, 7 Sep

4:41 a.m.

New subject: [PATCH common v2 1/4] options: Use new cryptsetup-open API if available.

Fall back to luks-open if we're using libguestfs <= 1.43.1. --- options/decrypt.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/options/decrypt.c b/options/decrypt.c index 683cf5e..d868f70 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -97,11 +97,15 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) /* Try each key in turn. */ for (j = 0; keys[j] != NULL; ++j) { - /* XXX Should we call guestfs_luks_open_ro if readonly flag + /* 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; -- 2.27.0

Richard W.M. Jones

4:41 a.m.

New subject: [PATCH common v2 2/4] options: Use cryptX instead of luksX as the temporary name.

--- options/decrypt.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/options/decrypt.c b/options/decrypt.c index d868f70..45de5b2 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -38,18 +38,18 @@ /** * Make a LUKS map name from the partition name, - * eg. C<"/dev/vda2" =E<gt> "luksvda2"> + * eg. C<"/dev/vda2" =E<gt> "cryptvda2"> */ static void make_mapname (const char *device, char *mapname, size_t len) { size_t i = 0; - if (len < 5) + if (len < 6) abort (); - strcpy (mapname, "luks"); - mapname += 4; - len -= 4; + strcpy (mapname, "crypt"); + mapname += 5; + len -= 5; if (STRPREFIX (device, "/dev/")) i = 5; -- 2.27.0

Richard W.M. Jones

4:41 a.m.

New subject: [PATCH common v2 3/4] options: Support Windows BitLocker (RHBZ#1808977).

--- mltools/tools_utils.mli | 5 ++--- options/decrypt.c | 9 ++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/mltools/tools_utils.mli b/mltools/tools_utils.mli index 102abff..1d1ac8a 100644 --- a/mltools/tools_utils.mli +++ b/mltools/tools_utils.mli @@ -195,9 +195,8 @@ val is_btrfs_subvolume : Guestfs.guestfs -> string -> bool (** Checks if a filesystem is a btrfs subvolume. *) val inspect_decrypt : Guestfs.guestfs -> key_store -> unit -(** Simple implementation of decryption: look for any [crypto_LUKS] - partitions and decrypt them, then rescan for VGs. This only works - for Fedora whole-disk encryption. *) +(** Simple implementation of decryption: look for any encrypted + partitions and decrypt them, then rescan for VGs. *) val with_timeout : string -> int -> ?sleep:int -> (unit -> 'a option) -> 'a (** [with_timeout op timeout ?sleep fn] implements a timeout loop. diff --git a/options/decrypt.c b/options/decrypt.c index 45de5b2..8eb24bc 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -65,10 +65,8 @@ make_mapname (const char *device, char *mapname, size_t len) } /** - * Simple implementation of decryption: look for any C<crypto_LUKS> - * partitions and decrypt them, then rescan for VGs. This only works - * for Fedora whole-disk encryption. WIP to make this work for other - * encryption schemes. + * Simple implementation of decryption: look for any encrypted + * partitions and decrypt them, then rescan for VGs. */ void inspect_do_decrypt (guestfs_h *g, struct key_store *ks) @@ -82,7 +80,8 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) for (i = 0; partitions[i] != NULL; ++i) { CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]); - if (type && STREQ (type, "crypto_LUKS")) { + if (type && + (STREQ (type, "crypto_LUKS") || STREQ (type, "BitLocker"))) { char mapname[32]; make_mapname (partitions[i], mapname, sizeof mapname); -- 2.27.0

Richard W.M. Jones

4:41 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

...

<rescue> cryptsetup luksUUID /dev/sda2 <rescue> echo $?

1 Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788. --- options/decrypt.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/options/decrypt.c b/options/decrypt.c index 8eb24bc..6b1c0a8 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <libintl.h> #include <error.h> @@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) 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 = guestfs_luks_uuid (g, partitions[i]); + CLEANUP_FREE char *uuid; + if (!is_bitlocker) + uuid = guestfs_luks_uuid (g, partitions[i]); + else { + /* This may fail for Windows BitLocker disks because + * cryptsetup luksUUID cannot read a UUID (unclear if + * this is a limitation of the format or cryptsetup). + */ + guestfs_push_error_handler (g, NULL, NULL); + uuid = guestfs_luks_uuid (g, partitions[i]); + guestfs_pop_error_handler (g); + } #else const char *uuid = NULL; #endif -- 2.27.0

Martin Kletzander

Tuesday, 6 October Tue, 6 Oct

8:25 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote:

...

For BitLocker disks cryptsetup does not (yet? ever?) support reading UUIDs and this function will fail. This does not matter here so just ignore the error. Note there is no error message, cryptsetup simply returns with a bad exit code: ><rescue> cryptsetup luksUUID /dev/sda2 ><rescue> echo $? 1 Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788. --- options/decrypt.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/options/decrypt.c b/options/decrypt.c index 8eb24bc..6b1c0a8 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <libintl.h> #include <error.h> @@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) 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 = guestfs_luks_uuid (g, partitions[i]); + CLEANUP_FREE char *uuid; + if (!is_bitlocker) + uuid = guestfs_luks_uuid (g, partitions[i]); + else { + /* This may fail for Windows BitLocker disks because + * cryptsetup luksUUID cannot read a UUID (unclear if + * this is a limitation of the format or cryptsetup). + */ + guestfs_push_error_handler (g, NULL, NULL); + uuid = guestfs_luks_uuid (g, partitions[i]);

I have yet to look at the libguestfs patches, but I do not completely understand what is the reason for calling "guestfs_luks_uuid" when you know it will fail. Or is there a case when it might be useful to get the result? Other than that the series looks fine to me.

...

+ guestfs_pop_error_handler (g); + } #else const char *uuid = NULL; #endif -- 2.27.0 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs

Richard W.M. Jones

9:06 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Tue, Oct 06, 2020 at 03:25:20PM +0200, Martin Kletzander wrote:

...

On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote: >For BitLocker disks cryptsetup does not (yet? ever?) support reading >UUIDs and this function will fail. This does not matter here so just >ignore the error. > >Note there is no error message, cryptsetup simply returns with a bad >exit code: > >><rescue> cryptsetup luksUUID /dev/sda2 >><rescue> echo $? >1 > >Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788. >--- >options/decrypt.c | 15 ++++++++++++++- >1 file changed, 14 insertions(+), 1 deletion(-) > >diff --git a/options/decrypt.c b/options/decrypt.c >index 8eb24bc..6b1c0a8 100644 >--- a/options/decrypt.c >+++ b/options/decrypt.c >@@ -25,6 +25,7 @@ > >#include <stdio.h> >#include <stdlib.h> >+#include <stdbool.h> >#include <string.h> >#include <libintl.h> >#include <error.h> >@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) > 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 = guestfs_luks_uuid (g, partitions[i]); >+ CLEANUP_FREE char *uuid; >+ if (!is_bitlocker) >+ uuid = guestfs_luks_uuid (g, partitions[i]); >+ else { >+ /* This may fail for Windows BitLocker disks because >+ * cryptsetup luksUUID cannot read a UUID (unclear if >+ * this is a limitation of the format or cryptsetup). >+ */ >+ guestfs_push_error_handler (g, NULL, NULL); >+ uuid = guestfs_luks_uuid (g, partitions[i]); I have yet to look at the libguestfs patches, but I do not completely understand what is the reason for calling "guestfs_luks_uuid" when you know it will fail. Or is there a case when it might be useful to get the result?

Yes I don't know what I was thinking there. It's difficult to revisit and revise patches months later. It would be better as simply: diff --git a/options/decrypt.c b/options/decrypt.c index 8eb24bc..ca2a0bb 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <libintl.h> #include <error.h> @@ -82,11 +83,19 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) 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 = guestfs_luks_uuid (g, partitions[i]); + 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 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

Martin Kletzander

Wednesday, 7 October Wed, 7 Oct

2:25 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Tue, Oct 06, 2020 at 03:06:54PM +0100, Richard W.M. Jones wrote:

...

On Tue, Oct 06, 2020 at 03:25:20PM +0200, Martin Kletzander wrote: > On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote: > >For BitLocker disks cryptsetup does not (yet? ever?) support reading > >UUIDs and this function will fail. This does not matter here so just > >ignore the error. > > > >Note there is no error message, cryptsetup simply returns with a bad > >exit code: > > > >><rescue> cryptsetup luksUUID /dev/sda2 > >><rescue> echo $? > >1 > > > >Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788. > >--- > >options/decrypt.c | 15 ++++++++++++++- > >1 file changed, 14 insertions(+), 1 deletion(-) > > > >diff --git a/options/decrypt.c b/options/decrypt.c > >index 8eb24bc..6b1c0a8 100644 > >--- a/options/decrypt.c > >+++ b/options/decrypt.c > >@@ -25,6 +25,7 @@ > > > >#include <stdio.h> > >#include <stdlib.h> > >+#include <stdbool.h> > >#include <string.h> > >#include <libintl.h> > >#include <error.h> > >@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) > > 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 = guestfs_luks_uuid (g, partitions[i]); > >+ CLEANUP_FREE char *uuid; > >+ if (!is_bitlocker) > >+ uuid = guestfs_luks_uuid (g, partitions[i]); > >+ else { > >+ /* This may fail for Windows BitLocker disks because > >+ * cryptsetup luksUUID cannot read a UUID (unclear if > >+ * this is a limitation of the format or cryptsetup). > >+ */ > >+ guestfs_push_error_handler (g, NULL, NULL); > >+ uuid = guestfs_luks_uuid (g, partitions[i]); > > I have yet to look at the libguestfs patches, but I do not completely understand > what is the reason for calling "guestfs_luks_uuid" when you know it will fail. > Or is there a case when it might be useful to get the result? Yes I don't know what I was thinking there. It's difficult to revisit and revise patches months later. It would be better as simply: diff --git a/options/decrypt.c b/options/decrypt.c index 8eb24bc..ca2a0bb 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <libintl.h> #include <error.h> @@ -82,11 +83,19 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) 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 = guestfs_luks_uuid (g, partitions[i]); + 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 Rich.

Glad I understood it then, ACK to that.

...

-- 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

Martin Kletzander

Friday, 9 October Fri, 9 Oct

7:29 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Wed, Oct 07, 2020 at 09:25:15AM +0200, Martin Kletzander wrote:

...

On Tue, Oct 06, 2020 at 03:06:54PM +0100, Richard W.M. Jones wrote: >On Tue, Oct 06, 2020 at 03:25:20PM +0200, Martin Kletzander wrote: >> On Mon, Sep 07, 2020 at 10:41:20AM +0100, Richard W.M. Jones wrote: >> >For BitLocker disks cryptsetup does not (yet? ever?) support reading >> >UUIDs and this function will fail. This does not matter here so just >> >ignore the error. >> > >> >Note there is no error message, cryptsetup simply returns with a bad >> >exit code: >> > >> >><rescue> cryptsetup luksUUID /dev/sda2 >> >><rescue> echo $? >> >1 >> > >> >Updates commit bb4a2dc17a78b53437896d4215ae82df8e11b788. >> >--- >> >options/decrypt.c | 15 ++++++++++++++- >> >1 file changed, 14 insertions(+), 1 deletion(-) >> > >> >diff --git a/options/decrypt.c b/options/decrypt.c >> >index 8eb24bc..6b1c0a8 100644 >> >--- a/options/decrypt.c >> >+++ b/options/decrypt.c >> >@@ -25,6 +25,7 @@ >> > >> >#include <stdio.h> >> >#include <stdlib.h> >> >+#include <stdbool.h> >> >#include <string.h> >> >#include <libintl.h> >> >#include <error.h> >> >@@ -82,11 +83,23 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) >> > 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 = guestfs_luks_uuid (g, partitions[i]); >> >+ CLEANUP_FREE char *uuid; >> >+ if (!is_bitlocker) >> >+ uuid = guestfs_luks_uuid (g, partitions[i]); >> >+ else { >> >+ /* This may fail for Windows BitLocker disks because >> >+ * cryptsetup luksUUID cannot read a UUID (unclear if >> >+ * this is a limitation of the format or cryptsetup). >> >+ */ >> >+ guestfs_push_error_handler (g, NULL, NULL); >> >+ uuid = guestfs_luks_uuid (g, partitions[i]); >> >> I have yet to look at the libguestfs patches, but I do not completely understand >> what is the reason for calling "guestfs_luks_uuid" when you know it will fail. >> Or is there a case when it might be useful to get the result? > >Yes I don't know what I was thinking there. It's difficult to revisit >and revise patches months later. It would be better as simply: > >diff --git a/options/decrypt.c b/options/decrypt.c >index 8eb24bc..ca2a0bb 100644 >--- a/options/decrypt.c >+++ b/options/decrypt.c >@@ -25,6 +25,7 @@ > > #include <stdio.h> > #include <stdlib.h> >+#include <stdbool.h> > #include <string.h> > #include <libintl.h> > #include <error.h> >@@ -82,11 +83,19 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks) > 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 = guestfs_luks_uuid (g, partitions[i]); >+ 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 > >Rich. > Glad I understood it then, ACK to that.

Oops, unfortunately that broke the php bindings =D But I'm guessing that again just needs a different include path. I only noticed it now when building Fedora 32 with all the build dependencies installed.

...

>-- >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

...

_______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs

Martin Kletzander

7:32 a.m.

New subject: [PATCH common v2 4/4] options: Ignore errors from guestfs_luks_uuid.

On Fri, Oct 09, 2020 at 02:29:02PM +0200, Martin Kletzander wrote:

...

Sorry, stupid me, this should have been a reply to the other thread related to the golang build breakage.

1504

days inactive

1536

days old

guestfs@lists.libguestfs.org

Manage subscription

9 comments

2 participants

tags (0)

participants (2)

Martin Kletzander
Richard W.M. Jones

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH common v2 0/4] Windows BitLocker support.