| Message ID | 20190814202219.1870-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RFC: luks/encrypted qcow2 key management | expand |
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.
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.
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 >
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
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 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
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
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
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