Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
On Wed, 2019-08-21 at 13:31 +0200, Markus Armbruster wrote:
> Maxim Levitsky 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
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
Maxim Levitsky 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.
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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
>
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
On Thu, 2019-08-15 at 16:18 +0200, Markus Armbruster wrote:
> Kevin Wolf 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
Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management
Kevin Wolf 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.
