On Wed, Mar 19, 2014 at 10:17:20AM -0600, Laine Stump wrote: > On 03/18/2014 01:09 AM, Martin Kletzander wrote: > > On Tue, Mar 18, 2014 at 12:07:17AM -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 0 in the commandline. > >> > >> <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 this would have generated an error, because "optional" > >> attributes weren't really optional). > >> > >> * As a safeguard, the parser checks for a leading "_" on service and > >> protocol, and fails if it is found. (Note that we can be guaranteed > >> that no existing valid configuration will have this, since until > >> now the parser had only allowed "tcp" or "udp" for protocol, and > >> the bridge driver wasn't prepending "_", making it a 100% certainty > >> that there was no example of working SRV record use in the wild). > >> > > That's valid for protocol, but not for service. For service there was > > a check for length only and it was not escaped at all. Even though > > there couldn't be any abuse for that, settings that resulted in > > generating invalid config file for dnsmasq were parsed and saved > > without any error from libvirt. > > Just to make sure what I wrote made sense: my point was that, due to the > fact that the current code would only be able to generate records like this: > > > [some-name].tcp.[some-domain] > [some-name].udp.[some-domain] > > (i.e. the protocol could only be the exact string "tcp" or "udp"), and > since the RFC *requires* that the protocol be prefixed with an > underscore, it is impossible that anyone has an actual working config > using the existing code (although it may have been saved with no error, > it wouldn't do anything useful). Because it is impossible that it could > have worked before, we don't need to worry about backward compatibility > problems - nothing we do could break a working config. > > But I *think* what you're pointing out is that, although the config may > not be working, it's possible that someone could have made a config like > this: > > <srv service='_sip' protocol='udp' domain='example.com' > target='sip.example.com' port='5060'/> > > which they could have saved, even though it would do nothing useful, it > would be saved. Then when that system was updated to the new libvirt, > that network's config would generate an error while loading, so the > network would be ignored. Gah!! > Yes, exactly :-( > So I *can't* add this new restriction in the parser. I could check for > it during networkValidate() in the bridge driver, though. Do you think > this is worthwhile? (I think I'll do it as a separate patch in any case). > The networkValidate() is called from DefineXML so that would make it ignored as well. I'd suggest networkStartNetwork() after the networkRunHook() since it can add anything. > > >> - > >> - /* Check whether protocol value is the supported one */ > >> - if (def->protocol && STRNEQ(def->protocol, "tcp") && > >> - (STRNEQ(def->protocol, "udp"))) { > >> + if (def->protocol && def->protocol[0] == '_') { > >> virReportError(VIR_ERR_XML_DETAIL, > > I'm not sure we want to allow anything not starting with '_'. OTOH, > > we allowed anything in the service (even dots and commas and so on > > (non-working configuration). > > I was bothered by this as well, and spent quite awhile searching RFCs to > get a definitive answer on what characters are allowed in a service name > or a protocol name. I couldn't find this information, though. I could > only find lists of currently assigned service/protocol names. I guess > since it is the number that goes over the wire, the RFCs are more > concerned about the number than about exactly what it is called (dhcp > option names suffer the same problem - there are only official option > *numbers*, but not official names). > > I then considered limiting the character set to what can currently be > found in /etc/services and /etc/protocol on a Linux system, but after > taking a quick look, it seemed like there wasn't a very strict code > about it - there are even some names that use "+". > > Eric did find a bit in "man services" that says that basically any > printable ASCII character is okay, but you should be conservative in > what you use (didn't stop people from adding things like "sql*net" and > "whois++" to /etc/services, though :-/) > OK, this is sick and I hate that we have to deal with it :-/ > So how about this - I will define a character set that contains only > alphanumerics, and those special characters currently used in > /etc/services and /etc/protocol, and check it against that. (except ".", > because dnsmasq uses that as a separator on the commandline, and there > is no method of escaping it). > > (As discussed above, I have to remove the restriction on _ as the > leading character.) > OK, that looks pretty good. I also thought about allowing '_' as well and prepending it only if the service *does not* already start with it, but that wouldn't make too much sense. Thinking about it for a while, how about we allow anything except '.' and '\n' and let the user deal with his/hers improper configurations? We'll just prepend '_' and be done with it. > >> + if (dns->srvs[i].port || > >> + dns->srvs[i].priority || dns->srvs[i].weight) > >> + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].port); > > According to dnsmasq(1), port defaults to 1, but you default it here > > to 0, which doesn't make sense. > > Good point, and although our config should be affected by the RFC rather > than by dnsmasq, the RFC doesn't say anything about default values > because in the DNS record all the fields are mandatory. Since a service > for port 0 makes no sense (port 0 is reserved for both tcp and udp), I > think we can safely make the value "0" mean "unspecified, use default of > 1", and then disallow config with an explicit "port='0'" to avoid > ambiguity. Then if we need to put something on the commandline to fill > the space, we will put "1" instead of "0". > That sound great. > I'll squash in the changes in the attached patch and send a v2. > From c104ab2370a7dd601c0ad2aff08e176e6eae1e3b Mon Sep 17 00:00:00 2001 > From: Laine Stump <laine@xxxxxxxxx> > Date: Tue, 18 Mar 2014 15:06:22 -0600 > Subject: [PATCH] Changes to squash into SRV patch > > 1) we must allow leading _ in service, just in case someone has it in an > already-saved config. > > 2) otherwise narrow down the acceptable characters in protocol and > service to alphnumerics plus those special characters already found > in /etc/services and /etc/protocol (with the exception of ".", > which can't be allowed due to its use as a separator character). > > 3) default for port is 1, not 0. Don't allow "port='0'" (so that value > can be reserved to mean "default"), and adjust dnsmasq commandline > and test cases accordingly. > --- > src/conf/network_conf.c | 30 +++++++++++++++++----- > src/network/bridge_driver.c | 9 +++++-- > .../nat-network-dns-srv-record.conf | 4 +-- > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index 3a28ac7..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" \ > + "_-+/*" > + > static int > virNetworkDNSSrvDefParseXML(const char *networkName, > xmlNodePtr node, > @@ -950,10 +965,10 @@ virNetworkDNSSrvDefParseXML(const char *networkName, > def->service, networkName, DNS_RECORD_LENGTH_SRV); > goto error; > } > - if (def->service[0] == '_') { > + if (strspn(def->service, SERVICE_CHARS) < strlen(def->service)) { > virReportError(VIR_ERR_XML_DETAIL, > - _("service attribute '%s' in network %s DNS SRV " > - "record cannot start with '_'"), > + _("invalid character in service attribute '%s' " > + "in network %s DNS SRV record"), > def->service, networkName); > goto error; > } > @@ -966,10 +981,11 @@ virNetworkDNSSrvDefParseXML(const char *networkName, > def->service, networkName); > goto error; > } > - if (def->protocol && def->protocol[0] == '_') { > + if (def->protocol && > + strspn(def->protocol, PROTOCOL_CHARS) < strlen(def->protocol)) { > virReportError(VIR_ERR_XML_DETAIL, > - _("protocol attribute '%s' in network %s DNS SRV record " > - "cannot start with '_'"), > + _("invalid character in protocol attribute '%s' " > + "in network %s DNS SRV record"), > def->protocol, networkName); > goto error; > } > @@ -986,7 +1002,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, > def->service, networkName); > goto error; > } > - if (ret == -2 || (ret >= 0 && def->port > 65535)) { > + if (ret == -2 || (ret >= 0 && (def->port < 1 || def->port > 65535))) { > virReportError(VIR_ERR_XML_DETAIL, > _("Invalid DNS SRV port attribute " > "for service %s in network %s"), > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 94400cf..3ad6874 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -909,11 +909,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, > > virBufferAsprintf(&configbuf, ",%s", dns->srvs[i].target); > /* port, priority, and weight are optional, but are > - * identified by their position in the line > + * identified by their position in the line. If an item is > + * unspecified, but something later in the line *is* > + * specified, we need to give the default value for the > + * unspecified item. (According to the dnsmasq manpage, > + * the default for port is 1). > */ > if (dns->srvs[i].port || > dns->srvs[i].priority || dns->srvs[i].weight) > - virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].port); > + virBufferAsprintf(&configbuf, ",%d", > + dns->srvs[i].port ? dns->srvs[i].port : 1); > if (dns->srvs[i].priority || dns->srvs[i].weight) > virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].priority); > if (dns->srvs[i].weight) > diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf > index b6fe6f9..16e7dca 100644 > --- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf > +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf > @@ -12,9 +12,9 @@ srv-host=_name._tcp.test-domain-name.com,test.example.com,1111,11,111 > srv-host=_name2._udp,test2.example.com,2222,22,222 > srv-host=_name3._tcp.test3.com,test3.example.com,3333,33 > srv-host=_name4._tcp.test4.com,test4.example.com,4444 > -srv-host=_name5._udp,test5.example.com,0,55,555 > +srv-host=_name5._udp,test5.example.com,1,55,555 > srv-host=_name6._tcp.test6.com,test6.example.com,6666,0,666 > -srv-host=_name7._tcp.test7.com,test7.example.com,0,0,777 > +srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777 > dhcp-range=192.168.122.2,192.168.122.254 > dhcp-no-override > dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases > -- > 1.8.5.3 >
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list