A helper function for allocating the virDomainGraphicsDef structure. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- src/conf/domain_conf.c | 21 +++++++++++++++++---- src/conf/domain_conf.h | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 222bb8c482..82672c6493 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14114,10 +14114,21 @@ virDomainGraphicsDefParseXMLEGLHeadless(virDomainGraphicsDefPtr def, return 0; } +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 +14136,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 +16248,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 +20859,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(a)redhat.com&gt; --- src/conf/domain_conf.c | 8 +++++++- src/conf/domain_conf.h | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82672c6493..21112c6336 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14115,13 +14115,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

On 1/16/19 2:41 AM, Ján Tomko wrote: Free'ing of tlsAlias is handled by qemuDomainGraphicsPrivateDispose, so this would be virObjectUnref(gfxPriv); QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics) = NULL; The second to avoid the virDomainGraphicsDefFree doing this as well. Still this is "unusual" (so to speak) for a qemuDomainSecret*Destroy function. Typically they just clear/destroy the *Secinfo data. Since you don't have that yet until patch 7, this could wait until then. On the other hand, I don't "foresee" anything wrong with properly free'ing the graphics def privateData now. Just a note/thought while reviewing... nothing all that important... Seems to be a bit of overkill w/ graphics->privateData only being used for VNC private data in this very specialized case. Still weighed vs the then need for "gfxPriv && gfxPriv->...", who am I to complain ;-) Interesting placement until one reads patch 7. I think if patch 5 and 6 were placed before this one, then it'd be clearer what the steps are. I'm OK with this here now since eventually it'd be added. With a couple of adjustments... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

Be generic instead of trying to enumerate all the involved device types. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- 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 aad6c12552..f4af673cdb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6050,7 +6050,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(a)redhat.com&gt; --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 6 ++++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 5 files changed, 12 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 18ad99c173..0f74fd1716 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -457,6 +457,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) @@ -1184,6 +1186,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(a)redhat.com&gt; --- 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..e17d7ddec7 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 && 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 6960f0569b..da9c4e566d 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); } @@ -1750,6 +1751,7 @@ qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics) return; VIR_FREE(gfxPriv->tlsAlias); + qemuDomainSecretInfoFree(&gfxPriv->secinfo); } @@ -1773,6 +1775,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 01e47996f5..e706ddca31 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -490,6 +490,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. Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index da9c4e566d..851cb6d622 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4127,6 +4127,7 @@ qemuDomainDefValidate(const virDomainDef *def, void *opaque) { virQEMUDriverPtr driver = opaque; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; int ret = -1; @@ -4249,10 +4250,19 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateMemory(def, qemuCaps) < 0) goto cleanup; + if (cfg->vncTLS && cfg->vncTLSx509secretUUID && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) { + 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