Repository: Azure/WALinuxAgent
Pull request #1281 (2018-07-31 21:56:33)

Delete JIT accounts
Delete flow. The intention here was to also send status back up through the wire service as something exposed to the fabric controller, but after talking with the Host Agent team, we'll need to make some changes their first so that will be implemented at a later date.

jasonzio: I thought about your solution. It works, and I'm not going to request a change, but it's not the best choice. Any newly-written tests should avoid assertRaisesRegexp, since that was a dead-end tool that was replaced by assertRaiseRegex. Adding an implementation of the deprecated method might be seen as encouraging its use; more accurately, deliberately leaving the deprecated method *unimplemented* would be seen as discouraging its use in favor of the newer approach.

As I said, what you've done is acceptable, and there's no need to change it.
matkin-msft: Not sure I follow.  It shouldn't encourage usage of assertRaisesRegexp in the tests... at least no more than before this change was made.  If you use assertRaisesRegexp it will still fail when runing python 3.2 or later.  This change just says if assertRaisesRegexp exists (then we know you're on 2.7-3.1) so create a new method assertRaisesRegex that just happens to use assertRaisesRegexp

---
In reply to: [410066544](https://github.com/Azure/WALinuxAgent/pull/1281#issuecomment-410066544) [](ancestors = 410066544)

Patch:
diff --git a/azurelinuxagent/common/exception.py b/azurelinuxagent/common/exception.py
index 46846560..7e144616 100644
--- a/azurelinuxagent/common/exception.py
+++ b/azurelinuxagent/common/exception.py
@@ -160,3 +160,12 @@ class ResourceGoneError(HttpError):
         if msg is None:
             msg = "Resource is gone"
         super(ResourceGoneError, self).__init__(msg, inner)
+
+
+class RemoteAccessError(AgentError):
+    """
+    Remote Access Error
+    """
+
+    def __init__(self, msg=None, inner=None):
+        super(RemoteAccessError, self).__init__(msg, inner)
diff --git a/azurelinuxagent/ga/remoteaccess.py b/azurelinuxagent/ga/remoteaccess.py
index 9be2176b..d11d39e3 100644
--- a/azurelinuxagent/ga/remoteaccess.py
+++ b/azurelinuxagent/ga/remoteaccess.py
@@ -46,7 +46,7 @@ from pwd import getpwall
 from azurelinuxagent.common.errorstate import ErrorState
 
 from azurelinuxagent.common.event import add_event, WALAEventOperation, elapsed_milliseconds
-from azurelinuxagent.common.exception import ExtensionError, ProtocolError
+from azurelinuxagent.common.exception import ExtensionError, ProtocolError, RemoteAccessError
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.protocol.restapi import ExtHandlerStatus, \
                                                     ExtensionStatus, \
@@ -80,6 +80,7 @@ class RemoteAccessHandler(object):
         self.cryptUtil = CryptUtil(conf.get_openssl_cmd())
         self.remote_access = None
         self.incarnation = 0
+        self.error_message = ""
 
     def run(self):
         try:
@@ -90,8 +91,7 @@ class RemoteAccessHandler(object):
                     # something changed. Handle remote access if any.
                     self.incarnation = current_incarnation
                     self.remote_access = self.protocol.client.get_remote_access()
-                    if self.remote_access is not None:
-                        self.handle_remote_access()
+                    self.handle_remote_access()
         except Exception as e:
             msg = u"Exception processing remote access handler: {0} {1}".format(ustr(e), traceback.format_exc())
             logger.error(msg)
@@ -102,19 +102,44 @@ class RemoteAccessHandler(object):
                       message=msg)
 
     def handle_remote_access(self):
+        # Get JIT user accounts.
+        all_users = self.os_util.get_users()
+        existing_jit_users = set(u[0] for u in all_users if self.validate_jit_user(u[4]))
+        self.err_message = ""
         if self.remote_access is not None:
-            # Get JIT user accounts.
-            all_users = self.os_util.get_users()
-            jit_users = set()
-            for usr in all_users:
-                if self.validate_jit_user(usr[4]):
-                    jit_users.add(usr[0])
+            goal_state_users = set(u.name for u in self.remote_access.user_list.users)
             for acc in self.remote_access.user_list.users:
-                raw_expiration = acc.expiration
-                account_expiration = datetime.strptime(raw_expiration, REMOTE_USR_EXPIRATION_FORMAT)
-                now = datetime.utcnow()
-                if acc.name not in jit_users and now < account_expiration:
-                    self.add_user(acc.name, acc.encrypted_password, account_expiration)
+                try:
+                    raw_expiration = acc.expiration
+                    account_expiration = datetime.strptime(raw_expiration, REMOTE_USR_EXPIRATION_FORMAT)
+                    now = datetime.utcnow()
+                    if acc.name not in existing_jit_users and now < account_expiration:
+                        self.add_user(acc.name, acc.encrypted_password, account_expiration)
+                    elif acc.name in existing_jit_users and now > account_expiration:
+                        # user account expired, delete it.
+                        logger.info("user {0} expired from remote_access".format(acc.name))
+                        self.remove_user(acc.name)
+                except RemoteAccessError as rae:
+                    self.err_message = self.err_message + "Error processing user {0}. Exception: {1}"\
+                        .format(acc.name, ustr(rae))
+            for user in existing_jit_users:
+                try:
+                    if user not in goal_state_users:
+                        # user explicitly removed
+                        logger.info("User {0} removed from remote_access".format(user))
+                        self.remove_user(user)
+                except RemoteAccessError as rae:
+                    self.err_message = self.err_message + "Error removing user {0}. Exception: {1}"\
+                        .format(user, ustr(rae))
+        else:
+            # All users removed, remove any remaining JIT accounts.
+            for user in existing_jit_users:
+                try:
+                    logger.info("User {0} removed from remote_access. remote_access empty".format(user))
+                    self.remove_user(user)
+                except RemoteAccessError as rae:
+                    self.err_message = self.err_message + "Error removing user {0}. Exception: {1}"\
+                        .format(user, ustr(rae))
 
     def validate_jit_user(self, comment):
         return comment == REMOTE_ACCESS_ACCOUNT_COMMENT
@@ -122,40 +147,36 @@ class RemoteAccessHandler(object):
     def add_user(self, username, encrypted_password, account_expiration):
         try:
             expiration_date = (account_expiration + timedelta(days=1)).strftime(DATE_FORMAT)
-            logger.verbose("Adding user {0} with expiration date {1}"
-                           .format(username, expiration_date))
+            logger.verbose("Adding user {0} with expiration date {1}".format(username, expiration_date))
             self.os_util.useradd(username, expiration_date, REMOTE_ACCESS_ACCOUNT_COMMENT)
-        except OSError as oe:
-            logger.error("Error adding user {0}. {1}"
-                         .format(username, oe.strerror))
-            return
         except Exception as e:
-            logger.error("Error adding user {0}. {1}".format(username, ustr(e)))
-            return
+            raise RemoteAccessError("Error adding user {0}. {1}".format(username, ustr(e)))
         try:
             prv_key = os.path.join(conf.get_lib_dir(), TRANSPORT_PRIVATE_CERT)
             pwd = self.cryptUtil.decrypt_secret(encrypted_password, prv_key)
             self.os_util.chpasswd(username, pwd, conf.get_password_cryptid(), conf.get_password_crypt_salt_len())
             self.os_util.conf_sudoer(username)
-            logger.info("User '{0}' added successfully with expiration in {1}"
-                        .format(username, expiration_date))
-            return
-        except OSError as oe:
-            self.handle_failed_create(username, oe.strerror)
+            logger.info("User '{0}' added successfully with expiration in {1}".format(username, expiration_date))
         except Exception as e:
-            self.handle_failed_create(username, ustr(e))
+            error = "Error adding user {0}. {1} ".format(username, str(e))
+            try:
+                self.handle_failed_create(username)
+                error += "cleanup successful"
+            except RemoteAccessError as rae:
+                error += "and error cleaning up {0}".format(str(rae))
+            raise RemoteAccessError("Error adding user {0} cleanup successful".format(username), ustr(e))
 
-    def handle_failed_create(self, username, error_message):
-        logger.error("Error creating user {0}. {1}"
-                     .format(username, error_message))
+    def handle_failed_create(self, username):
         try:
             self.delete_user(username)
-        except OSError as oe:
-            logger.error("Failed to clean up after account creation for {0}. {1}"
-                         .format(username, oe.strerror()))
         except Exception as e:
-            logger.error("Failed to clean up after account creation for {0}. {1}"
-                         .format(username, str(e)))
+            raise RemoteAccessError("Failed to clean up after account creation for {0}.".format(username), e)
+
+    def remove_user(self, username):
+        try:
+            self.delete_user(username)
+        except Exception as e:
+            raise RemoteAccessError("Failed to delete user {0}".format(username), e)
 
     def delete_user(self, username):
         self.os_util.del_account(username)


Test patch:
diff --git a/tests/ga/test_remoteaccess_handler.py b/tests/ga/test_remoteaccess_handler.py
index d0147351..71271c97 100644
--- a/tests/ga/test_remoteaccess_handler.py
+++ b/tests/ga/test_remoteaccess_handler.py
@@ -16,6 +16,8 @@
 #
 
 from datetime import timedelta
+
+from azurelinuxagent.common.exception import RemoteAccessError
 from azurelinuxagent.common.protocol.wire import *
 from azurelinuxagent.ga.remoteaccess import RemoteAccessHandler
 from tests.common.osutil.mock_osutil import MockOSUtil
@@ -41,12 +43,19 @@ def log_error(msg_format, *args):
     error_messages.append(msg_format.format(args))
 
 
+def mock_add_event(name, op, is_success, version, message):
+    TestRemoteAccessHandler.eventing_data = (name, op, is_success, version, message)
+
+
 class TestRemoteAccessHandler(AgentTestCase):
+    eventing_data = [()]
 
     def setUp(self):
         super(TestRemoteAccessHandler, self).setUp()
         del info_messages[:]
         del error_messages[:]
+        for data in TestRemoteAccessHandler.eventing_data:
+            del data
 
     # add_user tests
     @patch('azurelinuxagent.common.logger.Logger.info', side_effect=log_info)
@@ -79,14 +88,13 @@ class TestRemoteAccessHandler(AgentTestCase):
         rah.os_util = MockOSUtil()
         tstpassword = "]aPPEv}uNg1FPnl?"
         tstuser = ""
-        expiration_date = datetime.utcnow() + timedelta(days=1)
+        expiration = datetime.utcnow() + timedelta(days=1)
         pwd = tstpassword
-        rah.add_user(tstuser, pwd, expiration_date)
+        error = "Error adding user {0}. test exception for bad username".format(tstuser)
+        self.assertRaisesRegex(RemoteAccessError, error, rah.add_user, tstuser, pwd, expiration)
         self.assertEqual(0, len(rah.os_util.get_users()))
-        self.assertEqual(1, len(error_messages))
+        self.assertEqual(0, len(error_messages))
         self.assertEqual(0, len(info_messages))
-        error = "Error adding user {0}. test exception for bad username".format(tstuser)
-        self.assertEqual(error, error_messages[0])
 
     @patch('azurelinuxagent.common.logger.Logger.info', side_effect=log_info)
     @patch('azurelinuxagent.common.logger.Logger.error', side_effect=log_error)
@@ -96,16 +104,16 @@ class TestRemoteAccessHandler(AgentTestCase):
         rah.os_util = MockOSUtil()
         tstpassword = ""
         tstuser = "foobar"
-        expiration_date = datetime.utcnow() + timedelta(days=1)
+        expiration = datetime.utcnow() + timedelta(days=1)
         pwd = tstpassword
-        rah.add_user(tstuser, pwd, expiration_date)
+        error = "Error adding user {0} cleanup successful\nInner error: test exception for bad password".format(tstuser)
+        self.assertRaisesRegex(RemoteAccessError, error, rah.add_user, tstuser, pwd, expiration)
         self.assertEqual(0, len(rah.os_util.get_users()))
-        self.assertEqual(1, len(error_messages))
+        self.assertEqual(0, len(error_messages))
         self.assertEqual(1, len(info_messages))
-        error = "Error creating user {0}. test exception for bad password".format(tstuser)
-        self.assertEqual(error, error_messages[0])
         self.assertEqual("User deleted {0}".format(tstuser), info_messages[0])
 
+
     @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
            return_value="]aPPEv}uNg1FPnl?")
     def test_add_user_already_existing(self, _):
@@ -125,7 +133,7 @@ class TestRemoteAccessHandler(AgentTestCase):
         # this does not test the user add function as that's mocked, it tests processing skips the remaining
         # calls after the initial failure
         new_user_expiration = datetime.utcnow() + timedelta(days=5)
-        rah.add_user(tstuser, pwd, new_user_expiration)
+        self.assertRaises(RemoteAccessError, rah.add_user, tstuser, pwd, new_user_expiration)
         # refresh users
         users = get_user_dictionary(rah.os_util.get_users())
         self.assertTrue(tstuser in users, "{0} missing from users after dup user attempted".format(tstuser))
@@ -164,7 +172,7 @@ class TestRemoteAccessHandler(AgentTestCase):
         mock_os_util.all_users[testusr] = (testusr, None, None, None, None, None, None, None)
         rah = RemoteAccessHandler()
         rah.os_util = mock_os_util
-        rah.handle_failed_create("", "test message")
+        self.assertRaises(RemoteAccessError, rah.handle_failed_create, "")
         users = get_user_dictionary(rah.os_util.get_users())
         self.assertEqual(1, len(users.keys()))
         self.assertTrue(testusr in users, "Expected user {0} missing".format(testusr))
@@ -178,17 +186,14 @@ class TestRemoteAccessHandler(AgentTestCase):
         rah = RemoteAccessHandler()
         rah.os_util = mock_os_util
         testuser = "Carl"
-        test_message = "test message"
-        rah.handle_failed_create(testuser, test_message)
+        error = "Failed to clean up after account creation for {0}.\n" \
+                "Inner error: test exception, user does not exist to delete".format(testuser)
+        self.assertRaisesRegex(RemoteAccessError, error, rah.handle_failed_create, testuser)
         users = get_user_dictionary(rah.os_util.get_users())
         self.assertEqual(1, len(users.keys()))
         self.assertTrue(testusr in users, "Expected user {0} missing".format(testusr))
-        self.assertEqual(2, len(error_messages))
+        self.assertEqual(0, len(error_messages))
         self.assertEqual(0, len(info_messages))
-        self.assertEqual("Error creating user {0}. {1}".format(testuser, test_message), error_messages[0])
-        msg = "Failed to clean up after account creation for {0}. test exception, user does not exist to delete"\
-            .format(testuser)
-        self.assertEqual(msg, error_messages[1])
 
     @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
            return_value="]aPPEv}uNg1FPnl?")
@@ -230,14 +235,14 @@ class TestRemoteAccessHandler(AgentTestCase):
         tstuser = "foobar"
         expiration = datetime.utcnow() + timedelta(days=1)
         pwd = "bad password"
-        rah.add_user(tstuser, pwd, expiration)
+        error = "Error adding user foobar cleanup successful\n" \
+                "Inner error: \[CryptError\] Error decoding secret\n" \
+                "Inner error: Incorrect padding".format(tstuser)
+        self.assertRaisesRegex(RemoteAccessError, error, rah.add_user, tstuser, pwd, expiration)
         users = get_user_dictionary(rah.os_util.get_users())
         self.assertEqual(0, len(users))
-        self.assertEqual(1, len(error_messages))
+        self.assertEqual(0, len(error_messages))
         self.assertEqual(1, len(info_messages))
-        error = "Error creating user {0}. [CryptError] Error decoding secret\nInner error: Incorrect padding".\
-            format(tstuser)
-        self.assertEqual(error, error_messages[0])
         self.assertEqual("User deleted {0}".format(tstuser), info_messages[0])
 
     def test_handle_remote_access_no_users(self):
@@ -395,3 +400,128 @@ class TestRemoteAccessHandler(AgentTestCase):
         self.assertTrue(tstuser in users, "{0} missing from users".format(tstuser))
         rah.run()
         self.assertTrue(tstuser in users, "{0} missing from users".format(tstuser))
+
+    @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
+           return_value="]aPPEv}uNg1FPnl?")
+    def test_handle_remote_access_multiple_users_one_removed(self, _):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        data_str = load_data('wire/remote_access_10_accounts.xml')
+        remote_access = RemoteAccess(data_str)
+        count = 0
+        for user in remote_access.user_list.users:
+            count += 1
+            user.name = "tstuser{0}".format(count)
+            expiration_date = datetime.utcnow() + timedelta(days=count)
+            user.expiration = expiration_date.strftime("%a, %d %b %Y %H:%M:%S ") + "UTC"
+        rah.remote_access = remote_access
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(10, len(users))
+        # now remove the user from RemoteAccess
+        deleted_user = rah.remote_access.user_list.users[3]
+        del rah.remote_access.user_list.users[3]
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertTrue(deleted_user not in users, "{0} still in users".format(deleted_user))
+        self.assertEqual(9, len(users))
+
+    @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
+           return_value="]aPPEv}uNg1FPnl?")
+    def test_handle_remote_access_multiple_users_null_remote_access(self, _):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        data_str = load_data('wire/remote_access_10_accounts.xml')
+        remote_access = RemoteAccess(data_str)
+        count = 0
+        for user in remote_access.user_list.users:
+            count += 1
+            user.name = "tstuser{0}".format(count)
+            expiration_date = datetime.utcnow() + timedelta(days=count)
+            user.expiration = expiration_date.strftime("%a, %d %b %Y %H:%M:%S ") + "UTC"
+        rah.remote_access = remote_access
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(10, len(users))
+        # now remove the user from RemoteAccess
+        rah.remote_access = None
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(0, len(users))
+
+    @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
+           return_value="]aPPEv}uNg1FPnl?")
+    def test_handle_remote_access_multiple_users_error_with_null_remote_access(self, _):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        data_str = load_data('wire/remote_access_10_accounts.xml')
+        remote_access = RemoteAccess(data_str)
+        count = 0
+        for user in remote_access.user_list.users:
+            count += 1
+            user.name = "tstuser{0}".format(count)
+            expiration_date = datetime.utcnow() + timedelta(days=count)
+            user.expiration = expiration_date.strftime("%a, %d %b %Y %H:%M:%S ") + "UTC"
+        rah.remote_access = remote_access
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(10, len(users))
+        # now remove the user from RemoteAccess
+        rah.remote_access = None
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(0, len(users))
+
+    def test_remove_user_error(self):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        error = "Failed to delete user {0}\nInner error: test exception, bad data".format("")
+        self.assertRaisesRegex(RemoteAccessError, error, rah.remove_user, "")
+
+    def test_remove_user_not_exists(self):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        user = "bob"
+        error = "Failed to delete user {0}\n" \
+                "Inner error: test exception, user does not exist to delete".format(user)
+        self.assertRaisesRegex(RemoteAccessError, error, rah.remove_user, user)
+
+    @patch('azurelinuxagent.common.utils.cryptutil.CryptUtil.decrypt_secret',
+           return_value="]aPPEv}uNg1FPnl?")
+    def test_handle_remote_access_remove_and_add(self, _):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        data_str = load_data('wire/remote_access_10_accounts.xml')
+        remote_access = RemoteAccess(data_str)
+        count = 0
+        for user in remote_access.user_list.users:
+            count += 1
+            user.name = "tstuser{0}".format(count)
+            expiration_date = datetime.utcnow() + timedelta(days=count)
+            user.expiration = expiration_date.strftime("%a, %d %b %Y %H:%M:%S ") + "UTC"
+        rah.remote_access = remote_access
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertEqual(10, len(users))
+        # now remove the user from RemoteAccess
+        new_user = "tstuser11"
+        deleted_user = rah.remote_access.user_list.users[3]
+        rah.remote_access.user_list.users[3].name = new_user
+        rah.handle_remote_access()
+        users = rah.os_util.get_users()
+        self.assertTrue(deleted_user not in users, "{0} still in users".format(deleted_user))
+        self.assertTrue(new_user in [u[0] for u in users], "user {0} not in users".format(new_user))
+        self.assertEqual(10, len(users))
+
+    @patch('azurelinuxagent.ga.remoteaccess.add_event', side_effect=mock_add_event)
+    @patch('azurelinuxagent.common.protocol.util.ProtocolUtil.get_protocol', side_effect=RemoteAccessError("foobar!"))
+    def test_remote_access_handler_run_error(self, _1, _2):
+        rah = RemoteAccessHandler()
+        rah.os_util = MockOSUtil()
+        rah.run()
+        print(TestRemoteAccessHandler.eventing_data)
+        check_message = "foobar!"
+        self.assertTrue(check_message in TestRemoteAccessHandler.eventing_data[4],
+                        "expected message {0} not found in {1}"
+                        .format(check_message, TestRemoteAccessHandler.eventing_data[4]))
+        self.assertEqual(False, TestRemoteAccessHandler.eventing_data[2], "is_success is true")
diff --git a/tests/tools.py b/tests/tools.py
index e6bc4501..22278e31 100644
--- a/tests/tools.py
+++ b/tests/tools.py
@@ -123,6 +123,10 @@ class AgentTestCase(unittest.TestCase):
             cls.assertIsNone = cls.emulate_assertIsNone
         if not hasattr(cls, "assertIsNotNone"):
             cls.assertIsNone = cls.emulate_assertIsNotNone
+        if hasattr(cls, "assertRaisesRegexp"):
+            cls.assertRaisesRegex = cls.assertRaisesRegexp
+        if not hasattr(cls, "assertRaisesRegex"):
+            cls.assertRaisesRegex = cls.emulate_raises_regex
         if not hasattr(cls, "assertListEqual"):
             cls.assertListEqual = cls.emulate_assertListEqual
 
@@ -177,6 +181,18 @@ class AgentTestCase(unittest.TestCase):
             msg = msg if msg is not None else '{0} is None'.format(_safe_repr(x))
             self.fail(msg)
 
+
+    def emulate_raises_regex(self, exception_type, regex, function, *args, **kwargs):
+        try:
+            function(*args, **kwargs)
+        except Exception as e:
+            if re.search(regex, str(e), flags=1) is not None:
+                return
+            else:
+                self.fail("Expected exception {0} matching {1}.  Actual: {2}".format(
+                    exception_type, regex, str(e)))
+        self.fail("No exception was thrown.  Expected exception {0} matching {1}".format(exception_type, regex))
+
     def emulate_assertListEqual(self, seq1, seq2, msg=None, seq_type=None):
         """An equality assertion for ordered sequences (like lists and tuples).
 
@@ -280,7 +296,6 @@ class AgentTestCase(unittest.TestCase):
         msg = self._formatMessage(msg, standardMsg)
         self.fail(msg)
 
-
     @staticmethod
     def _create_files(tmp_dir, prefix, suffix, count, with_sleep=0):
         for i in range(count):