Re: [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)

[Daniel P . Berrangé]

On Tue, Aug 20, 2019 at 08:27:48PM +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > This adds:
> > 
> > * x-blockdev-update-encryption and x-blockdev-erase-encryption qmp commands
> >   Both commands take the QCryptoKeyManageOptions
> >   the x-blockdev-update-encryption is meant for non destructive addition
> >   of key slots / whatever the encryption driver supports in the future
> > 
> >   x-blockdev-erase-encryption is meant for destructive encryption key erase,
> >   in some cases even without way to recover the data.
> > 
> > 
> > * bdrv_setup_encryption callback in the block driver
> >   This callback does both the above functions with 'action' parameter
> > 
> > * QCryptoKeyManageOptions with set of options that drivers can use for 
> > encryption managment
> >   Currently it has all the options that LUKS needs, and later it can be 
> > extended
> >   (via union) to support more encryption drivers if needed
> > 
> > * blk_setup_encryption / bdrv_setup_encryption - the usual block layer 
> > wrappers.
> >   Note that bdrv_setup_encryption takes BlockDriverState and not BdrvChild,
> >   for the ease of use from the qmp code. It is not expected that this 
> > function
> >   will be used by anything but qmp and qemu-img code
> > 
> > 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  block/block-backend.c          |  9 ++++++++
> >  block/io.c                     | 24 ++++++++++++++++++++
> >  blockdev.c                     | 40 ++++++++++++++++++++++++++++++++++
> >  include/block/block.h          | 12 ++++++++++
> >  include/block/block_int.h      | 11 ++++++++++
> >  include/sysemu/block-backend.h |  7 ++++++
> >  qapi/block-core.json           | 36 ++++++++++++++++++++++++++++++
> >  qapi/crypto.json               | 26 ++++++++++++++++++++++
> >  8 files changed, 165 insertions(+)
> 
> Now I don’t know whether you want to keep this interface at all, because
> the cover letter seemed to imply you’d prefer a QMP amend.  But let it
> be said that a QMP amend is no trivial task.  I think the most difficult
> bit is that the qcow2 implementation currently is inherently an offline
> operation.  It isn’t a good idea to use it on a live image.  (Maybe it
> works, but it’s definitely not what I had in mind when I wrote it.)

For key mgmt we definitely need to have an online capability.

If layering it into a general purpose format "amend' option
is too complex, then we should keep it separate rather than
making our lives uncecessarily difficult IMHO.

> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 0d43d4f37c..53ed411eed 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5327,3 +5327,39 @@
> >    'data' : { 'node-name': 'str',
> >               'iothread': 'StrOrNull',
> >               '*force': 'bool' } }
> > +
> > +
> > +##
> > +# @x-blockdev-update-encryption:
> > +#
> > +# Update the encryption keys for an encrypted block device
> > +#
> > +# @node-name:        Name of the blockdev to operate on
> > +# @force:         Disable safety checks (use with care)
> > +# @options:       Driver specific options
> > +#
> > +
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-update-encryption',
> > +  'data': { 'node-name' : 'str',
> > +            '*force' : 'bool',
> > +            'options': 'QCryptoEncryptionSetupOptions' } }
> > +
> > +##
> > +# @x-blockdev-erase-encryption:
> > +#
> > +# Erase the encryption keys for an encrypted block device
> > +#
> > +# @node-name:        Name of the blockdev to operate on
> 
> Why the tab?
> 
> > +# @force:         Disable safety checks (use with care)
> 
> I think being a bit more verbose wouldn’t hurt.
> 
> (Same above.)
> 
> > +# @options:       Driver specific options
> > +#
> > +# Returns: @QCryptoKeyManageResult
> > +#
> > +# Since: 4.2
> > +##
> > +{ 'command': 'x-blockdev-erase-encryption',
> > +  'data': { 'node-name' : 'str',
> > +            '*force' : 'bool',
> > +            'options': 'QCryptoEncryptionSetupOptions' } }
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..69e8b086db 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -309,3 +309,29 @@
> >    'base': 'QCryptoBlockInfoBase',
> >    'discriminator': 'format',
> >    'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> > +
> > +
> > +##
> > +# @QCryptoEncryptionSetupOptions:
> > +#
> > +# Driver specific options for encryption key management.
> 
> The options do seem LUKS-specific, but the name of this structure does not.
> 
> > +# @key-secret: the ID of a QCryptoSecret object providing the password
> > +#              to add or to erase (optional for erase)
> > +#
> > +# @old-key-secret: the ID of a QCryptoSecret object providing the password
> > +#                  that can currently unlock the image
> > +#
> > +# @slot: Key slot to update/erase
> > +#        (optional, for update will select a free slot,
> > +#        for erase will erase all slots that match the password)
> > +#
> > +# @iter-time: number of milliseconds to spend in
> > +#             PBKDF passphrase processing. Currently defaults to 2000
> > +# Since: 4.2
> > +##
> 
> Does it really make sense to use the same structure for erasing and
> updating?  I think there are ways to represent @key-secret vs. @slot
> being alternatives to each other for erase; @iter-time doesn’t seem to
> make sense for erase; and @slot doesn’t seem to make sense for update.
> Also, I don’t know whether to use @key-secret or @old-key-secret for erase.
> 
> All in all, it seems more sensible to me to have separate structs for
> updating and erasing.

I agree, we can get a more understandable schema if each
command has its own struct wit the right options.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|