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

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

 



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

[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]