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

[John Ferlan]

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?

I also didn't make any hotplug changes.
John