Re: [PATCH] network: fix problems with SRV

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]