Merge lp:~mpontillo/maas/bug-1433622-disallow-absolute-fqdn-cluster-names into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Add a validator for domain names, and use it to validate the NodeGroup (cluster) name. In addition, add a CharField subclass specific to DNS names, which will strip any trailing '.' before saving to the database, and make use of the validator by default.

Refactored the hostname validator to separate hostname validation functionality from domain name validation functionality.

Added DomainNameField, a CharField subclass whose sole job is to normalize DNS names by removing all whitespace and trailing '.' characters, and use the new validator.

[Andres Rodriguez (andreserl)]

As discussed over IRC, using a final '.' in a zone is allowed per standard and MAAS would need to support this.

This means that if a user inputs the zone in the cluster controller page as:

'maas.com.' or 'maas.'

it should continue to be allowed and should have the same result as:

'maas.com' or 'maas'

That being said, even when a zone in the form of 'maas.com' or 'maas' is given, DNS config files are written with a final '.':

@ IN SOA maas.com. nobody.example.com. (

or

@ IN SOA maas. nobody.example.com. (

review: Needs Fixing

[Andres Rodriguez (andreserl)]

Changing this to needs information so we can start a discussion

review: Needs Information

[Mike Pontillo (mpontillo)]

There are two possible lines of thinking here:

(1) We should prevent the user from creating a database record that will cause incorrect behavior. (that's what this patch does)

(2) We should accept the user's valid-but-incorrect-behavior-causing input and transform it into something that does not cause incorrect behavior.

This patch addresses (1) but not (2). Thus, with this patch, if you type "andres.maas." into the name for a NodeGroup, you will get an error.

A better fix would automatically transform "andres.maas." into "andres.maas" *before* storing it in the database. (The reason you want to do it *before* is for database normalization; to MAAS, all DNS suffixes are absolute, fully-qualified names - so the trailing "." should have no meaning. Therefore, if someone enters "andres.maas." when there is already an "andres.maas", that should be a unique constraint violation.)

[Mike Pontillo (mpontillo)]

Suggestion: let's merge this simpler workaround first, to prevent users from shooting themselves in the foot until the complete fix is devised. Thoughts?

[Mike Pontillo (mpontillo)]

I figured out an elegant way to automatically strip off any trailing '.' characters in the model before the object is saved. I think this will avoid the issue. But I left the validators in place, just in case. (besides, there are likely to be other ways users can screw up their DNS names.)

[Blake Rouse (blake-rouse)]

Need more information on this implementation. I overall need more information on how MAAS validates hostnames, as this also affects my other branch that you reviewed. We should get this nailed down ASAP tomorrow.

review: Needs Information

[Mike Pontillo (mpontillo)]

Raphael and I discussed this. Our hostname validation is severely lacking at the moment. The validation code in dns.py you were looking at was recently moved there by myself, when I was refactoring some other validations. We should use it in more places, most likely.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2015-03-25 15:33:23 +0000
+++ src/maasserver/api/tests/test_node.py	2015-04-03 16:20:24 +0000
@@ -691,7 +691,7 @@
 
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
         self.assertEqual(
-            {'hostname': ["Hostname contains empty name."]},
+            {'hostname': ["DNS name contains an empty label."]},
             parsed_result)
 
     def test_PUT_refuses_to_update_invisible_node(self):

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2015-03-26 16:33:06 +0000
+++ src/maasserver/models/nodegroup.py	2015-04-03 16:20:24 +0000
@@ -43,6 +43,7 @@
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.models.user import get_creds_tuple
 from maasserver.rpc import getClientFor
+from maasserver.utils.dns import validate_domain_name
 from piston.models import (
     KEY_SIZE,
     Token,
@@ -142,6 +143,31 @@
         return self.filter(status=NODEGROUP_STATUS.ACCEPTED)
 
 
+class DomainNameField(CharField):
+    """Custom Django field that strips whitespace and trailing '.' characters
+    from DNS domain names before validating and saving to the database. Also,
+    validates that the domain name is valid according to RFCs 952 and 1123.
+    (Note that this field type should NOT be used for hostnames, since the set
+    of valid hostnames is smaller than the set of valid domain names.)
+    """
+    def __init__(self, *args, **kwargs):
+        validators = kwargs.pop('validators', [])
+        validators.append(validate_domain_name)
+        kwargs['validators'] = validators
+        super(DomainNameField, self).__init__(*args, **kwargs)
+
+    # Here we are using (abusing?) the to_pytion() function to coerce and
+    # normalize this type. Django does not have a function intended purely
+    # to normalize before saving to the database, so to_python() is the next
+    # closest alternative. For more information, see:
+    # https://docs.djangoproject.com/en/1.6/ref/forms/validation/
+    # https://code.djangoproject.com/ticket/6362
+    def to_python(self, value):
+        value = super(DomainNameField, self).to_python(value)
+        value = value.strip().rstrip('.')
+        return value
+
+
 NODEGROUP_CLUSTER_NAME_TEMPLATE = "Cluster %(uuid)s"
 
 
@@ -156,7 +182,7 @@
         max_length=100, unique=True, editable=True, blank=True, null=False)
 
     # A node group's name is also used for the group's DNS zone.
-    name = CharField(
+    name = DomainNameField(
         max_length=80, unique=False, editable=True, blank=True, null=False)
 
     status = IntegerField(

=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
--- src/maasserver/models/tests/test_nodegroup.py	2015-03-26 16:33:06 +0000
+++ src/maasserver/models/tests/test_nodegroup.py	2015-04-03 16:20:24 +0000
@@ -18,6 +18,7 @@
 from urlparse import urlparse
 
 from apiclient.creds import convert_string_to_tuple
+from django.core.exceptions import ValidationError
 from django.db.models.signals import post_save
 import django.dispatch
 from maasserver.bootresources import get_simplestream_endpoint
@@ -64,6 +65,7 @@
 )
 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
 from provisioningserver.utils.enum import map_enum
+from testtools import ExpectedException
 from testtools.matchers import (
     EndsWith,
     Equals,
@@ -251,6 +253,34 @@
 
 class TestNodeGroup(MAASServerTestCase):
 
+    def test_nodegroup_created_with_factory_validates(self):
+        try:
+            ng = factory.make_NodeGroup(name="maas")
+            ng.clean_fields()
+        except ValidationError:
+            self.fail("Factory should create a NodeGroup that validates.")
+
+    def test_create_nodegroup_with_absolute_dns_name_succeeds(self):
+        ng = factory.make_NodeGroup(name="maas.")
+        self.assertThat(ng.name, Equals("maas."))
+        ng.clean_fields()
+        self.assertThat(ng.name, Equals("maas"))
+        ng.save()
+        self.assertThat(ng.name, Equals("maas"))
+
+    def test_create_nodegroup_with_multiple_dots_dns_name_succeeds(self):
+        ng = factory.make_NodeGroup(name="maas.ubuntu.com.")
+        self.assertThat(ng.name, Equals("maas.ubuntu.com."))
+        ng.clean_fields()
+        self.assertThat(ng.name, Equals("maas.ubuntu.com"))
+        ng.save()
+        self.assertThat(ng.name, Equals("maas.ubuntu.com"))
+
+    def test_create_nodegroup_with_invalid_dns_name_fails(self):
+        with ExpectedException(ValidationError):
+            ng = factory.make_NodeGroup(name="m@@$")
+            ng.clean_fields()
+
     def test_delete_cluster_with_nodes(self):
         nodegroup = factory.make_NodeGroup()
         factory.make_Node(nodegroup=nodegroup)

=== modified file 'src/maasserver/utils/dns.py'
--- src/maasserver/utils/dns.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/utils/dns.py	2015-04-03 16:20:24 +0000
@@ -25,12 +25,12 @@
     ]
 
 
-def validate_hostname(hostname):
-    """Validator for hostnames.
+def validate_domain_name(name):
+    """Validator for domain names.
 
-    :param hostname: Input value for a host name.  May include domain.
-    :raise ValidationError: If the hostname is not valid according to RFCs 952
-        and 1123.
+    :param name: Input value for a domain name.  Must not include hostname.
+    :raise ValidationError: If the domain name is not valid according to
+    RFCs 952 and 1123.
     """
     # Valid characters within a hostname label: ASCII letters, ASCII digits,
     # hyphens, and underscores.  Not all are always valid.
@@ -38,28 +38,48 @@
     # very good for code maintenance.
     label_chars = re.compile('[a-zA-Z0-9_-]*$')
 
-    if len(hostname) > 255:
+    if len(name) > 255:
         raise ValidationError(
             "Hostname is too long.  Maximum allowed is 255 characters.")
     # A hostname consists of "labels" separated by dots.
-    labels = hostname.split('.')
-    if '_' in labels[0]:
-        # The host label cannot contain underscores; the rest of the name can.
-        raise ValidationError(
-            "Host label cannot contain underscore: %r." % labels[0])
+    labels = name.split('.')
     for label in labels:
         if len(label) == 0:
-            raise ValidationError("Hostname contains empty name.")
+            raise ValidationError("DNS name contains an empty label.")
         if len(label) > 63:
             raise ValidationError(
-                "Name is too long: %r.  Maximum allowed is 63 characters."
+                "Label is too long: %r.  Maximum allowed is 63 characters."
                 % label)
         if label.startswith('-') or label.endswith('-'):
             raise ValidationError(
-                "Name cannot start or end with hyphen: %r." % label)
+                "Label cannot start or end with hyphen: %r." % label)
         if not label_chars.match(label):
             raise ValidationError(
-                "Name contains disallowed characters: %r." % label)
+                "Label contains disallowed characters: %r." % label)
+
+
+def validate_hostname(hostname):
+    """Validator for hostnames.
+
+    :param hostname: Input value for a hostname.  May include domain.
+    :raise ValidationError: If the hostname is not valid according to RFCs 952
+        and 1123.
+    """
+    # Valid characters within a hostname label: ASCII letters, ASCII digits,
+    # hyphens, and underscores.  Not all are always valid.
+    # Technically we could write all of this as a single regex, but it's not
+    # very good for code maintenance.
+
+    if len(hostname) > 255:
+        raise ValidationError(
+            "Hostname is too long.  Maximum allowed is 255 characters.")
+    # A hostname consists of "labels" separated by dots.
+    host_part = hostname.split('.')[0]
+    if '_' in host_part:
+        # The host label cannot contain underscores; the rest of the name can.
+        raise ValidationError(
+            "Host label cannot contain underscore: %r." % host_part)
+    validate_domain_name(hostname)
 
 
 def get_ip_based_hostname(ip):

=== modified file 'src/maasserver/utils/tests/test_dns.py'
--- src/maasserver/utils/tests/test_dns.py	2015-03-26 00:12:12 +0000
+++ src/maasserver/utils/tests/test_dns.py	2015-04-03 16:20:24 +0000
@@ -13,6 +13,7 @@
 from django.core.exceptions import ValidationError
 from maasserver.utils.dns import (
     get_ip_based_hostname,
+    validate_domain_name,
     validate_hostname,
 )
 from testtools.matchers import (
@@ -70,12 +71,26 @@
         """Assertion: the validator rejects `hostname`."""
         self.assertRaises(ValidationError, validate_hostname, hostname)
 
+    def assertDomainValidatorAccepts(self, domain_name):
+        """Assertion: the validator rejects `domain_name`."""
+        try:
+            validate_domain_name(domain_name)
+        except ValidationError as e:
+            raise AssertionError(unicode(e))
+
+    def assertDomainValidatorRejects(self, hostname):
+        """Assertion: the validator rejects `hostname`."""
+        self.assertRaises(ValidationError, validate_domain_name, hostname)
+
     def test_accepts_ascii_letters(self):
         self.assertAccepts('abcde')
 
     def test_accepts_dots(self):
         self.assertAccepts('abc.def')
 
+    def test_accepts_subdomain(self):
+        self.assertAccepts('abc.def.ubuntu.com')
+
     def test_rejects_adjacent_dots(self):
         self.assertRejects('abc..def')
 
@@ -138,6 +153,11 @@
         # ASCII.
         self.assertRejects('\u03be')
 
+    def test_accepts_domain_underscores(self):
+        self.assertDomainValidatorAccepts('_foo')
+        self.assertDomainValidatorAccepts('_foo._bar')
+        self.assertDomainValidatorAccepts('_.o_O._')
+
 
 class TestIpBasedHostnameGenerator(MAASTestCase):