Merge lp:~jtv/maas/bug-1300059 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Validate hostnames.

Until now, hostnames entered for nodes were left completely unvalidated. They could contain anything, valid or not. This branch provides a validator.

The validator does not support unicode, because MAAS doesn't. If you want internationalized domain names, you'll need to encode them yourself and enter the resulting Punycode into MAAS. We could at some point support unicode input, but I believe the changes would probably have to be more extensive than just a change in validator.

Jeroen

[Raphaël Badin (rvb)]

Looks good. As discussed on IRC, it means people with existing nodes with invalid labels will need to rename their nodes before they can do any modifications to them but:
a) the problem will be quite easy to identify (the validation error will be pretty clear)
b) I don't expect many people will have to face this problem

[0]

> If the hostname is not valid according to RFCs 952 and 1123.

I know a hostname is a hostname (as in "A Big Mac is a Big Mac") but I think this is worth repeating in a place that makes it to the documentation, i.e. in ":ivar hostname: This `Node`'s hostname." in src/maasserver/models/node.py:Node

[1]

41 + if len(label) > 63:
42 + raise ValidationError("Label is too long: '%s'." % label)

The error would be more informative if it mentioned the limit.

[2]

40 + raise ValidationError("Hostname contains empty label.")
41 + if len(label) > 63:
42 + raise ValidationError("Label is too long: '%s'." % label)

Do you think it's obvious to people what a "label", in this context, means? I must admit it was not obvious to me before I read your comments. Maybe "part"?

[3]

44 + raise ValidationError(
45 + "Label cannot start or end with hyphen: '%s'." % label)

You can use %r here, this saves you from having to include the single quotes around the string.

[4]

59 + hostname = CharField(
60 + max_length=255, default='', blank=True, unique=True,
61 + validators=[validate_hostname])

Once more, maybe mention the RFCs in the help_text.

review: Approve

[Jeroen T. Vermeulen (jtv)]

> [0]
>
> > If the hostname is not valid according to RFCs 952 and 1123.
>
> I know a hostname is a hostname (as in "A Big Mac is a Big Mac") but I think
> this is worth repeating in a place that makes it to the documentation, i.e. in
> ":ivar hostname: This `Node`'s hostname." in
> src/maasserver/models/node.py:Node

OK, I put it in the ivar documentation.

> [1]
>
> 41 + if len(label) > 63:
> 42 + raise ValidationError("Label is too long: '%s'." % label)
>
> The error would be more informative if it mentioned the limit.

Made the message look much like the one for the overall length.

> [2]
>
> 40 + raise ValidationError("Hostname contains empty label.")
> 41 + if len(label) > 63:
> 42 + raise ValidationError("Label is too long: '%s'." % label)
>
> Do you think it's obvious to people what a "label", in this context, means? I
> must admit it was not obvious to me before I read your comments. Maybe "part"?

It's not obvious, no. I changed it to say "name."

> [3]
>
> 44 + raise ValidationError(
> 45 + "Label cannot start or end with hyphen: '%s'." %
> label)
>
> You can use %r here, this saves you from having to include the single quotes
> around the string.

I never liked that, because it introduces magic characters that are neither in the format string nor in the argument. But if you prefer that, I'll change it.

> [4]
>
> 59 + hostname = CharField(
> 60 + max_length=255, default='', blank=True, unique=True,
> 61 + validators=[validate_hostname])
>
> Once more, maybe mention the RFCs in the help_text.

There is no help_text here, or anywhere in our models that I know of. I think that's because we normally have the help text in the form. Wouldn't that one override any help_text from the model? Anyway, the text we have in the form is already quite long and goes into the relationship between the hostname and the FQDN, which I think is much more important and potentially surprising. So I left this as it was.

Thanks for the review.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~jtv/maas/bug-1300059 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com trusty Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com trusty Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty/main Sources [1,076 kB]
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com trusty/universe Sources [6,405 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages [1,370 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages [5,869 kB]
Get:7 http://nova.clouds.archive.ubuntu.com trusty/main Translation-en [772 kB]
Get:8 http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en [4,046 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 19.6 MB in 7s (2,667 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr ...

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-04-02 16:40:39 +0000
+++ src/maasserver/models/node.py	2014-04-03 16:12:48 +0000
@@ -23,6 +23,7 @@
     repeat,
     )
 import random
+import re
 from string import whitespace
 from uuid import uuid1
 
@@ -152,6 +153,43 @@
     """Raised when a node has an unknown power type."""
 
 
+def validate_hostname(hostname):
+    """Validator for hostnames.
+
+    :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.
+    """
+    # 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.
+    label_chars = re.compile('[a-zA-Z0-9_-]*$')
+
+    if len(hostname) > 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])
+    for label in labels:
+        if len(label) == 0:
+            raise ValidationError("Hostname contains empty name.")
+        if len(label) > 63:
+            raise ValidationError(
+                "Name 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)
+        if not label_chars.match(label):
+            raise ValidationError(
+                "Name contains disallowed characters: %r." % label)
+
+
 class NodeManager(Manager):
     """A utility to manage the collection of Nodes."""
 
@@ -408,7 +446,7 @@
 
     :ivar system_id: The unique identifier for this `Node`.
         (e.g. 'node-41eba45e-4cfa-11e1-a052-00225f89f211').
-    :ivar hostname: This `Node`'s hostname.
+    :ivar hostname: This `Node`'s hostname.  Must conform to RFCs 952 and 1123.
     :ivar status: This `Node`'s status. See the vocabulary
         :class:`NODE_STATUS`.
     :ivar owner: This `Node`'s owner if it's in use, None otherwise.
@@ -428,7 +466,9 @@
         max_length=41, unique=True, default=generate_node_system_id,
         editable=False)
 
-    hostname = CharField(max_length=255, default='', blank=True, unique=True)
+    hostname = CharField(
+        max_length=255, default='', blank=True, unique=True,
+        validators=[validate_hostname])
 
     status = IntegerField(
         max_length=10, choices=NODE_STATUS_CHOICES, editable=False,

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-03-27 04:15:45 +0000
+++ src/maasserver/models/tests/test_node.py	2014-04-03 16:12:48 +0000
@@ -40,6 +40,7 @@
 from maasserver.models.node import (
     generate_hostname,
     NODE_TRANSITIONS,
+    validate_hostname,
     )
 from maasserver.models.user import create_auth_token
 from maasserver.testing import reload_object
@@ -83,6 +84,114 @@
         self.assertEqual(sizes, [len(hostname) for hostname in hostnames])
 
 
+class TestHostnameValidator(MAASTestCase):
+    """Tests for the validation of hostnames.
+
+    Specifications based on:
+        http://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_host_names
+
+    This does not support Internationalized Domain Names.  To do so, we'd have
+    to accept and store unicode, but use the Punycode-encoded version.  The
+    validator would have to validate both versions: the unicode input for
+    invalid characters, and the encoded version for length.
+    """
+    def make_maximum_hostname(self):
+        """Create a hostname of the maximum permitted length.
+
+        The maximum permitted length is 255 characters.  The last label in the
+        hostname will not be of the maximum length, so tests can still append a
+        character to it without creating an invalid label.
+
+        The hostname is not randomised, so do not count on it being unique.
+        """
+        # A hostname may contain any number of labels, separated by dots.
+        # Each of the labels has a maximum length of 63 characters, so this has
+        # to be built up from multiple labels.
+        ten_chars = ('a' * 9) + '.'
+        hostname = ten_chars * 25 + ('b' * 5)
+        self.assertEqual(255, len(hostname))
+        return hostname
+
+    def assertAccepts(self, hostname):
+        """Assertion: the validator accepts `hostname`."""
+        try:
+            validate_hostname(hostname)
+        except ValidationError as e:
+            raise AssertionError(unicode(e))
+
+    def assertRejects(self, hostname):
+        """Assertion: the validator rejects `hostname`."""
+        self.assertRaises(ValidationError, validate_hostname, hostname)
+
+    def test_accepts_ascii_letters(self):
+        self.assertAccepts('abcde')
+
+    def test_accepts_dots(self):
+        self.assertAccepts('abc.def')
+
+    def test_rejects_adjacent_dots(self):
+        self.assertRejects('abc..def')
+
+    def test_rejects_leading_dot(self):
+        self.assertRejects('.abc')
+
+    def test_rejects_trailing_dot(self):
+        self.assertRejects('abc.')
+
+    def test_accepts_ascii_digits(self):
+        self.assertAccepts('abc123')
+
+    def test_accepts_leading_digits(self):
+        # Leading digits used to be forbidden, but are now allowed.
+        self.assertAccepts('123abc')
+
+    def test_rejects_whitespace(self):
+        self.assertRejects('a b')
+        self.assertRejects('a\nb')
+        self.assertRejects('a\tb')
+
+    def test_rejects_other_ascii_characters(self):
+        self.assertRejects('a?b')
+        self.assertRejects('a!b')
+        self.assertRejects('a,b')
+        self.assertRejects('a:b')
+        self.assertRejects('a;b')
+        self.assertRejects('a+b')
+        self.assertRejects('a=b')
+
+    def test_accepts_underscore_in_domain(self):
+        self.assertAccepts('host.local_domain')
+
+    def test_rejects_underscore_in_host(self):
+        self.assertRejects('host_name.local')
+
+    def test_accepts_hyphen(self):
+        self.assertAccepts('a-b')
+
+    def test_rejects_hyphen_at_start_of_label(self):
+        self.assertRejects('-ab')
+
+    def test_rejects_hyphen_at_end_of_label(self):
+        self.assertRejects('ab-')
+
+    def test_accepts_maximum_valid_length(self):
+        self.assertAccepts(self.make_maximum_hostname())
+
+    def test_rejects_oversized_hostname(self):
+        self.assertRejects(self.make_maximum_hostname() + 'x')
+
+    def test_accepts_maximum_label_length(self):
+        self.assertAccepts('a' * 63)
+
+    def test_rejects_oversized_label(self):
+        self.assertRejects('b' * 64)
+
+    def test_rejects_nonascii_letter(self):
+        # The \u03be is the Greek letter xi.  Perfectly good letter, just not
+        # ASCII.
+        self.assertRejects('\u03be')
+
+
 class NodeTest(MAASServerTestCase):
 
     def test_system_id(self):
@@ -94,6 +203,12 @@
         self.assertEqual(len(node.system_id), 41)
         self.assertTrue(node.system_id.startswith('node-'))
 
+    def test_hostname_is_validated(self):
+        bad_hostname = '-_?!@*-'
+        self.assertRaises(
+            ValidationError,
+            factory.make_node, hostname=bad_hostname)
+
     def test_work_queue_returns_nodegroup_uuid(self):
         nodegroup = factory.make_node_group()
         node = factory.make_node(nodegroup=nodegroup)
@@ -217,14 +332,14 @@
         Config.objects.set_config("enlistment_domain", '')
         existing_node = factory.make_node(hostname='hostname')
 
-        hostnames = [existing_node.hostname, "new_hostname"]
+        hostnames = [existing_node.hostname, "new-hostname"]
         self.patch(
             node_module, "generate_hostname",
             lambda size: hostnames.pop(0))
 
         node = factory.make_node()
         node.set_random_hostname()
-        self.assertEqual('new_hostname', node.hostname)
+        self.assertEqual('new-hostname', node.hostname)
 
     def test_get_effective_power_type_raises_if_not_set(self):
         node = factory.make_node(power_type='')

=== modified file 'src/maasserver/tests/test_api_node.py'
--- src/maasserver/tests/test_api_node.py	2014-03-06 01:29:56 +0000
+++ src/maasserver/tests/test_api_node.py	2014-04-03 16:12:48 +0000
@@ -503,14 +503,12 @@
             hostname='diane', owner=self.logged_in_user,
             architecture=make_usable_architecture(self))
         response = self.client_put(
-            self.get_node_uri(node), {'hostname': 'too long' * 100})
+            self.get_node_uri(node), {'hostname': '.'})
         parsed_result = json.loads(response.content)
 
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
         self.assertEqual(
-            {'hostname':
-                ['Ensure this value has at most 255 characters '
-                 '(it has 800).']},
+            {'hostname': ["Hostname contains empty name."]},
             parsed_result)
 
     def test_PUT_refuses_to_update_invisible_node(self):