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

On 06/07/2016 12:01 PM, Daniel P. Berrange wrote: > 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. > Hmm, right, sigh, but I had to start somewhere. So rather than be either/or in qemuDomainSecretDiskPrepare and qemuBuildDriveStr, there would need to be a way to have multiple 'secinfo' structs per disk or maybe one for <auth> and one for <encrypt>. Can we predict the future and go with a list or just two secinfo's... Any preference?