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@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.

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

On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:

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@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

I have a feeling that we are having trouble to understand each other. I'm not saying that you will have to set "tls='yes'" for every domain and every chardev. Forcing to set "tls='no'" for every domain and every chardevs if you want to use TLS only for one domain is not a good mechanism. What I would like to achieve is this: 1. Currently there is already existing "chardev_tls" config option that allows you to enable/disable TLS for all domain and all chardevs 2. This patch improves the current functionality by adding an option to explicitly disable TLS for some chardves by using "tls='no'" if "chardev_tls" is set to "1". 3. My additional proposal is to add another improvement to current functionality by allowing to explicitly enable TLS for some chardevs by using "tls='yes'" if "chardev_tls" is set to "0". And I can see the use-case, you have a shared host between several people and only one of them would like to use TLS encryption for his domain and not affect other users so he will configure certificates for TLS encryption and use it explicitly only for his domain.

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.

This is not a question whether it's a good idea or not. We simply have to do that in order to generate backward compatible XML. Yes, it adds extra code and extra checks but it's a price we have to pay to maintain backward compatibility.

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).

Yes, SPICE uses tlsPort and spiceTLS config option, but the spice is different because the tlsPort is separate and there is still non encrypted port. The VNC is not a good example because it has only the vncTLS config option and there is not attribute in XML to configure it (the current state for chardev before this patch changes it).

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...

Yes it could be a separate patch outside of this series but I would rather see it as part of this series. Pavel

On Mon, Oct 17, 2016 at 11:24:58AM -0400, John Ferlan wrote:

On 10/17/2016 10:37 AM, Pavel Hrdina wrote:

...

On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:

...

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@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

I have a feeling that we are having trouble to understand each other. I'm not saying that you will have to set "tls='yes'" for every domain and every chardev. Forcing to set "tls='no'" for every domain and every chardevs if you want to use TLS only for one domain is not a good mechanism.

Clearly ;-) Setting "tls='yes'" wasn't my implication either. The way this patch is written 'YES' or 'ABSENT' have the same meaning to take the host default.

So "forcing" setting of "tls='no'"

...

What I would like to achieve is this:

1. Currently there is already existing "chardev_tls" config option that allows you to enable/disable TLS for all domain and all chardevs

2. This patch improves the current functionality by adding an option to explicitly disable TLS for some chardves by using "tls='no'" if "chardev_tls" is set to "1".

3. My additional proposal is to add another improvement to current functionality by allowing to explicitly enable TLS for some chardevs by using "tls='yes'" if "chardev_tls" is set to "0". And I can see the use-case, you have a shared host between several people and only one of them would like to use TLS encryption for his domain and not affect other users so he will configure certificates for TLS encryption and use it explicitly only for his domain.

Feel free to write a competing patch... This patch as written I believe more closely follows the request from Daniel from patch 5 review:

"As default behaviour I think it is desirable that we can turn TLS on for every VM at once - I tend to view it as a host network integration task, rather than a VM configuration task. Same rationale that we use for TLS wth VNC/SPICE."

Don't forget this part of the same review: "There's no reason we can't have a tri-state TLS flag against the chardev in the XML too, to override the default behaviour of cfg->chardevTLS" That also means to override chardev_tls = "0" by "tls='yes'". Pavel

...

...

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.

This is not a question whether it's a good idea or not. We simply have to do that in order to generate backward compatible XML. Yes, it adds extra code and extra checks but it's a price we have to pay to maintain backward compatibility.

But unnecessary when using "tls='no'"

...

...

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).

Yes, SPICE uses tlsPort and spiceTLS config option, but the spice is different because the tlsPort is separate and there is still non encrypted port. The VNC is not a good example because it has only the vncTLS config option and there is not attribute in XML to configure it (the current state for chardev before this patch changes it).

...

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...

Yes it could be a separate patch outside of this series but I would rather see it as part of this series.

Other than the pointed out spots that need a haveTLS in patch 4 and 5, the other patches are different.

John

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

On Fri, Oct 14, 2016 at 04:23:05PM -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.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/libvirtd_qemu.aug | 2 ++ src/qemu/qemu.conf | 24 ++++++++++++++++++++++++ src/qemu/qemu_conf.c | 14 ++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 44 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..493c171 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

You've forgot to remove the extra "the".

+# the PEM file is required. This can be provided by creating a secret +# object in libvirt and then to uncomment this setting to set the 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.

s/uuid/UUID/ ACK

+# +# 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..109668b 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--; @@ -446,6 +448,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) goto cleanup; + if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", + &cfg->defaultTLSx509secretUUID) < 0) + goto cleanup; + if (virConfGetValueBool(conf, "vnc_auto_unix_socket", &cfg->vncAutoUnixSocket) < 0) goto cleanup; if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0) @@ -513,6 +519,14 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (rv == 0) cfg->chardevTLSx509verify = cfg->defaultTLSx509verify; + if (virConfGetValueString(conf, "chardev_tls_x509_secret_uuid", + &cfg->chardevTLSx509secretUUID) < 0) + goto cleanup; + if (!cfg->chardevTLSx509secretUUID && cfg->defaultTLSx509secretUUID) { + if (VIR_STRDUP(cfg->chardevTLSx509secretUUID, + cfg->defaultTLSx509secretUUID) < 0) + goto cleanup; + }

if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) goto cleanup; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index d32689a..12b2661 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -111,6 +111,7 @@ struct _virQEMUDriverConfig {

char *defaultTLSx509certdir; bool defaultTLSx509verify; + char *defaultTLSx509secretUUID;

bool vncAutoUnixSocket; bool vncTLS; @@ -132,6 +133,7 @@ struct _virQEMUDriverConfig { bool chardevTLS; char *chardevTLSx509certdir; bool chardevTLSx509verify; + char *chardevTLSx509secretUUID;

unsigned int remotePortMin; unsigned int remotePortMax; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index cd162ae..805fa0e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -4,6 +4,7 @@ module Test_libvirtd_qemu = test Libvirtd_qemu.lns get conf = { "default_tls_x509_cert_dir" = "/etc/pki/qemu" } { "default_tls_x509_verify" = "1" } +{ "default_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } { "vnc_listen" = "0.0.0.0" } { "vnc_auto_unix_socket" = "1" } { "vnc_tls" = "1" } @@ -23,6 +24,7 @@ module Test_libvirtd_qemu = { "chardev_tls" = "1" } { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } +{ "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } { "nographics_allow_host_audio" = "1" } { "remote_display_port_min" = "5900" } { "remote_display_port_max" = "65535" } -- 2.7.4

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

On 10/17/2016 10:11 AM, Pavel Hrdina wrote:

On Fri, Oct 14, 2016 at 04:23:07PM -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 | 32 ++++++- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 99 +++++++++++++++++++++- src/qemu/qemu_domain.h | 16 +++- src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 6 +- ...xml2argv-serial-tcp-tlsx509-secret-chardev.args | 38 +++++++++ ...uxml2argv-serial-tcp-tlsx509-secret-chardev.xml | 50 +++++++++++ tests/qemuxml2argvtest.c | 18 ++++ 9 files changed, 252 insertions(+), 9 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 aaf7018..b2dfee0 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; }

@@ -4946,6 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, + !!cfg->chardevTLSx509secretUUID, alias, qemuCaps) < 0) goto error;

@@ -8542,6 +8557,19 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager,

/* Use -chardev with -device if they are available */ if (virQEMUCapsSupportsChardev(def, qemuCaps, serial)) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(serial); + qemuDomainSecretInfoPtr secinfo = chardevPriv->secinfo; + + /* Add the secret object first if necessary. The + * secinfo is added only to a TCP serial device during + * qemuDomainSecretChardevPrepare. Subsequent called + * functions can just check the config fields */ + if (cfg->chardevTLS && cfg->chardevTLSx509secretUUID && + serial->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&

This needs to test also serial->source.data.tcp.haveTLS.

True, of course that depends on the order of the patches too. So if these 4 get accepted, but 1/5 is not accepted, then this would have to move to patch 1 and be handled with that.

...

+ qemuBuildObjectSecretCommandLine(cmd, 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 a07f981..02c67ae 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) @@ -1223,6 +1224,91 @@ 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 serial 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; + + if (!conn || chardev->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL ||

The check for !conn is redundant because the declaration uses ATTRIBUTE_NONNULL for the first attribute.

OK... It's cut-n-paste of Disk and Hostdev Prepare which should also change (separate patch). I actually have quite a few of these built up from a Coverity build which actually checks; whereas, the normal build doesn't. I'll adjust this one and send the other ones separately.

...

+ chardev->source.type != VIR_DOMAIN_CHR_TYPE_TCP) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + if (cfg->chardevTLS && 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 (qemuDomainSecretSetup(conn, priv, secinfo, chardev->info.alias, + 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; + } + + virObjectUnref(cfg); + return 0; + + error: + virObjectUnref(cfg); + qemuDomainSecretInfoFree(&secinfo); + return -1; +} + + /* qemuDomainSecretDestroy: * @vm: Domain object * @@ -1239,11 +1325,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 @@ -1256,6 +1346,7 @@ qemuDomainSecretDestroy(virDomainObjPtr vm) */ int qemuDomainSecretPrepare(virConnectPtr conn, + virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -1272,6 +1363,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 707a995..cc063d1 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 f4d7c17..aad7fa1 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 0f5a11b..643741d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5092,7 +5092,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, size_t i; char *nodeset = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);

The cfg cleanup could be moved to separate patch.

OK.

...

virCapsPtr caps;

if (!(caps = virQEMUDriverGetCapabilities(driver, false))) @@ -5158,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++) { @@ -5188,7 +5187,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, cleanup: VIR_FREE(nodeset); virObjectUnref(caps); - virObjectUnref(cfg); return ret; }

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..5859c74 --- /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=serial1-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +endpoint=client,verify-peer=yes,passwordid=serial1-secret0 \ +-chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ +tls-creds=objserial1_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 c65c769..1ca91d3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1165,6 +1165,24 @@ 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; + driver.config->chardevTLSx509verify = 1;

Setting the chardevTLSx509verify is not strictly required to test the generated command line if secret is required. The verify feature should has its own test case.

OK, right... Should be a separate patch, although it's almost a two-fer ;-)... Making this change alters the .args file to have 'verify-peer=no' instead of verify-peer=yes. Tks - John

Pavel

...

+ 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

On Fri, Oct 14, 2016 at 04:23:08PM -0400, John Ferlan wrote:

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 | 62 +++++++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_hotplug.h | 3 ++- tests/qemuhotplugtest.c | 2 +- 4 files changed, 61 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 aad7fa1..69d562f 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,30 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup;

+ if (qemuDomainSecretChardevPrepare(conn, driver, priv, chr) < 0) + goto cleanup; + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) { + qemuDomainChardevPrivatePtr chardevPriv = + QEMU_DOMAIN_CHARDEV_PRIVATE(chr); + qemuDomainSecretInfoPtr secinfo = chardevPriv->secinfo; + + /* Add a secret object in order to access the TLS environment. + * The secinfo will only be created for serial TCP device. */ + if (secinfo) { + if (qemuBuildSecretInfoProps(secinfo, &secProps) < 0) + goto cleanup; + + if (!(secAlias = qemuDomainGetSecretAESAlias(chr->info.alias, + false))) + goto cleanup; + } + if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - NULL, + secAlias, priv->qemuCaps, &tlsProps) < 0) goto cleanup; @@ -1745,6 +1767,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 +1806,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 +1815,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 +4422,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *objAlias = NULL; + char *secAlias = NULL; char *devstr = NULL;

if (!(tmpChr = virDomainChrFind(vmdef, chr))) { @@ -4400,9 +4436,21 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,

sa_assert(tmpChr->info.alias);

- if (cfg->chardevTLS && - !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) - goto cleanup; + if (cfg->chardevTLS) {

This needs to test serial->source.data.tcp.haveTLS. Pavel

+ if (!(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) + 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 && + tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && + tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && + !(secAlias = qemuDomainGetSecretAESAlias(tmpChr->info.alias, + false))) + goto cleanup; + }

if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; @@ -4416,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

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