[Libguestfs] [libguestfs-common PATCH 07/12] options: add back-end for LUKS decryption with Clevis+Tang
Laszlo Ersek
lersek at redhat.com
Wed Jun 29 12:43:46 UTC 2022
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.
>
More information about the Libguestfs
mailing list