| Message ID | 20190814202219.1870-8-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: luks/encrypted qcow2 key management | expand |
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 <mlevitsk@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.) 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' } } >
Maxim Levitsky <mlevitsk@redhat.com> 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 <mlevitsk@redhat.com> [...] > 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.
On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> 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 <mlevitsk@redhat.com> > > [...] > > 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 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? Uniting these means that I need to add some kind of 'action' to the options, which is kind of adding a subcommand to a qmp command, which is also feels kind of wrong. That is why internally this is implemented as one block driver callback, with action = {erase,update}, but qmp exposes two commands. I would personally prefer to have that erase field,and I would have to have it, if I switch to the amend interface. > > 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. > I kind of agree with that but The reason for that is that I designed that interface like that is to be not specific to luks. I pass the options structure down the stack till it reaches the luks driver where it can deal with it. If a new crypto driver is added, all you would have to do is to define new options in the json, and use them in the new crypto driver. The rest of the code doesn't know what is in that struct. Kind of the same as done with blockdev-create I guess. Thanks for the review, Best regards, Maxim Levitsky
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 <mlevitsk@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.) > > 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: 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. The reason for that was to save on code duplication internally. Internally (as in block device callback, and generic crypto code callback), both options are folded in one, with 'action' field to distinguish between them and that structure. If I use amend interface, I also would have to have some amend option that will tell to erase the key. It one of the things I wanted to ask you with that RFC, how would you solve this in single amend interface. > > Max > > > +{ 'struct': 'QCryptoEncryptionSetupOptions', > > + 'data': { '*key-secret': 'str', > > + '*old-key-secret': 'str', > > + '*slot': 'int', > > + '*iter-time': 'int' } } > > > > Best regards, Thanks for the review, Maxim Levitsky
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 <mlevitsk@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
Maxim Levitsky <mlevitsk@redhat.com> writes: > On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote: >> Maxim Levitsky <mlevitsk@redhat.com> 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 <mlevitsk@redhat.com> >> >> [...] >> > 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 placed it in a struct. > > So this struct is supposed to represent driver specific options, but > currently contains only luks options. Imagine we find driver DRV needs options. How would we extend your QAPI schema then? We can add DRV's options to this struct. QMP clients then must use only the LUKS members when the driver is actually LUKS, and only the DRV members when it's actually DRV. Works as long as all members are optional. Confusing, and ugly as sin. We can add a second struct for DRV's options. Call it QCryptoEncryptionSetupOptionsDRV. Add an optional parameter 'drv': 'QCryptoEncryptionSetupOptionsDRV' to x-blockdev-*-encryption, make existing parameter @options optional. Can't rename it to @luks (compatibility break). Probably want to rename QCryptoEncryptionSetupOptions to QCryptoEncryptionSetupOptionsLUKS. QMP clients then must use @options when the driver is actually LUKS, and @drv when it's actually DRV. Less confusing, still ugly. I'm not happy with either idea. Do you have a better one? >> > +# >> > +# @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 Let's scratch "currently". >> > +# 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? > > > Uniting these means that I need to add some kind of 'action' to the > options, which is kind of adding a subcommand to a qmp command, which is also feels > kind of wrong. > > That is why internally this is implemented as one block driver callback, > with action = {erase,update}, but qmp exposes two commands. Yes, multiplexed commands are ugly more often than not. > I would personally prefer to have that erase field,and I would have to have > it, if I switch to the amend interface. > > >> >> 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. >> > I kind of agree with that but The reason for that is that I designed that interface like that is to be not specific to luks. > > I pass the options structure down the stack till it reaches the luks driver where it can deal with > it. If a new crypto driver is added, all you would have to do is to define new options in the json, > and use them in the new crypto driver. The rest of the code doesn't know what is in that struct. > Kind of the same as done with blockdev-create I guess. Actually, blockdev-create is quite different in one important way: it uses a *union* for driver-specific arguments, not a struct. If we make QCryptoEncryptionSetupOptions ab union, like BlockdevCreateOptions is, then the way to grow it to more drivers is obvious. We can talk about nesting later.
On Thu, 2019-08-22 at 16:07 +0200, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > On Wed, 2019-08-21 at 13:47 +0200, Markus Armbruster wrote: > > > Maxim Levitsky <mlevitsk@redhat.com> 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 <mlevitsk@redhat.com> > > > > > > [...] > > > > 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
diff --git a/block/block-backend.c b/block/block-backend.c index 0056b526b8..1b75f28d0c 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2284,3 +2284,12 @@ const BdrvChild *blk_root(BlockBackend *blk) { return blk->root; } + +int blk_setup_encryption(BlockBackend *blk, + enum BlkSetupEncryptionAction action, + QCryptoEncryptionSetupOptions *options, + bool force, + Error **errp) +{ + return bdrv_setup_encryption(blk->root->bs, action, options, force, errp); +} diff --git a/block/io.c b/block/io.c index 06305c6ea6..50090afe68 100644 --- a/block/io.c +++ b/block/io.c @@ -3256,3 +3256,27 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, return tco.ret; } + + +int bdrv_setup_encryption(BlockDriverState *bs, + enum BlkSetupEncryptionAction action, + QCryptoEncryptionSetupOptions *options, + bool force, + Error **errp) +{ + Error *local_err = NULL; + int ret; + + if (!(bs->open_flags & BDRV_O_RDWR)) { + error_setg(errp, "Can't do key management on read only block device"); + return -ENOTSUP; + } + + ret = bs->drv->bdrv_setup_encryption(bs, action, options, force, + &local_err); + if (ret) { + error_propagate(errp, local_err); + return ret; + } + return 0; +} diff --git a/blockdev.c b/blockdev.c index 4d141e9a1f..27be251656 100644 --- a/blockdev.c +++ b/blockdev.c @@ -4563,6 +4563,46 @@ void qmp_block_latency_histogram_set( } } +void qmp_x_blockdev_update_encryption(const char *node_name, + bool has_force, bool force, + QCryptoEncryptionSetupOptions *options, + Error **errp) +{ + BlockDriverState *bs = bdrv_find_node(node_name); + Error *local_error = NULL; + + if (!bs) { + error_setg(errp, "Cannot find node %s", node_name); + return; + } + + if (bdrv_setup_encryption(bs, BLK_UPDATE_ENCRYPTION, options, + has_force ? force : false, &local_error)) { + error_propagate(errp, local_error); + } +} + + +void qmp_x_blockdev_erase_encryption(const char *node_name, + bool has_force, bool force, + QCryptoEncryptionSetupOptions *options, + Error **errp) +{ + BlockDriverState *bs = bdrv_find_node(node_name); + Error *local_error = NULL; + + if (!bs) { + error_setg(errp, "Cannot find node %s", node_name); + return; + } + + if (bdrv_setup_encryption(bs, BLK_ERASE_ENCRYPTION, options, + has_force ? force : false, &local_error)) { + error_propagate(errp, local_error); + } +} + + QemuOptsList qemu_common_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head), diff --git a/include/block/block.h b/include/block/block.h index 50a07c1c33..b55ef4c416 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -276,6 +276,12 @@ enum { DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH, }; +enum BlkSetupEncryptionAction { + BLK_UPDATE_ENCRYPTION, + BLK_ERASE_ENCRYPTION, + +}; + char *bdrv_perm_names(uint64_t perm); /* disk I/O throttling */ @@ -348,6 +354,12 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, Error **errp); +int bdrv_setup_encryption(BlockDriverState *bs, + enum BlkSetupEncryptionAction action, + QCryptoEncryptionSetupOptions *options, + bool force, + Error **errp); + int64_t bdrv_nb_sectors(BlockDriverState *bs); int64_t bdrv_getlength(BlockDriverState *bs); int64_t bdrv_get_allocated_file_size(BlockDriverState *bs); diff --git a/include/block/block_int.h b/include/block/block_int.h index 3aa1e832a8..64c71fe269 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -556,6 +556,16 @@ struct BlockDriver { void (*bdrv_unregister_buf)(BlockDriverState *bs, void *host); QLIST_ENTRY(BlockDriver) list; + + /* Manage encryption keys on the block device */ + int (*bdrv_setup_encryption)(BlockDriverState *bs, + enum BlkSetupEncryptionAction action, + QCryptoEncryptionSetupOptions *options, + bool force, + Error **errp); + + + /* Pointer to a NULL-terminated array of names of strong options * that can be specified for bdrv_open(). A strong option is one * that changes the data of a BDS. @@ -1271,4 +1281,5 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset, int refresh_total_sectors(BlockDriverState *bs, int64_t hint); + #endif /* BLOCK_INT_H */ diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 733c4957eb..18e98499fd 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -262,4 +262,11 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, const BdrvChild *blk_root(BlockBackend *blk); + +int blk_setup_encryption(BlockBackend *blk, + enum BlkSetupEncryptionAction action, + QCryptoEncryptionSetupOptions *options, + bool force, + Error **errp); + #endif 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 +# @force: Disable safety checks (use with care) +# @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. +# +# @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' } }
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 <mlevitsk@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(+)