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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list