Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support

[Glenn Washburn]

Finally getting back to this...

On Thu, 17 Feb 2022 17:18:47 -0500
James Bottomley <jejb@linux.ibm.com> wrote:

> On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote:
> > On Mon,  7 Feb 2022 10:29:43 -0500
> > James Bottomley <jejb@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.
> 
> It's not really an either/or, though is it?  It looks to me like one
> can trivially evolve into the other.

Yep, this patch can easily become essentially what the key protector
framework is. I make the comparison because (1) its pending so it seems
likely that this would be modified soon, (2) I like that the key
protectors framework is in its own file as opposed to put in with
cryptodisk (though this patch is smaller so makes less sense as a
separate file), and (3) I find it aestetically unpleasing to add this
patch and then turn around and take it out and put it somewhere else.
But yes, it can be done.

> >  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.

Yep, we're on the same page here.

> > 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.

Yep, agreed. Both patches provide minimal APIs for the feature sets
they are introducing, as it should be. Aside from name changes, I'm not
criticizing your API (which to be clear I'm meaning the function
signatures). So I'm not seeing the above as relevant here.

I suspect that the core of your issue is that I was too general in my
comments above giving the impression that I'm rejecting this patch
without a clear path towards making it acceptable. To be more specific,
I liked that the key protectors patch was separated from cryptodisk
both in naming and in a separate file. I've since reconsidered the
separate file, which also doesn't make as much sense for this patch
because its small. I forgot to mention it before, but I was also
thinking that the function names could have a grub_secret_ prefix
instead of grub_cryptodisk_.

I think another issue, in which I've been on your side of the table of,
is determining which suggestions or dislikes are deal breakers. In the
abcense of clear communication on what's a deal breaker it can seem
like all are deal breakers. Me registering a complaint about something
doesn't necessarily mean I think it can't be accepted as is. However,
as a reviewer I feel it is my duty to list everything I have a problem
with. Ideally it should be very clear, but sometimes its hard to pin
point how a patch might be changed to be better. And if you want more
clarity (eg. on the specifics of a displike or how much of a deal
breaker something is) asking never hurts.

As for perfect being the enemy of the good, that's only if one forego's
the good in hand for an unattainable perfect. Good should just be an
iteration toward perfect. Perfect is the north star that provides
guidance on what is good. So I'm noting where its not perfect or at
least can be better (according to me). That doesn't mean if its not
perfect it can't be committed. But if we can make changes to make good
better now, then I think we should do that.

Also, I don't believe I'm bikeshedding about future implementations
here unless you consider the TPM unlock patch series the future. I'm
commenting on the relative difference in two patch series that are
looking to get included right now. So this isn't some hypothetical
situation.

Also, some of my comments may seem minor, trivial, or nit-picking. And
I'd agree with that for somethings, and I think that's okay as long as
we don't let the trivial be the enemy of the good. If we can get a
better patch series committed, I think thats 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");

I didn't mention the first time, but I also like the idea of having an
inline function grub_secrets_find_provider which does the above 3 lines
(maybe 6). The one drawback I see is an extra branch to check the
functions return and return if error. Not a deal breaker.

> > > +
> > > +      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] <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"
> 
> Yes, that's effectively what the implementation turned out to be, I
> just forgot to update the description string.
> 
> > 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 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.
> 
> It's always going to be a pointer into the command line arguments, so
> the type char * is the correct one for that.  It could become char
> **arg and be passed &arg[1] so the provider could take multiple
> optional arguments, but since the first implementation doesn't consume
> this, I think that should be for future users to sort out.

I was saying this in the context of having a secret api that was less
tied to cryptodisk. We can change it down the road fairly easily if it
ever comes to that.

> > > +  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.
> 
> get_secret; release_secret?

This seems reasonable.

Glenn