API definition for LUKS key management

API definition for LUKS key management

[Maxim Levitsky]

Hi!

I would like to discuss the API for LUKS key management.

First of all very brief overview of LUKS v1 format:

Each sector of the image is encrypted with same master key, which
is not stored directly on the disk.

Instead in the LUKS header we have 8 slots. Each slot optionally stores
an encrypted version of the master key, encrypted by the user password.
Knowing the password, you can retrieve the master key from the keyslot.
Slot can be marked as active or inactive, inactive slots are not considered
when opening the image.

In addition to that LUKS header has a hash of the master key, so that
you can check if the password 'opens' a keyslot by decrypting it
with given the password, and then checking if 
the hash of the decrypted master key candidate obtained matches the stored hash.

That basically means that you can have up to 8 different passwords that will
open a luks volume and you can change them as you wish without re-encrypting
everything.

Now for raw luks volumes you have cryptsetup which allows to manage these
keyslots, but we also have so called encrypted qcow2 format which
basically has the luks header, together with keyslots embedded, plus each
cluster is encrypted with the master key as in raw luks.
Cryptsetup doesn't support this, thus I implemented this in qemu block layer.

Link to bugzilla here: https://bugzilla.redhat.com/show_bug.cgi?id=1662412


Relevant to the API,
first of all qemu has the notion of amend (qemu-img amend), which allows
currently to change format specific extensions of qcow2.

Since luks, especially luks inside qcow2 is a format on its own, it fits to 
use that interface to change the 'format' options, in this case,
the encryption key slots.


There are the following requirements (they are 100% hardcoded, we might discuss
to drop some of these):


1. ability to add a new password to a free keyslot 
(best is to let api to pick a free keyslot)
Also user should not need to know all the passwords in existing keyslots.


2. ability to erase a keyslot, usually by giving the password that should be erased, and erasing all
the keyslots that match the password, or by giving a keyslot index.
This will usually be done after adding a new password.


3. Allow to do so online, that is while qemu is running, but also support offline management.
Note that online management is even useful for raw luks volumes, since its not safe
to run cryptsetup on them while qemu is using the images.


I implemented those requirements using the following interface.
(I have sent the patches already)

I will try to explain the interface with bunch of examples:


# adds a new password, defined by qemu secret 'sec0' to first unused slot
# give user a error if all keyslots are occupied
qemu-img amend --secret ... -o key-secret=sec1 image.luks


# erases all keyslots that can be opened by password that is contained in a qemu secret 'sec0'
# active=off means that the given password/keyslot won't be active after the operation
qemu-img amend --secret ... -o key-secret=sec0,active=off image.luks


# erase the slot 5 (this is more low level command, less expected to be used)
qemu-img amend --secret ... -o slot=5,active=off image.luks

# add new secret to slot 5 (will fail if the slot is already marked as active)
qemu-img amend --secret ... -o slot=5,key-secret=sec1 image.luks


This is basically it.

The full option syntax is as following:

active=on/off (optional, default to on) - toggles if we enabling a keyslot or are erasing it.

slot=number (optional, advanced option) - specifies which exactly slot to erase or which
slot to put the new key on

key-secret = id of the secret object - specifies the secret. when slot is enabled,
it will be put into the new slot. when disabling (erasing a keyslot), all keyslots
matching that secret will be erased. 
Specifying both key-secret and slot index is treated as error I think


As as very advanced option, --force is added to qemu-img to allow to do unsafe operation,
which in this case is removing last keyslot which will render the encrypted image useless.


In addition to that, QMP interface was added for online version of the above.
It is very similiar, but since we don't have blockdev-amend,
I added one and it has the following interface:



##
# @x-blockdev-amend:
#
# Starts a job to amend format specific options of an existing open block device.
# The job is automatically finalized, but a manual job-dismiss is required.
#
# @job-id:          Identifier for the newly created job.
#
# @node-name:       Name of the block node to work on
#
# @options:         Options (same as for image creation)
#
# @force:           Allow unsafe operations, format specific
#                   For luks that allows erase of the last active keyslot
#                   (permanent loss of data),
#                   and replacement of an active keyslot
#                   (possible loss of data if IO error happens)
#
# Since: 4.2
##
{ 'command': 'x-blockdev-amend',
  'data': { 'job-id': 'str',
            'node-name': 'str',
            'options': 'BlockdevCreateOptions',
            '*force': 'bool' } }



It takes the same BlockdevCreateOptions as blockdev-create (this is open to debate if to leave this as is)


BlockdevCreateOptionsLUKS (its parent QCryptoBlockCreateOptionsLUKS technically is extended in this way):


--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -190,6 +190,21 @@
 #                  Currently defaults to 'sha256'
 # @hash-alg: the master key hash algorithm
 #            Currently defaults to 'sha256'
+#
+# @active: Should the new secret be added (true) or erased (false)
+#          (amend only, since 4.2)
+#
+# @slot: The slot in which to put/erase the secret
+#        if not given, will select first free slot for secret addtion
+#        and erase all keyslots that match the given @key-secret for erase.
+#        except the last one
+#        (optional, since 4.2)
+#
+# @unlock-secret: The secret to use to unlock the image
+#        If not given, will use the secret that was used
+#        when opening the image.
+#        (optional, for amend only, since 4.2)
+#
 # @iter-time: number of milliseconds to spend in
 #             PBKDF passphrase processing. Currently defaults
 #             to 2000. (since 2.8)
@@ -201,7 +216,12 @@
             '*cipher-mode': 'QCryptoCipherMode',
             '*ivgen-alg': 'QCryptoIVGenAlgorithm',
             '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
+
             '*hash-alg': 'QCryptoHashAlgorithm',
+            '*active' : 'bool',
+            '*slot': 'int',
+            '*unlock-secret': 'str',
+
             '*iter-time': 'int'}}


Here note that key-secret is already present in the in api, and I am adding the 'slot','active' and 'unlock-secret'

'slot' can be also used for new created image to specify where to place the the secret.
'active' not allowed to be false for blockdev-create of an image and can be true/false for 'blockdev-amend'

'unlock-secret' (might be removed later) covers an corner case that is specific for online key management.
The case is that if the keyslot used to open the image in first place is removed, it can be used to specify
the password to retrieve the master key from one of existing keyslots, since the driver doesn't officially
keep the master key all the time (it can be in theory only loaded in hardware crypto device)

That is why for adding a new keyslot, the secret that was used to open the image is tried first, and if it
doesn't open a keyslot, the 'unlock-secret' can be used instead. This can be thought of as the 'current password'
that is need to update the password on many web forums.


One of the concerns that was raised during the review was that amend interface for luks that I propose is
different from the amend inteface used currently for qcow2.

qcow2 amend interface specifies all the format options, thus overwrites the existing options.
Thus it seems natural to make the luks amend interface work the same way, that it receive an array
of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
This does allow to add and erase the keyslots, but it doesn't allow:

   * add a password without knowing all other passwords that exist in existing keyslots
     this can be mitigated by specifying which keyslots to modify for example by omitting the
     keyslots that shouldn't be touched from the array (passing null placeholder instead)
     but then it already doesn't follow the 'specify all the options each time' principle.

   * erase all keyslots matching a password - this is really hard to do using this approach,
     unless we give user some kind of api to try each keyslot with given password,
     which is kind of ugly and might be racy as well.


So what do you think?

Best regards,
	Maxim Levitsky

Re: API definition for LUKS key management

[Daniel P. Berrangé]

On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> Hi!
> 
> I would like to discuss the API for LUKS key management.
> 
> First of all very brief overview of LUKS v1 format:
> 
> Each sector of the image is encrypted with same master key, which
> is not stored directly on the disk.
> 
> Instead in the LUKS header we have 8 slots. Each slot optionally stores
> an encrypted version of the master key, encrypted by the user password.
> Knowing the password, you can retrieve the master key from the keyslot.
> Slot can be marked as active or inactive, inactive slots are not considered
> when opening the image.
> 
> In addition to that LUKS header has a hash of the master key, so that
> you can check if the password 'opens' a keyslot by decrypting it
> with given the password, and then checking if 
> the hash of the decrypted master key candidate obtained matches the stored hash.
> 
> That basically means that you can have up to 8 different passwords that will
> open a luks volume and you can change them as you wish without re-encrypting
> everything.
> 
> Now for raw luks volumes you have cryptsetup which allows to manage these
> keyslots, but we also have so called encrypted qcow2 format which
> basically has the luks header, together with keyslots embedded, plus each
> cluster is encrypted with the master key as in raw luks.
> Cryptsetup doesn't support this, thus I implemented this in qemu block layer.

Even for raw luks volumes, the traditional "cryptsetup" tool is
undesirable. eg consider LUKS on an RBD or ISCSI volume where
you are using the in-QEMU RBD/ISCSI client. You don't want to
have to configure the host kernel client just to change the
keyslot info. You don't want to use the in-QEMU clients for
qemu-img.

> 
> Link to bugzilla here: https://bugzilla.redhat.com/show_bug.cgi?id=1662412
> 
> 
> Relevant to the API,
> first of all qemu has the notion of amend (qemu-img amend), which allows
> currently to change format specific extensions of qcow2.
> 
> Since luks, especially luks inside qcow2 is a format on its own, it fits to 
> use that interface to change the 'format' options, in this case,
> the encryption key slots.
> 
> 
> There are the following requirements (they are 100% hardcoded, we might discuss
> to drop some of these):
> 
> 
> 1. ability to add a new password to a free keyslot 
> (best is to let api to pick a free keyslot)
> Also user should not need to know all the passwords in existing keyslots.
> 
> 
> 2. ability to erase a keyslot, usually by giving the password that should be erased, and erasing all
> the keyslots that match the password, or by giving a keyslot index.
> This will usually be done after adding a new password.
> 
> 
> 3. Allow to do so online, that is while qemu is running, but also support offline management.
> Note that online management is even useful for raw luks volumes, since its not safe
> to run cryptsetup on them while qemu is using the images.
> 
> 
> I implemented those requirements using the following interface.
> (I have sent the patches already)
> 
> I will try to explain the interface with bunch of examples:
> 
> 
> # adds a new password, defined by qemu secret 'sec0' to first unused slot
> # give user a error if all keyslots are occupied
> qemu-img amend --secret ... -o key-secret=sec1 image.luks

I think you meant "--object secret,...." instead of "--secret ..."

Also, this example needs to have 2 secrets provided. The first
secret to unlock the image using the existing password, and the
second secret is the one being added.

> # erases all keyslots that can be opened by password that is contained in a qemu secret 'sec0'
> # active=off means that the given password/keyslot won't be active after the operation
> qemu-img amend --secret ... -o key-secret=sec0,active=off image.luks
> 
> 
> # erase the slot 5 (this is more low level command, less expected to be used)
> qemu-img amend --secret ... -o slot=5,active=off image.luks
> 
> # add new secret to slot 5 (will fail if the slot is already marked as active)
> qemu-img amend --secret ... -o slot=5,key-secret=sec1 image.luks

This also needs two secrets provideed.

> 
> 
> This is basically it.
> 
> The full option syntax is as following:
> 
> active=on/off (optional, default to on) - toggles if we enabling a keyslot or are erasing it.
> 
> slot=number (optional, advanced option) - specifies which exactly slot to erase or which
> slot to put the new key on
> 
> key-secret = id of the secret object - specifies the secret. when slot is enabled,
> it will be put into the new slot. when disabling (erasing a keyslot), all keyslots
> matching that secret will be erased. 
> Specifying both key-secret and slot index is treated as error I think
> 
> 
> As as very advanced option, --force is added to qemu-img to allow to do unsafe operation,
> which in this case is removing last keyslot which will render the encrypted image useless.
> 
> 
> In addition to that, QMP interface was added for online version of the above.
> It is very similiar, but since we don't have blockdev-amend,
> I added one and it has the following interface:
> 
> 
> 
> ##
> # @x-blockdev-amend:
> #
> # Starts a job to amend format specific options of an existing open block device.
> # The job is automatically finalized, but a manual job-dismiss is required.
> #
> # @job-id:          Identifier for the newly created job.
> #
> # @node-name:       Name of the block node to work on
> #
> # @options:         Options (same as for image creation)
> #
> # @force:           Allow unsafe operations, format specific
> #                   For luks that allows erase of the last active keyslot
> #                   (permanent loss of data),
> #                   and replacement of an active keyslot
> #                   (possible loss of data if IO error happens)
> #
> # Since: 4.2
> ##
> { 'command': 'x-blockdev-amend',
>   'data': { 'job-id': 'str',
>             'node-name': 'str',
>             'options': 'BlockdevCreateOptions',
>             '*force': 'bool' } }
> 
> 
> 
> It takes the same BlockdevCreateOptions as blockdev-create (this is open to debate if to leave this as is)
> 
> 
> BlockdevCreateOptionsLUKS (its parent QCryptoBlockCreateOptionsLUKS technically is extended in this way):
> 
> 
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -190,6 +190,21 @@
>  #                  Currently defaults to 'sha256'
>  # @hash-alg: the master key hash algorithm
>  #            Currently defaults to 'sha256'
> +#
> +# @active: Should the new secret be added (true) or erased (false)
> +#          (amend only, since 4.2)
> +#
> +# @slot: The slot in which to put/erase the secret
> +#        if not given, will select first free slot for secret addtion
> +#        and erase all keyslots that match the given @key-secret for erase.
> +#        except the last one
> +#        (optional, since 4.2)
> +#
> +# @unlock-secret: The secret to use to unlock the image
> +#        If not given, will use the secret that was used
> +#        when opening the image.
> +#        (optional, for amend only, since 4.2)
> +#
>  # @iter-time: number of milliseconds to spend in
>  #             PBKDF passphrase processing. Currently defaults
>  #             to 2000. (since 2.8)
> @@ -201,7 +216,12 @@
>              '*cipher-mode': 'QCryptoCipherMode',
>              '*ivgen-alg': 'QCryptoIVGenAlgorithm',
>              '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> +
>              '*hash-alg': 'QCryptoHashAlgorithm',
> +            '*active' : 'bool',
> +            '*slot': 'int',
> +            '*unlock-secret': 'str',
> +
>              '*iter-time': 'int'}}
> 
> 
> Here note that key-secret is already present in the in api, and I am adding the 'slot','active' and 'unlock-secret'
> 
> 'slot' can be also used for new created image to specify where to place the the secret.
> 'active' not allowed to be false for blockdev-create of an image and can be true/false for 'blockdev-amend'
> 
> 'unlock-secret' (might be removed later) covers an corner case that is specific for online key management.
> The case is that if the keyslot used to open the image in first place is removed, it can be used to specify
> the password to retrieve the master key from one of existing keyslots, since the driver doesn't officially
> keep the master key all the time (it can be in theory only loaded in hardware crypto device)
> 
> That is why for adding a new keyslot, the secret that was used to open the image is tried first, and if it
> doesn't open a keyslot, the 'unlock-secret' can be used instead. This can be thought of as the 'current password'
> that is need to update the password on many web forums.
> 
> 
> One of the concerns that was raised during the review was that amend interface for luks that I propose is
> different from the amend inteface used currently for qcow2.
> 
> qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> This does allow to add and erase the keyslots, but it doesn't allow:
> 
>    * add a password without knowing all other passwords that exist in existing keyslots
>      this can be mitigated by specifying which keyslots to modify for example by omitting the
>      keyslots that shouldn't be touched from the array (passing null placeholder instead)
>      but then it already doesn't follow the 'specify all the options each time' principle.

I think this is highly undesirable, as we must not assume that the
mgmt app has access to all the passwords currently set.

The two key use cases for having multiple key slots are

  - To enable a two-phase change of passwords to ensure new password
    is safely written out before erasing the old password
    
  - To allow for multiple access passwords with different controls
    or access to when each password is made available.

    eg each VM may have a separate "backup password" securely
    stored off host that is only made available for use when
    doing disaster recovery.

the second use case is doomed if you need to always provide all
current passwords when changing any key slots.


>    * erase all keyslots matching a password - this is really hard to do using this approach,
>      unless we give user some kind of api to try each keyslot with given password,
>      which is kind of ugly and might be racy as well.

> So what do you think?

The point of using "amend" is that we already have some of the boilerplate
supporting framework around that, so it saves effort for both QEMU and
our users. If the semantics of "amend" don't fit nicely though, then the
benefit of re-using "amend" is cancelled out and we should go back to
considering a separate "key-manage" command.

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 :|

Re: API definition for LUKS key management

[Kevin Wolf]

Am 11.11.2019 um 19:34 hat Daniel P. Berrangé geschrieben:
> On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> > One of the concerns that was raised during the review was that amend interface for luks that I propose is
> > different from the amend inteface used currently for qcow2.
> > 
> > qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> > Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> > of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> > This does allow to add and erase the keyslots, but it doesn't allow:
> > 
> >    * add a password without knowing all other passwords that exist in existing keyslots
> >      this can be mitigated by specifying which keyslots to modify for example by omitting the
> >      keyslots that shouldn't be touched from the array (passing null placeholder instead)
> >      but then it already doesn't follow the 'specify all the options each time' principle.
> 
> I think this is highly undesirable, as we must not assume that the
> mgmt app has access to all the passwords currently set.

And I think this shows the problem that we realy have with the crypto
driver and amend: For every other driver, if you must, you can query the
current settings and just write them back.

The difference here is that crypto doesn't allow to directly query or
specify the content of some options (the keyslots), but provides only a
way to derives that content from a secret, and obviously there is no way
back from the stored data to the secret (that's what it's for).

I think we have two options here:

1. Add a special "don't touch this" value for keyslots. Normally, just
   leaving out the value would be suitable syntax for this. Here,
   however, we have a list of keyslots, so we can't leave anything out.

   We could use something like an alternate between str (new secret ID),
   null (erase keyslot) and empty dict (leave it alone) - the latter
   feels a bit hackish, but maybe it's not too bad. If the list is
   shorter than 8 entries, the rest is assumed to mean "leave it alone",
   too.

2. Allow to query and set the raw key, which doesn't require a password

> The two key use cases for having multiple key slots are
> 
>   - To enable a two-phase change of passwords to ensure new password
>     is safely written out before erasing the old password
>     
>   - To allow for multiple access passwords with different controls
>     or access to when each password is made available.
> 
>     eg each VM may have a separate "backup password" securely
>     stored off host that is only made available for use when
>     doing disaster recovery.
> 
> the second use case is doomed if you need to always provide all
> current passwords when changing any key slots.

That providing all current passwords doesn't work is obvious.

> >    * erase all keyslots matching a password - this is really hard to do using this approach,
> >      unless we give user some kind of api to try each keyslot with given password,
> >      which is kind of ugly and might be racy as well.
> 
> > So what do you think?
> 
> The point of using "amend" is that we already have some of the boilerplate
> supporting framework around that, so it saves effort for both QEMU and
> our users. If the semantics of "amend" don't fit nicely though, then the
> benefit of re-using "amend" is cancelled out and we should go back to
> considering a separate "key-manage" command.

This wouldn't solve the fundamental problem that the crypto block
driver, as it currently is, isn't able to provide a blockdev-amend
callback. It's worse for qcow2 because qcow2 already implements amend.

I think we need to find a solution for the amend API.

Kevin

Re: API definition for LUKS key management

[Daniel P. Berrangé]

On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> I will try to explain the interface with bunch of examples:

I want to fill in equiv examples from cryptsetup for sake
of comparison

> # adds a new password, defined by qemu secret 'sec0' to first unused slot
> # give user a error if all keyslots are occupied
> qemu-img amend --secret ... -o key-secret=sec1 image.luks

  cryptsetup luksAddKey --key-file currentkey.txt \
                        --new-key-file newkey.txt \
                        /dev/mapper/foo

> # erases all keyslots that can be opened by password that is contained in a qemu secret 'sec0'
> # active=off means that the given password/keyslot won't be active after the operation
> qemu-img amend --secret ... -o key-secret=sec0,active=off image.luks


  cryptsetup luksRemoveKey --key-file currentkey.txt \
                           /dev/mapper/foo

> # erase the slot 5 (this is more low level command, less expected to be used)
> qemu-img amend --secret ... -o slot=5,active=off image.luks

  cryptsetup luksKillSlot /dev/mapper/foo 5

> # add new secret to slot 5 (will fail if the slot is already marked as active)
> qemu-img amend --secret ... -o slot=5,key-secret=sec1 image.luks

  cryptsetup luksAddKey --key-file currentkey.txt \
                        --new-key-file newkey.txt \
			--key-slot 5 \
                        /dev/mapper/foo

They look very different in syntax because they are taking two differnet
approaches.

The cryptsetup approach is functional, where the user states what action
they want performed.

The qemu-img amend approach is declarative, where the user expresses what
end state they want the image to be in.

The challenge is that the qemu-img proposal isn't fully declarative as
we are only partially expressing the end state, attempting to leave other
slots unspecified. This is good as it means we don't have to specify a
huge amount of data on the CLI. This is bad as makes it less obvious
what actual effects of the commands will be and I feel users will end
up needing to read the manual every time to re-learn it.

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 :|

Re: API definition for LUKS key management

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4954 bytes --]

On 12.11.19 10:12, Kevin Wolf wrote:
> Am 11.11.2019 um 19:34 hat Daniel P. Berrangé geschrieben:
>> On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
>>> One of the concerns that was raised during the review was that amend interface for luks that I propose is
>>> different from the amend inteface used currently for qcow2.
>>>
>>> qcow2 amend interface specifies all the format options, thus overwrites the existing options.
>>> Thus it seems natural to make the luks amend interface work the same way, that it receive an array
>>> of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
>>> This does allow to add and erase the keyslots, but it doesn't allow:
>>>
>>>    * add a password without knowing all other passwords that exist in existing keyslots
>>>      this can be mitigated by specifying which keyslots to modify for example by omitting the
>>>      keyslots that shouldn't be touched from the array (passing null placeholder instead)
>>>      but then it already doesn't follow the 'specify all the options each time' principle.
>>
>> I think this is highly undesirable, as we must not assume that the
>> mgmt app has access to all the passwords currently set.
> 
> And I think this shows the problem that we realy have with the crypto
> driver and amend: For every other driver, if you must, you can query the
> current settings and just write them back.
> 
> The difference here is that crypto doesn't allow to directly query or
> specify the content of some options (the keyslots), but provides only a
> way to derives that content from a secret, and obviously there is no way
> back from the stored data to the secret (that's what it's for).
> 
> I think we have two options here:
> 
> 1. Add a special "don't touch this" value for keyslots. Normally, just
>    leaving out the value would be suitable syntax for this. Here,
>    however, we have a list of keyslots, so we can't leave anything out.
> 
>    We could use something like an alternate between str (new secret ID),
>    null (erase keyslot) and empty dict (leave it alone) - the latter
>    feels a bit hackish, but maybe it's not too bad.

I thought of something similar, but how would that look on the command line?

Though I suppose if the worst thing were how it looks on the command
line, we could introduce a new qemu-img subcommand that then internally
translates into the right amend syntax.

>    If the list is
>    shorter than 8 entries, the rest is assumed to mean "leave it alone",
>    too.
> 
> 2. Allow to query and set the raw key, which doesn't require a password
> 
>> The two key use cases for having multiple key slots are
>>
>>   - To enable a two-phase change of passwords to ensure new password
>>     is safely written out before erasing the old password
>>     
>>   - To allow for multiple access passwords with different controls
>>     or access to when each password is made available.
>>
>>     eg each VM may have a separate "backup password" securely
>>     stored off host that is only made available for use when
>>     doing disaster recovery.
>>
>> the second use case is doomed if you need to always provide all
>> current passwords when changing any key slots.
> 
> That providing all current passwords doesn't work is obvious.
> 
>>>    * erase all keyslots matching a password - this is really hard to do using this approach,
>>>      unless we give user some kind of api to try each keyslot with given password,
>>>      which is kind of ugly and might be racy as well.
>>
>>> So what do you think?
>>
>> The point of using "amend" is that we already have some of the boilerplate
>> supporting framework around that, so it saves effort for both QEMU and
>> our users. If the semantics of "amend" don't fit nicely though, then the
>> benefit of re-using "amend" is cancelled out and we should go back to
>> considering a separate "key-manage" command.
> 
> This wouldn't solve the fundamental problem that the crypto block
> driver, as it currently is, isn't able to provide a blockdev-amend
> callback. It's worse for qcow2 because qcow2 already implements amend.

Hm, well, I would have assumed this is only bad on the premise that we
want to have amend be complete at some point.  Do we?

While I do think it might be nice to be able to change e.g. cluster_size
especially for the upcoming subcluster extension (in addition to
enabling subclusters on an existing image), I seriously doubt anyone’s
going to implement it.  (Maybe enabling subclusters, but not changing
cluster_size.)

> I think we need to find a solution for the amend API.

I do think it’s weird to look for non-amend solutions when it clearly
looks like an amend problem, but OTOH I don’t think it would be that bad
to disregard amend.  (Provided there are good reasons for disregarding it.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: API definition for LUKS key management

[Daniel P. Berrangé]

On Tue, Nov 12, 2019 at 10:12:45AM +0100, Kevin Wolf wrote:
> Am 11.11.2019 um 19:34 hat Daniel P. Berrangé geschrieben:
> > On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> > > One of the concerns that was raised during the review was that amend interface for luks that I propose is
> > > different from the amend inteface used currently for qcow2.
> > > 
> > > qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> > > Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> > > of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> > > This does allow to add and erase the keyslots, but it doesn't allow:
> > > 
> > >    * add a password without knowing all other passwords that exist in existing keyslots
> > >      this can be mitigated by specifying which keyslots to modify for example by omitting the
> > >      keyslots that shouldn't be touched from the array (passing null placeholder instead)
> > >      but then it already doesn't follow the 'specify all the options each time' principle.
> > 
> > I think this is highly undesirable, as we must not assume that the
> > mgmt app has access to all the passwords currently set.
> 
> And I think this shows the problem that we realy have with the crypto
> driver and amend: For every other driver, if you must, you can query the
> current settings and just write them back.
> 
> The difference here is that crypto doesn't allow to directly query or
> specify the content of some options (the keyslots), but provides only a
> way to derives that content from a secret, and obviously there is no way
> back from the stored data to the secret (that's what it's for).
> 
> I think we have two options here:
> 
> 1. Add a special "don't touch this" value for keyslots. Normally, just
>    leaving out the value would be suitable syntax for this. Here,
>    however, we have a list of keyslots, so we can't leave anything out.
> 
>    We could use something like an alternate between str (new secret ID),
>    null (erase keyslot) and empty dict (leave it alone) - the latter
>    feels a bit hackish, but maybe it's not too bad. If the list is
>    shorter than 8 entries, the rest is assumed to mean "leave it alone",
>    too.

I'd be very wary of having a "null" vs "empty dict" distinction to
mean "erase" vs "don't touch".

It feels like that is designed to maximise the chances of someone
shooting themselves in the foot by accidentally using "null" instead
of an "empty dict".

The reason for the use of "active=yes" / "active=no" is because that
was reasonable explicit about wanting to erase a keyslot, and it does
does actually map to the key slot on disk which has an "active" field
taking two magic values.

> 2. Allow to query and set the raw key, which doesn't require a password

I don't think I understand what you mean here. If you don't have a
password the only change you can make is to erase key slots.

> > >    * erase all keyslots matching a password - this is really hard to do using this approach,
> > >      unless we give user some kind of api to try each keyslot with given password,
> > >      which is kind of ugly and might be racy as well.
> > 
> > > So what do you think?
> > 
> > The point of using "amend" is that we already have some of the boilerplate
> > supporting framework around that, so it saves effort for both QEMU and
> > our users. If the semantics of "amend" don't fit nicely though, then the
> > benefit of re-using "amend" is cancelled out and we should go back to
> > considering a separate "key-manage" command.
> 
> This wouldn't solve the fundamental problem that the crypto block
> driver, as it currently is, isn't able to provide a blockdev-amend
> callback. It's worse for qcow2 because qcow2 already implements amend.
> 
> I think we need to find a solution for the amend API.


BTW, looking forward to the future, if we ever implement LUKS version 2
support there are a bunch more things can be tweaked at runtime. Most
notable is that it is possible to change the master key, and change the
encryption algorithm choices. Both of these then need to trigger a bulk
re-encrypt of the entire disk contents, which takes a long time.

I doubt we'll do this in the near term, but we should consider how this
might fit into whatever scheme we pick for updates.

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 :|

Re: API definition for LUKS key management

[Maxim Levitsky]

On Mon, 2019-11-11 at 18:34 +0000, Daniel P. Berrangé wrote:
> On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> > Hi!
> > 
> > I would like to discuss the API for LUKS key management.
> > 
> > First of all very brief overview of LUKS v1 format:
> > 
> > Each sector of the image is encrypted with same master key, which
> > is not stored directly on the disk.
> > 
> > Instead in the LUKS header we have 8 slots. Each slot optionally stores
> > an encrypted version of the master key, encrypted by the user password.
> > Knowing the password, you can retrieve the master key from the keyslot.
> > Slot can be marked as active or inactive, inactive slots are not considered
> > when opening the image.
> > 
> > In addition to that LUKS header has a hash of the master key, so that
> > you can check if the password 'opens' a keyslot by decrypting it
> > with given the password, and then checking if 
> > the hash of the decrypted master key candidate obtained matches the stored hash.
> > 
> > That basically means that you can have up to 8 different passwords that will
> > open a luks volume and you can change them as you wish without re-encrypting
> > everything.
> > 
> > Now for raw luks volumes you have cryptsetup which allows to manage these
> > keyslots, but we also have so called encrypted qcow2 format which
> > basically has the luks header, together with keyslots embedded, plus each
> > cluster is encrypted with the master key as in raw luks.
> > Cryptsetup doesn't support this, thus I implemented this in qemu block layer.
> 
> Even for raw luks volumes, the traditional "cryptsetup" tool is
> undesirable. eg consider LUKS on an RBD or ISCSI volume where
> you are using the in-QEMU RBD/ISCSI client. You don't want to
> have to configure the host kernel client just to change the
> keyslot info. You don't want to use the in-QEMU clients for
> qemu-img.

I didn't thought about it. This is a very good point!

> 
> > 
> > Link to bugzilla here: https://bugzilla.redhat.com/show_bug.cgi?id=1662412
> > 
> > 
> > Relevant to the API,
> > first of all qemu has the notion of amend (qemu-img amend), which allows
> > currently to change format specific extensions of qcow2.
> > 
> > Since luks, especially luks inside qcow2 is a format on its own, it fits to 
> > use that interface to change the 'format' options, in this case,
> > the encryption key slots.
> > 
> > 
> > There are the following requirements (they are 100% hardcoded, we might discuss
> > to drop some of these):
> > 
> > 
> > 1. ability to add a new password to a free keyslot 
> > (best is to let api to pick a free keyslot)
> > Also user should not need to know all the passwords in existing keyslots.
> > 
> > 
> > 2. ability to erase a keyslot, usually by giving the password that should be erased, and erasing all
> > the keyslots that match the password, or by giving a keyslot index.
> > This will usually be done after adding a new password.
> > 
> > 
> > 3. Allow to do so online, that is while qemu is running, but also support offline management.
> > Note that online management is even useful for raw luks volumes, since its not safe
> > to run cryptsetup on them while qemu is using the images.
> > 
> > 
> > I implemented those requirements using the following interface.
> > (I have sent the patches already)
> > 
> > I will try to explain the interface with bunch of examples:
> > 
> > 
> > # adds a new password, defined by qemu secret 'sec0' to first unused slot
> > # give user a error if all keyslots are occupied
> > qemu-img amend --secret ... -o key-secret=sec1 image.luks
> 
> I think you meant "--object secret,...." instead of "--secret ..."
> 
True, sorry about that!

> Also, this example needs to have 2 secrets provided. The first
> secret to unlock the image using the existing password, and the
> second secret is the one being added.
> 
> > # erases all keyslots that can be opened by password that is contained in a qemu secret 'sec0'
> > # active=off means that the given password/keyslot won't be active after the operation
> > qemu-img amend --secret ... -o key-secret=sec0,active=off image.luks
> > 
> > 
> > # erase the slot 5 (this is more low level command, less expected to be used)
> > qemu-img amend --secret ... -o slot=5,active=off image.luks
> > 
> > # add new secret to slot 5 (will fail if the slot is already marked as active)
> > qemu-img amend --secret ... -o slot=5,key-secret=sec1 image.luks
> 
> This also needs two secrets provideed.
> 
> > 
> > 
> > This is basically it.
> > 
> > The full option syntax is as following:
> > 
> > active=on/off (optional, default to on) - toggles if we enabling a keyslot or are erasing it.
> > 
> > slot=number (optional, advanced option) - specifies which exactly slot to erase or which
> > slot to put the new key on
> > 
> > key-secret = id of the secret object - specifies the secret. when slot is enabled,
> > it will be put into the new slot. when disabling (erasing a keyslot), all keyslots
> > matching that secret will be erased. 
> > Specifying both key-secret and slot index is treated as error I think
> > 
> > 
> > As as very advanced option, --force is added to qemu-img to allow to do unsafe operation,
> > which in this case is removing last keyslot which will render the encrypted image useless.
> > 
> > 
> > In addition to that, QMP interface was added for online version of the above.
> > It is very similiar, but since we don't have blockdev-amend,
> > I added one and it has the following interface:
> > 
> > 
> > 
> > ##
> > # @x-blockdev-amend:
> > #
> > # Starts a job to amend format specific options of an existing open block device.
> > # The job is automatically finalized, but a manual job-dismiss is required.
> > #
> > # @job-id:          Identifier for the newly created job.
> > #
> > # @node-name:       Name of the block node to work on
> > #
> > # @options:         Options (same as for image creation)
> > #
> > # @force:           Allow unsafe operations, format specific
> > #                   For luks that allows erase of the last active keyslot
> > #                   (permanent loss of data),
> > #                   and replacement of an active keyslot
> > #                   (possible loss of data if IO error happens)
> > #
> > # Since: 4.2
> > ##
> > { 'command': 'x-blockdev-amend',
> >   'data': { 'job-id': 'str',
> >             'node-name': 'str',
> >             'options': 'BlockdevCreateOptions',
> >             '*force': 'bool' } }
> > 
> > 
> > 
> > It takes the same BlockdevCreateOptions as blockdev-create (this is open to debate if to leave this as is)
> > 
> > 
> > BlockdevCreateOptionsLUKS (its parent QCryptoBlockCreateOptionsLUKS technically is extended in this way):
> > 
> > 
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -190,6 +190,21 @@
> >  #                  Currently defaults to 'sha256'
> >  # @hash-alg: the master key hash algorithm
> >  #            Currently defaults to 'sha256'
> > +#
> > +# @active: Should the new secret be added (true) or erased (false)
> > +#          (amend only, since 4.2)
> > +#
> > +# @slot: The slot in which to put/erase the secret
> > +#        if not given, will select first free slot for secret addtion
> > +#        and erase all keyslots that match the given @key-secret for erase.
> > +#        except the last one
> > +#        (optional, since 4.2)
> > +#
> > +# @unlock-secret: The secret to use to unlock the image
> > +#        If not given, will use the secret that was used
> > +#        when opening the image.
> > +#        (optional, for amend only, since 4.2)
> > +#
> >  # @iter-time: number of milliseconds to spend in
> >  #             PBKDF passphrase processing. Currently defaults
> >  #             to 2000. (since 2.8)
> > @@ -201,7 +216,12 @@
> >              '*cipher-mode': 'QCryptoCipherMode',
> >              '*ivgen-alg': 'QCryptoIVGenAlgorithm',
> >              '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> > +
> >              '*hash-alg': 'QCryptoHashAlgorithm',
> > +            '*active' : 'bool',
> > +            '*slot': 'int',
> > +            '*unlock-secret': 'str',
> > +
> >              '*iter-time': 'int'}}
> > 
> > 
> > Here note that key-secret is already present in the in api, and I am adding the 'slot','active' and 'unlock-secret'
> > 
> > 'slot' can be also used for new created image to specify where to place the the secret.
> > 'active' not allowed to be false for blockdev-create of an image and can be true/false for 'blockdev-amend'
> > 
> > 'unlock-secret' (might be removed later) covers an corner case that is specific for online key management.
> > The case is that if the keyslot used to open the image in first place is removed, it can be used to specify
> > the password to retrieve the master key from one of existing keyslots, since the driver doesn't officially
> > keep the master key all the time (it can be in theory only loaded in hardware crypto device)
> > 
> > That is why for adding a new keyslot, the secret that was used to open the image is tried first, and if it
> > doesn't open a keyslot, the 'unlock-secret' can be used instead. This can be thought of as the 'current password'
> > that is need to update the password on many web forums.
> > 
> > 
> > One of the concerns that was raised during the review was that amend interface for luks that I propose is
> > different from the amend inteface used currently for qcow2.
> > 
> > qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> > Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> > of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> > This does allow to add and erase the keyslots, but it doesn't allow:
> > 
> >    * add a password without knowing all other passwords that exist in existing keyslots
> >      this can be mitigated by specifying which keyslots to modify for example by omitting the
> >      keyslots that shouldn't be touched from the array (passing null placeholder instead)
> >      but then it already doesn't follow the 'specify all the options each time' principle.
> 
> I think this is highly undesirable, as we must not assume that the
> mgmt app has access to all the passwords currently set.
> 
> The two key use cases for having multiple key slots are
> 
>   - To enable a two-phase change of passwords to ensure new password
>     is safely written out before erasing the old password
>     
>   - To allow for multiple access passwords with different controls
>     or access to when each password is made available.
> 
>     eg each VM may have a separate "backup password" securely
>     stored off host that is only made available for use when
>     doing disaster recovery.
> 
> the second use case is doomed if you need to always provide all
> current passwords when changing any key slots.
Fully agree, and thanks for these examples!


> 
> 
> >    * erase all keyslots matching a password - this is really hard to do using this approach,
> >      unless we give user some kind of api to try each keyslot with given password,
> >      which is kind of ugly and might be racy as well.
> > So what do you think?
> 
> The point of using "amend" is that we already have some of the boilerplate
> supporting framework around that, so it saves effort for both QEMU and
> our users. If the semantics of "amend" don't fit nicely though, then the
> benefit of re-using "amend" is cancelled out and we should go back to
> considering a separate "key-manage" command.

I guess we should anyway use amend interface, while updating its definition
a bit to suit the broader requirements of the drivers, e.g luks.

I see the amend interface being like generic 'edit the device options' thing,
which is maybe better that adding device specific commands like add-key,remove-key,
etc. No strong opinion on this though.


Best regards,
	Maxim Levitsky

Re: API definition for LUKS key management

[Maxim Levitsky]

On Tue, 2019-11-12 at 11:02 +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 12, 2019 at 10:12:45AM +0100, Kevin Wolf wrote:
> > Am 11.11.2019 um 19:34 hat Daniel P. Berrangé geschrieben:
> > > On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> > > > One of the concerns that was raised during the review was that amend interface for luks that I propose is
> > > > different from the amend inteface used currently for qcow2.
> > > > 
> > > > qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> > > > Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> > > > of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> > > > This does allow to add and erase the keyslots, but it doesn't allow:
> > > > 
> > > >    * add a password without knowing all other passwords that exist in existing keyslots
> > > >      this can be mitigated by specifying which keyslots to modify for example by omitting the
> > > >      keyslots that shouldn't be touched from the array (passing null placeholder instead)
> > > >      but then it already doesn't follow the 'specify all the options each time' principle.
> > > 
> > > I think this is highly undesirable, as we must not assume that the
> > > mgmt app has access to all the passwords currently set.
> > 
> > And I think this shows the problem that we realy have with the crypto
> > driver and amend: For every other driver, if you must, you can query the
> > current settings and just write them back.
> > 
> > The difference here is that crypto doesn't allow to directly query or
> > specify the content of some options (the keyslots), but provides only a
> > way to derives that content from a secret, and obviously there is no way
> > back from the stored data to the secret (that's what it's for).
> > 
> > I think we have two options here:
> > 
> > 1. Add a special "don't touch this" value for keyslots. Normally, just
> >    leaving out the value would be suitable syntax for this. Here,
> >    however, we have a list of keyslots, so we can't leave anything out.
> > 
> >    We could use something like an alternate between str (new secret ID),
> >    null (erase keyslot) and empty dict (leave it alone) - the latter
> >    feels a bit hackish, but maybe it's not too bad. If the list is
> >    shorter than 8 entries, the rest is assumed to mean "leave it alone",
> >    too.
> 
> I'd be very wary of having a "null" vs "empty dict" distinction to
> mean "erase" vs "don't touch".
> 
> It feels like that is designed to maximise the chances of someone
> shooting themselves in the foot by accidentally using "null" instead
> of an "empty dict".
> 
> The reason for the use of "active=yes" / "active=no" is because that
> was reasonable explicit about wanting to erase a keyslot, and it does
> does actually map to the key slot on disk which has an "active" field
> taking two magic values.
> 
> > 2. Allow to query and set the raw key, which doesn't require a password
> 
> I don't think I understand what you mean here. If you don't have a
> password the only change you can make is to erase key slots.
Well in the theory the keyslot has the hash of the password, the salt, the hash function
iteration count and the active field. 

In theory you can let the user read these values directly and write them back 
as is without knowing the password.
This is very ugly IMHO but will fit the classical amend definition.


> 
> > > >    * erase all keyslots matching a password - this is really hard to do using this approach,
> > > >      unless we give user some kind of api to try each keyslot with given password,
> > > >      which is kind of ugly and might be racy as well.
> > > > So what do you think?
> > > 
> > > The point of using "amend" is that we already have some of the boilerplate
> > > supporting framework around that, so it saves effort for both QEMU and
> > > our users. If the semantics of "amend" don't fit nicely though, then the
> > > benefit of re-using "amend" is cancelled out and we should go back to
> > > considering a separate "key-manage" command.
> > 
> > This wouldn't solve the fundamental problem that the crypto block
> > driver, as it currently is, isn't able to provide a blockdev-amend
> > callback. It's worse for qcow2 because qcow2 already implements amend.
> > 
> > I think we need to find a solution for the amend API.
I also think so. Amend interface can be *ahem* amended to be more generic :-)
Currently it is designed for just one use case.

> 
> 
> BTW, looking forward to the future, if we ever implement LUKS version 2
> support there are a bunch more things can be tweaked at runtime. Most
> notable is that it is possible to change the master key, and change the
> encryption algorithm choices. Both of these then need to trigger a bulk
> re-encrypt of the entire disk contents, which takes a long time.
This is good to know, and would mean that I was right to make blockdev-amend
a block job.

This also means that when user wants to tweak some setting, he might not
want to pass all the keyslots again.

BTW, I guess that to change the master key, user ought to know all passwords,
since each keyslot is basically an encrypted master key. I didn't read the LUKS v2
spec though.


> 
> I doubt we'll do this in the near term, but we should consider how this
> might fit into whatever scheme we pick for updates.
We might have to since LUKS v2 becomes default in more and more distros,
so users will eventually expect it be used by us as well.


Best regards,
	Maxim Levitsky

Re: API definition for LUKS key management

[Maxim Levitsky]

On Tue, 2019-11-12 at 10:12 +0100, Kevin Wolf wrote:
> Am 11.11.2019 um 19:34 hat Daniel P. Berrangé geschrieben:
> > On Mon, Nov 11, 2019 at 05:58:20PM +0200, Maxim Levitsky wrote:
> > > One of the concerns that was raised during the review was that amend interface for luks that I propose is
> > > different from the amend inteface used currently for qcow2.
> > > 
> > > qcow2 amend interface specifies all the format options, thus overwrites the existing options.
> > > Thus it seems natural to make the luks amend interface work the same way, that it receive an array
> > > of 8 slots, and for each slot specify if it is active, and if true what password to put in it.
> > > This does allow to add and erase the keyslots, but it doesn't allow:
> > > 
> > >    * add a password without knowing all other passwords that exist in existing keyslots
> > >      this can be mitigated by specifying which keyslots to modify for example by omitting the
> > >      keyslots that shouldn't be touched from the array (passing null placeholder instead)
> > >      but then it already doesn't follow the 'specify all the options each time' principle.
> > 
> > I think this is highly undesirable, as we must not assume that the
> > mgmt app has access to all the passwords currently set.
> 
> And I think this shows the problem that we realy have with the crypto
> driver and amend: For every other driver, if you must, you can query the
> current settings and just write them back.
> 
> The difference here is that crypto doesn't allow to directly query or
> specify the content of some options (the keyslots), but provides only a
> way to derives that content from a secret, and obviously there is no way
> back from the stored data to the secret (that's what it's for).
> 
> I think we have two options here:
> 
> 1. Add a special "don't touch this" value for keyslots. Normally, just
>    leaving out the value would be suitable syntax for this. Here,
>    however, we have a list of keyslots, so we can't leave anything out.
> 
>    We could use something like an alternate between str (new secret ID),
>    null (erase keyslot) and empty dict (leave it alone) - the latter
>    feels a bit hackish, but maybe it's not too bad. If the list is
>    shorter than 8 entries, the rest is assumed to mean "leave it alone",
>    too.
> 
> 2. Allow to query and set the raw key, which doesn't require a password
> 
> > The two key use cases for having multiple key slots are
> > 
> >   - To enable a two-phase change of passwords to ensure new password
> >     is safely written out before erasing the old password
> >     
> >   - To allow for multiple access passwords with different controls
> >     or access to when each password is made available.
> > 
> >     eg each VM may have a separate "backup password" securely
> >     stored off host that is only made available for use when
> >     doing disaster recovery.
> > 
> > the second use case is doomed if you need to always provide all
> > current passwords when changing any key slots.
> 
> That providing all current passwords doesn't work is obvious.

I also want to *emphasise* that not being able to provide all the keyslots
is the smaller problem here, since it is relatively easy to omit slots that
should be left untouched.
The bigger problem is supporting the 'erase all keyslots that match the password'
That doesn't fit into current amend definition at all.

> 
> > >    * erase all keyslots matching a password - this is really hard to do using this approach,
> > >      unless we give user some kind of api to try each keyslot with given password,
> > >      which is kind of ugly and might be racy as well.
> > > So what do you think?
> > 
> > The point of using "amend" is that we already have some of the boilerplate
> > supporting framework around that, so it saves effort for both QEMU and
> > our users. If the semantics of "amend" don't fit nicely though, then the
> > benefit of re-using "amend" is cancelled out and we should go back to
> > considering a separate "key-manage" command.
> 
> This wouldn't solve the fundamental problem that the crypto block
> driver, as it currently is, isn't able to provide a blockdev-amend
> callback. It's worse for qcow2 because qcow2 already implements amend.
> 
> I think we need to find a solution for the amend API.
> 
> Kevin

Best regards,
	Maxim Levitsky

Re: API definition for LUKS key management [V2]

[Maxim Levitsky]

Hi!

This is the second version of the proposed QMP API for key management,
after discussion with Keven and Max.

Will this work?

Adding Peter Krempa to CC, to hear his opinion from the 
libvirt side.

Best regards,
	Maxim Levitsky


diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fea14..63b4cd2a27 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4725,6 +4725,69 @@
   'data': { 'job-id': 'str',
             'options': 'BlockdevCreateOptions' } }
 
+
+##
+# @BlockdevAmendOptionsQcow2:
+#
+# Options for amending the qcow2 image format
+# Currently only crypto related options can be amended
+#
+# @driver           block driver to create the image format
+#
+# Since: 5.0
+##
+{ 'struct': 'BlockdevAmendOptionsQcow2',
+  'data': {
+            '*encrypt': 'QCryptoBlockAmendOptions' } }
+
+##
+# @BlockdevAmendOptionsLUKS:
+#
+# Options for amending the luks image format
+#
+# @driver  block driver to create the image format
+#
+# Since: 5.0
+##
+{ 'struct': 'BlockdevAmendOptionsLUKS',
+  'base': 'QCryptoBlockAmendOptionsLUKS',
+  'data': {  }
+}
+
+##
+# @BlockdevAmendOptions:
+#
+# Options for amending blockdev configuration
+#
+# @driver   block driver that was used to create the block device
+#
+# Since: 5.0
+##
+{ 'union': 'BlockdevAmendOptions',
+  'base': {
+      'driver':         'BlockdevDriver' },
+  'discriminator': 'driver',
+  'data': {
+      'luks':           'BlockdevAmendOptionsLUKS',
+      'qcow2':          'BlockdevAmendOptionsQcow2'
+  } }
+
+##
+# @x-blockdev-amend:
+#
+# Starts a job to create an image format on a given node. The job is
+# automatically finalized, but a manual job-dismiss is required.
+#
+# @job-id:          Identifier for the newly created job.
+#
+# @options:         Options for the image creation.
+#
+# Since: 5.0
+##
+{ 'command': 'x-blockdev-amend',
+  'data': { 'job-id': 'str',
+            'options': 'BlockdevAmendOptions' } }
+
 ##
 # @blockdev-open-tray:
 #
diff --git a/qapi/crypto.json b/qapi/crypto.json
index b2a4cff683..019db682cd 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -309,3 +309,56 @@
   'base': 'QCryptoBlockInfoBase',
   'discriminator': 'format',
   'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
+
+
+##
+# @LUKSKeyslotUpdate:
+#
+# @keyslot:         If specified, will update only keyslot with this index
+#
+# @old-secret:      If specified, will only update keyslots that
+#                   can be opened with password which is contained in
+#                   QCryptoSecret with @old-secret ID
+#
+#                   If neither @keyslot nor @old-secret is specified,
+#                   first empty keyslot is selected for the update
+#
+# @new-secret:      The ID of a QCryptoSecret object providing a new decryption
+#                   key to place in all matching keyslots. Empty string erases the
+#                   keyslot.
+# @iter-time:       number of milliseconds to spend in
+#                   PBKDF passphrase processing
+##
+{ 'struct': 'LUKSKeyslotUpdate',
+  'data': {
+         '*keyslot': 'int',
+         '*old-secret': 'str',
+         'new-secret' : 'str',
+         '*iter-time' : 'int' } }
+
+
+##
+# @QCryptoBlockAmendOptionsLUKS:
+#
+# The options that can be changed on existing luks encrypted device
+# @keys: list of keyslot updates to perform (updates are performed in order)
+#
+# Since: 5.0
+##
+{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
+  'data' : { 'keys': ['LUKSKeyslotUpdate'] } }
+
+
+##
+# @QCryptoBlockAmendOptions:
+#
+# The options that are available for all encryption formats
+# when initializing a new volume
+#
+# Since: 5.0
+##
+{ 'union': 'QCryptoBlockAmendOptions',
+  'base': 'QCryptoBlockOptionsBase',
+  'discriminator': 'format',
+  'data': {
+            'luks': 'QCryptoBlockAmendOptionsLUKS' } }

Re: API definition for LUKS key management [V2]

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1936 bytes --]

On 22.11.19 15:22, Maxim Levitsky wrote:
> Hi!
> 
> This is the second version of the proposed QMP API for key management,
> after discussion with Keven and Max.
> 
> Will this work?
> 
> Adding Peter Krempa to CC, to hear his opinion from the 
> libvirt side.
> 
> Best regards,
> 	Maxim Levitsky

Looks good to me overall.  I don’t think we need to overly push having
the same interface for create and amend, because I don’t see much to be
gained from it.

[...]

> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index b2a4cff683..019db682cd 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -309,3 +309,56 @@
>    'base': 'QCryptoBlockInfoBase',
>    'discriminator': 'format',
>    'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> +
> +
> +##
> +# @LUKSKeyslotUpdate:
> +#
> +# @keyslot:         If specified, will update only keyslot with this index
> +#
> +# @old-secret:      If specified, will only update keyslots that
> +#                   can be opened with password which is contained in
> +#                   QCryptoSecret with @old-secret ID
> +#
> +#                   If neither @keyslot nor @old-secret is specified,
> +#                   first empty keyslot is selected for the update
> +#
> +# @new-secret:      The ID of a QCryptoSecret object providing a new decryption
> +#                   key to place in all matching keyslots. Empty string erases the
> +#                   keyslot.

Hm...  Can’t we make this some string-or-null alternate type so that
null will erase the keyslot?  That would make more sense to me.

Max

> +# @iter-time:       number of milliseconds to spend in
> +#                   PBKDF passphrase processing
> +##
> +{ 'struct': 'LUKSKeyslotUpdate',
> +  'data': {
> +         '*keyslot': 'int',
> +         '*old-secret': 'str',
> +         'new-secret' : 'str',
> +         '*iter-time' : 'int' } }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: API definition for LUKS key management [V2]

[Maxim Levitsky]

On Mon, 2019-11-25 at 19:45 +0100, Max Reitz wrote:
> On 22.11.19 15:22, Maxim Levitsky wrote:
> > Hi!
> > 
> > This is the second version of the proposed QMP API for key management,
> > after discussion with Keven and Max.
> > 
> > Will this work?
> > 
> > Adding Peter Krempa to CC, to hear his opinion from the 
> > libvirt side.
> > 
> > Best regards,
> > 	Maxim Levitsky
> 
> Looks good to me overall.  I don’t think we need to overly push having
> the same interface for create and amend, because I don’t see much to be
> gained from it.

Thanks!!
> 
> [...]
> 
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..019db682cd 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -309,3 +309,56 @@
> >    'base': 'QCryptoBlockInfoBase',
> >    'discriminator': 'format',
> >    'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> > +
> > +
> > +##
> > +# @LUKSKeyslotUpdate:
> > +#
> > +# @keyslot:         If specified, will update only keyslot with this index
> > +#
> > +# @old-secret:      If specified, will only update keyslots that
> > +#                   can be opened with password which is contained in
> > +#                   QCryptoSecret with @old-secret ID
> > +#
> > +#                   If neither @keyslot nor @old-secret is specified,
> > +#                   first empty keyslot is selected for the update
> > +#
> > +# @new-secret:      The ID of a QCryptoSecret object providing a new decryption
> > +#                   key to place in all matching keyslots. Empty string erases the
> > +#                   keyslot.
> 
> Hm...  Can’t we make this some string-or-null alternate type so that
> null will erase the keyslot?  That would make more sense to me.

Agree. I'll see if can get an alternate to work here.

> 
> Max
> 
> > +# @iter-time:       number of milliseconds to spend in
> > +#                   PBKDF passphrase processing
> > +##
> > +{ 'struct': 'LUKSKeyslotUpdate',
> > +  'data': {
> > +         '*keyslot': 'int',
> > +         '*old-secret': 'str',
> > +         'new-secret' : 'str',
> > +         '*iter-time' : 'int' } }
> 
> 

Best regards,
	Maxim Levitsky

Re: API definition for LUKS key management [V2]

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

Am 25.11.2019 um 19:45 hat Max Reitz geschrieben:
> On 22.11.19 15:22, Maxim Levitsky wrote:
> > Hi!
> > 
> > This is the second version of the proposed QMP API for key management,
> > after discussion with Keven and Max.
> > 
> > Will this work?
> > 
> > Adding Peter Krempa to CC, to hear his opinion from the 
> > libvirt side.
> > 
> > Best regards,
> > 	Maxim Levitsky
> 
> Looks good to me overall.  I don’t think we need to overly push having
> the same interface for create and amend, because I don’t see much to be
> gained from it.
> 
> [...]
> 
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..019db682cd 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -309,3 +309,56 @@
> >    'base': 'QCryptoBlockInfoBase',
> >    'discriminator': 'format',
> >    'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
> > +
> > +
> > +##
> > +# @LUKSKeyslotUpdate:
> > +#
> > +# @keyslot:         If specified, will update only keyslot with this index
> > +#
> > +# @old-secret:      If specified, will only update keyslots that
> > +#                   can be opened with password which is contained in
> > +#                   QCryptoSecret with @old-secret ID
> > +#
> > +#                   If neither @keyslot nor @old-secret is specified,
> > +#                   first empty keyslot is selected for the update
> > +#
> > +# @new-secret:      The ID of a QCryptoSecret object providing a new decryption
> > +#                   key to place in all matching keyslots. Empty string erases the
> > +#                   keyslot.
> 
> Hm...  Can’t we make this some string-or-null alternate type so that
> null will erase the keyslot?  That would make more sense to me.

The only problem is that it doesn't map nicely to the command line.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]