Re: [PATCH 4/5] cryptodisk: Support key protectors

[Hernan Gatta]

On Tue, 25 Jan 2022, James Bottomley wrote:

> On Mon, 2022-01-24 at 23:42 -0600, Glenn Washburn wrote:
> > On Mon, 24 Jan 2022 06:12:17 -0800
> > Hernan Gatta <hegatta@linux.microsoft.com> wrote:
> [...]
> > > +    }
> > > +
> > >    if (state[0].set) /* uuid */
> > >      {
> > >        int found_uuid;
> > > @@ -1385,7 +1404,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] [-k protector[:args]]
> > > <SOURCE|-u UUID|-a|-b>"),
> >
> > This looks eerily similiar to what I proposed to James in response to
> > his SEV patches[1]. As such, I am in favor of this syntax and it
> > looks to me like a framework that James can use for his SEV series.
>
> Well, I could, but I've got to say for a shell like system, which grub
> is, argument lists passed as a single argument always cause problems
> with quoting and interpolation.  if the protector needs additional
> arguments to initialize, then it should really be done as a separate
> module to avoid the arguments within argument problem, so
>
> protector_init [args]
> crtptomount -k protector ...
>
> means that [args] can use the standard arguments and doesn't have any
> internal quoting issues.

I agree that having nested argument lists can cause trouble, particularly 
for any future key protectors where escaping may be required.

As Glenn mentioned in his E-mail regarding the question of whether we 
should assume fallback on a passphrase, would you be in favor of a system 
where we do, say:

tpm2_protector_init [args]
sev_protector_init [args]

cryptomount -k sev,tpm2,passphrase

And then have cryptomount go through the list, falling back if protectors 
error out? (I'm not suggesting that the TPM2 and SEV protectors would be 
used simultaneously, I'm only using them as examples). Readily existing 
uses of cryptomount without -k would assume a passphrase protector by 
default where the user would be prompted in the usual way.

Another option would be to remove the -k parameter altogether and assume 
the order of execution on the basis of the initialization order of the 
protectors::

A_protector_init [args]
B_protector_init [args]
cryptomount  // Would try A then B

protectors_clear

B_protector_init [args]
cryptomount  // Would try B only

I like the first option best since it requires no global state.

Thoughts? Do you think that having the -k parameter as it is now would 
prevent you from using this patch series as it is for your purposes?

> James

Thank you,
Hernan