Laszlo Ersek
2022-Jun-30 12:20 UTC
[Libguestfs] [libguestfs-common PATCH v2 06/12] options: add back-end for LUKS decryption with Clevis+Tang
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:
v2:
- check GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK; error out if unavailable [Rich]
v1 notes were:
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 | 13 ++++++++++++-
options/keys.c | 7 ++++++-
3 files changed, 24 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..97c8b88d16aa 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -155,11 +155,22 @@ 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)
+#ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK
+ r = guestfs_clevis_luks_unlock (g, mountable, mapname);
+#else
+ error (EXIT_FAILURE, 0,
+ _("'clevis_luks_unlock', needed for decrypting %s,
is "
+ "unavailable in this libguestfs version"),
mountable);
+#endif
+ 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
Richard W.M. Jones
2022-Jun-30 16:10 UTC
[Libguestfs] [libguestfs-common PATCH v2 06/12] options: add back-end for LUKS decryption with Clevis+Tang
On Thu, Jun 30, 2022 at 02:20:22PM +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: > v2: > > - check GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK; error out if unavailable [Rich] > > v1 notes were: > > 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 | 13 ++++++++++++- > options/keys.c | 7 ++++++- > 3 files changed, 24 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..97c8b88d16aa 100644 > --- a/options/decrypt.c > +++ b/options/decrypt.c > @@ -155,11 +155,22 @@ 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) > +#ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK > + r = guestfs_clevis_luks_unlock (g, mountable, mapname); > +#else > + error (EXIT_FAILURE, 0, > + _("'clevis_luks_unlock', needed for decrypting %s, is " > + "unavailable in this libguestfs version"), mountable);To be clear on why this is correct, it's because this code is only used during option parsing in command line tools, where we want to exit when an error happens.> +#endif > + 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 *Reviewed-by: Richard W.M. Jones <rjones at redhat.com> -- 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
Laszlo Ersek
2022-Jul-01 09:56 UTC
[Libguestfs] [libguestfs-common PATCH v2 06/12] options: add back-end for LUKS decryption with Clevis+Tang
On 06/30/22 18:10, Richard W.M. Jones wrote:> On Thu, Jun 30, 2022 at 02:20:22PM +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: >> v2: >> >> - check GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK; error out if unavailable [Rich] >> >> v1 notes were: >> >> 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 | 13 ++++++++++++- >> options/keys.c | 7 ++++++- >> 3 files changed, 24 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..97c8b88d16aa 100644 >> --- a/options/decrypt.c >> +++ b/options/decrypt.c >> @@ -155,11 +155,22 @@ 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) >> +#ifdef GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK >> + r = guestfs_clevis_luks_unlock (g, mountable, mapname); >> +#else >> + error (EXIT_FAILURE, 0, >> + _("'clevis_luks_unlock', needed for decrypting %s, is " >> + "unavailable in this libguestfs version"), mountable); > > To be clear on why this is correct, it's because this code is only > used during option parsing in command line tools, where we want to > exit when an error happens.That's not the case. This is what I've been trying to express, but I've failed. :( See: $ virt-v2v -on converted --key /dev/sda2:clevis clevis [ 0.0] Setting up the source: -i libvirt clevis [ 1.2] Opening the source virt-v2v: 'clevis_luks_unlock', needed for decrypting /dev/sda2, is unavailable in this libguestfs version This code runs (in virt-v2v, for example) on the following call path: main [v2v/v2v.ml] create_standard_options [common/mltools/tools_utils.ml] Getopt.parse parse_key_selector [common/mltools/tools_utils.ml] List.push_back (* at this point, cmdline_options.ks * has been populated *) convert [convert/convert.ml] (* skipped in virt-v2v, because it * enables networking anyway: *) key_store_requires_network [common/mltools/tools_utils.ml] g#set_network g#launch inspect_decrypt [common/mltools/tools_utils.ml] c_inspect_decrypt [common/mltools/tools_utils.ml] guestfs_int_mllib_inspect_decrypt [common/mltools/tools_utils-c.c] loop over cmdline_options.ks: key_store_import_key [common/options/keys.c] inspect_do_decrypt [common/options/decrypt.c] decrypt_mountables [common/options/decrypt.c] MACRO CHECK HERE guestfs_clevis_luks_unlock free_key_store [common/options/keys.c] This is why I said "it's too late to report this error in decrypt_mountables()"! In comparison, here's how the C tools work (such as virt-cat): main [cat/cat.c] OPTION_key [common/options/options.h] key_store_add_from_selector [common/options/keys.c] key_store_import_key [common/options/keys.c] /* "ks" has been populated */ key_store_requires_network [common/options/keys.c] guestfs_set_network guestfs_launch inspect_mount [common/options/options.h] inspect_mount_handle [common/options/inspect.c] inspect_do_decrypt [common/options/decrypt.c] decrypt_mountables [common/options/decrypt.c] MACRO CHECK HERE guestfs_clevis_luks_unlock free_key_store [common/options/keys.c] Note the following important facts: (1) The macro check is reached, in both kinds of tools, way after the options have been parsed, and quite a bit after the appliance has been launched. (2) The OCaml and C *option parsers* share *nothing* related to keys. Consider: - OCaml does not call key_store_add_from_selector() at all, - while OCaml does call key_store_import_key(), it does so *outside* of option parsing. Namely, "parse_key_selector" records the command line options in an OCaml-only data structure (cmdline_options.ks). That is then translated to an *ephemeral* C-language keystore much later, with key_store_import_key() -- only in guestfs_int_mllib_inspect_decrypt()! That is, after the appliance is running. - Correspondingly: in the C tools, the C-language keystore is freed *after* inspection completes (that is, free_key_store() is called *after* inspect_mount() returns), but in OCaml, the ephemeral keystore is released (correctly) *inside* guestfs_int_mllib_inspect_decrypt() -- that is, internally to the outer function "inspect_decrypt". If we want to catch the lack of GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK during option parsing, then I need to implement a brand new C function, and wrap it for OCaml too (OCaml cannot check the C-language macro GUESTFS_HAVE_CLEVIS_LUKS_UNLOCK). For this: because we already have "key_store_requires_network" (same name, but totally separate impl in C and OCaml), I'd have to rename that function (both impls) to "key_store_contains_clevis", and use it for two purposes: - right after option parsing, if the key store contains clevis, but the feature test macro is missing, bail. - just before launching the appliance, if the key store contains clevis, enable networking. (My previous proposal went one step further: check the "clevisluks" option group too, once the appliance is up and running.) I'm sorry that I've failed to explain this before. I've tried, but it's really complicated. I hope the call trees help! Thanks, Laszlo> >> +#endif >> + 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 * > > Reviewed-by: Richard W.M. Jones <rjones at redhat.com> > >