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@redhat.com> --- 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 </devices> ...</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. Pavel

+ </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 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 14, 2016 at 09:32:45AM -0400, John Ferlan wrote:

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@redhat.com> --- 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 </devices> ...</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.

The thing is that with libvirt-2.3.0 you can set chardev_tls=1 in qemu.conf and therefore all chardevs for all domains uses TLS to encrypt communication. This patch introduces a new 'tls' attribute and its default value is 'no' and that means if the user upgrades from libvirt-2.3.0 to libvirt-2.4.0 all chardevs stop encrypting communication unless he sets tls="yes" for every single chardev in every single domain. The correct solution is: - if chardev_tls is set to 1 and tls attribute is not configured in the XML the default after starting the domain should be tls=yes - if chardev_tls is not set and tls attribute is not configured in the XML the default after starting the domain should be tls=no Pavel

John

...

Pavel

...

+ </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 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@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 Fri, Oct 14, 2016 at 10:16:19AM -0400, John Ferlan wrote:

On 10/14/2016 09:49 AM, Pavel Hrdina wrote:

...

On Fri, Oct 14, 2016 at 09:32:45AM -0400, John Ferlan wrote:

...

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@redhat.com> --- 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 </devices> ...</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.

The thing is that with libvirt-2.3.0 you can set chardev_tls=1 in qemu.conf and therefore all chardevs for all domains uses TLS to encrypt communication. This patch introduces a new 'tls' attribute and its default value is 'no' and that means if the user upgrades from libvirt-2.3.0 to libvirt-2.4.0 all chardevs stop encrypting communication unless he sets tls="yes" for every single chardev in every single domain.

UGH... mea culpa - in my mind the TLS hadn't been added to the chardev yet and that was the rest of this series... Of course the rest of this series is adding the passphrase for the environment (<sigh>).

If I could have got that review earlier, then this situation wouldn't be a problem (see in my mind it wasn't).

If a domain is already running, then encryption is occurring and this setting has no effect except for hotplug.

If we were using encryption in 2.3.0... stopped our domain... installed 2.4.0... started our domain, then yes, I see/agree.... Frustrating since there's really no "simple" way to determine this.

...

The correct solution is:

- if chardev_tls is set to 1 and tls attribute is not configured in the XML the default after starting the domain should be tls=yes

So IOW:

+ if (cfg->chardevTLS && + dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) {

changes to

+ if (cfg->chardevTLS && + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) {

or (haveTLS == YES || haveTLS == ABSENT)

This of course is essentially the logic from v6 which said disableTLS on a case by case basis.... All we have now is the positive form...

So for both qemuBuildChrChardevStr and qemuDomainAttachChrDevice I'd have a similar change. Does that suffice and do you need to see the change?

I think that we should reflect the state in live XML if the attribute exists. Add a code to qemuProcessPrepareDomain() that will set dev->data.tcp.haveTLS to config value if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_ABSENT and in qemuBuildChrChardevStr() you can just check if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES. This will also ensure that the live XML contains the 'tls' attribute. But make sure, that migration works properly for all combinations. I guess a another version would be better if you agree with exposing current state into live XML. Pavel

...

- if chardev_tls is not set and tls attribute is not configured in the XML the default after starting the domain should be tls=no

We're not getting anywhere if chardev_tls is not set.

John

[...]

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/14/2016 10:53 AM, Pavel Hrdina wrote:

On Fri, Oct 07, 2016 at 07:00:27AM -0400, John Ferlan wrote:

...

Add a new qemu.conf variables to store the UUID for the secret that could be used to present credentials to access the TLS chardev. Since this will be a server level and it's possible to use some sort of default, introduce both the default and chardev logic at the same time making the setting of the chardev check for it's own value, then if not present checking whether the default value had been set.

The chardevTLSx509haveUUID bool will be used as the marker for whether the chardevTLSx509secretUUID was successfully read. In the future this is how it'd determined whether to add the secret object for a TLS object.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 2 ++ src/qemu/qemu.conf | 24 ++++++++++++++++++++++++ src/qemu/qemu_conf.c | 22 ++++++++++++++++++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 53 insertions(+)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 988201e..73ebeda 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -29,6 +29,7 @@ module Libvirtd_qemu = (* Config entry grouped by function - same order as example config *) let default_tls_entry = str_entry "default_tls_x509_cert_dir" | bool_entry "default_tls_x509_verify" + | str_entry "default_tls_x509_secret_uuid"

let vnc_entry = str_entry "vnc_listen" | bool_entry "vnc_auto_unix_socket" @@ -51,6 +52,7 @@ module Libvirtd_qemu = let chardev_entry = bool_entry "chardev_tls" | str_entry "chardev_tls_x509_cert_dir" | bool_entry "chardev_tls_x509_verify" + | str_entry "chardev_tls_x509_secret_uuid"

let nogfx_entry = bool_entry "nographics_allow_host_audio"

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e4c2aae..f136fa9 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -28,6 +28,20 @@ # #default_tls_x509_verify = 1

+# +# Libvirt assumes the server-key.pem file is unencrypted by default. +# To use an encrypted server-key.pem file, the password to decrypt the

s/ the//

OK

...

+# the PEM file is requird. This can be provided by creating a secret

Fixed required too..

...

+# object in libvirt and then uncommenting this setting to set the UUID

changed uncommenting to "to uncomment"

...

+# UUID of the secret. +# +# NB This default all-zeros UUID will not work. Replace it with the +# output from the UUID for the TLS secret from a 'virsh secret-list' +# command and then uncomment the entry +# +#default_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" + + # VNC is configured to listen on 127.0.0.1 by default. # To make it listen on all public interfaces, uncomment # this next option. @@ -214,6 +228,16 @@ #chardev_tls_x509_verify = 1

+# Uncomment and use the following option to override the default secret +# uuid provided in the default_tls_x509_secret_uuid parameter. +# +# NB This default all-zeros UUID will not work. Replace it with the +# output from the UUID for the TLS secret from a 'virsh secret-list' +# command and then uncomment the entry +# +#chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" + + # By default, if no graphical front end is configured, libvirt will disable # QEMU audio output since directly talking to alsa/pulseaudio may not work # with various security settings. If you know what you're doing, enable diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 635fa27..c650961 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -365,6 +365,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->nvramDir);

VIR_FREE(cfg->defaultTLSx509certdir); + VIR_FREE(cfg->defaultTLSx509secretUUID);

VIR_FREE(cfg->vncTLSx509certdir); VIR_FREE(cfg->vncListen); @@ -377,6 +378,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->spiceSASLdir);

VIR_FREE(cfg->chardevTLSx509certdir); + VIR_FREE(cfg->chardevTLSx509secretUUID);

while (cfg->nhugetlbfs) { cfg->nhugetlbfs--; @@ -424,6 +426,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, int ret = -1; int rv; size_t i, j; + bool defaultTLSx509haveUUID; char *stdioHandler = NULL; char *user = NULL, *group = NULL; char **controllers = NULL; @@ -446,6 +449,12 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) goto cleanup; + if ((rv = virConfGetValueString(conf, "default_tls_x509_secret_uuid", + &cfg->defaultTLSx509secretUUID)) < 0) + goto cleanup; + if (rv == 1) + defaultTLSx509haveUUID = true;

I don't see any reason to use the extra bool variable. Function virConfGetValueString() has this logic:

-1 - we error out 0 - string was not found, value is set to NULL 1 - string was found, value is set

IOW you can just use defaultTLSx509secretUUID to detect whether there is something configured or not. The same applies to chardevTLSx509haveUUID.

Well it's been so long I have no idea what I was thinking, but yeah the booleans are unnecessary. I'll remove them.

...

+ if (virConfGetValueBool(conf, "vnc_auto_unix_socket", &cfg->vncAutoUnixSocket) < 0) goto cleanup; if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0) @@ -513,6 +522,19 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (rv == 0) cfg->chardevTLSx509verify = cfg->defaultTLSx509verify; + if ((rv = virConfGetValueString(conf, "chardev_tls_x509_secret_uuid", + &cfg->chardevTLSx509secretUUID)) < 0) + goto cleanup; + if (rv == 1) { + cfg->chardevTLSx509haveUUID = true; + } else { + if (defaultTLSx509haveUUID) { + if (VIR_STRDUP(cfg->chardevTLSx509secretUUID, + cfg->defaultTLSx509secretUUID) < 0) + goto cleanup; + cfg->chardevTLSx509haveUUID = true; + }

Copying the defaultTLSx509secretUUID should be done only in case rv == 0, otherwise we need to call goto cleanup;

This is changed to: if (!cfg->chardevTLSx509secretUUID && cfg->defaultTLSx509secretUUID) { IOW: We didn't find a chardev specific UUID, but do have a default one, so copy it. The setting affects patch 4 as well (since chardevTLSx509haveUUID would be removed). Tks - John [...]