On Thu, Oct 20, 2016 at 10:28:04AM -0400, John Ferlan wrote: > > > On 10/20/2016 02:51 AM, Pavel Hrdina wrote: > > On Wed, Oct 19, 2016 at 04:53:54PM -0400, John Ferlan wrote: > >> Add an optional "tls='yes|no'" attribute for a TCP chardev. > >> > >> For QEMU, this will allow for disabling the host config setting of the > >> 'chardev_tls' for a domain chardev channel by setting the value to "no" or > >> to attempt to use a host TLS environment when setting the value to "yes" > >> when the host config 'chardev_tls' setting is disabled, but a TLS environment > >> is configured via either the host config 'chardev_tls_x509_cert_dir' or > >> 'default_tls_x509_cert_dir' > >> > >> Alter qemuDomainSupportTLSChardevTCP to augment the decision points for > >> choosing whether to try to use TLS. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> docs/formatdomain.html.in | 28 ++++++++++++ > >> docs/schemas/domaincommon.rng | 5 +++ > >> src/conf/domain_conf.c | 22 +++++++++- > >> src/conf/domain_conf.h | 1 + > >> src/qemu/qemu_command.c | 2 +- > >> src/qemu/qemu_domain.c | 20 +++++++-- > >> src/qemu/qemu_domain.h | 3 +- > >> src/qemu/qemu_hotplug.c | 4 +- > >> ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++ > >> ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++ > >> tests/qemuxml2argvtest.c | 3 ++ > >> ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 + > >> tests/qemuxml2xmltest.c | 1 + > >> 13 files changed, 162 insertions(+), 8 deletions(-) > >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args > >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml > >> create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml > >> > >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > >> index 9051178..da6be67 100644 > >> --- a/docs/formatdomain.html.in > >> +++ b/docs/formatdomain.html.in > >> @@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null > >> </devices> > >> ...</pre> > >> > >> + <p> > >> + <span class="since">Since 2.4.0,</span> the optional attribute > >> + <code>tls</code> can be used to control whether a serial chardev > >> + TCP communication channel would utilize a hypervisor configured > >> + TLS X.509 certificate environment in order to encrypt the data > >> + channel. For the QEMU hypervisor, usage of a TLS envronment can > >> + be controlled on the host by the <code>chardev_tls</code> and > >> + <code>chardev_tls_x509_cert_dir</code> or > >> + <code>default_tls_x509_cert_dir</code> settings in the file > >> + /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled, > >> + then unless the <code>tls</code> attribute is set to "no", libvirt > >> + will use the host configured TLS environment. > >> + If <code>chardev_tls</code> is disabled, but the <code>tls</code> > >> + attribute is set to "yes", then libvirt will attempt to use the > >> + host TLS environment if either the <code>chardev_tls_x509_cert_dir</code> > >> + or <code>default_tls_x509_cert_dir</code> TLS directory structure exists. > >> + </p> > > > > Nice, this is a good description how to use the *tls* attribute. > > > > BTW (regarding your followup reply): > > The 4 "consumers" of virDomainChrSourceDefParseXML (where this would be > parsed) refer to this as a "serial chardev" This is a generic function that parses source for a lot of different device types/ > The 'virDomainChrDefParseXML' comments have a list of "<serial ..." XML > types. The location of the above description is describing a <serial > type="tcp"> definition. Well, the comment have that list but is used to parse all character devices, not only serial char device. TLS encryption can be used also for those types: parallel, channel, and console. > The 'smartcard' discussion for a 'passthrough' device that would use > this code says "Rather than having the hypervisor directly communicate > with the host, it is possible to tunnel all requests through a secondary > character device to a third-party provider (which may in turn be talking > to a smartcard or using three certificate files). In this mode of > operation, an additional attribute type is required, matching one of the > supported serial device types, to describe the host side of the tunnel;..." This comment is wrong, it should be "supported character device types". This attribute tells what interface is presented to the host. Check this part of documentation for all character devices: <http://libvirt.org/formatdomain.html#elementsConsole> there is this sentence: "The interface presented to the host is given in the type attribute of the top-level element. The host interface is configured by the source element." So it refers to all host interfaces: <http://libvirt.org/formatdomain.html#elementsCharHostInterface> > The 'rng' discussion for backend that would use this code says "This > backend connects to a source using the EGD protocol. The source is > specified as a character device. Refer to character device host > interface for more information. ..." This is correct, there is no reference to serial character device. > Redevdir says "An additional attribute type is required, matching one of > the supported serial device types, to describe the host side of the > tunnel; type='tcp' or type='spicevmc' ..." This is the same case as smartcard device, this is wrong. > So the long and short of it is, IMO it's a serial chardev device. > Semantically it could be claimed otherwise, but the parsing proves > otherwise as does the existing documentation of "Host interface" > character devices. > > I prefer to keep it described as is. It's only ever used, parsed, etc. > when <devices>... <serial type="tcp">... <source mode='connect'..." > > If anything, the description should become more restrictive to indicate > that the option shouldn't be used for smartcards, rngs, and redirdevs, > but I'll save that discussion for patch 3. Based on the documentation it may appear that it should be a serial chardev, but that's misleading and it should refer only to the "host interfaces of character devices". > >> +<pre> > >> + ... > >> + <devices> > >> + <serial type="tcp"> > >> + <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> > >> + <protocol type="raw"/> > >> + <target port="0"/> > >> + </serial> > >> + </devices> > >> + ...</pre> > >> + > >> <h6><a name="elementsCharUDP">UDP network console</a></h6> > >> > >> <p> > >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > >> index 3106510..e6741bb 100644 > >> --- a/docs/schemas/domaincommon.rng > >> +++ b/docs/schemas/domaincommon.rng > >> @@ -3453,6 +3453,11 @@ > >> <ref name="virOnOff"/> > >> </attribute> > >> </optional> > >> + <optional> > >> + <attribute name="tls"> > >> + <ref name="virYesNo"/> > >> + </attribute> > >> + </optional> > >> <zeroOrMore> > >> <ref name='devSeclabel'/> > >> </zeroOrMore> > >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > >> index 89473db..e4fa9ad 100644 > >> --- a/src/conf/domain_conf.c > >> +++ b/src/conf/domain_conf.c > >> @@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, > >> > >> if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0) > >> return -1; > >> + > >> + dest->data.tcp.haveTLS = src->data.tcp.haveTLS; > >> break; > >> > >> case VIR_DOMAIN_CHR_TYPE_UNIX: > >> @@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > >> char *master = NULL; > >> char *slave = NULL; > >> char *append = NULL; > >> + char *haveTLS = NULL; > >> int remaining = 0; > >> > >> while (cur != NULL) { > >> @@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > >> if (xmlStrEqual(cur->name, BAD_CAST "source")) { > >> if (!mode) > >> mode = virXMLPropString(cur, "mode"); > >> + if (!haveTLS) > >> + haveTLS = virXMLPropString(cur, "tls"); > >> > >> switch ((virDomainChrType) def->type) { > >> case VIR_DOMAIN_CHR_TYPE_FILE: > >> @@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > >> def->data.tcp.listen = true; > >> } > >> > >> + if (haveTLS && > >> + (def->data.tcp.haveTLS = > >> + virTristateBoolTypeFromString(haveTLS)) <= 0) { > >> + virReportError(VIR_ERR_XML_ERROR, > >> + _("unknown chardev 'tls' setting '%s'"), > >> + haveTLS); > >> + goto error; > >> + } > >> + > >> if (!protocol) > >> def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW; > >> else if ((def->data.tcp.protocol = > >> @@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, > >> VIR_FREE(append); > >> VIR_FREE(logappend); > >> VIR_FREE(logfile); > >> + VIR_FREE(haveTLS); > >> > >> return remaining; > >> > >> @@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf, > >> virBufferAsprintf(buf, "<source mode='%s' ", > >> def->data.tcp.listen ? "bind" : "connect"); > >> virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host); > >> - virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service); > >> + virBufferEscapeString(buf, "service='%s'", def->data.tcp.service); > >> + if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT) > >> + virBufferAsprintf(buf, " tls='%s'", > >> + virTristateBoolTypeToString(def->data.tcp.haveTLS)); > >> + virBufferAddLit(buf, "/>\n"); > >> + > >> virBufferAsprintf(buf, "<protocol type='%s'/>\n", > >> virDomainChrTcpProtocolTypeToString( > >> def->data.tcp.protocol)); > >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > >> index 04f2e40..fcadf6c 100644 > >> --- a/src/conf/domain_conf.h > >> +++ b/src/conf/domain_conf.h > >> @@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef { > >> bool listen; > >> int protocol; > >> bool tlscreds; > >> + int haveTLS; /* enum virTristateBool */ > >> } tcp; > >> struct { > >> char *bindHost; > >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > >> index d45a7de..f00751a 100644 > >> --- a/src/qemu/qemu_command.c > >> +++ b/src/qemu/qemu_command.c > >> @@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, > >> if (dev->data.tcp.listen) > >> virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); > >> > >> - if (qemuDomainSupportTLSChardevTCP(cfg)) { > >> + if (qemuDomainSupportTLSChardevTCP(cfg, dev)) { > >> char *objalias = NULL; > >> > >> if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, > >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > >> index 746d94f..7b518c6 100644 > >> --- a/src/qemu/qemu_domain.c > >> +++ b/src/qemu/qemu_domain.c > >> @@ -6302,15 +6302,29 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video, > >> > >> /* qemuDomainSupportTLSChardevTCP > >> * @cfg: Pointer to driver cfg > >> + * @dev: Pointer to chardev source > >> * > >> - * Let's check if this host supports using the TLS environment for chardev. > >> + * Let's check if this host and/or domain supports or desires to use > >> + * the TLS environment for the passed chardev TCP. > >> + * > >> + * If we have an environment and as long as the domain config doesn't have > >> + * the "tls='no'" property, then we assume it's desired. > >> + * > >> + * If the host global isn't set, but the domain chardev config is requesting > >> + * to use TLS and we find what appears to be some environment configured, > >> + * then let's also try. This action could fail later in QEMU if the environment > >> + * isn't set up to the exact specifications. > >> * > >> * Returns true if we want to use TLS, false otherwise. > >> */ > >> bool > >> -qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg) > >> +qemuDomainSupportTLSChardevTCP(virQEMUDriverConfigPtr cfg, > >> + const virDomainChrSourceDef *dev) > >> { > >> - if (cfg->chardevTLS) > >> + if (cfg->chardevTLS && dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) > >> + return true; > >> + if (!cfg->chardevTLS && dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && > >> + virFileExists(cfg->chardevTLSx509certdir)) > >> return true; > >> return false; > >> } > > > > So this function let's you decide whether we should try to set up *tls* for > > chardev or not. It work's but I have few issues with it. > > > > At first I don't like that libvirt would try to do something smart and don't > > even tell user about the result. This will silently ignore the *tls* > > attribute if no certificate is found. In case that *tls* attribute is set > > to "yes" in XML and there is no certificate file to use we shouldn't start > > that domain and print an error to user. > > This is a boolean function - so printing an error here isn't right. Well, my comment implies to not use boolean function. > Adding something to post parse processing is possible, but there's an > impact based on whether we're setting a default value for haveTLS > (something I disagree with doing). > > Beyond that would a check go in qemuDomainDeviceDefPostParse or > qemuDomainDeviceDefValidate? It's never crystal clear to me which should > be used when just reading the code. Although since I see this as a "new" > and "optional" value, I'd lean toward PostParse. Then there's dealing > with the parseFlags that it's impacted by other decisions. > > I could also rationalize that someone adding "tls='yes'" to their > chardev would "know" what they're doing because they read the > documentation. How else would they know to have this very specific > combination (unless of course 'something' set things up that way based > on the assumption of how a domain is currently running). > > Again, IMO 'haveTLS' is new and optional. The only indication that a > domain is using TLS was handled via 'tlscreds'. IIRC, the "reason" that > 'tlscreds' exists and how "tls-creds" gets added to the command line is > because the JSON code processing for a chardev is buried in > qemu_monitor_json.c and fishing for a host configuration option at that > time wasn't viable. > > > > > Secondly this way we don't reflect the current state for live domain in the > > live XML. This was probably lost during the discussion, but in general if > > there is an attribute that can affect running domain we should reflect the > > current state using that attribute. I know, there are some cases where we > > probably don't do that and they should be fixed. > > > > No it wasn't lost - I considered it as I see you've seen from the cover. > And you're probably right regarding attributes that trigger usage of > some qemu option that we don't specifically save in the status XML. > That's a different rat hole. > > > I figure out that we cannot simply use haveTLS = cfg->chardevTLS but we > > can set the haveTLS based on cfg->chardevTLS. The whole purpose of > > qemuProcessPrepareDomain() is to prepare the domain definition so the > > qemuBuildCommandLine() don't have to check other places to enable some > > feature and not update the live definition. If the *tls* attribute is > > properly set in live definition that it will be saved to status XML and > > there is no need to do anything for qemuProcessReconnect. > > I disagree on setting haveTLS during qemuProcessReconnect based upon > chardevTLS. The 'haveTLS' is an optional attribute and by setting a > value I believe we end up making an assumption. I wrote that there is no need to do anything for qemuProcessReconnect. > If *anything* was to be done it would be based solely on whether > "tls-creds" is set on the command line of the reconnected domain. > However, that too has a similar problem about setting a value for an > optional attribute based upon the assumption that we know better. > > Again, the only indication that 'tls-creds' is on the command line was > from the 'tlscreds' boolean that was set because the host configuration > information was available. A domain that is running and has tls-creds > will continue to have it. Altering that domain's configuration file > because we add a new optional *configuration* value has no bearing on > the *status* XML. When/if a configuration XML is updated, it's not > checking that 'tlscreds' value to determine that at some point in > history the domain used TLS because the host was configured that way. The whole point of having 'tls' attribute in live XML is to ensure that when libvirtd is restarted the attribute is still present in the XML because it will be saved to status XML and loaded again from status XML. There is no need to do any magic by parsing qemu command line, we have statu XML for that purpose to store all information about domain. > Let's consider the bool function from above and this automagic setting > being requested. Let's say we reconnect to a domain, find the > 'tls-creds' set on the command line, and set 'haveTLS=YES' based solely > on that. Let's say at some point in time after, someone edits their > qemu.conf file and sets 'chardev_tls=0' (or comments out the > 'chardev_tls=1'). In their mind, they've now disabled chardev TLS for > their host and any domain they will run in the future. They stop the > domain they knew was running using TLS before and restart it expecting > that it won't use TLS anymore, but on restart they discover that in our > infinite wisdom we have set the optional "tls='yes'" property for that > chardev on that domain. Now if we 'error' out on that start like you > request above, then that means they will have to edit their domain and > remove the seemingly optional property. > > Next, let's assume they read the documentation and found that they can > disable the qemu.conf value, but still have the domain chardev value if > they set the "tls='yes'" property as long as they have their valid TLS > directory configuration. In this case, they have made the conscious > decision how they want their domain configured based on what they know > is configured on the host. > > TBH: I take this single patch as a "feature request" add-on to the > original feature request that I believe in the long run won't be used. I > could be wrong, but it's a feeling. > > Furthermore, the purpose of any optional attribute is just that. It's > optional based on some host wide setting. It's up to the consumer to > decide how they should proceed, not the software to make that decision > for them. I guess that I'm unable to describe exactly what I mean so I'm attaching two patches, one is for introducing TLS attribute and the second one is to make sure that migration to libvirt-2.3.0 will work. Pavel
From 7b70eeb03f4fe190d78f50e72a1b0e5264af5299 Mon Sep 17 00:00:00 2001
Message-Id: <7b70eeb03f4fe190d78f50e72a1b0e5264af5299.1476980707.git.phrdina@xxxxxxxxxx>
From: Pavel Hrdina <phrdina@xxxxxxxxxx>
Date: Thu, 20 Oct 2016 17:07:05 +0200
Subject: [PATCH 1/4] domain: Add optional 'tls' attribute for TCP chardev
From: John Ferlan <jferlan@xxxxxxxxxx>
Add an optional "tls='yes|no'" attribute for a TCP chardev.
For QEMU, this will allow for disabling the host config setting of the
'chardev_tls' for a domain chardev channel by setting the value to "no" or
to attempt to use a host TLS environment when setting the value to "yes"
when the host config 'chardev_tls' setting is disabled, but a TLS environment
is configured via either the host config 'chardev_tls_x509_cert_dir' or
'default_tls_x509_cert_dir'
Alter qemuDomainSupportTLSChardevTCP to augment the decision points for
choosing whether to try to use TLS.
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
---
docs/formatdomain.html.in | 28 ++++++++++++
docs/schemas/domaincommon.rng | 5 +++
src/conf/domain_conf.c | 22 +++++++++-
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_hotplug.c | 4 +-
src/qemu/qemu_process.c | 29 +++++++++++++
...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++
...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 3 ++
...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml | 1 +
tests/qemuxml2xmltest.c | 1 +
12 files changed, 172 insertions(+), 4 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..3ed4769 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6204,6 +6204,34 @@ qemu-kvm -net nic,model=? /dev/null
</devices>
...</pre>
+ <p>
+ <span class="since">Since 2.4.0,</span> the optional attribute
+ <code>tls</code> can be used to control whether a chardev
+ TCP communication channel would utilize a hypervisor configured
+ TLS X.509 certificate environment in order to encrypt the data
+ channel. For the QEMU hypervisor, usage of a TLS envronment can
+ be controlled on the host by the <code>chardev_tls</code> and
+ <code>chardev_tls_x509_cert_dir</code> or
+ <code>default_tls_x509_cert_dir</code> settings in the file
+ /etc/libvirt/qemu.conf. If <code>chardev_tls</code> is enabled,
+ then unless the <code>tls</code> attribute is set to "no", libvirt
+ will use the host configured TLS environment.
+ If <code>chardev_tls</code> is disabled, but the <code>tls</code>
+ attribute is set to "yes", then libvirt will attempt to use the
+ host TLS environment if either the <code>chardev_tls_x509_cert_dir</code>
+ or <code>default_tls_x509_cert_dir</code> TLS directory structure exists.
+ </p>
+<pre>
+ ...
+ <devices>
+ <serial type="tcp">
+ <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/>
+ <protocol type="raw"/>
+ <target port="0"/>
+ </serial>
+ </devices>
+ ...</pre>
+
<h6><a name="elementsCharUDP">UDP network console</a></h6>
<p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3106510..e6741bb 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3453,6 +3453,11 @@
<ref name="virOnOff"/>
</attribute>
</optional>
+ <optional>
+ <attribute name="tls">
+ <ref name="virYesNo"/>
+ </attribute>
+ </optional>
<zeroOrMore>
<ref name='devSeclabel'/>
</zeroOrMore>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 89473db..e4fa9ad 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1997,6 +1997,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
if (VIR_STRDUP(dest->data.tcp.service, src->data.tcp.service) < 0)
return -1;
+
+ dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
@@ -10040,6 +10042,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
char *master = NULL;
char *slave = NULL;
char *append = NULL;
+ char *haveTLS = NULL;
int remaining = 0;
while (cur != NULL) {
@@ -10047,6 +10050,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
if (xmlStrEqual(cur->name, BAD_CAST "source")) {
if (!mode)
mode = virXMLPropString(cur, "mode");
+ if (!haveTLS)
+ haveTLS = virXMLPropString(cur, "tls");
switch ((virDomainChrType) def->type) {
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -10223,6 +10228,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
def->data.tcp.listen = true;
}
+ if (haveTLS &&
+ (def->data.tcp.haveTLS =
+ virTristateBoolTypeFromString(haveTLS)) <= 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("unknown chardev 'tls' setting '%s'"),
+ haveTLS);
+ goto error;
+ }
+
if (!protocol)
def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
else if ((def->data.tcp.protocol =
@@ -10307,6 +10321,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
VIR_FREE(append);
VIR_FREE(logappend);
VIR_FREE(logfile);
+ VIR_FREE(haveTLS);
return remaining;
@@ -21466,7 +21481,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<source mode='%s' ",
def->data.tcp.listen ? "bind" : "connect");
virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host);
- virBufferEscapeString(buf, "service='%s'/>\n", def->data.tcp.service);
+ virBufferEscapeString(buf, "service='%s'", def->data.tcp.service);
+ if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT)
+ virBufferAsprintf(buf, " tls='%s'",
+ virTristateBoolTypeToString(def->data.tcp.haveTLS));
+ virBufferAddLit(buf, "/>\n");
+
virBufferAsprintf(buf, "<protocol type='%s'/>\n",
virDomainChrTcpProtocolTypeToString(
def->data.tcp.protocol));
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 04f2e40..fcadf6c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1094,6 +1094,7 @@ struct _virDomainChrSourceDef {
bool listen;
int protocol;
bool tlscreds;
+ int haveTLS; /* enum virTristateBool */
} tcp;
struct {
char *bindHost;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8282162..189147c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4943,7 +4943,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
if (dev->data.tcp.listen)
virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1);
- if (cfg->chardevTLS) {
+ if (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 2cb2267..e2b3ad3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1730,7 +1730,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
goto cleanup;
if (dev->type == VIR_DOMAIN_CHR_TYPE_TCP &&
- cfg->chardevTLS) {
+ dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) {
if (qemuBuildTLSx509BackendProps(cfg->chardevTLSx509certdir,
dev->data.tcp.listen,
cfg->chardevTLSx509verify,
@@ -4404,7 +4404,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
goto cleanup;
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
- cfg->chardevTLS &&
+ tmpChr->source.data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
!(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d641f33..cd3d376 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5070,6 +5070,26 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,
}
+static int
+qemuProcessPrepareDomainChardevIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainChrDefPtr chr,
+ void *opaque)
+{
+ virQEMUDriverConfigPtr cfg = opaque;
+
+ if (chr->source.type == VIR_DOMAIN_CHR_TYPE_TCP) {
+ if (chr->source.data.tcp.haveTLS == VIR_TRISTATE_BOOL_ABSENT) {
+ if (cfg->chardevTLS)
+ chr->source.data.tcp.haveTLS = VIR_TRISTATE_BOOL_YES;
+ else
+ chr->source.data.tcp.haveTLS = VIR_TRISTATE_BOOL_NO;
+ }
+ }
+
+ return 0;
+}
+
+
/**
* qemuProcessPrepareDomain
*
@@ -5092,6 +5112,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
size_t i;
char *nodeset = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virCapsPtr caps;
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
@@ -5167,6 +5188,13 @@ qemuProcessPrepareDomain(virConnectPtr conn,
goto cleanup;
}
+ VIR_DEBUG("Add default 'tls' attribute for chardev devices if missing");
+ if (virDomainChrDefForeach(vm->def,
+ true,
+ qemuProcessPrepareDomainChardevIter,
+ cfg) < 0)
+ goto cleanup;
+
if (VIR_ALLOC(priv->monConfig) < 0)
goto cleanup;
@@ -5187,6 +5215,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
cleanup:
VIR_FREE(nodeset);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
new file mode 100644
index 0000000..cac0d85
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
@@ -0,0 +1,30 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefconfig \
+-nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\
+server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\
+localport=1111 \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-chardev socket,id=charserial1,host=127.0.0.1,port=5555 \
+-device isa-serial,chardev=charserial1,id=serial1 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
new file mode 100644
index 0000000..debc69b
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
@@ -0,0 +1,50 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='udp'>
+ <source mode='bind' host='127.0.0.1' service='1111'/>
+ <source mode='connect' host='127.0.0.1' service='2222'/>
+ <target port='0'/>
+ </serial>
+ <serial type='tcp'>
+ <source mode='connect' host='127.0.0.1' service='5555' tls='no'/>
+ <protocol type='raw'/>
+ <target port='0'/>
+ </serial>
+ <console type='udp'>
+ <source mode='bind' host='127.0.0.1' service='1111'/>
+ <source mode='connect' host='127.0.0.1' service='2222'/>
+ <target type='serial' port='0'/>
+ </console>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 3e9f825..52d85fa 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1167,6 +1167,9 @@ mymain(void)
QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_OBJECT_TLS_CREDS_X509);
driver.config->chardevTLSx509verify = 0;
+ DO_TEST("serial-tcp-tlsx509-chardev-notls",
+ QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
+ QEMU_CAPS_OBJECT_TLS_CREDS_X509);
driver.config->chardevTLS = 0;
VIR_FREE(driver.config->chardevTLSx509certdir);
DO_TEST("serial-many-chardev",
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
new file mode 120000
index 0000000..26484c9
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 95c0bf2..64da80a 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -534,6 +534,7 @@ mymain(void)
DO_TEST("serial-udp", NONE);
DO_TEST("serial-tcp-telnet", NONE);
DO_TEST("serial-tcp-tlsx509-chardev", NONE);
+ DO_TEST("serial-tcp-tlsx509-chardev-notls", NONE);
DO_TEST("serial-many", NONE);
DO_TEST("serial-spiceport", NONE);
DO_TEST("serial-spiceport-nospice", NONE);
--
2.10.1
From d7a676ece48ca9958a119aab4e04c9cec3ed69f4 Mon Sep 17 00:00:00 2001
Message-Id: <d7a676ece48ca9958a119aab4e04c9cec3ed69f4.1476980707.git.phrdina@xxxxxxxxxx>
In-Reply-To: <7b70eeb03f4fe190d78f50e72a1b0e5264af5299.1476980707.git.phrdina@xxxxxxxxxx>
References: <7b70eeb03f4fe190d78f50e72a1b0e5264af5299.1476980707.git.phrdina@xxxxxxxxxx>
From: Pavel Hrdina <phrdina@xxxxxxxxxx>
Date: Wed, 19 Oct 2016 09:57:41 +0200
Subject: [PATCH 2/4] domain: fix migration to older libvirt
Since TLS feature was introduced in libvirt 2.3.0 we have to modify
migratable XML for specific case where 'tls' attribute is based on
setting from qemu.conf.
Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
---
src/conf/domain_conf.c | 24 +++++++++++++++++++++++-
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 1 +
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e4fa9ad..aa425e2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1999,6 +1999,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
return -1;
dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
+ dest->data.tcp.tlsFromConfig = src->data.tcp.tlsFromConfig;
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
@@ -10043,6 +10044,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
char *slave = NULL;
char *append = NULL;
char *haveTLS = NULL;
+ char *tlsFromConfig = NULL;
int remaining = 0;
while (cur != NULL) {
@@ -10052,6 +10054,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
mode = virXMLPropString(cur, "mode");
if (!haveTLS)
haveTLS = virXMLPropString(cur, "tls");
+ if (!tlsFromConfig)
+ tlsFromConfig = virXMLPropString(cur, "tlsFromConfig");
switch ((virDomainChrType) def->type) {
case VIR_DOMAIN_CHR_TYPE_FILE:
@@ -10237,6 +10241,18 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
goto error;
}
+ if (tlsFromConfig &&
+ flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
+ int tmp;
+ if (virStrToLong_i(tlsFromConfig, NULL, 10, &tmp) < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid tlsFromConfig value: %s"),
+ tlsFromConfig);
+ goto error;
+ }
+ def->data.tcp.tlsFromConfig = !!tmp;
+ }
+
if (!protocol)
def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
else if ((def->data.tcp.protocol =
@@ -10322,6 +10338,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
VIR_FREE(logappend);
VIR_FREE(logfile);
VIR_FREE(haveTLS);
+ VIR_FREE(tlsFromConfig);
return remaining;
@@ -21482,9 +21499,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
def->data.tcp.listen ? "bind" : "connect");
virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host);
virBufferEscapeString(buf, "service='%s'", def->data.tcp.service);
- if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT)
+ if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT &&
+ !(flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
+ def->data.tcp.tlsFromConfig))
virBufferAsprintf(buf, " tls='%s'",
virTristateBoolTypeToString(def->data.tcp.haveTLS));
+ if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
+ virBufferAsprintf(buf, " tlsFromConfig='%d'",
+ def->data.tcp.tlsFromConfig);
virBufferAddLit(buf, "/>\n");
virBufferAsprintf(buf, "<protocol type='%s'/>\n",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fcadf6c..7e12a57 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1095,6 +1095,7 @@ struct _virDomainChrSourceDef {
int protocol;
bool tlscreds;
int haveTLS; /* enum virTristateBool */
+ bool tlsFromConfig;
} tcp;
struct {
char *bindHost;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index cd3d376..04bc3c6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5083,6 +5083,7 @@ qemuProcessPrepareDomainChardevIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
chr->source.data.tcp.haveTLS = VIR_TRISTATE_BOOL_YES;
else
chr->source.data.tcp.haveTLS = VIR_TRISTATE_BOOL_NO;
+ chr->source.data.tcp.tlsFromConfig = true;
}
}
--
2.10.1
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list