launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/metadata-auth into lp:maas with lp:~jtv/maas/reusable-error-middleware as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/metadata-auth/+merge/94169

Authorization for the metadata service.  Essential for knowing which node is requesting — and therefore which node the service should return data for.

As it stands, this leaves open an authorization issue: the node-init user will still be able to access the MaaS API.  But given the problems with Piston's oauth integration (it presents unauthorized requests to django middleware!) we're leaving that for a separate branch.
-- 
https://code.launchpad.net/~jtv/maas/metadata-auth/+merge/94169
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/metadata-auth into lp:maas.

=== modified file 'src/maas/api_auth.py'
--- src/maas/api_auth.py	2012-02-20 13:35:51 +0000
+++ src/maas/api_auth.py	2012-02-22 14:11:21 +0000
@@ -31,7 +31,7 @@
                 MaasAPIAuthentication, self).is_authenticated(request)
 
     def challenge(self):
-        # Beware: this returns 401: Unauthenticated, not 403: Forbidden
+        # Beware: this returns 401: Unauthorized, not 403: Forbidden
         # as the name implies.
         return rc.FORBIDDEN
 

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2012-02-21 01:01:10 +0000
+++ src/maas/development.py	2012-02-22 14:11:21 +0000
@@ -142,6 +142,7 @@
     'debug_toolbar.middleware.DebugToolbarMiddleware',
     'maasserver.middleware.AccessMiddleware',
     'maasserver.middleware.APIErrorsMiddleware',
+    'metadataserver.middleware.MetadataErrorsMiddleware',
 )
 
 ROOT_URLCONF = 'maas.urls'

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-02-21 01:01:10 +0000
+++ src/maas/settings.py	2012-02-22 14:11:21 +0000
@@ -33,7 +33,9 @@
 LOGOUT_URL = '/'
 LOGIN_REDIRECT_URL = '/'
 
-API_URL_REGEXP = '^/(api|metadata)/'
+API_URL_REGEXP = '^/api/'
+METADATA_URL_REGEXP = '^/metadata/'
+
 
 # We handle exceptions ourselves (in
 # maasserver.middleware.APIErrorsMiddleware)
@@ -157,6 +159,7 @@
     'django.contrib.messages.middleware.MessageMiddleware',
     'maasserver.middleware.AccessMiddleware',
     'maasserver.middleware.APIErrorsMiddleware',
+    'metadataserver.middleware.MetadataErrorsMiddleware',
 )
 
 ROOT_URLCONF = 'maas.urls'

=== modified file 'src/maasserver/testing/oauthclient.py'
--- src/maasserver/testing/oauthclient.py	2012-02-21 11:00:32 +0000
+++ src/maasserver/testing/oauthclient.py	2012-02-22 14:11:21 +0000
@@ -29,9 +29,20 @@
     """OAuth-authenticated client for Piston API testing.
     """
 
-    def __init__(self, user):
+    def __init__(self, user, token=None):
+        """Initialize an oauth-authenticated test client.
+
+        :param user: The user to authenticate.
+        :type user: django.contrib.auth.models.User
+        :param token: Optional token to authenticate `user` with.  If
+            no `token` is given, the user's first token will be used.
+        :type token: oauth.oauth.OAuthToken
+        """
         super(OAuthAuthenticatedClient, self).__init__()
-        token = user.get_profile().get_authorisation_tokens()[0]
+        if token is None:
+            # Get the user's first token.
+            token = user.get_profile().get_authorisation_tokens()[0]
+        assert token.user == user, "Token does not match User."
         consumer = token.consumer
         self.consumer = OAuthConsumer(str(consumer.key), str(consumer.secret))
         self.token = OAuthToken(str(token.key), str(token.secret))

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-02-16 14:09:01 +0000
+++ src/metadataserver/api.py	2012-02-22 14:11:21 +0000
@@ -10,18 +10,20 @@
 
 __metaclass__ = type
 __all__ = [
-    'metadata_index',
-    'meta_data',
-    'version_index',
-    'user_data',
+    'IndexHandler',
+    'MetaDataHandler',
+    'UserDataHandler',
+    'VersionIndexHandler',
     ]
 
 from django.http import HttpResponse
 from maasserver.exceptions import (
     MaasAPINotFound,
+    PermissionDenied,
     Unauthorized,
     )
 from metadataserver.models import NodeKey
+from piston.handler import BaseHandler
 
 
 class UnknownMetadataVersion(MaasAPINotFound):
@@ -39,7 +41,7 @@
         if len(key_value) == 2:
             key, value = key_value
             if key == 'oauth_token':
-                return value
+                return value.rstrip(',').strip('"')
     raise Unauthorized("No oauth token found for metadata request.")
 
 
@@ -49,7 +51,10 @@
     if auth_header is None:
         raise Unauthorized("No authorization header received.")
     key = extract_oauth_key(auth_header)
-    return NodeKey.objects.get_node_for_key(key)
+    try:
+        return NodeKey.objects.get_node_for_key(key)
+    except NodeKey.DoesNotExist:
+        raise PermissionDenied("Not authenticated as a known node.")
 
 
 def make_text_response(contents):
@@ -68,47 +73,74 @@
         raise UnknownMetadataVersion("Unknown metadata version: %s" % version)
 
 
-def metadata_index(request):
-    """View: top-level metadata listing."""
-    return make_list_response(['latest'])
-
-
-def version_index(request, version):
-    """View: listing for a given metadata version."""
-    check_version(version)
-    return make_list_response(['meta-data', 'user-data'])
-
-
-def retrieve_unknown_item(node, item_path):
-    """Retrieve meta-data: unknown sub-item."""
-    raise MaasAPINotFound("No such metadata item: %s" % '/'.join(item_path))
-
-
-def retrieve_local_hostname(node, item_path):
-    """Retrieve meta-data: local-hostname."""
-    return make_text_response(node.hostname)
-
-
-def meta_data(request, version, item=None):
-    """View: meta-data listing for a given version, or meta-data item."""
-    check_version(version)
-    node = get_node_for_request(request)
-
-    # Functions to retrieve meta-data items.
-    retrievers = {
-        'local-hostname': retrieve_local_hostname,
+class MetadataViewHandler(BaseHandler):
+    allowed_methods = ('GET',)
+
+    def read(self, request):
+        return make_list_response(self.fields)
+
+
+class IndexHandler(MetadataViewHandler):
+    """Top-level metadata listing."""
+
+    fields = ('latest',)
+
+
+class VersionIndexHandler(MetadataViewHandler):
+    """Listing for a given metadata version."""
+
+    fields = ('meta-data', 'user-data')
+
+    def read(self, request, version):
+        check_version(version)
+        return super(VersionIndexHandler, self).read(request)
+
+
+class MetaDataHandler(VersionIndexHandler):
+    """Meta-data listing for a given version."""
+
+    fields = ('local-hostname',)
+
+    def get_attribute_producer(self, item):
+        """Return a callable to deliver a given metadata item.
+
+        :param item: Sub-path for the attribute, e.g. "local-hostname" to
+            get a handler that returns the logged-in node's hostname.
+        :type item: basestring
+        :return: A callable that accepts as arguments the logged-in node;
+            the requested metadata version (e.g. "latest"); and `item`.  It
+            returns an HttpResponse.
+        :rtype: Callable
+        """
+        field = item.split('/')[0]
+        if field not in self.fields:
+            raise MaasAPINotFound("Unknown metadata attribute: %s" % field)
+
+        producers = {
+            'local-hostname': self.local_hostname,
         }
 
-    if not item:
-        return make_list_response(sorted(retrievers.keys()))
-
-    item_path = item.split('/')
-    retriever = retrievers.get(item_path[0], retrieve_unknown_item)
-    return retriever(node, item_path[1:])
-
-
-def user_data(request, version):
-    """View: user-data blob for a given version."""
-    check_version(version)
-    data = b"User data here."
-    return HttpResponse(data, mimetype='application/octet-stream')
+        return producers[field]
+
+    def read(self, request, version, item=None):
+        check_version(version)
+        if item is None or len(item) == 0:
+            # Requesting the list of attributes, not any particular
+            # attribute.
+            return super(VersionIndexHandler, self).read(request)
+
+        node = get_node_for_request(request)
+        producer = self.get_attribute_producer(item)
+        return producer(node, version, item)
+
+    def local_hostname(self, node, version, item):
+        return make_text_response(node.hostname)
+
+
+class UserDataHandler(MetadataViewHandler):
+    """User-data blob for a given version."""
+
+    def read(self, request, version):
+        check_version(version)
+        data = b"User data here."
+        return HttpResponse(data, mimetype='application/octet-stream')

=== modified file 'src/metadataserver/fixtures/initial_data.yaml'
--- src/metadataserver/fixtures/initial_data.yaml	2012-02-16 12:11:00 +0000
+++ src/metadataserver/fixtures/initial_data.yaml	2012-02-22 14:11:21 +0000
@@ -7,7 +7,7 @@
     email: ''
     password: '!'
     is_staff: false
-    is_active: true
+    is_active: false
     is_superuser: false
     last_login: 2012-02-16
     date_joined: 2012-02-16

=== added file 'src/metadataserver/middleware.py'
--- src/metadataserver/middleware.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/middleware.py	2012-02-22 14:11:21 +0000
@@ -0,0 +1,24 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Django "middlewares" for the metadata API."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'MetadataErrorsMiddleware',
+    ]
+
+from django.conf import settings
+
+from maasserver.middleware import ExceptionMiddleware
+
+
+class MetadataErrorsMiddleware(ExceptionMiddleware):
+    """Report exceptions from the metadata app as HTTP responses."""
+
+    path_regex = settings.METADATA_URL_REGEXP

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-21 12:20:00 +0000
+++ src/metadataserver/models.py	2012-02-22 14:11:21 +0000
@@ -42,7 +42,7 @@
         :return: Token for the node to use.  It will belong to the
             maas-init-node user.  If passed the token's key,
             `get_node_for_key` will return `node`.
-        :rtype: Token
+        :rtype: piston.models.Token
         """
         token = create_auth_token(get_node_init_user())
         self.create(node=node, key=token.key)

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-02-21 12:20:00 +0000
+++ src/metadataserver/tests/test_api.py	2012-02-22 14:11:21 +0000
@@ -11,90 +11,166 @@
 __metaclass__ = type
 __all__ = []
 
+from collections import namedtuple
 import httplib
+from random import randint
+from time import time
 
+from maasserver.exceptions import Unauthorized
 from maasserver.testing.factory import factory
+from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from maastesting import TestCase
 from metadataserver.api import (
     check_version,
+    extract_oauth_key,
+    get_node_for_request,
     make_list_response,
     make_text_response,
     UnknownMetadataVersion,
     )
 from metadataserver.models import NodeKey
+from metadataserver.nodeinituser import get_node_init_user
 
 
 class TestHelpers(TestCase):
     """Tests for the API helper functions."""
 
-    def make_text_response_presents_text_as_text_plain(self):
+    def make_oauth_header(self, **kwargs):
+        """Fake an OAuth authorization header.
+
+        This will use arbitrary values.  Pass as keyword arguments any
+        header items that you wish to override.
+        """
+        items = {
+            'realm': factory.getRandomString(),
+            'oauth_nonce': randint(0, 99999),
+            'oauth_timestamp': time(),
+            'oauth_consumer_key': factory.getRandomString(18),
+            'oauth_signature_method': 'PLAINTEXT',
+            'oauth_version': '1.0',
+            'oauth_token': factory.getRandomString(18),
+            'oauth_signature': "%%26%s" % factory.getRandomString(32),
+        }
+        items.update(kwargs)
+        return "OAuth " + ", ".join([
+            '%s="%s"' % (key, value) for key, value in items.items()])
+
+    def fake_request(self, **kwargs):
+        """Produce a cheap fake request, fresh from the sweat shop.
+
+        Pass as arguments any header items you want to include.
+        """
+        return namedtuple('FakeRequest', ['META'])(kwargs)
+
+    def test_make_text_response_presents_text_as_text_plain(self):
         input_text = "Hello."
         response = make_text_response(input_text)
         self.assertEqual('text/plain', response['Content-Type'])
         self.assertEqual(input_text, response.content)
 
-    def make_list_response_presents_list_as_newline_separated_text(self):
+    def test_make_list_response_presents_list_as_newline_separated_text(self):
         response = make_list_response(['aaa', 'bbb'])
         self.assertEqual('text/plain', response['Content-Type'])
         self.assertEqual("aaa\nbbb", response.content)
 
-    def check_version_accepts_latest(self):
+    def test_check_version_accepts_latest(self):
         check_version('latest')
         # The test is that we get here without exception.
         pass
 
-    def check_version_raises_UnknownMetadataVersion_for_unknown_version(self):
+    def test_check_version_reports_unknown_version(self):
         self.assertRaises(UnknownMetadataVersion, check_version, '1.0')
 
+    def test_extract_oauth_key_extracts_oauth_token_from_oauth_header(self):
+        token = factory.getRandomString(18)
+        self.assertEqual(
+            token,
+            extract_oauth_key(self.make_oauth_header(oauth_token=token)))
+
+    def test_extract_oauth_key_rejects_auth_without_oauth_key(self):
+        self.assertRaises(Unauthorized, extract_oauth_key, '')
+
+    def test_get_node_for_request_finds_node(self):
+        node = factory.make_node()
+        token = NodeKey.objects.create_token(node)
+        request = self.fake_request(
+            HTTP_AUTHORIZATION=self.make_oauth_header(oauth_token=token.key))
+        self.assertEqual(node, get_node_for_request(request))
+
+    def test_get_node_for_request_reports_missing_auth_header(self):
+        self.assertRaises(
+            Unauthorized,
+            get_node_for_request, self.fake_request())
+
 
 class TestViews(TestCase):
     """Tests for the API views."""
 
-    def get(self, path, **headers):
+    def make_node_client(self, node=None):
+        """Create a test client logged in as if it were `node`."""
+        if node is None:
+            node = factory.make_node()
+        token = NodeKey.objects.create_token(node)
+        return OAuthAuthenticatedClient(get_node_init_user(), token)
+
+    def get(self, path, client=None):
+        """GET a resource from the metadata API.
+
+        :param path: Path within the metadata API to access.  Should start
+            with a slash.
+        :param token: If given, authenticate the request using this token.
+        :type token: oauth.oauth.OAuthToken
+        :return: A response to the GET request.
+        :rtype: django.http.HttpResponse
+        """
+        assert path.startswith('/'), "Give absolute metadata API path."
+        if client is None:
+            client = self.client
         # Root of the metadata API service.
         metadata_root = "/metadata"
-        return self.client.get(metadata_root + path, **headers)
+        return client.get(metadata_root + path)
 
-    def make_node_and_auth_header(self):
-        node = factory.make_node()
-        token = NodeKey.objects.create_token(node)
-        header = 'oauth_token=%s' % token.key
-        return node, header
+    def test_no_anonymous_access(self):
+        self.assertEqual(httplib.UNAUTHORIZED, self.get('/').status_code)
 
     def test_metadata_index_shows_latest(self):
-        self.assertIn('latest', self.get('/').content)
+        client = self.make_node_client()
+        self.assertIn('latest', self.get('/', client).content)
 
     def test_metadata_index_shows_only_known_versions(self):
-        for item in self.get('/').content.splitlines():
+        client = self.make_node_client()
+        for item in self.get('/', client).content.splitlines():
             check_version(item)
         # The test is that we get here without exception.
         pass
 
     def test_version_index_shows_meta_data_and_user_data(self):
-        items = self.get('/latest/').content.splitlines()
+        client = self.make_node_client()
+        items = self.get('/latest/', client).content.splitlines()
         self.assertIn('meta-data', items)
         self.assertIn('user-data', items)
 
     def test_meta_data_view_returns_text_response(self):
-        self.assertIn(
-            'text/plain', self.get('/latest/meta-data/')['Content-Type'])
+        client = self.make_node_client()
+        response = self.get('/latest/meta-data/', client)
+        self.assertIn('text/plain', response['Content-Type'])
 
     def test_meta_data_unknown_item_is_not_found(self):
-        node, header = self.make_node_and_auth_header()
-        response = self.get(
-            '/latest/meta-data/UNKNOWN-ITEM-HA-HA-HA',
-            HTTP_AUTHORIZATION=header)
+        client = self.make_node_client()
+        response = self.get('/latest/meta-data/UNKNOWN-ITEM-HA-HA-HA', client)
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_meta_data_local_hostname(self):
-        node, header = self.make_node_and_auth_header()
-        response = self.get(
-            '/latest/meta-data/local-hostname', HTTP_AUTHORIZATION=header)
-        self.assertEqual(httplib.OK, response.status_code)
+        hostname = factory.getRandomString()
+        client = self.make_node_client(factory.make_node(hostname=hostname))
+        response = self.get('/latest/meta-data/local-hostname', client)
+        self.assertEqual(
+            (httplib.OK, hostname),
+            (response.status_code, response.content.decode('ascii')))
         self.assertIn('text/plain', response['Content-Type'])
-        self.assertEqual(node.hostname, response.content)
 
     def test_user_data_view_returns_binary_blob(self):
-        response = self.get('/latest/user-data')
+        client = self.make_node_client()
+        response = self.get('/latest/user-data', client)
         self.assertEqual('application/octet-stream', response['Content-Type'])
         self.assertIsInstance(response.content, str)

=== modified file 'src/metadataserver/urls.py'
--- src/metadataserver/urls.py	2012-02-15 23:38:49 +0000
+++ src/metadataserver/urls.py	2012-02-22 14:11:21 +0000
@@ -17,22 +17,33 @@
     patterns,
     url,
     )
+from maas.api_auth import api_auth
 from metadataserver.api import (
-    meta_data,
-    metadata_index,
-    user_data,
-    version_index,
+    IndexHandler,
+    MetaDataHandler,
+    UserDataHandler,
+    VersionIndexHandler,
     )
+from piston.resource import Resource
+
+
+meta_data_handler = Resource(MetaDataHandler, authentication=api_auth)
+user_data_handler = Resource(UserDataHandler, authentication=api_auth)
+version_index_handler = Resource(VersionIndexHandler, authentication=api_auth)
+index_handler = Resource(IndexHandler, authentication=api_auth)
 
 
 urlpatterns = patterns(
     '',
     url(
-        r'(?P<version>[^/]+)/meta-data/(?P<item>.*)$', meta_data,
+        r'(?P<version>[^/]+)/meta-data/(?P<item>.*)$',
+        meta_data_handler,
         name='metadata_meta_data'),
     url(
-        r'(?P<version>[^/]+)/user-data$', user_data,
+        r'(?P<version>[^/]+)/user-data$', user_data_handler,
         name='metadata_user_data'),
-    url(r'(?P<version>[^/]+)/', version_index, name='metadata_version'),
-    url(r'', metadata_index, name='metadata'),
+    url(
+        r'(?P<version>[^/]+)/', version_index_handler,
+        name='metadata_version'),
+    url(r'', index_handler, name='metadata'),
     )