Re: [libvirt] [PATCH RFC 00/16] Add support for LUKS encrypted devices

On Tue, Jun 07, 2016 at 10:45:29AM -0400, John Ferlan wrote: > Patches 1-3 were posted separately: > > http://www.redhat.com/archives/libvir-list/2016-June/msg00256.html > > But perhaps seeing the final direction will make things more clear as > to why a "real" flag system wasn't used and keeping the current paradigm > of constant value returns still works just fine. > > Patches 4-5 were posted separately: > > http://www.redhat.com/archives/libvir-list/2016-June/msg00091.html (4) > http://www.redhat.com/archives/libvir-list/2016-June/msg00094.html (5) > > Although at one point patch 4 had an ACK: > > http://www.redhat.com/archives/libvir-list/2016-May/msg02115.html > > It wasn't clear if the more recent review rescinded that, so it still > remains "in the list". I understand the concern about adding secret to > cfg.mk checking, but without a better idea of how to handle - I left > things as they were. > > Patches 6-16 are all new. Some parts are separable, but rather than continue > piecemeal I just figured going with an RFC will at least > > Patch 6 is only there to "prove" that using the current encryption paradigm > XML still works, although if I've read the tea leaves correctly, the qemu > support isn't working as desired/expected. > > Patch 7 adds "usage" as an XML attribute for encryption and the associated > tests with that. I've chosen to "reuse" the <encryption> XML element rather > than inventing something new. I'm not opposed to something new, but let's > decide up a name quickly... > > Patch 8-9 adds the ability for the storage backend to create/recognize a > luks volume > > Patches 10-13 adds support for luks encryption in the storage backend. > The new "<secret>" format uses "luks" as the usage type and "<key>" as > the 'name'. If those names cause angst, I'm fine with changing, but just > give a better suggestion! Adding <cipher> and <ivgen> were a result of > using qemu constructs from qemu commit id '3e308f20'. Since we are parsing > something new, I figure failing in the domain parse code for this new type > was acceptible as opposed to some post processing check. > > Patches 14-16 adds support for luks encryption to the domain using > <encryption type='luks'... <secret format='key' usage/uuid='xxx'>> > > I've tested using a "good" and "bad" password and got the expected results > for starting a domain. I did not add 'virsh vol-create-as' support just > yet. I figured that would be less to go back and redo if the names of > elements changes. I've also run the changes through Coverity with no > new issues detected. > > The whole series is a result of the following bz: > > https://bugzilla.redhat.com/show_bug.cgi?id=1301021 BTW, one of the core reasons for the LUKS support in QEMU is to facilitate use of LUKS with non-local-file based disks. eg, LUKS over RBD, or LUKS over NBD or LUKS over iSCSI, etc. The diffstat for the files shows you've wired up luks-over-plain-file ok, but I'm wondering if the code posted here is also dealing with the broader support, or if that's something to be addressed later. No big deal either way, particularly since qemu-img can't actually create LUKS volumes on anything other than plain files/block devs yet.