Laszlo Ersek
2022-Jun-29 12:43 UTC
[Libguestfs] [libguestfs-common PATCH 07/12] options: add back-end for LUKS decryption with Clevis+Tang
On 06/28/22 16:33, Richard W.M. Jones wrote:> On Tue, Jun 28, 2022 at 01:49:10PM +0200, Laszlo Ersek wrote: >> 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 at redhat.com> >> --- >> >> 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. > > ^ Yes, we need to do this one. > >> (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. > > I'm not sure I see the problem here. If we don't have the interface > at compile time [or the feature at runtime - but that's extra > complexity], _and_ the user uses the new option, we should fail with > an error (using the normal mechanism for errors, don't print stuff on > stderr). It's an error that we need to report to the user if they're > expecting a clevis selector to work and it cannot.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> >> 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. > > The trouble with (2) is it pushes the complexity to distros. We now > need to synchronise releases across 3 packages (and we have no choice > about it), for what is a relatively minor new feature in the big picture. > >> 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 * > > The patch itself looks fine, apart from use of the new API. > > Rich. >
Richard W.M. Jones
2022-Jun-29 13:49 UTC
[Libguestfs] [libguestfs-common PATCH 07/12] options: add back-end for LUKS decryption with Clevis+Tang
On Wed, Jun 29, 2022 at 02:43:46PM +0200, Laszlo Ersek wrote:> On 06/28/22 16:33, Richard W.M. Jones wrote: > > On Tue, Jun 28, 2022 at 01:49:10PM +0200, Laszlo Ersek wrote: > >> 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 at redhat.com> > >> --- > >> > >> 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. > > > > ^ Yes, we need to do this one. > > > >> (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. > > > > I'm not sure I see the problem here. If we don't have the interface > > at compile time [or the feature at runtime - but that's extra > > complexity], _and_ the user uses the new option, we should fail with > > an error (using the normal mechanism for errors, don't print stuff on > > stderr). It's an error that we need to report to the user if they're > > expecting a clevis selector to work and it cannot. > > 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.It sounds over-complicated. Why wouldn't this work: #ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK clevis_luks_unlock (g, blah); #else error (g, f_"libguestfs was not compiled with clevis/tang functionality"); return -1; #endif (and nothing else). I wouldn't overthink this, we just want an actionable error message and for downstream packagers not to have to upgrade libguestfs + guestfs-tools + virt-v2v all at the same time. Rich.> Thanks, > Laszlo > > > > > >> 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. > > > > The trouble with (2) is it pushes the complexity to distros. We now > > need to synchronise releases across 3 packages (and we have no choice > > about it), for what is a relatively minor new feature in the big picture. > > > >> 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 * > > > > The patch itself looks fine, apart from use of the new API. > > > > 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