[libvirt] [PATCHv2 0/8]
For: https://bugzilla.redhat.com/show_bug.cgi?id=1602418 v1: https://www.redhat.com/archives/libvir-list/2019-January/msg00490.html v2: * fixed memory leaks pointed out by John in v1 Patches without R-b: [4/8] cowardly refused to unref the private data in Secret.*Destroy [8/8] fixed the logic to include checking whether the domain actually has a VNC graphics and amended the commit message Ján Tomko (8): conf: introduce virDomainGraphicsNew conf: add privateData to virDomainGraphicsDef qemu: add qemuDomainGraphicsPrivate data with a tlsAlias qemu: prepare secret for the graphics upfront qemu_process: fix debug message qemu.conf: add vnc_tls_x509_secret_uuid qemu: add support for encrypted VNC TLS keys qemu: error out when vnc vncTLSx509secretUUID is unsupported src/conf/domain_conf.c | 30 ++++- src/conf/domain_conf.h | 5 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 6 + src/qemu/qemu_command.c | 19 +++- src/qemu/qemu_conf.c | 4 + src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 107 ++++++++++++++++++ src/qemu/qemu_domain.h | 13 +++ src/qemu/qemu_process.c | 2 +- src/qemu/test_libvirtd_qemu.aug.in | 1 + ...graphics-vnc-tls-secret.x86_64-latest.args | 36 ++++++ .../graphics-vnc-tls-secret.xml | 30 +++++ tests/qemuxml2argvtest.c | 5 + 14 files changed, 250 insertions(+), 10 deletions(-) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-tls-secret.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-tls-secret.xml -- 2.20.1
A helper function for allocating the virDomainGraphicsDef structure. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 23 +++++++++++++++++++---- src/conf/domain_conf.h | 2 ++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 222bb8c482..761f9bffef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14115,9 +14115,22 @@ virDomainGraphicsDefParseXMLEGLHeadless(virDomainGraphicsDefPtr def, } +virDomainGraphicsDefPtr +virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) +{ + virDomainGraphicsDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) + return NULL; + + return def; +} + + /* Parse the XML definition for a graphics device */ static virDomainGraphicsDefPtr -virDomainGraphicsDefParseXML(xmlNodePtr node, +virDomainGraphicsDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -14125,7 +14138,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, char *type = NULL; int typeVal; - if (VIR_ALLOC(def) < 0) + if (!(def = virDomainGraphicsDefNew(xmlopt))) return NULL; type = virXMLPropString(node, "type"); @@ -16237,7 +16250,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_GRAPHICS: - if (!(dev->data.graphics = virDomainGraphicsDefParseXML(node, ctxt, flags))) + if (!(dev->data.graphics = virDomainGraphicsDefParseXML(xmlopt, node, + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_HUB: @@ -20847,7 +20861,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->graphics, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainGraphicsDefPtr graphics = virDomainGraphicsDefParseXML(nodes[i], + virDomainGraphicsDefPtr graphics = virDomainGraphicsDefParseXML(xmlopt, + nodes[i], ctxt, flags); if (!graphics) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fae130668f..2a97ad8ab3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2954,6 +2954,8 @@ virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt); virDomainChrDefPtr virDomainChrDefNew(virDomainXMLOptionPtr xmlopt); +virDomainGraphicsDefPtr +virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt); virDomainDefPtr virDomainDefNew(void); void virDomainObjAssignDef(virDomainObjPtr domain, -- 2.20.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 9 ++++++++- src/conf/domain_conf.h | 3 +++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 761f9bffef..54d6364f4f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1431,6 +1431,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) virDomainGraphicsListenDefClear(&def->listens[i]); VIR_FREE(def->listens); + virObjectUnref(def->privateData); VIR_FREE(def); } @@ -14116,13 +14117,19 @@ virDomainGraphicsDefParseXMLEGLHeadless(virDomainGraphicsDefPtr def, virDomainGraphicsDefPtr -virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED) +virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt) { virDomainGraphicsDefPtr def = NULL; if (VIR_ALLOC(def) < 0) return NULL; + if (xmlopt && xmlopt->privateData.graphicsNew && + !(def->privateData = xmlopt->privateData.graphicsNew())) { + VIR_FREE(def); + def = NULL; + } + return def; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2a97ad8ab3..7776a3afb2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1603,6 +1603,8 @@ struct _virDomainGraphicsListenDef { }; struct _virDomainGraphicsDef { + virObjectPtr privateData; + /* Port value discipline: * Value -1 is legacy syntax indicating that it should be auto-allocated. * Value 0 means port wasn't specified in XML at all. @@ -2783,6 +2785,7 @@ struct _virDomainXMLPrivateDataCallbacks { virDomainXMLPrivateDataNewFunc vcpuNew; virDomainXMLPrivateDataNewFunc chrSourceNew; virDomainXMLPrivateDataNewFunc vsockNew; + virDomainXMLPrivateDataNewFunc graphicsNew; virDomainXMLPrivateDataFormatFunc format; virDomainXMLPrivateDataParseFunc parse; /* following function shall return a pointer which will be used as the -- 2.20.1
Also introduce the necessary callbacks. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 12 ++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e07c1646f1..4b11cba1bd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1234,6 +1234,44 @@ qemuDomainVsockPrivateDispose(void *obj ATTRIBUTE_UNUSED) } +static virClassPtr qemuDomainGraphicsPrivateClass; +static void qemuDomainGraphicsPrivateDispose(void *obj); + +static int +qemuDomainGraphicsPrivateOnceInit(void) +{ + if (!VIR_CLASS_NEW(qemuDomainGraphicsPrivate, virClassForObject())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(qemuDomainGraphicsPrivate) + +static virObjectPtr +qemuDomainGraphicsPrivateNew(void) +{ + qemuDomainGraphicsPrivatePtr priv; + + if (qemuDomainGraphicsPrivateInitialize() < 0) + return NULL; + + if (!(priv = virObjectNew(qemuDomainGraphicsPrivateClass))) + return NULL; + + return (virObjectPtr) priv; +} + + +static void +qemuDomainGraphicsPrivateDispose(void *obj) +{ + qemuDomainGraphicsPrivatePtr priv = obj; + + VIR_FREE(priv->tlsAlias); +} + + /* qemuDomainSecretPlainSetup: * @secinfo: Pointer to secret info * @usageType: The virSecretUsageType @@ -3020,6 +3058,7 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .vcpuNew = qemuDomainVcpuPrivateNew, .chrSourceNew = qemuDomainChrSourcePrivateNew, .vsockNew = qemuDomainVsockPrivateNew, + .graphicsNew = qemuDomainGraphicsPrivateNew, .parse = qemuDomainObjPrivateXMLParse, .format = qemuDomainObjPrivateXMLFormat, .getParseOpaque = qemuDomainObjPrivateXMLGetParseOpaque, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0119de515a..6df355fe78 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -476,6 +476,18 @@ struct _qemuDomainVsockPrivate { }; +# define QEMU_DOMAIN_GRAPHICS_PRIVATE(dev) \ + ((qemuDomainGraphicsPrivatePtr) (dev)->privateData) + +typedef struct _qemuDomainGraphicsPrivate qemuDomainGraphicsPrivate; +typedef qemuDomainGraphicsPrivate *qemuDomainGraphicsPrivatePtr; +struct _qemuDomainGraphicsPrivate { + virObject parent; + + char *tlsAlias; +}; + + typedef enum { QEMU_PROCESS_EVENT_WATCHDOG = 0, QEMU_PROCESS_EVENT_GUESTPANIC, -- 2.20.1
Instead of hardcoding the TLS creds alias in qemuBuildGraphicsVNCCommandLine, store it in the domain private data. Given that we only support one VNC graphics and thus have only one alias per-domain, this is overengineered, but it will allow us to prepare the secret upfront when we start supporting encrypted server TLS keys. Note that the alias is not formatted anywhere since we won't need to access it after domain startup. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 822d5f8669..d130d0463c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8035,18 +8035,18 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, virBufferAddLit(&opt, ",password"); if (cfg->vncTLS) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) { - const char *alias = "vnc-tls-creds0"; + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics); + if (gfxPriv->tlsAlias) { if (qemuBuildTLSx509CommandLine(cmd, cfg->vncTLSx509certdir, true, cfg->vncTLSx509verify, NULL, - alias, + gfxPriv->tlsAlias, qemuCaps) < 0) goto error; - virBufferAsprintf(&opt, ",tls-creds=%s", alias); + virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias); } else { virBufferAddLit(&opt, ",tls"); if (cfg->vncTLSx509verify) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4b11cba1bd..b35c217d65 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1726,6 +1726,42 @@ qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg, } +static void +qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics) +{ + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics); + + if (!gfxPriv) + return; + + VIR_FREE(gfxPriv->tlsAlias); +} + + +static int +qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg, + qemuDomainObjPrivatePtr priv, + virDomainGraphicsDefPtr graphics) +{ + virQEMUCapsPtr qemuCaps = priv->qemuCaps; + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics); + + if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) + return 0; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) + return 0; + + if (!cfg->vncTLS) + return 0; + + if (VIR_STRDUP(gfxPriv->tlsAlias, "vnc-tls-creds0") < 0) + return -1; + + return 0; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1767,6 +1803,9 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) for (i = 0; i < vm->def->nredirdevs; i++) qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source); + + for (i = 0; i < vm->def->ngraphics; i++) + qemuDomainSecretGraphicsDestroy(vm->def->graphics[i]); } @@ -1850,6 +1889,11 @@ qemuDomainSecretPrepare(virQEMUDriverPtr driver, goto cleanup; } + for (i = 0; i < vm->def->ngraphics; i++) { + if (qemuDomainSecretGraphicsPrepare(cfg, priv, vm->def->graphics[i]) < 0) + goto cleanup; + } + ret = 0; cleanup: -- 2.20.1
On 1/21/19 7:59 AM, Ján Tomko wrote:
Instead of hardcoding the TLS creds alias in qemuBuildGraphicsVNCCommandLine, store it in the domain private data.
Given that we only support one VNC graphics and thus have only one alias per-domain, this is overengineered, but it will allow us to prepare the secret upfront when we start supporting encrypted server TLS keys.
Note that the alias is not formatted anywhere since we won't need to access it after domain startup.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-)
Rather than respond in v1... Irony is: sizeof(*gfxPriv) = 24 sizeof("vnc-tls-creds0") = 15 and the reason for the free during GraphicsDestroy is "to not have the temporary data allocated during the whole time the domain is running", but the algorithm leaves privateData allocated for every graphics device and not just VNC one's. It's used once and doesn't need to stick around. Of course who knows what the future brings and the algorithm essentially follows storage and chardev source (even though they are each a bit different). Since it's not possible to have a struct with just "virObject parent;" alone, an alternative is to have a "bool useTLSCreds;" and then continue with the use of "const char *alias = "vnc-tls-creds0";" in qemuBuildGraphicsVNCCommandLine. Then again you did say overengineered above, so even though I may have done things differently, what you have isn't wrong, so consider it, Reviewed-by: John Ferlan <jferlan@redhat.com> John
Be generic instead of trying to enumerate all the involved device types. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 855bd9cb14..4f45773dbf 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6051,7 +6051,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, VIR_DEBUG("Prepare chardev source backends for TLS"); qemuDomainPrepareChardevSource(vm->def, cfg); - VIR_DEBUG("Add secrets to hostdevs and chardevs"); + VIR_DEBUG("Prepare device secrets"); if (qemuDomainSecretPrepare(driver, vm) < 0) goto cleanup; -- 2.20.1
Add an option that lets the user specify the secret that unlocks the server TLS key. Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 6 ++++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 13 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 28bd851411..b311f02da6 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -35,6 +35,7 @@ module Libvirtd_qemu = | bool_entry "vnc_auto_unix_socket" | bool_entry "vnc_tls" | str_entry "vnc_tls_x509_cert_dir" + | str_entry "vnc_tls_x509_secret_uuid" | bool_entry "vnc_tls_x509_verify" | str_entry "vnc_password" | bool_entry "vnc_sasl" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 28e51b2c59..c1f1201134 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -95,6 +95,12 @@ #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc" +# Uncomment and use the following option to override the default secret +# UUID provided in the default_tls_x509_secret_uuid parameter. +# +#vnc_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" + + # The default TLS configuration only uses certificates for the server # allowing the client to verify the server's identity and establish # an encrypted channel. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 256aad2c0b..1808fdd4cb 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -361,6 +361,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->defaultTLSx509secretUUID); VIR_FREE(cfg->vncTLSx509certdir); + VIR_FREE(cfg->vncTLSx509secretUUID); VIR_FREE(cfg->vncListen); VIR_FREE(cfg->vncPassword); VIR_FREE(cfg->vncSASLdir); @@ -458,6 +459,8 @@ virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfigPtr cfg, cfg->vncTLSx509verifyPresent = true; if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) return -1; + if (virConfGetValueString(conf, "vnc_tls_x509_secret_uuid", &cfg->vncTLSx509secretUUID) < 0) + return -1; if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) return -1; if (virConfGetValueString(conf, "vnc_password", &cfg->vncPassword) < 0) @@ -1189,6 +1192,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg) } \ } while (0) + SET_TLS_SECRET_UUID_DEFAULT(vnc); SET_TLS_SECRET_UUID_DEFAULT(chardev); SET_TLS_SECRET_UUID_DEFAULT(migrate); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index bce8364c5a..14c9d15a72 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -125,6 +125,7 @@ struct _virQEMUDriverConfig { bool vncTLSx509verifyPresent; bool vncSASL; char *vncTLSx509certdir; + char *vncTLSx509secretUUID; char *vncListen; char *vncPassword; char *vncSASLdir; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index f1e8806ad2..4235464530 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -9,6 +9,7 @@ module Test_libvirtd_qemu = { "vnc_auto_unix_socket" = "1" } { "vnc_tls" = "1" } { "vnc_tls_x509_cert_dir" = "/etc/pki/libvirt-vnc" } +{ "vnc_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } { "vnc_tls_x509_verify" = "1" } { "vnc_password" = "XYZ12345" } { "vnc_sasl" = "1" } -- 2.20.1
Use the password stored in the secret driver under the uuid specified by the vnc_tls_x509_secret_uuid option in qemu.conf. https://bugzilla.redhat.com/show_bug.cgi?id=1602418 Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 11 +++++- src/qemu/qemu_domain.c | 9 +++++ src/qemu/qemu_domain.h | 1 + ...graphics-vnc-tls-secret.x86_64-latest.args | 36 +++++++++++++++++++ .../graphics-vnc-tls-secret.xml | 30 ++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++ 6 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/graphics-vnc-tls-secret.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/graphics-vnc-tls-secret.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d130d0463c..167b942196 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8037,11 +8037,20 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, if (cfg->vncTLS) { qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics); if (gfxPriv->tlsAlias) { + const char *secretAlias = NULL; + + if (gfxPriv->secinfo) { + if (qemuBuildObjectSecretCommandLine(cmd, + gfxPriv->secinfo) < 0) + goto error; + secretAlias = gfxPriv->secinfo->s.aes.alias; + } + if (qemuBuildTLSx509CommandLine(cmd, cfg->vncTLSx509certdir, true, cfg->vncTLSx509verify, - NULL, + secretAlias, gfxPriv->tlsAlias, qemuCaps) < 0) goto error; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b35c217d65..22d93d56f9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1269,6 +1269,7 @@ qemuDomainGraphicsPrivateDispose(void *obj) qemuDomainGraphicsPrivatePtr priv = obj; VIR_FREE(priv->tlsAlias); + qemuDomainSecretInfoFree(&priv->secinfo); } @@ -1735,6 +1736,7 @@ qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics) return; VIR_FREE(gfxPriv->tlsAlias); + qemuDomainSecretInfoFree(&gfxPriv->secinfo); } @@ -1758,6 +1760,13 @@ qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg, if (VIR_STRDUP(gfxPriv->tlsAlias, "vnc-tls-creds0") < 0) return -1; + if (cfg->vncTLSx509secretUUID) { + gfxPriv->secinfo = qemuDomainSecretInfoTLSNew(priv, gfxPriv->tlsAlias, + cfg->vncTLSx509secretUUID); + if (!gfxPriv->secinfo) + return -1; + } + return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6df355fe78..defbffbf94 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -485,6 +485,7 @@ struct _qemuDomainGraphicsPrivate { virObject parent; char *tlsAlias; + qemuDomainSecretInfoPtr secinfo; }; diff --git a/tests/qemuxml2argvdata/graphics-vnc-tls-secret.x86_64-latest.args b/tests/qemuxml2argvdata/graphics-vnc-tls-secret.x86_64-latest.args new file mode 100644 index 0000000000..737c4fe8fb --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-vnc-tls-secret.x86_64-latest.args @@ -0,0 +1,36 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +SASL_CONF_PATH=/root/.sasl2 \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ +-object secret,id=vnc-tls-creds0-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-object tls-creds-x509,id=vnc-tls-creds0,dir=/etc/pki/libvirt-vnc,\ +endpoint=server,verify-peer=yes,passwordid=vnc-tls-creds0-secret0 \ +-vnc 127.0.0.1:3,tls-creds=vnc-tls-creds0,sasl \ +-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/graphics-vnc-tls-secret.xml b/tests/qemuxml2argvdata/graphics-vnc-tls-secret.xml new file mode 100644 index 0000000000..079f6241c4 --- /dev/null +++ b/tests/qemuxml2argvdata/graphics-vnc-tls-secret.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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-system-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2cb8860d26..ba6fd4db35 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1290,6 +1290,11 @@ mymain(void) DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST_CAPS_VER("graphics-vnc-tls", "2.4.0"); DO_TEST_CAPS_LATEST("graphics-vnc-tls"); + if (VIR_STRDUP_QUIET(driver.config->vncTLSx509secretUUID, + "6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea") < 0) + return EXIT_FAILURE; + DO_TEST_CAPS_LATEST("graphics-vnc-tls-secret"); + VIR_FREE(driver.config->vncTLSx509secretUUID); driver.config->vncSASL = driver.config->vncTLSx509verify = driver.config->vncTLS = 0; VIR_FREE(driver.config->vncSASLdir); VIR_FREE(driver.config->vncTLSx509certdir); -- 2.20.1
Add a capability check to qemuDomainDefValidate and refuse to start a domain with VNC graphics if the TLS secret was set in qemu.conf and it's not supported. Note that qemuDomainSecretGraphicsPrepare does not generate any secret data if the capability is not present and qemuBuildTLSx509BackendProps is not called at all. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 22d93d56f9..32a43f2064 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4112,8 +4112,10 @@ qemuDomainDefValidate(const virDomainDef *def, void *opaque) { virQEMUDriverPtr driver = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; int ret = -1; + size_t i; if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) @@ -4234,10 +4236,23 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateMemory(def, qemuCaps) < 0) goto cleanup; + if (cfg->vncTLS && cfg->vncTLSx509secretUUID && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) { + for (i = 0; i < def->ngraphics; i++) { + if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("encrypted VNC TLS keys are not supported with " + "this QEMU binary")); + goto cleanup; + } + } + } + ret = 0; cleanup: virObjectUnref(qemuCaps); + virObjectUnref(cfg); return ret; } -- 2.20.1
On 1/21/19 7:59 AM, Ján Tomko wrote:
Add a capability check to qemuDomainDefValidate and refuse to start a domain with VNC graphics if the TLS secret was set in qemu.conf and it's not supported.
Note that qemuDomainSecretGraphicsPrepare does not generate any secret data if the capability is not present and qemuBuildTLSx509BackendProps is not called at all.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Some day I see a patch to create a qemuDomainDefValidateGraphics being created ;-) Whether you add the NB below or not, Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 22d93d56f9..32a43f2064 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4112,8 +4112,10 @@ qemuDomainDefValidate(const virDomainDef *def, void *opaque) { virQEMUDriverPtr driver = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; int ret = -1; + size_t i;
if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator))) @@ -4234,10 +4236,23 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateMemory(def, qemuCaps) < 0) goto cleanup;
/* NB: It is possible that vncTLS is set and we're using old style * certificate processing without an X.509 object */
+ if (cfg->vncTLS && cfg->vncTLSx509secretUUID && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) { + for (i = 0; i < def->ngraphics; i++) { + if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("encrypted VNC TLS keys are not supported with " + "this QEMU binary")); + goto cleanup; + } + } + } + ret = 0;
cleanup: virObjectUnref(qemuCaps); + virObjectUnref(cfg); return ret; }
participants (2)
-
John Ferlan -
Ján Tomko