Re: Question on loading trusted key with keyctl command

[Mimi Zohar <zohar@linux.ibm.com>]

On Tue, 2022-12-20 at 09:23 -0500, James Bottomley wrote:
> On Tue, 2022-12-20 at 09:13 -0500, Mimi Zohar wrote:
> > On Tue, 2022-12-20 at 09:03 -0500, James Bottomley wrote:
> > > On Tue, 2022-12-20 at 08:54 -0500, Mimi Zohar wrote:
> > > > On Tue, 2022-12-20 at 07:50 -0500, James Bottomley wrote:
> > > > > On Tue, 2022-12-20 at 12:03 +0530, Sughosh Ganu wrote:
> > > [...]
> > > > > > I was able to load the key after clearing the keyring. Thanks
> > > > > > James and Mimi for your pointers.
> > > > > 
> > > > > Actually, I think this is a bug in trusted keys.  Add on
> > > > > existing key is supposed to go through the update path.  If the
> > > > > path doesn't exist it returns -EEXIST.  Trusted keys have an
> > > > > update path but they return - EINVAL if the trusted key command
> > > > > is anything but update (which is used to reseal a key). 
> > > > > Obviously this is incorrect and the code should be returning -
> > > > > EEXIST for a key we refuse to update to match every other key
> > > > > type.
> > > > 
> > > > Re-loading an existing key was previously permitted.  Obviously
> > > > this changed at some point.   Any "fixes" should point out when
> > > > it changed.
> > > 
> > > Git history doesn't think so.  It thinks when you    added trusted
> > > keys with d00a1c72f7f4661212299e6cb132dfa58030bcdb the update path
> > > already had the -EINVAL return, so reload has always failed this
> > > way unless we were doing a reseal update.  We could certainly
> > > permit overwriting an existing key with load, but that would be a
> > > more extensive change.
> > 
> > Replacing existing keys/keyrings was part of the infrastructure.  I
> > don't think this change has anything to do with trusted type keys.  
> > The ability of replacing keys/keyrings was one of the main reasons
> > for trusted keyrings (dot prefixed keyrings).
> 
> Keys can only be replaced by calling the ->update() helper for the key
> type.  If that doesn't exist keyctl add will return -EEXIST (behaviour
> for at least the last 12 years).  Most key type update routines do
> unconditionally update, so the error they return is the same error they
> would have returned for an add of a non existent key (EINVAL if the
> payload is too large, for instance).  The trusted keys ->update()
> helper (trusted_update()) only allows update if the trusted operation
> is update, so they've always failed a load with EINVAL going back to
> the original commit I quoted.  At no time that I can find has there
> ever been a modification to this supporting updating trusted keys with
> anything other than an update trusted operation.  So they've supported
> changing the sealing parameters (PCR values) but not changing the
> payload.  This contrasts with user keys where add of a new payload on
> an existing key changes the payload.

Yes, my mistake.  With your change, it's now returning "add_key: File
exists".

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
-- 
thanks,

Mimi