From 5452084bb54810c645d48d6b0a18073c95710171 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 15 Jun 2023 14:08:16 +0000 Subject: [PATCH 01/12] Drop messages from banned dns - compares message signer with dns in banned dns list - which is got from config file and sorted through --- ssm/agents.py | 31 +++++++++++++++++++++++++++++++ ssm/ssm2.py | 14 ++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/ssm/agents.py b/ssm/agents.py index 46e401d7..14c13704 100644 --- a/ssm/agents.py +++ b/ssm/agents.py @@ -298,6 +298,10 @@ def run_receiver(protocol, brokers, project, token, cp, log, dn_file): dns = get_dns(dn_file, log) ssm.set_dns(dns) + log.info('Fetching banned DNs.') + banned_dns = get_banned_dns(log) + ssm.set_banned_dns(banned_dns) + except Exception as e: log.fatal('Failed to initialise SSM: %s', e) log.info(LOG_BREAK) @@ -379,3 +383,30 @@ def get_dns(dn_file, log): log.debug('%s DNs found.', len(dns)) return dns + + +def get_banned_dns(log): + """Retrieve the list of banned dns""" + banned_dns_list = [] + try: + banned_dns_path = cp.get('auth', 'banned-dns') + banned_dns_file = os.path.normpath(os.path.expandvars(banned_dns_path)) + except ConfigParser.NoOptionError: + banned_dns_file = None + f = None + try: + f = open(banned_dns_file, 'r') + lines = f.readlines() + for line in lines: + if line.isspace() or line.strip().startswith('#'): + continue + elif line.strip().startswith('/'): + banned_dns_list.append(line.strip()) + else: + log.warning('DN in banned dns list is not in correct format: %s', line) + finally: + if f is not None: + f.close() + + return banned_dns_list + diff --git a/ssm/ssm2.py b/ssm/ssm2.py index 72099ad8..3108155a 100644 --- a/ssm/ssm2.py +++ b/ssm/ssm2.py @@ -90,6 +90,7 @@ def __init__(self, hosts_and_ports, qpath, cert, key, dest=None, listen=None, self._dest = dest self._valid_dns = [] + self._banned_dns = [] self._pidfile = pidfile # Used to differentiate between STOMP and AMS methods @@ -189,6 +190,10 @@ def set_dns(self, dn_list): """Set the list of DNs which are allowed to sign incoming messages.""" self._valid_dns = dn_list + def set_banned_dns(self, banned_dn_list): + """Set the list of banned dns, so their messages can be dropped.""" + self._banned_dns = banned_dn_list + ########################################################################## # Methods called by stomppy ########################################################################## @@ -283,8 +288,10 @@ def _handle_msg(self, text): Namely: - decrypt if necessary - verify signature + - drop the message if the sender is a banned dn - Return plain-text message, signer's DN and an error/None. """ + if text is None or text == '': warning = 'Empty text passed to _handle_msg.' log.warning(warning) @@ -307,6 +314,13 @@ def _handle_msg(self, text): log.error(error) return None, None, error + # If the message has been sent from a banned dn, + # don't send the message to the reject queue. + # Instead, just drop the message. + if signer in self._banned_dns: + log.info("Message deleted as was sent from banned dn: %s", signer) + return + if signer not in self._valid_dns: warning = 'Signer not in valid DNs list: %s' % signer log.warning(warning) From 728b8d7542e411adb4f77e39b52fad2b20b1143c Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 21 Jun 2023 10:07:15 +0000 Subject: [PATCH 02/12] Change method of dealing with messsages from banned dns - now makes more sense - has an error message - reworking of some if/elif/else statements --- ssm/ssm2.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/ssm/ssm2.py b/ssm/ssm2.py index 3108155a..bad24bbe 100644 --- a/ssm/ssm2.py +++ b/ssm/ssm2.py @@ -288,7 +288,7 @@ def _handle_msg(self, text): Namely: - decrypt if necessary - verify signature - - drop the message if the sender is a banned dn + - send an error message if the message wasn't sent from a valid DN - Return plain-text message, signer's DN and an error/None. """ @@ -314,17 +314,23 @@ def _handle_msg(self, text): log.error(error) return None, None, error - # If the message has been sent from a banned dn, - # don't send the message to the reject queue. - # Instead, just drop the message. + # If the message has been sent from a banned DN, + # set a specific error message that can be + # checked for later. if signer in self._banned_dns: - log.info("Message deleted as was sent from banned dn: %s", signer) - return + warning = 'Signer is in the banned DNs list' + log.warning(warning) + return None, signer, warning - if signer not in self._valid_dns: + # Else, if the signer is not in valid DNs list, + # but also not a banned dn, + # set a specific error message + elif signer not in self._valid_dns: warning = 'Signer not in valid DNs list: %s' % signer log.warning(warning) return None, signer, warning + + # Else, the message has been sent from a valid DN else: log.info('Valid signer: %s', signer) @@ -334,9 +340,15 @@ def _save_msg_to_queue(self, body, empaid): """Extract message contents and add to the accept or reject queue.""" extracted_msg, signer, err_msg = self._handle_msg(body) try: + # If the warning states the message was sent from a banned DN, + # don't send the message to the reject queue. + # Instead, drop the message (don't send it to any queue) + if err_msg == "Signer is in the banned DNs list": + log.info("Message dropped as was sent from a banned dn: %s", signer) + # If the message is empty or the error message is not empty # then reject the message. - if extracted_msg is None or err_msg is not None: + elif extracted_msg is None or err_msg is not None: if signer is None: # crypto failed signer = 'Not available.' elif extracted_msg is not None: From 87a90679c1d1b41661e124d7a29150a49088b70d Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 20 Jul 2023 15:14:08 +0000 Subject: [PATCH 03/12] Initial workings on unit test --- test/test_ssm.py | 227 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 226 insertions(+), 1 deletion(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 5f96fd78..0c9f9046 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -4,11 +4,17 @@ import shutil import tempfile import unittest -from subprocess import call +import dirq +from subprocess import call, Popen, PIPE +import logging from ssm.message_directory import MessageDirectory from ssm.ssm2 import Ssm2, Ssm2Exception +logging.basicConfig() + +schema = {"body": "string", "signer": "string", + "empaid": "string?", "error": "string?"} class TestSsm(unittest.TestCase): ''' @@ -134,6 +140,225 @@ def test_ssm_init_non_dirq(self): # Assert the outbound queue is of the expected type. self.assertTrue(isinstance(ssm._outq, MessageDirectory)) +class TestMsgToQueue(unittest.TestCase): + ''' + Class used for testing how messages are sent to queues based + upon the DN that sent them. + ''' + + def setUp(self): + # Create temporary directory for message queues and pidfiles + self.dir_path = tempfile.mkdtemp() + + """Set up a test directory and certificates.""" + self._tmp_dir = tempfile.mkdtemp(prefix='ssm') + + # Below is not currently being used, and was an alternate method + # I would like to keep it until the test is working + """ + # Some functions require the hardcoded expired certificate and + # key to be files. + key_fd, self._key_path = tempfile.mkstemp(prefix='key', + dir=self._tmp_dir) + os.close(key_fd) + with open(self._key_path, 'w') as key: + key.write(TEST_KEY) + + cert_fd, self._expired_cert_path = tempfile.mkstemp(prefix='cert', + dir=self._tmp_dir) + os.close(cert_fd) + with open(self._expired_cert_path, 'w') as cert: + cert.write(TEST_CERT_FILE) + + valid_dn_file, self.valid_dn_path = tempfile.mkstemp( + prefix='valid', dir=self._tmp_dir) + os.close(valid_dn_file) + with open(self.valid_dn_path, 'w') as dn: + dn.write('/test/dn') + + # Create a new certificate using the hardcoded key. + # The subject has been hardcoded so that the generated + # certificate subject matches the subject of the hardcoded, + # expired, certificate at the bottom of this file. + # 2 days used so that verify_cert_date doesn't think it expires soon. + call(['openssl', 'req', '-x509', '-nodes', '-days', '2', '-new', + '-key', self._key_path, '-out', TEST_CERT_FILE, + '-subj', '/C=UK/O=STFC/OU=SC/CN=Test Cert']) + """ + + self._brokers = [('not.a.broker', 123)] + self._capath = '/not/a/path' + self._check_crls = False + self._pidfile = self._tmp_dir + '/pidfile' + + self._listen = '/topic/test' + self._dest = '/topic/test' + + self._msgdir = tempfile.mkdtemp(prefix='msgq') + + + def test_banned_dns_not_saved_to_queue(self): + ''' + Test that messages sent from banned dns are dropped + and not sent to the accept or reject queue. + ''' + + # we need to setup an incoming and reject queue to make sure + # the banned messages aren't passed into either of them + in_q = dirq.queue.Queue(os.path.join(self._tmp_dir, 'incoming'), + schema=schema) + + re_q = dirq.queue.Queue(os.path.join(self._tmp_dir, 'reject'), + schema=schema) + + # create a list of fake valid dns that will send the messages + # to make sure these aren't sent to the reject queue + valid_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-1.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-2.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-3.esc.rl.ac.uk") + + # create a list of fake banned dns that feature in the dn list + # these should be dropped, and not sent to a queue + banned_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-1.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-2.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-3.esc.rl.ac.uk") + + # to check the valid dns aren't sent to the reject queue + # for each dn in the valid dn list + # method needs to be passed message and empaid + # empaid can just be 1 + # message needs to contain the body, a signer dn from the dn list, + # and an error msg (none) + for dn in valid_dns: + # create a key/cert pair + call(['openssl', 'req', '-x509', '-nodes', '-days', '2', + '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, + '-out', TEST_CERT_FILE, '-subj', dn]) + + # Set up an openssl-style CA directory, containing the + # self-signed certificate as its own CA certificate, but with its + # name as .0. + p1 = Popen(['openssl', 'x509', '-subject_hash', '-noout'], + stdin=PIPE, stdout=PIPE, stderr=PIPE, + universal_newlines=True) + + with open(TEST_CERT_FILE, 'r') as test_cert: + cert_string = test_cert.read() + + hash_name, _unused_error = p1.communicate(cert_string) + + self.ca_certpath = os.path.join(TEST_CA_DIR, hash_name.strip() + '.0') + with open(self.ca_certpath, 'w') as ca_cert: + ca_cert.write(cert_string) + + empaid = "1" + message_valid = {"body": """APEL-summary-job-message: v0.2 + Site: RAL-LCG2 + Month: 3 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %% + Site: RAL-LCG2 + Month: 4 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %%""", + "signer": dn, "empaid": empaid, "error": ""} + + ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, + TEST_KEY_FILE, dest=self._dest, listen=self._listen, + capath=self.ca_certpath) + ssm._save_msg_to_queue(message_valid["body"], empaid) + + # check the valid message hasn't been sent to the reject queue + self.assertEquals(re_q.count(), 0) + + # check the valid message reached the incoming queue + self.assertEquals(in_q.count(), 1) + + + # to check the banned dns aren't sent to a queue + # for each dn in the banned dn list + # method needs to be passed message and empaid + # empaid can just be 1 + # message needs to contain the body, a signer dn from the dn list, + # and an error msg (Signer is in the banned DNs list) + for dn in banned_dns: + # create a key/cert pair + call(['openssl', 'req', '-x509', '-nodes', '-days', '2', + '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, + '-out', TEST_CERT_FILE, '-subj', dn]) + + # Set up an openssl-style CA directory, containing the + # self-signed certificate as its own CA certificate, but with its + # name as .0. + p1 = Popen(['openssl', 'x509', '-subject_hash', '-noout'], + stdin=PIPE, stdout=PIPE, stderr=PIPE, + universal_newlines=True) + + with open(TEST_CERT_FILE, 'r') as test_cert: + cert_string = test_cert.read() + + hash_name, _unused_error = p1.communicate(cert_string) + + self.ca_certpath = os.path.join(TEST_CA_DIR, hash_name.strip() + '.0') + with open(self.ca_certpath, 'w') as ca_cert: + ca_cert.write(cert_string) + + + empaid = "1" + message_banned = {"body": """APEL-summary-job-message: v0.2 + Site: RAL-LCG2 + Month: 3 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %% + Site: RAL-LCG2 + Month: 4 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %%""", + "signer": dn, "empaid": empaid, "error": "Signer is in the banned DNs list"} + + ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, + TEST_KEY_FILE, dest=self._dest, listen=self._listen, + capath=self.ca_certpath) + ssm._save_msg_to_queue(message_banned["body"], empaid) + + # check the banned message hasn't been sent to the reject queue + self.assertEquals(re_q.count(), 0) + + # check the banned message hasn't been sent to the incoming queue + self.assertEquals(in_q.count(), 0) + + +TEST_KEY_FILE = '/tmp/test.key' + +TEST_CA_DIR='/tmp' TEST_CERT_FILE = '/tmp/test.crt' From ded312198ab6ceda15675bee8ae51e189cf71033 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 18 Sep 2023 15:39:53 +0000 Subject: [PATCH 04/12] Add mocking of _handle_msg() function to prevent errors - Mock out the _handle_msg() function called by _save_msg_to_queue() (the function we are testing), as it fails due to client signers and certificates not matching. - It is easier to mock out the failing function instead, as it is not the function we are testing in this test. - To test _save_msg_to_queue, we are assuming the message's certificate and signer match. --- ssm/ssm2.py | 2 +- test/test_ssm.py | 61 +++++++++++++++++++++--------------------------- 2 files changed, 27 insertions(+), 36 deletions(-) diff --git a/ssm/ssm2.py b/ssm/ssm2.py index bad24bbe..e0a21643 100644 --- a/ssm/ssm2.py +++ b/ssm/ssm2.py @@ -344,7 +344,7 @@ def _save_msg_to_queue(self, body, empaid): # don't send the message to the reject queue. # Instead, drop the message (don't send it to any queue) if err_msg == "Signer is in the banned DNs list": - log.info("Message dropped as was sent from a banned dn: %s", signer) + log.warning("Message dropped as was sent from a banned dn: %s", signer) # If the message is empty or the error message is not empty # then reject the message. diff --git a/test/test_ssm.py b/test/test_ssm.py index 0c9f9046..894d254b 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -5,6 +5,8 @@ import tempfile import unittest import dirq +import mock +from mock import patch from subprocess import call, Popen, PIPE import logging @@ -144,6 +146,13 @@ class TestMsgToQueue(unittest.TestCase): ''' Class used for testing how messages are sent to queues based upon the DN that sent them. + The _handle_msg() function called by _save_msg_to_queue() + (the function we are testing) is being mocked, as it fails + due to client signers and certificates not matching. + It is easier to mock out the failing function instead, as it is + not the function we are testing in this test. To test + _save_msg_to_queue, we are assuming the message's certificate and + signer match. ''' def setUp(self): @@ -196,8 +205,8 @@ def setUp(self): self._msgdir = tempfile.mkdtemp(prefix='msgq') - - def test_banned_dns_not_saved_to_queue(self): + @patch.object(Ssm2, '_handle_msg') + def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ''' Test that messages sent from banned dns are dropped and not sent to the accept or reject queue. @@ -223,6 +232,8 @@ def test_banned_dns_not_saved_to_queue(self): "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-3.esc.rl.ac.uk") + empaid = "1" + # to check the valid dns aren't sent to the reject queue # for each dn in the valid dn list # method needs to be passed message and empaid @@ -251,8 +262,7 @@ def test_banned_dns_not_saved_to_queue(self): with open(self.ca_certpath, 'w') as ca_cert: ca_cert.write(cert_string) - empaid = "1" - message_valid = {"body": """APEL-summary-job-message: v0.2 + message_valid = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 Month: 3 Year: 2010 @@ -263,24 +273,14 @@ def test_banned_dns_not_saved_to_queue(self): WallDuration: 234256 CpuDuration: 2345 NumberOfJobs: 100 - %% - Site: RAL-LCG2 - Month: 4 - Year: 2010 - GlobalUserName: """ + dn + """ - VO: atlas - VOGroup: /atlas - VORole: Role=production - WallDuration: 234256 - CpuDuration: 2345 - NumberOfJobs: 100 - %%""", - "signer": dn, "empaid": empaid, "error": ""} + %%""" + + mock_handle_msg.return_value = message_valid, dn, None ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, TEST_KEY_FILE, dest=self._dest, listen=self._listen, capath=self.ca_certpath) - ssm._save_msg_to_queue(message_valid["body"], empaid) + ssm._save_msg_to_queue(message_valid, empaid) # check the valid message hasn't been sent to the reject queue self.assertEquals(re_q.count(), 0) @@ -288,6 +288,7 @@ def test_banned_dns_not_saved_to_queue(self): # check the valid message reached the incoming queue self.assertEquals(in_q.count(), 1) + print("Success!\n") # to check the banned dns aren't sent to a queue # for each dn in the banned dn list @@ -317,9 +318,7 @@ def test_banned_dns_not_saved_to_queue(self): with open(self.ca_certpath, 'w') as ca_cert: ca_cert.write(cert_string) - - empaid = "1" - message_banned = {"body": """APEL-summary-job-message: v0.2 + message_banned = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 Month: 3 Year: 2010 @@ -330,24 +329,14 @@ def test_banned_dns_not_saved_to_queue(self): WallDuration: 234256 CpuDuration: 2345 NumberOfJobs: 100 - %% - Site: RAL-LCG2 - Month: 4 - Year: 2010 - GlobalUserName: """ + dn + """ - VO: atlas - VOGroup: /atlas - VORole: Role=production - WallDuration: 234256 - CpuDuration: 2345 - NumberOfJobs: 100 - %%""", - "signer": dn, "empaid": empaid, "error": "Signer is in the banned DNs list"} + %%""" + + mock_handle_msg.return_value = message_banned, dn, "Signer is in the banned DNs list" ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, TEST_KEY_FILE, dest=self._dest, listen=self._listen, capath=self.ca_certpath) - ssm._save_msg_to_queue(message_banned["body"], empaid) + ssm._save_msg_to_queue(message_banned, empaid) # check the banned message hasn't been sent to the reject queue self.assertEquals(re_q.count(), 0) @@ -355,6 +344,8 @@ def test_banned_dns_not_saved_to_queue(self): # check the banned message hasn't been sent to the incoming queue self.assertEquals(in_q.count(), 0) + print("Success!\n") + TEST_KEY_FILE = '/tmp/test.key' From b3ea5fef5279360c0e66222ee4345d5c8f640471 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Tue, 19 Sep 2023 10:32:12 +0000 Subject: [PATCH 05/12] Simplify command line output and remove unused queues --- test/test_ssm.py | 98 +++++++++++++++++------------------------------- 1 file changed, 34 insertions(+), 64 deletions(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 894d254b..890fef97 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -13,7 +13,7 @@ from ssm.message_directory import MessageDirectory from ssm.ssm2 import Ssm2, Ssm2Exception -logging.basicConfig() +logging.basicConfig(level=logging.INFO) schema = {"body": "string", "signer": "string", "empaid": "string?", "error": "string?"} @@ -210,16 +210,11 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ''' Test that messages sent from banned dns are dropped and not sent to the accept or reject queue. + The logging output will tell us if the message + was sent to the correct queue, or dropped completely. + Therefore we don't need to create incoming or reject queues ''' - # we need to setup an incoming and reject queue to make sure - # the banned messages aren't passed into either of them - in_q = dirq.queue.Queue(os.path.join(self._tmp_dir, 'incoming'), - schema=schema) - - re_q = dirq.queue.Queue(os.path.join(self._tmp_dir, 'reject'), - schema=schema) - # create a list of fake valid dns that will send the messages # to make sure these aren't sent to the reject queue valid_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-1.esc.rl.ac.uk", @@ -232,35 +227,30 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-3.esc.rl.ac.uk") + # a custom empaid isn't necessary, and can just be 1 empaid = "1" - # to check the valid dns aren't sent to the reject queue - # for each dn in the valid dn list - # method needs to be passed message and empaid - # empaid can just be 1 - # message needs to contain the body, a signer dn from the dn list, - # and an error msg (none) - for dn in valid_dns: - # create a key/cert pair - call(['openssl', 'req', '-x509', '-nodes', '-days', '2', - '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, - '-out', TEST_CERT_FILE, '-subj', dn]) + # Set up an openssl-style CA directory, containing the + # self-signed certificate as its own CA certificate, but with its + # name as .0. + p1 = Popen(['openssl', 'x509', '-subject_hash', '-noout'], + stdin=PIPE, stdout=PIPE, stderr=PIPE, + universal_newlines=True) - # Set up an openssl-style CA directory, containing the - # self-signed certificate as its own CA certificate, but with its - # name as .0. - p1 = Popen(['openssl', 'x509', '-subject_hash', '-noout'], - stdin=PIPE, stdout=PIPE, stderr=PIPE, - universal_newlines=True) + with open(TEST_CERT_FILE, 'r') as test_cert: + cert_string = test_cert.read() - with open(TEST_CERT_FILE, 'r') as test_cert: - cert_string = test_cert.read() + hash_name, _unused_error = p1.communicate(cert_string) - hash_name, _unused_error = p1.communicate(cert_string) + self.ca_certpath = os.path.join(TEST_CA_DIR, hash_name.strip() + '.0') + with open(self.ca_certpath, 'w') as ca_cert: + ca_cert.write(cert_string) - self.ca_certpath = os.path.join(TEST_CA_DIR, hash_name.strip() + '.0') - with open(self.ca_certpath, 'w') as ca_cert: - ca_cert.write(cert_string) + # For each dn in the valid dns list, + # Pass it and the message to ssm and use the log output to + # make sure it was dealt with correctly + for dn in valid_dns: + print("Testing dn:", dn) message_valid = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 @@ -280,43 +270,22 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, TEST_KEY_FILE, dest=self._dest, listen=self._listen, capath=self.ca_certpath) + ssm._save_msg_to_queue(message_valid, empaid) # check the valid message hasn't been sent to the reject queue - self.assertEquals(re_q.count(), 0) + #self.assertEquals(re_q.count(), 0) # check the valid message reached the incoming queue - self.assertEquals(in_q.count(), 1) + #self.assertEquals(in_q.count(), 1) - print("Success!\n") + print("") - # to check the banned dns aren't sent to a queue - # for each dn in the banned dn list - # method needs to be passed message and empaid - # empaid can just be 1 - # message needs to contain the body, a signer dn from the dn list, - # and an error msg (Signer is in the banned DNs list) + # For each dn in the banned dns list, + # Pass it and the message to ssm and use the log output to + # make sure it was dealt with correctly for dn in banned_dns: - # create a key/cert pair - call(['openssl', 'req', '-x509', '-nodes', '-days', '2', - '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, - '-out', TEST_CERT_FILE, '-subj', dn]) - - # Set up an openssl-style CA directory, containing the - # self-signed certificate as its own CA certificate, but with its - # name as .0. - p1 = Popen(['openssl', 'x509', '-subject_hash', '-noout'], - stdin=PIPE, stdout=PIPE, stderr=PIPE, - universal_newlines=True) - - with open(TEST_CERT_FILE, 'r') as test_cert: - cert_string = test_cert.read() - - hash_name, _unused_error = p1.communicate(cert_string) - - self.ca_certpath = os.path.join(TEST_CA_DIR, hash_name.strip() + '.0') - with open(self.ca_certpath, 'w') as ca_cert: - ca_cert.write(cert_string) + print("Testing dn:", dn) message_banned = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 @@ -336,15 +305,16 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, TEST_KEY_FILE, dest=self._dest, listen=self._listen, capath=self.ca_certpath) + ssm._save_msg_to_queue(message_banned, empaid) # check the banned message hasn't been sent to the reject queue - self.assertEquals(re_q.count(), 0) + #self.assertEquals(re_q.count(), 0) # check the banned message hasn't been sent to the incoming queue - self.assertEquals(in_q.count(), 0) + #self.assertEquals(in_q.count(), 0) - print("Success!\n") + print("") TEST_KEY_FILE = '/tmp/test.key' From 4024cf92daccded8967860395290d8717ca73332 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Tue, 19 Sep 2023 16:20:09 +0000 Subject: [PATCH 06/12] Removing unnecessary code - this was outdated code no longer in use --- test/test_ssm.py | 56 ++++-------------------------------------------- 1 file changed, 4 insertions(+), 52 deletions(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 890fef97..8a89a761 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -4,8 +4,6 @@ import shutil import tempfile import unittest -import dirq -import mock from mock import patch from subprocess import call, Popen, PIPE import logging @@ -15,9 +13,6 @@ logging.basicConfig(level=logging.INFO) -schema = {"body": "string", "signer": "string", - "empaid": "string?", "error": "string?"} - class TestSsm(unittest.TestCase): ''' Class used for testing SSM. @@ -153,48 +148,17 @@ class TestMsgToQueue(unittest.TestCase): not the function we are testing in this test. To test _save_msg_to_queue, we are assuming the message's certificate and signer match. + We then use the log output from ssm2.py to see if the messages + are sent to the queues we expect them to be. ''' def setUp(self): # Create temporary directory for message queues and pidfiles self.dir_path = tempfile.mkdtemp() - """Set up a test directory and certificates.""" + # Set up a test directory and certificates self._tmp_dir = tempfile.mkdtemp(prefix='ssm') - # Below is not currently being used, and was an alternate method - # I would like to keep it until the test is working - """ - # Some functions require the hardcoded expired certificate and - # key to be files. - key_fd, self._key_path = tempfile.mkstemp(prefix='key', - dir=self._tmp_dir) - os.close(key_fd) - with open(self._key_path, 'w') as key: - key.write(TEST_KEY) - - cert_fd, self._expired_cert_path = tempfile.mkstemp(prefix='cert', - dir=self._tmp_dir) - os.close(cert_fd) - with open(self._expired_cert_path, 'w') as cert: - cert.write(TEST_CERT_FILE) - - valid_dn_file, self.valid_dn_path = tempfile.mkstemp( - prefix='valid', dir=self._tmp_dir) - os.close(valid_dn_file) - with open(self.valid_dn_path, 'w') as dn: - dn.write('/test/dn') - - # Create a new certificate using the hardcoded key. - # The subject has been hardcoded so that the generated - # certificate subject matches the subject of the hardcoded, - # expired, certificate at the bottom of this file. - # 2 days used so that verify_cert_date doesn't think it expires soon. - call(['openssl', 'req', '-x509', '-nodes', '-days', '2', '-new', - '-key', self._key_path, '-out', TEST_CERT_FILE, - '-subj', '/C=UK/O=STFC/OU=SC/CN=Test Cert']) - """ - self._brokers = [('not.a.broker', 123)] self._capath = '/not/a/path' self._check_crls = False @@ -216,7 +180,7 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ''' # create a list of fake valid dns that will send the messages - # to make sure these aren't sent to the reject queue + # these should be sent to the incoming queue valid_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-1.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-3.esc.rl.ac.uk") @@ -273,12 +237,6 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ssm._save_msg_to_queue(message_valid, empaid) - # check the valid message hasn't been sent to the reject queue - #self.assertEquals(re_q.count(), 0) - - # check the valid message reached the incoming queue - #self.assertEquals(in_q.count(), 1) - print("") # For each dn in the banned dns list, @@ -308,12 +266,6 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): ssm._save_msg_to_queue(message_banned, empaid) - # check the banned message hasn't been sent to the reject queue - #self.assertEquals(re_q.count(), 0) - - # check the banned message hasn't been sent to the incoming queue - #self.assertEquals(in_q.count(), 0) - print("") From 60c8f37fdbfb9fb5546e7da334eb8f1153431596 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 20 Sep 2023 10:02:09 +0000 Subject: [PATCH 07/12] Add assertion tests using LogCapture - we can check the log output matches an expected string - to check the test has passed or failed --- test/test_ssm.py | 101 +++++++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 42 deletions(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 8a89a761..7a1269e7 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -8,6 +8,11 @@ from subprocess import call, Popen, PIPE import logging +# For Python 2.7, make sure testfixtures version is < 7.0.0 +# testfixtures version 6.18.5 works fine +# run: pip install testfixtures==6.18.5 +from testfixtures import LogCapture + from ssm.message_directory import MessageDirectory from ssm.ssm2 import Ssm2, Ssm2Exception @@ -214,59 +219,71 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): # Pass it and the message to ssm and use the log output to # make sure it was dealt with correctly for dn in valid_dns: - print("Testing dn:", dn) - - message_valid = """APEL-summary-job-message: v0.2 - Site: RAL-LCG2 - Month: 3 - Year: 2010 - GlobalUserName: """ + dn + """ - VO: atlas - VOGroup: /atlas - VORole: Role=production - WallDuration: 234256 - CpuDuration: 2345 - NumberOfJobs: 100 - %%""" - - mock_handle_msg.return_value = message_valid, dn, None + # Capture the log output so we can use it in assertions + with LogCapture() as log: + print("Testing dn:", dn) - ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, - TEST_KEY_FILE, dest=self._dest, listen=self._listen, - capath=self.ca_certpath) + message_valid = """APEL-summary-job-message: v0.2 + Site: RAL-LCG2 + Month: 3 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %%""" + + mock_handle_msg.return_value = message_valid, dn, None + + ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, + TEST_KEY_FILE, dest=self._dest, listen=self._listen, + capath=self.ca_certpath) + + ssm._save_msg_to_queue(message_valid, empaid) + + print(str(log)) - ssm._save_msg_to_queue(message_valid, empaid) + self.assertIn('Message saved to incoming queue', str(log)) - print("") + print("Test Passed.\n") # For each dn in the banned dns list, # Pass it and the message to ssm and use the log output to # make sure it was dealt with correctly for dn in banned_dns: - print("Testing dn:", dn) - - message_banned = """APEL-summary-job-message: v0.2 - Site: RAL-LCG2 - Month: 3 - Year: 2010 - GlobalUserName: """ + dn + """ - VO: atlas - VOGroup: /atlas - VORole: Role=production - WallDuration: 234256 - CpuDuration: 2345 - NumberOfJobs: 100 - %%""" - - mock_handle_msg.return_value = message_banned, dn, "Signer is in the banned DNs list" + # Capture the log output so we can use it in assertions + with LogCapture() as log: + print("Testing dn:", dn) - ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, - TEST_KEY_FILE, dest=self._dest, listen=self._listen, - capath=self.ca_certpath) + message_banned = """APEL-summary-job-message: v0.2 + Site: RAL-LCG2 + Month: 3 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %%""" + + mock_handle_msg.return_value = message_banned, dn, "Signer is in the banned DNs list" + + ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, + TEST_KEY_FILE, dest=self._dest, listen=self._listen, + capath=self.ca_certpath) + + ssm._save_msg_to_queue(message_banned, empaid) + + print(str(log)) - ssm._save_msg_to_queue(message_banned, empaid) + self.assertIn('Message dropped as was sent from a banned dn', str(log)) - print("") + print("Test Passed.\n") TEST_KEY_FILE = '/tmp/test.key' From e47b15ef6b2b7172f3f291ea53c9a9ec657b8479 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 20 Sep 2023 12:06:18 +0000 Subject: [PATCH 08/12] Add testing for messages being sent to the reject queue - also re-adds the creation of the certificate which was missing --- test/test_ssm.py | 76 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 9 deletions(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 7a1269e7..fb032d45 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -174,24 +174,36 @@ def setUp(self): self._msgdir = tempfile.mkdtemp(prefix='msgq') + # create a key/cert pair + call(['openssl', 'req', '-x509', '-nodes', '-days', '2', + '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, + '-out', TEST_CERT_FILE, '-subj', + '/C=UK/O=STFC/OU=SC/CN=Test Cert']) + @patch.object(Ssm2, '_handle_msg') - def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): + def test_dns_saved_to_queue(self, mock_handle_msg): ''' - Test that messages sent from banned dns are dropped - and not sent to the accept or reject queue. - The logging output will tell us if the message + Test that messages sent from different dns are sent + to the correct queue or dropped, depending on their dn. + The logging output is checked to tell us if the message was sent to the correct queue, or dropped completely. - Therefore we don't need to create incoming or reject queues + Therefore we don't need to create incoming or reject queues. ''' - # create a list of fake valid dns that will send the messages - # these should be sent to the incoming queue + # Create a list of fake valid dns that will send the messages + # These should be sent to the incoming queue valid_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-1.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=valid-3.esc.rl.ac.uk") - # create a list of fake banned dns that feature in the dn list - # these should be dropped, and not sent to a queue + # Create a list of fake dns that will result in a rejected message + # These should be sent to the rejected queue + rejected_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=rejected-1.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=rejected-2.esc.rl.ac.uk", + "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=rejected-3.esc.rl.ac.uk") + + # Create a list of fake banned dns that feature in the dn list + # These should be dropped, and not sent to a queue banned_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-1.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-3.esc.rl.ac.uk") @@ -250,6 +262,52 @@ def test_banned_dns_not_saved_to_queue(self, mock_handle_msg): print("Test Passed.\n") + # As there are several different ways messages can be rejected, + # Keep a count to test a different method for each dn + dnCount = 1 + for dn in rejected_dns: + # Capture the log output so we can use it in assertions + with LogCapture() as log: + print("Testing dn:", dn) + + message_rejected = """APEL-summary-job-message: v0.2 + Site: RAL-LCG2 + Month: 3 + Year: 2010 + GlobalUserName: """ + dn + """ + VO: atlas + VOGroup: /atlas + VORole: Role=production + WallDuration: 234256 + CpuDuration: 2345 + NumberOfJobs: 100 + %%""" + + # Change the reason for method rejection for each dn + if dnCount == 1: + # Pass no message, which will also need an error message + mock_handle_msg.return_value = None, dn, "Empty text passed to _handle_msg" + elif dnCount == 2: + # Pass an error message + mock_handle_msg.return_value = message_rejected, dn, "Signer not in valid DNs list" + else: + # Pass a different error message + mock_handle_msg.return_value = message_rejected, dn, "Failed to verify message" + + ssm = Ssm2(self._brokers, self._msgdir, TEST_CERT_FILE, + TEST_KEY_FILE, dest=self._dest, listen=self._listen, + capath=self.ca_certpath) + + ssm._save_msg_to_queue(message_rejected, empaid) + + print(str(log)) + + self.assertIn('Message saved to reject queue', str(log)) + + print("Test Passed.\n") + + dnCount = dnCount + 1 + # For each dn in the banned dns list, # Pass it and the message to ssm and use the log output to # make sure it was dealt with correctly From 7ef624b9485a3ac94f14b3fc3af92204d57fce6b Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 27 Sep 2023 11:27:18 +0000 Subject: [PATCH 09/12] Address comments from code review - removes printing in test - rewrites line using with context manager - changes variable name - passes cp as an argument to the fucntion - shortens long lines --- ssm/agents.py | 31 ++++++++++++++++--------------- test/test_ssm.py | 12 ------------ 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/ssm/agents.py b/ssm/agents.py index 14c13704..bc853002 100644 --- a/ssm/agents.py +++ b/ssm/agents.py @@ -299,7 +299,7 @@ def run_receiver(protocol, brokers, project, token, cp, log, dn_file): ssm.set_dns(dns) log.info('Fetching banned DNs.') - banned_dns = get_banned_dns(log) + banned_dns = get_banned_dns(log, cp) ssm.set_banned_dns(banned_dns) except Exception as e: @@ -385,28 +385,29 @@ def get_dns(dn_file, log): return dns -def get_banned_dns(log): +def get_banned_dns(log, cp): """Retrieve the list of banned dns""" - banned_dns_list = [] + banned_dns = [] try: banned_dns_path = cp.get('auth', 'banned-dns') - banned_dns_file = os.path.normpath(os.path.expandvars(banned_dns_path)) + banned_dns_file = os.path.normpath( + os.path.expandvars(banned_dns_path)) except ConfigParser.NoOptionError: banned_dns_file = None f = None try: - f = open(banned_dns_file, 'r') - lines = f.readlines() - for line in lines: - if line.isspace() or line.strip().startswith('#'): - continue - elif line.strip().startswith('/'): - banned_dns_list.append(line.strip()) - else: - log.warning('DN in banned dns list is not in correct format: %s', line) + with open(banned_dns_file, 'r') as f: + lines = f.readlines() + for line in lines: + if line.isspace() or line.strip().startswith('#'): + continue + elif line.strip().startswith('/'): + banned_dns.append(line.strip()) + else: + log.warning('DN in banned dns list is not in ' + 'the correct format: %s', line) finally: if f is not None: f.close() - return banned_dns_list - + return banned_dns diff --git a/test/test_ssm.py b/test/test_ssm.py index fb032d45..32f63b7e 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -233,8 +233,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg): for dn in valid_dns: # Capture the log output so we can use it in assertions with LogCapture() as log: - print("Testing dn:", dn) - message_valid = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 Month: 3 @@ -260,16 +258,12 @@ def test_dns_saved_to_queue(self, mock_handle_msg): self.assertIn('Message saved to incoming queue', str(log)) - print("Test Passed.\n") - # As there are several different ways messages can be rejected, # Keep a count to test a different method for each dn dnCount = 1 for dn in rejected_dns: # Capture the log output so we can use it in assertions with LogCapture() as log: - print("Testing dn:", dn) - message_rejected = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 Month: 3 @@ -304,8 +298,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg): self.assertIn('Message saved to reject queue', str(log)) - print("Test Passed.\n") - dnCount = dnCount + 1 # For each dn in the banned dns list, @@ -314,8 +306,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg): for dn in banned_dns: # Capture the log output so we can use it in assertions with LogCapture() as log: - print("Testing dn:", dn) - message_banned = """APEL-summary-job-message: v0.2 Site: RAL-LCG2 Month: 3 @@ -341,8 +331,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg): self.assertIn('Message dropped as was sent from a banned dn', str(log)) - print("Test Passed.\n") - TEST_KEY_FILE = '/tmp/test.key' From 6b5ef754b82e4f119b674e0242bb3ad5b01a94e5 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 28 Sep 2023 16:30:45 +0000 Subject: [PATCH 10/12] Add testfixtures to requirements-test.txt - this makes it available in Travis - used in class TestMsgToQueue --- requirements-test.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements-test.txt b/requirements-test.txt index 131ea0e8..6c530e58 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -3,4 +3,5 @@ coveralls<=1.2.0 mock<4.0.0 # Pinned because version 4 dropped support for Python 2.7 codecov +testfixtures<7.0.0 pre-commit From 72438628e0b7073eee65172402a35e2e9cf53c03 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 28 Sep 2023 16:32:28 +0000 Subject: [PATCH 11/12] Add tear-down and remove final print output - removes remaining print statements to simplify logging output - adds teardown - also improves comments --- test/test_ssm.py | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/test/test_ssm.py b/test/test_ssm.py index 32f63b7e..51773e32 100644 --- a/test/test_ssm.py +++ b/test/test_ssm.py @@ -174,12 +174,22 @@ def setUp(self): self._msgdir = tempfile.mkdtemp(prefix='msgq') - # create a key/cert pair + # Create a key/cert pair call(['openssl', 'req', '-x509', '-nodes', '-days', '2', '-newkey', 'rsa:2048', '-keyout', TEST_KEY_FILE, '-out', TEST_CERT_FILE, '-subj', '/C=UK/O=STFC/OU=SC/CN=Test Cert']) + def tearDown(self): + # Remove test directory and all contents + try: + shutil.rmtree(self._msgdir) + shutil.rmtree(self._tmp_dir) + except OSError as e: + print('Error removing temporary directory %s' % self._tmp_dir) + print(e) + + @patch.object(Ssm2, '_handle_msg') def test_dns_saved_to_queue(self, mock_handle_msg): ''' @@ -202,13 +212,13 @@ def test_dns_saved_to_queue(self, mock_handle_msg): "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=rejected-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=rejected-3.esc.rl.ac.uk") - # Create a list of fake banned dns that feature in the dn list + # Create a list of fake banned dns that feature in the banned dn list # These should be dropped, and not sent to a queue banned_dns = ("/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-1.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-2.esc.rl.ac.uk", "/C=UK/O=eScience/OU=CLRC/L=RAL/CN=banned-3.esc.rl.ac.uk") - # a custom empaid isn't necessary, and can just be 1 + # A custom empaid isn't necessary, and can just be 1 empaid = "1" # Set up an openssl-style CA directory, containing the @@ -228,8 +238,8 @@ def test_dns_saved_to_queue(self, mock_handle_msg): ca_cert.write(cert_string) # For each dn in the valid dns list, - # Pass it and the message to ssm and use the log output to - # make sure it was dealt with correctly + # pass it and the message to ssm and use the log output to + # make sure it was dealt with correctly. for dn in valid_dns: # Capture the log output so we can use it in assertions with LogCapture() as log: @@ -254,12 +264,13 @@ def test_dns_saved_to_queue(self, mock_handle_msg): ssm._save_msg_to_queue(message_valid, empaid) - print(str(log)) - self.assertIn('Message saved to incoming queue', str(log)) + # For each dn in the rejected dns list, + # pass it and the message to ssm and use the log output to + # make sure it was dealt with correctly. # As there are several different ways messages can be rejected, - # Keep a count to test a different method for each dn + # keep a count to test a different method for each dn dnCount = 1 for dn in rejected_dns: # Capture the log output so we can use it in assertions @@ -294,15 +305,13 @@ def test_dns_saved_to_queue(self, mock_handle_msg): ssm._save_msg_to_queue(message_rejected, empaid) - print(str(log)) - self.assertIn('Message saved to reject queue', str(log)) dnCount = dnCount + 1 # For each dn in the banned dns list, - # Pass it and the message to ssm and use the log output to - # make sure it was dealt with correctly + # pass it and the message to ssm and use the log output to + # make sure it was dealt with correctly. for dn in banned_dns: # Capture the log output so we can use it in assertions with LogCapture() as log: @@ -327,8 +336,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg): ssm._save_msg_to_queue(message_banned, empaid) - print(str(log)) - self.assertIn('Message dropped as was sent from a banned dn', str(log)) From 0cabbe50da098f936a8031f5962ece4005a3e5cc Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 28 Sep 2023 16:33:54 +0000 Subject: [PATCH 12/12] Removes unnecessary try...finally wrapper - with the addition of a context manager for a file in an earlier commit, this wasn't needed --- ssm/agents.py | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/ssm/agents.py b/ssm/agents.py index ea37704b..9f40e7bf 100644 --- a/ssm/agents.py +++ b/ssm/agents.py @@ -407,20 +407,16 @@ def get_banned_dns(log, cp): os.path.expandvars(banned_dns_path)) except ConfigParser.NoOptionError: banned_dns_file = None - f = None - try: - with open(banned_dns_file, 'r') as f: - lines = f.readlines() - for line in lines: - if line.isspace() or line.strip().startswith('#'): - continue - elif line.strip().startswith('/'): - banned_dns.append(line.strip()) - else: - log.warning('DN in banned dns list is not in ' - 'the correct format: %s', line) - finally: - if f is not None: - f.close() + + with open(banned_dns_file, 'r') as f: + lines = f.readlines() + for line in lines: + if line.isspace() or line.strip().startswith('#'): + continue + elif line.strip().startswith('/'): + banned_dns.append(line.strip()) + else: + log.warning('DN in banned dns list is not in ' + 'the correct format: %s', line) return banned_dns