launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/worker-auth into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

All node-group workers log onto the API under the same user identity (the “worker user”), but using different OAuth keys.  One worker isn't allowed to overwrite another worker's DHCP leases.  Neither is anyone else; the worker that manages the leases has to be the one to provide them.

This is all really quite similar to what happens in the metadata server, where the oauth key identifies the requesting node.  Except here, more sensibly I think, the node-group name is explicit in the request.

Two changes off by the side are small but significant:

1. The factory now goes through NodeGroup.objects.new, as it should.  This means that it no longer lets you provide an API token for the worker; the manager class creates one internally.

2. The worker user is no longer a deactivated user.  Which also means that it gets access to the API just like a regular user.  We ought to rein that in a bit, to limit the damage that could be done with compromised credentials.  I put a task on the board.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/worker-auth/+merge/119505
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/worker-auth into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-08-14 05:14:33 +0000
+++ src/maasserver/api.py	2012-08-14 10:38:26 +0000
@@ -886,6 +886,25 @@
         return HttpResponse("Sending worker refresh.", status=httplib.OK)
 
 
+def verify_worker_login(request, nodegroup):
+    """Verify that `request` was issued by the worker for `nodegroup`.
+
+    This is checked by comparing the OAuth key used for signing the request
+    to the node group's API key.
+
+    If things check out, returns the :class:`NodeGroup`.  Otherwise,
+    raises :class:`PermissionDenied`.
+    """
+    try:
+        key = extract_oauth_key(request)
+    except Unauthorized as e:
+        raise PermissionDenied(unicode(e))
+
+    if key != nodegroup.api_key:
+        raise PermissionDenied(
+            "Only allowed for the %s worker." % nodegroup.name)
+
+
 @api_operations
 class NodeGroupHandler(BaseHandler):
     """Node-group API."""
@@ -909,6 +928,7 @@
     def update_leases(self, request, name):
         leases = get_mandatory_param(request.data, 'leases')
         nodegroup = get_object_or_404(NodeGroup, name=name)
+        verify_worker_login(request, nodegroup)
         DHCPLease.objects.update_leases(nodegroup, json.loads(leases))
         return HttpResponse("Leases updated.", status=httplib.OK)
 

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2012-07-30 11:02:33 +0000
+++ src/maasserver/testing/factory.py	2012-08-14 10:38:26 +0000
@@ -32,7 +32,6 @@
     SSHKey,
     )
 from maasserver.models.node import NODE_TRANSITIONS
-from maasserver.models.user import create_auth_token
 from maasserver.testing import (
     get_data,
     reload_object,
@@ -112,9 +111,8 @@
         return reload_object(node)
 
     def make_node_group(self, name=None, worker_ip=None, router_ip=None,
-                        api_token=None, network=None, subnet_mask=None,
-                        broadcast_ip=None, ip_range_low=None,
-                        ip_range_high=None, **kwargs):
+                        network=None, subnet_mask=None, broadcast_ip=None,
+                        ip_range_low=None, ip_range_high=None, **kwargs):
         """Create a :class:`NodeGroup`.
 
         If network (an instance of IPNetwork) is provided, use it to populate
@@ -127,9 +125,6 @@
         """
         if name is None:
             name = self.make_name('nodegroup')
-        if api_token is None:
-            user = self.make_user()
-            api_token = create_auth_token(user)
         if network is not None:
             subnet_mask = str(network.netmask)
             broadcast_ip = str(network.broadcast)
@@ -150,9 +145,8 @@
                 router_ip = self.getRandomIPAddress()
             if worker_ip is None:
                 worker_ip = self.getRandomIPAddress()
-        ng = NodeGroup(
-            name=name, api_token=api_token, api_key=api_token.key,
-            worker_ip=worker_ip, subnet_mask=subnet_mask,
+        ng = NodeGroup.objects.new(
+            name=name, worker_ip=worker_ip, subnet_mask=subnet_mask,
             broadcast_ip=broadcast_ip, router_ip=router_ip,
             ip_range_low=ip_range_low, ip_range_high=ip_range_high, **kwargs)
         ng.save()

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-14 05:56:34 +0000
+++ src/maasserver/tests/test_api.py	2012-08-14 10:38:26 +0000
@@ -80,6 +80,7 @@
     TestCase,
     )
 from maasserver.utils import map_enum
+from maasserver.worker_user import get_worker_user
 from maastesting.djangotestcase import TransactionTestCase
 from maastesting.fakemethod import FakeMethod
 from maastesting.matchers import ContainsAll
@@ -2452,6 +2453,21 @@
             (response.status_code, response.content))
 
 
+def explain_unexpected_response(expected_status, response):
+    """Return human-readable failure message: unexpected http response."""
+    return "Unexpected http status (expected %s): %s - %s" % (
+        expected_status,
+        response.status_code,
+        response.content,
+        )
+
+
+def make_worker_client(nodegroup):
+    """Create a test client logged in as if it were `nodegroup`."""
+    return OAuthAuthenticatedClient(
+        get_worker_user(), token=nodegroup.api_token)
+
+
 class TestNodeGroupAPI(APITestCase):
 
     def test_reverse_points_to_nodegroup(self):
@@ -2475,7 +2491,8 @@
     def test_update_leases_processes_empty_leases_dict(self):
         nodegroup = factory.make_node_group()
         factory.make_dhcp_lease(nodegroup=nodegroup)
-        response = self.client.post(
+        client = make_worker_client(nodegroup)
+        response = client.post(
             reverse('nodegroup', args=[nodegroup.name]),
             {
                 'op': 'update_leases',
@@ -2491,7 +2508,8 @@
         nodegroup = factory.make_node_group()
         ip = factory.getRandomIPAddress()
         mac = factory.getRandomMACAddress()
-        response = self.client.post(
+        client = make_worker_client(nodegroup)
+        response = client.post(
             reverse('nodegroup', args=[nodegroup.name]),
             {
                 'op': 'update_leases',
@@ -2505,9 +2523,49 @@
             for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
 
 
-class TestAnonNodeGroupsAPI(AnonAPITestCase):
+class TestNodeGroupAPIAuth(APIv10TestMixin, TestCase):
+    """Authorization tests for nodegroup API."""
+
+    def log_in_as_normal_user(self):
+        """Log `self.client` in as a normal user."""
+        user = factory.make_user()
+        password = factory.getRandomString()
+        user.set_password(password)
+        user.save()
+        self.client.login(username=user.username, password=password)
 
     def test_nodegroup_requires_authentication(self):
         nodegroup = factory.make_node_group()
         response = self.client.get(reverse('nodegroup', args=[nodegroup.name]))
         self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
+    def test_update_leases_works_for_nodegroup_worker(self):
+        nodegroup = factory.make_node_group()
+        client = make_worker_client(nodegroup)
+        response = client.post(
+            reverse('nodegroup', args=[nodegroup.name]),
+            {'op': 'update_leases', 'leases': json.dumps({})})
+        self.assertEqual(
+            httplib.OK, response.status_code,
+            explain_unexpected_response(httplib.OK, response))
+
+    def test_update_leases_does_not_work_for_normal_user(self):
+        nodegroup = factory.make_node_group()
+        self.log_in_as_normal_user()
+        response = self.client.post(
+            reverse('nodegroup', args=[nodegroup.name]),
+            {'op': 'update_leases', 'leases': json.dumps({})})
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code,
+            explain_unexpected_response(httplib.FORBIDDEN, response))
+
+    def test_update_leases_does_not_let_worker_update_other_nodegroup(self):
+        requesting_nodegroup = factory.make_node_group()
+        about_nodegroup = factory.make_node_group()
+        client = make_worker_client(requesting_nodegroup)
+        response = client.post(
+            reverse('nodegroup', args=[about_nodegroup.name]),
+            {'op': 'update_leases', 'leases': json.dumps({})})
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code,
+            explain_unexpected_response(httplib.FORBIDDEN, response))

=== modified file 'src/maasserver/worker_user.py'
--- src/maasserver/worker_user.py	2012-08-07 21:15:54 +0000
+++ src/maasserver/worker_user.py	2012-08-14 10:38:26 +0000
@@ -37,6 +37,6 @@
                 first_name="Node-group worker",
                 last_name="Special user",
                 email="maas-nodegroup-worker@localhost",
-                is_staff=False, is_active=False, is_superuser=False))
+                is_staff=False, is_superuser=False))
         cache.set(WORKER_USER_CACHE_KEY, worker_user)
     return worker_user