Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key

[James Bottomley]

On Tue, 2021-01-05 at 14:03 -0600, Glenn Washburn wrote:
> On Mon, 04 Jan 2021 22:48:49 -0800
> James Bottomley <jejb@linux.ibm.com> wrote:
> 
> > On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> > > James,
> > > 
> > > I like the improvements here. However, I've been thinking more
> > > about this and other improvements that deal with passing
> > > parameters to modules used by cryptomount. I have some ideas that
> > > I've not had the time to fully investigate or code up proof of
> > > concepts. One idea is that we shouldn't be changing the function
> > > declaration of recover key, that is to say adding new parameters.
> > > Instead we should be adding the parameters to grub_cryptodisk_t
> > > and setting them in grub_cryptodisk_scan_device_real. This would
> > > satisfy needs of this patch series as well as the key file,
> > > detached header, sending password as cryptomount arg, and master
> > > key features without cluttering the function signature.
> > 
> > Keeping large amounts of shared state between caller and callee can
> > be a debugger's nightmare.  In this case the only consumer of the
> > password callback is the recover function, so it seems appropriate
> > it should be an argument to that function.
> 
> Please see my recently submitted RFC patch for a more concrete idea
> of exactly what I'm suggesting. I don't see the concern about shared
> state as being applicable in this case, even though your point is
> valid in other situations.

Well, I've seen the patch, but it doesn't seem to address the root of
the problem of where the password data might be used: if the password
data is used by more than one function then it should likely be part of
the shared data; if it's only used by a single function it makes more
sense as an argument.  I think you need to flesh your RFC out further
to make that determination.

On what is passed, we do have a question about whether it's data or a
functional callback.  I do tend to prefer callbacks in situations like
this because they solve the lifetime issues (secrets should have well
defined lifetimes to make sure there's a limited window for leaking
them).  A simple data pointer doesn't necessarily do this.

So I think the important question is functional callback or data and
where it's passed is simply a more minor detail the solution to which
will become apparent in the use cases and which it's not hugely
important to get right in the first instance.

James

> > > So, I don't think this is the right approach.
> > 
> > The thing this patch demonstrates is that altering the function
> > signatures is fairly easy, so it would be a simple patch to alter
> > them again if the password callback were decided to be an essential
> > component of the cryptodisk device ... but it should really driven
> > the need which isn't apparent yet.
> 
> By easy, I take you to mean there aren't a lot of places needing to
> be modified because of the change in signature, and in that sense its
> easy, which is good. But its also unnecessary. I'm not sure if you've
> looked at the struct grub_cryptodisk yet, but its already pretty
> cluttered. So I can see why you might not want to add more. However,
> I think my solution (again see RFC patch) is the cleaner, more
> scalable solution.