8:54 a.m.
On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
> Add an optional "tls='yes|no'" attribute for a TCP chardev for the
> express purpose to disable setting up TLS for the specific chardev in
> the event the qemu.conf settings have enabled hypervisor wide TLS for
> serial TCP chardevs.
>
> Signed-off-by: John Ferlan <jferlan(a)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 +-
> ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++
> ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++
> .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml | 2 +-
> tests/qemuxml2argvtest.c | 3 ++
> ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.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-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..6145d65 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6204,6 +6204,27 @@ 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 TLS is controlled by the
> + <code>chardev_tls</code> setting in file /etc/libvirt/qemu.conf.
If
> + enabled, then by setting this attribute to "no" will disable usage
> + of the TLS environment for this particular serial TCP data channel.
> + </p>
Previous discussion:
http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
So now there is no functional issue if we agree that this design is what we
want. I personally thing that there could be also use-case where you want to
configure certificates in qemu.conf and use 'tls' attribute to explicitly
enable TLS encryption only for some VMs and only for some chardev and this is
not currently possible whit this design. Now user can enable the TLS encryption
for every chardev for every domain and explicitly disable for some chardevs.
This would require to add all the extra code from that discussion to handle
migration properly and that's why I've started the discussion.
w/r/t: design - re-read what I pulled from Dan's v5 review:
http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
While forcing to set "tls='yes'" is certainly a mechanism that could be
used and adding extra code is possible, is that something that we want
to require of everyone that's using TLS chardev's? If you go through the
trouble of configuring for your host, then how would you feel about
having to update all your domains (for those that have more than 1 or 2)
to set the property in order to be able to use it? Again, I really don't
think it's a good idea to be mucking around with changing XML of running
domains, adding extra checks in the migration code, altering the
format/parse code to check flags, and/or anything else that may come up.
The less we do that the better - introduces less risk for making or
missing assumptions.
For comparison spice uses an attribute mechanism "tlsPort={-1|0|port}"
in order to use TLS, but it also checks cfg->spiceTLS. Alternatively,
vnc will just take the cfg->vncTLS to decide whether to use or not
(e.g., there's no way to avoid).
So one of the existing mechanisms forces you to define something to use
TLS configured for the host and the other doesn't. Spice has the
additional configuration option to determine specific channels by name
that would use the secure port.
This patch can still be considered "outside" of the subsequent 4 in this
series...
> +<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 93b34e0..9062544 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:
> @@ -10038,6 +10040,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> char *master = NULL;
> char *slave = NULL;
> char *append = NULL;
> + char *haveTLS = NULL;
> int remaining = 0;
>
> while (cur != NULL) {
> @@ -10045,6 +10048,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:
> @@ -10221,6 +10226,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 =
> @@ -10305,6 +10319,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> VIR_FREE(append);
> VIR_FREE(logappend);
> VIR_FREE(logfile);
> + VIR_FREE(haveTLS);
>
> return remaining;
>
> @@ -21453,7 +21468,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 3e85ae4..c34e046 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 21fd85c..aaf7018 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4939,7 +4939,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
> if (dev->data.tcp.listen)
> virBufferAdd(&buf, nowait ? ",server,nowait" :
",server", -1);
>
> - if (cfg->chardevTLS) {
> + if (cfg->chardevTLS &&
> + dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) {
> char *objalias = NULL;
>
> if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 14af4e1..f4d7c17 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1729,7 +1729,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_NO) {
> if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir,
> dev->data.tcp.listen,
> cfg->chardevTLSx509verify,
> 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/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'/>
This is not required and should work without the attribute.
True - I can remove the " tls='yes'", the test though is still valid to
ensure we build the command line properly
John
Pavel
> <protocol type='raw'/>
> <target port='0'/>
> </serial>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e8438b4..c65c769 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1162,6 +1162,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-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/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 62bff8c..3ab4c50 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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list