From 7866b0487317a6b3a26fe811a47f8f753bdac508 Mon Sep 17 00:00:00 2001 From: Aldo Troiano <98324414+atroiano-glue@users.noreply.github.com> Date: Fri, 22 Nov 2024 18:02:01 +0000 Subject: [PATCH] Fix: gnupg on encryption error (#117) * catching gnupg error on file encryption. * added tests for parallel batch enc & dec * Update changelog * bump version v24.47.0 -> v24.47.1 * Ensure dummy key files exist so as not to break all tests * Fix dummy file creation * Alter PGP key file creation --------- Co-authored-by: Adam McDonagh <27215476+adammcdonagh@users.noreply.github.com> --- CHANGELOG.md | 4 + pyproject.toml | 4 +- src/opentaskpy/taskhandlers/transfer.py | 7 +- .../cfg/transfers/decrypt-file-1-sftp.json.j2 | 25 +++++ .../cfg/transfers/decrypt-file-2-sftp.json.j2 | 25 +++++ .../cfg/transfers/encrypt-file-1-sftp.json.j2 | 20 ++++ .../cfg/transfers/encrypt-file-2-sftp.json.j2 | 20 ++++ test/cfg/variables.json.j2 | 6 +- test/createTestFiles.sh | 4 + tests/fixtures/pgp.py | 31 ++++++ tests/fixtures/ssh_clients.py | 10 +- tests/test_config_loader.py | 4 + tests/test_docker_task_run.py | 16 +++ tests/test_taskhandler_batch_encryption.py | 100 ++++++++++++++++++ 14 files changed, 271 insertions(+), 5 deletions(-) create mode 100644 test/cfg/transfers/decrypt-file-1-sftp.json.j2 create mode 100644 test/cfg/transfers/decrypt-file-2-sftp.json.j2 create mode 100644 test/cfg/transfers/encrypt-file-1-sftp.json.j2 create mode 100644 test/cfg/transfers/encrypt-file-2-sftp.json.j2 create mode 100644 tests/test_taskhandler_batch_encryption.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 40ab48c..d72c6c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +# v24.47.1 + +- Add missing try/catch to decryption code when trying to delete gnupg directory + # v24.47.0 - Refactored dependency checker method to support Running/Failed state diff --git a/pyproject.toml b/pyproject.toml index 8a84b5f..03a7210 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "opentaskpy" -version = "v24.47.0" +version = "v24.47.1" authors = [{ name = "Adam McDonagh", email = "adam@elitemonkey.net" }] license = { text = "GPLv3" } classifiers = [ @@ -71,7 +71,7 @@ otf-batch-validator = "opentaskpy.cli.batch_validator:main" profile = 'black' [tool.bumpver] -current_version = "v24.47.0" +current_version = "v24.47.1" version_pattern = "vYY.WW.PATCH[-TAG]" commit_message = "bump version {old_version} -> {new_version}" commit = true diff --git a/src/opentaskpy/taskhandlers/transfer.py b/src/opentaskpy/taskhandlers/transfer.py index d7b9b35..fa18872 100644 --- a/src/opentaskpy/taskhandlers/transfer.py +++ b/src/opentaskpy/taskhandlers/transfer.py @@ -772,7 +772,12 @@ def encrypt_files( # Remove the temporary gnupg keychain files under f"{tmpdir}/.gnupg" if path.exists(f"{tmpdir}/.gnupg"): - shutil.rmtree(f"{tmpdir}/.gnupg") + try: + shutil.rmtree(f"{tmpdir}/.gnupg") + except FileNotFoundError as e: + self.logger.warning( + f".gnupg deletion failed - FileNotFound but continuing: {e}" + ) return encrypted_files diff --git a/test/cfg/transfers/decrypt-file-1-sftp.json.j2 b/test/cfg/transfers/decrypt-file-1-sftp.json.j2 new file mode 100644 index 0000000..7f879f2 --- /dev/null +++ b/test/cfg/transfers/decrypt-file-1-sftp.json.j2 @@ -0,0 +1,25 @@ +{ + "type": "transfer", + "source": { + "hostname": "172.16.0.22", + "directory": "/home/application/testFiles/dest", + "fileRegex": "file-encrypt1\\.txt.gpg", + "protocol": { "name": "sftp", "credentials": { "username": "application" } } + }, + "destination": [ + { + "hostname": "172.16.0.21", + "directory": "/home/application/testFiles/dest", + "protocol": { + "name": "sftp", + "credentials": { "username": "application" }, + "supportsPosixRename": false + }, + "encryption": { "decrypt": true, "private_key": "{{ PRIVATE_KEY1 }}" }, + "rename": { + "pattern": "^file-encrypt1\\.txt.gpg$", + "sub": "file-encrypt1-decrypted.txt" + } + } + ] +} diff --git a/test/cfg/transfers/decrypt-file-2-sftp.json.j2 b/test/cfg/transfers/decrypt-file-2-sftp.json.j2 new file mode 100644 index 0000000..dae75b3 --- /dev/null +++ b/test/cfg/transfers/decrypt-file-2-sftp.json.j2 @@ -0,0 +1,25 @@ +{ + "type": "transfer", + "source": { + "hostname": "172.16.0.22", + "directory": "/home/application/testFiles/dest", + "fileRegex": "file-encrypt2\\.txt.gpg", + "protocol": { "name": "sftp", "credentials": { "username": "application" } } + }, + "destination": [ + { + "hostname": "172.16.0.21", + "directory": "/home/application/testFiles/dest", + "protocol": { + "name": "sftp", + "credentials": { "username": "application" }, + "supportsPosixRename": false + }, + "encryption": { "decrypt": true, "private_key": "{{ PRIVATE_KEY2 }}" }, + "rename": { + "pattern": "^file-encrypt2\\.txt.gpg$", + "sub": "file-encrypt2-decrypted.txt" + } + } + ] +} diff --git a/test/cfg/transfers/encrypt-file-1-sftp.json.j2 b/test/cfg/transfers/encrypt-file-1-sftp.json.j2 new file mode 100644 index 0000000..ad1ff0d --- /dev/null +++ b/test/cfg/transfers/encrypt-file-1-sftp.json.j2 @@ -0,0 +1,20 @@ +{ + "type": "transfer", + "source": { + "hostname": "172.16.0.21", + "directory": "/home/application/testFiles/src", + "fileRegex": "file-encrypt1\\.txt", + "protocol": { "name": "sftp", "credentials": { "username": "application" } } + }, + "destination": [ + { + "hostname": "172.16.0.22", + "directory": "/home/application/testFiles/dest", + "protocol": { + "name": "sftp", + "credentials": { "username": "application" } + }, + "encryption": { "encrypt": true, "public_key": "{{ PUBLIC_KEY1 }}" } + } + ] +} diff --git a/test/cfg/transfers/encrypt-file-2-sftp.json.j2 b/test/cfg/transfers/encrypt-file-2-sftp.json.j2 new file mode 100644 index 0000000..2c72dad --- /dev/null +++ b/test/cfg/transfers/encrypt-file-2-sftp.json.j2 @@ -0,0 +1,20 @@ +{ + "type": "transfer", + "source": { + "hostname": "172.16.0.21", + "directory": "/home/application/testFiles/src", + "fileRegex": "file-encrypt2\\.txt", + "protocol": { "name": "sftp", "credentials": { "username": "application" } } + }, + "destination": [ + { + "hostname": "172.16.0.22", + "directory": "/home/application/testFiles/dest", + "protocol": { + "name": "sftp", + "credentials": { "username": "application" } + }, + "encryption": { "encrypt": true, "public_key": "{{ PUBLIC_KEY2 }}" } + } + ] +} diff --git a/test/cfg/variables.json.j2 b/test/cfg/variables.json.j2 index 039e517..85ebcad 100644 --- a/test/cfg/variables.json.j2 +++ b/test/cfg/variables.json.j2 @@ -26,5 +26,9 @@ "testLookup": "{{ lookup('file', path='/tmp/variable_lookup.txt') }}", "testLookup2": "{{ lookup('http_json', url='https://jsonplaceholder.typicode.com/posts/1', jsonpath='$.title') }}", "global_protocol_vars": {"name": "email", "smtp_port": 587, "smtp_server": "smtp.gmail.com"}, - "SSH_VARS": {"name": "ssh", "credentials": {"username": "someuser"}} + "SSH_VARS": {"name": "ssh", "credentials": {"username": "someuser"}}, + "PUBLIC_KEY1": "{{ lookup('file', path='/tmp/public_key_1.txt') }}", + "PUBLIC_KEY2": "{{ lookup('file', path='/tmp/public_key_2.txt') }}", + "PRIVATE_KEY1": "{{ lookup('file', path='/tmp/private_key_1.txt') }}", + "PRIVATE_KEY2": "{{ lookup('file', path='/tmp/private_key_2.txt') }}" } diff --git a/test/createTestFiles.sh b/test/createTestFiles.sh index 470b0ce..7e140d8 100755 --- a/test/createTestFiles.sh +++ b/test/createTestFiles.sh @@ -2,6 +2,10 @@ # Create dummy variable for lookup command echo "file_variable" > /tmp/variable_lookup.txt +echo "" > /tmp/private_key_1.txt +echo "" > /tmp/private_key_2.txt +echo "" > /tmp/public_key_1.txt +echo "" > /tmp/public_key_2.txt # Get current script directory DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" diff --git a/tests/fixtures/pgp.py b/tests/fixtures/pgp.py index e83d8a2..819d075 100644 --- a/tests/fixtures/pgp.py +++ b/tests/fixtures/pgp.py @@ -1,5 +1,8 @@ # pylint: skip-file +import os + import pytest +from pytest_shell import fs @pytest.fixture(scope="session") @@ -262,3 +265,31 @@ def private_key_2() -> str: 5FJn9I4DYA== =cmHi -----END PGP PRIVATE KEY BLOCK-----""" + + +@pytest.fixture(scope="function") +def store_pgp_keys(public_key, public_key_2, private_key, private_key_2) -> bool: + pub1 = "\\\\n".join(public_key.splitlines()) + pub2 = "\\\\n".join(public_key_2.splitlines()) + prv1 = "\\\\n".join(private_key.splitlines()) + prv2 = "\\\\n".join(private_key_2.splitlines()) + + # Remove all the files and the recreate them + if os.path.exists("/tmp/public_key_1.txt"): + os.remove("/tmp/public_key_1.txt") + if os.path.exists("/tmp/public_key_2.txt"): + os.remove("/tmp/public_key_2.txt") + if os.path.exists("/tmp/private_key_1.txt"): + os.remove("/tmp/private_key_1.txt") + if os.path.exists("/tmp/private_key_2.txt"): + os.remove("/tmp/private_key_2.txt") + + fs.create_files( + [ + {"/tmp/public_key_1.txt": {"content": pub1}}, + {"/tmp/public_key_2.txt": {"content": pub2}}, + {"/tmp/private_key_1.txt": {"content": prv1}}, + {"/tmp/private_key_2.txt": {"content": prv2}}, + ] + ) + return True diff --git a/tests/fixtures/ssh_clients.py b/tests/fixtures/ssh_clients.py index eed7523..4815701 100644 --- a/tests/fixtures/ssh_clients.py +++ b/tests/fixtures/ssh_clients.py @@ -22,7 +22,15 @@ def env_vars() -> None: del os.environ["OTF_NO_LOG"] # We're using the proper config file for this, so we need to make sure something exist in /tmp/variable_lookup.txt - fs.create_files([{"/tmp/variable_lookup.txt": {"content": "test1234"}}]) + fs.create_files( + [ + {"/tmp/variable_lookup.txt": {"content": "test1234"}}, + {"/tmp/public_key_1.txt": {"content": "test1234"}}, + {"/tmp/public_key_2.txt": {"content": "test1234"}}, + {"/tmp/private_key_1.txt": {"content": "test1234"}}, + {"/tmp/private_key_2.txt": {"content": "test1234"}}, + ] + ) @pytest.fixture(scope="session") diff --git a/tests/test_config_loader.py b/tests/test_config_loader.py index 8c34d80..087ca33 100644 --- a/tests/test_config_loader.py +++ b/tests/test_config_loader.py @@ -47,6 +47,10 @@ def test_load_variables(tmpdir): "content": "hello", } }, + {"/tmp/public_key_1.txt": {"content": "test1234"}}, + {"/tmp/public_key_2.txt": {"content": "test1234"}}, + {"/tmp/private_key_1.txt": {"content": "test1234"}}, + {"/tmp/private_key_2.txt": {"content": "test1234"}}, ] ) diff --git a/tests/test_docker_task_run.py b/tests/test_docker_task_run.py index 316de1e..c16764e 100644 --- a/tests/test_docker_task_run.py +++ b/tests/test_docker_task_run.py @@ -203,6 +203,14 @@ def test_docker_run( f"{log_dir}:/logs", "--volume", "/tmp/variable_lookup.txt:/tmp/variable_lookup.txt", + "--volume", + "/tmp/public_key_1.txt:/tmp/public_key_1.txt", + "--volume", + "/tmp/public_key_2.txt:/tmp/public_key_2.txt", + "--volume", + "/tmp/private_key_1.txt:/tmp/private_key_1.txt", + "--volume", + "/tmp/private_key_2.txt:/tmp/private_key_2.txt", "-e", "OTF_SSH_KEY=/test/testFiles/id_rsa", image_name_dev, @@ -258,6 +266,14 @@ def test_standard_docker_image( "--volume", "/tmp/variable_lookup.txt:/tmp/variable_lookup.txt", "--volume", + "/tmp/public_key_1.txt:/tmp/public_key_1.txt", + "--volume", + "/tmp/public_key_2.txt:/tmp/public_key_2.txt", + "--volume", + "/tmp/private_key_1.txt:/tmp/private_key_1.txt", + "--volume", + "/tmp/private_key_2.txt:/tmp/private_key_2.txt", + "--volume", f"{root_dir}/testLogs:/logs", "--env", "OTF_RUN_ID=docker_log_test", diff --git a/tests/test_taskhandler_batch_encryption.py b/tests/test_taskhandler_batch_encryption.py new file mode 100644 index 0000000..dc659d3 --- /dev/null +++ b/tests/test_taskhandler_batch_encryption.py @@ -0,0 +1,100 @@ +# pylint: skip-file +# ruff: noqa +import os + +from pytest_shell import fs + +import opentaskpy.otflogging +from opentaskpy.config.loader import ConfigLoader +from opentaskpy.taskhandlers import batch, execution, transfer +from tests.file_helper import * # noqa: F403 +from tests.fixtures.pgp import * # noqa: F403, F401 +from tests.fixtures.ssh_clients import * # noqa: F403 + +os.environ["OTF_NO_LOG"] = "1" +os.environ["OTF_LOG_LEVEL"] = "DEBUG" + +parallel_encryption_batch = { + "type": "batch", + "tasks": [ + {"order_id": 1, "task_id": "encrypt-file-1-sftp"}, + {"order_id": 2, "task_id": "encrypt-file-2-sftp"}, + ], +} + +parallel_decryption_batch = { + "type": "batch", + "tasks": [ + {"order_id": 1, "task_id": "decrypt-file-1-sftp"}, + {"order_id": 2, "task_id": "decrypt-file-2-sftp"}, + ], +} + + +def test_batch_parallel_encryption( + root_dir, env_vars, clear_logs, store_pgp_keys, setup_sftp_keys +): + # Assert one of the keys randomly + assert os.path.exists("/tmp/public_key_1.txt") + + config_loader = ConfigLoader("test/cfg") + + fs.create_files( + [ + { + f"{root_dir}/testFiles/sftp_1/src/file-encrypt1.txt": { + "content": "hellothere!" + } + }, + { + f"{root_dir}/testFiles/sftp_1/src/file-encrypt2.txt": { + "content": "hellothere!" + } + }, + ] + ) + assert os.path.exists(f"{root_dir}/testFiles/sftp_1/src/file-encrypt1.txt") + assert os.path.exists(f"{root_dir}/testFiles/sftp_1/src/file-encrypt2.txt") + + batch_obj = batch.Batch( + None, + f"parallel-encryption-batch", + parallel_encryption_batch, + config_loader, + ) + + # Run the batch and expect a true status + assert batch_obj.run() + + assert os.path.exists(f"{root_dir}/testFiles/sftp_2/dest/file-encrypt1.txt.gpg") + assert os.path.exists(f"{root_dir}/testFiles/sftp_2/dest/file-encrypt2.txt.gpg") + + +def test_batch_parallel_decryption( + root_dir, env_vars, clear_logs, store_pgp_keys, setup_sftp_keys +): + # Assert one of the keys randomly + assert os.path.exists("/tmp/private_key_1.txt") + + config_loader = ConfigLoader("test/cfg") + + assert os.path.exists(f"{root_dir}/testFiles/sftp_2/dest/file-encrypt1.txt.gpg") + assert os.path.exists(f"{root_dir}/testFiles/sftp_2/dest/file-encrypt2.txt.gpg") + + batch_obj = batch.Batch( + None, + f"parallel-decryption-batch", + parallel_decryption_batch, + config_loader, + ) + + # Run the batch and expect a true status + assert batch_obj.run() + + # Assert files have been decrypted and transferred back to sftp1 + assert os.path.exists( + f"{root_dir}/testFiles/sftp_1/dest/file-encrypt1-decrypted.txt" + ) + assert os.path.exists( + f"{root_dir}/testFiles/sftp_1/dest/file-encrypt2-decrypted.txt" + )