launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/metadata-node-user into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/metadata-node-user/+merge/93320

Boy, I hope X doesn't crash while I write this merge proposal.  (Update: actually I had a low-power warning followed by a shutdown.  But the browser remembered what I was typing!)

Nodes will authenticate for accessing the metadata service.  The metadata service will provide metadata based on the authenticated node (not on, say, a RARP lookup of the requesting IP address — much too fragile).

Raphaël and I discussed this, and it turns out there is no need to have a separate User per Node as we feared.  Instead, we can have a special user that will have one set of OAuth tokens for each node.  We keep it in a stateless wrapper class that knows about the mapping between keys and nodes.  The mapping itself lives in a new very simple model class, NodeKey.

The process of commissioning and deploying a node will, at some point, create a key for the node and seed the whole oauth token (token key, token secret, consumer key, and the “consumer secret” which I can tell you and the rest of the internet in confidence will be the empty string).  There's no need to store all of that in NodeKey, as far as I can see; the request header's oauth_key should uniquely identify a node's key.  We just need to get it to cloud-init.

This may not be enough to get authentication working for the metadata API, since its implementation is currently not based on Piston whose OAuth we use.  Should be easy enough to fix — but in a separate branch please.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/metadata-node-user/+merge/93320
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/metadata-node-user into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-02-14 16:05:46 +0000
+++ src/maasserver/models.py	2012-02-15 23:28:43 +0000
@@ -35,6 +35,13 @@
     )
 
 
+# User names reserved to the system.
+SYSTEM_USERS = [
+    # For nodes' access to the metadata API:
+    'maas-node-init',
+    ]
+
+
 class CommonInfo(models.Model):
     """A base model which records the creation date and the last modification
     date.
@@ -383,6 +390,30 @@
 GENERIC_CONSUMER = 'Maas consumer'
 
 
+def create_auth_token(user):
+    """Create new Token and Consumer (OAuth authorisation) for `user`.
+
+    :param user: The user to create a token for.
+    :type user: User
+    :return: A tuple containing the Consumer and the Token that were
+        created.
+    :rtype: tuple
+
+    """
+    consumer = Consumer.objects.create(
+        user=user, name=GENERIC_CONSUMER, status='accepted')
+    consumer.generate_random_codes()
+    # This is a 'generic' consumer aimed to service many clients, hence
+    # we don't authenticate the consumer with key/secret key.
+    consumer.secret = ''
+    consumer.save()
+    token = Token.objects.create(
+        user=user, token_type=Token.ACCESS, consumer=consumer,
+        is_approved=True)
+    token.generate_random_codes()
+    return consumer, token
+
+
 class UserProfile(models.Model):
     """A User profile to store Maas specific methods and fields.
 
@@ -418,18 +449,7 @@
         :rtype: tuple
 
         """
-        consumer = Consumer.objects.create(
-            user=self.user, name=GENERIC_CONSUMER, status='accepted')
-        consumer.generate_random_codes()
-        # This is a 'generic' consumer aimed to service many clients, hence
-        # we don't authenticate the consumer with key/secret key.
-        consumer.secret = ''
-        consumer.save()
-        token = Token.objects.create(
-            user=self.user, token_type=Token.ACCESS, consumer=consumer,
-            is_approved=True)
-        token.generate_random_codes()
-        return consumer, token
+        return create_auth_token(self.user)
 
     def delete_authorisation_token(self, token_key):
         """Delete the user's OAuth token wich key token_key.
@@ -448,7 +468,7 @@
 # When a user is created: create the related profile and the default
 # consumer/token.
 def create_user(sender, instance, created, **kwargs):
-    if created:
+    if created and instance.username not in SYSTEM_USERS:
         # Create related UserProfile.
         profile = UserProfile.objects.create(user=instance)
 

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-02-14 15:53:08 +0000
+++ src/maasserver/tests/test_models.py	2012-02-15 23:28:43 +0000
@@ -24,6 +24,7 @@
     Node,
     NODE_STATUS,
     NODE_STATUS_CHOICES_DICT,
+    SYSTEM_USERS,
     UserProfile,
     )
 from maasserver.testing.factory import factory
@@ -277,12 +278,17 @@
         self.assertEqual(KEY_SIZE, len(consumer.key))
         self.assertEqual('', consumer.secret)
 
-    def test_profile_creation(self):
-        # A profile is created each time a user is created.
+    def test_profile_is_created_for_regular_user(self):
+        # A profile is created each time a normal user is created.
         user = factory.make_user()
         self.assertIsInstance(user.get_profile(), UserProfile)
         self.assertEqual(user, user.get_profile().user)
 
+    def test_profile_is_not_created_for_system_user(self):
+        # No profile is created for a system user.
+        users = [factory.make_user(username=name) for name in SYSTEM_USERS]
+        self.assertItemsEqual([], UserProfile.objects.filter(user__in=users))
+
     def test_consumer_creation(self):
         # A generic consumer is created each time a user is created.
         user = factory.make_user()

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-07 18:14:14 +0000
+++ src/metadataserver/models.py	2012-02-15 23:28:43 +0000
@@ -10,8 +10,19 @@
 
 __metaclass__ = type
 __all__ = [
+    'NodeKey',
     ]
 
-#from django.db import models
-
-# Nothing here yet.
+from django.db.models import (
+    CharField,
+    ForeignKey,
+    Model,
+    )
+from maasserver.models import Node
+from piston.models import KEY_SIZE
+
+
+class NodeKey(Model):
+    """Associate a Node with its OAuth (token) key."""
+    node = ForeignKey(Node, null=False, editable=False)
+    key = CharField(max_length=KEY_SIZE, null=False, editable=False)

=== added file 'src/metadataserver/nodeinituser.py'
--- src/metadataserver/nodeinituser.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/nodeinituser.py	2012-02-15 23:28:43 +0000
@@ -0,0 +1,72 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""User management for nodes' access to the metadata service."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'NodeInitUser',
+    ]
+
+from django.contrib.auth.models import User
+from maasserver.models import create_auth_token
+from metadataserver.models import NodeKey
+
+
+class NodeInitUser:
+    """Wrapper for the maas-node-init user.
+
+    This is the "user" that makes metadata requests on behalf of the nodes.
+    Each node logs in as this user, but each using its own token.
+
+    All instances of `NodeInitUser` share the same `User` identity.   There
+    is no mutable state in this class.
+
+    :ivar user_name: The reserved user name for this special user.
+    :ivar user: The User object, once it has been loaded or created.
+    """
+    user_name = 'maas-node-init'
+    user = None
+
+    def __init__(self):
+        """Do not instantiate these yourself; rely on `get` instead."""
+        existing_user = list(
+            User.objects.filter(username=NodeInitUser.user_name))
+        if existing_user:
+            # Special user already existed in the database.
+            [self.user] = existing_user
+        else:
+            # Create special user.
+            # Django won't let us create a user without email address,
+            # so unfortunately we _have_ to make one up.
+            self.user = User.objects.create_user(
+                username=self.user_name, email='sample@xxxxxxxxxxx')
+
+    def create_token(self, node):
+        """Create an OAuth token for a given node.
+
+        The node will be able to use this information for accessing the
+        metadata service.  It will see its own, custom metadata.
+
+        :param node: The system that is to be allowed access to the metadata
+            service under the node init user's identity.
+        :type node: Node
+        :return: Consumer and Token for the node to use.  If passed the
+            token's key, `self.get_node_for_key` will return `node`.
+        :rtype: tuple
+        """
+        consumer, token = create_auth_token(self.user)
+        NodeKey.objects.create(node=node, key=token.key).save()
+        return consumer, token
+
+    def get_node_for_key(self, key):
+        """Find the `Node` that `key` was created for.
+
+        :raise NodeKey.DoesNotExist: if `key` is not associated with any node.
+        """
+        return NodeKey.objects.get(key=key).node

=== added file 'src/metadataserver/tests/test_nodeinituser.py'
--- src/metadataserver/tests/test_nodeinituser.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/tests/test_nodeinituser.py	2012-02-15 23:28:43 +0000
@@ -0,0 +1,54 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Model tests for metadata server."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from django.contrib.auth.models import User
+from maasserver.models import UserProfile
+from maasserver.testing.factory import factory
+from maastesting import TestCase
+from metadataserver.models import NodeKey
+from metadataserver.nodeinituser import NodeInitUser
+
+
+class TestNodeInitUser(TestCase):
+    """Test the special "user" that makes metadata requests from nodes."""
+
+    def test_always_wraps_same_user(self):
+        node_init_user = NodeInitUser()
+        self.assertEqual(node_init_user.user.id, NodeInitUser().user.id)
+
+    def test_reloads_if_already_created(self):
+        user = NodeInitUser().user
+        self.assertEqual(user.id, NodeInitUser().user.id)
+
+    def test_holds_node_init_user(self):
+        user = NodeInitUser().user
+        self.assertIsInstance(user, User)
+        self.assertEqual(NodeInitUser.user_name, user.username)
+        self.assertItemsEqual([], UserProfile.objects.filter(user=user))
+
+    def test_create_key_registers_node_key(self):
+        node = factory.make_node()
+        consumer, token = NodeInitUser().create_token(node)
+        nodekey = NodeKey.objects.get(node=node, key=token.key)
+        self.assertNotEqual(None, nodekey)
+
+    def test_get_node_for_key_finds_node(self):
+        node = factory.make_node()
+        consumer, token = NodeInitUser().create_token(node)
+        self.assertEqual(node, NodeInitUser().get_node_for_key(token.key))
+
+    def test_get_node_for_key_raises_DoesNotExist_if_key_not_found(self):
+        node_init_user = NodeInitUser()
+        non_key = factory.getRandomString()
+        self.assertRaises(
+            NodeKey.DoesNotExist, node_init_user.get_node_for_key, non_key)