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@xxxxxxxxxx>
>> ---
>> 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:
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list