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

[Maxim Levitsky]

On Thu, 2019-08-22 at 16:07 +0200, Markus Armbruster wrote:
> Maxim Levitsky <address@hidden> writes:
> 
> > On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote:
> > > Maxim Levitsky <address@hidden> writes:
> > > 
> > > > 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>
> > > 
> > > [...]
> > > > 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)
> > > 
> > > What checks excactly are disabled?
> > 
> > Ability to overwrite an used slot with a different password. 
> > If overwrite fails, the image won't be recoverable.
> > 
> > The safe way is to add a new slot, then erase the old
> > one, but this changes the slot where the password
> > is stored, unless this procedure is used twice
> 
> Would this be a useful addition to the doc comment?
> 
> > > > +# @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
> > > > +# @force:         Disable safety checks (use with care)
> > > 
> > > Likewise.
> > 
> > 1. Erase a slot which is already marked as
> > erased. Mostly harmless but pointless as well.
> > 
> > 2. Erase last keyslot. This irreversibly destroys
> > any ability to read the data from the device,
> > unless a backup of the header and the key material is
> > done prior. Still can be useful when it is desired to
> > erase the data fast.
> 
> Would this be a useful addition to the doc comment?
Yea, but since I'll will switch to the amend interface,
I'll leave it like that for now.


[...]


Best regards,
        Maxim Levitsky