[libvirt] [PATCH v3 10/10] qemu: Add luks support for domain disk
John Ferlan
jferlan at redhat.com
Sun Jun 26 13:24:54 UTC 2016
On 06/25/2016 03:07 AM, Ján Tomko wrote:
> On Fri, Jun 24, 2016 at 04:53:39PM -0400, John Ferlan wrote:
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021
>>
>> 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.
>>
>> For hotplug, check if the encinfo exists and if so, add the AES secret
>> for the passphrase for the secret object used to decrypt the device.
>>
>> Add tests for sample output
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/qemu/qemu_command.c | 9 +++
>> src/qemu/qemu_domain.c | 25 ++++++-
>> src/qemu/qemu_hotplug.c | 82
>> +++++++++++++++++++---
>> .../qemuxml2argv-luks-disk-cipher.args | 36 ++++++++++
>> .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 ++++++++++
>> tests/qemuxml2argvtest.c | 11 ++-
>> 6 files changed, 187 insertions(+), 12 deletions(-)
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 71e9e63..038a60c 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1098,6 +1098,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,6 +1238,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>> secinfo->s.aes.alias);
>> }
>>
>> + if (encinfo)
>> + virQEMUBuildLuksOpts(&opt, &disk->src->encryption->encinfo,
>> + encinfo->s.aes.alias);
>> +
>> if (disk->src->format > 0 &&
>> disk->src->type != VIR_STORAGE_TYPE_DIR)
>> virBufferAsprintf(&opt, "format=%s,",
>> @@ -1939,6 +1944,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 &&
>> @@ -1967,6 +1973,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 d51e82b..5405365 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -945,7 +945,8 @@ qemuDomainSecretSetup(virConnectPtr conn,
>> {
>> if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) &&
>> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
>> - secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) {
>> + (secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH ||
>> + secretUsageType == VIR_SECRET_USAGE_TYPE_PASSPHRASE)) {
>
> Since we have not allowed VIR_SECRET_USAGE_TYPE_PASSPHRASE before,
> can we error out if we don't have both VIR_CRYPTO_CIPHER_AES256CBC and
> QEMU_CAPS_OBJECT_SECRET to avoid putting plaintext on qemu command line?
>
Unfortunately because of the iSCSI issue, no. Long story short, adding
an AES secret for an iSCSI had an issue because the parameter used to
process was only found on the -iscsi command line. In order to add the
proper ID (alias) for an -iscsi command line involved providing the IQN
which has a ':' in it and that character is illegal for an alias. A
change Dan proposed to fix it in QEMU (late) for 2.6 was rejected:
http://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02198.html
>> if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias,
>> secretUsageType, username,
>> seclookupdef, isLuks) < 0)
>> @@ -364,7 +370,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>> if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
>> goto error;
>>
>> - /* Attach the device - possible 3 step process */
>> + /* Attach the device - possible 4 step process */
>
> If you delete this outdated comment in 8/10, you don't have to update it
> here.
>
OK gone in 8/10
>> qemuDomainObjEnterMonitor(driver, vm);
>>
>> if (secobjProps && qemuMonitorAddObject(priv->mon, "secret",
>> @@ -373,6 +379,12 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>> goto failaddobjsecret;
>> secobjProps = NULL;
>>
>> + if (encProps && qemuMonitorAddObject(priv->mon, "secret",
>> + encinfo->s.aes.alias,
>> + encProps) < 0)
>> + goto failaddencsecret;
>
> s/failaddencsecret/delete_secinfo/
>
While it's not clear to me what the issue is with the label, this does
bring up an interesting point about error recovery...
It would seem the setting of secobjProps and encProps to NULL should not
occur until after we exit the monitor; otherwise, they won't be properly
deleted in the error path. So back in 8/10, rather than a secobjProps =
NULL right away, it'll move to just before the call to virDomainAuditDisk
>> + encProps = NULL;
>> +
>> if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>> goto failadddrive;
>>
>> @@ -391,6 +403,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>>
>> cleanup:
>> virJSONValueFree(secobjProps);
>> + virJSONValueFree(encProps);
>> qemuDomainSecretDiskDestroy(disk);
>> VIR_FREE(devstr);
>> VIR_FREE(drivestr);
>> @@ -410,8 +423,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>> }
>>
>> failadddrive:
>> + if (encProps)
>> + ignore_value(qemuMonitorDelObject(priv->mon,
>> encinfo->s.aes.alias));
>
> This can also overwrite the error.
>
Fixed those
>> +
>> + failaddencsecret:
>> if (secobjProps)
>> ignore_value(qemuMonitorDelObject(priv->mon,
>> secinfo->s.aes.alias));
>> + encProps = NULL; /* qemuMonitorAddObject consumes props on
>> failure too */
>>
>> failaddobjsecret:
>> if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> @@ -571,6 +589,9 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>> char *devstr = NULL;
>> int ret = -1;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> + virJSONValuePtr encProps = NULL;
>> + qemuDomainDiskPrivatePtr diskPriv;
>> + qemuDomainSecretInfoPtr encinfo;
>>
>> if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0)
>> goto cleanup;
>> @@ -589,6 +610,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>> if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0)
>> goto error;
>>
>> + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>> + encinfo = diskPriv->encinfo;
>> + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0)
>> + goto error;
>> +
>> if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0,
>> priv->qemuCaps)))
>> goto error;
>>
>> @@ -598,9 +624,15 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
>> if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
>> goto error;
>>
>> - /* Attach the device - 2 step process */
>> + /* Attach the device - possible 3 step process */
>> qemuDomainObjEnterMonitor(driver, vm);
>>
>> + if (encProps && qemuMonitorAddObject(priv->mon, "secret",
>> + encinfo->s.aes.alias,
>> + encProps) < 0)
>
> Don't we need to deal with secinfo like we do for virtio disks?
>
No, iSCSI cannot support it. When/if that resolved in qemu, then yes,
this follows that mechanism.
>> + goto failaddencsecret;
>
> s/failaddencsecret/exit_monitor/
>
I can change the label name to exit_monitor, it really doesn't matter
that much to me.
>> + encProps = NULL;
>> +
>> if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>> goto failadddrive;
>>
>
> ACK only if we don't need to deal with diskPriv->secinfo;
>
I assume this comment refers to -> secinfo for AttachSCSIDisk...
However, I'm in a quandary. Even though I had requested to extend the
deadline to generate an -rc1 for this libvirt release, it seems it wasn't.
Now that we've frozen, IIUC technically according to our rules none of
this can now go in; however, this is something important for the Red Hat
release process timing.
I can repost 8 & 10 just to be "safe" so it's "clear" what the changes were.
John
More information about the libvir-list
mailing list