On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote:
> On Mon, 7 Feb 2022 10:29:43 -0500
> James Bottomley 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.
It's not really an either/or, though is it? It looks to me like one
can trivially evolve into the other.
> 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.
So if you evolve one into the other the put API would naturally remain
even if the tpm2 protector didn't want it.
> 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.
Well, I don't think we would either. Looking at the protectors API, it
would become an incremental patch to this. Where I come from, this is
how we do API development: you merge an implementation and an API
that's as minimal as possible. You try to make sure the API can be
extended if necessary, but you don't extend it yourself until another
implementation comes along that needs the extension. This way you
don't need to bikeshed about future implementations because the perfect
is the enemy of the good.
> 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.
[...]
> + 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.
The original implementation was based on the grub password limitations
in that it had to be a zero terminated ASCII string. While the zero
termination could now be relaxed, the ASCII requirement remains,
because the LUKS tools don't like passphrases with NULLS in them. It
does simplify the code to pass the length pointer into the get API, so
I'll do that.
[...]
> > @@ -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] > b>"),
> > + N_("[-p password] > s MOD [ID]>"),
>
> Seems like this should be:
> "[-p password|-s MOD [ID]] 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/
OK
> > + /* 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 see
