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

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

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.

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)

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

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

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