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 <mlevi...@redhat.com> > > --- > > 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 :|