Hi!

This patch series implements key management for luks based encryption
It supports both raw luks images and qcow2 encrypted images.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1731898

There are still several issues that need to be figured out,
on which the feedback is very welcome, but other than that the code mostly works.

The main issues are:

1. Instead of the proposed blockdev-update-encryption/blockdev-erase-encryption
interface, it is probably better to implement 'blockdev-amend-options' in qmp,
and use this both for offline and online key update (with some translation
layer to convert the qemu-img 'options' to qmp structures)

This interface already exists for offline qcow2 format options update/

This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
for the idea!

We agreed that this new qmp interface should take the same options as
blockdev-create does, however since we want to be able to edit the encryption
slots separately, this implies that we sort of need to allow this on creation
time as well.

Also the BlockdevCreateOptions is a union, which is specialized by the driver name
which is great for creation, but for update, the driver name is already known,
and thus the user should not be forced to pass it again.
However qmp doesn't seem to support union type guessing based on actual fields
given (this might not be desired either), which complicates this somewhat.

2. 'crypto' driver (the raw luks block device/file) has special behavior for

share-rw=on. write sharing usually is only allowed for raw files, files that
qemu doesn't itself touch, but only guest does. For such files a well behaved guests can
share the storage.

On the other hand most of the format drivers need to store the metadata, and we don't
have any format driver which implements some kind of sync vs other users of the same
file, thus this is not allowed.

However since for luks which is technically a format driver, the metadata is readonly,
such write sharing was allowed till now, and due to backward compatibility should
still be allowed in the future.

This causes an issue with online updating of the keys, and the solution that was suggested
by Keven that I implemented was to request the exclusive write access only during the key
update.

Testing. This was lightly tested with manual testing and with few iotests that I prepared.
I haven't yet tested fully the write sharing behavior, nor did I run the whole iotests
suite to see if this code causes some regressions. Since I will need probably
to rewrite some chunks of it to change to 'amend' interface, I decided to post it now,
to see if you have other ideas/comments to add.

Best regards,
	Maxim Levitsky

Maxim Levitsky (13):
  block-crypto: misc refactoring
  qcrypto-luks: misc refactoring
  qcrypto-luks: refactoring: extract load/store/check/parse header
    functions
  qcrypto-luks: refactoring: simplify the math used for keyslot
    locations
  qcrypto-luks: clear the masterkey and password before freeing them
    always
  qcrypto-luks: implement more rigorous header checking
  block: add manage-encryption command (qmp and blockdev)
  qcrypto: add the plumbing for encryption management
  qcrypto-luks: implement the encryption key management
  block/crypto: implement the encryption key management
  block/qcow2: implement the encryption key managment
  qemu-img: implement key management
  iotests : add tests for encryption key management

 block/block-backend.c            |    9 +
 block/crypto.c                   |  127 ++-
 block/crypto.h                   |    3 +
 block/io.c                       |   24 +
 block/qcow2.c                    |   27 +
 blockdev.c                       |   40 +
 crypto/block-luks.c              | 1673 ++++++++++++++++++++----------
 crypto/block.c                   |   29 +
 crypto/blockpriv.h               |    9 +
 include/block/block.h            |   12 +
 include/block/block_int.h        |   11 +
 include/crypto/block.h           |   27 +
 include/sysemu/block-backend.h   |    7 +
 qapi/block-core.json             |   36 +
 qapi/crypto.json                 |   26 +
 qemu-img-cmds.hx                 |   13 +
 qemu-img.c                       |  140 +++
 tests/qemu-iotests/257           |  197 ++++
 tests/qemu-iotests/257.out       |   96 ++
 tests/qemu-iotests/258           |   95 ++
 tests/qemu-iotests/258.out       |   30 +
 tests/qemu-iotests/259           |  199 ++++
 tests/qemu-iotests/259.out       |    5 +
 tests/qemu-iotests/common.filter |    5 +-
 tests/qemu-iotests/group         |    3 +
 25 files changed, 2286 insertions(+), 557 deletions(-)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out
 create mode 100755 tests/qemu-iotests/258
 create mode 100644 tests/qemu-iotests/258.out
 create mode 100644 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

-- 
2.17.2

On 8/14/19 3:22 PM, Maxim Levitsky wrote:

> This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> for the idea!
> 
> We agreed that this new qmp interface should take the same options as
> blockdev-create does, however since we want to be able to edit the encryption
> slots separately, this implies that we sort of need to allow this on creation
> time as well.
> 
> Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> which is great for creation, but for update, the driver name is already known,
> and thus the user should not be forced to pass it again.
> However qmp doesn't seem to support union type guessing based on actual fields
> given (this might not be desired either), which complicates this somewhat.

Does the idea of a union type with a default value for the discriminator
help?  Maybe we have a discriminator which defaults to 'auto', and add a
union branch 'auto':'any'.  During creation, if the "driver":"auto"
branch is selected (usually implicitly by omitting "driver", but also
possible explicitly), the creation attempt is rejected as invalid
regardless of the contents of the remaining 'any'.  But during amend
usage, if the 'auto' branch is selected, we then add in the proper
"driver":"xyz" and reparse the QAPI object to determine if the remaining
fields in 'any' still meet the specification for the required driver branch.

This idea may still require some tweaks to the QAPI generator, but it's
the best I can come up with for a way to parse an arbitrary JSON object
with unknown validation, then reparse it again after adding more
information that would constrain the parse differently.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, 2019-08-14 at 16:08 -0500, Eric Blake wrote:
> On 8/14/19 3:22 PM, Maxim Levitsky wrote:
> 
> > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> > for the idea!
> > 
> > We agreed that this new qmp interface should take the same options as
> > blockdev-create does, however since we want to be able to edit the encryption
> > slots separately, this implies that we sort of need to allow this on creation
> > time as well.
> > 
> > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> > which is great for creation, but for update, the driver name is already known,
> > and thus the user should not be forced to pass it again.
> > However qmp doesn't seem to support union type guessing based on actual fields
> > given (this might not be desired either), which complicates this somewhat.
> 
> Does the idea of a union type with a default value for the discriminator
> help?  Maybe we have a discriminator which defaults to 'auto', and add a
> union branch 'auto':'any'.  During creation, if the "driver":"auto"
> branch is selected (usually implicitly by omitting "driver", but also
> possible explicitly), the creation attempt is rejected as invalid
> regardless of the contents of the remaining 'any'.  But during amend
> usage, if the 'auto' branch is selected, we then add in the proper
> "driver":"xyz" and reparse the QAPI object to determine if the remaining
> fields in 'any' still meet the specification for the required driver branch.
> 
> This idea may still require some tweaks to the QAPI generator, but it's
> the best I can come up with for a way to parse an arbitrary JSON object
> with unknown validation, then reparse it again after adding more
> information that would constrain the parse differently.
> 

This could work, but the idea of doing the parsing twice might not be easy to implement.
We currently have the qmp parser completely separated from the rest of the qemu,
so only once the qmp command parsing is done, the corresponding callback is called.


I am thinking. Since any 'update' commmand would need to sepecify the node to work on,
one could add some kind of expression for the qmp frontend to query the driver of that
node itself, which would solve that problem


Something like that:

{ 'union': 'BlockdevAmendOptions',

  'base': {
      'node-name':         'str' },

  'discriminator': { 'get_block_driver(node-name)' } ,

  'data': {
      'file':           'BlockdevCreateOptionsFile',
      'gluster':        'BlockdevCreateOptionsGluster',
      'luks':           'BlockdevCreateOptionsLUKS',
      'nfs':            'BlockdevCreateOptionsNfs',
      'parallels':      'BlockdevCreateOptionsParallels',
      'qcow':           'BlockdevCreateOptionsQcow',
      'qcow2':          'BlockdevCreateOptionsQcow2',
      'qed':            'BlockdevCreateOptionsQed',
      'rbd':            'BlockdevCreateOptionsRbd',
      'sheepdog':       'BlockdevCreateOptionsSheepdog',
      'ssh':            'BlockdevCreateOptionsSsh',
      'vdi':            'BlockdevCreateOptionsVdi',
      'vhdx':           'BlockdevCreateOptionsVhdx',
      'vmdk':           'BlockdevCreateOptionsVmdk',
      'vpc':            'BlockdevCreateOptionsVpc'
  } }


The 'get_block_driver' expression will make the QMP frontend, take the value of the node-name union field,
and look up the block driver associated with it and use that as a discriminator.

Syntax wise we can (at some expense of readability) use json to express the same like

'discriminator': { 'field' : 'node-name', 'transform': 'getdrivername' },

Best regards,
	Maxim Levitsky

Am 14.08.2019 um 23:08 hat Eric Blake geschrieben:
> On 8/14/19 3:22 PM, Maxim Levitsky wrote:
> 
> > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> > for the idea!
> > 
> > We agreed that this new qmp interface should take the same options as
> > blockdev-create does, however since we want to be able to edit the encryption
> > slots separately, this implies that we sort of need to allow this on creation
> > time as well.
> > 
> > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> > which is great for creation, but for update, the driver name is already known,
> > and thus the user should not be forced to pass it again.
> > However qmp doesn't seem to support union type guessing based on actual fields
> > given (this might not be desired either), which complicates this somewhat.
> 
> Does the idea of a union type with a default value for the discriminator
> help?  Maybe we have a discriminator which defaults to 'auto', and add a
> union branch 'auto':'any'.  During creation, if the "driver":"auto"
> branch is selected (usually implicitly by omitting "driver", but also
> possible explicitly), the creation attempt is rejected as invalid
> regardless of the contents of the remaining 'any'.  But during amend
> usage, if the 'auto' branch is selected, we then add in the proper
> "driver":"xyz" and reparse the QAPI object to determine if the remaining
> fields in 'any' still meet the specification for the required driver branch.
> 
> This idea may still require some tweaks to the QAPI generator, but it's
> the best I can come up with for a way to parse an arbitrary JSON object
> with unknown validation, then reparse it again after adding more
> information that would constrain the parse differently.

Feels like this would be a lot of code just to allow the client to omit
passing a value that it knows anyway. If this were a human interface, I
could understand the desire to make commands less verbose, but for QMP I
honestly don't see the point when it's not trivial.

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.08.2019 um 23:08 hat Eric Blake geschrieben:
>> On 8/14/19 3:22 PM, Maxim Levitsky wrote:
>> 
>> > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
>> > for the idea!
>> > 
>> > We agreed that this new qmp interface should take the same options as
>> > blockdev-create does, however since we want to be able to edit the encryption
>> > slots separately, this implies that we sort of need to allow this on creation
>> > time as well.
>> > 
>> > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
>> > which is great for creation, but for update, the driver name is already known,
>> > and thus the user should not be forced to pass it again.
>> > However qmp doesn't seem to support union type guessing based on actual fields
>> > given (this might not be desired either), which complicates this somewhat.
>> 
>> Does the idea of a union type with a default value for the discriminator
>> help?  Maybe we have a discriminator which defaults to 'auto', and add a
>> union branch 'auto':'any'.  During creation, if the "driver":"auto"
>> branch is selected (usually implicitly by omitting "driver", but also
>> possible explicitly), the creation attempt is rejected as invalid
>> regardless of the contents of the remaining 'any'.  But during amend
>> usage, if the 'auto' branch is selected, we then add in the proper
>> "driver":"xyz" and reparse the QAPI object to determine if the remaining
>> fields in 'any' still meet the specification for the required driver branch.
>> 
>> This idea may still require some tweaks to the QAPI generator, but it's
>> the best I can come up with for a way to parse an arbitrary JSON object
>> with unknown validation, then reparse it again after adding more
>> information that would constrain the parse differently.
>
> Feels like this would be a lot of code just to allow the client to omit
> passing a value that it knows anyway. If this were a human interface, I
> could understand the desire to make commands less verbose, but for QMP I
> honestly don't see the point when it's not trivial.

Seconded.

On Thu, 2019-08-15 at 16:18 +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.08.2019 um 23:08 hat Eric Blake geschrieben:
> > > On 8/14/19 3:22 PM, Maxim Levitsky wrote:
> > > 
> > > > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> > > > for the idea!
> > > > 
> > > > We agreed that this new qmp interface should take the same options as
> > > > blockdev-create does, however since we want to be able to edit the encryption
> > > > slots separately, this implies that we sort of need to allow this on creation
> > > > time as well.
> > > > 
> > > > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> > > > which is great for creation, but for update, the driver name is already known,
> > > > and thus the user should not be forced to pass it again.
> > > > However qmp doesn't seem to support union type guessing based on actual fields
> > > > given (this might not be desired either), which complicates this somewhat.
> > > 
> > > Does the idea of a union type with a default value for the discriminator
> > > help?  Maybe we have a discriminator which defaults to 'auto', and add a
> > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
> > > branch is selected (usually implicitly by omitting "driver", but also
> > > possible explicitly), the creation attempt is rejected as invalid
> > > regardless of the contents of the remaining 'any'.  But during amend
> > > usage, if the 'auto' branch is selected, we then add in the proper
> > > "driver":"xyz" and reparse the QAPI object to determine if the remaining
> > > fields in 'any' still meet the specification for the required driver branch.
> > > 
> > > This idea may still require some tweaks to the QAPI generator, but it's
> > > the best I can come up with for a way to parse an arbitrary JSON object
> > > with unknown validation, then reparse it again after adding more
> > > information that would constrain the parse differently.
> > 
> > Feels like this would be a lot of code just to allow the client to omit
> > passing a value that it knows anyway. If this were a human interface, I
> > could understand the desire to make commands less verbose, but for QMP I
> > honestly don't see the point when it's not trivial.
> 
> Seconded.


But what about my suggestion of adding something like:

{ 'union': 'BlockdevAmendOptions',

  'base': {
      'node-name':         'str' },

  'discriminator': { 'get_block_driver(node-name)' } ,

  'data': {
      'file':           'BlockdevCreateOptionsFile',
      'gluster':        'BlockdevCreateOptionsGluster',
      'luks':           'BlockdevCreateOptionsLUKS',
      'nfs':            'BlockdevCreateOptionsNfs',
      'parallels':      'BlockdevCreateOptionsParallels',
      'qcow':           'BlockdevCreateOptionsQcow',
      'qcow2':          'BlockdevCreateOptionsQcow2',
      'qed':            'BlockdevCreateOptionsQed',
      'rbd':            'BlockdevCreateOptionsRbd',
      'sheepdog':       'BlockdevCreateOptionsSheepdog',
      'ssh':            'BlockdevCreateOptionsSsh',
      'vdi':            'BlockdevCreateOptionsVdi',
      'vhdx':           'BlockdevCreateOptionsVhdx',
      'vmdk':           'BlockdevCreateOptionsVmdk',
      'vpc':            'BlockdevCreateOptionsVpc'
  } }


This shouldn't be hard to do IMHO.

Best regards,
	Maxim Levitsky

On 8/15/19 9:44 AM, Maxim Levitsky wrote:

>>>> Does the idea of a union type with a default value for the discriminator
>>>> help?  Maybe we have a discriminator which defaults to 'auto', and add a
>>>> union branch 'auto':'any'.  During creation, if the "driver":"auto"
>>>> branch is selected (usually implicitly by omitting "driver", but also
>>>> possible explicitly), the creation attempt is rejected as invalid
>>>> regardless of the contents of the remaining 'any'.  But during amend
>>>> usage, if the 'auto' branch is selected, we then add in the proper
>>>> "driver":"xyz" and reparse the QAPI object to determine if the remaining
>>>> fields in 'any' still meet the specification for the required driver branch.
>>>>
>>>> This idea may still require some tweaks to the QAPI generator, but it's
>>>> the best I can come up with for a way to parse an arbitrary JSON object
>>>> with unknown validation, then reparse it again after adding more
>>>> information that would constrain the parse differently.
>>>
>>> Feels like this would be a lot of code just to allow the client to omit
>>> passing a value that it knows anyway. If this were a human interface, I
>>> could understand the desire to make commands less verbose, but for QMP I
>>> honestly don't see the point when it's not trivial.
>>
>> Seconded.
> 
> 
> But what about my suggestion of adding something like:
> 
> { 'union': 'BlockdevAmendOptions',
> 
>   'base': {
>       'node-name':         'str' },
> 
>   'discriminator': { 'get_block_driver(node-name)' } ,

Not worth it. It makes the QAPI generator more complex (to invoke
arbitrary code instead of a fixed name) just to avoid a little bit of
complexity in the caller (which is assumed to be a computer, and thus
shouldn't have a hard time providing a sane 'driver' unconditionally).
An HMP wrapper around the QMP command can do whatever magic it needs to
omit driver, but making driver mandatory for QMP is just fine.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
> On 8/15/19 9:44 AM, Maxim Levitsky wrote:
> 
> > > > > Does the idea of a union type with a default value for the discriminator
> > > > > help?  Maybe we have a discriminator which defaults to 'auto', and add a
> > > > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
> > > > > branch is selected (usually implicitly by omitting "driver", but also
> > > > > possible explicitly), the creation attempt is rejected as invalid
> > > > > regardless of the contents of the remaining 'any'.  But during amend
> > > > > usage, if the 'auto' branch is selected, we then add in the proper
> > > > > "driver":"xyz" and reparse the QAPI object to determine if the remaining
> > > > > fields in 'any' still meet the specification for the required driver branch.
> > > > > 
> > > > > This idea may still require some tweaks to the QAPI generator, but it's
> > > > > the best I can come up with for a way to parse an arbitrary JSON object
> > > > > with unknown validation, then reparse it again after adding more
> > > > > information that would constrain the parse differently.
> > > > 
> > > > Feels like this would be a lot of code just to allow the client to omit
> > > > passing a value that it knows anyway. If this were a human interface, I
> > > > could understand the desire to make commands less verbose, but for QMP I
> > > > honestly don't see the point when it's not trivial.
> > > 
> > > Seconded.
> > 
> > 
> > But what about my suggestion of adding something like:
> > 
> > { 'union': 'BlockdevAmendOptions',
> > 
> >   'base': {
> >       'node-name':         'str' },
> > 
> >   'discriminator': { 'get_block_driver(node-name)' } ,
> 
> Not worth it. It makes the QAPI generator more complex (to invoke
> arbitrary code instead of a fixed name) just to avoid a little bit of
> complexity in the caller (which is assumed to be a computer, and thus
> shouldn't have a hard time providing a sane 'driver' unconditionally).
> An HMP wrapper around the QMP command can do whatever magic it needs to
> omit driver, but making driver mandatory for QMP is just fine.

All right! I kind of not agree with that, since I think even though QMP is a machine language,
it still should be consistent since humans still use it, even if this is humans that code some
tool that use it.

I won't argue with you though, let it be like that.

Best regards,
	Maxim Levitsky

> 

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
>> On 8/15/19 9:44 AM, Maxim Levitsky wrote:
>> 
>> > > > > Does the idea of a union type with a default value for the discriminator
>> > > > > help?  Maybe we have a discriminator which defaults to 'auto', and add a
>> > > > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
>> > > > > branch is selected (usually implicitly by omitting "driver", but also
>> > > > > possible explicitly), the creation attempt is rejected as invalid
>> > > > > regardless of the contents of the remaining 'any'.  But during amend
>> > > > > usage, if the 'auto' branch is selected, we then add in the proper
>> > > > > "driver":"xyz" and reparse the QAPI object to determine if the remaining
>> > > > > fields in 'any' still meet the specification for the required driver branch.
>> > > > > 
>> > > > > This idea may still require some tweaks to the QAPI generator, but it's
>> > > > > the best I can come up with for a way to parse an arbitrary JSON object
>> > > > > with unknown validation, then reparse it again after adding more
>> > > > > information that would constrain the parse differently.
>> > > > 
>> > > > Feels like this would be a lot of code just to allow the client to omit
>> > > > passing a value that it knows anyway. If this were a human interface, I
>> > > > could understand the desire to make commands less verbose, but for QMP I
>> > > > honestly don't see the point when it's not trivial.
>> > > 
>> > > Seconded.
>> > 
>> > 
>> > But what about my suggestion of adding something like:
>> > 
>> > { 'union': 'BlockdevAmendOptions',
>> > 
>> >   'base': {
>> >       'node-name':         'str' },
>> > 
>> >   'discriminator': { 'get_block_driver(node-name)' } ,
>> 
>> Not worth it. It makes the QAPI generator more complex (to invoke
>> arbitrary code instead of a fixed name) just to avoid a little bit of
>> complexity in the caller (which is assumed to be a computer, and thus
>> shouldn't have a hard time providing a sane 'driver' unconditionally).
>> An HMP wrapper around the QMP command can do whatever magic it needs to
>> omit driver, but making driver mandatory for QMP is just fine.
>
> All right! I kind of not agree with that, since I think even though QMP is a machine language,
> it still should be consistent since humans still use it, even if this is humans that code some
> tool that use it.
>
> I won't argue with you though, let it be like that.

Software's fundamental limit is complexity.  We need to pick what we use
it for.  Sometimes, that means saying no to things that would be nice to
have.

On Wed, 2019-08-21 at 13:31 +0200, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
> > > On 8/15/19 9:44 AM, Maxim Levitsky wrote:
> > > 
> > > > > > > Does the idea of a union type with a default value for the discriminator
> > > > > > > help?  Maybe we have a discriminator which defaults to 'auto', and add a
> > > > > > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
> > > > > > > branch is selected (usually implicitly by omitting "driver", but also
> > > > > > > possible explicitly), the creation attempt is rejected as invalid
> > > > > > > regardless of the contents of the remaining 'any'.  But during amend
> > > > > > > usage, if the 'auto' branch is selected, we then add in the proper
> > > > > > > "driver":"xyz" and reparse the QAPI object to determine if the remaining
> > > > > > > fields in 'any' still meet the specification for the required driver branch.
> > > > > > > 
> > > > > > > This idea may still require some tweaks to the QAPI generator, but it's
> > > > > > > the best I can come up with for a way to parse an arbitrary JSON object
> > > > > > > with unknown validation, then reparse it again after adding more
> > > > > > > information that would constrain the parse differently.
> > > > > > 
> > > > > > Feels like this would be a lot of code just to allow the client to omit
> > > > > > passing a value that it knows anyway. If this were a human interface, I
> > > > > > could understand the desire to make commands less verbose, but for QMP I
> > > > > > honestly don't see the point when it's not trivial.
> > > > > 
> > > > > Seconded.
> > > > 
> > > > 
> > > > But what about my suggestion of adding something like:
> > > > 
> > > > { 'union': 'BlockdevAmendOptions',
> > > > 
> > > >   'base': {
> > > >       'node-name':         'str' },
> > > > 
> > > >   'discriminator': { 'get_block_driver(node-name)' } ,
> > > 
> > > Not worth it. It makes the QAPI generator more complex (to invoke
> > > arbitrary code instead of a fixed name) just to avoid a little bit of
> > > complexity in the caller (which is assumed to be a computer, and thus
> > > shouldn't have a hard time providing a sane 'driver' unconditionally).
> > > An HMP wrapper around the QMP command can do whatever magic it needs to
> > > omit driver, but making driver mandatory for QMP is just fine.
> > 
> > All right! I kind of not agree with that, since I think even though QMP is a machine language,
> > it still should be consistent since humans still use it, even if this is humans that code some
> > tool that use it.
> > 
> > I won't argue with you though, let it be like that.
> 
> Software's fundamental limit is complexity.  We need to pick what we use
> it for.  Sometimes, that means saying no to things that would be nice to
> have.

I fully agree with that and that is usually the exact reason I argue about such things:
Sometimes avoiding complexity in one place, and/or breaking consistency can introduce complexity in other place (like libvirt).

In this particular case, I won't argue about this, but still it kind of feels like
it is a precedent of requiring the user to supply redundant data.

Of all issues all of you pointed out (thanks!!) this is probably the least important one that I should be arguing about, 
so let it be like you say.

Best regards,
	Maxim Levitsky

On Wed, Aug 14, 2019 at 11:22:06PM +0300, Maxim Levitsky wrote:
> Hi!
> 
> This patch series implements key management for luks based encryption
> It supports both raw luks images and qcow2 encrypted images.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1731898
> 
> There are still several issues that need to be figured out,
> on which the feedback is very welcome, but other than that the code mostly works.
> 
> The main issues are:
> 
> 1. Instead of the proposed blockdev-update-encryption/blockdev-erase-encryption
> interface, it is probably better to implement 'blockdev-amend-options' in qmp,
> and use this both for offline and online key update (with some translation
> layer to convert the qemu-img 'options' to qmp structures)
> 
> This interface already exists for offline qcow2 format options update/
> 
> This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> for the idea!
> 
> We agreed that this new qmp interface should take the same options as
> blockdev-create does, however since we want to be able to edit the encryption
> slots separately, this implies that we sort of need to allow this on creation
> time as well.
> 
> Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> which is great for creation, but for update, the driver name is already known,
> and thus the user should not be forced to pass it again.
> However qmp doesn't seem to support union type guessing based on actual fields
> given (this might not be desired either), which complicates this somewhat.

Given this design question around the integration into blockdev, I'd
suggest splitting the series into two parts.

One series should do all the work in crypto/ code to support adding
and erasing key slots.

One series should focus on block/ layer QMP/qemu-img integration.

The block layer QAPI stuff shouldn't leak into the crypto/ code.

So this will let us get on with reviewing & unit testing the
crypto code, while we discuss block layer design options in more
detail.

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

On Thu, 2019-08-22 at 12:35 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:06PM +0300, Maxim Levitsky wrote:
> > Hi!
> > 
> > This patch series implements key management for luks based encryption
> > It supports both raw luks images and qcow2 encrypted images.
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1731898
> > 
> > There are still several issues that need to be figured out,
> > on which the feedback is very welcome, but other than that the code mostly works.
> > 
> > The main issues are:
> > 
> > 1. Instead of the proposed blockdev-update-encryption/blockdev-erase-encryption
> > interface, it is probably better to implement 'blockdev-amend-options' in qmp,
> > and use this both for offline and online key update (with some translation
> > layer to convert the qemu-img 'options' to qmp structures)
> > 
> > This interface already exists for offline qcow2 format options update/
> > 
> > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> > for the idea!
> > 
> > We agreed that this new qmp interface should take the same options as
> > blockdev-create does, however since we want to be able to edit the encryption
> > slots separately, this implies that we sort of need to allow this on creation
> > time as well.
> > 
> > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> > which is great for creation, but for update, the driver name is already known,
> > and thus the user should not be forced to pass it again.
> > However qmp doesn't seem to support union type guessing based on actual fields
> > given (this might not be desired either), which complicates this somewhat.
> 
> Given this design question around the integration into blockdev, I'd
> suggest splitting the series into two parts.
> 
> One series should do all the work in crypto/ code to support adding
> and erasing key slots.
> 
> One series should focus on block/ layer QMP/qemu-img integration.
> 
> The block layer QAPI stuff shouldn't leak into the crypto/ code.
> 
> So this will let us get on with reviewing & unit testing the
> crypto code, while we discuss block layer design options in more
> detail.
> 
> Regards,
> Daniel


I think we need 3 series here.


1. All the re-factoring/preparation work I done in luks crypto driver, which can be merged
now, pending minor changes from the review.
I think that it at least doesn't make the code worse.

2. Common code for the block layer to support key management this way or another,
   can be even added with not a single driver implementing it.

1,2 don't depend on each other mostly.


3. Key management in LUKS, which needs both 1,2, but thankfully is mostly implemented,
and won't need to change much from the current implementation.


So I'll send 1 now, and I will star working on 2.

Last week we (I and Daniel) defined a draft of amend interface,
and if time permits we will work on that tomorrow to finalize the
interface.

Best regards,
	Maxim Levitsky

On 14.08.19 22:22, Maxim Levitsky wrote:

[...]

> Testing. This was lightly tested with manual testing and with few iotests that I prepared.
> I haven't yet tested fully the write sharing behavior, nor did I run the whole iotests
> suite to see if this code causes some regressions. Since I will need probably
> to rewrite some chunks of it to change to 'amend' interface, I decided to post it now,
> to see if you have other ideas/comments to add.

I can see that, because half of the qcow2 tests that contain the string
“secret” break:

Failures: 087 134 158 178 188 198 206
Failed 7 of 13 tests

Also, 210 when run with -luks.

Some are just due to different test outputs (because you change
_filter_img_create to filter some encrypt.* parameters), but some of
them are due to aborts.  All of them look like different kinds of heap
corruptions.


I can fully understand not running all iotests (because only the
maintainers do that before pull requests), but just running the iotests
that immediately concern a series seems prudent to me (unless the series
is trivial).

(Just “(cd tests/qemu-iotests && grep -l secret ???)” tells you which
tests to run that may concern themselves with qcow2 encryption, for
example.)


So I suppose I’ll stop reviewing the series in detail and just give a
more cursory glance from now on.

Max

On Tue, 2019-08-20 at 19:59 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> 
> [...]
> 
> > Testing. This was lightly tested with manual testing and with few iotests that I prepared.
> > I haven't yet tested fully the write sharing behavior, nor did I run the whole iotests
> > suite to see if this code causes some regressions. Since I will need probably
> > to rewrite some chunks of it to change to 'amend' interface, I decided to post it now,
> > to see if you have other ideas/comments to add.
> 
> I can see that, because half of the qcow2 tests that contain the string
> “secret” break:
> 
> Failures: 087 134 158 178 188 198 206
> Failed 7 of 13 tests
> 
> Also, 210 when run with -luks.
> 
> Some are just due to different test outputs (because you change
> _filter_img_create to filter some encrypt.* parameters), but some of
> them are due to aborts.  All of them look like different kinds of heap
> corruptions.
> 
> 
> I can fully understand not running all iotests (because only the
> maintainers do that before pull requests), but just running the iotests
> that immediately concern a series seems prudent to me (unless the series
> is trivial).
> 
> (Just “(cd tests/qemu-iotests && grep -l secret ???)” tells you which
> tests to run that may concern themselves with qcow2 encryption, for
> example.)
> 
> 
> So I suppose I’ll stop reviewing the series in detail and just give a
> more cursory glance from now on.

Sorry about that! I posted this as RFC, and the reason it is mostly done as opposed to typical RFC which might not
even contain any code was that for most of the time I was sure that API of this is straightforward and won't need
any significant discussion, and in the last minute after I discussed with Kevin on IRC one 
obscure case of block backend permissions that was failing, he told me about the amend interface.
Next time I guess, when new a API is involved, I will post an API RFC first always and then start the implementation.

I fixed both issues that iotests uncovered locally, now all luks and most qcow2 tests pass 
(118 and 194 sometimes fail with qcow2, and this happens regardless of my patches, and same for 162 which seems to fail
always now, also regardless of my patches.
I have a git head after the merge window opened so probably some bugs were added, and maybe already fixed)


The first issue was in 'qcrypto-luks: implement the encryption key management'
where I accidentally stored the name of the secret without strdup'ing in the create flow, so I got double free,
which indeed caused the heap corruptions you have seen.

Basically this line:
    luks->secret = options->u.luks.key_secret;

The second issue as you mention is indeed the change in filters I did. Do you agree with that change btw?
If you ask me, I would even change the filter further and filter all the image options from the qemu command line since these are test inputs anyway.
This allowed me to have the same test for both luks and qcow2 luks encrypted test.

Also I didn't even expect you to run the iotests for me, but
rather just wanted a general RFC level feedback on the whole thing, that is why I even mentioned that I didn't run them.
So sorry for the trouble I caused!

I btw don't agree with you that only maintainers need to run all the iotests fully. 
I think that the patch submitter  should run all the tests that he can to catch as many problems as he can,
_unless_ of course this is an RFC.


Best regards,
Thanks for the review,
Sorry again for the trouble,

Maxim Levitsky