[libvirt] [PATCHv2] network: fix problems with SRV records
Martin Kletzander
mkletzan at redhat.com
Tue Mar 25 15:21:30 UTC 2014
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
-------------- 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/20140325/8ba18790/attachment-0001.sig>
More information about the libvir-list
mailing list