Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
On Thu, 2019-08-22 at 16:07 +0200, Markus Armbruster wrote:
> Maxim Levitsky writes:
>
> > On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote:
> > > Maxim Levitsky 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
> > >
> > > [...]
> > > > 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
Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
Maxim Levitsky writes:
> On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote:
>> Maxim Levitsky 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
>>
>> [...]
>> > 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?
>> > +# @options: Driver specific options
>> > +#
>> > +# Returns: @QCryptoKeyManageResult
>>
>> Doc comment claims the command returns something, even though it
>> doesn't. Please fix. Sadly, the doc generator fails to flag that.
> This is leftover, fixed now although most likely this interface will die.
> I was initially planning to return
> information on which slot was allocated when user left that
> decision to the driver.
>
>>
>> > +#
>> > +# Since: 4.2
>> > +##
>> > +{ 'command': 'x-blockdev-erase-encryption',
>> > + 'data': { 'node-name' : 'str',
>> > +'*force' : 'bool',
>> > +'options': 'QCryptoEncryptionSetupOptions' } }
Hmm, all members of @options are optional. If I don't want to specify
any of them, I still have to say "options": {}. Should @options be
optional, too?
Question is not relevant for x-blockdev-update-encryption, because
there, options.key-secret isn't actually optional. Correct?
>> > 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.
>>
>> Specific to which driver?
>
> This is the same issue, of not beeing able to detect an union.
>
> I was planning to have an union here where we could add
> add the driver specific options if we need to have another crypto driver,
> however since I discovered that union needs user to pass the driver name,
> I just
Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
On Tue, 2019-08-20 at 20:27 +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
> > ---
> > 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?
Because checkpatch.pl doesn't warn about this :-)
>
> > +# @force: Disable safety checks (use with care)
>
> I think being a bit more verbose wouldn’t hurt.
>
> (Same above.)
True about this - this is another reason this is RFC,
I honestly didn't finish the documentation,
since the sudden change to drop all of this interface.
>
> > +# @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.
This is because to be not luks specific we must use some kind of an union
which means that the user has to specify the driver which I didn't want to do.
Now all of you convinced me ( :-) ) to do this so this will be done when I
switch
to the amend interface.
>
> > +# @key-secret: the ID of a QCryptoSecret object providing the password
> > +# to add or to erase (optional for erase)
> > +#
> > +# @old-key-secret:
Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote:
> Maxim Levitsky 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
>
> [...]
> > 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
>
> > +# @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.
>
> > +# @options: Driver specific options
> > +#
> > +# Returns: @QCryptoKeyManageResult
>
> Doc comment claims the command returns something, even though it
> doesn't. Please fix. Sadly, the doc generator fails to flag that.
This is leftover, fixed now although most likely this interface will die.
I was initially planning to return
information on which slot was allocated when user left that
decision to the driver.
>
> > +#
> > +# 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.
>
> Specific to which driver?
This is the same issue, of not beeing able to detect an union.
I was planning to have an union here where we could add
add the driver specific options if we need to have another crypto driver,
however since I discovered that union needs user to pass the driver name,
I just placed it in a struct.
So this struct is supposed to represent driver specific options, but
currently contains only luks options.
>
> > +#
> > +# @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
Re: [Qemu-block] [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
Maxim Levitsky 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
[...]
> 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?
> +# @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.
> +# @options: Driver specific options
> +#
> +# Returns: @QCryptoKeyManageResult
Doc comment claims the command returns something, even though it
doesn't. Please fix. Sadly, the doc generator fails to flag that.
> +#
> +# 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.
Specific to which driver?
> +#
> +# @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
> +##
> +{ 'struct': 'QCryptoEncryptionSetupOptions',
> + 'data': { '*key-secret': 'str',
> +'*old-key-secret': 'str',
> +'*slot': 'int',
> +'*iter-time': 'int' } }
The two new commands have identical arguments. Some of them you factor
out into their own struct. Can you explain what makes them special?
The extra nesting on the wire is kind of ugly. We can talk about how to
avoid it once I understand why we want the extra struct.
