[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management |
Date: |
Wed, 11 Mar 2020 14:55:49 +0200 |
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
[PATCH v2 03/14] block/amend: add 'force' option, Maxim Levitsky, 2020/03/08
[PATCH v2 04/14] block/amend: separate amend and create options for qemu-img, Maxim Levitsky, 2020/03/08
[PATCH v2 06/14] block/crypto: rename two functions, Maxim Levitsky, 2020/03/08
[PATCH v2 07/14] block/crypto: implement the encryption key management, Maxim Levitsky, 2020/03/08
[PATCH v2 08/14] block/qcow2: extend qemu-img amend interface with crypto options, Maxim Levitsky, 2020/03/08
[PATCH v2 09/14] iotests: filter few more luks specific create options, Maxim Levitsky, 2020/03/08
[PATCH v2 10/14] iotests: qemu-img tests for luks key management, Maxim Levitsky, 2020/03/08
[PATCH v2 05/14] block/amend: refactor qcow2 amend options, Maxim Levitsky, 2020/03/08
[PATCH v2 11/14] block/core: add generic infrastructure for x-blockdev-amend qmp command, Maxim Levitsky, 2020/03/08
[PATCH v2 12/14] block/crypto: implement blockdev-amend, Maxim Levitsky, 2020/03/08