Re: [libvirt] [PATCHv2] network: fix problems with SRV records

On Wed, Mar 19, 2014 at 11:48:14AM -0600, Laine Stump wrote:

A patch submitted by Steven Malin last week pointed out a problem with libvirt's DNS SRV record configuration:

https://www.redhat.com/archives/libvir-list/2014-March/msg00536.html

When searching for that message later, I found another series that had been posted by Guannan Ren back in 2012 that somehow slipped between the cracks:

https://www.redhat.com/archives/libvir-list/2012-July/msg00236.html

That patch was very much out of date, but also pointed out some real problems.

This patch fixes all the noted problems by refactoring virNetworkDNSSrvDefParseXML() and networkDnsmasqConfContents(), then verifies those fixes by added several new records to the test case.

Problems fixed:

* both service and protocol now have an underscore ("_") prepended on the commandline, as required by RFC2782.

<srv service='sip' protocol='udp' domain='example.com' target='tests.example.com' port='5060' priority='10' weight='150'/>

before: srv-host=sip.udp.example.com,tests.example.com,5060,10,150 after: srv-host=_sip._udp.example.com,tests.example.com,5060,10,150

* if "domain" wasn't specified in the <srv> element, the extra trailing "." will no longer be added to the dnsmasq commandline.

<srv service='sip' protocol='udp' target='tests.example.com' port='5060' priority='10' weight='150'/>

before: srv-host=sip.udp.,tests.example.com,5060,10,150 after: srv-host=_sip._udp,tests.example.com,5060,10,150

* when optional attributes aren't specified, the separating comma is also now not placed on the dnsmasq commandline. If optional attributes in the middle of the line are not specified, they are replaced with a default value in the commandline (1 for port, 0 for priority and weight).

<srv service='sip' protocol='udp' target='tests.example.com' port='5060'/>

before: srv-host=sip.udp.,tests.example.com,5060,, after: srv-host=_sip._udp,tests.example.com,5060

(actually the would have generated an error, because "optional" attributes weren't really optional.)

* The allowed characters for both service and protocol are now limited to alphanumerics, plus a few special characters that are found in existing names in /etc/services and /etc/protocols. (One exception is that both of these files contain names with an embedded ".", but "." can't be used in these fields of an SRV record because it is used as a field separator and there is no method to escape a "." into a field.) (Previously only the strings "tcp" and "udp" were allowed for protocol, but this restriction has been removed, since RFC2782 specifically says that it isn't limited to those, and that anyway it is case insensitive.)

* the "domain" attribute is no longer required in order to recognize the port, priority, and weight attributes during parsing. Only "target" is required for this.

* if "target" isn't specified, port, priority, and weight are not allowed (since they are meaningless - an empty target means "this service is *not available* for this domain").

* port, priority, and weight are now truly optional, as the comments originally suggested, but which was not actually true. --- Changes from V1:

https://www.redhat.com/archives/libvir-list/2014-March/msg01172.html

src/conf/network_conf.c | 129 ++++++++++++++------- src/network/bridge_driver.c | 80 ++++++++----- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 8 +- .../nat-network-dns-srv-record.xml | 8 +- 5 files changed, 149 insertions(+), 78 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9be06d3..f1e6243 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -924,6 +924,21 @@ error: return -1; }

+/* This includes all characters used in the names of current + * /etc/services and /etc/protocols files (on Fedora 20), except ".", + * which we can't allow because it would conflict with the use of "." + * as a field separator in the SRV record, there appears to be no way + * to escape it in, and the protocols/services that use "." in the + * name are obscure and unlikely to be used anyway. + */ +#define PROTOCOL_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" \ + "-+/" + +#define SERVICE_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" \ + "_-+/*" +

We are using this [a-zA-Z0-9] string on few places, maybe we could put those together, but that's just a thought, definitely not related to this patch.

static int virNetworkDNSSrvDefParseXML(const char *networkName, xmlNodePtr node, @@ -931,80 +946,108 @@ virNetworkDNSSrvDefParseXML(const char *networkName, virNetworkDNSSrvDefPtr def, bool partialOkay) { + int ret; + xmlNodePtr save_ctxt = ctxt->node; + + ctxt->node = node; + if (!(def->service = virXMLPropString(node, "service")) && !partialOkay) { virReportError(VIR_ERR_XML_DETAIL, _("Missing required service attribute in DNS SRV record " "of network %s"), networkName); goto error; } - if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) { - virReportError(VIR_ERR_XML_DETAIL, - _("Service name '%s' in network %s is too long, limit is %d bytes"), - def->service, networkName, DNS_RECORD_LENGTH_SRV); - goto error; + if (def->service) { + if (strlen(def->service) > DNS_RECORD_LENGTH_SRV) { + virReportError(VIR_ERR_XML_DETAIL, + _("service attribute '%s' in network %s is too long, "

The patch looks great, if I may, I'd suggest few super-tiny clean-ups. Here it's pre-existing, but on other places in this patch it is created with the similar pattern. The thing I don't really like is that one parameter is "escaped" and one not, I mean the difference between 'def->service' and networkName. Please add those apostrophes around other %s parts of these strings to unify that.

+ "limit is %d bytes"), + def->service, networkName, DNS_RECORD_LENGTH_SRV); + goto error; + } + if (strspn(def->service, SERVICE_CHARS) < strlen(def->service)) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid character in service attribute '%s' " + "in network %s DNS SRV record"),

And here I'd use a bit different wording, what would you say for: "in DNS SRV record of network '%s'"

+ def->service, networkName); + goto error; + } }

if (!(def->protocol = virXMLPropString(node, "protocol")) && !partialOkay) { virReportError(VIR_ERR_XML_DETAIL, _("Missing required protocol attribute "

You were changing messages to lowercase, but forgot this one.

- "in dns srv record '%s' of network %s"), + "in DNS SRV record '%s' of network %s"), def->service, networkName); goto error; } - - /* Check whether protocol value is the supported one */ - if (def->protocol && STRNEQ(def->protocol, "tcp") && - (STRNEQ(def->protocol, "udp"))) { + if (def->protocol && + strspn(def->protocol, PROTOCOL_CHARS) < strlen(def->protocol)) { virReportError(VIR_ERR_XML_DETAIL, - _("Invalid protocol attribute value '%s' " - "in DNS SRV record of network %s"), + _("invalid character in protocol attribute '%s' " + "in network %s DNS SRV record"), def->protocol, networkName); goto error; }

/* Following attributes are optional */ - if ((def->target = virXMLPropString(node, "target")) && - (def->domain = virXMLPropString(node, "domain"))) { - xmlNodePtr save_ctxt = ctxt->node; - - ctxt->node = node; - if (virXPathUInt("string(./@port)", ctxt, &def->port) < 0 || - def->port > 65535) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing or invalid port attribute " - "in network %s"), networkName); - goto error; - } - - if (virXPathUInt("string(./@priority)", ctxt, &def->priority) < 0 || - def->priority > 65535) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing or invalid priority attribute " - "in network %s"), networkName); - goto error; - } + def->domain = virXMLPropString(node, "domain"); + def->target = virXMLPropString(node, "target");

- if (virXPathUInt("string(./@weight)", ctxt, &def->weight) < 0 || - def->weight > 65535) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing or invalid weight attribute " - "in network %s"), networkName); - goto error; - } + ret = virXPathUInt("string(./@port)", ctxt, &def->port); + if (ret >= 0 && !def->target) { + virReportError(VIR_ERR_XML_DETAIL, + _("DNS SRV port attribute not permitted without " + "target for service %s in network %s"), + def->service, networkName); + goto error; + } + if (ret == -2 || (ret >= 0 && (def->port < 1 || def->port > 65535))) { + virReportError(VIR_ERR_XML_DETAIL, + _("Invalid DNS SRV port attribute "

And for example here you have uppercase 'I' again. ACK to this patch with cleaned up apostrophes and unified Capital/all-lower-case messages. Thanks, Martin