[libvirt] [PATCH v10 0/4] Add native TLS encrypted chardev TCP support

v9: http://www.redhat.com/archives/libvir-list/2016-October/msg00726.html "Theorically speaking" patch #2 is "separate" from patches 1, 3, & 4. That is patch 3 and 4 are adding the secret uuid processing handling which is different than the enable/disable property logic for patch 2. I've left them all together though since just to be consistent with previous series. Differences in v10 ... Pushed the previous series 2/5 and 3/5 since they were ACK'd ... Create a new patch 1 to have helper qemuDomainSupportTLSChardevTCP It's mostly unnecessary without patch 2 though, but it made adding or "separating" patch 2 from patches 3 & 4 a whole lot easier... ... Modified former patch 1 (now patch 2) to accommodate for a paradigm where tls='yes' and chardev_tls=0 might be possible. The new helper is used to whether to add the TLS information or not. ... Modified former patch 4 (now patch 3) to accommodate for the changes Pavel has made to the code and to generate the secalias using the "charAlias" ... Modified former patch 4 (now patch 4) to use the "charAlias" as well and merge in Pavel's changes NOTE: Even though 'yes' is a now possibility, it is an option that's assuming chardev_tls=0 so I don't feel the issues raised during review of v8 regarding needing to consider a currently running 2.3.0 domain that still needs to work when 2.4.0 is applied. I believe it will be with the way the optional property is being used, thus with respect to the points in: http://www.redhat.com/archives/libvir-list/2016-October/msg00732.html The proposed qemuProcessPrepareDomain change is invalid since haveTLS is a tristate and chardevTLS is a bistate. This is what I meant about being a bit dangerous (e.g. BOOL_NO=2, BOOL_YES=1, and BOOL_ABSENT=0); however, "chardevTLS=1" is enabled (yes) and "chardevTLS=0" is disabled (absent). While it looks good when typing, when you get down to the details sometimes you find those 'gotchas'. Even if the shorthand logic were fixed, it's not going to be good to assume that setting the domain property or disabling the domain property is the desired action. The qemuProcessAttach is for qemu-attach and not the path that libvirt uses to reconnect to running domains (which is qemuProcessReconnect). There's so much broken from the qemu-attach right now - I doubt it really works at all. With respect to the reconnect processing (since that's really what you were thinking about)... There is no "options" provided/found in that code. New code could possibly "read" the '/proc/$pid/cmdline' file and look for 'tls-creds', but the only purpose of that would be to manage 'assumptions' with how the "tls='{yes|no}'" property is used. Altering virDomainChrSourceDefParseXML and virDomainChrSourceDefFormat to manage some new boolean 'tlsFromConfig' that I'm not sure could be set properly is something I think is outside these patches. John Ferlan (4): qemu: Introduce qemuDomainSupportTLSChardevTCP domain: Add optional 'tls' attribute for TCP chardev qemu: Add a secret object to/for a chardev tcp with secret qemu: Add secret object hotplug for TCP chardev TLS docs/formatdomain.html.in | 28 +++++ docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 22 +++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 33 ++++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 133 ++++++++++++++++++++- src/qemu/qemu_domain.h | 18 ++- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 58 ++++++++- src/qemu/qemu_hotplug.h | 3 +- src/qemu/qemu_process.c | 4 +- tests/qemuhotplugtest.c | 2 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++ ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 ++++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++++ tests/qemuxml2argvtest.c | 20 ++++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 20 files changed, 483 insertions(+), 17 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-secret-chardev.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml -- 2.7.4

It's very simple right now, but it's about to get a bit more complicated. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 16 ++++++++++++++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_hotplug.c | 4 ++-- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8282162..d45a7de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); - if (cfg->chardevTLS) { + if (qemuDomainSupportTLSChardevTCP(cfg)) { char *objalias = NULL; if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4c118ff..746d94f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6298,3 +6298,19 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, return true; } + + +/* qemuDomainSupportTLSChardevTCP + * @cfg: Pointer to driver cfg + * + * Let's check if this host supports using the TLS environment for chardev. + * + * Returns true if we want to use TLS, false otherwise. + */ +bool +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +{ + if (cfg->chardevTLS) + return true; + return false; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 707a995..7ecafac 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -755,4 +755,5 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps); +bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2cb2267..c2b43b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1730,7 +1730,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup; if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP && - cfg->chardevTLS) { + qemuDomainSupportTLSChardevTCP(cfg)) { if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, @@ -4404,7 +4404,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && - cfg->chardevTLS && + qemuDomainSupportTLSChardevTCP(cfg) && !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; -- 2.7.4

Add an optional "tls='yes|no'" attribute for a TCP chardev. For QEMU, this will allow for disabling the host config setting of the 'chardev_tls' for a domain chardev channel by setting the value to "no" or to attempt to use a host TLS environment when setting the value to "yes" when the host config 'chardev_tls' setting is disabled, but a TLS environment is configured via either the host config 'chardev_tls_x509_cert_dir' or 'default_tls_x509_cert_dir' Alter qemuDomainSupportTLSChardevTCP to augment the decision points for choosing whether to try to use TLS. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 28 ++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 20 +++++++-- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 4 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 13 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9051178..da6be67 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null </devices> ...</pre> + <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p> +<pre> + ... + <devices> + <serial type="tcp"> + <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> + <protocol type="raw"/> + <target port="0"/> + </serial> + </devices> + ...</pre> + <h6><a name="elementsCharUDP">UDP network console</a></h6> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3106510..e6741bb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3453,6 +3453,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <attribute name="tls"> + <ref name="virYesNo"/> + </attribute> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89473db..e4fa9ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0) return -1; + + dest->data.tcp.haveTLS = src->data.tcp.haveTLS; break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *master = NULL; char *slave = NULL; char *append = NULL; + char *haveTLS = NULL; int remaining = 0; while (cur != NULL) { @@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (xmlStrEqual(cur->name, BAD_CAST "source")) { if (!mode) mode = virXMLPropString(cur, "mode"); + if (!haveTLS) + haveTLS = virXMLPropString(cur, "tls"); switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: @@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, def->data.tcp.listen = true; } + if (haveTLS && + (def->data.tcp.haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown chardev 'tls' setting '%s'"), + haveTLS); + goto error; + } + if (!protocol) def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW; else if ((def->data.tcp.protocol = @@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(append); VIR_FREE(logappend); VIR_FREE(logfile); + VIR_FREE(haveTLS); return remaining; @@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<source mode='%s' ", def->data.tcp.listen ? "bind" : "connect"); virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); - virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); + virBufferEscapeString(buf, "service='%s'", def->data.tcp.service); + if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " tls='%s'", + virTristateBoolTypeToString(def->data.tcp.haveTLS)); + virBufferAddLit(buf, "/>\n"); + virBufferAsprintf(buf, "<protocol type='%s'/>\n", virDomainChrTcpProtocolTypeToString( def->data.tcp.protocol)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 04f2e40..fcadf6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef { bool listen; int protocol; bool tlscreds; + int haveTLS; /* enum virTristateBool */ } tcp; struct { char *bindHost; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d45a7de..f00751a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); - if (qemuDomainSupportTLSChardevTCP(cfg)) { + if (qemuDomainSupportTLSChardevTCP(cfg, dev)) { char *objalias = NULL; if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 746d94f..7b518c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6302,15 +6302,29 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, /* qemuDomainSupportTLSChardevTCP * @cfg: Pointer to driver cfg + * @dev: Pointer to chardev source * - * Let's check if this host supports using the TLS environment for chardev. + * Let's check if this host and/or domain supports or desires to use + * the TLS environment for the passed chardev TCP. + * + * If we have an environment and as long as the domain config doesn't have + * the "tls='no'" property, then we assume it's desired. + * + * If the host global isn't set, but the domain chardev config is requesting + * to use TLS and we find what appears to be some environment configured, + * then let's also try. This action could fail later in QEMU if the environment + * isn't set up to the exact specifications. * * Returns true if we want to use TLS, false otherwise. */ bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7ecafac..156e0fc 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -755,5 +755,6 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps); -bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg); +bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2b43b1..9643a68 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1730,7 +1730,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup; if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg)) { + qemuDomainSupportTLSChardevTCP(cfg, dev)) { if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, @@ -4404,7 +4404,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg) && + qemuDomainSupportTLSChardevTCP(cfg, &tmpChr->source) && !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args new file mode 100644 index 0000000..cac0d85 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-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 \ +-chardev socket,id=charserial1,host=127.0.0.1,port=5555 \ +-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-chardev-notls.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml new file mode 100644 index 0000000..debc69b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.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' tls='no'/> + <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 3e9f825..52d85fa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1167,6 +1167,9 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLSx509verify = 0; + DO_TEST("serial-tcp-tlsx509-chardev-notls", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLS = 0; VIR_FREE(driver.config->chardevTLSx509certdir); DO_TEST("serial-many-chardev", diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml new file mode 120000 index 0000000..26484c9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 95c0bf2..64da80a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -534,6 +534,7 @@ mymain(void) DO_TEST("serial-udp", NONE); DO_TEST("serial-tcp-telnet", NONE); DO_TEST("serial-tcp-tlsx509-chardev", NONE); + DO_TEST("serial-tcp-tlsx509-chardev-notls", NONE); DO_TEST("serial-many", NONE); DO_TEST("serial-spiceport", NONE); DO_TEST("serial-spiceport-nospice", NONE); -- 2.7.4

On Wed, Oct 19, 2016 at 04:53:54PM -0400, John Ferlan wrote:
Add an optional "tls='yes|no'" attribute for a TCP chardev.
For QEMU, this will allow for disabling the host config setting of the 'chardev_tls' for a domain chardev channel by setting the value to "no" or to attempt to use a host TLS environment when setting the value to "yes" when the host config 'chardev_tls' setting is disabled, but a TLS environment is configured via either the host config 'chardev_tls_x509_cert_dir' or 'default_tls_x509_cert_dir'
Alter qemuDomainSupportTLSChardevTCP to augment the decision points for choosing whether to try to use TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 28 ++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 20 +++++++-- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 4 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 13 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9051178..da6be67 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null </devices> ...</pre>
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
Nice, this is a good description how to use the *tls* attribute.
+<pre> + ... + <devices> + <serial type="tcp"> + <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> + <protocol type="raw"/> + <target port="0"/> + </serial> + </devices> + ...</pre> + <h6><a name="elementsCharUDP">UDP network console</a></h6>
<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3106510..e6741bb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3453,6 +3453,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <attribute name="tls"> + <ref name="virYesNo"/> + </attribute> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89473db..e4fa9ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0) return -1; + + dest->data.tcp.haveTLS = src->data.tcp.haveTLS; break;
case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *master = NULL; char *slave = NULL; char *append = NULL; + char *haveTLS = NULL; int remaining = 0;
while (cur != NULL) { @@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (xmlStrEqual(cur->name, BAD_CAST "source")) { if (!mode) mode = virXMLPropString(cur, "mode"); + if (!haveTLS) + haveTLS = virXMLPropString(cur, "tls");
switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: @@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, def->data.tcp.listen = true; }
+ if (haveTLS && + (def->data.tcp.haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown chardev 'tls' setting '%s'"), + haveTLS); + goto error; + } + if (!protocol) def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW; else if ((def->data.tcp.protocol = @@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(append); VIR_FREE(logappend); VIR_FREE(logfile); + VIR_FREE(haveTLS);
return remaining;
@@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<source mode='%s' ", def->data.tcp.listen ? "bind" : "connect"); virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); - virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); + virBufferEscapeString(buf, "service='%s'", def->data.tcp.service); + if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " tls='%s'", + virTristateBoolTypeToString(def->data.tcp.haveTLS)); + virBufferAddLit(buf, "/>\n"); + virBufferAsprintf(buf, "<protocol type='%s'/>\n", virDomainChrTcpProtocolTypeToString( def->data.tcp.protocol)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 04f2e40..fcadf6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef { bool listen; int protocol; bool tlscreds; + int haveTLS; /* enum virTristateBool */ } tcp; struct { char *bindHost; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d45a7de..f00751a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
- if (qemuDomainSupportTLSChardevTCP(cfg)) { + if (qemuDomainSupportTLSChardevTCP(cfg, dev)) { char *objalias = NULL;
if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 746d94f..7b518c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6302,15 +6302,29 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/* qemuDomainSupportTLSChardevTCP * @cfg: Pointer to driver cfg + * @dev: Pointer to chardev source * - * Let's check if this host supports using the TLS environment for chardev. + * Let's check if this host and/or domain supports or desires to use + * the TLS environment for the passed chardev TCP. + * + * If we have an environment and as long as the domain config doesn't have + * the "tls='no'" property, then we assume it's desired. + * + * If the host global isn't set, but the domain chardev config is requesting + * to use TLS and we find what appears to be some environment configured, + * then let's also try. This action could fail later in QEMU if the environment + * isn't set up to the exact specifications. * * Returns true if we want to use TLS, false otherwise. */ bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; }
So this function let's you decide whether we should try to set up *tls* for chardev or not. It work's but I have few issues with it. At first I don't like that libvirt would try to do something smart and don't even tell user about the result. This will silently ignore the *tls* attribute if no certificate is found. In case that *tls* attribute is set to "yes" in XML and there is no certificate file to use we shouldn't start that domain and print an error to user. Secondly this way we don't reflect the current state for live domain in the live XML. This was probably lost during the discussion, but in general if there is an attribute that can affect running domain we should reflect the current state using that attribute. I know, there are some cases where we probably don't do that and they should be fixed. I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we can set the haveTLS based on cfg->chardevTLS. The whole purpose of qemuProcessPrepareDomain() is to prepare the domain definition so the qemuBuildCommandLine() don't have to check other places to enable some feature and not update the live definition. If the *tls* attribute is properly set in live definition that it will be saved to status XML and there is no need to do anything for qemuProcessReconnect. Rest of the patch looks good. Pavel
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7ecafac..156e0fc 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -755,5 +755,6 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps);
-bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg); +bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2b43b1..9643a68 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1730,7 +1730,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup;
if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg)) { + qemuDomainSupportTLSChardevTCP(cfg, dev)) { if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, @@ -4404,7 +4404,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup;
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg) && + qemuDomainSupportTLSChardevTCP(cfg, &tmpChr->source) && !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args new file mode 100644 index 0000000..cac0d85 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-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 \ +-chardev socket,id=charserial1,host=127.0.0.1,port=5555 \ +-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-chardev-notls.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml new file mode 100644 index 0000000..debc69b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.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' tls='no'/> + <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 3e9f825..52d85fa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1167,6 +1167,9 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLSx509verify = 0; + DO_TEST("serial-tcp-tlsx509-chardev-notls", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLS = 0; VIR_FREE(driver.config->chardevTLSx509certdir); DO_TEST("serial-many-chardev", diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml new file mode 120000 index 0000000..26484c9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 95c0bf2..64da80a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -534,6 +534,7 @@ mymain(void) DO_TEST("serial-udp", NONE); DO_TEST("serial-tcp-telnet", NONE); DO_TEST("serial-tcp-tlsx509-chardev", NONE); + DO_TEST("serial-tcp-tlsx509-chardev-notls", NONE); DO_TEST("serial-many", NONE); DO_TEST("serial-spiceport", NONE); DO_TEST("serial-spiceport-nospice", NONE); -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 20, 2016 at 08:51:45AM +0200, Pavel Hrdina wrote:
On Wed, Oct 19, 2016 at 04:53:54PM -0400, John Ferlan wrote:
Add an optional "tls='yes|no'" attribute for a TCP chardev.
For QEMU, this will allow for disabling the host config setting of the 'chardev_tls' for a domain chardev channel by setting the value to "no" or to attempt to use a host TLS environment when setting the value to "yes" when the host config 'chardev_tls' setting is disabled, but a TLS environment is configured via either the host config 'chardev_tls_x509_cert_dir' or 'default_tls_x509_cert_dir'
Alter qemuDomainSupportTLSChardevTCP to augment the decision points for choosing whether to try to use TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 28 ++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 20 +++++++-- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 4 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 13 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9051178..da6be67 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null </devices> ...</pre>
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev
Remove reference to "serial" because this is valid for all chardevs. Pavel
+ TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
[...]

On 10/20/2016 02:51 AM, Pavel Hrdina wrote:
On Wed, Oct 19, 2016 at 04:53:54PM -0400, John Ferlan wrote:
Add an optional "tls='yes|no'" attribute for a TCP chardev.
For QEMU, this will allow for disabling the host config setting of the 'chardev_tls' for a domain chardev channel by setting the value to "no" or to attempt to use a host TLS environment when setting the value to "yes" when the host config 'chardev_tls' setting is disabled, but a TLS environment is configured via either the host config 'chardev_tls_x509_cert_dir' or 'default_tls_x509_cert_dir'
Alter qemuDomainSupportTLSChardevTCP to augment the decision points for choosing whether to try to use TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 28 ++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 20 +++++++-- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 4 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 13 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9051178..da6be67 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null </devices> ...</pre>
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
Nice, this is a good description how to use the *tls* attribute.
BTW (regarding your followup reply): The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be parsed) refer to this as a "serial chardev" The 'virDomainChrDefParseXML' comments have a list of "<serial ..." XML types. The location of the above description is describing a <serial type="tcp"> definition. The 'smartcard' discussion for a 'passthrough' device that would use this code says "Rather than having the hypervisor directly communicate with the host, it is possible to tunnel all requests through a secondary character device to a third-party provider (which may in turn be talking to a smartcard or using three certificate files). In this mode of operation, an additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel;..." The 'rng' discussion for backend that would use this code says "This backend connects to a source using the EGD protocol. The source is specified as a character device. Refer to character device host interface for more information. ..." Redevdir says "An additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel; type='tcp' or type='spicevmc' ..." So the long and short of it is, IMO it's a serial chardev device. Semantically it could be claimed otherwise, but the parsing proves otherwise as does the existing documentation of "Host interface" character devices. I prefer to keep it described as is. It's only ever used, parsed, etc. when <devices>... <serial type="tcp">... <source mode='connect'..." If anything, the description should become more restrictive to indicate that the option shouldn't be used for smartcards, rngs, and redirdevs, but I'll save that discussion for patch 3.
+<pre> + ... + <devices> + <serial type="tcp"> + <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> + <protocol type="raw"/> + <target port="0"/> + </serial> + </devices> + ...</pre> + <h6><a name="elementsCharUDP">UDP network console</a></h6>
<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3106510..e6741bb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3453,6 +3453,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <attribute name="tls"> + <ref name="virYesNo"/> + </attribute> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89473db..e4fa9ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0) return -1; + + dest->data.tcp.haveTLS = src->data.tcp.haveTLS; break;
case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *master = NULL; char *slave = NULL; char *append = NULL; + char *haveTLS = NULL; int remaining = 0;
while (cur != NULL) { @@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (xmlStrEqual(cur->name, BAD_CAST "source")) { if (!mode) mode = virXMLPropString(cur, "mode"); + if (!haveTLS) + haveTLS = virXMLPropString(cur, "tls");
switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: @@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, def->data.tcp.listen = true; }
+ if (haveTLS && + (def->data.tcp.haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown chardev 'tls' setting '%s'"), + haveTLS); + goto error; + } + if (!protocol) def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW; else if ((def->data.tcp.protocol = @@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(append); VIR_FREE(logappend); VIR_FREE(logfile); + VIR_FREE(haveTLS);
return remaining;
@@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<source mode='%s' ", def->data.tcp.listen ? "bind" : "connect"); virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); - virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); + virBufferEscapeString(buf, "service='%s'", def->data.tcp.service); + if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " tls='%s'", + virTristateBoolTypeToString(def->data.tcp.haveTLS)); + virBufferAddLit(buf, "/>\n"); + virBufferAsprintf(buf, "<protocol type='%s'/>\n", virDomainChrTcpProtocolTypeToString( def->data.tcp.protocol)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 04f2e40..fcadf6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef { bool listen; int protocol; bool tlscreds; + int haveTLS; /* enum virTristateBool */ } tcp; struct { char *bindHost; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d45a7de..f00751a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
- if (qemuDomainSupportTLSChardevTCP(cfg)) { + if (qemuDomainSupportTLSChardevTCP(cfg, dev)) { char *objalias = NULL;
if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 746d94f..7b518c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6302,15 +6302,29 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/* qemuDomainSupportTLSChardevTCP * @cfg: Pointer to driver cfg + * @dev: Pointer to chardev source * - * Let's check if this host supports using the TLS environment for chardev. + * Let's check if this host and/or domain supports or desires to use + * the TLS environment for the passed chardev TCP. + * + * If we have an environment and as long as the domain config doesn't have + * the "tls='no'" property, then we assume it's desired. + * + * If the host global isn't set, but the domain chardev config is requesting + * to use TLS and we find what appears to be some environment configured, + * then let's also try. This action could fail later in QEMU if the environment + * isn't set up to the exact specifications. * * Returns true if we want to use TLS, false otherwise. */ bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; }
So this function let's you decide whether we should try to set up *tls* for chardev or not. It work's but I have few issues with it.
At first I don't like that libvirt would try to do something smart and don't even tell user about the result. This will silently ignore the *tls* attribute if no certificate is found. In case that *tls* attribute is set to "yes" in XML and there is no certificate file to use we shouldn't start that domain and print an error to user.
This is a boolean function - so printing an error here isn't right. Adding something to post parse processing is possible, but there's an impact based on whether we're setting a default value for haveTLS (something I disagree with doing). Beyond that would a check go in qemuDomainDeviceDefPostParse or qemuDomainDeviceDefValidate? It's never crystal clear to me which should be used when just reading the code. Although since I see this as a "new" and "optional" value, I'd lean toward PostParse. Then there's dealing with the parseFlags that it's impacted by other decisions. I could also rationalize that someone adding "tls='yes'" to their chardev would "know" what they're doing because they read the documentation. How else would they know to have this very specific combination (unless of course 'something' set things up that way based on the assumption of how a domain is currently running). Again, IMO 'haveTLS' is new and optional. The only indication that a domain is using TLS was handled via 'tlscreds'. IIRC, the "reason" that 'tlscreds' exists and how "tls-creds" gets added to the command line is because the JSON code processing for a chardev is buried in qemu_monitor_json.c and fishing for a host configuration option at that time wasn't viable.
Secondly this way we don't reflect the current state for live domain in the live XML. This was probably lost during the discussion, but in general if there is an attribute that can affect running domain we should reflect the current state using that attribute. I know, there are some cases where we probably don't do that and they should be fixed.
No it wasn't lost - I considered it as I see you've seen from the cover. And you're probably right regarding attributes that trigger usage of some qemu option that we don't specifically save in the status XML. That's a different rat hole.
I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we can set the haveTLS based on cfg->chardevTLS. The whole purpose of qemuProcessPrepareDomain() is to prepare the domain definition so the qemuBuildCommandLine() don't have to check other places to enable some feature and not update the live definition. If the *tls* attribute is properly set in live definition that it will be saved to status XML and there is no need to do anything for qemuProcessReconnect.
I disagree on setting haveTLS during qemuProcessReconnect based upon chardevTLS. The 'haveTLS' is an optional attribute and by setting a value I believe we end up making an assumption. If *anything* was to be done it would be based solely on whether "tls-creds" is set on the command line of the reconnected domain. However, that too has a similar problem about setting a value for an optional attribute based upon the assumption that we know better. Again, the only indication that 'tls-creds' is on the command line was from the 'tlscreds' boolean that was set because the host configuration information was available. A domain that is running and has tls-creds will continue to have it. Altering that domain's configuration file because we add a new optional *configuration* value has no bearing on the *status* XML. When/if a configuration XML is updated, it's not checking that 'tlscreds' value to determine that at some point in history the domain used TLS because the host was configured that way. Let's consider the bool function from above and this automagic setting being requested. Let's say we reconnect to a domain, find the 'tls-creds' set on the command line, and set 'haveTLS=YES' based solely on that. Let's say at some point in time after, someone edits their qemu.conf file and sets 'chardev_tls=0' (or comments out the 'chardev_tls=1'). In their mind, they've now disabled chardev TLS for their host and any domain they will run in the future. They stop the domain they knew was running using TLS before and restart it expecting that it won't use TLS anymore, but on restart they discover that in our infinite wisdom we have set the optional "tls='yes'" property for that chardev on that domain. Now if we 'error' out on that start like you request above, then that means they will have to edit their domain and remove the seemingly optional property. Next, let's assume they read the documentation and found that they can disable the qemu.conf value, but still have the domain chardev value if they set the "tls='yes'" property as long as they have their valid TLS directory configuration. In this case, they have made the conscious decision how they want their domain configured based on what they know is configured on the host. TBH: I take this single patch as a "feature request" add-on to the original feature request that I believe in the long run won't be used. I could be wrong, but it's a feeling. Furthermore, the purpose of any optional attribute is just that. It's optional based on some host wide setting. It's up to the consumer to decide how they should proceed, not the software to make that decision for them. John
Rest of the patch looks good.
Pavel
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7ecafac..156e0fc 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -755,5 +755,6 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, virQEMUCapsPtr qemuCaps);
-bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg); +bool qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev); #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2b43b1..9643a68 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1730,7 +1730,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto cleanup;
if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg)) { + qemuDomainSupportTLSChardevTCP(cfg, dev)) { if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, @@ -4404,7 +4404,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup;
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg) && + qemuDomainSupportTLSChardevTCP(cfg, &tmpChr->source) && !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args new file mode 100644 index 0000000..cac0d85 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-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 \ +-chardev socket,id=charserial1,host=127.0.0.1,port=5555 \ +-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-chardev-notls.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml new file mode 100644 index 0000000..debc69b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.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' tls='no'/> + <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 3e9f825..52d85fa 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1167,6 +1167,9 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLSx509verify = 0; + DO_TEST("serial-tcp-tlsx509-chardev-notls", + QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_OBJECT_TLS_CREDS_X509); driver.config->chardevTLS = 0; VIR_FREE(driver.config->chardevTLSx509certdir); DO_TEST("serial-many-chardev", diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml new file mode 120000 index 0000000..26484c9 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 95c0bf2..64da80a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -534,6 +534,7 @@ mymain(void) DO_TEST("serial-udp", NONE); DO_TEST("serial-tcp-telnet", NONE); DO_TEST("serial-tcp-tlsx509-chardev", NONE); + DO_TEST("serial-tcp-tlsx509-chardev-notls", NONE); DO_TEST("serial-many", NONE); DO_TEST("serial-spiceport", NONE); DO_TEST("serial-spiceport-nospice", NONE); -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 20, 2016 at 10:28:04AM -0400, John Ferlan wrote:
On 10/20/2016 02:51 AM, Pavel Hrdina wrote:
On Wed, Oct 19, 2016 at 04:53:54PM -0400, John Ferlan wrote:
Add an optional "tls='yes|no'" attribute for a TCP chardev.
For QEMU, this will allow for disabling the host config setting of the 'chardev_tls' for a domain chardev channel by setting the value to "no" or to attempt to use a host TLS environment when setting the value to "yes" when the host config 'chardev_tls' setting is disabled, but a TLS environment is configured via either the host config 'chardev_tls_x509_cert_dir' or 'default_tls_x509_cert_dir'
Alter qemuDomainSupportTLSChardevTCP to augment the decision points for choosing whether to try to use TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 28 ++++++++++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 22 +++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 20 +++++++-- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_hotplug.c | 4 +- ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + tests/qemuxml2xmltest.c | 1 + 13 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9051178..da6be67 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null </devices> ...</pre>
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
Nice, this is a good description how to use the *tls* attribute.
BTW (regarding your followup reply):
The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be parsed) refer to this as a "serial chardev"
This is a generic function that parses source for a lot of different device types/
The 'virDomainChrDefParseXML' comments have a list of "<serial ..." XML types. The location of the above description is describing a <serial type="tcp"> definition.
Well, the comment have that list but is used to parse all character devices, not only serial char device. TLS encryption can be used also for those types: parallel, channel, and console.
The 'smartcard' discussion for a 'passthrough' device that would use this code says "Rather than having the hypervisor directly communicate with the host, it is possible to tunnel all requests through a secondary character device to a third-party provider (which may in turn be talking to a smartcard or using three certificate files). In this mode of operation, an additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel;..."
This comment is wrong, it should be "supported character device types". This attribute tells what interface is presented to the host. Check this part of documentation for all character devices: <http://libvirt.org/formatdomain.html#elementsConsole> there is this sentence: "The interface presented to the host is given in the type attribute of the top-level element. The host interface is configured by the source element." So it refers to all host interfaces: <http://libvirt.org/formatdomain.html#elementsCharHostInterface>
The 'rng' discussion for backend that would use this code says "This backend connects to a source using the EGD protocol. The source is specified as a character device. Refer to character device host interface for more information. ..."
This is correct, there is no reference to serial character device.
Redevdir says "An additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel; type='tcp' or type='spicevmc' ..."
This is the same case as smartcard device, this is wrong.
So the long and short of it is, IMO it's a serial chardev device. Semantically it could be claimed otherwise, but the parsing proves otherwise as does the existing documentation of "Host interface" character devices.
I prefer to keep it described as is. It's only ever used, parsed, etc. when <devices>... <serial type="tcp">... <source mode='connect'..."
If anything, the description should become more restrictive to indicate that the option shouldn't be used for smartcards, rngs, and redirdevs, but I'll save that discussion for patch 3.
Based on the documentation it may appear that it should be a serial chardev, but that's misleading and it should refer only to the "host interfaces of character devices".
+<pre> + ... + <devices> + <serial type="tcp"> + <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> + <protocol type="raw"/> + <target port="0"/> + </serial> + </devices> + ...</pre> + <h6><a name="elementsCharUDP">UDP network console</a></h6>
<p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3106510..e6741bb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3453,6 +3453,11 @@ <ref name="virOnOff"/> </attribute> </optional> + <optional> + <attribute name="tls"> + <ref name="virYesNo"/> + </attribute> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89473db..e4fa9ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0) return -1; + + dest->data.tcp.haveTLS = src->data.tcp.haveTLS; break;
case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *master = NULL; char *slave = NULL; char *append = NULL; + char *haveTLS = NULL; int remaining = 0;
while (cur != NULL) { @@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, if (xmlStrEqual(cur->name, BAD_CAST "source")) { if (!mode) mode = virXMLPropString(cur, "mode"); + if (!haveTLS) + haveTLS = virXMLPropString(cur, "tls");
switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: @@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, def->data.tcp.listen = true; }
+ if (haveTLS && + (def->data.tcp.haveTLS = + virTristateBoolTypeFromString(haveTLS)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown chardev 'tls' setting '%s'"), + haveTLS); + goto error; + } + if (!protocol) def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW; else if ((def->data.tcp.protocol = @@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(append); VIR_FREE(logappend); VIR_FREE(logfile); + VIR_FREE(haveTLS);
return remaining;
@@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<source mode='%s' ", def->data.tcp.listen ? "bind" : "connect"); virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); - virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); + virBufferEscapeString(buf, "service='%s'", def->data.tcp.service); + if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " tls='%s'", + virTristateBoolTypeToString(def->data.tcp.haveTLS)); + virBufferAddLit(buf, "/>\n"); + virBufferAsprintf(buf, "<protocol type='%s'/>\n", virDomainChrTcpProtocolTypeToString( def->data.tcp.protocol)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 04f2e40..fcadf6c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef { bool listen; int protocol; bool tlscreds; + int haveTLS; /* enum virTristateBool */ } tcp; struct { char *bindHost; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d45a7de..f00751a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
- if (qemuDomainSupportTLSChardevTCP(cfg)) { + if (qemuDomainSupportTLSChardevTCP(cfg, dev)) { char *objalias = NULL;
if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 746d94f..7b518c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6302,15 +6302,29 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/* qemuDomainSupportTLSChardevTCP * @cfg: Pointer to driver cfg + * @dev: Pointer to chardev source * - * Let's check if this host supports using the TLS environment for chardev. + * Let's check if this host and/or domain supports or desires to use + * the TLS environment for the passed chardev TCP. + * + * If we have an environment and as long as the domain config doesn't have + * the "tls='no'" property, then we assume it's desired. + * + * If the host global isn't set, but the domain chardev config is requesting + * to use TLS and we find what appears to be some environment configured, + * then let's also try. This action could fail later in QEMU if the environment + * isn't set up to the exact specifications. * * Returns true if we want to use TLS, false otherwise. */ bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; }
So this function let's you decide whether we should try to set up *tls* for chardev or not. It work's but I have few issues with it.
At first I don't like that libvirt would try to do something smart and don't even tell user about the result. This will silently ignore the *tls* attribute if no certificate is found. In case that *tls* attribute is set to "yes" in XML and there is no certificate file to use we shouldn't start that domain and print an error to user.
This is a boolean function - so printing an error here isn't right.
Well, my comment implies to not use boolean function.
Adding something to post parse processing is possible, but there's an impact based on whether we're setting a default value for haveTLS (something I disagree with doing).
Beyond that would a check go in qemuDomainDeviceDefPostParse or qemuDomainDeviceDefValidate? It's never crystal clear to me which should be used when just reading the code. Although since I see this as a "new" and "optional" value, I'd lean toward PostParse. Then there's dealing with the parseFlags that it's impacted by other decisions.
I could also rationalize that someone adding "tls='yes'" to their chardev would "know" what they're doing because they read the documentation. How else would they know to have this very specific combination (unless of course 'something' set things up that way based on the assumption of how a domain is currently running).
Again, IMO 'haveTLS' is new and optional. The only indication that a domain is using TLS was handled via 'tlscreds'. IIRC, the "reason" that 'tlscreds' exists and how "tls-creds" gets added to the command line is because the JSON code processing for a chardev is buried in qemu_monitor_json.c and fishing for a host configuration option at that time wasn't viable.
Secondly this way we don't reflect the current state for live domain in the live XML. This was probably lost during the discussion, but in general if there is an attribute that can affect running domain we should reflect the current state using that attribute. I know, there are some cases where we probably don't do that and they should be fixed.
No it wasn't lost - I considered it as I see you've seen from the cover. And you're probably right regarding attributes that trigger usage of some qemu option that we don't specifically save in the status XML. That's a different rat hole.
I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we can set the haveTLS based on cfg->chardevTLS. The whole purpose of qemuProcessPrepareDomain() is to prepare the domain definition so the qemuBuildCommandLine() don't have to check other places to enable some feature and not update the live definition. If the *tls* attribute is properly set in live definition that it will be saved to status XML and there is no need to do anything for qemuProcessReconnect.
I disagree on setting haveTLS during qemuProcessReconnect based upon chardevTLS. The 'haveTLS' is an optional attribute and by setting a value I believe we end up making an assumption.
I wrote that there is no need to do anything for qemuProcessReconnect.
If *anything* was to be done it would be based solely on whether "tls-creds" is set on the command line of the reconnected domain. However, that too has a similar problem about setting a value for an optional attribute based upon the assumption that we know better.
Again, the only indication that 'tls-creds' is on the command line was from the 'tlscreds' boolean that was set because the host configuration information was available. A domain that is running and has tls-creds will continue to have it. Altering that domain's configuration file because we add a new optional *configuration* value has no bearing on the *status* XML. When/if a configuration XML is updated, it's not checking that 'tlscreds' value to determine that at some point in history the domain used TLS because the host was configured that way.
The whole point of having 'tls' attribute in live XML is to ensure that when libvirtd is restarted the attribute is still present in the XML because it will be saved to status XML and loaded again from status XML. There is no need to do any magic by parsing qemu command line, we have statu XML for that purpose to store all information about domain.
Let's consider the bool function from above and this automagic setting being requested. Let's say we reconnect to a domain, find the 'tls-creds' set on the command line, and set 'haveTLS=YES' based solely on that. Let's say at some point in time after, someone edits their qemu.conf file and sets 'chardev_tls=0' (or comments out the 'chardev_tls=1'). In their mind, they've now disabled chardev TLS for their host and any domain they will run in the future. They stop the domain they knew was running using TLS before and restart it expecting that it won't use TLS anymore, but on restart they discover that in our infinite wisdom we have set the optional "tls='yes'" property for that chardev on that domain. Now if we 'error' out on that start like you request above, then that means they will have to edit their domain and remove the seemingly optional property.
Next, let's assume they read the documentation and found that they can disable the qemu.conf value, but still have the domain chardev value if they set the "tls='yes'" property as long as they have their valid TLS directory configuration. In this case, they have made the conscious decision how they want their domain configured based on what they know is configured on the host.
TBH: I take this single patch as a "feature request" add-on to the original feature request that I believe in the long run won't be used. I could be wrong, but it's a feeling.
Furthermore, the purpose of any optional attribute is just that. It's optional based on some host wide setting. It's up to the consumer to decide how they should proceed, not the software to make that decision for them.
I guess that I'm unable to describe exactly what I mean so I'm attaching two patches, one is for introducing TLS attribute and the second one is to make sure that migration to libvirt-2.3.0 will work. Pavel

[...]
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
Nice, this is a good description how to use the *tls* attribute.
BTW (regarding your followup reply):
The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be parsed) refer to this as a "serial chardev"
This is a generic function that parses source for a lot of different device types/
Shortcutting in my mind to <source mode='{connect|bind} host='%s' service='%s'/> which is the VIR_DOMAIN_DEVICE_CHR, smartcard, rng, and redirdev. And yes, VIR_DOMAIN_DEVICE_CHR has paths for parallels, serials, consoles, and channels that are defined using <%s type='tcp'>.
The 'virDomainChrDefParseXML' comments have a list of "<serial ..." XML types. The location of the above description is describing a <serial type="tcp"> definition.
Well, the comment have that list but is used to parse all character devices, not only serial char device. TLS encryption can be used also for those types: parallel, channel, and console.
My continual battle against under documentation. The code is not self documenting in all cases...
The 'smartcard' discussion for a 'passthrough' device that would use this code says "Rather than having the hypervisor directly communicate with the host, it is possible to tunnel all requests through a secondary character device to a third-party provider (which may in turn be talking to a smartcard or using three certificate files). In this mode of operation, an additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel;..."
This comment is wrong, it should be "supported character device types". This attribute tells what interface is presented to the host. Check this part of documentation for all character devices:
<http://libvirt.org/formatdomain.html#elementsConsole>
there is this sentence:
"The interface presented to the host is given in the type attribute of the top-level element. The host interface is configured by the source element."
So it refers to all host interfaces:
<http://libvirt.org/formatdomain.html#elementsCharHostInterface>
The 'rng' discussion for backend that would use this code says "This backend connects to a source using the EGD protocol. The source is specified as a character device. Refer to character device host interface for more information. ..."
This is correct, there is no reference to serial character device.
Redevdir says "An additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel; type='tcp' or type='spicevmc' ..."
This is the same case as smartcard device, this is wrong.
So the long and short of it is, IMO it's a serial chardev device. Semantically it could be claimed otherwise, but the parsing proves otherwise as does the existing documentation of "Host interface" character devices.
I prefer to keep it described as is. It's only ever used, parsed, etc. when <devices>... <serial type="tcp">... <source mode='connect'..."
If anything, the description should become more restrictive to indicate that the option shouldn't be used for smartcards, rngs, and redirdevs, but I'll save that discussion for patch 3.
Based on the documentation it may appear that it should be a serial chardev, but that's misleading and it should refer only to the "host interfaces of character devices".
I cannot begin to describe how many times I've scrolled up and down through that discussion and thought how does anyone get this stuff correct... Trial and error I suppose. In any case, it seems of the 3 the rng is the most correct and the other two should get patches in order to be more correct. Not sure I can do it justice. It would seem to me that smartcard and redirdev should use the pointer to the elementsCharHostInterface. Still for the purposes of supported 'elementsCharHostInterface' when being used for specific smartcard, rng, and redirdev entries that are using "type='tcp'", only the <source mode='{connect|bind}' .../> would "appear to me" to apply as the "style" one would use in order to use TLS. That style just happens to have examples that list <serial type="tcp" <source mode=... />. Hence why I see this as "a serial chardev TCP" or a "host interface serial chardev TCP". There's got to be some means to describe it that focuses the attention on the <source ...> and not the "<%s type="tcp">" that I focused on. [...]
bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; }
So this function let's you decide whether we should try to set up *tls* for chardev or not. It work's but I have few issues with it.
At first I don't like that libvirt would try to do something smart and don't even tell user about the result. This will silently ignore the *tls* attribute if no certificate is found. In case that *tls* attribute is set to "yes" in XML and there is no certificate file to use we shouldn't start that domain and print an error to user.
This is a boolean function - so printing an error here isn't right.
Well, my comment implies to not use boolean function.
The callers were boolean checks, hence the generation of a boolean function.
Adding something to post parse processing is possible, but there's an impact based on whether we're setting a default value for haveTLS (something I disagree with doing).
Beyond that would a check go in qemuDomainDeviceDefPostParse or qemuDomainDeviceDefValidate? It's never crystal clear to me which should be used when just reading the code. Although since I see this as a "new" and "optional" value, I'd lean toward PostParse. Then there's dealing with the parseFlags that it's impacted by other decisions.
I could also rationalize that someone adding "tls='yes'" to their chardev would "know" what they're doing because they read the documentation. How else would they know to have this very specific combination (unless of course 'something' set things up that way based on the assumption of how a domain is currently running).
Again, IMO 'haveTLS' is new and optional. The only indication that a domain is using TLS was handled via 'tlscreds'. IIRC, the "reason" that 'tlscreds' exists and how "tls-creds" gets added to the command line is because the JSON code processing for a chardev is buried in qemu_monitor_json.c and fishing for a host configuration option at that time wasn't viable.
Secondly this way we don't reflect the current state for live domain in the live XML. This was probably lost during the discussion, but in general if there is an attribute that can affect running domain we should reflect the current state using that attribute. I know, there are some cases where we probably don't do that and they should be fixed.
No it wasn't lost - I considered it as I see you've seen from the cover. And you're probably right regarding attributes that trigger usage of some qemu option that we don't specifically save in the status XML. That's a different rat hole.
I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we can set the haveTLS based on cfg->chardevTLS. The whole purpose of qemuProcessPrepareDomain() is to prepare the domain definition so the qemuBuildCommandLine() don't have to check other places to enable some feature and not update the live definition. If the *tls* attribute is properly set in live definition that it will be saved to status XML and there is no need to do anything for qemuProcessReconnect.
I disagree on setting haveTLS during qemuProcessReconnect based upon chardevTLS. The 'haveTLS' is an optional attribute and by setting a value I believe we end up making an assumption.
I wrote that there is no need to do anything for qemuProcessReconnect.
I misinterpreted, but I also still have in my mind the previous discussion on this.
If *anything* was to be done it would be based solely on whether "tls-creds" is set on the command line of the reconnected domain. However, that too has a similar problem about setting a value for an optional attribute based upon the assumption that we know better.
Again, the only indication that 'tls-creds' is on the command line was from the 'tlscreds' boolean that was set because the host configuration information was available. A domain that is running and has tls-creds will continue to have it. Altering that domain's configuration file because we add a new optional *configuration* value has no bearing on the *status* XML. When/if a configuration XML is updated, it's not checking that 'tlscreds' value to determine that at some point in history the domain used TLS because the host was configured that way.
The whole point of having 'tls' attribute in live XML is to ensure that when libvirtd is restarted the attribute is still present in the XML because it will be saved to status XML and loaded again from status XML. There is no need to do any magic by parsing qemu command line, we have statu XML for that purpose to store all information about domain.
Let's see, you see the "tls={'yes'|'no'}" as essentially replacing the chardev_tls qemu.conf variable. I don't see it that way. The whole purpose of an optional property is just that to be optional. I should be able to choose to add it or not. If it's there and I remove it, but then on every restart libvirt replaces it - that seems to go against the idea of being optional. If it never existed and then it shows up as soon as I start the domain; is that right? If I'm comparing pure migration xml, wouldn't the to/newer system now have the field defined (thus creating different XML). Does that migrated safely back to the previous version (2.3.0)? It would have a field that the old system wouldn't know what to do with.
Let's consider the bool function from above and this automagic setting being requested. Let's say we reconnect to a domain, find the 'tls-creds' set on the command line, and set 'haveTLS=YES' based solely on that. Let's say at some point in time after, someone edits their qemu.conf file and sets 'chardev_tls=0' (or comments out the 'chardev_tls=1'). In their mind, they've now disabled chardev TLS for their host and any domain they will run in the future. They stop the domain they knew was running using TLS before and restart it expecting that it won't use TLS anymore, but on restart they discover that in our infinite wisdom we have set the optional "tls='yes'" property for that chardev on that domain. Now if we 'error' out on that start like you request above, then that means they will have to edit their domain and remove the seemingly optional property.
Next, let's assume they read the documentation and found that they can disable the qemu.conf value, but still have the domain chardev value if they set the "tls='yes'" property as long as they have their valid TLS directory configuration. In this case, they have made the conscious decision how they want their domain configured based on what they know is configured on the host.
TBH: I take this single patch as a "feature request" add-on to the original feature request that I believe in the long run won't be used. I could be wrong, but it's a feeling.
Furthermore, the purpose of any optional attribute is just that. It's optional based on some host wide setting. It's up to the consumer to decide how they should proceed, not the software to make that decision for them.
I guess that I'm unable to describe exactly what I mean so I'm attaching two patches, one is for introducing TLS attribute and the second one is to make sure that migration to libvirt-2.3.0 will work.
And I think the provided code proves that point. While "technically" still optional in the schema, the change in qemuProcessPrepareDomain forces it to be set as soon as a domain is started. Again, a bistate != tristate. We don't track the difference between undefined or defined, but set to 0 other than in the return values from qemu.conf parsing. I already had to remove a boolean that tracked that from a prior review. I think it should be strictly optional and that's where we differ. I see no reason to change the domain xml unless as a consumer that's what you want to do - be able to control which domains will have the setting. What else would be the purpose of a host wide setting to go with a domain optional setting? Finally, if your idea is accepted, that means for any configuration with chardev_tls=0 (either because it's commented or set that way), every domain that starts will be updated to have this new attribute "tls='no'". Then one day, I read up on this wonderful new feature and modify my qemu.conf file to set chardev_tls=1 and set up the TLS environment properly. I go to start my domain, but wait it's not using it. Closer inspection finds, someone put "tls='no'" into my domain... To me that's not right. And I won't necessarily know unless I know to look at the cmdline of the started domain to find that 'tls-creds' or I in some way "track" when TLS is being used. John

On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
[...]
+ <p> + <span class="since">Since 2.4.0,</span> the optional attribute + <code>tls</code> can be used to control whether a serial chardev + TCP communication channel would utilize a hypervisor configured + TLS X.509 certificate environment in order to encrypt the data + channel. For the QEMU hypervisor, usage of a TLS envronment can + be controlled on the host by the <code>chardev_tls</code> and + <code>chardev_tls_x509_cert_dir</code> or + <code>default_tls_x509_cert_dir</code> settings in the file + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, + then unless the <code>tls</code> attribute is set to "no", libvirt + will use the host configured TLS environment. + If <code>chardev_tls</code> is disabled, but the <code>tls</code> + attribute is set to "yes", then libvirt will attempt to use the + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. + </p>
Nice, this is a good description how to use the *tls* attribute.
BTW (regarding your followup reply):
The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be parsed) refer to this as a "serial chardev"
This is a generic function that parses source for a lot of different device types/
Shortcutting in my mind to <source mode='{connect|bind} host='%s' service='%s'/> which is the VIR_DOMAIN_DEVICE_CHR, smartcard, rng, and redirdev. And yes, VIR_DOMAIN_DEVICE_CHR has paths for parallels, serials, consoles, and channels that are defined using <%s type='tcp'>.
The 'virDomainChrDefParseXML' comments have a list of "<serial ..." XML types. The location of the above description is describing a <serial type="tcp"> definition.
Well, the comment have that list but is used to parse all character devices, not only serial char device. TLS encryption can be used also for those types: parallel, channel, and console.
My continual battle against under documentation. The code is not self documenting in all cases...
I agree, we should be better in documenting things.
The 'smartcard' discussion for a 'passthrough' device that would use this code says "Rather than having the hypervisor directly communicate with the host, it is possible to tunnel all requests through a secondary character device to a third-party provider (which may in turn be talking to a smartcard or using three certificate files). In this mode of operation, an additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel;..."
This comment is wrong, it should be "supported character device types". This attribute tells what interface is presented to the host. Check this part of documentation for all character devices:
<http://libvirt.org/formatdomain.html#elementsConsole>
there is this sentence:
"The interface presented to the host is given in the type attribute of the top-level element. The host interface is configured by the source element."
So it refers to all host interfaces:
<http://libvirt.org/formatdomain.html#elementsCharHostInterface>
The 'rng' discussion for backend that would use this code says "This backend connects to a source using the EGD protocol. The source is specified as a character device. Refer to character device host interface for more information. ..."
This is correct, there is no reference to serial character device.
Redevdir says "An additional attribute type is required, matching one of the supported serial device types, to describe the host side of the tunnel; type='tcp' or type='spicevmc' ..."
This is the same case as smartcard device, this is wrong.
So the long and short of it is, IMO it's a serial chardev device. Semantically it could be claimed otherwise, but the parsing proves otherwise as does the existing documentation of "Host interface" character devices.
I prefer to keep it described as is. It's only ever used, parsed, etc. when <devices>... <serial type="tcp">... <source mode='connect'..."
If anything, the description should become more restrictive to indicate that the option shouldn't be used for smartcards, rngs, and redirdevs, but I'll save that discussion for patch 3.
Based on the documentation it may appear that it should be a serial chardev, but that's misleading and it should refer only to the "host interfaces of character devices".
I cannot begin to describe how many times I've scrolled up and down through that discussion and thought how does anyone get this stuff correct... Trial and error I suppose.
In any case, it seems of the 3 the rng is the most correct and the other two should get patches in order to be more correct. Not sure I can do it justice. It would seem to me that smartcard and redirdev should use the pointer to the elementsCharHostInterface.
Yes and since now we know about this flaw in our documentation it should be fixed.
Still for the purposes of supported 'elementsCharHostInterface' when being used for specific smartcard, rng, and redirdev entries that are using "type='tcp'", only the <source mode='{connect|bind}' .../> would "appear to me" to apply as the "style" one would use in order to use TLS. That style just happens to have examples that list <serial type="tcp" <source mode=... />.
Hence why I see this as "a serial chardev TCP" or a "host interface serial chardev TCP". There's got to be some means to describe it that focuses the attention on the <source ...> and not the "<%s type="tcp">" that I focused on.
Yes, it's tricky and it took me a while to understand all of this, but I had to combine the documentation with source code and that means we need to update that documentation.
[...]
bool -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, + const virDomainChrSourceDef *dev) { - if (cfg->chardevTLS) + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) + return true; + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && + virFileExists(cfg->chardevTLSx509certdir)) return true; return false; }
So this function let's you decide whether we should try to set up *tls* for chardev or not. It work's but I have few issues with it.
At first I don't like that libvirt would try to do something smart and don't even tell user about the result. This will silently ignore the *tls* attribute if no certificate is found. In case that *tls* attribute is set to "yes" in XML and there is no certificate file to use we shouldn't start that domain and print an error to user.
This is a boolean function - so printing an error here isn't right.
Well, my comment implies to not use boolean function.
The callers were boolean checks, hence the generation of a boolean function.
Adding something to post parse processing is possible, but there's an impact based on whether we're setting a default value for haveTLS (something I disagree with doing).
Beyond that would a check go in qemuDomainDeviceDefPostParse or qemuDomainDeviceDefValidate? It's never crystal clear to me which should be used when just reading the code. Although since I see this as a "new" and "optional" value, I'd lean toward PostParse. Then there's dealing with the parseFlags that it's impacted by other decisions.
I could also rationalize that someone adding "tls='yes'" to their chardev would "know" what they're doing because they read the documentation. How else would they know to have this very specific combination (unless of course 'something' set things up that way based on the assumption of how a domain is currently running).
Again, IMO 'haveTLS' is new and optional. The only indication that a domain is using TLS was handled via 'tlscreds'. IIRC, the "reason" that 'tlscreds' exists and how "tls-creds" gets added to the command line is because the JSON code processing for a chardev is buried in qemu_monitor_json.c and fishing for a host configuration option at that time wasn't viable.
Secondly this way we don't reflect the current state for live domain in the live XML. This was probably lost during the discussion, but in general if there is an attribute that can affect running domain we should reflect the current state using that attribute. I know, there are some cases where we probably don't do that and they should be fixed.
No it wasn't lost - I considered it as I see you've seen from the cover. And you're probably right regarding attributes that trigger usage of some qemu option that we don't specifically save in the status XML. That's a different rat hole.
I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we can set the haveTLS based on cfg->chardevTLS. The whole purpose of qemuProcessPrepareDomain() is to prepare the domain definition so the qemuBuildCommandLine() don't have to check other places to enable some feature and not update the live definition. If the *tls* attribute is properly set in live definition that it will be saved to status XML and there is no need to do anything for qemuProcessReconnect.
I disagree on setting haveTLS during qemuProcessReconnect based upon chardevTLS. The 'haveTLS' is an optional attribute and by setting a value I believe we end up making an assumption.
I wrote that there is no need to do anything for qemuProcessReconnect.
I misinterpreted, but I also still have in my mind the previous discussion on this.
If *anything* was to be done it would be based solely on whether "tls-creds" is set on the command line of the reconnected domain. However, that too has a similar problem about setting a value for an optional attribute based upon the assumption that we know better.
Again, the only indication that 'tls-creds' is on the command line was from the 'tlscreds' boolean that was set because the host configuration information was available. A domain that is running and has tls-creds will continue to have it. Altering that domain's configuration file because we add a new optional *configuration* value has no bearing on the *status* XML. When/if a configuration XML is updated, it's not checking that 'tlscreds' value to determine that at some point in history the domain used TLS because the host was configured that way.
The whole point of having 'tls' attribute in live XML is to ensure that when libvirtd is restarted the attribute is still present in the XML because it will be saved to status XML and loaded again from status XML. There is no need to do any magic by parsing qemu command line, we have statu XML for that purpose to store all information about domain.
Let's see, you see the "tls={'yes'|'no'}" as essentially replacing the chardev_tls qemu.conf variable.
No, I don't see it that way, it's still an optional attribute that don't have to be specified while defining domain, in other words it don't have to be present in config XML.
I don't see it that way. The whole purpose of an optional property is just that to be optional. I should be able to choose to add it or not. If it's there and I remove it, but then on every restart libvirt replaces it - that seems to go against the idea of being optional. If it never existed and then it shows up as soon as I start the domain; is that right?
This part is right, if the attribute is not configured in config XML it should be updated when the domain is started to reflect the current state and it will be present only in online XML.
If I'm comparing pure migration xml, wouldn't the to/newer system now have the field defined (thus creating different XML). Does that migrated safely back to the previous version (2.3.0)? It would have a field that the old system wouldn't know what to do with.
The second patch solves this issue.
Let's consider the bool function from above and this automagic setting being requested. Let's say we reconnect to a domain, find the 'tls-creds' set on the command line, and set 'haveTLS=YES' based solely on that. Let's say at some point in time after, someone edits their qemu.conf file and sets 'chardev_tls=0' (or comments out the 'chardev_tls=1'). In their mind, they've now disabled chardev TLS for their host and any domain they will run in the future. They stop the domain they knew was running using TLS before and restart it expecting that it won't use TLS anymore, but on restart they discover that in our infinite wisdom we have set the optional "tls='yes'" property for that chardev on that domain. Now if we 'error' out on that start like you request above, then that means they will have to edit their domain and remove the seemingly optional property.
Next, let's assume they read the documentation and found that they can disable the qemu.conf value, but still have the domain chardev value if they set the "tls='yes'" property as long as they have their valid TLS directory configuration. In this case, they have made the conscious decision how they want their domain configured based on what they know is configured on the host.
TBH: I take this single patch as a "feature request" add-on to the original feature request that I believe in the long run won't be used. I could be wrong, but it's a feeling.
Furthermore, the purpose of any optional attribute is just that. It's optional based on some host wide setting. It's up to the consumer to decide how they should proceed, not the software to make that decision for them.
I guess that I'm unable to describe exactly what I mean so I'm attaching two patches, one is for introducing TLS attribute and the second one is to make sure that migration to libvirt-2.3.0 will work.
And I think the provided code proves that point. While "technically" still optional in the schema, the change in qemuProcessPrepareDomain forces it to be set as soon as a domain is started. Again, a bistate != tristate. We don't track the difference between undefined or defined, but set to 0 other than in the return values from qemu.conf parsing. I already had to remove a boolean that tracked that from a prior review.
Now I see why we are not able to agree on this change. The modification done in qemuProcessPrepareDomain() are only to live definition, the config definition remains untouched, which means the *tls* attribute (if it's based on qemu.conf) will appear only in live XML. The config XML will remain the same after the guest is stopped.
I think it should be strictly optional and that's where we differ. I see no reason to change the domain xml unless as a consumer that's what you want to do - be able to control which domains will have the setting. What else would be the purpose of a host wide setting to go with a domain optional setting?
Finally, if your idea is accepted, that means for any configuration with chardev_tls=0 (either because it's commented or set that way), every domain that starts will be updated to have this new attribute "tls='no'". Then one day, I read up on this wonderful new feature and
Not the domain, only the live XML which is not saved as config XML ...
modify my qemu.conf file to set chardev_tls=1 and set up the TLS environment properly. I go to start my domain, but wait it's not using
And after you start the domain there will be "tls='yes'" because the config XML doesn't contain any *tls* attribute. I've tested all of those cases before proposing this patch: prerequisite: prepare certificate files to be used for chardev devices for running domain: live XML - virsh dumpxml $domain config XML - virsh dumpxml $domain --config migratable XML - virsh dumpxml $domain --migratable 1. set chardev_tls = 1 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'" c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'" 2. set chardev_tls = 0 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'" c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'" Pavel
it. Closer inspection finds, someone put "tls='no'" into my domain... To me that's not right. And I won't necessarily know unless I know to look at the cmdline of the started domain to find that 'tls-creds' or I in some way "track" when TLS is being used.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
[...]
Since I assume you have these changes in your local branch - let's go with the two patches and move on. It would be nice while it's still fresh to update the documentation, but that's a separate patch. So "officially" it's an ACK of your changes on top of and in addition to my changes. John [...]
Now I see why we are not able to agree on this change. The modification done in qemuProcessPrepareDomain() are only to live definition, the config definition remains untouched, which means the *tls* attribute (if it's based on qemu.conf) will appear only in live XML. The config XML will remain the same after the guest is stopped.
I think it should be strictly optional and that's where we differ. I see no reason to change the domain xml unless as a consumer that's what you want to do - be able to control which domains will have the setting. What else would be the purpose of a host wide setting to go with a domain optional setting?
Finally, if your idea is accepted, that means for any configuration with chardev_tls=0 (either because it's commented or set that way), every domain that starts will be updated to have this new attribute "tls='no'". Then one day, I read up on this wonderful new feature and
Not the domain, only the live XML which is not saved as config XML ...
modify my qemu.conf file to set chardev_tls=1 and set up the TLS environment properly. I go to start my domain, but wait it's not using
And after you start the domain there will be "tls='yes'" because the config XML doesn't contain any *tls* attribute.
I've tested all of those cases before proposing this patch:
prerequisite: prepare certificate files to be used for chardev devices
for running domain: live XML - virsh dumpxml $domain config XML - virsh dumpxml $domain --config migratable XML - virsh dumpxml $domain --migratable
1. set chardev_tls = 1 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure
b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'"
c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'"
2. set chardev_tls = 0 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure
b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'"
c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'"
Pavel
it. Closer inspection finds, someone put "tls='no'" into my domain... To me that's not right. And I won't necessarily know unless I know to look at the cmdline of the started domain to find that 'tls-creds' or I in some way "track" when TLS is being used.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 21, 2016 at 07:15:55AM -0400, John Ferlan wrote:
On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
[...]
Since I assume you have these changes in your local branch - let's go with the two patches and move on.
It would be nice while it's still fresh to update the documentation, but that's a separate patch.
So "officially" it's an ACK of your changes on top of and in addition to my changes.
John
We should probably figure out also rng, smartcard and redirdevs before pushing. They also use the source as you've pointed out and currently they can be configured to use TLS with the chardev_tls config option. I'll send a new version of patch series to cover all users of TCP source type. Pavel
[...]
Now I see why we are not able to agree on this change. The modification done in qemuProcessPrepareDomain() are only to live definition, the config definition remains untouched, which means the *tls* attribute (if it's based on qemu.conf) will appear only in live XML. The config XML will remain the same after the guest is stopped.
I think it should be strictly optional and that's where we differ. I see no reason to change the domain xml unless as a consumer that's what you want to do - be able to control which domains will have the setting. What else would be the purpose of a host wide setting to go with a domain optional setting?
Finally, if your idea is accepted, that means for any configuration with chardev_tls=0 (either because it's commented or set that way), every domain that starts will be updated to have this new attribute "tls='no'". Then one day, I read up on this wonderful new feature and
Not the domain, only the live XML which is not saved as config XML ...
modify my qemu.conf file to set chardev_tls=1 and set up the TLS environment properly. I go to start my domain, but wait it's not using
And after you start the domain there will be "tls='yes'" because the config XML doesn't contain any *tls* attribute.
I've tested all of those cases before proposing this patch:
prerequisite: prepare certificate files to be used for chardev devices
for running domain: live XML - virsh dumpxml $domain config XML - virsh dumpxml $domain --config migratable XML - virsh dumpxml $domain --migratable
1. set chardev_tls = 1 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure
b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'"
c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'"
2. set chardev_tls = 0 a) start domain where there is no *tls* attribute in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is no *tls* attribute - in the migratable XML there is no *tls* attribure
b) start domain where there is "tls='no'" in config XML - the domain is started and TLS is not configured - in the live XML there is "tls='no'" - in the config XML there is "tls='no'" - in the migratable XML there is "tls='no'"
c) start domain where there is "tls='yes'" in config XML - the domain is started and TLS is properly configured - in the live XML there is "tls='yes'" - in the config XML there is "tls='yes'" - in the migratable XML there is "tls='yes'"
Pavel
it. Closer inspection finds, someone put "tls='no'" into my domain... To me that's not right. And I won't necessarily know unless I know to look at the cmdline of the started domain to find that 'tls-creds' or I in some way "track" when TLS is being used.
John
-- 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/21/2016 07:57 AM, Pavel Hrdina wrote:
On Fri, Oct 21, 2016 at 07:15:55AM -0400, John Ferlan wrote:
On 10/21/2016 02:29 AM, Pavel Hrdina wrote:
On Thu, Oct 20, 2016 at 03:48:30PM -0400, John Ferlan wrote:
[...]
Since I assume you have these changes in your local branch - let's go with the two patches and move on.
It would be nice while it's still fresh to update the documentation, but that's a separate patch.
So "officially" it's an ACK of your changes on top of and in addition to my changes.
John
We should probably figure out also rng, smartcard and redirdevs before pushing. They also use the source as you've pointed out and currently they can be configured to use TLS with the chardev_tls config option. I'll send a new version of patch series to cover all users of TCP source type.
The rng, smartcard, and redirdevs for hotplug is a different issue though. They come more into play when adding the passphrase. As painful as it is (or will be to review) - I'm trying to move the privateData from virDomainChrDef to virDomainChrSourceDef. That will then magically do the right thing for rng, smartcard, and redirdevs. Right now for command line processing they work fine since adding the tls-creds is based on virDomainChrSourceDef John

Add the secret object prior to the chardev tcp so the 'passwordid=' can be added if the domain XML has a <secret> for the chardev TLS. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 ++++++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 103 ++++++++++++++++++++- src/qemu/qemu_domain.h | 16 +++- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 4 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 ++++++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++++++ tests/qemuxml2argvtest.c | 17 ++++ 9 files changed, 254 insertions(+), 7 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 f00751a..9a14b44 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; } @@ -4949,6 +4963,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + !!cfg->chardevTLSx509secretUUID, charAlias, qemuCaps) < 0) goto error; @@ -8546,6 +8561,18 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, /* Use -chardev with -device if they are available */ if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(serial); + + /* 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 (serial->source.type == VIR_DOMAIN_CHR_TYPE_TCP && + chardevPriv && chardevPriv->secinfo && + qemuBuildObjectSecretCommandLine(cmd, chardevPriv->secinfo) < 0) + return -1; + if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &serial->source, serial->info.alias, 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 7b518c6..adc078b 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,95 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, } +/* qemuDomainSecretChardevDestroy: + * @disk: Pointer to a chardev definition + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) +{ + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + if (!chardevPriv || !chardevPriv->secinfo) + return; + + qemuDomainSecretInfoFree(&chardevPriv->secinfo); +} + + +/* qemuDomainSecretChardevPrepare: + * @conn: Pointer to connection + * @driver: Pointer to driver object + * @priv: pointer to domain private object + * @chardev: Pointer to a chardev 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, + virDomainChrDefPtr chardev) +{ + virQEMUDriverConfigPtr cfg; + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + char *charAlias = NULL; + + if (chardev->source.type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + if (qemuDomainSupportTLSChardevTCP(cfg, &chardev->source) && + cfg->chardevTLSx509secretUUID) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + 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(chardev->info.alias))) + 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; + } + + chardevPriv->secinfo = secinfo; + } + + VIR_FREE(charAlias); + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1236,11 +1326,15 @@ 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]); } /* 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 +1347,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1269,6 +1364,12 @@ qemuDomainSecretPrepare(virConnectPtr conn, return -1; } + for (i = 0; i < vm->def->nserials; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->serials[i]) < 0) + return -1; + } + return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 156e0fc..1bb2f2d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -726,11 +726,23 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + virDomainChrDefPtr chardev) + 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 9643a68..59c3b25 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1734,6 +1734,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + NULL, priv->qemuCaps, &tlsProps) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d641f33..643741d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5157,8 +5157,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; - VIR_DEBUG("Add secrets to disks and hostdevs"); - if (qemuDomainSecretPrepare(conn, vm) < 0) + 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++) { 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..23c244b --- /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' tls='yes'/> + <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 Wed, Oct 19, 2016 at 04:53:55PM -0400, John Ferlan wrote:
Add the secret object prior to the chardev tcp so the 'passwordid=' can be added if the domain XML has a <secret> for the chardev TLS.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 31 ++++++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 103 ++++++++++++++++++++- src/qemu/qemu_domain.h | 16 +++- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 4 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 ++++++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 ++++++++++ tests/qemuxml2argvtest.c | 17 ++++ 9 files changed, 254 insertions(+), 7 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 f00751a..9a14b44 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; }
@@ -4949,6 +4963,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + !!cfg->chardevTLSx509secretUUID, charAlias, qemuCaps) < 0) goto error;
@@ -8546,6 +8561,18 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
/* Use -chardev with -device if they are available */ if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(serial); + + /* 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 (serial->source.type == VIR_DOMAIN_CHR_TYPE_TCP && + chardevPriv && chardevPriv->secinfo && + qemuBuildObjectSecretCommandLine(cmd, chardevPriv->secinfo) < 0) + return -1; +
This should be used also for other chardev devices.
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &serial->source, serial->info.alias, 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 7b518c6..adc078b 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,95 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretChardevDestroy: + * @disk: Pointer to a chardev definition + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) +{ + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + if (!chardevPriv || !chardevPriv->secinfo) + return; + + qemuDomainSecretInfoFree(&chardevPriv->secinfo); +} + + +/* qemuDomainSecretChardevPrepare: + * @conn: Pointer to connection + * @driver: Pointer to driver object + * @priv: pointer to domain private object + * @chardev: Pointer to a chardev 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, + virDomainChrDefPtr chardev) +{ + virQEMUDriverConfigPtr cfg; + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + char *charAlias = NULL; + + if (chardev->source.type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + if (qemuDomainSupportTLSChardevTCP(cfg, &chardev->source) && + cfg->chardevTLSx509secretUUID) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + 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(chardev->info.alias))) + 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; + } + + chardevPriv->secinfo = secinfo; + } + + VIR_FREE(charAlias); + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1236,11 +1326,15 @@ 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]);
This should iterate over all chardevs, not only over serial ...
}
/* 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 +1347,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1269,6 +1364,12 @@ qemuDomainSecretPrepare(virConnectPtr conn, return -1; }
+ for (i = 0; i < vm->def->nserials; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->serials[i]) < 0) + return -1; + }
... and same here. Pavel
+ return 0; }
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 156e0fc..1bb2f2d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -726,11 +726,23 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+void qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + virDomainChrDefPtr chardev) + 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 9643a68..59c3b25 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1734,6 +1734,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + NULL, priv->qemuCaps, &tlsProps) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d641f33..643741d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5157,8 +5157,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup;
- VIR_DEBUG("Add secrets to disks and hostdevs"); - if (qemuDomainSecretPrepare(conn, vm) < 0) + 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++) { 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..23c244b --- /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' tls='yes'/> + <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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

[...]
@@ -8546,6 +8561,18 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,
/* Use -chardev with -device if they are available */ if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(serial); + + /* 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 (serial->source.type == VIR_DOMAIN_CHR_TYPE_TCP && + chardevPriv && chardevPriv->secinfo && + qemuBuildObjectSecretCommandLine(cmd, chardevPriv->secinfo) < 0) + return -1; +
This should be used also for other chardev devices.
Easily done for parallels, consoles, and channel... IOW: Others that call virDomainChrDefParseXML which calls virDomainChrDefNew(xmlopt) in order to set up privateData. Less easy for smartcard, rng, and redirdev's. Unfortunately the PRIVATE stuff is based off a virDomainChrDefPtr while the smartcard, rng, and redirdev's each use their own virDomain*DefPtr that includes a virDomainChrSourceDefPtr (for RNG) or virDomainChrSourceDef (for smartcard and redirdev). BTW: The hotplug code for smartcard, rng, and redirdev isn't implemented... Follow qemuDomainAttachChrDevice callers and you'll see separate calls for redirdev, rng, and smartcard. Clearly I misread all the various permutations... Looks like I'll need to either implement privateData for virDomainCharSourceDefPtr or duplicate what's done for ChrDef for redevdir, rng, and smartcard. Neither is appealing... John
if (!(devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, &serial->source, serial->info.alias, 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 7b518c6..adc078b 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,95 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, }
+/* qemuDomainSecretChardevDestroy: + * @disk: Pointer to a chardev definition + * + * Clear and destroy memory associated with the secret + */ +void +qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) +{ + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + if (!chardevPriv || !chardevPriv->secinfo) + return; + + qemuDomainSecretInfoFree(&chardevPriv->secinfo); +} + + +/* qemuDomainSecretChardevPrepare: + * @conn: Pointer to connection + * @driver: Pointer to driver object + * @priv: pointer to domain private object + * @chardev: Pointer to a chardev 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, + virDomainChrDefPtr chardev) +{ + virQEMUDriverConfigPtr cfg; + virSecretLookupTypeDef seclookupdef = {0}; + qemuDomainSecretInfoPtr secinfo = NULL; + char *charAlias = NULL; + + if (chardev->source.type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + if (qemuDomainSupportTLSChardevTCP(cfg, &chardev->source) && + cfg->chardevTLSx509secretUUID) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chardev); + + 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(chardev->info.alias))) + 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; + } + + chardevPriv->secinfo = secinfo; + } + + VIR_FREE(charAlias); + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1236,11 +1326,15 @@ 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]);
This should iterate over all chardevs, not only over serial ...
}
/* 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 +1347,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1269,6 +1364,12 @@ qemuDomainSecretPrepare(virConnectPtr conn, return -1; }
+ for (i = 0; i < vm->def->nserials; i++) { + if (qemuDomainSecretChardevPrepare(conn, driver, priv, + vm->def->serials[i]) < 0) + return -1; + }
... and same here.
Pavel
+ return 0; }
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 156e0fc..1bb2f2d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -726,11 +726,23 @@ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+void qemuDomainSecretChardevDestroy(virDomainChrDefPtr chardev) + ATTRIBUTE_NONNULL(1); + +int qemuDomainSecretChardevPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + virDomainChrDefPtr chardev) + 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 9643a68..59c3b25 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1734,6 +1734,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + NULL, priv->qemuCaps, &tlsProps) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d641f33..643741d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5157,8 +5157,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup;
- VIR_DEBUG("Add secrets to disks and hostdevs"); - if (qemuDomainSecretPrepare(conn, vm) < 0) + 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++) { 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..23c244b --- /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' tls='yes'/> + <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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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 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 | 2 +- src/qemu/qemu_hotplug.c | 57 ++++++++++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_hotplug.h | 3 ++- tests/qemuhotplugtest.c | 2 +- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8789c9d..5a1cf7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -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; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 59c3b25..4935bbf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1690,7 +1690,8 @@ qemuDomainAttachChrDeviceAssignAddr(virDomainDefPtr def, return ret; } -int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, +int qemuDomainAttachChrDevice(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr) { @@ -1704,8 +1705,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 && @@ -1729,12 +1733,28 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; + if (qemuDomainSecretChardevPrepare(conn, driver, priv, chr) < 0) + goto cleanup; + if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP && qemuDomainSupportTLSChardevTCP(cfg, dev)) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chr); + + /* Add a secret object in order to access the TLS environment. + * The secinfo will only be created for serial TCP device. */ + if (chardevPriv && chardevPriv->secinfo) { + if (qemuBuildSecretInfoProps(chardevPriv->secinfo, &secProps) < 0) + goto cleanup; + + if (!(secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + goto cleanup; + } + if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - NULL, + secAlias, priv->qemuCaps, &tlsProps) < 0) goto cleanup; @@ -1745,6 +1765,15 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, } 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); @@ -1775,6 +1804,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); @@ -1782,6 +1813,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 */ @@ -4387,6 +4420,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *objAlias = NULL; + char *secAlias = NULL; char *devstr = NULL; char *charAlias = NULL; @@ -4405,9 +4439,18 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && - qemuDomainSupportTLSChardevTCP(cfg, &tmpChr->source) && - !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) - goto cleanup; + qemuDomainSupportTLSChardevTCP(cfg, &tmpChr->source)) { + if (!(objAlias = 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; + } if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; @@ -4421,6 +4464,10 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (objAlias && qemuMonitorDelObject(priv->mon, objAlias) < 0) goto exit_monitor; + /* If it fails, then so be it - it was a best shot */ + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias)); + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index b048cf4..8f23176 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -92,7 +92,8 @@ 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, 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
participants (2)
-
John Ferlan
-
Pavel Hrdina