QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Fam Zheng" <fam@euphon.net>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	"Max Reitz" <mreitz@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev)
Date: Thu, 22 Aug 2019 16:07:24 +0200
Message-ID: <877e75cmzn.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <2ac0407fbbed8558dc22fc0b8a30ef77bddcea6f.camel@redhat.com>

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.


  reply index

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 20:22 [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring Maxim Levitsky
2019-08-20 16:38   ` Max Reitz
2019-08-22  0:05     ` Maxim Levitsky
2019-08-22 14:34       ` Max Reitz
2019-08-22 15:04         ` Maxim Levitsky
2019-08-21 15:39   ` Daniel P. Berrangé
2019-08-22  0:08     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 02/13] qcrypto-luks: " Maxim Levitsky
2019-08-15 21:40   ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-19 14:21     ` Maxim Levitsky
2019-08-22 10:29     ` Daniel P. Berrangé
2019-08-22 11:04       ` Maxim Levitsky
2019-08-22 11:10         ` Daniel P. Berrangé
2019-08-22 11:13           ` Maxim Levitsky
2019-08-20 17:36   ` [Qemu-devel] " Max Reitz
2019-08-21 23:59     ` Maxim Levitsky
2019-08-22 14:32       ` Max Reitz
2019-08-25 10:46         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions Maxim Levitsky
2019-08-20 18:01   ` Max Reitz
2019-08-21 22:43     ` Maxim Levitsky
2019-08-22 10:32       ` Daniel P. Berrangé
2019-08-22 10:57         ` Maxim Levitsky
2019-08-22 10:34       ` Daniel P. Berrangé
2019-08-25 14:11         ` Maxim Levitsky
2019-08-22 10:38   ` Daniel P. Berrangé
2019-08-25 14:09     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
2019-08-22 10:47   ` Daniel P. Berrangé
2019-08-25 14:30     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always Maxim Levitsky
2019-08-20 18:12   ` Max Reitz
2019-08-21 22:40     ` Maxim Levitsky
2019-08-22 10:49     ` Daniel P. Berrangé
2019-08-22 10:56       ` Maxim Levitsky
2019-08-25 15:31         ` Maxim Levitsky
2019-08-25 17:15           ` Maxim Levitsky
2019-08-27  8:55           ` Daniel P. Berrangé
2019-08-21 23:01   ` [Qemu-devel] [Qemu-block] " Nir Soffer
2019-08-21 23:11     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
2019-08-22 11:04   ` Daniel P. Berrangé
2019-08-25 15:40     ` Maxim Levitsky
2019-08-25 16:08       ` Maxim Levitsky
2019-08-26 13:31         ` Eric Blake
2019-08-26 13:39           ` Maxim Levitsky
2019-08-27  8:56         ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev) Maxim Levitsky
2019-08-20 18:27   ` Max Reitz
2019-08-21 22:32     ` Maxim Levitsky
2019-08-22 11:14     ` Daniel P. Berrangé
2019-08-21 11:47   ` Markus Armbruster
2019-08-21 22:24     ` Maxim Levitsky
2019-08-22 14:07       ` Markus Armbruster [this message]
2019-08-25 16:42         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 08/13] qcrypto: add the plumbing for encryption management Maxim Levitsky
2019-08-22 11:16   ` Daniel P. Berrangé
2019-08-22 11:47     ` Maxim Levitsky
2019-08-22 11:49       ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 09/13] qcrypto-luks: implement the encryption key management Maxim Levitsky
2019-08-22 11:27   ` Daniel P. Berrangé
2019-08-25 17:01     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 10/13] block/crypto: " Maxim Levitsky
2019-08-22 11:29   ` Daniel P. Berrangé
2019-08-22 11:36     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 11/13] block/qcow2: implement the encryption key managment Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 12/13] qemu-img: implement key management Maxim Levitsky
2019-08-20 18:29   ` Max Reitz
2019-08-21 22:33     ` Maxim Levitsky
2019-08-22 11:32     ` Daniel P. Berrangé
2019-08-22 14:42       ` Max Reitz
2019-08-25 17:04         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 13/13] iotests : add tests for encryption " Maxim Levitsky
2019-08-14 21:08 ` [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 " Eric Blake
2019-08-15  8:49   ` Maxim Levitsky
2019-08-15  9:10   ` Kevin Wolf
2019-08-15 14:18     ` Markus Armbruster
2019-08-15 14:44       ` Maxim Levitsky
2019-08-15 15:00         ` Eric Blake
2019-08-19 12:35           ` Maxim Levitsky
2019-08-21 11:31             ` Markus Armbruster
2019-08-21 13:22               ` Maxim Levitsky
2019-08-20 17:59 ` Max Reitz
2019-08-21 22:00   ` Maxim Levitsky
2019-08-22 11:35 ` Daniel P. Berrangé
2019-08-25 17:10   ` Maxim Levitsky

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877e75cmzn.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox