[libvirt] [PATCH] network: fix problems with SRV
Martin Kletzander
mkletzan at redhat.com
Wed Mar 19 17:25:20 UTC 2014
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 at laine.org>
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140319/8beefad5/attachment-0001.sig>
More information about the libvir-list
mailing list