[libvirt] [PATCH] network: fix problems with SRV
Martin Kletzander
mkletzan at redhat.com
Tue Mar 18 07:09:04 UTC 2014
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.
> * the "domain" attribute is no longer required in order to recognize
> the port, priority, and weight attributes. 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.
> ---
> src/conf/network_conf.c | 113 +++++++++++++--------
> src/network/bridge_driver.c | 75 ++++++++------
> .../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, 128 insertions(+), 78 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 9be06d3..3a28ac7 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -931,80 +931,107 @@ 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, "
> + "limit is %d bytes"),
> + def->service, networkName, DNS_RECORD_LENGTH_SRV);
> + goto error;
> + }
> + if (def->service[0] == '_') {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("service attribute '%s' in network %s DNS SRV "
> + "record cannot start with '_'"),
> + def->service, networkName);
> + goto error;
> + }
> }
>
> if (!(def->protocol = virXMLPropString(node, "protocol")) && !partialOkay) {
> virReportError(VIR_ERR_XML_DETAIL,
> _("Missing required protocol attribute "
> - "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 && 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).
> - _("Invalid protocol attribute value '%s' "
> - "in DNS SRV record of network %s"),
> + _("protocol attribute '%s' in network %s DNS SRV record "
> + "cannot start with '_'"),
> 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;
> + def->domain = virXMLPropString(node, "domain");
> + def->target = virXMLPropString(node, "target");
>
> - 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;
> - }
> -
> - 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 > 65535)) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Invalid DNS SRV port attribute "
> + "for service %s in network %s"),
> + def->service, networkName);
> + goto error;
> + }
>
> - ctxt->node = save_ctxt;
> + ret = virXPathUInt("string(./@priority)", ctxt, &def->priority);
> + if (ret >= 0 && !def->target) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("DNS SRV priority attribute not permitted without "
> + "target for service %s in network %s"),
> + def->service, networkName);
> + goto error;
> + }
> + if (ret == -2 || (ret >= 0 && def->priority > 65535)) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Invalid DNS SRV priority attribute "
> + "for service %s in network %s"),
> + def->service, networkName);
> + goto error;
> }
>
> - if (!(def->service || def->protocol)) {
> + ret = virXPathUInt("string(./@weight)", ctxt, &def->weight);
> + if (ret >= 0 && !def->target) {
> virReportError(VIR_ERR_XML_DETAIL,
> - _("Missing required service attribute or protocol "
> - "in DNS SRV record of network %s"), networkName);
> + _("DNS SRV weight attribute not permitted without "
> + "target for service %s in network %s"),
> + def->service, networkName);
> goto error;
> }
> + if (ret == -2 || (ret >= 0 && def->weight > 65535)) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Invalid DNS SRV weight attribute "
> + "for service %s in network %s"),
> + def->service, networkName);
> + goto error;
> + }
> +
> + ctxt->node = save_ctxt;
> return 0;
>
> error:
> virNetworkDNSSrvDefClear(def);
> + ctxt->node = save_ctxt;
> return -1;
> }
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 181541e..94400cf 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -734,10 +734,6 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
> int r, ret = -1;
> int nbleases = 0;
> size_t i;
> - char *record = NULL;
> - char *recordPort = NULL;
> - char *recordWeight = NULL;
> - char *recordPriority = NULL;
> virNetworkDNSDefPtr dns = &network->def->dns;
> virNetworkIpDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
> bool ipv6SLAAC;
> @@ -878,33 +874,52 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
> }
>
> for (i = 0; i < dns->nsrvs; i++) {
> - if (dns->srvs[i].service && dns->srvs[i].protocol) {
> - if (dns->srvs[i].port &&
> - virAsprintf(&recordPort, "%d", dns->srvs[i].port) < 0)
> - goto cleanup;
> - if (dns->srvs[i].priority &&
> - virAsprintf(&recordPriority, "%d", dns->srvs[i].priority) < 0)
> - goto cleanup;
> - if (dns->srvs[i].weight &&
> - virAsprintf(&recordWeight, "%d", dns->srvs[i].weight) < 0)
> - goto cleanup;
> + /* service/protocol are required, and should have been validated
> + * by the parser.
> + */
> + if (!dns->srvs[i].service) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Missing required 'service' "
> + "attribute in SRV record of network '%s'"),
> + network->def->name);
> + goto cleanup;
> + }
> + if (!dns->srvs[i].protocol) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Missing required 'service' "
> + "attribute in SRV record of network '%s'"),
> + network->def->name);
> + goto cleanup;
> + }
> + /* RFC2782 requires that service and protocol be preceded by
> + * an underscore.
> + */
> + virBufferAsprintf(&configbuf, "srv-host=_%s._%s",
> + dns->srvs[i].service, dns->srvs[i].protocol);
>
> - if (virAsprintf(&record, "%s.%s.%s,%s,%s,%s,%s",
> - dns->srvs[i].service,
> - dns->srvs[i].protocol,
> - dns->srvs[i].domain ? dns->srvs[i].domain : "",
> - dns->srvs[i].target ? dns->srvs[i].target : "",
> - recordPort ? recordPort : "",
> - recordPriority ? recordPriority : "",
> - recordWeight ? recordWeight : "") < 0)
> - goto cleanup;
> + /* domain is optional - it defaults to the domain of this network */
> + if (dns->srvs[i].domain)
> + virBufferAsprintf(&configbuf, ".%s", dns->srvs[i].domain);
>
> - virBufferAsprintf(&configbuf, "srv-host=%s\n", record);
> - VIR_FREE(record);
> - VIR_FREE(recordPort);
> - VIR_FREE(recordWeight);
> - VIR_FREE(recordPriority);
> + /* If target is empty or ".", that means "the service is
> + * decidedly not available at this domain" (RFC2782). In that
> + * case, any port, priority, or weight is irrelevant.
> + */
> + if (dns->srvs[i].target && STRNEQ(dns->srvs[i].target, ".")) {
> +
> + virBufferAsprintf(&configbuf, ",%s", dns->srvs[i].target);
> + /* port, priority, and weight are optional, but are
> + * identified by their position in the line
> + */
> + 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.
Rest looks fine.
Martin
> + if (dns->srvs[i].priority || dns->srvs[i].weight)
> + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].priority);
> + if (dns->srvs[i].weight)
> + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].weight);
> }
> + virBufferAddLit(&configbuf, "\n");
> }
>
> /* Find the first dhcp for both IPv4 and IPv6 */
> @@ -1080,10 +1095,6 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>
> cleanup:
> virBufferFreeAndReset(&configbuf);
> - VIR_FREE(record);
> - VIR_FREE(recordPort);
> - VIR_FREE(recordWeight);
> - VIR_FREE(recordPriority);
> return ret;
> }
>
> diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
> index ce4dd6f..e60411b 100644
> --- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
> +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
> @@ -12,7 +12,7 @@ listen-address=192.168.123.1
> listen-address=fc00:db8:ac10:fe01::1
> listen-address=fc00:db8:ac10:fd01::1
> listen-address=10.24.10.1
> -srv-host=name.tcp.,,,,
> +srv-host=_name._tcp
> dhcp-range=192.168.122.2,192.168.122.254
> dhcp-no-override
> dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases
> diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
> index b47cbe7..b6fe6f9 100644
> --- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf
> +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
> @@ -8,7 +8,13 @@ strict-order
> except-interface=lo
> bind-dynamic
> interface=virbr0
> -srv-host=name.tcp.test-domain-name,.,1024,10,10
> +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=_name6._tcp.test6.com,test6.example.com,6666,0,666
> +srv-host=_name7._tcp.test7.com,test7.example.com,0,0,777
> dhcp-range=192.168.122.2,192.168.122.254
> dhcp-no-override
> dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases
> diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.xml b/tests/networkxml2confdata/nat-network-dns-srv-record.xml
> index 3dd19e6..d01b331 100644
> --- a/tests/networkxml2confdata/nat-network-dns-srv-record.xml
> +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.xml
> @@ -6,7 +6,13 @@
> </forward>
> <bridge name='virbr0' stp='on' delay='0'/>
> <dns>
> - <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10'/>
> + <srv service='name' protocol='tcp' domain='test-domain-name.com' target='test.example.com' port='1111' priority='11' weight='111'/>
> + <srv service='name2' protocol='udp' target='test2.example.com' port='2222' priority='22' weight='222'/>
> + <srv service='name3' protocol='tcp' domain='test3.com' target='test3.example.com' port='3333' priority='33'/>
> + <srv service='name4' protocol='tcp' domain='test4.com' target='test4.example.com' port='4444'/>
> + <srv service='name5' protocol='udp' target='test5.example.com' priority='55' weight='555'/>
> + <srv service='name6' protocol='tcp' domain='test6.com' target='test6.example.com' port='6666' weight='666'/>
> + <srv service='name7' protocol='tcp' domain='test7.com' target='test7.example.com' weight='777'/>
> </dns>
> <ip address='192.168.122.1' netmask='255.255.255.0'>
> <dhcp>
> --
> 1.8.5.3
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20140318/8962bc5b/attachment-0001.sig>
More information about the libvir-list
mailing list