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

[Max Reitz]

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.)

So I’ll still take a quick glance at the interface here.

[...]

> 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.

Max

> +{ 'struct': 'QCryptoEncryptionSetupOptions',
> +  'data': { '*key-secret': 'str',
> +            '*old-key-secret': 'str',
> +            '*slot': 'int',
> +            '*iter-time': 'int' } }
>

signature.asc
Description: OpenPGP digital signature