Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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 :|
Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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 signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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
Re: [Qemu-block] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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 signature.asc Description: OpenPGP digital signature
