[libvirt] [PATCH 19/19] qemu: Add luks support for domain disk

[John Ferlan]

On 06/21/2016 09:03 AM, Peter Krempa wrote:
> On Mon, Jun 13, 2016 at 20:27:58 -0400, John Ferlan wrote:
>> Generate the luks command line using the AES secret key to encrypt the
>> luks secret. A luks secret object will be in addition to a an AES secret.
>>
>> Add tests for sample output
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_command.c                            | 12 ++++++--
>>  src/qemu/qemu_domain.c                             | 19 ++++++++++--
>>  .../qemuxml2argv-luks-disk-cipher.args             | 36 ++++++++++++++++++++++
>>  .../qemuxml2argvdata/qemuxml2argv-luks-disks.args  | 36 ++++++++++++++++++++++
>>  tests/qemuxml2argvtest.c                           | 11 ++++++-
>>  5 files changed, 109 insertions(+), 5 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args
> 
> Note that this won't work with disk hotplug until you add code that will
> add the secret objects too.

Oh right - I had forgotten about the secret object... I had the same
issue with the TLS code, so I know what has to be done...

> 
> Also I've noticed that RBD hotplug is now broken too due to the same
> reason. The code that selects whether to use plaintext password or pass
> it via the secret object does not check whether it's called on the
> hotplug path and thus uses the secret object.

Oh damn...  I think that's a disconnect in qemuDomainSecretSetup (called
from qemuDomainSecretDiskPrepare from hotplug) and the assumption of an
AES secinfo for RBD when there's a secret object available.

> 
> The secret object is not added using the monitor though. You'll need to
> fix that first and if you opt for the correct approach this will then
> work automagically.
> 
> Looks okay but I can't ACK this with hotplug not working.
> 
> Also please note that on hot-unplug you need to remove the secrets as
> you'll possibly be adding a secret with the same name (Since alias will
> be reused)

yep...

> 
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 490260f..7062c17 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1103,6 +1103,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>>      int actualType = virStorageSourceGetActualType(disk->src);
>>      qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>      qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>> +    qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>>      bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
>>  
>>      if (idx < 0) {
>> @@ -1237,10 +1238,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>>          qemuBufferEscapeComma(&opt, source);
>>          virBufferAddLit(&opt, ",");
>>  
>> -        if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> +        if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
>>              virBufferAsprintf(&opt, "password-secret=%s,",
>>                                secinfo->s.aes.alias);
>> -        }
>> +
>> +        if (encinfo)
>> +            virQEMUBuildLuksOpts(&opt, disk->src->encryption,
>> +                                 encinfo->s.aes.alias);
> 
> This wrapper is not really useful here. It only adds "key-secret=" all
> the other options are necessary only if creating the volume.
> 

I was thinking of the future if/when new things are added.  I'm not
clear yet what would have to be done for block-copy and snapshots.

>>  
>>          if (disk->src->format > 0 &&
>>              disk->src->type != VIR_STORAGE_TYPE_DIR)
>> @@ -1920,6 +1924,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>          virDomainDiskDefPtr disk = def->disks[i];
>>          qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>          qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>> +        qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>>  
>>          /* PowerPC pseries based VMs do not support floppy device */
>>          if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
>> @@ -1949,6 +1954,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>          if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
>>              return -1;
>>  
>> +        if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0)
>> +            return -1;
>> +
>>          virCommandAddArg(cmd, "-drive");
>>  
>>          optstr = qemuBuildDriveStr(disk,
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c288fa0..fb3e91f 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
> 
> [...]
> 
>> @@ -1026,7 +1028,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>>           src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
>>  
>>          virSecretUsageType secretUsageType;
>> -        qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>  
>>          if (VIR_ALLOC(secinfo) < 0)
>>              return -1;
>> @@ -1044,6 +1045,20 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>>          diskPriv->secinfo = secinfo;
>>      }
>>  
>> +    if (conn && !virStorageSourceIsEmpty(src) &&
> 
> This part is the same with the block above. It may be worth extracting
> it ...
>

Oh right...  I think I had it that way at one time, too...

Thanks again -

John

>> +        src->encryption && src->format == VIR_STORAGE_FILE_LUKS) {
>> +
>> +        if (VIR_ALLOC(secinfo) < 0)
>> +            return -1;
>> +
>> +        if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
>> +                                  VIR_SECRET_USAGE_TYPE_KEY, NULL,
>> +                                  &src->encryption->secrets[0]->secdef) < 0)
>> +            goto error;
>> +
>> +        diskPriv->encinfo = secinfo;
>> +    }
>> +
>>      return 0;
>>  
>>   error: