launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/anon-meta-lookup into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #942943 in MAAS: "Provide option to allow unauth'd access to metadata service for debug"
  https://bugs.launchpad.net/maas/+bug/942943

For more details, see:
https://code.launchpad.net/~jtv/maas/anon-meta-lookup/+merge/108292

As discussed with Daviey and Julian, in separate sessions.  This change supports two different use-cases for an alternative access path to nodes' metadata: debug access to any node's metadata in development environments, and testing with hardware that we have trouble passing boot parameters to (such as metadata credentials).  For the former we may want to add a more human-friendly lookup key later, such as a node's system_id, but this branch lays the groundwork.

A later branch will hook a new subtree into the metadata API that provides access to metadata keyed by MAC address.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/anon-meta-lookup/+merge/108292
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/anon-meta-lookup into lp:maas.

=== modified file 'src/maas/demo.py'
--- src/maas/demo.py	2012-04-25 16:45:57 +0000
+++ src/maas/demo.py	2012-06-01 07:00:26 +0000
@@ -73,3 +73,9 @@
         },
      }
 }
+
+
+# For demo purposes, give nodes unauthenticated access to their metadata
+# even if we can't pass boot parameters.  This is not safe; do not
+# enable it on a production MAAS.
+ALLOW_ANONYMOUS_METADATA_ACCESS = True

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2012-04-25 16:45:57 +0000
+++ src/maas/development.py	2012-06-01 07:00:26 +0000
@@ -79,5 +79,9 @@
     'INTERCEPT_REDIRECTS': False,
     }
 
+# Make all nodes' metadata visible.  This is not safe; do not enable it
+# on a production MAAS.
+ALLOW_ANONYMOUS_METADATA_ACCESS = True
+
 # Allow the user to override settings in maas_local_settings.
 import_local_settings()

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-05-21 05:58:17 +0000
+++ src/maas/settings.py	2012-06-01 07:00:26 +0000
@@ -280,5 +280,11 @@
 # to have failed and mark it as FAILED_TESTS.
 COMMISSIONING_TIMEOUT = 60
 
+# Allow anonymous access to the metadata for a node, keyed by its MAC
+# address?  This is for development purposes only.  DO NOT ENABLE THIS
+# IN PRODUCTION or private metadata, including MAAS access credentials
+# for all nodes, will be exposed on your network.
+ALLOW_ANONYMOUS_METADATA_ACCESS = False
+
 # Allow the user to override settings in maas_local_settings.
 import_local_settings()

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-04-30 06:14:16 +0000
+++ src/metadataserver/api.py	2012-06-01 07:00:26 +0000
@@ -17,6 +17,7 @@
     'VersionIndexHandler',
     ]
 
+from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from maasserver.api import (
@@ -34,7 +35,10 @@
     MAASAPINotFound,
     NodeStateViolation,
     )
-from maasserver.models import SSHKey
+from maasserver.models import (
+    MACAddress,
+    SSHKey,
+    )
 from metadataserver.models import (
     NodeCommissionResult,
     NodeKey,
@@ -53,7 +57,13 @@
 
 
 def get_node_for_request(request):
-    """Return the `Node` that `request` is authorized to query for."""
+    """Return the `Node` that `request` queries metadata for.
+
+    For this form of access, a node can only query its own metadata.  Thus
+    the oauth key used to authenticate the request must belong to the same
+    node that is being queried.  Any request that is not made by an
+    authenticated node will be denied.
+    """
     key = extract_oauth_key(request)
     try:
         return NodeKey.objects.get_node_for_key(key)
@@ -61,6 +71,42 @@
         raise PermissionDenied("Not authenticated as a known node.")
 
 
+def get_node_for_mac(mac):
+    """Identify node being queried based on its MAC address.
+
+    This form of access is a security hazard, and thus it is permitted only
+    on development systems where ALLOW_ANONYMOUS_METADATA_ACCESS is enabled.
+    """
+    if not settings.ALLOW_ANONYMOUS_METADATA_ACCESS:
+        raise PermissionDenied(
+            "Unauthenticated metadata access is not allowed on this MAAS.")
+    matching_macs = list(MACAddress.objects.filter(mac_address=mac))
+    if len(matching_macs) == 0:
+        raise MAASAPINotFound()
+    [match] = matching_macs
+    return match.node
+
+
+def get_queried_node(request, for_mac=None):
+    """Identify and authorize the node whose metadata is being queried.
+
+    :param request: HTTP request.  In normal usage, this is authenticated
+        with an oauth key; the key maps to the querying node, and the
+        querying node always queries itself.
+    :param for_mac: Optional MAC address for the node being queried.  If
+        this is given, and anonymous metadata access is enabled (do in
+        development environments only!) then the node is looked up by its
+        MAC address.
+    :return: The :class:`Node` whose metadata is being queried.
+    """
+    if for_mac is None:
+        # Identify node, and authorize access, by oauth key.
+        return get_node_for_request(request)
+    else:
+        # Access keyed by MAC address.
+        return get_node_for_mac(for_mac)
+
+
 def make_text_response(contents):
     """Create a response containing `contents` as plain text."""
     return HttpResponse(contents, mimetype='text/plain')
@@ -115,7 +161,7 @@
     def read(self, request, version):
         """Read the metadata index for this version."""
         check_version(version)
-        if NodeUserData.objects.has_user_data(get_node_for_request(request)):
+        if NodeUserData.objects.has_user_data(get_queried_node(request)):
             shown_fields = self.fields
         else:
             shown_fields = list(self.fields)
@@ -146,7 +192,7 @@
             (overwriting any previous error string), and displayed in the MAAS
             UI.  If not given, any previous error string will be cleared.
         """
-        node = get_node_for_request(request)
+        node = get_queried_node(request)
         status = request.POST.get('status', None)
 
         status = get_mandatory_param(request.POST, 'status')
@@ -210,7 +256,7 @@
 
     def read(self, request, version, item=None):
         check_version(version)
-        node = get_node_for_request(request)
+        node = get_queried_node(request)
 
         # Requesting the list of attributes, not any particular
         # attribute.
@@ -247,7 +293,7 @@
 
     def read(self, request, version):
         check_version(version)
-        node = get_node_for_request(request)
+        node = get_queried_node(request)
         try:
             return HttpResponse(
                 NodeUserData.objects.get_user_data(node),

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-04-20 15:16:41 +0000
+++ src/metadataserver/tests/test_api.py	2012-06-01 07:00:26 +0000
@@ -16,8 +16,13 @@
 import httplib
 from io import BytesIO
 
+from django.conf import settings
+from django.core.exceptions import PermissionDenied
 from maasserver.enum import NODE_STATUS
-from maasserver.exceptions import Unauthorized
+from maasserver.exceptions import (
+    MAASAPINotFound,
+    Unauthorized,
+    )
 from maasserver.models import SSHKey
 from maasserver.provisioning import get_provisioning_api_proxy
 from maasserver.testing import reload_object
@@ -26,7 +31,9 @@
 from maastesting.djangotestcase import DjangoTestCase
 from metadataserver.api import (
     check_version,
+    get_node_for_mac,
     get_node_for_request,
+    get_queried_node,
     make_list_response,
     make_text_response,
     MetaDataHandler,
@@ -83,6 +90,33 @@
             Unauthorized,
             get_node_for_request, self.fake_request())
 
+    def test_get_node_for_mac_refuses_if_anonymous_access_disabled(self):
+        self.patch(settings, 'ALLOW_ANONYMOUS_METADATA_ACCESS', False)
+        self.assertRaises(
+            PermissionDenied, get_node_for_mac, factory.getRandomMACAddress())
+
+    def test_get_node_for_mac_raises_404_for_unknown_mac(self):
+        self.assertRaises(
+            MAASAPINotFound, get_node_for_mac, factory.getRandomMACAddress())
+
+    def test_get_node_for_mac_finds_node_by_mac(self):
+        mac = factory.make_mac_address()
+        self.assertEqual(mac.node, get_node_for_mac(mac.mac_address))
+
+    def test_get_queried_node_looks_up_by_mac_if_given(self):
+        mac = factory.make_mac_address()
+        self.assertEqual(
+            mac.node,
+            get_queried_node(object(), for_mac=mac.mac_address))
+
+    def test_get_queried_node_looks_up_oauth_key_by_default(self):
+        node = factory.make_node()
+        token = NodeKey.objects.get_token_for_node(node)
+        request = self.fake_request(
+            HTTP_AUTHORIZATION=factory.make_oauth_header(
+                oauth_token=token.key))
+        self.assertEqual(node, get_queried_node(request))
+
 
 class TestViews(DjangoTestCase, ProvisioningFakeFactory):
     """Tests for the API views."""