Merge lp:~jtv/maas/bug-1059569 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Run the cluster controller under a customizable user/group identity (by default, maas/maas).

Discussed with... everyone, probably. We had a vague hope that this would also fix Upstart's problems tracking the celeryd fork(), but actually that required a separate branch. The problem there was that we did a different fork first, and so upstart got to track the wrong one. All that _really_ changes in the branch you see here is that a fork/exec sequence becomes a fork/setuid/exec sequence.

I do as much as possible before the fork, because the error feedback channel from the child process is always going to be a bit narrower and harder to manage. If a failure is coming, we'd best give up early and report it while we still can. It would have been worth checking privileges for the setuid/setgid beforehand as well, except maas-provision won't run unless you're root. For our purposes here it'd be fine if you ran it as maas (or yourself, in dev mode) and made it setuid to that same user; setuid would allow that but maas-provision, as it stands, will not.

Interesting fact: this code has undergone so many changes in how we kick off the child process that the test abstractions for faking the related system calls keep growing.

Jeroen

[Raphaël Badin (rvb)]

Looks good.

[0]

17 + parser.add_argument(
18 + '--user', '-u', metavar='USER', default='maas',
19 + help="System user identity that should run the cluster controller.")
20 + parser.add_argument(
21 + '--group', '-g', metavar='GROUP', default='maas',
22 + help="System group that should run the cluster controller.")

and

58 +def start_up(server_url, connection_details, user='maas', group='maas'):

That's two places where you set the default ('maas'/'maas'). The first one is probably enough don't you think?

review: Approve

[Gavin Panella (allenap)]

Looks good. I haven't looked at everything because I saw that rvba is
doing this, but I had a couple of comments.

[1]

-    Popen(command, env=env)
+
+    pid = os.fork()
+    if pid == 0:
+        # Child process.  Become the right user, and start celery.
+        os.setuid(uid)
+        os.setgid(gid)
+        os.execvpe(command[0], command, env)

Doesn't matter much, but you could use Popen's preexec_fn argument
too:

    def switch_user():
        os.setuid(uid)
        os.setgid(gid)
    Popen(command, env=env, preexec_fn=switch_user)

[2]

+    parser.add_argument(
+        '--user', '-u', metavar='USER', default='maas',
+        help="System user identity that should run the cluster controller.")
+    parser.add_argument(
+        '--group', '-g', metavar='GROUP', default='maas',
+        help="System group that should run the cluster controller.")

Perhaps the default should be the current user, unless invoked by
root, in which case there is no default and it becomes a mandatory
argument (argparse supports required=True)? Might be handy for
development.

[Jeroen T. Vermeulen (jtv)]

Raphael: you're absolutely right and I fixed it. Complicates the tests a tiny bit but it's really not so bad.

Gavin: nice idea but not now please! Doing it the way I did here solves the bug immediately. Doing it your way requires a packaging change first.

[Gavin Panella (allenap)]

On 2 October 2012 11:22, Jeroen T. Vermeulen <...> wrote:
> Gavin: nice idea but not now please! Doing it the way I did here
> solves the bug immediately. Doing it your way requires a packaging
> change first.

Good point.

=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py	2012-09-28 10:16:59 +0000
+++ src/provisioningserver/start_cluster_controller.py	2012-10-02 10:24:23 +0000
@@ -18,7 +18,7 @@
 import httplib
 import json
 import os
-from subprocess import Popen
+from pwd import getpwnam
 from time import sleep
 from urllib2 import (
     HTTPError,
@@ -43,6 +43,12 @@
     """For use by :class:`MainScript`."""
     parser.add_argument(
         'server_url', metavar='URL', help="URL to the MAAS region controller.")
+    parser.add_argument(
+        '--user', '-u', metavar='USER', default='maas',
+        help="System user identity that should run the cluster controller.")
+    parser.add_argument(
+        '--group', '-g', metavar='GROUP', default='maas',
+        help="System group that should run the cluster controller.")
 
 
 def log_error(exception):
@@ -115,8 +121,10 @@
         raise AssertionError("Unexpected return code: %r" % status_code)
 
 
-def start_celery(connection_details):
+def start_celery(connection_details, user, group):
     broker_url = connection_details['BROKER_URL']
+    uid = getpwnam(user).pw_uid
+    gid = getpwnam(group).pw_gid
 
     # Copy environment, but also tell celeryd what broker to listen to.
     env = dict(os.environ, CELERY_BROKER_URL=broker_url)
@@ -128,7 +136,13 @@
         '--beat',
         '-Q', get_cluster_uuid(),
         ]
-    Popen(command, env=env)
+
+    pid = os.fork()
+    if pid == 0:
+        # Child process.  Become the right user, and start celery.
+        os.setuid(uid)
+        os.setgid(gid)
+        os.execvpe(command[0], command, env)
 
 
 def request_refresh(server_url):
@@ -141,13 +155,13 @@
             % e.reason)
 
 
-def start_up(server_url, connection_details):
+def start_up(server_url, connection_details, user, group):
     """We've been accepted as a cluster controller; start doing the job.
 
     This starts up celeryd, listening to the broker that the region
     controller pointed us to, and on the appropriate queue.
     """
-    start_celery(connection_details)
+    start_celery(connection_details, user=user, group=group)
     sleep(10)
     request_refresh(server_url)
 
@@ -162,4 +176,5 @@
     while connection_details is None:
         sleep(60)
         connection_details = register(args.server_url)
-    start_up(args.server_url, connection_details)
+    start_up(
+        args.server_url, connection_details, user=args.user, group=args.group)

=== modified file 'src/provisioningserver/tests/test_start_cluster_controller.py'
--- src/provisioningserver/tests/test_start_cluster_controller.py	2012-09-28 10:16:59 +0000
+++ src/provisioningserver/tests/test_start_cluster_controller.py	2012-10-02 10:24:23 +0000
@@ -17,6 +17,8 @@
 import httplib
 from io import BytesIO
 import json
+import os
+from random import randint
 from urllib2 import (
     HTTPError,
     URLError,
@@ -26,7 +28,6 @@
 from apiclient.testing.django import parse_headers_and_body_with_django
 from fixtures import EnvironmentVariableFixture
 from maastesting.factory import factory
-from mock import Mock
 from provisioningserver import start_cluster_controller
 from provisioningserver.testing.testcase import PservTestCase
 
@@ -50,13 +51,15 @@
         )
 
 
-FakeArgs = namedtuple('FakeArgs', ['server_url'])
+FakeArgs = namedtuple('FakeArgs', ['server_url', 'user', 'group'])
 
 
 def make_args(server_url=None):
     if server_url is None:
         server_url = make_url('region')
-    return FakeArgs(server_url)
+    user = factory.make_name('user')
+    group = factory.make_name('group')
+    return FakeArgs(server_url, user, group)
 
 
 class FakeURLOpenResponse:
@@ -77,12 +80,19 @@
 
     def setUp(self):
         super(TestStartClusterController, self).setUp()
+        # Patch out anything that could be remotely harmful if we did it
+        # accidentally in the test.  Make the really outrageous ones
+        # raise exceptions.
         self.patch(start_cluster_controller, 'sleep').side_effect = Sleeping()
-        self.patch(start_cluster_controller, 'Popen').side_effect = (
-            Executing())
-        self.cluster_uuid = factory.getRandomUUID()
-        self.patch(start_cluster_controller, 'get_cluster_uuid', Mock(
-            return_value=self.cluster_uuid))
+        self.patch(start_cluster_controller, 'getpwnam')
+        start_cluster_controller.getpwnam.pw_uid = randint(3000, 4000)
+        start_cluster_controller.getpwnam.pw_gid = randint(3000, 4000)
+        self.patch(os, 'fork').side_effect = Executing()
+        self.patch(os, 'execvpe').side_effect = Executing()
+        self.patch(os, 'setuid')
+        self.patch(os, 'setgid')
+        get_uuid = self.patch(start_cluster_controller, 'get_cluster_uuid')
+        get_uuid.return_value = factory.getRandomUUID()
 
     def make_connection_details(self):
         return {
@@ -123,17 +133,36 @@
         """Prepare to return "request pending" from API request."""
         self.prepare_response(httplib.ACCEPTED)
 
+    def pretend_to_fork_into_child(self):
+        """Make `fork` act as if it's returning into the child process.
+
+        The start_cluster_controller child process then executes celeryd,
+        so this call also patches up the call that does that so it pretends
+        to be successful.
+        """
+        self.patch(os, 'fork').return_value = 0
+        self.patch(os, 'execvpe')
+
+    def pretend_to_fork_into_parent(self):
+        """Make `fork` act as if it's returning into the parent process."""
+        self.patch(os, 'fork').return_value = randint(2, 65535)
+
     def test_run_command(self):
         # We can't really run the script, but we can verify that (with
         # the right system functions patched out) we can run it
         # directly.
-        self.patch(start_cluster_controller, 'Popen')
+        self.pretend_to_fork_into_child()
         self.patch(start_cluster_controller, 'sleep')
         self.prepare_success_response()
         parser = ArgumentParser()
         start_cluster_controller.add_arguments(parser)
         start_cluster_controller.run(parser.parse_args((make_url(),)))
-        self.assertNotEqual(0, start_cluster_controller.Popen.call_count)
+        self.assertEqual(1, os.fork.call_count)
+        self.assertEqual(1, os.execvpe.call_count)
+        os.setuid.assert_called_once_with(
+            start_cluster_controller.getpwnam.return_value.pw_uid)
+        os.setgid.assert_called_once_with(
+            start_cluster_controller.getpwnam.return_value.pw_gid)
 
     def test_uses_given_url(self):
         url = make_url('region')
@@ -184,24 +213,29 @@
         headers, body = kwargs["headers"], kwargs["data"]
         post, files = self.parse_headers_and_body(headers, body)
         self.assertEqual([interface], json.loads(post['interfaces']))
-        self.assertEqual(self.cluster_uuid, post['uuid'])
+        self.assertEqual(
+            start_cluster_controller.get_cluster_uuid.return_value,
+            post['uuid'])
 
     def test_starts_up_once_accepted(self):
         self.patch(start_cluster_controller, 'start_up')
         connection_details = self.prepare_success_response()
         server_url = make_url()
         start_cluster_controller.run(make_args(server_url=server_url))
-        start_cluster_controller.start_up.assert_called_once_with(
-            server_url, connection_details)
+        self.assertItemsEqual(
+            start_cluster_controller.start_up.call_args[0],
+            (server_url, connection_details))
 
     def test_start_up_calls_refresh_secrets(self):
         url = make_url('region')
         connection_details = self.make_connection_details()
-        self.patch(start_cluster_controller, 'Popen')
+        self.pretend_to_fork_into_parent()
         self.patch(start_cluster_controller, 'sleep')
         self.prepare_success_response()
 
-        start_cluster_controller.start_up(url, connection_details)
+        start_cluster_controller.start_up(
+            url, connection_details,
+            factory.make_name('user'), factory.make_name('group'))
 
         (args, kwargs) = MAASDispatcher.dispatch_query.call_args
         self.assertEqual(url + 'api/1.0/nodegroups/', args[0])
@@ -212,12 +246,13 @@
         self.assertEqual("refresh_workers", post["op"])
 
     def test_start_up_ignores_failure_on_refresh_secrets(self):
-        self.patch(start_cluster_controller, 'Popen')
+        self.pretend_to_fork_into_parent()
         self.patch(start_cluster_controller, 'sleep')
         self.patch(MAASDispatcher, 'dispatch_query').side_effect = URLError(
             "Simulated HTTP failure.")
 
         start_cluster_controller.start_up(
-            make_url(), self.make_connection_details())
+            make_url(), self.make_connection_details(),
+            factory.make_name('user'), factory.make_name('group'))
 
-        self.assertNotEqual(0, start_cluster_controller.Popen.call_count)
+        self.assertEqual(1, os.fork.call_count)