From 533e01a3f997a98d01fb8e3a35376e4aefefe8aa Mon Sep 17 00:00:00 2001 From: quidame Date: Mon, 5 Apr 2021 14:40:36 +0200 Subject: [PATCH] java_keystore/fix 1667 improve temp files storage (#2163) * improve temporary files storage (naming/removal) * update unit tests * Update changelogs/fragments/2163-java_keystore_1667_improve_temp_files_storage.yml Co-authored-by: Felix Fontein * add dedicated function to randomize PKCS#12 filename fix unit tests (also mock the new function) Co-authored-by: Felix Fontein --- ...ystore_1667_improve_temp_files_storage.yml | 5 +++ plugins/modules/system/java_keystore.py | 32 +++++++++------- .../modules/system/test_java_keystore.py | 38 +++++++++++++------ 3 files changed, 51 insertions(+), 24 deletions(-) create mode 100644 changelogs/fragments/2163-java_keystore_1667_improve_temp_files_storage.yml diff --git a/changelogs/fragments/2163-java_keystore_1667_improve_temp_files_storage.yml b/changelogs/fragments/2163-java_keystore_1667_improve_temp_files_storage.yml new file mode 100644 index 0000000000..43d183707c --- /dev/null +++ b/changelogs/fragments/2163-java_keystore_1667_improve_temp_files_storage.yml @@ -0,0 +1,5 @@ +--- +bugfixes: + - "java_keystore - use tempfile lib to create temporary files with randomized + names, and remove the temporary PKCS#12 keystore as well as other materials + (https://github.com/ansible-collections/community.general/issues/1667)." diff --git a/plugins/modules/system/java_keystore.py b/plugins/modules/system/java_keystore.py index db37bdee91..82bd03678c 100644 --- a/plugins/modules/system/java_keystore.py +++ b/plugins/modules/system/java_keystore.py @@ -114,13 +114,15 @@ cmd: description: Executed command to get action done returned: changed and failure type: str - sample: "openssl x509 -noout -in /tmp/cert.crt -fingerprint -sha256" + sample: "/usr/bin/openssl x509 -noout -in /tmp/user/1000/tmp8jd_lh23 -fingerprint -sha256" ''' -from ansible.module_utils.basic import AnsibleModule import os import re +import tempfile + +from ansible.module_utils.basic import AnsibleModule def read_certificate_fingerprint(module, openssl_bin, certificate_path): @@ -170,18 +172,25 @@ def run_commands(module, cmd, data=None, check_rc=True): return module.run_command(cmd, check_rc=check_rc, data=data) -def create_file(path, content): - with open(path, 'w') as f: +def create_path(): + tmpfd, tmpfile = tempfile.mkstemp() + os.remove(tmpfile) + return tmpfile + + +def create_file(content): + tmpfd, tmpfile = tempfile.mkstemp() + with os.fdopen(tmpfd, 'w') as f: f.write(content) - return path + return tmpfile def create_tmp_certificate(module): - return create_file("/tmp/%s.crt" % module.params['name'], module.params['certificate']) + return create_file(module.params['certificate']) def create_tmp_private_key(module): - return create_file("/tmp/%s.key" % module.params['name'], module.params['private_key']) + return create_file(module.params['private_key']) def cert_changed(module, openssl_bin, keytool_bin, keystore_path, keystore_pass, alias): @@ -200,17 +209,13 @@ def create_jks(module, name, openssl_bin, keytool_bin, keystore_path, password, else: certificate_path = create_tmp_certificate(module) private_key_path = create_tmp_private_key(module) + keystore_p12_path = create_path() try: if os.path.exists(keystore_path): os.remove(keystore_path) - keystore_p12_path = "/tmp/keystore.p12" - if os.path.exists(keystore_p12_path): - os.remove(keystore_p12_path) - export_p12_cmd = [openssl_bin, "pkcs12", "-export", "-name", name, "-in", certificate_path, - "-inkey", private_key_path, "-out", - keystore_p12_path, "-passout", "stdin"] + "-inkey", private_key_path, "-out", keystore_p12_path, "-passout", "stdin"] # when keypass is provided, add -passin cmd_stdin = "" @@ -249,6 +254,7 @@ def create_jks(module, name, openssl_bin, keytool_bin, keystore_path, password, finally: os.remove(certificate_path) os.remove(private_key_path) + os.remove(keystore_p12_path) def update_jks_perm(module, keystore_path): diff --git a/tests/unit/plugins/modules/system/test_java_keystore.py b/tests/unit/plugins/modules/system/test_java_keystore.py index c2f3421c72..68863c149e 100644 --- a/tests/unit/plugins/modules/system/test_java_keystore.py +++ b/tests/unit/plugins/modules/system/test_java_keystore.py @@ -26,8 +26,8 @@ class TestCreateJavaKeystore(ModuleTestCase): orig_exists = os.path.exists self.spec = ArgumentSpec() - self.mock_create_file = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.create_file', - side_effect=lambda path, content: path) + self.mock_create_file = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.create_file') + self.mock_create_path = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.create_path') self.mock_run_commands = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.run_commands') self.mock_os_path_exists = patch('os.path.exists', side_effect=lambda path: True if path == '/path/to/keystore.jks' else orig_exists(path)) @@ -37,6 +37,7 @@ class TestCreateJavaKeystore(ModuleTestCase): side_effect=lambda path: (False, None)) self.run_commands = self.mock_run_commands.start() self.create_file = self.mock_create_file.start() + self.create_path = self.mock_create_path.start() self.selinux_context = self.mock_selinux_context.start() self.is_special_selinux_path = self.mock_is_special_selinux_path.start() self.os_path_exists = self.mock_os_path_exists.start() @@ -45,6 +46,7 @@ class TestCreateJavaKeystore(ModuleTestCase): """Teardown.""" super(TestCreateJavaKeystore, self).tearDown() self.mock_create_file.stop() + self.mock_create_path.stop() self.mock_run_commands.stop() self.mock_selinux_context.stop() self.mock_is_special_selinux_path.stop() @@ -67,13 +69,15 @@ class TestCreateJavaKeystore(ModuleTestCase): module.exit_json = Mock() with patch('os.remove', return_value=True): + self.create_path.side_effect = ['/tmp/tmpgrzm2ah7'] + self.create_file.side_effect = ['/tmp/etacifitrec', '/tmp/yek_etavirp'] self.run_commands.side_effect = lambda module, cmd, data: (0, '', '') create_jks(module, "test", "openssl", "keytool", "/path/to/keystore.jks", "changeit", "") module.exit_json.assert_called_once_with( changed=True, cmd=["keytool", "-importkeystore", "-destkeystore", "/path/to/keystore.jks", - "-srckeystore", "/tmp/keystore.p12", "-srcstoretype", "pkcs12", "-alias", "test", + "-srckeystore", "/tmp/tmpgrzm2ah7", "-srcstoretype", "pkcs12", "-alias", "test", "-deststorepass", "changeit", "-srcstorepass", "changeit", "-noprompt"], msg='', rc=0, @@ -98,12 +102,15 @@ class TestCreateJavaKeystore(ModuleTestCase): module.fail_json = Mock() with patch('os.remove', return_value=True): + self.create_path.side_effect = ['/tmp/tmp1cyp12xa'] + self.create_file.side_effect = ['/tmp/tmpvalcrt32', '/tmp/tmpwh4key0c'] self.run_commands.side_effect = [(1, '', ''), (0, '', '')] create_jks(module, "test", "openssl", "keytool", "/path/to/keystore.jks", "changeit", "passphrase-foo") module.fail_json.assert_called_once_with( cmd=["openssl", "pkcs12", "-export", "-name", "test", - "-in", "/tmp/foo.crt", "-inkey", "/tmp/foo.key", - "-out", "/tmp/keystore.p12", + "-in", "/tmp/tmpvalcrt32", + "-inkey", "/tmp/tmpwh4key0c", + "-out", "/tmp/tmp1cyp12xa", "-passout", "stdin", "-passin", "stdin"], msg='', @@ -127,12 +134,15 @@ class TestCreateJavaKeystore(ModuleTestCase): module.fail_json = Mock() with patch('os.remove', return_value=True): + self.create_path.side_effect = ['/tmp/tmp1cyp12xa'] + self.create_file.side_effect = ['/tmp/tmpvalcrt32', '/tmp/tmpwh4key0c'] self.run_commands.side_effect = [(1, '', ''), (0, '', '')] create_jks(module, "test", "openssl", "keytool", "/path/to/keystore.jks", "changeit", "") module.fail_json.assert_called_once_with( cmd=["openssl", "pkcs12", "-export", "-name", "test", - "-in", "/tmp/foo.crt", "-inkey", "/tmp/foo.key", - "-out", "/tmp/keystore.p12", + "-in", "/tmp/tmpvalcrt32", + "-inkey", "/tmp/tmpwh4key0c", + "-out", "/tmp/tmp1cyp12xa", "-passout", "stdin"], msg='', rc=1 @@ -155,12 +165,14 @@ class TestCreateJavaKeystore(ModuleTestCase): module.fail_json = Mock() with patch('os.remove', return_value=True): + self.create_path.side_effect = ['/tmp/tmpgrzm2ah7'] + self.create_file.side_effect = ['/tmp/etacifitrec', '/tmp/yek_etavirp'] self.run_commands.side_effect = [(0, '', ''), (1, '', '')] create_jks(module, "test", "openssl", "keytool", "/path/to/keystore.jks", "changeit", "") module.fail_json.assert_called_once_with( cmd=["keytool", "-importkeystore", "-destkeystore", "/path/to/keystore.jks", - "-srckeystore", "/tmp/keystore.p12", "-srcstoretype", "pkcs12", "-alias", "test", + "-srckeystore", "/tmp/tmpgrzm2ah7", "-srcstoretype", "pkcs12", "-alias", "test", "-deststorepass", "changeit", "-srcstorepass", "changeit", "-noprompt"], msg='', rc=1 @@ -174,8 +186,7 @@ class TestCertChanged(ModuleTestCase): """Setup.""" super(TestCertChanged, self).setUp() self.spec = ArgumentSpec() - self.mock_create_file = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.create_file', - side_effect=lambda path, content: path) + self.mock_create_file = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.create_file') self.mock_run_commands = patch('ansible_collections.community.general.plugins.modules.system.java_keystore.run_commands') self.run_commands = self.mock_run_commands.start() self.create_file = self.mock_create_file.start() @@ -201,6 +212,7 @@ class TestCertChanged(ModuleTestCase): ) with patch('os.remove', return_value=True): + self.create_file.side_effect = ['/tmp/placeholder'] self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''), (0, 'SHA256: abcd:1234:efgh', '')] result = cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo') self.assertFalse(result, 'Fingerprint is identical') @@ -220,6 +232,7 @@ class TestCertChanged(ModuleTestCase): ) with patch('os.remove', return_value=True): + self.create_file.side_effect = ['/tmp/placeholder'] self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''), (0, 'SHA256: wxyz:9876:stuv', '')] result = cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo') self.assertTrue(result, 'Fingerprint mismatch') @@ -239,6 +252,7 @@ class TestCertChanged(ModuleTestCase): ) with patch('os.remove', return_value=True): + self.create_file.side_effect = ['/tmp/placeholder'] self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''), (1, 'keytool error: java.lang.Exception: Alias does not exist', '')] result = cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo') @@ -261,10 +275,11 @@ class TestCertChanged(ModuleTestCase): module.fail_json = Mock() with patch('os.remove', return_value=True): + self.create_file.side_effect = ['/tmp/tmpdj6bvvme'] self.run_commands.side_effect = [(1, '', 'Oops'), (0, 'SHA256: wxyz:9876:stuv', '')] cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo') module.fail_json.assert_called_once_with( - cmd=["openssl", "x509", "-noout", "-in", "/tmp/foo.crt", "-fingerprint", "-sha256"], + cmd=["openssl", "x509", "-noout", "-in", "/tmp/tmpdj6bvvme", "-fingerprint", "-sha256"], msg='', err='Oops', rc=1 @@ -287,6 +302,7 @@ class TestCertChanged(ModuleTestCase): module.fail_json = Mock(return_value=True) with patch('os.remove', return_value=True): + self.create_file.side_effect = ['/tmp/placeholder'] self.run_commands.side_effect = [(0, 'foo: wxyz:9876:stuv', ''), (1, '', 'Oops')] cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo') module.fail_json.assert_called_with(