Merge lp:~rvb/maas/pxe-off-preseed into lp:maas/trunk

[Raphaël Badin]

Add an anonymous method to turn off PXE booting. Use that method in the preseed template.

This branch adds an anonymous method to turn off PXE booting (to the metadata service) and uses that method to generate the url put in the preseed file. That url will be called by the node once at the end of the installation process.

= Pre-imp =

This is part of the plan which was devised last week with Gavin (see http://bit.ly/KXW5ZU for details) and I've had a pre-imp call with Julian about this.

= Notes =

- I've filed bug 1015559 about the fact that the new method does not require authentication. The bug is referenced in the code.

- The call to "wget" is now a POST request. Cobbler uses a GET request to modify data :(.

- Drive-by fix: I've fixed the value of 'server_name' in the context used when rendering the preseed file. It now uses the method get_maas_server_host.

[Francesco Banconi (frankban)]

The code looks good Raphaël.
Just 3 details/suggestions for the absolute_reverse function:

1. :param `view_name` can also be a view callable reference.
2. maybe we could continue supporting the other kwargs that can be passed to `reverse`, i.e. `urlconf` and `current_app`.
3. you can take into consideration `urlparse.urljoin`

review: Approve (code*)

[Gavin Panella (allenap)]

Francesco wrote:
> The code looks good Raphaël.
> Just 3 details/suggestions for the absolute_reverse function:
>
> 1. :param `view_name` can also be a view callable reference.

Took a (small) bit of hunting around to find what you were referring
to. Including some context would be useful when writing a review. It's
useful also when reading comments after the preview diff has been
updated with newer changes.

> 2. maybe we could continue supporting the other kwargs that can be
> passed to `reverse`, i.e. `urlconf` and `current_app`.

It looks like they are accepted... or am I missing something?

> 3. you can take into consideration `urlparse.urljoin`

+1

I have some other comments, but they're all minor.

[1]

+ wget "{{node_disable_pxe_url}}" --post-data "{{node_disable_pxe_data}}" -O /dev/null && \

These parameters should probably be escaped with pipes.quote(), a bit
like ShellTemplate does by default (though, for preseeds, not by
default).

Adding, say, an "escape" object into the template's namespace might be
an interesting approach here, then you could write:

    wget "{{node_disable_pxe_url | escape.shell}}" ...

or, for example:

    {{something_else | escape.html}}

But that's just an idea off the top of my head.

[2]

+def absolute_reverse(view_name, *args, **kwargs):

Not tempted to call it canonical_reverse()? ;)

[3]

+ response = self.client.post(
+ anon_netboot_off_url, {'op': 'netboot_off'})

The phrase "netboot off" does not contain a verb. It might be clearer
if it did, "disable netboot" or "switch netboot off" for example. Or,
for bonus points, make it RESTfulish and permit something like:

  wget ".../path/to/node?op=update" --post-data '{"netboot": false}'

[4]

+ # XXX: rvb 2012-06-20 bug=1015559: This method is accessible
+ # without authentication. This is a security threat.

I guess it's a denial of service vulnerability. It won't leak
information or permit unrelated modifications, so it's relatively
minor. I've updated the bug.

review: Approve

[Francesco Banconi (frankban)]

> Took a (small) bit of hunting around to find what you were referring
> to. Including some context would be useful when writing a review. It's
> useful also when reading comments after the preview diff has been
> updated with newer changes.

Good point Gavin, thank you.

> > 2. maybe we could continue supporting the other kwargs that can be
> > passed to `reverse`, i.e. `urlconf` and `current_app`.
>
> It looks like they are accepted... or am I missing something?

No you are not, my fault, they are already accepted.

[lp:~rvb/maas/pxe-off-preseed]

660. By Raphaël Badin on 2012-06-20

Use urlparse.urljoin.

[Raphaël Badin (rvb)]

Thanks for the review Francesco!

Like Gavin said, I think it's really worth it to add a little of context when you're suggesting something. My suggestion would be to simply include 1 or 2 lines from the diff. Like:

35 + server_host = get_maas_server_host()
36 + # Create the url and the url-data (POST parameters) used to turn off

> 1. :param `view_name` can also be a view callable reference.

Indeed, I've updated the docstring.

> 2. maybe we could continue supporting the other kwargs that can be passed to
> `reverse`, i.e. `urlconf` and `current_app`.

It's already done actually, they will be passed down to Django's reverse method as part of *args, **kwargs.

> 3. you can take into consideration `urlparse.urljoin`

Good idea, done.

[lp:~rvb/maas/pxe-off-preseed]

661. By Raphaël Badin on 2012-06-20

Fix doc.

662. By Raphaël Badin on 2012-06-20

Add escape filter.

[Raphaël Badin (rvb)]

Thanks for the review!

> [1]
>
> + wget "{{node_disable_pxe_url}}" --post-data "{{node_disable_pxe_data}}"
> -O /dev/null && \
>
> These parameters should probably be escaped with pipes.quote(), a bit
> like ShellTemplate does by default (though, for preseeds, not by
> default).
>
> Adding, say, an "escape" object into the template's namespace might be
> an interesting approach here, then you could write:
>
> wget "{{node_disable_pxe_url | escape.shell}}" ...
>
> or, for example:
>
> {{something_else | escape.html}}
>
> But that's just an idea off the top of my head.

That's an excellent idea, I've implemented it. It also justifies the existence of the specialized template class PreseedTemplate.

> [2]

> Not tempted to call it canonical_reverse()? ;)

Well… no :)

> [3]
>
> + response = self.client.post(
> + anon_netboot_off_url, {'op': 'netboot_off'})
>
> The phrase "netboot off" does not contain a verb. It might be clearer
> if it did, "disable netboot" or "switch netboot off" for example. Or,
> for bonus points, make it RESTfulish and permit something like:
>
> wget ".../path/to/node?op=update" --post-data '{"netboot": false}'

Currently, this is in sync with the authenticated version of the same API method. I'll check with Julian who's created the original implementation before I change that.

> [4]
>
> + # XXX: rvb 2012-06-20 bug=1015559: This method is accessible
> + # without authentication. This is a security threat.
>
> I guess it's a denial of service vulnerability. It won't leak
> information or permit unrelated modifications, so it's relatively
> minor. I've updated the bug.

Right, thanks.

[lp:~rvb/maas/pxe-off-preseed]

663. By Raphaël Badin on 2012-06-20

Fix doc.

664. By Raphaël Badin on 2012-06-20

Fix class name.

[Julian Edwards (julian-edwards)]

On Wednesday 20 June 2012 16:27:21 you wrote:
> > [3]
> >
> >
> >
> > + response = self.client.post(
> > + anon_netboot_off_url, {'op': 'netboot_off'})
> >
> >
> >
> > The phrase "netboot off" does not contain a verb. It might be clearer
> > if it did, "disable netboot" or "switch netboot off" for example. Or,
> >
> > for bonus points, make it RESTfulish and permit something like:
> >
> > wget ".../path/to/node?op=update" --post-data '{"netboot": false}'
>
> Currently, this is in sync with the authenticated version of the same API
> method. I'll check with Julian who's created the original implementation
> before I change that.

I thought long and hard about whether to have a single operation with multiple
parameters.

In the end I went with two, "netboot_on" and "netboot_off". I guess they are
not verbish enough, but the rationale was that it would be easier for shell
scripts to pass fewer args and it's also more consistent with Cobbler (which
may be considered bad :D)

Making it RESTful-ish (-ish making me laugh here) is an interesting one. If
we want this anonymous for now it may be better to move this entirely over to
the maasserver API? The only reason I put it on the metadata server is
because of the existing OAuth key arrangement.

Thoughts?

=== modified file 'contrib/preseeds_v2/generic'
--- contrib/preseeds_v2/generic	2012-06-19 16:17:12 +0000
+++ contrib/preseeds_v2/generic	2012-06-20 16:31:19 +0000
@@ -19,6 +19,6 @@
 # Executes late command and disables PXE.
 d-i	preseed/late_command string true && \
     in-target sh -c 'f=$1; shift; echo $0 > $f && chmod 0440 $f $*' 'ubuntu ALL=(ALL) NOPASSWD: ALL' /etc/sudoers.d/maas && \
-    wget "{{node_disable_pxe_url}}" -O /dev/null && \
+    wget "{{node_disable_pxe_url|escape.shell}}" --post-data "{{node_disable_pxe_data|escape.shell}}" -O /dev/null && \
     true
 {{enddef}}

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-06-19 16:01:07 +0000
+++ src/maasserver/preseed.py	2012-06-20 16:31:19 +0000
@@ -12,12 +12,16 @@
 __metaclass__ = type
 __all__ = []
 
+from collections import namedtuple
 from os.path import join
+from pipes import quote
+from urllib import urlencode
 from urlparse import urlparse
 
 from django.conf import settings
 from maasserver.models import Config
 from maasserver.provisioning import compose_preseed
+from maasserver.utils import absolute_reverse
 import tempita
 
 
@@ -89,8 +93,23 @@
         return None, None
 
 
+def get_escape_singleton():
+    """Return a singleton containing methods to escape various formats used in
+    the preseed templates.
+    """
+    Escape = namedtuple('Escape', 'shell')
+    return Escape(shell=quote)
+
+
 class PreseedTemplate(tempita.Template):
-    """A Tempita template specialised for preseed rendering."""
+    """A Tempita template specialised for preseed rendering.
+
+    It provides a filter named 'escape' which contains methods to escape
+    various formats used in the template."""
+
+    default_namespace = dict(
+        tempita.Template.default_namespace,
+        escape=get_escape_singleton())
 
 
 class TemplateNotFoundError(Exception):
@@ -145,13 +164,19 @@
     :return: The context dictionary.
     :rtype: dict.
     """
-    server_host = ''
+    server_host = get_maas_server_host()
+    # Create the url and the url-data (POST parameters) used to turn off
+    # PXE booting once the install of the node is finished.
+    node_disable_pxe_url = absolute_reverse(
+        'metadata-anon-node-edit', args=['latest', node.system_id])
+    node_disable_pxe_data = urlencode({'op': 'netboot_off'})
     return {
         'node': node,
         'release': release,
         'server_host': server_host,
         'preseed_data': compose_preseed(node),
-        'node_disable_pxe_url': '',  # TODO.
+        'node_disable_pxe_url': node_disable_pxe_url,
+        'node_disable_pxe_data': node_disable_pxe_data,
     }
 
 

=== modified file 'src/maasserver/tests/test_preseed.py'
--- src/maasserver/tests/test_preseed.py	2012-06-19 16:01:07 +0000
+++ src/maasserver/tests/test_preseed.py	2012-06-20 16:31:19 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 import os
+from pipes import quote
 
 from django.conf import settings
 from maasserver.models import Config
@@ -304,10 +305,20 @@
         context = get_preseed_context(node, release)
         self.assertItemsEqual(
             ['node', 'release', 'server_host', 'preseed_data',
-             'node_disable_pxe_url'],
+             'node_disable_pxe_url', 'node_disable_pxe_data'],
             context)
 
 
+class TestPreseedTemplate(TestCase):
+    """Tests for class:`PreseedTemplate`."""
+
+    def test_escape_shell(self):
+        template = PreseedTemplate("{{var|escape.shell}}")
+        var = "$ ! ()"
+        observed = template.substitute(var=var)
+        self.assertEqual(quote(var), observed)
+
+
 class TestRenderPreseed(TestCase):
     """Tests for `render_preseed`.
 

=== modified file 'src/maasserver/tests/test_utils.py'
--- src/maasserver/tests/test_utils.py	2012-04-30 16:26:38 +0000
+++ src/maasserver/tests/test_utils.py	2012-06-20 16:31:19 +0000
@@ -12,7 +12,14 @@
 __metaclass__ = type
 __all__ = []
 
-from maasserver.utils import map_enum
+from django.conf import settings
+from django.core.urlresolvers import reverse
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase as DjangoTestCase
+from maasserver.utils import (
+    absolute_reverse,
+    map_enum,
+    )
 from maastesting.testcase import TestCase
 
 
@@ -49,3 +56,28 @@
             THREE = 3
 
         self.assertEqual({'ONE': 1, 'THREE': 3}, map_enum(Enum))
+
+
+class TestAbsoluteReverse(DjangoTestCase):
+
+    def test_absolute_reverse_uses_DEFAULT_MAAS_URL(self):
+        maas_url = 'http://%s' % factory.getRandomString()
+        self.patch(settings, 'DEFAULT_MAAS_URL', maas_url)
+        absolute_url = absolute_reverse('settings')
+        expected_url = settings.DEFAULT_MAAS_URL + reverse('settings')
+        self.assertEqual(expected_url, absolute_url)
+
+    def test_absolute_reverse_uses_kwargs(self):
+        node = factory.make_node()
+        self.patch(settings, 'DEFAULT_MAAS_URL', '')
+        absolute_url = absolute_reverse(
+            'node-view', kwargs={'system_id': node.system_id})
+        expected_url = reverse('node-view', args=[node.system_id])
+        self.assertEqual(expected_url, absolute_url)
+
+    def test_absolute_reverse_uses_args(self):
+        node = factory.make_node()
+        self.patch(settings, 'DEFAULT_MAAS_URL', '')
+        absolute_url = absolute_reverse('node-view', args=[node.system_id])
+        expected_url = reverse('node-view', args=[node.system_id])
+        self.assertEqual(expected_url, absolute_url)

=== modified file 'src/maasserver/utils.py'
--- src/maasserver/utils.py	2012-06-15 05:07:40 +0000
+++ src/maasserver/utils.py	2012-06-20 16:31:19 +0000
@@ -11,11 +11,17 @@
 
 __metaclass__ = type
 __all__ = [
+    'absolute_reverse',
     'get_db_state',
     'ignore_unused',
     'map_enum',
     ]
 
+from urlparse import urljoin
+
+from django.conf import settings
+from django.core.urlresolvers import reverse
+
 
 def get_db_state(instance, field_name):
     """Get the persisted state of a given field for a given model instance.
@@ -53,3 +59,18 @@
         for key, value in vars(enum_class).items()
             if not key.startswith('_')
     }
+
+
+def absolute_reverse(view_name, *args, **kwargs):
+    """Return the absolute URL (i.e. including the URL scheme specifier and
+    the network location of the MAAS server).  Internally this method simply
+    calls Django's 'reverse' method and prefixes the result of that call with
+    the configured DEFAULT_MAAS_URL.
+
+    :param view_name: Django's view function name/reference or URL pattern
+        name for which to compute the absolute URL.
+    :param args: Positional arguments for Django's 'reverse' method.
+    :param kwargs: Named arguments for Django's 'reverse' method.
+    """
+    return urljoin(
+        settings.DEFAULT_MAAS_URL, reverse(view_name, *args, **kwargs))

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-06-18 04:46:17 +0000
+++ src/metadataserver/api.py	2012-06-20 16:31:19 +0000
@@ -11,6 +11,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'AnonMetaDataHandler',
     'IndexHandler',
     'MetaDataHandler',
     'UserDataHandler',
@@ -20,6 +21,7 @@
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
+from django.shortcuts import get_object_or_404
 from maasserver.api import (
     api_exported,
     api_operations,
@@ -37,6 +39,7 @@
     )
 from maasserver.models import (
     MACAddress,
+    Node,
     SSHKey,
     )
 from metadataserver.models import (
@@ -321,3 +324,20 @@
                 mimetype='application/octet-stream')
         except NodeUserData.DoesNotExist:
             raise MAASAPINotFound("No user data available for this node.")
+
+
+@api_operations
+class AnonMetaDataHandler(VersionIndexHandler):
+    """Anonymous metadata."""
+
+    @api_exported('POST')
+    def netboot_off(self, request, version=None, system_id=None):
+        """Turn off netboot on the node.
+
+        A commissioning node can call this to turn off netbooting when
+        it finishes installing itself.
+        """
+        node = get_object_or_404(Node, system_id=system_id)
+        node.netboot = False
+        node.save()
+        return rc.ALL_OK

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-06-18 05:09:25 +0000
+++ src/metadataserver/tests/test_api.py	2012-06-20 16:31:19 +0000
@@ -541,3 +541,15 @@
         response = client.post(self.make_url('/latest/'), {'op': 'netboot_on'})
         node = reload_object(node)
         self.assertTrue(node.netboot, response)
+
+    def test_anonymous_netboot_off(self):
+        node = factory.make_node(netboot=True)
+        anon_netboot_off_url = self.make_url(
+            '/latest/%s/edit/' % node.system_id)
+        response = self.client.post(
+            anon_netboot_off_url, {'op': 'netboot_off'})
+        node = reload_object(node)
+        self.assertEqual(
+            (httplib.OK, False),
+            (response.status_code, node.netboot),
+            response)

=== modified file 'src/metadataserver/urls.py'
--- src/metadataserver/urls.py	2012-06-06 03:11:48 +0000
+++ src/metadataserver/urls.py	2012-06-20 16:31:19 +0000
@@ -20,6 +20,7 @@
     )
 from maasserver.api_auth import api_auth
 from metadataserver.api import (
+    AnonMetaDataHandler,
     IndexHandler,
     MetaDataHandler,
     UserDataHandler,
@@ -34,6 +35,10 @@
 index_handler = Resource(IndexHandler, authentication=api_auth)
 
 
+# Handlers for anonymous node operations.
+meta_data_node_anon_handler = Resource(AnonMetaDataHandler)
+
+
 # Handlers for anonymous random metadata access.
 meta_data_by_mac_handler = Resource(MetaDataHandler)
 user_data_by_mac_handler = Resource(UserDataHandler)
@@ -56,6 +61,18 @@
     url(r'', index_handler, name='metadata'),
     )
 
+# Anonymous random metadata access.  These serve requests from the nodes
+# which happen when the environment is so minimal that proper
+# authenticated calls are not possible.
+anon_patterns = patterns(
+    '',
+    # XXX: rvb 2012-06-20 bug=1015559:  This method is accessible
+    # without authentication.  This is a security threat.
+    url(
+        r'(?P<version>[^/]+)/(?P<system_id>[\w\-]+)/edit/$',
+        meta_data_node_anon_handler,
+        name='metadata-anon-node-edit'),
+    )
 
 # Anonymous random metadata access keyed by MAC address.  These won't
 # work unless ALLOW_ANONYMOUS_METADATA_ACCESS is enabled, which you
@@ -80,4 +97,4 @@
 # URL patterns.  The anonymous patterns are listed first because they're
 # so recognizable: there's no chance of a regular metadata access being
 # mistaken for one of these based on URL pattern match.
-urlpatterns = by_mac_patterns + node_patterns
+urlpatterns = anon_patterns + by_mac_patterns + node_patterns