[711] made BoB.uid/gid "private", and raise a separate exception if setXX fails

src/bin/bind10/bind10_src.py.in

@@ -105,6 +105,16 @@ _BASETIME = time.gmtime()
 
 class ProcessInfoError(Exception): pass
 
+class ChangeUserError(Exception):
+    '''Exception raised when setuid/setgid fails.
+
+    When raised, it's expected to be propagated via underlying component
+    management modules to the top level so that it will help provide useful
+    fatal error message.
+
+    '''
+    pass
+
 class ProcessInfo:
     """Information about a process"""
 
@@ -206,8 +216,8 @@ class BoB:
         # restart. Components manage their own restart schedule now
         self.components_to_restart = []
         self.runnable = False
-        self.uid = setuid
-        self.gid = setgid
+        self.__uid = setuid
+        self.__gid = setgid
         self.username = username
         self.verbose = verbose
         self.nokill = nokill
@@ -277,12 +287,22 @@ class BoB:
         (i.e., once all components that need the privilege of the original
         user have started).
         '''
-        if self.gid is not None:
-            logger.info(BIND10_SETGID, self.gid)
-            posix.setgid(self.gid)
-        if self.uid is not None:
-            logger.info(BIND10_SETUID, self.uid)
-            posix.setuid(self.uid)
+        try:
+            if self.__gid is not None:
+                logger.info(BIND10_SETGID, self.__gid)
+                posix.setgid(self.__gid)
+        except Exception as ex:
+            raise ChangeUserError('failed to change group: ' + str(ex))
+
+        try:
+            if self.__uid is not None:
+                posix.setuid(self.__uid)
+                # We use one-shot logger after setuid here.  This will
+                # detect any permission issue regarding logging due to the
+                # result of setuid at the earliest opportunity.
+                isc.log.Logger("boss").info(BIND10_SETUID, self.__uid)
+        except Exception as ex:
+            raise ChangeUserError('failed to change user: ' + str(ex))
 
     def config_handler(self, new_config):
         # If this is initial update, don't do anything now, leave it to startup
@@ -593,7 +613,7 @@ class BoB:
             are pure speculation.  As with the auth daemon, they should be
             read from the configuration database.
         """
-        if self.uid is not None and self.__started:
+        if self.__uid is not None and self.__started:
             logger.warn(BIND10_START_AS_NON_ROOT_RESOLVER)
         self.curproc = "b10-resolver"
         # XXX: this must be read from the configuration manager in the future

src/bin/bind10/tests/bind10_test.py.in

@@ -342,12 +342,16 @@ class TestCacheCommands(unittest.TestCase):
 
 class TestBoB(unittest.TestCase):
     def setUp(self):
+        # Save original values that may be tweaked in some tests
         self.__orig_setgid = bind10_src.posix.setgid
         self.__orig_setuid = bind10_src.posix.setuid
+        self.__orig_logger_class = isc.log.Logger
 
     def tearDown(self):
+        # Restore original values saved in setUp()
         bind10_src.posix.setgid = self.__orig_setgid
         bind10_src.posix.setuid = self.__orig_setuid
+        isc.log.Logger = self.__orig_logger_class
 
     def test_init(self):
         bob = BoB()
@@ -357,7 +361,6 @@ class TestBoB(unittest.TestCase):
         self.assertEqual(bob.ccs, None)
         self.assertEqual(bob.components, {})
         self.assertEqual(bob.runnable, False)
-        self.assertEqual(bob.uid, None)
         self.assertEqual(bob.username, None)
         self.assertIsNone(bob._socket_cache)
 
@@ -379,13 +382,35 @@ class TestBoB(unittest.TestCase):
         self.assertIsNone(self.__gid_set)
         self.assertIsNone(self.__uid_set)
 
-        bob.gid = 4200
-        bob.uid = 42
-        bob.change_user()
+        BoB(setuid=42, setgid=4200).change_user()
         # This time, it get's called
         self.assertEqual(4200, self.__gid_set)
         self.assertEqual(42, self.__uid_set)
 
+        def raising_set_xid(gid_or_uid):
+            ex = OSError()
+            ex.errno, ex.strerror = errno.EPERM, 'Operation not permitted'
+            raise ex
+
+        # Let setgid raise an exception
+        bind10_src.posix.setgid = raising_set_xid
+        bind10_src.posix.setuid = self.__setuid
+        self.assertRaises(bind10_src.ChangeUserError,
+                          BoB(setuid=42, setgid=4200).change_user)
+
+        # Let setuid raise an exception
+        bind10_src.posix.setgid = self.__setgid
+        bind10_src.posix.setuid = raising_set_xid
+        self.assertRaises(bind10_src.ChangeUserError,
+                          BoB(setuid=42, setgid=4200).change_user)
+
+        # Let initial log output after setuid raise an exception
+        bind10_src.posix.setgid = self.__setgid
+        bind10_src.posix.setuid = self.__setuid
+        isc.log.Logger = raising_set_xid
+        self.assertRaises(bind10_src.ChangeUserError,
+                          BoB(setuid=42, setgid=4200).change_user)
+
     def test_set_creator(self):
         """
         Test the call to set_creator. First time, the cache is created
@@ -456,7 +481,6 @@ class TestBoB(unittest.TestCase):
         self.assertEqual(bob.ccs, None)
         self.assertEqual(bob.components, {})
         self.assertEqual(bob.runnable, False)
-        self.assertEqual(bob.uid, None)
         self.assertEqual(bob.username, None)
 
     def test_command_handler(self):