Merge lp:~allenap/maas/rpc-give-shared-secret-to-accepted-cluster into lp:~maas-committers/maas/trunk

[Gavin Panella]

When a cluster is accepted, give it the shared secret with which to authenticate RPC connections.

[Raphaël Badin (rvb)]

Looks good; couple of remarks but nothing major.

review: Approve

[Christian Reis (kiko)]

This is crazy. We are not going to land something which sends a password over the wire, over my dead body.

review: Disapprove

[Andres Rodriguez (andreserl)]

Can we send the secret in base 64 at the very least? I know it is not
encryption but it is better than sending it over clear text.
On Oct 1, 2014 3:33 PM, "Gavin Panella" <email address hidden> wrote:

> Gavin Panella has proposed merging
> lp:~allenap/maas/rpc-give-shared-secret-to-accepted-cluster into lp:maas.
>
> Commit message:
> When a cluster is accepted, give it the shared secret with which to
> authenticate RPC connections.
>
> Requested reviews:
> MAAS Maintainers (maas-maintainers)
>
> For more details, see:
>
> https://code.launchpad.net/~allenap/maas/rpc-give-shared-secret-to-accepted-cluster/+merge/236777
> --
>
> https://code.launchpad.net/~allenap/maas/rpc-give-shared-secret-to-accepted-cluster/+merge/236777
> You are subscribed to branch lp:maas.
>
> === modified file 'src/maasserver/api/node_groups.py'
> --- src/maasserver/api/node_groups.py 2014-09-24 14:20:51 +0000
> +++ src/maasserver/api/node_groups.py 2014-10-01 20:32:40 +0000
> @@ -29,6 +29,7 @@
> from django.http import HttpResponse
> from django.shortcuts import get_object_or_404
> from formencode import validators
> +from maasserver import security
> from maasserver.api.logger import maaslog
> from maasserver.api.support import (
> admin_method,
> @@ -115,10 +116,12 @@
> The response is based on the status of the `nodegroup` after
> registration,
> and whether it had already been registered before the call.
>
> - If the nodegroup was accepted, this returns an empty 200 response.
> + If the nodegroup was accepted, this returns a JSON-encoded response
> + containing the shared-secret to be used for RPC communications.
> """
> if nodegroup.status == NODEGROUP_STATUS.ACCEPTED:
> - return {} # Previously returned task-runner credentials.
> + secret_hex = security.get_shared_secret().encode("hex")
> + return {"secret": secret_hex.decode("ascii")}
> elif nodegroup.status == NODEGROUP_STATUS.REJECTED:
> raise PermissionDenied('Rejected cluster.')
> elif nodegroup.status == NODEGROUP_STATUS.PENDING:
>
> === renamed file 'src/maasserver/api/tests/test_nodegroup.py' =>
> 'src/maasserver/api/tests/test_node_groups.py'
> === modified file 'src/maasserver/api/tests/test_register.py'
> --- src/maasserver/api/tests/test_register.py 2014-09-24 14:20:51 +0000
> +++ src/maasserver/api/tests/test_register.py 2014-10-01 20:32:40 +0000
> @@ -26,6 +26,8 @@
> )
> from django.core.urlresolvers import reverse
> from django.test.client import RequestFactory
> +from fixtures import EnvironmentVariableFixture
> +from maasserver import security
> from maasserver.api import node_groups as nodegroups_module
> from maasserver.api.node_groups import (
> compose_nodegroup_register_response,
> @@ -172,11 +174,14 @@
> class TestComposeNodegroupRegisterResponse(MAASServerTestCase):
> """Tests for `compose_nodegroup_register_response`."""
>
> - def test_returns_empty_response_if_accepted(self):
> + def test_returns_shared_secret_if_accepted(self):
> + self.useFixture(EnvironmentVariableFixture(
> + "MAAS_ROOT", self.make_dir()))
> + secret_hex = security.get_...

[Gavin Panella (allenap)]

> This is crazy. We are not going to land something which sends a
> password over the wire, over my dead body.

That's why I haven't landed it yet :-/

[Gavin Panella (allenap)]

> Can we send the secret in base 64 at the very least? I know it is not
> encryption but it is better than sending it over clear text.

As it is it's going over base16, which is way more 133t and 5ekure than
base64 ;)

Unmerged revisions

3144. By Gavin Panella on 2014-10-01

Save secret from register call in start_cluster_controller.

3143. By Gavin Panella on 2014-10-01

Return the shared secret to an accepted cluster.

3142. By Gavin Panella on 2014-10-01

Rename test module to match the module it's testing.

=== modified file 'src/maasserver/api/node_groups.py'
--- src/maasserver/api/node_groups.py	2014-09-24 14:20:51 +0000
+++ src/maasserver/api/node_groups.py	2014-10-01 20:32:40 +0000
@@ -29,6 +29,7 @@
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from formencode import validators
+from maasserver import security
 from maasserver.api.logger import maaslog
 from maasserver.api.support import (
     admin_method,
@@ -115,10 +116,12 @@
     The response is based on the status of the `nodegroup` after registration,
     and whether it had already been registered before the call.
 
-    If the nodegroup was accepted, this returns an empty 200 response.
+    If the nodegroup was accepted, this returns a JSON-encoded response
+    containing the shared-secret to be used for RPC communications.
     """
     if nodegroup.status == NODEGROUP_STATUS.ACCEPTED:
-        return {}  # Previously returned task-runner credentials.
+        secret_hex = security.get_shared_secret().encode("hex")
+        return {"secret": secret_hex.decode("ascii")}
     elif nodegroup.status == NODEGROUP_STATUS.REJECTED:
         raise PermissionDenied('Rejected cluster.')
     elif nodegroup.status == NODEGROUP_STATUS.PENDING:

=== renamed file 'src/maasserver/api/tests/test_nodegroup.py' => 'src/maasserver/api/tests/test_node_groups.py'
=== modified file 'src/maasserver/api/tests/test_register.py'
--- src/maasserver/api/tests/test_register.py	2014-09-24 14:20:51 +0000
+++ src/maasserver/api/tests/test_register.py	2014-10-01 20:32:40 +0000
@@ -26,6 +26,8 @@
     )
 from django.core.urlresolvers import reverse
 from django.test.client import RequestFactory
+from fixtures import EnvironmentVariableFixture
+from maasserver import security
 from maasserver.api import node_groups as nodegroups_module
 from maasserver.api.node_groups import (
     compose_nodegroup_register_response,
@@ -172,11 +174,14 @@
 class TestComposeNodegroupRegisterResponse(MAASServerTestCase):
     """Tests for `compose_nodegroup_register_response`."""
 
-    def test_returns_empty_response_if_accepted(self):
+    def test_returns_shared_secret_if_accepted(self):
+        self.useFixture(EnvironmentVariableFixture(
+            "MAAS_ROOT", self.make_dir()))
+        secret_hex = security.get_shared_secret().encode("hex")
         nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
         existed = factory.pick_bool()
         self.assertEqual(
-            {},
+            {"secret": secret_hex.decode("ascii")},
             compose_nodegroup_register_response(nodegroup, existed))
 
     def test_returns_forbidden_if_rejected(self):

=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py	2014-09-23 20:16:54 +0000
+++ src/provisioningserver/start_cluster_controller.py	2014-10-01 20:32:40 +0000
@@ -19,6 +19,7 @@
 
 import httplib
 import json
+from sys import stderr
 from time import sleep
 from urllib2 import (
     HTTPError,
@@ -33,6 +34,7 @@
 from provisioningserver.cluster_config import get_cluster_uuid
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.network import discover_networks
+from provisioningserver.security import set_shared_secret_on_filesystem
 
 
 maaslog = get_maas_logger("cluster")
@@ -46,6 +48,9 @@
     """For use by :class:`MainScript`."""
     parser.add_argument(
         'server_url', metavar='URL', help="URL to the MAAS region controller.")
+    parser.add_argument(
+        '--debug', action="store_true", help="Show debugging output.")
+    parser.set_defaults(debug=False)
 
 
 def log_error(exception):
@@ -59,7 +64,7 @@
     return MAASClient(NoAuth(), MAASDispatcher(), server_url)
 
 
-def register(server_url):
+def maybe_register(server_url):
     """Register with the region controller.
 
     Offers this machine to the region controller as a potential cluster
@@ -108,14 +113,36 @@
         raise AssertionError("Unexpected return code: %r" % status_code)
 
 
+def register(server_url):
+    """Poll the region controller until this node is accepted or rejected.
+
+    :returns: The secret returned from the region controller.
+    """
+    while True:
+        response = maybe_register(server_url)
+        if response is not None:
+            secret_hex = response["secret"].encode("ascii")
+            return secret_hex.decode("hex")
+        # Bad luck. Wait a minute, then try again.
+        sleep(60)
+
+
 def run(args):
     """Start the cluster controller.
 
     If this system is still awaiting approval as a cluster controller, this
     command will keep looping until it gets a definite answer.
     """
-    maaslog.info("Starting cluster controller %s." % get_cluster_uuid())
-    connection_details = register(args.server_url)
-    while connection_details is None:
-        sleep(60)
-        connection_details = register(args.server_url)
+    try:
+        cluster_uuid = get_cluster_uuid()
+        maaslog.info("Starting cluster controller %s.", cluster_uuid)
+        secret = register(args.server_url)
+        set_shared_secret_on_filesystem(secret)
+    except Exception as error:
+        if args.debug:
+            raise
+        else:
+            print(unicode(error), file=stderr)
+            raise SystemExit(2)
+    else:
+        raise SystemExit(0)

=== modified file 'src/provisioningserver/tests/test_start_cluster_controller.py'
--- src/provisioningserver/tests/test_start_cluster_controller.py	2014-09-23 20:21:50 +0000
+++ src/provisioningserver/tests/test_start_cluster_controller.py	2014-10-01 20:32:40 +0000
@@ -26,6 +26,7 @@
 from fixtures import EnvironmentVariableFixture
 from maastesting.factory import factory
 from provisioningserver import start_cluster_controller
+from provisioningserver.security import get_shared_secret_from_filesystem
 from provisioningserver.testing.testcase import PservTestCase
 from testtools.matchers import StartsWith
 
@@ -48,13 +49,13 @@
         )
 
 
-FakeArgs = namedtuple('FakeArgs', ['server_url'])
-
-
-def make_args(server_url=None):
+FakeArgs = namedtuple('FakeArgs', ['server_url', 'debug'])
+
+
+def make_args(server_url=None, debug=True):
     if server_url is None:
         server_url = make_url('region')
-    return FakeArgs(server_url)
+    return FakeArgs(server_url, debug)
 
 
 class FakeURLOpenResponse:
@@ -84,10 +85,10 @@
             start_cluster_controller, 'get_cluster_uuid')
         get_uuid.return_value = factory.make_UUID()
 
-    def make_connection_details(self):
-        return {
-            'BROKER_URL': make_url('broker'),
-        }
+    def make_connection_details(self, secret=None):
+        if secret is None:
+            secret = factory.make_bytes()
+        return {'secret': secret.encode("hex").decode("ascii")}
 
     def parse_headers_and_body(self, headers, body):
         """Parse ingredients of a web request.
@@ -109,9 +110,9 @@
         fake.return_value = FakeURLOpenResponse(content, status=http_code)
         return fake
 
-    def prepare_success_response(self):
+    def prepare_success_response(self, secret=None):
         """Prepare to return connection details from API request."""
-        details = self.make_connection_details()
+        details = self.make_connection_details(secret)
         self.prepare_response(httplib.OK, json.dumps(details))
         return details
 
@@ -131,14 +132,21 @@
         self.prepare_success_response()
         parser = ArgumentParser()
         start_cluster_controller.add_arguments(parser)
-        self.assertIsNone(
-            start_cluster_controller.run(
-                parser.parse_args((make_url(),))))
+
+        sysexit = self.assertRaises(
+            SystemExit, start_cluster_controller.run,
+            parser.parse_args((make_url(),)))
+        self.assertEqual(0, sysexit.code)
 
     def test_uses_given_url(self):
         url = make_url('region')
         self.prepare_success_response()
-        start_cluster_controller.run(make_args(server_url=url))
+
+        sysexit = self.assertRaises(
+            SystemExit, start_cluster_controller.run,
+            make_args(server_url=url))
+        self.assertEqual(0, sysexit.code)
+
         (args, kwargs) = MAASDispatcher.dispatch_query.call_args
         self.assertThat(args[0], StartsWith(url + 'api/1.0/nodegroups/'))
 
@@ -180,3 +188,14 @@
         self.assertEqual(
             start_cluster_controller.get_cluster_uuid.return_value,
             post['uuid'])
+
+    def test_register_saves_secret(self):
+        secret = factory.make_bytes()
+        self.prepare_success_response(secret)
+
+        sysexit = self.assertRaises(
+            SystemExit, start_cluster_controller.run, make_args())
+        self.assertEqual(0, sysexit.code)
+
+        self.assertEqual(
+            secret, get_shared_secret_from_filesystem())