Fix hostname validation for nameservers

Fixes the hostname validation to align with the RFC's demands[1].
This was done by replacing the full regex with a function that
broke the FQDN into individual components that were easier to
reason about with regular expressions.

Also added several test cases for domains so if someone wants
to convert it back to pure regex there will be better test vectors.

1. RFC 1123 says an all-digit hostname is allowed in section 2.1. It
   says that this more liberal syntax MUST be supported.

Closes-Bug: #1396932
Change-Id: I003cf14d95070707e43e40d55da62e11a28dfa4e

diff --git a/neutron/api/v2/attributes.py b/neutron/api/v2/attributes.py
index 0301aac03d4764796aa5b519c1276ad7d62b4a71..b9ac5730d95756621f27de6f3c2837f096ae52d7 100644 (file)
--- a/neutron/api/v2/attributes.py
+++ b/neutron/api/v2/attributes.py
@@ -233,27 +233,38 @@ def _validate_fixed_ips(data, valid_values=None):
                 return msg
 
 
+def _validate_ip_or_hostname(host):
+    ip_err = _validate_ip_address(host)
+    if not ip_err:
+        return
+    name_err = _validate_hostname(host)
+    if not name_err:
+        return
+    msg = _("%(host)s is not a valid IP or hostname. Details: "
+            "%(ip_err)s, %(name_err)s") % {'ip_err': ip_err, 'host': host,
+                                           'name_err': name_err}
+    return msg
+
+
 def _validate_nameservers(data, valid_values=None):
     if not hasattr(data, '__iter__'):
         msg = _("Invalid data format for nameserver: '%s'") % data
         LOG.debug(msg)
         return msg
 
-    ips = []
-    for ip in data:
-        msg = _validate_ip_address(ip)
+    hosts = []
+    for host in data:
+        # This may be an IP or a hostname
+        msg = _validate_ip_or_hostname(host)
         if msg:
-            # This may be a hostname
-            msg = _validate_regex(ip, HOSTNAME_PATTERN)
-            if msg:
-                msg = _("'%s' is not a valid nameserver") % ip
-                LOG.debug(msg)
-                return msg
-        if ip in ips:
-            msg = _("Duplicate nameserver '%s'") % ip
+            msg = _("'%(host)s' is not a valid nameserver. %(msg)s") % {
+                'host': host, 'msg': msg}
+            return msg
+        if host in hosts:
+            msg = _("Duplicate nameserver '%s'") % host
             LOG.debug(msg)
             return msg
-        ips.append(ip)
+        hosts.append(host)
 
 
 def _validate_hostroutes(data, valid_values=None):
@@ -330,6 +341,41 @@ def _validate_subnet_or_none(data, valid_values=None):
     return _validate_subnet(data, valid_values)
 
 
+def _validate_hostname(data):
+    # NOTE: An individual name regex instead of an entire FQDN was used
+    # because its easier to make correct. Feel free to replace with a
+    # full regex solution. The logic should validate that the hostname
+    # matches RFC 1123 (section 2.1) and RFC 952.
+    hostname_pattern = "[a-zA-Z0-9-]{1,63}$"
+    try:
+        # Trailing periods are allowed to indicate that a name is fully
+        # qualified per RFC 1034 (page 7).
+        trimmed = data if data[-1] != '.' else data[:-1]
+        if len(trimmed) > 255:
+            raise TypeError(
+                _("'%s' exceeds the 255 character hostname limit") % trimmed)
+        names = trimmed.split('.')
+        for name in names:
+            if not name:
+                raise TypeError(_("Encountered an empty component."))
+            if name[-1] == '-' or name[0] == '-':
+                raise TypeError(
+                    _("Name '%s' must not start or end with a hyphen.") % name)
+            if not re.match(hostname_pattern, name):
+                raise TypeError(
+                    _("Name '%s' must be 1-63 characters long, each of "
+                      "which can only be alphanumeric or a hyphen.") % name)
+        # RFC 1123 hints that a TLD can't be all numeric. last is a TLD if
+        # it's an FQDN.
+        if len(names) > 1 and re.match("^[0-9]+$", names[-1]):
+            raise TypeError(_("TLD '%s' must not be all numeric") % names[-1])
+    except TypeError as e:
+        msg = _("'%(data)s' is not a valid hostname. Reason: %(reason)s") % {
+            'data': data, 'reason': e.message}
+        LOG.debug(msg)
+        return msg
+
+
 def _validate_regex(data, valid_values=None):
     try:
         if re.match(valid_values, data):
@@ -538,9 +584,6 @@ def convert_to_list(data):
         return [data]
 
 
-HOSTNAME_PATTERN = ("(?=^.{1,254}$)(^(?:(?!\d+.|-)[a-zA-Z0-9_\-]{1,62}"
-                    "[a-zA-Z0-9]\.?)+(?:[a-zA-Z]{2,})$)")
-
 HEX_ELEM = '[0-9A-Fa-f]'
 UUID_PATTERN = '-'.join([HEX_ELEM + '{8}', HEX_ELEM + '{4}',
                          HEX_ELEM + '{4}', HEX_ELEM + '{4}',

diff --git a/neutron/tests/unit/test_attributes.py b/neutron/tests/unit/test_attributes.py
index c94179dc3af3e061fc85ae7c2826ac95b2b928b8..54a44b1f863415073883b58c64bda4990be337ac 100644 (file)
--- a/neutron/tests/unit/test_attributes.py
+++ b/neutron/tests/unit/test_attributes.py
@@ -309,9 +309,7 @@ class TestAttributes(base.BaseTestCase):
     def test_validate_nameservers(self):
         ns_pools = [['1.1.1.2', '1.1.1.2'],
                     ['www.hostname.com', 'www.hostname.com'],
-                    ['77.hostname.com'],
                     ['1000.0.0.1'],
-                    ['1' * 59],
                     None]
 
         for ns in ns_pools:
@@ -322,6 +320,8 @@ class TestAttributes(base.BaseTestCase):
                     ['www.hostname.com'],
                     ['www.great.marathons.to.travel'],
                     ['valid'],
+                    ['77.hostname.com'],
+                    ['1' * 59],
                     ['www.internal.hostname.com']]
 
         for ns in ns_pools:
@@ -372,13 +372,19 @@ class TestAttributes(base.BaseTestCase):
         self.assertEqual(msg, "'%s' is not a valid IP address" % ip_addr)
 
     def test_hostname_pattern(self):
-        data = '@openstack'
-        msg = attributes._validate_regex(data, attributes.HOSTNAME_PATTERN)
-        self.assertIsNotNone(msg)
+        bad_values = ['@openstack', 'ffff.abcdefg' * 26, 'f' * 80, '-hello',
+                      'goodbye-', 'example..org']
+        for data in bad_values:
+            msg = attributes._validate_hostname(data)
+            self.assertIsNotNone(msg)
 
-        data = 'www.openstack.org'
-        msg = attributes._validate_regex(data, attributes.HOSTNAME_PATTERN)
-        self.assertIsNone(msg)
+        # All numeric hostnames are allowed per RFC 1123 section 2.1
+        good_values = ['www.openstack.org', '1234x', '1234',
+                       'openstack-1', 'v.xyz', '1' * 50, 'a1a',
+                       'x.x1x', 'x.yz', 'example.org.']
+        for data in good_values:
+            msg = attributes._validate_hostname(data)
+            self.assertIsNone(msg)
 
     def test_uuid_pattern(self):
         data = 'garbage'