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

[Maxim Levitsky]

On Tue, 2020-03-10 at 12:59 +0100, Kevin Wolf wrote:
> 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?
If I cave in and keep a copy of the master key in the memory :-)

Best regards,
        Maxim Levitsky

> 
> Kevin