Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management

[Kevin Wolf]

Am 10.03.2020 um 12:05 hat Maxim Levitsky geschrieben:
> On Tue, 2020-03-10 at 11:58 +0100, Max Reitz wrote:
> > On 08.03.20 16:18, Maxim Levitsky wrote:
> > > Next few patches will expose that functionality
> > > to the user.
> > > 
> > > Signed-off-by: Maxim Levitsky <address@hidden>
> > > ---
> > >  crypto/block-luks.c | 398 +++++++++++++++++++++++++++++++++++++++++++-
> > >  qapi/crypto.json    |  61 ++++++-
> > >  2 files changed, 455 insertions(+), 4 deletions(-)
> > 
> > [...]
> > 
> > > +##
> > > +# @QCryptoBlockAmendOptionsLUKS:
> > > +#
> > > +# This struct defines the update parameters that activate/de-activate set
> > > +# of keyslots
> > > +#
> > > +# @state: the desired state of the keyslots
> > > +#
> > > +# @new-secret:    The ID of a QCryptoSecret object providing the 
> > > password to be
> > > +#                 written into added active keyslots
> > > +#
> > > +# @old-secret:    Optional (for deactivation only)
> > > +#                 If given will deactive all keyslots that
> > > +#                 match password located in QCryptoSecret with this ID
> > > +#
> > > +# @iter-time:     Optional (for activation only)
> > > +#                 Number of milliseconds to spend in
> > > +#                 PBKDF passphrase processing for the newly activated 
> > > keyslot.
> > > +#                 Currently defaults to 2000.
> > > +#
> > > +# @keyslot:       Optional. ID of the keyslot to activate/deactivate.
> > > +#                 For keyslot activation, keyslot should not be active 
> > > already
> > > +#                 (this is unsafe to update an active keyslot),
> > > +#                 but possible if 'force' parameter is given.
> > > +#                 If keyslot is not given, first free keyslot will be 
> > > written.
> > > +#
> > > +#                 For keyslot deactivation, this parameter specifies the 
> > > exact
> > > +#                 keyslot to deactivate
> > > +#
> > > +# @unlock-secret: Optional. The ID of a QCryptoSecret object providing 
> > > the
> > > +#                 password to use to retrive current master key.
> > > +#                 Defaults to the same secret that was used to open the 
> > > image
> > 
> > So this matches Markus’ proposal except everything is flattened (because
> > we don’t support nested unions, AFAIU).  Sounds OK to me.  The only
> > difference is @unlock-secret, which did not appear in his proposal.  Why
> > do we need it again?
> 
> That a little undocumented hack that will disappear one day.

It is very much documented (just a few lines above this one), and even
if it weren't documented, that wouldn't make it an unstable ABI.

If you don't want to make it to become stable ABI, you either need to
drop it or it needs an x- prefix, and its documentation should specify
what prevents it from being a stable ABI.

> Its because the driver currently doesn't keep a copy of the master key,
> and instead only keeps ciper objects, often from outside libraries,
> and in theory these objects might even be implemented in hardware so that
> master key might be not in memory at all, so I kind of don't want yet
> to keep it in memory.
> Thus when doing the key management, I need to retrieve the master key again,
> similar to how it is done on image opening. I use the same secret as was used 
> for opening,
> but in case the keys were changed already, that secret might not work anymore.
> Thus I added this parameter to specify basically the old password, which is 
> reasonable
> when updating passwords.
> I usually omit this hack in the discussions as it is orthogonal to the rest 
> of the API.

How will this requirement disappear one day?

Kevin