Re: [libvirt] [PATCH v3 10/10] qemu: Add luks support for domain disk

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(a)redhat.com&gt; > --- > 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?

> 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.

> 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/

> + 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.

> + > + 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?

> + goto failaddencsecret; s/failaddencsecret/exit_monitor/

> + encProps = NULL; > + > if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) > goto failadddrive; > ACK only if we don't need to deal with diskPriv->secinfo;