[libvirt] [PATCH v11 0/5] Add native TLS encrypted chardev TCP support

v10: http://www.redhat.com/archives/libvir-list/2016-October/msg00866.html Changes since v10... This is essentially the adding the secret to/for the TLS chardev TCP. Lots of messy stuff. Patches 1-3 are new... In particular, I kept looking at the RNG removal code and kept thinking, something just doesn't look right when compared to the RemoveVirtioDisk code. If I'm incorrect with order of removal it's an easy enough change. Kept trying to think what depends on what and for the RNG it would seem a need a to unplug the backend before removing the front end object would be more logical. Patches 4-5 are the old patch 3-4. They (hopefully) encompass the previous review plus other things since discovered. John Ferlan (5): qemu: Move TLS object remove from DetachChr to RemoveChr qemu: Swap order of RNG hot unplug removal qemu: Need to remove TLS object in RemoveRNGDevice qemu: Add a secret object to/for a char source dev qemu: Add secret object hotplug for TCP chardev TLS src/qemu/qemu_command.c | 30 +++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 173 +++++++++++++++++- src/qemu/qemu_domain.h | 17 +- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_hotplug.c | 195 +++++++++++++++++---- src/qemu/qemu_hotplug.h | 9 +- src/qemu/qemu_process.c | 9 +- tests/qemuhotplugtest.c | 2 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 ++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++ tests/qemuxml2argvtest.c | 17 ++ 12 files changed, 495 insertions(+), 52 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml -- 2.7.4

Commit id '2c32237' added the TLS object removal to the DetachChrDevice all when it should have been added to the RemoveChrDevice since that's the norm for similar processing (e.g. disk) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cf69945..31b5876 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { virObjectEventPtr event; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *charAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int rc; @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup; + if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && + chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + + if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0) + goto exit_monitor; + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); + VIR_FREE(tlsAlias); + virObjectUnref(cfg); return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; } @@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; - char *objAlias = NULL; char *devstr = NULL; - char *charAlias = NULL; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, sa_assert(tmpChr->info.alias); - if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias))) - goto cleanup; - - if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && - tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && - !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; - if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) - goto exit_monitor; - - if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0) - goto exit_monitor; - + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); - VIR_FREE(charAlias); - virObjectUnref(cfg); return ret; - - exit_monitor: - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; } -- 2.7.4

On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
Commit id '2c32237' added the TLS object removal to the DetachChrDevice all when it should have been added to the RemoveChrDevice since that's the norm for similar processing (e.g. disk)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cf69945..31b5876 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { virObjectEventPtr event; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *charAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int rc; @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup;
+ if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && + chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + + if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0) + goto exit_monitor; +
Those two qemu commands should be swapped as well. This is similar to the issues that the next patch fixes. The chardev uses tls object so the chardev should be detached before the tls object. ACK with that fixed. Pavel
rc = qemuMonitorDetachCharDev(priv->mon, charAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
cleanup: VIR_FREE(charAlias); + VIR_FREE(tlsAlias); + virObjectUnref(cfg); return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; }
@@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; - char *objAlias = NULL; char *devstr = NULL; - char *charAlias = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
sa_assert(tmpChr->info.alias);
- if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias))) - goto cleanup; - - if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && - tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && - !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; - if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup;
qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) - goto exit_monitor; - - if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0) - goto exit_monitor; - + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
@@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); - VIR_FREE(charAlias); - virObjectUnref(cfg); return ret; - - exit_monitor: - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/25/2016 08:42 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:17PM -0400, John Ferlan wrote:
Commit id '2c32237' added the TLS object removal to the DetachChrDevice all when it should have been added to the RemoveChrDevice since that's the norm for similar processing (e.g. disk)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cf69945..31b5876 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3549,7 +3549,9 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { virObjectEventPtr event; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *charAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int rc; @@ -3560,7 +3562,16 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup;
+ if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && + chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + + if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0) + goto exit_monitor; +
Those two qemu commands should be swapped as well. This is similar to the issues that the next patch fixes. The chardev uses tls object so the chardev should be detached before the tls object.
Took out my trusty pen and paper today and drew a large chart of attach order, error order, and detach/remove order. Long story short, for this one yes, moving the TLS deletion after the chardev deletion is proper due to dependencies. I've pushed this change. John
ACK with that fixed.
Pavel
rc = qemuMonitorDetachCharDev(priv->mon, charAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3579,7 +3590,13 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
cleanup: VIR_FREE(charAlias); + VIR_FREE(tlsAlias); + virObjectUnref(cfg); return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; }
@@ -4461,13 +4478,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr) { int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; - char *objAlias = NULL; char *devstr = NULL; - char *charAlias = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4480,26 +4494,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
sa_assert(tmpChr->info.alias);
- if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias))) - goto cleanup; - - if (tmpChr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && - tmpChr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && - !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; - if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup;
qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) - goto exit_monitor; - - if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0) - goto exit_monitor; - + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
@@ -4511,13 +4515,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); - VIR_FREE(charAlias); - virObjectUnref(cfg); return ret; - - exit_monitor: - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Rather than remove the frontend first and then the backend, let's swap the order. The order of addition is add the chardev backend if EGD being used, then add the RNG object. So there's a dependency there. When we remove, unplug the backend first, then remove the object would seem to be a more logical step. This would then follow similar processing for disk removal where dependent objects (secret and encryption) of the drive are removed before the drive is removed. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 31b5876..e287aaf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3623,11 +3623,11 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDelObject(priv->mon, objAlias); - - if (rc == 0 && rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + rc = qemuMonitorDelObject(priv->mon, objAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; -- 2.7.4

On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
Rather than remove the frontend first and then the backend, let's swap the order. The order of addition is add the chardev backend if EGD being used, then add the RNG object. So there's a dependency there. When we remove, unplug the backend first, then remove the object would seem to be a more logical step. This would then follow similar processing for disk removal where dependent objects (secret and encryption) of the drive are removed before the drive is removed.
Nice catch, qemuDomainRemoveChrDevice needs the same fix. ACK Pavel

On 10/25/2016 08:48 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
Rather than remove the frontend first and then the backend, let's swap the order. The order of addition is add the chardev backend if EGD being used, then add the RNG object. So there's a dependency there. When we remove, unplug the backend first, then remove the object would seem to be a more logical step. This would then follow similar processing for disk removal where dependent objects (secret and encryption) of the drive are removed before the drive is removed.
Nice catch, qemuDomainRemoveChrDevice needs the same fix.
Going through my pencil/paper exercise, I think I was wrong... Too many objects and similar names. During the attach processing we have (up to this point at least) 1. Add TLS Object (if necessary) 2. Attach chardev for EGD Backend (if necessary) -> NB: Could depend on 1 via 'tlsProps' 3. Add frontend object -> NB: Could depend on 2 via 'props' 4. Add device So the corollary then becomes: 1. Detach Device (in DetachRNGDevice) 2. Delete frontend object (in RemoveRNGDevice) 3. Detach chardev for EGD Backend (if necessary) 4. Delete TLS object When the secobj is added later it's added before TLS and removed after TLS. Since the existing code has the above order, this patch I think is dropped... That does effect the next one, but only insomuch as placement of the delete TLS object per the above order. John

On Tue, Oct 25, 2016 at 03:57:34PM -0400, John Ferlan wrote:
On 10/25/2016 08:48 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:18PM -0400, John Ferlan wrote:
Rather than remove the frontend first and then the backend, let's swap the order. The order of addition is add the chardev backend if EGD being used, then add the RNG object. So there's a dependency there. When we remove, unplug the backend first, then remove the object would seem to be a more logical step. This would then follow similar processing for disk removal where dependent objects (secret and encryption) of the drive are removed before the drive is removed.
Nice catch, qemuDomainRemoveChrDevice needs the same fix.
Going through my pencil/paper exercise, I think I was wrong... Too many objects and similar names.
During the attach processing we have (up to this point at least)
1. Add TLS Object (if necessary) 2. Attach chardev for EGD Backend (if necessary) -> NB: Could depend on 1 via 'tlsProps' 3. Add frontend object -> NB: Could depend on 2 via 'props' 4. Add device
So the corollary then becomes:
1. Detach Device (in DetachRNGDevice) 2. Delete frontend object (in RemoveRNGDevice) 3. Detach chardev for EGD Backend (if necessary) 4. Delete TLS object
When the secobj is added later it's added before TLS and removed after TLS.
Since the existing code has the above order, this patch I think is dropped... That does effect the next one, but only insomuch as placement of the delete TLS object per the above order.
OK I've checked it again and properly this time and I've check qemu command line too: -chardev socket,id=charrng0,host=localhost,port=4466,server,nowait -object rng-egd,id=objrng0,chardev=charrng0 -device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x2 So the dependency is clear and yes this patch was making it wrong. I've for some reason thought that the objAlias means a different object, sigh, shame on me :) Pavel

Commit id '6e6b4bfc' added the object, but forgot the other end. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e287aaf..f401b48 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing RNG device %s from domain %p %s", rng->info.alias, vm, vm->def->name); + if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup; if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + } rc = qemuMonitorDelObject(priv->mon, objAlias); @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(objAlias); + VIR_FREE(tlsAlias); return ret; } -- 2.7.4

On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
Commit id '6e6b4bfc' added the object, but forgot the other end.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
ACK if you agree with review on the first patch that the order of qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped. Pavel
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e287aaf..f401b48 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing RNG device %s from domain %p %s", rng->info.alias, vm, vm->def->name);
+ if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup;
if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup;
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + }
rc = qemuMonitorDelObject(priv->mon, objAlias);
@@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(objAlias); + VIR_FREE(tlsAlias); return ret; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
Commit id '6e6b4bfc' added the object, but forgot the other end.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
ACK if you agree with review on the first patch that the order of qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
Pavel
I do agree, but I hadn't fully convinced myself - I needed the extra set of eyes to help with that... Of course, I botched that again here by removing the tlsobj before the chardev. A problem which naturally persists in patch 5, but is easily adjusted.. I looked at and thought about qemuDomainRemoveRNGDevice long enough and compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my mind and really didn't want to be creating "extra" patches for perhaps all the erroneous removals. If my comparison is {Attach|Remove}Disk, which has... Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive can rely on secobj and/or encobj. The encobj and secobj do not rely on each other. The secobj is the possible secret for the iSCSI/RBD server, while encobj is the possible secret for On attach error the removal is Del Drive, Del secobj, Del encobj (order of secobj/encobj doesn't matter). Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and Del Drive. Then, the RemoveDisk is probably wrong (Del Drive should be first), but I didn't want to continue to bog down the chardev changes if my thoughts in patch 2 were wrong. Long way of saying, I want to get it right for this path and then if I'm right generate a RemoveDisk patch to swap order. John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e287aaf..f401b48 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing RNG device %s from domain %p %s", rng->info.alias, vm, vm->def->name);
+ if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup;
if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup;
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + }
rc = qemuMonitorDelObject(priv->mon, objAlias);
@@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(objAlias); + VIR_FREE(tlsAlias); return ret; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Oct 25, 2016 at 09:19:58AM -0400, John Ferlan wrote:
On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
Commit id '6e6b4bfc' added the object, but forgot the other end.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
ACK if you agree with review on the first patch that the order of qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
Pavel
I do agree, but I hadn't fully convinced myself - I needed the extra set of eyes to help with that... Of course, I botched that again here by removing the tlsobj before the chardev. A problem which naturally persists in patch 5, but is easily adjusted..
I looked at and thought about qemuDomainRemoveRNGDevice long enough and compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my mind and really didn't want to be creating "extra" patches for perhaps all the erroneous removals.
If my comparison is {Attach|Remove}Disk, which has...
Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive can rely on secobj and/or encobj. The encobj and secobj do not rely on each other. The secobj is the possible secret for the iSCSI/RBD server, while encobj is the possible secret for
On attach error the removal is Del Drive, Del secobj, Del encobj (order of secobj/encobj doesn't matter).
Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and Del Drive.
Then, the RemoveDisk is probably wrong (Del Drive should be first), but I didn't want to continue to bog down the chardev changes if my thoughts in patch 2 were wrong.
I think that the RemoveDisk is wrong and that the Del Drive should be first. As you've wrote, the drive can rely on secobj and/or encobj so we should not remove the object while they can be still used. Pavel
Long way of saying, I want to get it right for this path and then if I'm right generate a RemoveDisk patch to swap order.
John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e287aaf..f401b48 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing RNG device %s from domain %p %s", rng->info.alias, vm, vm->def->name);
+ if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup;
if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup;
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + }
rc = qemuMonitorDelObject(priv->mon, objAlias);
@@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(objAlias); + VIR_FREE(tlsAlias); return ret; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/25/2016 12:04 PM, Pavel Hrdina wrote:
On Tue, Oct 25, 2016 at 09:19:58AM -0400, John Ferlan wrote:
On 10/25/2016 08:53 AM, Pavel Hrdina wrote:
On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote:
Commit id '6e6b4bfc' added the object, but forgot the other end.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
ACK if you agree with review on the first patch that the order of qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped.
Pavel
I do agree, but I hadn't fully convinced myself - I needed the extra set of eyes to help with that... Of course, I botched that again here by removing the tlsobj before the chardev. A problem which naturally persists in patch 5, but is easily adjusted..
I looked at and thought about qemuDomainRemoveRNGDevice long enough and compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my mind and really didn't want to be creating "extra" patches for perhaps all the erroneous removals.
If my comparison is {Attach|Remove}Disk, which has...
Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive can rely on secobj and/or encobj. The encobj and secobj do not rely on each other. The secobj is the possible secret for the iSCSI/RBD server, while encobj is the possible secret for
On attach error the removal is Del Drive, Del secobj, Del encobj (order of secobj/encobj doesn't matter).
Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and Del Drive.
Then, the RemoveDisk is probably wrong (Del Drive should be first), but I didn't want to continue to bog down the chardev changes if my thoughts in patch 2 were wrong.
I think that the RemoveDisk is wrong and that the Del Drive should be first. As you've wrote, the drive can rely on secobj and/or encobj so we should not remove the object while they can be still used.
RemoveDisk is a different/separate issue - I sent a separate patch. As for the rest of this - I think in my latest response to 2/5 - the pencil and paper exercise shows the delete TLS object goes after the Detach chardev below. That then just leaves fixing up the order for the secret object in patch 5 of this series. John
Pavel
Long way of saying, I want to get it right for this path and then if I'm right generate a RemoveDisk patch to swap order.
John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e287aaf..f401b48 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; + char *tlsAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing RNG device %s from domain %p %s", rng->info.alias, vm, vm->def->name);
+ if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup;
if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup;
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + }
rc = qemuMonitorDelObject(priv->mon, objAlias);
@@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(objAlias); + VIR_FREE(tlsAlias); return ret; }
-- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Add the secret object so the 'passwordid=' can be added if the command line if there's a secret defined in/on the host for TCP chardev TLS objects. Preparation for the secret involves adding the secinfo to the char source device prior to command line processing. There are multiple possibilities for TCP chardev source backend usage. Add test for at least a serial chardev as an example. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 30 +++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 173 ++++++++++++++++++++- src/qemu/qemu_domain.h | 17 +- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 9 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 +++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++ tests/qemuxml2argvtest.c | 17 ++ 9 files changed, 327 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6bf6510..1c07959 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -695,6 +695,7 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, * @tlspath: path to the TLS credentials * @listen: boolen listen for client or server setting * @verifypeer: boolean to enable peer verification (form of authorization) + * @secalias: if one exists, the alias of the security object for passwordid * @qemuCaps: capabilities * @propsret: json properties to return * @@ -706,6 +707,7 @@ int qemuBuildTLSx509BackendProps(const char *tlspath, bool isListen, bool verifypeer, + const char *secalias, virQEMUCapsPtr qemuCaps, virJSONValuePtr *propsret) { @@ -731,6 +733,10 @@ qemuBuildTLSx509BackendProps(const char *tlspath, NULL) < 0) goto cleanup; + if (secalias && + virJSONValueObjectAdd(*propsret, "s:passwordid", secalias, NULL) < 0) + goto cleanup; + ret = 0; cleanup: @@ -745,6 +751,7 @@ qemuBuildTLSx509BackendProps(const char *tlspath, * @tlspath: path to the TLS credentials * @listen: boolen listen for client or server setting * @verifypeer: boolean to enable peer verification (form of authorization) + * @addpasswordid: boolean to handle adding passwordid to object * @inalias: Alias for the parent to generate object alias * @qemuCaps: capabilities * @@ -757,6 +764,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, const char *tlspath, bool isListen, bool verifypeer, + bool addpasswordid, const char *inalias, virQEMUCapsPtr qemuCaps) { @@ -764,11 +772,16 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, char *objalias = NULL; virJSONValuePtr props = NULL; char *tmp = NULL; + char *secalias = NULL; - if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, - qemuCaps, &props) < 0) + if (addpasswordid && + !(secalias = qemuDomainGetSecretAESAlias(inalias, false))) return -1; + if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, secalias, + qemuCaps, &props) < 0) + goto cleanup; + if (!(objalias = qemuAliasTLSObjFromChardevAlias(inalias))) goto cleanup; @@ -784,6 +797,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, virJSONValueFree(props); VIR_FREE(objalias); VIR_FREE(tmp); + VIR_FREE(secalias); return ret; } @@ -4936,11 +4950,23 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); char *objalias = NULL; + /* Add the secret object first if necessary. The + * secinfo is added only to a TCP serial device during + * qemuDomainSecretChardevPrepare. Subsequently called + * functions can just check the config fields */ + if (chrSourcePriv && chrSourcePriv->secinfo && + qemuBuildObjectSecretCommandLine(cmd, + chrSourcePriv->secinfo) < 0) + goto error; + if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + !!cfg->chardevTLSx509secretUUID, charAlias, qemuCaps) < 0) goto error; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2f2a6ff..a793fb6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -69,6 +69,7 @@ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, int qemuBuildTLSx509BackendProps(const char *tlspath, bool isListen, bool verifypeer, + const char *secalias, virQEMUCapsPtr qemuCaps, virJSONValuePtr *propsret); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 41ac52d..38c0f18 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1042,7 +1042,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_VOLUME)) { + secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME || + secretUsageType == VIR_SECRET_USAGE_TYPE_TLS)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, seclookupdef, isLuks) < 0) @@ -1220,6 +1221,97 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, } +/* qemuDomainSecretChardevDestroy: + * @disk: Pointer to a chardev definition + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) +{ + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); + + if (!chrSourcePriv || !chrSourcePriv->secinfo) + return; + + qemuDomainSecretInfoFree(&chrSourcePriv->secinfo); +} + + +/* qemuDomainSecretChardevPrepare: + * @conn: Pointer to connection + * @driver: Pointer to driver object + * @priv: pointer to domain private object + * @chrAlias: Alias of the chr device + * @dev: Pointer to a char source definition + * + * For a TCP character device, generate a qemuDomainSecretInfo to be used + * by the command line code to generate the secret for the tls-creds to use. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + const char *chrAlias, + virDomainChrSourceDefPtr dev) +{ + virQEMUDriverConfigPtr cfg; + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + char *charAlias = NULL; + + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + cfg->chardevTLSx509secretUUID) { + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); + + if (virUUIDParse(cfg->chardevTLSx509secretUUID, + seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("malformed chardev TLS secret uuid in qemu.conf")); + goto error; + } + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + + if (VIR_ALLOC(secinfo) < 0) + goto error; + + if (!(charAlias = qemuAliasChardevFromDevAlias(chrAlias))) + goto error; + + if (qemuDomainSecretSetup(conn, priv, secinfo, charAlias, + VIR_SECRET_USAGE_TYPE_TLS, NULL, + &seclookupdef, false) < 0) + goto error; + + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TLS X.509 requires encrypted secrets " + "to be supported")); + goto error; + } + + chrSourcePriv->secinfo = secinfo; + } + + VIR_FREE(charAlias); + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1236,11 +1328,38 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) for (i = 0; i < vm->def->nhostdevs; i++) qemuDomainSecretHostdevDestroy(vm->def->hostdevs[i]); + + for (i = 0; i < vm->def->nserials; i++) + qemuDomainSecretChardevDestroy(vm->def->serials[i]->source); + + for (i = 0; i < vm->def->nparallels; i++) + qemuDomainSecretChardevDestroy(vm->def->parallels[i]->source); + + for (i = 0; i < vm->def->nchannels; i++) + qemuDomainSecretChardevDestroy(vm->def->channels[i]->source); + + for (i = 0; i < vm->def->nconsoles; i++) + qemuDomainSecretChardevDestroy(vm->def->consoles[i]->source); + + for (i = 0; i < vm->def->nsmartcards; i++) { + if (vm->def->smartcards[i]->type == + VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH) + qemuDomainSecretChardevDestroy(vm->def->smartcards[i]->data.passthru); + } + + for (i = 0; i < vm->def->nrngs; i++) { + if (vm->def->rngs[i]->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + qemuDomainSecretChardevDestroy(vm->def->rngs[i]->source.chardev); + } + + for (i = 0; i < vm->def->nredirdevs; i++) + qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source); } /* qemuDomainSecretPrepare: * @conn: Pointer to connection + * @driver: Pointer to driver object * @vm: Domain object * * For any objects that may require an auth/secret setup, create a @@ -1253,6 +1372,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1269,6 +1389,57 @@ qemuDomainSecretPrepare(virConnectPtr conn, return -1; } + for (i = 0; i < vm->def->nserials; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->serials[i]->info.alias, + vm->def->serials[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nparallels; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->parallels[i]->info.alias, + vm->def->parallels[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nchannels; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->channels[i]->info.alias, + vm->def->channels[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nconsoles; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->consoles[i]->info.alias, + vm->def->consoles[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nsmartcards; i++) + if (vm->def->smartcards[i]->type == + VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH && + qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->smartcards[i]->info.alias, + vm->def->smartcards[i]->data.passthru) < 0) + return -1; + + for (i = 0; i < vm->def->nrngs; i++) { + if (vm->def->rngs[i]->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->rngs[i]->info.alias, + vm->def->rngs[i]->source.chardev) < 0) + return -1; + } + + for (i = 0; i < vm->def->nredirdevs; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->redirdevs[i]->info.alias, + vm->def->redirdevs[i]->source) < 0) + return -1; + } + return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4f9bf82..0820afd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -726,11 +726,24 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + const char *chrAlias, + virDomainChrSourceDefPtr dev) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4); + void qemuDomainSecretDestroy(virDomainObjPtr vm) ATTRIBUTE_NONNULL(1); -int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + virDomainObjPtr vm) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuDomainDefValidateDiskLunSource(const virStorageSource *src) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f401b48..ba339a3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1489,6 +1489,7 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + NULL, priv->qemuCaps, tlsProps) < 0) return -1; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 33b78b1..1b67aee 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5157,8 +5157,11 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; - VIR_DEBUG("Add secrets to disks and hostdevs"); - if (qemuDomainSecretPrepare(conn, vm) < 0) + VIR_DEBUG("Prepare chardev source backends for TLS"); + qemuDomainPrepareChardevSource(vm->def, driver); + + VIR_DEBUG("Add secrets to disks, hostdevs, and chardevs"); + if (qemuDomainSecretPrepare(conn, driver, vm) < 0) goto cleanup; for (i = 0; i < vm->def->nchannels; i++) { @@ -5167,8 +5170,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; } - qemuDomainPrepareChardevSource(vm->def, driver); - if (VIR_ALLOC(priv->monConfig) < 0) goto cleanup; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args new file mode 100644 index 0000000..7f9fedb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args @@ -0,0 +1,38 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ +localport=1111 \ +-device isa-serial,chardev=charserial0,id=serial0 \ +-object secret,id=charserial1-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +endpoint=client,verify-peer=no,passwordid=charserial1-secret0 \ +-chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ +tls-creds=objcharserial1_tls0 \ +-device isa-serial,chardev=charserial1,id=serial1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml new file mode 100644 index 0000000..832e2a2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml @@ -0,0 +1,50 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <serial type='udp'> + <source mode='bind' host='127.0.0.1' service='1111'/> + <source mode='connect' host='127.0.0.1' service='2222'/> + <target port='0'/> + </serial> + <serial type='tcp'> + <source mode='connect' host='127.0.0.1' service='5555'/> + <protocol type='raw'/> + <target port='0'/> + </serial> + <console type='udp'> + <source mode='bind' host='127.0.0.1' service='1111'/> + <source mode='connect' host='127.0.0.1' service='2222'/> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 52d85fa..541a498 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1170,6 +1170,23 @@ mymain(void) DO_TEST("serial-tcp-tlsx509-chardev-notls", QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_TLS_CREDS_X509); + VIR_FREE(driver.config->chardevTLSx509certdir); + if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509certdir, "/etc/pki/libvirt-chardev") < 0) + return EXIT_FAILURE; + if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509secretUUID, + "6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea") < 0) + return EXIT_FAILURE; +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + DO_TEST("serial-tcp-tlsx509-secret-chardev", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_SECRET, + QEMU_CAPS_OBJECT_TLS_CREDS_X509); +# else + DO_TEST_FAILURE("serial-tcp-tlsx509-secret-chardev", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_SECRET, + QEMU_CAPS_OBJECT_TLS_CREDS_X509); +# endif driver.config->chardevTLS = 0; VIR_FREE(driver.config->chardevTLSx509certdir); DO_TEST("serial-many-chardev", -- 2.7.4

On Mon, Oct 24, 2016 at 06:46:20PM -0400, John Ferlan wrote:
Add the secret object so the 'passwordid=' can be added if the command line if there's a secret defined in/on the host for TCP chardev TLS objects.
Preparation for the secret involves adding the secinfo to the char source device prior to command line processing. There are multiple possibilities for TCP chardev source backend usage.
Add test for at least a serial chardev as an example.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 30 +++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 173 ++++++++++++++++++++- src/qemu/qemu_domain.h | 17 +- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 9 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 +++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++ tests/qemuxml2argvtest.c | 17 ++ 9 files changed, 327 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6bf6510..1c07959 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -695,6 +695,7 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, * @tlspath: path to the TLS credentials * @listen: boolen listen for client or server setting * @verifypeer: boolean to enable peer verification (form of authorization) + * @secalias: if one exists, the alias of the security object for passwordid * @qemuCaps: capabilities * @propsret: json properties to return * @@ -706,6 +707,7 @@ int qemuBuildTLSx509BackendProps(const char *tlspath, bool isListen, bool verifypeer, + const char *secalias, virQEMUCapsPtr qemuCaps, virJSONValuePtr *propsret) { @@ -731,6 +733,10 @@ qemuBuildTLSx509BackendProps(const char *tlspath, NULL) < 0) goto cleanup;
+ if (secalias && + virJSONValueObjectAdd(*propsret, "s:passwordid", secalias, NULL) < 0) + goto cleanup; + ret = 0;
cleanup: @@ -745,6 +751,7 @@ qemuBuildTLSx509BackendProps(const char *tlspath, * @tlspath: path to the TLS credentials * @listen: boolen listen for client or server setting * @verifypeer: boolean to enable peer verification (form of authorization) + * @addpasswordid: boolean to handle adding passwordid to object * @inalias: Alias for the parent to generate object alias * @qemuCaps: capabilities * @@ -757,6 +764,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, const char *tlspath, bool isListen, bool verifypeer, + bool addpasswordid, const char *inalias, virQEMUCapsPtr qemuCaps) { @@ -764,11 +772,16 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, char *objalias = NULL; virJSONValuePtr props = NULL; char *tmp = NULL; + char *secalias = NULL;
- if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, - qemuCaps, &props) < 0) + if (addpasswordid && + !(secalias = qemuDomainGetSecretAESAlias(inalias, false))) return -1;
+ if (qemuBuildTLSx509BackendProps(tlspath, isListen, verifypeer, secalias, + qemuCaps, &props) < 0) + goto cleanup; + if (!(objalias = qemuAliasTLSObjFromChardevAlias(inalias))) goto cleanup;
@@ -784,6 +797,7 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, virJSONValueFree(props); VIR_FREE(objalias); VIR_FREE(tmp); + VIR_FREE(secalias); return ret; }
@@ -4936,11 +4950,23 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); char *objalias = NULL;
+ /* Add the secret object first if necessary. The + * secinfo is added only to a TCP serial device during + * qemuDomainSecretChardevPrepare. Subsequently called + * functions can just check the config fields */ + if (chrSourcePriv && chrSourcePriv->secinfo && + qemuBuildObjectSecretCommandLine(cmd, + chrSourcePriv->secinfo) < 0) + goto error; + if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + !!cfg->chardevTLSx509secretUUID, charAlias, qemuCaps) < 0) goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2f2a6ff..a793fb6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -69,6 +69,7 @@ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, int qemuBuildTLSx509BackendProps(const char *tlspath, bool isListen, bool verifypeer, + const char *secalias, virQEMUCapsPtr qemuCaps, virJSONValuePtr *propsret);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 41ac52d..38c0f18 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1042,7 +1042,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_VOLUME)) { + secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME || + secretUsageType == VIR_SECRET_USAGE_TYPE_TLS)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, seclookupdef, isLuks) < 0) @@ -1220,6 +1221,97 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretChardevDestroy: + * @disk: Pointer to a chardev definition + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) +{ + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); + + if (!chrSourcePriv || !chrSourcePriv->secinfo) + return; + + qemuDomainSecretInfoFree(&chrSourcePriv->secinfo); +} + + +/* qemuDomainSecretChardevPrepare: + * @conn: Pointer to connection + * @driver: Pointer to driver object + * @priv: pointer to domain private object + * @chrAlias: Alias of the chr device + * @dev: Pointer to a char source definition + * + * For a TCP character device, generate a qemuDomainSecretInfo to be used + * by the command line code to generate the secret for the tls-creds to use. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + const char *chrAlias, + virDomainChrSourceDefPtr dev) +{ + virQEMUDriverConfigPtr cfg; + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + char *charAlias = NULL; + + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver);
The *driver* is used only to get the *cfg* and the whole function is used in several loops so it would be better to pass *cfg* directly to this function in order to save a lot of locks & unlocks and refs & unrefs.
+ if (dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + cfg->chardevTLSx509secretUUID) { + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); + + if (virUUIDParse(cfg->chardevTLSx509secretUUID, + seclookupdef.u.uuid) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("malformed chardev TLS secret uuid in qemu.conf")); + goto error; + } + seclookupdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; + + if (VIR_ALLOC(secinfo) < 0) + goto error; + + if (!(charAlias = qemuAliasChardevFromDevAlias(chrAlias))) + goto error; + + if (qemuDomainSecretSetup(conn, priv, secinfo, charAlias, + VIR_SECRET_USAGE_TYPE_TLS, NULL, + &seclookupdef, false) < 0) + goto error; + + if (secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TLS X.509 requires encrypted secrets " + "to be supported")); + goto error; + } + + chrSourcePriv->secinfo = secinfo; + } + + VIR_FREE(charAlias); + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1236,11 +1328,38 @@ qemuDomainSecretDestroy(virDomainObjPtr vm)
for (i = 0; i < vm->def->nhostdevs; i++) qemuDomainSecretHostdevDestroy(vm->def->hostdevs[i]); + + for (i = 0; i < vm->def->nserials; i++) + qemuDomainSecretChardevDestroy(vm->def->serials[i]->source); + + for (i = 0; i < vm->def->nparallels; i++) + qemuDomainSecretChardevDestroy(vm->def->parallels[i]->source); + + for (i = 0; i < vm->def->nchannels; i++) + qemuDomainSecretChardevDestroy(vm->def->channels[i]->source); + + for (i = 0; i < vm->def->nconsoles; i++) + qemuDomainSecretChardevDestroy(vm->def->consoles[i]->source); + + for (i = 0; i < vm->def->nsmartcards; i++) { + if (vm->def->smartcards[i]->type == + VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH) + qemuDomainSecretChardevDestroy(vm->def->smartcards[i]->data.passthru); + } + + for (i = 0; i < vm->def->nrngs; i++) { + if (vm->def->rngs[i]->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + qemuDomainSecretChardevDestroy(vm->def->rngs[i]->source.chardev); + } + + for (i = 0; i < vm->def->nredirdevs; i++) + qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source); }
/* qemuDomainSecretPrepare: * @conn: Pointer to connection + * @driver: Pointer to driver object * @vm: Domain object * * For any objects that may require an auth/secret setup, create a @@ -1253,6 +1372,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1269,6 +1389,57 @@ qemuDomainSecretPrepare(virConnectPtr conn, return -1; }
+ for (i = 0; i < vm->def->nserials; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->serials[i]->info.alias, + vm->def->serials[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nparallels; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->parallels[i]->info.alias, + vm->def->parallels[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nchannels; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->channels[i]->info.alias, + vm->def->channels[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nconsoles; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->consoles[i]->info.alias, + vm->def->consoles[i]->source) < 0) + return -1; + } + + for (i = 0; i < vm->def->nsmartcards; i++) + if (vm->def->smartcards[i]->type == + VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH && + qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->smartcards[i]->info.alias, + vm->def->smartcards[i]->data.passthru) < 0) + return -1; + + for (i = 0; i < vm->def->nrngs; i++) { + if (vm->def->rngs[i]->backend == VIR_DOMAIN_RNG_BACKEND_EGD && + qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->rngs[i]->info.alias, + vm->def->rngs[i]->source.chardev) < 0) + return -1; + } + + for (i = 0; i < vm->def->nredirdevs; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->redirdevs[i]->info.alias, + vm->def->redirdevs[i]->source) < 0) + return -1; + } + return 0; }
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4f9bf82..0820afd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -726,11 +726,24 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+void qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + const char *chrAlias, + virDomainChrSourceDefPtr dev) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4);
And ATTRIBUTE_NONNULL(5) ACK with the issues fixed. Pavel

https://bugzilla.redhat.com/show_bug.cgi?id=1300776 Complete the implementation of support for TLS encryption on chardev TCP transports by adding the hotplug ability of a secret to generate the passwordid for the TLS object for chrdev, RNG, and redirdev. Likewise, add the ability to hot unplug that secret object as well Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_hotplug.c | 148 ++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_hotplug.h | 9 ++- tests/qemuhotplugtest.c | 2 +- 4 files changed, 141 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 93ea5e2..7555cd7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7558,7 +7558,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainAttachRedirdevDevice(driver, vm, + ret = qemuDomainAttachRedirdevDevice(conn, driver, vm, dev->data.redirdev); if (!ret) { alias = dev->data.redirdev->info.alias; @@ -7567,7 +7567,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainAttachChrDevice(driver, vm, + ret = qemuDomainAttachChrDevice(conn, driver, vm, dev->data.chr); if (!ret) { alias = dev->data.chr->info.alias; @@ -7576,7 +7576,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainAttachRNGDevice(driver, vm, + ret = qemuDomainAttachRNGDevice(conn, driver, vm, dev->data.rng); if (!ret) { alias = dev->data.rng->info.alias; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ba339a3..189e03f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1480,16 +1480,31 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, virDomainChrSourceDefPtr dev, char *charAlias, virJSONValuePtr *tlsProps, - char **tlsAlias) + char **tlsAlias, + virJSONValuePtr *secProps, + char **secAlias) { + qemuDomainChrSourcePrivatePtr chrSourcePriv = + QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); + if (dev->type != VIR_DOMAIN_CHR_TYPE_TCP || dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_YES) return 0; + /* Add a secret object in order to access the TLS environment. + * The secinfo will only be created for serial TCP device. */ + if (chrSourcePriv && chrSourcePriv->secinfo) { + if (qemuBuildSecretInfoProps(chrSourcePriv->secinfo, secProps) < 0) + return -1; + + if (!(*secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + return -1; + } + if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - NULL, + *secAlias, priv->qemuCaps, tlsProps) < 0) return -1; @@ -1502,7 +1517,8 @@ qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg, } -int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, +int qemuDomainAttachRedirdevDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr redirdev) { @@ -1515,8 +1531,11 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, char *devstr = NULL; bool chardevAdded = false; bool tlsobjAdded = false; + bool secobjAdded = false; virJSONValuePtr tlsProps = NULL; + virJSONValuePtr secProps = NULL; char *tlsAlias = NULL; + char *secAlias = NULL; virErrorPtr orig_err; qemuDomainPrepareChardevSourceTLS(redirdev->source, cfg); @@ -1533,11 +1552,26 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, if (VIR_REALLOC_N(def->redirdevs, def->nredirdevs+1) < 0) goto cleanup; + if (qemuDomainSecretChardevPrepare(conn, driver, priv, redirdev->info.alias, + redirdev->source) < 0) + goto cleanup; + if (qemuDomainGetChardevTLSObjects(cfg, priv, redirdev->source, - charAlias, &tlsProps, &tlsAlias) < 0) + charAlias, &tlsProps, &tlsAlias, + &secProps, &secAlias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); + + if (secAlias) { + rc = qemuMonitorAddObject(priv->mon, "secret", + secAlias, secProps); + secProps = NULL; + if (rc < 0) + goto exit_monitor; + secobjAdded = true; + } + if (tlsAlias) { rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", tlsAlias, tlsProps); @@ -1566,6 +1600,8 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(tlsAlias); virJSONValueFree(tlsProps); + VIR_FREE(secAlias); + virJSONValueFree(secProps); VIR_FREE(charAlias); VIR_FREE(devstr); virObjectUnref(cfg); @@ -1573,6 +1609,8 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, exit_monitor: orig_err = virSaveLastError(); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (tlsobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); /* detach associated chardev on error */ @@ -1753,7 +1791,8 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainObjPtr vm, return ret; } -int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, +int qemuDomainAttachChrDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { @@ -1767,8 +1806,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, char *charAlias = NULL; bool chardevAttached = false; bool tlsobjAdded = false; + bool secobjAdded = false; virJSONValuePtr tlsProps = NULL; char *tlsAlias = NULL; + virJSONValuePtr secProps = NULL; + char *secAlias = NULL; bool need_release = false; if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && @@ -1794,11 +1836,25 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; + if (qemuDomainSecretChardevPrepare(conn, driver, priv, chr->info.alias, + dev) < 0) + goto cleanup; + if (qemuDomainGetChardevTLSObjects(cfg, priv, dev, charAlias, - &tlsProps, &tlsAlias) < 0) + &tlsProps, &tlsAlias, + &secProps, &secAlias) < 0) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); + if (secAlias) { + rc = qemuMonitorAddObject(priv->mon, "secret", + secAlias, secProps); + secProps = NULL; + if (rc < 0) + goto exit_monitor; + secobjAdded = true; + } + if (tlsAlias) { rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509", tlsAlias, tlsProps); @@ -1829,6 +1885,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); VIR_FREE(tlsAlias); virJSONValueFree(tlsProps); + VIR_FREE(secAlias); + virJSONValueFree(secProps); VIR_FREE(charAlias); VIR_FREE(devstr); virObjectUnref(cfg); @@ -1836,6 +1894,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, exit_monitor: orig_err = virSaveLastError(); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (tlsobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); /* detach associated chardev on error */ @@ -1852,7 +1912,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, int -qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, +qemuDomainAttachRNGDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng) { @@ -1863,12 +1924,15 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, char *charAlias = NULL; char *objAlias = NULL; char *tlsAlias = NULL; + char *secAlias = NULL; bool releaseaddr = false; bool chardevAdded = false; bool objAdded = false; bool tlsobjAdded = false; + bool secobjAdded = false; virJSONValuePtr props = NULL; virJSONValuePtr tlsProps = NULL; + virJSONValuePtr secProps = NULL; virDomainCCWAddressSetPtr ccwaddrs = NULL; const char *type; int ret = -1; @@ -1923,13 +1987,28 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && - qemuDomainGetChardevTLSObjects(cfg, priv, rng->source.chardev, - charAlias, &tlsProps, &tlsAlias) < 0) - goto cleanup; + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, rng->info.alias, + rng->source.chardev) < 0) + goto cleanup; + + if (qemuDomainGetChardevTLSObjects(cfg, priv, rng->source.chardev, + charAlias, &tlsProps, &tlsAlias, + &secProps, &secAlias) < 0) + goto cleanup; + } qemuDomainObjEnterMonitor(driver, vm); + if (secAlias) { + rv = qemuMonitorAddObject(priv->mon, "secret", + secAlias, secProps); + secProps = NULL; + if (rv < 0) + goto exit_monitor; + secobjAdded = true; + } + if (tlsAlias) { rv = qemuMonitorAddObject(priv->mon, "tls-creds-x509", tlsAlias, tlsProps); @@ -1967,10 +2046,12 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0); cleanup: virJSONValueFree(tlsProps); + virJSONValueFree(secProps); virJSONValueFree(props); if (ret < 0 && releaseaddr) qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); VIR_FREE(tlsAlias); + VIR_FREE(secAlias); VIR_FREE(charAlias); VIR_FREE(objAlias); VIR_FREE(devstr); @@ -1980,6 +2061,8 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, exit_monitor: orig_err = virSaveLastError(); + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); if (tlsobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); if (objAdded) @@ -3553,6 +3636,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *charAlias = NULL; char *tlsAlias = NULL; + char *secAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int rc; @@ -3564,15 +3648,29 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, goto cleanup; if (chr->source->type == VIR_DOMAIN_CHR_TYPE_TCP && - chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && - !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; + chr->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { + + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + + /* Best shot at this as the secinfo is destroyed after process launch + * and this path does not recreate it. Thus, if the config has the + * secret UUID and we have a serial TCP chardev, then formulate a + * secAlias which we'll attempt to destroy. */ + if (cfg->chardevTLSx509secretUUID && + !(secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + goto cleanup; + } qemuDomainObjEnterMonitor(driver, vm); if (tlsAlias && qemuMonitorDelObject(priv->mon, tlsAlias) < 0) goto exit_monitor; + /* If it fails, then so be it - it was a best shot */ + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; @@ -3592,6 +3690,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(charAlias); VIR_FREE(tlsAlias); + VIR_FREE(secAlias); virObjectUnref(cfg); return ret; @@ -3607,9 +3706,11 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virDomainRNGDefPtr rng) { virObjectEventPtr event; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *charAlias = NULL; char *objAlias = NULL; char *tlsAlias = NULL; + char *secAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; int ret = -1; @@ -3625,14 +3726,25 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && - !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) + goto cleanup; + + /* Best shot at this as the secinfo is destroyed after process launch + * and this path does not recreate it. Thus, if the config has the + * secret UUID and we have a serial TCP chardev, then formulate a + * secAlias which we'll attempt to destroy. */ + if (cfg->chardevTLSx509secretUUID && + !(secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + goto cleanup; + } qemuDomainObjEnterMonitor(driver, vm); if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { if (tlsAlias) ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); } @@ -3659,6 +3771,8 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, VIR_FREE(charAlias); VIR_FREE(objAlias); VIR_FREE(tlsAlias); + VIR_FREE(secAlias); + virObjectUnref(cfg); return ret; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index b048cf4..d64f1da 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -43,7 +43,8 @@ int qemuDomainAttachDeviceDiskLive(virConnectPtr conn, int qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainNetDefPtr net); -int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, +int qemuDomainAttachRedirdevDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr hostdev); int qemuDomainAttachHostDevice(virConnectPtr conn, @@ -92,13 +93,15 @@ int qemuDomainAttachLease(virQEMUDriverPtr driver, int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); -int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, +int qemuDomainAttachChrDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr); int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr); -int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, +int qemuDomainAttachRNGDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 43eb1cf..14561a8 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -118,7 +118,7 @@ testQemuHotplugAttach(virDomainObjPtr vm, ret = qemuDomainAttachDeviceDiskLive(NULL, &driver, vm, dev); break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + ret = qemuDomainAttachChrDevice(NULL, &driver, vm, dev->data.chr); break; default: VIR_TEST_VERBOSE("device type '%s' cannot be attached\n", -- 2.7.4

On Mon, Oct 24, 2016 at 06:46:21PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1300776
Complete the implementation of support for TLS encryption on chardev TCP transports by adding the hotplug ability of a secret to generate the passwordid for the TLS object for chrdev, RNG, and redirdev.
Likewise, add the ability to hot unplug that secret object as well
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_hotplug.c | 148 ++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_hotplug.h | 9 ++- tests/qemuhotplugtest.c | 2 +- 4 files changed, 141 insertions(+), 24 deletions(-)
ACK with the ordering fixed. Pavel
participants (2)
-
John Ferlan
-
Pavel Hrdina