launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/setuid-setgid-ordering into lp:maas.

Commit message:
Revert the accidental change of setuid/setgid ordering in rev 1150, and ensures that groups are looked up with grp.getgrnam.

Previously pwd.getpwnam was being used, but this looks in the passwd database, not the group database. It was working because it's common for users to have a group of the same name, with the same gid as their uid.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/setuid-setgid-ordering/+merge/127801
-- 
https://code.launchpad.net/~allenap/maas/setuid-setgid-ordering/+merge/127801
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/setuid-setgid-ordering into lp:maas.

=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py	2012-10-03 15:10:46 +0000
+++ src/provisioningserver/start_cluster_controller.py	2012-10-03 15:31:23 +0000
@@ -15,6 +15,7 @@
     'run',
     ]
 
+from grp import getgrnam
 import httplib
 import json
 import os
@@ -129,7 +130,7 @@
 def start_celery(connection_details, user, group):
     broker_url = connection_details['BROKER_URL']
     uid = getpwnam(user).pw_uid
-    gid = getpwnam(group).pw_gid
+    gid = getgrnam(group).gr_gid
 
     # Copy environment, but also tell celeryd what broker to listen to.
     env = dict(os.environ, CELERY_BROKER_URL=broker_url)
@@ -145,8 +146,8 @@
 
     # Change gid first, just in case changing the uid might deprive
     # us of the privileges required to setgid.
+    os.setgid(gid)
     os.setuid(uid)
-    os.setgid(gid)
 
     os.execvpe(command[0], command, env=env)
 

=== modified file 'src/provisioningserver/tests/test_start_cluster_controller.py'
--- src/provisioningserver/tests/test_start_cluster_controller.py	2012-10-03 11:35:18 +0000
+++ src/provisioningserver/tests/test_start_cluster_controller.py	2012-10-03 15:31:23 +0000
@@ -18,7 +18,6 @@
 from io import BytesIO
 import json
 import os
-from random import randint
 from urllib2 import (
     HTTPError,
     URLError,
@@ -28,6 +27,10 @@
 from apiclient.testing.django import parse_headers_and_body_with_django
 from fixtures import EnvironmentVariableFixture
 from maastesting.factory import factory
+from mock import (
+    call,
+    sentinel,
+    )
 from provisioningserver import start_cluster_controller
 from provisioningserver.testing.testcase import PservTestCase
 
@@ -85,8 +88,7 @@
         # raise exceptions.
         self.patch(start_cluster_controller, 'sleep').side_effect = Sleeping()
         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(start_cluster_controller, 'getgrnam')
         self.patch(os, 'setuid')
         self.patch(os, 'setgid')
         self.patch(os, 'execvpe').side_effect = Executing()
@@ -240,3 +242,22 @@
             factory.make_name('user'), factory.make_name('group'))
 
         self.assertEqual(1, os.execvpe.call_count)
+
+    def test_start_celery_sets_gid_before_uid(self):
+        # The gid should be changed before the uid; it may not be possible to
+        # change the gid once privileges are dropped.
+        start_cluster_controller.getpwnam.return_value.pw_uid = sentinel.uid
+        start_cluster_controller.getgrnam.return_value.gr_gid = sentinel.gid
+        # Patch setuid and setgid, using the same mock for both, so that we
+        # can observe call ordering.
+        setuidgid = self.patch(os, "setuid")
+        self.patch(os, "setgid", setuidgid)
+        self.assertRaises(
+            Executing, start_cluster_controller.start_celery,
+            self.make_connection_details(), factory.make_name("user"),
+            factory.make_name("group"))
+        # The arguments to the mocked setuid/setgid calls demonstrate that the
+        # gid was selected first.
+        self.assertEqual(
+            [call(sentinel.gid), call(sentinel.uid)],
+            setuidgid.call_args_list)