Re: [libvirt] [PATCH v8 1/5] domain: Add optional 'tls' attribute for TCP chardev

On 10/14/2016 09:18 AM, Pavel Hrdina wrote: > On Fri, Oct 07, 2016 at 07:00:26AM -0400, John Ferlan wrote: >> Add an optional "tls='yes'" option for a TCP chardev for the >> express purpose to enable setting up TLS for the chardev. This >> will assume that the qemu.conf settings have been adjusted as >> well as the environment configured properly. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> docs/formatdomain.html.in | 21 +++++++++ >> docs/schemas/domaincommon.rng | 5 +++ >> src/conf/domain_conf.c | 22 +++++++++- >> src/conf/domain_conf.h | 1 + >> src/qemu/qemu_command.c | 3 +- >> src/qemu/qemu_hotplug.c | 3 +- >> ...emuxml2argv-serial-tcp-tlsx509-chardev-tls.args | 30 +++++++++++++ >> ...qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml | 50 ++++++++++++++++++++++ >> .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml | 2 +- >> tests/qemuxml2argvtest.c | 3 ++ >> ...muxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml | 1 + >> .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml | 2 +- >> tests/qemuxml2xmltest.c | 1 + >> 13 files changed, 139 insertions(+), 5 deletions(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.args >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml >> create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index 1266e9d..010530e 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -6203,6 +6203,27 @@ qemu-kvm -net nic,model=? /dev/null >> &lt;/devices&gt; >> ...</pre> >> >> + <p> >> + <span class="since">Since 2.4.0,</span> some hypervisors support using >> + a TLS X.509 certificate environment in order to encrypt all serial TCP >> + connections via a hypervisor configuration option. In order to enable >> + TLS for the domain an optional attribute <code>tls</code> can be set to >> + "yes". Combined with the hypervisor's capability to utilize the TLS >> + environment allows for the character device to use the encrypted >> + communication. If the attribute is not present, then the default >> + setting is "no". > > This is not correct in case of QEMU. Before this patch the default was based on > chardev_tls from qemu.conf. After this patch the default will be "no" even if > you set chardev_tls=1 and that breaks behavior of libvirt. The test case > "serial-tcp-tlsx509-chardev-tls" where you had to add tls="yes" should also work > without this change. > > This patch needs to be modified to take chardev_tls=1 in account and add tests > to make sure that we don't break it in the future. > This change was in response to v5 (and v6) review comments from Daniel : v5: http://www.redhat.com/archives/libvir-list/2016-September/msg00294.html v6: http://www.redhat.com/archives/libvir-list/2016-September/msg00317.html I read those review requests as we need a way to be able to disable TLS on a chardev on a domain by domain and even chardev by chardev basis rather than it being the default enabled for every domain and every chardev in the domain. Could you elaborate how it breaks the behavior of libvirt? Currently a chardev doesn't have any TLS attribute - that qemu.conf change is from already agreed upon changes. This set of changes should have been able to go with those, but they've languished on the least through a release cycle (2.3.0), so while it may appear that something is one way - it in a way isn't. Right now it's just pixie/fairy dust and a pipe dream.

John > Pavel > >> + </p> >> +<pre> >> + ... >> + &lt;devices&gt; >> + &lt;serial type="tcp"&gt; >> + &lt;source mode='connect' host="127.0.0.1" service="5555" tls="yes"/&gt; >> + &lt;protocol type="raw"/&gt; >> + &lt;target port="0"/&gt; >> + &lt;/serial&gt; >> + &lt;/devices&gt; >> + ...</pre> >> + >> <h6><a name="elementsCharUDP">UDP network console</a></h6> >> >> <p> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 6eeb4e9..362b90d 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 f562323..1f7c43f 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -1961,6 +1961,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: >> @@ -9999,6 +10001,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, >> char *master = NULL; >> char *slave = NULL; >> char *append = NULL; >> + char *haveTLS = NULL; >> int remaining = 0; >> >> while (cur != NULL) { >> @@ -10006,6 +10009,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: >> @@ -10182,6 +10187,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 = >> @@ -10266,6 +10280,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, >> VIR_FREE(append); >> VIR_FREE(logappend); >> VIR_FREE(logfile); >> + VIR_FREE(haveTLS); >> >> return remaining; >> >> @@ -21417,7 +21432,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 a70bc21..da203c3 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 578ff8b..b290ade 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -5002,7 +5002,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, >> telnet ? ",telnet" : "", >> dev->data.tcp.listen ? ",server,nowait" : ""); >> >> - if (cfg->chardevTLS) { >> + if (cfg->chardevTLS && >> + dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { >> char *objalias = NULL; >> >> if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 72dd93b..419296e 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -1677,7 +1677,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, >> if (qemuDomainChrPreInsert(vmdef, chr) < 0) >> goto cleanup; >> >> - if (cfg->chardevTLS) { >> + if (cfg->chardevTLS && >> + dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { >> if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, >> dev->data.tcp.listen, >> cfg->chardevTLSx509verify, >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.args >> new file mode 100644 >> index 0000000..cac0d85 >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.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-tls.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml >> new file mode 100644 >> index 0000000..debc69b >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.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/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.xml >> index 1618b02..1d7896d 100644 >> --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.xml >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.xml >> @@ -27,7 +27,7 @@ >> <target port='0'/> >> </serial> >> <serial type='tcp'> >> - <source mode='connect' host='127.0.0.1' service='5555'/> >> + <source mode='connect' host='127.0.0.1' service='5555' tls='yes'/> >> <protocol type='raw'/> >> <target port='0'/> >> </serial> >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index 4b9ecb8..41adff4 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c >> @@ -1150,6 +1150,9 @@ mymain(void) >> DO_TEST("serial-tcp-tlsx509-chardev", >> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG, >> QEMU_CAPS_OBJECT_TLS_CREDS_X509); >> + DO_TEST("serial-tcp-tlsx509-chardev-tls", >> + 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-tls.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml >> new file mode 120000 >> index 0000000..3453497 >> --- /dev/null >> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml >> @@ -0,0 +1 @@ >> +../qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml >> \ No newline at end of file >> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml >> index 832e2a2..23c244b 100644 >> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml >> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml >> @@ -32,7 +32,7 @@ >> <target port='0'/> >> </serial> >> <serial type='tcp'> >> - <source mode='connect' host='127.0.0.1' service='5555'/> >> + <source mode='connect' host='127.0.0.1' service='5555' tls='yes'/> >> <protocol type='raw'/> >> <target port='0'/> >> </serial> >> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c >> index 4b58986..02fe32e 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-tls", 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(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list