On Mon, 7 Feb 2022 10:29:43 -0500
James Bottomley <j...@linux.ibm.com> wrote:
> Make use of the new OS provided secrets API so that if the new '-s'
> option is passed in we try to extract the secret from the API rather
> than prompting for it.
>
> The primary consumer of this is AMD SEV, which has been programmed to
> provide an injectable secret to the encrypted virtual machine. OVMF
> provides the secret area and passes it into the EFI Configuration
> Tables. The grub EFI layer pulls the secret out and primes the
> secrets API with it. The upshot of all of this is that a SEV
> protected VM can do an encrypted boot with a protected boot secret.
I think I prefer the key protector framework proposed in the first
patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It feels
like a more generic mechanism (though admittedly very similar to yours)
because its not tied to cryptodisk. I'm not sure where we'd want to use
the secrets/protectors framework outside of cryptodisk, but it seems
like it could be useful. The advantage of this patch is that it allows
for the clearing of key data from memory.
I don't think we should have both a secrets and a key protectors
framework, as its seems they are aiming to accomplish basically the
same thing.
The name "secrets" seems a bit more generic than "protectors" because,
as in this case, the secret is not protected. There is something I
don't like about the word "secrets", but I don't have a better
suggestion, so this might be what sticks. One thing going for "secrets"
over "protectors", is that the cryptomount option "-s" works better
than "-p" for protectors because that's already taken by the password
option.
>
> Signed-off-by: James Bottomley <j...@linux.ibm.com>
>
> ---
>
> v2: add callback for after secret use
> v3: update to use named module mechanism for secret loading
> v4: update for new secret passing API
> ---
> grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++++++++--
> include/grub/cryptodisk.h | 14 ++++++++++
> 2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..f9c86f958 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> + {"secret", 's', 0, N_("Get secret passphrase from named module and
> optional identifier"), 0, 0},
> {0, 0, 0, 0, 0, 0}
> };
>
> @@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> +/* variable to hold the list of secret providers */
> +static struct grub_secret_entry *secret_providers;
> +
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> {
> @@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
> return dev;
> }
>
> +void
> +grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e)
> +{
> + grub_list_push(GRUB_AS_LIST_P (&secret_providers), GRUB_AS_LIST (e));
> +}
> +
> +void
> +grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e)
> +{
> + grub_list_remove (GRUB_AS_LIST (e));
> +}
> +
> #ifdef GRUB_UTIL
> #include <grub/util/misc.h>
> grub_err_t
> @@ -1160,10 +1176,14 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> {
> struct grub_arg_list *state = ctxt->state;
> struct grub_cryptomount_args cargs = {0};
> + struct grub_secret_entry *se = NULL;
>
> - if (argc < 1 && !state[1].set && !state[2].set)
> + if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> + if (state[3].set && state[4].set)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive
> options");
> +
> if (grub_cryptodisk_list == NULL)
> return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
>
> @@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> cargs.key_data = (grub_uint8_t *) state[3].arg;
> cargs.key_len = grub_strlen (state[3].arg);
> }
> + else if (state[4].set) /* secret module */
> + {
> + grub_err_t rc;
> +
> + if (argc < 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must be
> specified");
> +#ifndef GRUB_UTIL
> + grub_dl_load (args[0]);
> +#endif
> + se = grub_named_list_find (GRUB_AS_NAMED_LIST (secret_providers),
> args[0]);
> + if (se == NULL)
> + return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret provider is
> found");
> +
> + rc = se->get (args[1], &cargs.key_data);
> + if (rc)
> + return rc;
> + cargs.key_len = grub_strlen((char *) cargs.key_data);
It seems better to me to send a pointer to cargs.key_len to se->get()
because it already knows the length without having to do a strlen. And
this will allow NULLs in the key data.
> + }
>
> if (state[0].set) /* uuid */
> {
> @@ -1190,6 +1228,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> cargs.search_uuid = args[0];
> found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device,
> &cargs);
>
> + if (state[4].set)
> + {
> + se->put (args[1], found_uuid, &cargs.key_data);
> + }
> +
> if (found_uuid)
> return GRUB_ERR_NONE;
> else if (grub_errno == GRUB_ERR_NONE)
> @@ -1208,6 +1251,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> {
> cargs.check_boot = state[2].set;
> grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> + if (state[4].set)
> + {
> + se->put (args[1], 1, &cargs.key_data);
> + }
> return GRUB_ERR_NONE;
> }
> else
> @@ -1252,6 +1299,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> if (disklast)
> *disklast = ')';
>
> + if (state[4].set)
> + {
> + se->put (args[1], dev != NULL, &cargs.key_data);
> + }
> +
> return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
> }
> }
> @@ -1385,7 +1437,7 @@ GRUB_MOD_INIT (cryptodisk)
> {
> grub_disk_dev_register (&grub_cryptodisk_dev);
> cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> - N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
> + N_("[-p password] <SOURCE|-u UUID|-a|-b|-s MOD
> [ID]>"),
Seems like this should be:
"[-p password|-s MOD [ID]] <SOURCE|-u UUID|-a|-b"
> N_("Mount a crypto device."), options);
> grub_procfs_register ("luks_script", &luks_script);
> }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index c6524c9ea..60249f1fc 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
> grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
> grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
>
> +struct grub_secret_entry {
s/grub_secret_entry/grub_secret_provider/
> + /* as named list */
> + struct grub_secret_entry *next;
> + struct grub_secret_entry **prev;
> + const char *name;
> +
> + /* additional entries */
> + grub_err_t (*get) (const char *arg, grub_uint8_t **secret);
I like having this first arg to pass some data to the secret provider.
I'm guessing this is to accomodate a keyfiles secrets provider. It seems
like it could also be used to differentiate between different elements
using the same secrets provider. Hypotheticaly say one had a USB device
with a bunch of key slots, a numerical value could be passed to choose
which slot to use. In light of that, perhaps the arg should be a void *,
to be more generic.
> + grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t **secret);
I don't really like the names get and put. Something more descriptive
would be nice. I like "recover_key" or perhaps "recover_secret" for
"get", and "dispose" or "release" for "put". I'm open to other names
also.
> +};
> +
> +void grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e);
> +void grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e);
> +
> #endif
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
