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

[Maxim Levitsky]

On Tue, 2020-03-10 at 14:02 +0200, Maxim Levitsky wrote:
> 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
> 
> 
OK folks, besides this hack (which I can remove if you insist, although I don't
think it matters), what else should I do to move forward to get this accepted?

Best regards,
        Maxim Levitsky