On Tue, Jun 28, 2022 at 01:49:04PM +0200, Laszlo Ersek wrote: A slightly better reproducer is this, since it doesn't require you to have an encrypted guest around: $ echo TEST | guestfish --keys-from-stdin -N part luks-format /dev/sda1 0 $ virt-cat $(seq -f '--key /dev/sda1:key:%g' 200) -a test1.img /no-such-file Segmentation fault (core dumped) $ rm test1.img Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; 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

On 06/29/22 13:50, Laszlo Ersek wrote: In order to get the CVE fix upstream reasonably quickly, I've pushed this one (--> commit 35467027f657), with the following commit message updates: 1: 35b49ce142fb ! 1: 35467027f657 options: fix buffer overflow in get_keys() [CVE-2022-2211] @@ -25,6 +25,14 @@ which passes 200 (different) passphrases for the LUKS-encrypted block device "/dev/sda2", crashes with a SIGSEGV. + A slightly better reproducer from Rich Jones is the following, since it + doesn't require an encrypted guest disk image: + + $ echo TEST | guestfish --keys-from-stdin -N part luks-format /dev/sda1 0 + $ virt-cat $(seq -f '--key /dev/sda1:key:%g' 200) -a test1.img /no-such-file + Segmentation fault (core dumped) + $ rm test1.img + ( The buffer overflow is possible to trigger in OCaml-language tools as @@ -71,6 +79,8 @@ Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2100862 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; + Message-Id: <20220628114915.5030-2-lersek(a)redhat.com&gt; + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; diff --git a/options/keys.c b/options/keys.c --- a/options/keys.c Thanks, Laszlo

On Tue, Jun 28, 2022 at 01:49:05PM +0200, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; 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

On Tue, Jun 28, 2022 at 01:49:06PM +0200, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- 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

On Tue, Jun 28, 2022 at 01:49:07PM +0200, Laszlo Ersek wrote: I was _going_ to say you don't need the parentheses here. Luckily I tested it before saying that, and it turns out you do! Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

Currently, get_keys() returns a NULL-terminated array of (char*) elements. In a later patch in this series, we'll want to change the element type to a structure type, at which point the NULL-termination would become awkward. Replace the NULL-termination scheme with the number-of-elements scheme. No behavioral changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- options/options.h | 4 +++- options/decrypt.c | 17 +++++++------- options/keys.c | 24 ++++++++++++++------ 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/options/options.h b/options/options.h index 80df91a85f98..d7a3aeff6f41 100644 --- a/options/options.h +++ b/options/options.h @@ -149,11 +149,13 @@ extern void inspect_mount_root (guestfs_h *g, const char *root); #define inspect_mount() inspect_mount_handle (g, ks) extern void print_inspect_prompt (void); /* in key.c */ extern char *read_key (const char *param); -extern char **get_keys (struct key_store *ks, const char *device, const char *uuid); +extern char **get_keys (struct key_store *ks, const char *device, const char *uuid, + size_t *nr_matches); +extern void free_keys (char **keys, size_t nr_matches); extern struct key_store *key_store_add_from_selector (struct key_store *ks, const char *selector); extern struct key_store *key_store_import_key (struct key_store *ks, const struct key_store_key *key); extern void free_key_store (struct key_store *ks); /* in options.c */ diff --git a/options/decrypt.c b/options/decrypt.c index 1cd7b627e264..c25b5888d8b4 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -122,14 +122,14 @@ decrypt_mountables (guestfs_h *g, const char * const *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 **keys; + size_t nr_matches; CLEANUP_FREE char *mapname = NULL; - const char * const *key_scan; - const char *key; + size_t scan; type = guestfs_vfs_type (g, mountable); if (type == NULL) continue; @@ -142,37 +142,38 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, 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); + keys = get_keys (ks, mountable, uuid, &nr_matches); + assert (nr_matches > 0); /* Generate a node name for the plaintext (decrypted) device node. */ if (uuid == NULL || asprintf (&mapname, "luks-%s", uuid) == -1) mapname = make_mapname (mountable); /* Try each key in turn. */ - key_scan = (const char * const *)keys; - while ((key = *key_scan++) != NULL) { + for (scan = 0; scan < nr_matches; ++scan) { + const char *key = keys[scan]; int r; guestfs_push_error_handler (g, NULL, NULL); r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1); guestfs_pop_error_handler (g); if (r == 0) break; } - if (key == NULL) + if (scan == nr_matches) 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)); + free_keys (keys, nr_matches); decrypted_some = true; } return decrypted_some; } diff --git a/options/keys.c b/options/keys.c index 8713372a305e..1d97d980a460 100644 --- a/options/keys.c +++ b/options/keys.c @@ -124,11 +124,12 @@ read_first_line_from_file (const char *filename) /* Return the key(s) matching this particular device from the * keystore. There may be multiple. If none are read from the * keystore, ask the user. */ char ** -get_keys (struct key_store *ks, const char *device, const char *uuid) +get_keys (struct key_store *ks, const char *device, const char *uuid, + size_t *nr_matches) { size_t i, j, nmemb; char **r; char *s; @@ -137,18 +138,16 @@ get_keys (struct key_store *ks, const char *device, const char *uuid) */ nmemb = 1; if (ks && ks->nr_keys > nmemb) nmemb = ks->nr_keys; - /* make room for the terminating NULL */ - if (nmemb == (size_t)-1) + if (nmemb > (size_t)-1 / sizeof *r) error (EXIT_FAILURE, 0, _("size_t overflow")); - nmemb++; - r = calloc (nmemb, sizeof (char *)); + r = malloc (nmemb * sizeof *r); if (r == NULL) - error (EXIT_FAILURE, errno, "calloc"); + error (EXIT_FAILURE, errno, "malloc"); j = 0; if (ks) { for (i = 0; i < ks->nr_keys; ++i) { @@ -175,16 +174,27 @@ get_keys (struct key_store *ks, const char *device, const char *uuid) if (j == 0) { /* Key not found in the key store, ask the user for it. */ s = read_key (device); if (!s) error (EXIT_FAILURE, 0, _("could not read key from user")); - r[0] = s; + r[j++] = s; } + *nr_matches = j; return r; } +void +free_keys (char **keys, size_t nr_matches) +{ + size_t i; + + for (i = 0; i < nr_matches; ++i) + free (keys[i]); + free (keys); +} + struct key_store * key_store_add_from_selector (struct key_store *ks, const char *selector) { CLEANUP_FREE_STRING_LIST char **fields = guestfs_int_split_string (':', selector); -- 2.19.1.3.g30247aa5d201

For adding Clevis support later in this patch set, replace the (char *) element type of the get_keys() result array with a new structure: "struct matching_key". No behavioral changes. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- options/options.h | 13 +++++++-- options/decrypt.c | 6 ++-- options/keys.c | 30 ++++++++++++-------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/options/options.h b/options/options.h index d7a3aeff6f41..9bd812525d8a 100644 --- a/options/options.h +++ b/options/options.h @@ -132,10 +132,17 @@ struct key_store_key { struct key_store { struct key_store_key *keys; size_t nr_keys; }; +/* A key matching a particular ID (pathname of the libguestfs device node that + * stands for the encrypted block device, or LUKS UUID). + */ +struct matching_key { + char *passphrase; +}; + /* in config.c */ extern void parse_config (void); /* in decrypt.c */ extern void inspect_do_decrypt (guestfs_h *g, struct key_store *ks); @@ -149,13 +156,13 @@ extern void inspect_mount_root (guestfs_h *g, const char *root); #define inspect_mount() inspect_mount_handle (g, ks) extern void print_inspect_prompt (void); /* in key.c */ extern char *read_key (const char *param); -extern char **get_keys (struct key_store *ks, const char *device, const char *uuid, - size_t *nr_matches); -extern void free_keys (char **keys, size_t nr_matches); +extern struct matching_key *get_keys (struct key_store *ks, const char *device, + const char *uuid, size_t *nr_matches); +extern void free_keys (struct matching_key *keys, size_t nr_matches); extern struct key_store *key_store_add_from_selector (struct key_store *ks, const char *selector); extern struct key_store *key_store_import_key (struct key_store *ks, const struct key_store_key *key); extern void free_key_store (struct key_store *ks); /* in options.c */ diff --git a/options/decrypt.c b/options/decrypt.c index c25b5888d8b4..421a38c2a11f 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -122,11 +122,11 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, const char *mountable; while ((mountable = *mnt_scan++) != NULL) { CLEANUP_FREE char *type = NULL; CLEANUP_FREE char *uuid = NULL; - char **keys; + struct matching_key *keys; size_t nr_matches; CLEANUP_FREE char *mapname = NULL; size_t scan; type = guestfs_vfs_type (g, mountable); @@ -151,15 +151,15 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, if (uuid == NULL || asprintf (&mapname, "luks-%s", uuid) == -1) mapname = make_mapname (mountable); /* Try each key in turn. */ for (scan = 0; scan < nr_matches; ++scan) { - const char *key = keys[scan]; + struct matching_key *key = keys + scan; int r; guestfs_push_error_handler (g, NULL, NULL); - r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1); + r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname, -1); guestfs_pop_error_handler (g); if (r == 0) break; } diff --git a/options/keys.c b/options/keys.c index 1d97d980a460..56fca17a94b5 100644 --- a/options/keys.c +++ b/options/keys.c @@ -123,16 +123,16 @@ read_first_line_from_file (const char *filename) /* Return the key(s) matching this particular device from the * keystore. There may be multiple. If none are read from the * keystore, ask the user. */ -char ** +struct matching_key * get_keys (struct key_store *ks, const char *device, const char *uuid, size_t *nr_matches) { - size_t i, j, nmemb; - char **r; + size_t i, nmemb; + struct matching_key *r, *match; char *s; /* We know the returned list must have at least one element and not * more than ks->nr_keys. */ @@ -145,11 +145,11 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, r = malloc (nmemb * sizeof *r); if (r == NULL) error (EXIT_FAILURE, errno, "malloc"); - j = 0; + match = r; if (ks) { for (i = 0; i < ks->nr_keys; ++i) { struct key_store_key *key = &ks->keys[i]; @@ -159,39 +159,45 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, switch (key->type) { case key_string: s = strdup (key->string.s); if (!s) error (EXIT_FAILURE, errno, "strdup"); - r[j++] = s; + match->passphrase = s; + ++match; break; case key_file: s = read_first_line_from_file (key->file.name); - r[j++] = s; + match->passphrase = s; + ++match; break; } } } - if (j == 0) { + if (match == r) { /* Key not found in the key store, ask the user for it. */ s = read_key (device); if (!s) error (EXIT_FAILURE, 0, _("could not read key from user")); - r[j++] = s; + match->passphrase = s; + ++match; } - *nr_matches = j; + *nr_matches = (size_t)(match - r); return r; } void -free_keys (char **keys, size_t nr_matches) +free_keys (struct matching_key *keys, size_t nr_matches) { size_t i; - for (i = 0; i < nr_matches; ++i) - free (keys[i]); + for (i = 0; i < nr_matches; ++i) { + struct matching_key *key = keys + i; + + free (key->passphrase); + } free (keys); } struct key_store * key_store_add_from_selector (struct key_store *ks, const char *selector) -- 2.19.1.3.g30247aa5d201

Extend the "matching_key" structure with a new boolean field, "clevis". "clevis" is mutually exclusive with a non-NULL "passphrase" field. If "clevis" is set, open the LUKS device with guestfs_clevis_luks_unlock() -- which requires no explicit passphrase --; otherwise, continue calling guestfs_cryptsetup_open(). This patch introduces no change in observable behavior; there is no user interface yet for setting the "clevis" field to "true". Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: This patch introduces a call to guestfs_clevis_luks_unlock(), which is a new libguestfs API (introduced in the sibling libguestfs series). Assuming we still care about building guestfs-tools and virt-v2v against an independent libguestfs (library and appliance), we need to do one of the following things: (1) Make the call dependent on the GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK feature test macro. If it is missing, the library is too old, just set "r = -1", and (possibly) print a warning to stderr. (2) Cut a new libguestfs release, and bump the minimum version in "m4/guestfs-libraries.m4" (in both guestfs-tools and virt-v2v) to the new version. Both of these have drawbacks. Disadvantages of (1): - Printing a raw warning to stderr is OK for the C-language tools, but the code is used by OCaml tools as well, which have pretty-printed warnings. - Simply skipping the guestfs_clevis_luks_unlock() call -- i.e., pretending it fails -- is not user-friendly. In particular, once we add the new selector type for "--key" later in this series, and document it in "options/key-option.pod", all the tools' docs will pick it up at once, even if the library is too old to provide the interface. Disadvantages of (2): - We may not want to leap over a large version distance in "m4/guestfs-libraries.m4" (in both guestfs-tools and virt-v2v), for whatever reason. - Even if we make the guestfs_clevis_luks_unlock() API mandatory in the library, the daemon may not implement it -- it's ultimately appliance (host distro) dependent, which is why the API belongs to a new option group called "clevisluks". That is, the actual behavior of guestfs_clevis_luks_unlock() may still differ from what the user expects based on "options/key-option.pod". This drawback is mitigated however: the documentation of the new selector in "options/key-option.pod" references "ENCRYPTED DISKS" in guestfs(3), which in turn references "guestfs_clevis_luks_unlock", which does highlight the "clevisluks" option group. I vote (2) -- cut a new libguestfs release, then bump the "m4/guestfs-libraries.m4" requirements. I'm not doing that just yet in those projects (in the sibling series here): if I did that, those series would not compile; the libguestfs version number would have to be increased first! And I don't know if we want *something else* in the new libguestfs release, additionally. options/options.h | 6 ++++++ options/decrypt.c | 7 ++++++- options/keys.c | 7 ++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/options/options.h b/options/options.h index 9bd812525d8a..61a385da13ae 100644 --- a/options/options.h +++ b/options/options.h @@ -136,10 +136,16 @@ struct key_store { /* A key matching a particular ID (pathname of the libguestfs device node that * stands for the encrypted block device, or LUKS UUID). */ struct matching_key { + /* True iff the passphrase should be reconstructed using Clevis, talking to + * Tang servers over the network. + */ + bool clevis; + + /* Explicit passphrase, otherwise. */ char *passphrase; }; /* in config.c */ extern void parse_config (void); diff --git a/options/decrypt.c b/options/decrypt.c index 421a38c2a11f..820fbc5629cd 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -155,11 +155,16 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, for (scan = 0; scan < nr_matches; ++scan) { struct matching_key *key = keys + scan; int r; guestfs_push_error_handler (g, NULL, NULL); - r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname, -1); + assert (key->clevis == (key->passphrase == NULL)); + if (key->clevis) + r = guestfs_clevis_luks_unlock (g, mountable, mapname); + else + r = guestfs_cryptsetup_open (g, mountable, key->passphrase, mapname, + -1); guestfs_pop_error_handler (g); if (r == 0) break; } diff --git a/options/keys.c b/options/keys.c index 56fca17a94b5..75c659561c52 100644 --- a/options/keys.c +++ b/options/keys.c @@ -159,15 +159,17 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, switch (key->type) { case key_string: s = strdup (key->string.s); if (!s) error (EXIT_FAILURE, errno, "strdup"); + match->clevis = false; match->passphrase = s; ++match; break; case key_file: s = read_first_line_from_file (key->file.name); + match->clevis = false; match->passphrase = s; ++match; break; } } @@ -176,10 +178,11 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, if (match == r) { /* Key not found in the key store, ask the user for it. */ s = read_key (device); if (!s) error (EXIT_FAILURE, 0, _("could not read key from user")); + match->clevis = false; match->passphrase = s; ++match; } *nr_matches = (size_t)(match - r); @@ -192,11 +195,13 @@ free_keys (struct matching_key *keys, size_t nr_matches) size_t i; for (i = 0; i < nr_matches; ++i) { struct matching_key *key = keys + i; - free (key->passphrase); + assert (key->clevis == (key->passphrase == NULL)); + if (!key->clevis) + free (key->passphrase); } free (keys); } struct key_store * -- 2.19.1.3.g30247aa5d201

On 06/28/22 16:33, Richard W.M. Jones wrote: How about this: - In this patch, if GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK is not defined, but "key->clevis" is set, then abort(). The idea being, the utilities should never permit (or cause) "key->clevis" to be set when the API is missing. The utilities need to catch the unsatisfiable request for clevis earlier. (In this patch, it is too late for reporting that kind of error!) - At the end of this series, rename "key_store_requires_network" to "key_store_contains_clevis". - Append another patch: introduce a C function that checks both GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK and guestfs_available("clevisluks"), and returns a bool. Add an OCaml wrapper too. (OCaml has g#available, but cannot check the GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK feature test macro!) - In the tools, don't just call key_store_contains_clevis before launching the appliance(s), for enabling networking. After launching the appliance, check the new function too (if "key_store_contains_clevis"), and exit then if the functionality is missing. Thanks, Laszlo

From an earlier patch in this series, we can now represent LUKS decryption with Clevis+Tang in those matching keys that we distill for a particular guestfs device or LUKS UUID. Now extend the keystore (composed from command line options), i.e. the store that is filtered into matching keys, with a selector type that stands for Clevis+Tang. Again, this patch introduces no change in observable behavior; there is still no user interface for placing a selector of the new type into the keystore. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- options/options.h | 1 + options/keys.c | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/options/options.h b/options/options.h index 61a385da13ae..e7a0364cc926 100644 --- a/options/options.h +++ b/options/options.h @@ -113,10 +113,11 @@ struct key_store_key { char *id; enum { key_string, /* key specified as string */ key_file, /* key stored in a file */ + key_clevis, /* key reconstructed with Clevis+Tang */ } type; union { struct { char *s; /* string of the key */ } string; diff --git a/options/keys.c b/options/keys.c index 75c659561c52..7729fe79c99b 100644 --- a/options/keys.c +++ b/options/keys.c @@ -169,10 +169,15 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, s = read_first_line_from_file (key->file.name); match->clevis = false; match->passphrase = s; ++match; break; + case key_clevis: + match->clevis = true; + match->passphrase = NULL; + ++match; + break; } } } if (match == r) { @@ -289,9 +294,12 @@ free_key_store (struct key_store *ks) free (key->string.s); break; case key_file: free (key->file.name); break; + case key_clevis: + /* nothing */ + break; } free (key->id); } } -- 2.19.1.3.g30247aa5d201

On 06/28/22 16:34, Richard W.M. Jones wrote: (I need to correct the "tpe" typo on the subject line.)

On Tue, Jun 28, 2022 at 01:49:12PM +0200, Laszlo Ersek wrote: This code isn't wrong, but there is a "fun" catch with __attribute__((cleanup)) to be aware of. It wasn't obvious to me until I'd hit this problem a few times. __attribute__((cleanup, free_strings)) char **fields; fields = something; is fine, but: __attribute__((cleanup, free_strings)) char **fields; /* Add some new code */ if (!check_something) return; fields = something; is very bad. It will call free_strings on an uninitialized stack value. That was probably the reason why the original code tries to initialize fields where it was defined. Now again, this is not wrong, as long as everyone is aware that a future code addition could break things. Libvirt style insists that such variables are always initialized to NULL, but we don't do that consistently. Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- 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

In another patch in this series, we generalize the "--key" selector parsing for C-language utilities. Adapt the OCaml-langauge "--key" parser: - Incorporate the new (more informative) error messages for consistency. - Prepare for selector types that do not take any type-specific parameters. These will be represented with constant constructors of the "key_store_key" type, and such values are not blocks, but unboxed integers: <https://v2.ocaml.org/manual/intfc.html#ss:c-concrete-datatypes>;. (This patch is best shown with "git show -b" for review.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- mltools/tools_utils.ml | 14 ++++++- mltools/tools_utils-c.c | 44 ++++++++++++-------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml index 6006ab7e4f6c..e534cbead47a 100644 --- a/mltools/tools_utils.ml +++ b/mltools/tools_utils.ml @@ -390,18 +390,28 @@ let create_standard_options argspec ?anon_fun ?(key_opts = false) L"colour"; L"colours" ], Getopt.Unit set_colours, s_"Use ANSI colour sequences even if not tty"); add_argspec ([ L"wrap" ], Getopt.Unit set_wrap, s_"Wrap log messages even if not tty"); if key_opts then ( let parse_key_selector arg = - let parts = String.nsplit ~max:3 ":" arg in + let parts = String.nsplit ":" arg in match parts with + | [] -> + error (f_"selector '%s': missing ID") arg + | [ _ ] -> + error (f_"selector '%s': missing TYPE") arg + | [ _; "key" ] + | _ :: "key" :: _ :: _ :: _ -> + error (f_"selector '%s': missing KEY_STRING, or too many fields") arg | [ device; "key"; key ] -> List.push_back ks.keys (device, KeyString key) + | [ _; "file" ] + | _ :: "file" :: _ :: _ :: _ -> + error (f_"selector '%s': missing FILENAME, or too many fields") arg | [ device; "file"; file ] -> List.push_back ks.keys (device, KeyFileName file) | _ -> - error (f_"invalid selector string for --key: %s") arg + error (f_"selector '%s': invalid TYPE") arg in add_argspec ([ L"echo-keys" ], Getopt.Unit c_set_echo_keys, s_"Don’t turn off echo for passphrases"); add_argspec ([ L"keys-from-stdin" ], Getopt.Unit c_set_keys_from_stdin, s_"Read passphrases from stdin"); add_argspec ([ L"key" ], Getopt.String (s_"SELECTOR", parse_key_selector), s_"Specify a LUKS key"); diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c index 081466776666..e9f273ec857f 100644 --- a/mltools/tools_utils-c.c +++ b/mltools/tools_utils-c.c @@ -60,28 +60,36 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv) key.id = strdup (String_val (Field (elemv, 0))); if (!key.id) caml_raise_out_of_memory (); v = Field (elemv, 1); - switch (Tag_val (v)) { - case 0: /* KeyString of string */ - key.type = key_string; - key.string.s = strdup (String_val (Field (v, 0))); - if (!key.string.s) - caml_raise_out_of_memory (); - break; - case 1: /* KeyFileName of string */ - key.type = key_file; - key.file.name = strdup (String_val (Field (v, 0))); - if (!key.file.name) - caml_raise_out_of_memory (); - break; - default: - error (EXIT_FAILURE, 0, - "internal error: unhandled Tag_val (v) = %d", - Tag_val (v)); - } + if (Is_block (v)) + switch (Tag_val (v)) { + case 0: /* KeyString of string */ + key.type = key_string; + key.string.s = strdup (String_val (Field (v, 0))); + if (!key.string.s) + caml_raise_out_of_memory (); + break; + case 1: /* KeyFileName of string */ + key.type = key_file; + key.file.name = strdup (String_val (Field (v, 0))); + if (!key.file.name) + caml_raise_out_of_memory (); + break; + default: + error (EXIT_FAILURE, 0, + "internal error: unhandled Tag_val (v) = %d", + Tag_val (v)); + } + else + switch (Int_val (v)) { + default: + error (EXIT_FAILURE, 0, + "internal error: unhandled Int_val (v) = %d", + Int_val (v)); + } ks = key_store_import_key (ks, &key); keysv = Field (keysv, 1); } -- 2.19.1.3.g30247aa5d201

On 06/28/22 16:47, Richard W.M. Jones wrote: Argh, yes. I make this typo too frequently. Ugh, yes it does -- but, regarding the C option parser, don't we have the same issue there already (pre-patch)? For example, "--key ID:key:foo:bar" causes guestfs_int_split_string() to return "foo" and "bar" separately, doesn't it? I think if a password contains a ":", the user just has to use a file selector. OK, I'll change the pre-patch default to abort() separately, and then stick with abort() in the new switch too. Thanks Laszlo

Provide the user interface (in both the C and the OCaml tools) for selecting network-based, passphrase-less decryption. This is the front-end exposing the previously added back-end. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- mltools/tools_utils.ml | 5 +++++ mltools/tools_utils-c.c | 3 +++ options/keys.c | 4 ++++ options/key-option.pod | 6 ++++++ 4 files changed, 18 insertions(+) diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml index e534cbead47a..1da5850340d4 100644 --- a/mltools/tools_utils.ml +++ b/mltools/tools_utils.ml @@ -32,10 +32,11 @@ type key_store = { keys : (string * key_store_key) list ref; } and key_store_key = | KeyString of string | KeyFileName of string + | KeyClevis external c_inspect_decrypt : Guestfs.t -> int64 -> (string * key_store_key) list -> unit = "guestfs_int_mllib_inspect_decrypt" external c_set_echo_keys : unit -> unit = "guestfs_int_mllib_set_echo_keys" [@@noalloc] external c_set_keys_from_stdin : unit -> unit = "guestfs_int_mllib_set_keys_from_stdin" [@@noalloc] external c_rfc3339_date_time_string : unit -> string = "guestfs_int_mllib_rfc3339_date_time_string" @@ -406,10 +407,14 @@ let create_standard_options argspec ?anon_fun ?(key_opts = false) | [ _; "file" ] | _ :: "file" :: _ :: _ :: _ -> error (f_"selector '%s': missing FILENAME, or too many fields") arg | [ device; "file"; file ] -> List.push_back ks.keys (device, KeyFileName file) + | _ :: "clevis" :: _ :: _ -> + error (f_"selector '%s': too many fields") arg + | [ device; "clevis" ] -> + List.push_back ks.keys (device, KeyClevis) | _ -> error (f_"selector '%s': invalid TYPE") arg in add_argspec ([ L"echo-keys" ], Getopt.Unit c_set_echo_keys, s_"Don’t turn off echo for passphrases"); diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c index e9f273ec857f..f429d7708772 100644 --- a/mltools/tools_utils-c.c +++ b/mltools/tools_utils-c.c @@ -81,10 +81,13 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv) "internal error: unhandled Tag_val (v) = %d", Tag_val (v)); } else switch (Int_val (v)) { + case 0: /* KeyClevis */ + key.type = key_clevis; + break; default: error (EXIT_FAILURE, 0, "internal error: unhandled Int_val (v) = %d", Int_val (v)); } diff --git a/options/keys.c b/options/keys.c index a6ef2d78b589..d53e3e774a9b 100644 --- a/options/keys.c +++ b/options/keys.c @@ -248,10 +248,14 @@ key_store_add_from_selector (struct key_store *ks, const char *selector) _("selector '%s': missing FILENAME, or too many fields"), selector); key.file.name = strdup (fields[2]); if (!key.file.name) error (EXIT_FAILURE, errno, "strdup"); + } else if (STREQ (fields[1], "clevis")) { + key.type = key_clevis; + if (field_count != 2) + error (EXIT_FAILURE, 0, _("selector '%s': too many fields"), selector); } else error (EXIT_FAILURE, 0, _("selector '%s': invalid TYPE"), selector); return key_store_import_key (ks, &key); } diff --git a/options/key-option.pod b/options/key-option.pod index 90a3b15c57a2..34229ce9cbb2 100644 --- a/options/key-option.pod +++ b/options/key-option.pod @@ -12,6 +12,12 @@ Use the specified C<KEY_STRING> as passphrase. =item B<--key> C<ID>:file:FILENAME Read the passphrase from F<FILENAME>. +=item B<--key> C<ID>:clevis + +Attempt passphrase-less unlocking for C<ID> with Clevis, over the +network. Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more +information on network-bound disk encryption (NBDE). + =back -- 2.19.1.3.g30247aa5d201

For "--key ID:clevis" to actually work, the appliance needs network connectivity. Some tools allow the user to control that (for example, guestfish takes "--network"); those tools can already use "--key ID:clevis", provided they enable inspection and networking. However, other tools have had no use for networking thus far. Introduce a helper function for the OCaml utils, and another for the C utils, so that the utils can query if there's at least one clevis selector, and if so, enable networking for the appliance, before launching the appliance. Note that we need to implement both helpers separately from each other; that is, the OCaml one is not based on the C one. The reason is that "struct key_store" (from "options/options.h"), upon which we could logically base a common helper, exists in the OCaml tools only ephemerally, in guestfs_int_mllib_inspect_decrypt(): inspect_decrypt [mltools/tools_utils.ml] c_inspect_decrypt [mltools/tools_utils.ml] guestfs_int_mllib_inspect_decrypt [mltools/tools_utils-c.c] key_store_import_key() [options/keys.c] inspect_do_decrypt() [options/decrypt.c] free_key_store() [options/keys.c] At that time, it's too late for enabling networking for the appliance. Therefore, in the OCaml tools, search the "earlier" data structure called "cmdline_options.ks". *That* "key_store" type comes from "mltools/tools_utils.ml". Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- options/options.h | 1 + mltools/tools_utils.mli | 4 ++++ mltools/tools_utils.ml | 5 +++++ options/keys.c | 15 +++++++++++++++ options/key-option.pod | 3 +++ 5 files changed, 28 insertions(+) diff --git a/options/options.h b/options/options.h index e7a0364cc926..60d5d8064113 100644 --- a/options/options.h +++ b/options/options.h @@ -168,10 +168,11 @@ extern char *read_key (const char *param); extern struct matching_key *get_keys (struct key_store *ks, const char *device, const char *uuid, size_t *nr_matches); extern void free_keys (struct matching_key *keys, size_t nr_matches); extern struct key_store *key_store_add_from_selector (struct key_store *ks, const char *selector); extern struct key_store *key_store_import_key (struct key_store *ks, const struct key_store_key *key); +extern bool key_store_requires_network (const struct key_store *ks); extern void free_key_store (struct key_store *ks); /* in options.c */ extern void option_a (const char *arg, const char *format, int blocksize, struct drv **drvsp); extern void option_d (const char *arg, struct drv **drvsp); diff --git a/mltools/tools_utils.mli b/mltools/tools_utils.mli index 8d9af7a0a661..ec900e6389bc 100644 --- a/mltools/tools_utils.mli +++ b/mltools/tools_utils.mli @@ -194,10 +194,14 @@ val inspect_mount_root_ro : Guestfs.guestfs -> string -> unit read-only. *) val is_btrfs_subvolume : Guestfs.guestfs -> string -> bool (** Checks if a filesystem is a btrfs subvolume. *) +val key_store_requires_network : key_store -> bool +(** [key_store_requires_network ks] returns [true] iff [ks] contains at least + one "ID:clevis" selector. *) + val inspect_decrypt : Guestfs.guestfs -> key_store -> unit (** 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 diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml index 1da5850340d4..562bfadc18a8 100644 --- a/mltools/tools_utils.ml +++ b/mltools/tools_utils.ml @@ -694,10 +694,15 @@ let is_btrfs_subvolume g fs = ignore (g#mountable_subvolume fs); true with Guestfs.Error msg as exn -> if g#last_errno () = Guestfs.Errno.errno_EINVAL then false else raise exn +let key_store_requires_network ks = + List.exists (function + | _, KeyClevis -> true + | _ -> false) !(ks.keys) + let inspect_decrypt g ks = (* Note we pass original 'g' even though it is not used by the * callee. This is so that 'g' is kept as a root on the stack, and * so cannot be garbage collected while we are in the c_inspect_decrypt * function. diff --git a/options/keys.c b/options/keys.c index d53e3e774a9b..97da6df93ee6 100644 --- a/options/keys.c +++ b/options/keys.c @@ -282,10 +282,25 @@ key_store_import_key (struct key_store *ks, const struct key_store_key *key) ++ks->nr_keys; return ks; } +bool +key_store_requires_network (const struct key_store *ks) +{ + size_t i; + + if (ks == NULL) + return false; + + for (i = 0; i < ks->nr_keys; ++i) + if (ks->keys[i].type == key_clevis) + return true; + + return false; +} + void free_key_store (struct key_store *ks) { size_t i; diff --git a/options/key-option.pod b/options/key-option.pod index 34229ce9cbb2..6bc04df177b1 100644 --- a/options/key-option.pod +++ b/options/key-option.pod @@ -18,6 +18,9 @@ Read the passphrase from F<FILENAME>. Attempt passphrase-less unlocking for C<ID> with Clevis, over the network. Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more information on network-bound disk encryption (NBDE). +Note that if any such option is present on the command line, QEMU user +networking will be automatically enabled for the libguestfs appliance. + =back -- 2.19.1.3.g30247aa5d201

On Tue, Jun 28, 2022 at 01:54:16PM +0200, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

Introduce a new guestfs API called "clevis_luks_unlock". At the libguestfs level, it is quite simple; it wraps the "clevis luks unlock" guest command (implemented by the "clevis-luks-unlock" executable, which is in fact a shell script). The complexity is instead in the network-based disk encryption (Clevis/Tang) scheme. Useful documentation: - https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/9/... - https://github.com/latchset/clevis#clevis - https://github.com/latchset/tang#tang The package providing "clevis-luks-unlock" is usually called "clevis-luks", occasionally "clevis". Some distros don't package clevis at all. Add the new API under a new option group (which may not be available) called "clevisluks". Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- daemon/Makefile.am | 1 + generator/actions_core.ml | 38 +++++++++++++ generator/proc_nr.ml | 1 + daemon/clevis-luks.c | 58 ++++++++++++++++++++ appliance/packagelist.in | 4 ++ lib/MAX_PROC_NR | 2 +- lib/guestfs.pod | 19 +++++-- 7 files changed, 118 insertions(+), 5 deletions(-) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index bbd49f9ea1e0..f50faecd6930 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -96,10 +96,11 @@ guestfsd_SOURCES = \ btrfs.c \ caml-stubs.c \ cap.c \ checksum.c \ cleanups.c \ + clevis-luks.c \ cmp.c \ command.c \ command.h \ compress.c \ copy.c \ diff --git a/generator/actions_core.ml b/generator/actions_core.ml index 807150615426..1b1fdcf77209 100644 --- a/generator/actions_core.ml +++ b/generator/actions_core.ml @@ -9718,6 +9718,44 @@ and I<not> the name of the underlying block device." }; style = RErr, [String (Pathname, "dir"); String (FileOut, "filename")], []; visibility = VInternal; shortdesc = "read directories entries"; longdesc = "Internal function for readdir." }; + { defaults with + name = "clevis_luks_unlock"; added = (1, 49, 3); + style = RErr, + [String (Device, "device"); String (PlainString, "mapname")], + []; + optional = Some "clevisluks"; + test_excuse = "needs networking and a configured Tang server"; + shortdesc = "open an encrypted LUKS block device with Clevis and Tang"; + longdesc = "\ +This command opens a block device that has been encrypted according to the Linux +Unified Key Setup (LUKS) standard, using network-bound disk encryption (NBDE). + +C<device> is the encrypted block device. + +The appliance will connect to the Tang servers noted in the tree of Clevis pins +that is bound to a keyslot of the LUKS header. The Clevis pin tree may comprise +C<sss> (redudancy) pins as internal nodes (optionally), and C<tang> pins as +leaves. C<tpm2> pins are not supported. The appliance unlocks the encrypted +block device by combining responses from the Tang servers with metadata from the +LUKS header; there is no C<key> parameter. + +This command will fail if networking has not been enabled for the appliance. +Refer to C<guestfs_set_network>. + +The command creates a new block device called F</dev/mapper/mapname>. Reads and +writes to this block device are decrypted from and encrypted to the underlying +C<device> respectively. Close the decrypted block device with +C<guestfs_cryptsetup_close>. + +C<mapname> cannot be C<\"control\"> because that name is reserved by +device-mapper. + +If this block device contains LVM volume groups, then calling +C<guestfs_lvm_scan> with the C<activate> parameter C<true> will make them +visible. + +Use C<guestfs_list_dm_devices> to list all device mapper devices." }; + ] diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml index bdced51c9acd..edd9bd99d2da 100644 --- a/generator/proc_nr.ml +++ b/generator/proc_nr.ml @@ -512,10 +512,11 @@ let proc_nr = [ 507, "luks_uuid"; 508, "cryptsetup_open"; 509, "cryptsetup_close"; 510, "internal_list_rpm_applications"; 511, "internal_readdir"; +512, "clevis_luks_unlock" ] (* End of list. If adding a new entry, add it at the end of the list * above. Do not modify anything below this line. *---------------------------------------------------------------------- diff --git a/daemon/clevis-luks.c b/daemon/clevis-luks.c new file mode 100644 index 000000000000..d3d970d78098 --- /dev/null +++ b/daemon/clevis-luks.c @@ -0,0 +1,58 @@ +/* libguestfs - the guestfsd daemon + * Copyright (C) 2009-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. + */ + +#include <config.h> + +#include "daemon.h" +#include "actions.h" +#include "optgroups.h" + +#define MAX_ARGS 8 + +int +optgroup_clevisluks_available (void) +{ + return prog_exists ("clevis-luks-unlock"); +} + +int +do_clevis_luks_unlock (const char *device, const char *mapname) +{ + const char *argv[MAX_ARGS]; + size_t i = 0; + int r; + CLEANUP_FREE char *err = NULL; + + ADD_ARG (argv, i, "clevis"); + ADD_ARG (argv, i, "luks"); + ADD_ARG (argv, i, "unlock"); + ADD_ARG (argv, i, "-d"); + ADD_ARG (argv, i, device); + ADD_ARG (argv, i, "-n"); + ADD_ARG (argv, i, mapname); + ADD_ARG (argv, i, NULL); + + r = commandv (NULL, &err, argv); + if (r == -1) { + reply_with_error ("%s: %s: %s", device, mapname, err); + return -1; + } + + udev_settle (); + return 0; +} diff --git a/appliance/packagelist.in b/appliance/packagelist.in index 77a07acc6ad0..0b79edcdd55d 100644 --- a/appliance/packagelist.in +++ b/appliance/packagelist.in @@ -21,10 +21,11 @@ dnl ./configure --with-extra-packages="gdb valgrind [etc]" dnl Basically the same with a few minor tweaks. ifelse(UBUNTU,1,`define(`DEBIAN',1)') ifelse(REDHAT,1, + clevis-luks cryptsetup cryptsetup-luks dnl old name used before Fedora 17 dhclient gfs-utils gfs2-utils @@ -51,10 +52,11 @@ ifelse(REDHAT,1, ifelse(DEBIAN,1, bsdmainutils dnl old name used in Jessie and earlier btrfs-tools + clevis-luks cryptsetup dash extlinux dnl gfs-tools, gfs2-tools have been renamed to gfs2-utils gfs-tools @@ -90,10 +92,11 @@ dnl iproute has been renamed to iproute2 ) ifelse(ARCHLINUX,1, cdrkit cdrtools + clevis cryptsetup dhclient dhcpcd gptfdisk grub @@ -117,10 +120,11 @@ ifelse(ARCHLINUX,1, ifelse(SUSE,1, dnl It seems no other augeas package depends on it. augeas-lenses btrfsprogs cdrkit-cdrtools-compat + clevis cryptsetup dhcpcd dhcp-client glibc-locale gptfdisk diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR index c0556fb20f37..4d0e90cbcbaf 100644 --- a/lib/MAX_PROC_NR +++ b/lib/MAX_PROC_NR @@ -1 +1 @@ -511 +512 diff --git a/lib/guestfs.pod b/lib/guestfs.pod index 1ad44e7c2878..e10bf1fbec30 100644 --- a/lib/guestfs.pod +++ b/lib/guestfs.pod @@ -589,15 +589,26 @@ devices. For LUKS it returns the string C<crypto_LUKS>. For Windows BitLocker it returns C<BitLocker>. Then open these devices by calling L</guestfs_cryptsetup_open>. Obviously you will require the passphrase! +Passphrase-less unlocking is supported for LUKS (not BitLocker) +block devices that have been encrypted with network-bound disk +encryption (NBDE), using Clevis on the Linux guest side, and +Tang on a separate Linux server. Open such devices with +L</guestfs_clevis_luks_unlock>. The appliance will need +networking enabled (refer to L</guestfs_set_network>) and actual +connectivity to the Tang servers noted in the C<tang> Clevis +pins that are bound to the LUKS header. (This includes the +ability to resolve the names of the Tang servers.) + Opening an encrypted device creates a new device mapper device -called F</dev/mapper/mapname> (where C<mapname> is the -string you supply to L</guestfs_cryptsetup_open>). -Reads and writes to this mapper device are decrypted from and -encrypted to the underlying block device respectively. +called F</dev/mapper/mapname> (where C<mapname> is the string +you supply to L</guestfs_cryptsetup_open> or +L</guestfs_clevis_luks_unlock>). Reads and writes to this mapper +device are decrypted from and encrypted to the underlying block +device respectively. LVM volume groups on the device can be made visible by calling L</guestfs_vgscan> followed by L</guestfs_vg_activate_all>. The logical volume(s) can now be mounted in the usual way. -- 2.19.1.3.g30247aa5d201

On 06/28/22 16:57, Richard W.M. Jones wrote: Right, I evaluated those (and some more) questions in <https://bugzilla.redhat.com/show_bug.cgi?id=1809453#c13>;: - guestfs_cryptsetup_open() permits "crypttype" = bitlk, but that makes no sense with clevis (no BitLocker, just LUKS, with Clevis -- I read this in the Clevis documentation) - guestfs_cryptsetup_open() permits "readonly" = 1, but read-only mappings are not supported by clevis-luks-unlock(1). No, the longest line (excluding the \n in the source code) is exactly 80 characters long. Is that a problem as well? I've noticed that the pod files use shorter lines, but couldn't figure out if it was 72 chars, 64 chars, or something else. They are not all consistent. Thanks! Laszlo

Call the C-language helper key_store_requires_network() in guestfish and guestmount. (Short log for the "common" submodule, commit range 35b49ce142fb..0399dea30e63: Laszlo Ersek (11): options: fix UUID comparison logic bug in get_keys() mltools/tools_utils: remove unused function "key_store_to_cli" mltools/tools_utils: allow multiple "--key" options for OCaml tools too options: replace NULL-termination with number-of-elements in get_keys() options: wrap each passphrase from get_keys() into a struct options: add back-end for LUKS decryption with Clevis+Tang options: introduce selector tpe "key_clevis" options: generalize "--key" selector parsing for C-language utilities mltools/tools_utils: generalize "--key" selector parsing for OCaml utils options, mltools/tools_utils: parse "--key ID:clevis" options options, mltools/tools_utils: add helper for network dependency ). Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: The submodule commit range 35b49ce142fb..0399dea30e63 needs to be refreshed in both the commit message and the "common" hunk, once the libguestfs-common series is upstream. fish/fish.c | 3 +++ fuse/guestmount.c | 4 ++++ common | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/fish/fish.c b/fish/fish.c index 23d9bb94f3a4..19e3d2799e84 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -474,10 +474,13 @@ main (int argc, char *argv[]) CHECK_OPTION_blocksize_consumed; /* If we've got drives to add, add them now. */ add_drives (drvs); + if (key_store_requires_network (ks) && guestfs_set_network (g, 1) == -1) + exit (EXIT_FAILURE); + /* If we've got mountpoints or prepared drives or -i option, we must * launch the guest and mount them. */ if (next_prepared_drive > 1 || mps != NULL || inspector) { /* RHBZ#612178: If --listen flag is given, then we will fork into diff --git a/fuse/guestmount.c b/fuse/guestmount.c index 77c534828fcd..3c6d57bde0d6 100644 --- a/fuse/guestmount.c +++ b/fuse/guestmount.c @@ -346,10 +346,14 @@ main (int argc, char *argv[]) if (guestfs_set_recovery_proc (g, !do_fork) == -1) exit (EXIT_FAILURE); /* Do the guest drives and mountpoints. */ add_drives (drvs); + + if (key_store_requires_network (ks) && guestfs_set_network (g, 1) == -1) + exit (EXIT_FAILURE); + if (guestfs_launch (g) == -1) exit (EXIT_FAILURE); if (inspector) inspect_mount (); mount_mps (mps); diff --git a/common b/common index 35b49ce142fb..0399dea30e63 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit 35b49ce142fb906bcf91edc6a6718d28c8fca9e3 +Subproject commit 0399dea30e6353870183a07a82d05b4ec8f20ca0 -- 2.19.1.3.g30247aa5d201

Call the OCaml-language helper "key_store_requires_network" in those OCaml utilities that pass "~key_opts:true" to "create_standard_options", and do not have any code related to networking yet. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- get-kernel/get_kernel.ml | 1 + sparsify/copying.ml | 1 + sparsify/in_place.ml | 1 + 3 files changed, 3 insertions(+) diff --git a/get-kernel/get_kernel.ml b/get-kernel/get_kernel.ml index e485cf4952da..9c2aa17c225b 100644 --- a/get-kernel/get_kernel.ml +++ b/get-kernel/get_kernel.ml @@ -174,10 +174,11 @@ let main () = let add, output, unversioned, prefix, ks = parse_cmdline () in (* Connect to libguestfs. *) let g = open_guestfs () in add g; + g#set_network (key_store_requires_network ks); g#launch (); (* Decrypt the disks. *) inspect_decrypt g ks; diff --git a/sparsify/copying.ml b/sparsify/copying.ml index 39d06c94c0ac..21a603d63548 100644 --- a/sparsify/copying.ml +++ b/sparsify/copying.ml @@ -185,10 +185,11 @@ You can ignore this warning or change it to a hard failure using the if not (quiet ()) then ( let machine_readable = machine_readable () <> None in Progress.set_up_progress_bar ~machine_readable g ); + g#set_network (key_store_requires_network ks); g#launch (); g in (* Decrypt the disks. *) diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml index 00f0e056478c..0eec63e6f049 100644 --- a/sparsify/in_place.ml +++ b/sparsify/in_place.ml @@ -56,10 +56,11 @@ let run disk format ignores zeroes ks = if not (quiet ()) then ( let machine_readable = machine_readable () <> None in Progress.set_up_progress_bar ~machine_readable g ); + g#set_network (key_store_requires_network ks); g#launch (); (* If discard is not supported in the appliance, we must return exit * code 3. See the man page. *) -- 2.19.1.3.g30247aa5d201

Similarly to virt-customize, virt-sysprep has prior "--network" and "--no-network" options. Unlike virt-customize though, virt-sysprep defaults to disabling the appliance network. Therefore we can't tell whether the network is disabled "by default" or because the user requested it. That's a problem: "--key ID:clevis" is supposed to override the former, but not the latter. Add a separate option for tracking "--no-network", and only if "--no-network" is absent, permit "--network" or "--key ID:clevis" to turn on the network. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- sysprep/main.ml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sysprep/main.ml b/sysprep/main.ml index b760618ad58a..1f722dfb06be 100644 --- a/sysprep/main.ml +++ b/sysprep/main.ml @@ -42,10 +42,11 @@ let main () = let dryrun = ref false in let files = ref [] in let libvirturi = ref "" in let mount_opts = ref "" in let network = ref false in + let no_network = ref false in let operations = ref None in let format = ref "auto" in let format_consumed = ref true in let set_format s = @@ -129,11 +130,11 @@ let main () = [ L"enable" ], Getopt.String (s_"operations", set_enable), s_"Enable specific operations"; [ L"format" ], Getopt.String (s_"format", set_format), s_"Set format (default: auto)"; [ L"list-operations" ], Getopt.Unit list_operations, s_"List supported operations"; [ L"mount-options" ], Getopt.Set_string (s_"opts", mount_opts), s_"Set mount options (eg /:noatime;/var:rw,noatime)"; [ L"network" ], Getopt.Set network, s_"Enable appliance network"; - [ L"no-network" ], Getopt.Clear network, s_"Disable appliance network (default)"; + [ L"no-network" ], Getopt.Set no_network, s_"Disable appliance network (default)"; [ L"operation"; L"operations" ], Getopt.String (s_"operations", set_operations), s_"Enable/disable specific operations"; ] in let args = basic_args @ Sysprep_operation.extra_args () in let usage_msg = sprintf (f_"\ @@ -186,10 +187,11 @@ read the man page virt-sysprep(1). in (* Dereference the rest of the args. *) let dryrun = !dryrun in let network = !network in + let no_network = !no_network in let operations = !operations in (* At this point we know which operations are enabled. So call the * not_enabled_check_args method of all *disabled* operations, so * they have a chance to check for unused command line args. @@ -206,11 +208,12 @@ read the man page virt-sysprep(1). message (f_"Examining the guest ..."); (* Connect to libguestfs. *) let g = open_guestfs () in - g#set_network network; + g#set_network (not no_network && + (network || key_store_requires_network opthandle.ks)); add g dryrun; g#launch (); operations, g, mount_opts, opthandle.ks in -- 2.19.1.3.g30247aa5d201

On Tue, Jun 28, 2022 at 01:58:56PM +0200, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Phew, thanks! 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

On 06/28/22 16:59, Richard W.M. Jones wrote: Commit 98fa5ab26853. Thanks! :) Laszlo