From f7e632025f643d2073f2e0280da6c6d908caf152 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 09:27:19 -0400 Subject: [PATCH 01/11] Island: Reformat IEncryptor docstrings --- .../cc/server_utils/encryption/i_encryptor.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py index d83198b7b..d46e51c1a 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py @@ -5,16 +5,18 @@ from typing import Any class IEncryptor(ABC): @abstractmethod def encrypt(self, plaintext: Any) -> Any: - """Encrypts data and returns the ciphertext. + """ + Encrypts data and returns the ciphertext. + :param plaintext: Data that will be encrypted - :return: Ciphertext generated by encrypting value - :rtype: Any + :return: Ciphertext generated by encrypting the plaintext """ @abstractmethod def decrypt(self, ciphertext: Any): - """Decrypts data and returns the plaintext. - :param ciphertext: Ciphertext that will be decrypted - :return: Plaintext generated by decrypting value - :rtype: Any + """ + Decrypts data and returns the plaintext. + + :param ciphertext: Ciphertext that will be decrypted + :return: Plaintext generated by decrypting the ciphertext """ From 54c1eef3099d71917a2e4ece8d51b52d8e5b66fc Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 09:39:05 -0400 Subject: [PATCH 02/11] Island: Make IEncryptor interface consistent An IEncryptor could take Any type and encrypt it, returning Any other type. This is a poorly defined interface and, IIRC, it was defined this way so some encryptors could take strings and others could take bytes. This commit modifies the interface so that it accepts and returns bytes in all cases. If a string encryptor is needed for convenience, we can add IStringEncryptor and write a decorator that wraps IEncryptors. --- .../server_utils/encryption/data_store_encryptor.py | 4 ++-- .../encryption/field_encryptors/string_encryptor.py | 4 ++-- .../field_encryptors/string_list_encryptor.py | 6 +++--- .../cc/server_utils/encryption/i_encryptor.py | 5 ++--- .../server_utils/encryption/key_based_encryptor.py | 10 +++++----- .../encryption/test_data_store_encryptor.py | 2 +- .../encryption/test_key_based_encryptor.py | 12 ++++++------ 7 files changed, 21 insertions(+), 22 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py index 0af258d19..6b927e20b 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py @@ -46,10 +46,10 @@ class DataStoreEncryptor(IEncryptor): return KeyBasedEncryptor(plaintext_key) - def encrypt(self, plaintext: str) -> str: + def encrypt(self, plaintext: bytes) -> bytes: return self._key_based_encryptor.encrypt(plaintext) - def decrypt(self, ciphertext: str): + def decrypt(self, ciphertext: bytes) -> bytes: return self._key_based_encryptor.decrypt(ciphertext) diff --git a/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_encryptor.py index 28f0f2c93..6de6cb385 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_encryptor.py @@ -5,8 +5,8 @@ from . import IFieldEncryptor class StringEncryptor(IFieldEncryptor): @staticmethod def encrypt(value: str): - return get_datastore_encryptor().encrypt(value) + return get_datastore_encryptor().encrypt(value.encode()) @staticmethod def decrypt(value: str): - return get_datastore_encryptor().decrypt(value) + return get_datastore_encryptor().decrypt(value).decode() diff --git a/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_list_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_list_encryptor.py index ce0ceb8dd..9adf733a4 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_list_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/field_encryptors/string_list_encryptor.py @@ -7,8 +7,8 @@ from . import IFieldEncryptor class StringListEncryptor(IFieldEncryptor): @staticmethod def encrypt(value: List[str]): - return [get_datastore_encryptor().encrypt(string) for string in value] + return [get_datastore_encryptor().encrypt(string.encode()) for string in value] @staticmethod - def decrypt(value: List[str]): - return [get_datastore_encryptor().decrypt(string) for string in value] + def decrypt(value: List[bytes]): + return [get_datastore_encryptor().decrypt(bytes_).decode() for bytes_ in value] diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py index d46e51c1a..8bc701bf9 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_encryptor.py @@ -1,10 +1,9 @@ from abc import ABC, abstractmethod -from typing import Any class IEncryptor(ABC): @abstractmethod - def encrypt(self, plaintext: Any) -> Any: + def encrypt(self, plaintext: bytes) -> bytes: """ Encrypts data and returns the ciphertext. @@ -13,7 +12,7 @@ class IEncryptor(ABC): """ @abstractmethod - def decrypt(self, ciphertext: Any): + def decrypt(self, ciphertext: bytes) -> bytes: """ Decrypts data and returns the plaintext. diff --git a/monkey/monkey_island/cc/server_utils/encryption/key_based_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/key_based_encryptor.py index 630094989..67e59f94d 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/key_based_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/key_based_encryptor.py @@ -31,15 +31,15 @@ class KeyBasedEncryptor(IEncryptor): # something up. The main drawback to fernet is that it uses AES-128, which is not # quantum-safe. At the present time, human error is probably a greater risk than quantum # computers. - def encrypt(self, plaintext: str) -> str: + def encrypt(self, plaintext: bytes) -> bytes: cipher_iv = Random.new().read(AES.block_size) cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) - padded_plaintext = Padding.pad(plaintext.encode(), self._BLOCK_SIZE) - return base64.b64encode(cipher_iv + cipher.encrypt(padded_plaintext)).decode() + padded_plaintext = Padding.pad(plaintext, self._BLOCK_SIZE) + return base64.b64encode(cipher_iv + cipher.encrypt(padded_plaintext)) - def decrypt(self, ciphertext: str): + def decrypt(self, ciphertext: bytes) -> bytes: enc_message = base64.b64decode(ciphertext) cipher_iv = enc_message[0 : AES.block_size] cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) padded_plaintext = cipher.decrypt(enc_message[AES.block_size :]) - return Padding.unpad(padded_plaintext, self._BLOCK_SIZE).decode() + return Padding.unpad(padded_plaintext, self._BLOCK_SIZE) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py index da4a9ec09..1887185d4 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py @@ -11,7 +11,7 @@ from monkey_island.cc.server_utils.encryption import ( # Mark all tests in this module as slow pytestmark = pytest.mark.slow -PLAINTEXT = "Hello, Monkey!" +PLAINTEXT = b"Hello, Monkey!" MOCK_SECRET = "53CR31" diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py index d41f866a7..d17a5cd1a 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py @@ -14,19 +14,19 @@ kb_encryptor = KeyBasedEncryptor(KEY) def test_encrypt_decrypt_string_with_key(): - encrypted = kb_encryptor.encrypt(PLAINTEXT) - decrypted = kb_encryptor.decrypt(encrypted) + encrypted = kb_encryptor.encrypt(PLAINTEXT.encode()) + decrypted = kb_encryptor.decrypt(encrypted).decode() assert decrypted == PLAINTEXT @pytest.mark.parametrize("plaintext", [PLAINTEXT_UTF8_1, PLAINTEXT_UTF8_2, PLAINTEXT_UTF8_3]) def test_encrypt_decrypt_string_utf8_with_key(plaintext): - encrypted = kb_encryptor.encrypt(plaintext) - decrypted = kb_encryptor.decrypt(encrypted) + encrypted = kb_encryptor.encrypt(plaintext.encode()) + decrypted = kb_encryptor.decrypt(encrypted).decode() assert decrypted == plaintext def test_encrypt_decrypt_string_multiple_block_size_with_key(): - encrypted = kb_encryptor.encrypt(PLAINTEXT_MULTIPLE_BLOCK_SIZE) - decrypted = kb_encryptor.decrypt(encrypted) + encrypted = kb_encryptor.encrypt(PLAINTEXT_MULTIPLE_BLOCK_SIZE.encode()) + decrypted = kb_encryptor.decrypt(encrypted).decode() assert decrypted == PLAINTEXT_MULTIPLE_BLOCK_SIZE From bd2d79fd43eedcb4717f90e56a7d8f6f07814cab Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 10:02:06 -0400 Subject: [PATCH 03/11] Island: Add ILockableEncryptor --- .../cc/server_utils/encryption/__init__.py | 1 + .../encryption/i_lockable_encryptor.py | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py diff --git a/monkey/monkey_island/cc/server_utils/encryption/__init__.py b/monkey/monkey_island/cc/server_utils/encryption/__init__.py index a6671afb6..c3ed8c46d 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/__init__.py +++ b/monkey/monkey_island/cc/server_utils/encryption/__init__.py @@ -7,6 +7,7 @@ from .password_based_bytes_encryptor import ( InvalidCredentialsError, InvalidCiphertextError, ) +from .i_lockable_encryptor import ILockableEncryptor from .data_store_encryptor import ( get_datastore_encryptor, unlock_datastore_encryptor, diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py new file mode 100644 index 000000000..b1b52f1d8 --- /dev/null +++ b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py @@ -0,0 +1,64 @@ +from abc import abstractmethod + +from . import IEncryptor + +# NOTE: The ILockableEncryptor introduces temporal coupling, that is, you must first unlock the +# encryptor before you can use it. This is because the key material used to encrypt repository +# contents is encrypted using the user's username and password. This adds extra security by +# allowing us to fully encrypt data at rest. Without this, we'd need to store the repository +# key in plaintext. Alternative solutions are as follows: +# 1. Store the repository key in plaintext +# 2. Add an initialization phase to the island's boot sequence. At the moment, this is the +# only element of the design that would benefit from adding an additional phase. If +# other temporal coupling begins to creep in, we can add the initialization phase at +# that time and remove this interface. + + +class LockedKeyError(Exception): + """ + Raised when an ILockableEncryptor attemps to encrypt or decrypt data before the + ILockableEncryptor has been unlocked. + """ + + +class ILockableEncryptor(IEncryptor): + """ + An encryptor that can be locked or unlocked. + + ILockableEncryptor's require a secret in order to access their key material. These encryptors + must be unlocked before use and can be re-locked at the user's request. + """ + + @abstractmethod + def unlock(self, secret: bytes): + """ + Unlock the encryptor + + :param secret: A secret that must be used to access the ILockableEncryptor's key material. + """ + + @abstractmethod + def lock(self): + """ + Lock the encryptor, making it unusable + """ + + @abstractmethod + def encrypt(self, plaintext: bytes) -> bytes: + """ + Encrypts data and returns the ciphertext. + + :param plaintext: Data that will be encrypted + :return: Ciphertext generated by encrypting the plaintext + :raises LockedKeyError: If encrypt() is called while the ILockableEncryptor is locked + """ + + @abstractmethod + def decrypt(self, ciphertext: bytes) -> bytes: + """ + Decrypts data and returns the plaintext. + + :param ciphertext: Ciphertext that will be decrypted + :return: Plaintext generated by decrypting the ciphertext + :raises LockedKeyError: If decrypt() is called while the ILockableEncryptor is locked + """ From c7257cf00068124166c31e50749db98e22f097f9 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 10:51:37 -0400 Subject: [PATCH 04/11] Island: Add RepositoryEncryptor --- .../cc/server_utils/encryption/__init__.py | 3 +- .../encryption/repository_encryptor.py | 59 ++++++++++++++++ .../encryption/test_repository_encryptor.py | 70 +++++++++++++++++++ 3 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py create mode 100644 monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py diff --git a/monkey/monkey_island/cc/server_utils/encryption/__init__.py b/monkey/monkey_island/cc/server_utils/encryption/__init__.py index c3ed8c46d..d90468cc4 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/__init__.py +++ b/monkey/monkey_island/cc/server_utils/encryption/__init__.py @@ -7,7 +7,8 @@ from .password_based_bytes_encryptor import ( InvalidCredentialsError, InvalidCiphertextError, ) -from .i_lockable_encryptor import ILockableEncryptor +from .i_lockable_encryptor import ILockableEncryptor, LockedKeyError +from .repository_encryptor import RepositoryEncryptor from .data_store_encryptor import ( get_datastore_encryptor, unlock_datastore_encryptor, diff --git a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py new file mode 100644 index 000000000..0e0310147 --- /dev/null +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -0,0 +1,59 @@ +import os +import secrets +from pathlib import Path + +from monkey_island.cc.server_utils.file_utils import open_new_securely_permissioned_file + +from . import ILockableEncryptor, LockedKeyError +from .key_based_encryptor import KeyBasedEncryptor +from .password_based_bytes_encryptor import PasswordBasedBytesEncryptor + + +class RepositoryEncryptor(ILockableEncryptor): + _KEY_LENGTH_BYTES = 32 + + def __init__(self, key_file: Path): + self._key_file = key_file + self._password_based_encryptor = None + self._key_based_encryptor = None + + def unlock(self, secret: bytes): + self._password_based_encryptor = PasswordBasedBytesEncryptor(secret.decode()) + self._key_based_encryptor = self._initialize_key_based_encryptor() + + def _initialize_key_based_encryptor(self): + if os.path.exists(self._key_file): + return self._load_key() + + return self._create_key() + + def _load_key(self) -> KeyBasedEncryptor: + with open(self._key_file, "rb") as f: + encrypted_key = f.read() + + plaintext_key = self._password_based_encryptor.decrypt(encrypted_key) + return KeyBasedEncryptor(plaintext_key) + + def _create_key(self) -> KeyBasedEncryptor: + plaintext_key = secrets.token_bytes(RepositoryEncryptor._KEY_LENGTH_BYTES) + + encrypted_key = self._password_based_encryptor.encrypt(plaintext_key) + with open_new_securely_permissioned_file(str(self._key_file), "wb") as f: + f.write(encrypted_key) + + return KeyBasedEncryptor(plaintext_key) + + def lock(self): + self._key_based_encryptor = None + + def encrypt(self, plaintext: bytes) -> bytes: + if self._key_based_encryptor is None: + raise LockedKeyError("Cannot encrypt while the encryptor is locked)") + + return self._key_based_encryptor.encrypt(plaintext) + + def decrypt(self, ciphertext: bytes) -> bytes: + if self._key_based_encryptor is None: + raise LockedKeyError("Cannot decrypt while the encryptor is locked)") + + return self._key_based_encryptor.decrypt(ciphertext) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py new file mode 100644 index 000000000..74b02fb9e --- /dev/null +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -0,0 +1,70 @@ +import random +import string + +import pytest + +from common.utils.file_utils import get_file_sha256_hash +from monkey_island.cc.server_utils.encryption import LockedKeyError, RepositoryEncryptor + +PLAINTEXT = b"Hello, Monkey!" +SECRET = b"53CR31" + + +@pytest.fixture +def key_file(tmp_path): + return tmp_path / "test_key.bin" + + +@pytest.fixture +def encryptor(key_file): + return RepositoryEncryptor(key_file) + + +def test_encryption(encryptor): + plaintext = "".join( + random.choice(string.ascii_uppercase + string.digits) for _ in range(128) # noqa: DUO102 + ).encode() + encryptor.unlock(SECRET) + + encrypted_data = encryptor.encrypt(plaintext) + assert encrypted_data != plaintext + + decrypted_data = encryptor.decrypt(encrypted_data) + assert decrypted_data == plaintext + + +def test_key_creation(encryptor, key_file): + assert not key_file.is_file() + encryptor.unlock(SECRET) + assert key_file.is_file() + + +def test_existing_key_reused(encryptor, key_file): + assert not key_file.is_file() + + encryptor.unlock(SECRET) + key_file_hash_1 = get_file_sha256_hash(key_file) + + encryptor.unlock(SECRET) + key_file_hash_2 = get_file_sha256_hash(key_file) + + assert key_file_hash_1 == key_file_hash_2 + + +def test_use_locked_encryptor__encrypt(encryptor): + with pytest.raises(LockedKeyError): + encryptor.encrypt(PLAINTEXT) + + +def test_use_locked_encryptor__decrypt(encryptor): + with pytest.raises(LockedKeyError): + encryptor.decrypt(PLAINTEXT) + + +def test_lock(encryptor): + encryptor.unlock(SECRET) + encrypted_data = encryptor.encrypt(PLAINTEXT) + encryptor.lock() + + with pytest.raises(LockedKeyError): + encryptor.decrypt(encrypted_data) From 92c9ad3c713e768f989a601197e30311d1195b62 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 10:53:17 -0400 Subject: [PATCH 05/11] Island: Add note to DataStoreEncryptor --- .../cc/server_utils/encryption/data_store_encryptor.py | 1 + 1 file changed, 1 insertion(+) diff --git a/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py index 6b927e20b..0677df95c 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py @@ -15,6 +15,7 @@ _KEY_FILE_NAME = "mongo_key.bin" _encryptor: Union[None, IEncryptor] = None +# NOTE: This class is being replaced by RepositoryEncryptor class DataStoreEncryptor(IEncryptor): _KEY_LENGTH_BYTES = 32 From 0356596a41fff50fba180c7b04f3f6bfb6cc8ec9 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 11:05:47 -0400 Subject: [PATCH 06/11] Island: Add ILockableEncryptor.reset_key() --- .../encryption/i_lockable_encryptor.py | 6 ++++++ .../encryption/repository_encryptor.py | 7 +++++-- .../encryption/test_repository_encryptor.py | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py index b1b52f1d8..f0f5ac7d3 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py @@ -43,6 +43,12 @@ class ILockableEncryptor(IEncryptor): Lock the encryptor, making it unusable """ + @abstractmethod + def reset_key(self): + """ + Reset the encryptor's key + """ + @abstractmethod def encrypt(self, plaintext: bytes) -> bytes: """ diff --git a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py index 0e0310147..b4d0722f4 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -1,4 +1,3 @@ -import os import secrets from pathlib import Path @@ -22,7 +21,7 @@ class RepositoryEncryptor(ILockableEncryptor): self._key_based_encryptor = self._initialize_key_based_encryptor() def _initialize_key_based_encryptor(self): - if os.path.exists(self._key_file): + if self._key_file.is_file(): return self._load_key() return self._create_key() @@ -46,6 +45,10 @@ class RepositoryEncryptor(ILockableEncryptor): def lock(self): self._key_based_encryptor = None + def reset_key(self): + if self._key_file.is_file(): + self._key_file.unlink() + def encrypt(self, plaintext: bytes) -> bytes: if self._key_based_encryptor is None: raise LockedKeyError("Cannot encrypt while the encryptor is locked)") diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py index 74b02fb9e..6b99ff36d 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -68,3 +68,19 @@ def test_lock(encryptor): with pytest.raises(LockedKeyError): encryptor.decrypt(encrypted_data) + + +def test_reset(encryptor, key_file): + encryptor.unlock(SECRET) + key_file_hash_1 = get_file_sha256_hash(key_file) + + encryptor.reset_key() + encryptor.unlock(SECRET) + key_file_hash_2 = get_file_sha256_hash(key_file) + + assert key_file_hash_1 != key_file_hash_2 + + +def test_reset_before_unlock(encryptor): + # Test will fail if an exception is raised + encryptor.reset_key() From 5c65d581b526f32dc0fe0acd855619129038bf10 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 11:21:43 -0400 Subject: [PATCH 07/11] Island: Add UnlockError --- .../cc/server_utils/encryption/__init__.py | 2 +- .../encryption/i_lockable_encryptor.py | 7 +++++++ .../encryption/repository_encryptor.py | 9 ++++++--- .../encryption/test_repository_encryptor.py | 20 ++++++++++++++++++- 4 files changed, 33 insertions(+), 5 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/__init__.py b/monkey/monkey_island/cc/server_utils/encryption/__init__.py index d90468cc4..592d42381 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/__init__.py +++ b/monkey/monkey_island/cc/server_utils/encryption/__init__.py @@ -7,7 +7,7 @@ from .password_based_bytes_encryptor import ( InvalidCredentialsError, InvalidCiphertextError, ) -from .i_lockable_encryptor import ILockableEncryptor, LockedKeyError +from .i_lockable_encryptor import ILockableEncryptor, LockedKeyError, UnlockError from .repository_encryptor import RepositoryEncryptor from .data_store_encryptor import ( get_datastore_encryptor, diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py index f0f5ac7d3..231af0a16 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py @@ -21,6 +21,12 @@ class LockedKeyError(Exception): """ +class UnlockError(Exception): + """ + Raised if an error occurs while attempting to unlock an ILockableEncryptor + """ + + class ILockableEncryptor(IEncryptor): """ An encryptor that can be locked or unlocked. @@ -35,6 +41,7 @@ class ILockableEncryptor(IEncryptor): Unlock the encryptor :param secret: A secret that must be used to access the ILockableEncryptor's key material. + :raises UnlockError: If the ILockableEncryptor could not be unlocked """ @abstractmethod diff --git a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py index b4d0722f4..2c845fe43 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -3,7 +3,7 @@ from pathlib import Path from monkey_island.cc.server_utils.file_utils import open_new_securely_permissioned_file -from . import ILockableEncryptor, LockedKeyError +from . import ILockableEncryptor, LockedKeyError, UnlockError from .key_based_encryptor import KeyBasedEncryptor from .password_based_bytes_encryptor import PasswordBasedBytesEncryptor @@ -17,8 +17,11 @@ class RepositoryEncryptor(ILockableEncryptor): self._key_based_encryptor = None def unlock(self, secret: bytes): - self._password_based_encryptor = PasswordBasedBytesEncryptor(secret.decode()) - self._key_based_encryptor = self._initialize_key_based_encryptor() + try: + self._password_based_encryptor = PasswordBasedBytesEncryptor(secret.decode()) + self._key_based_encryptor = self._initialize_key_based_encryptor() + except Exception as err: + raise UnlockError(err) def _initialize_key_based_encryptor(self): if self._key_file.is_file(): diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py index 6b99ff36d..d1592219a 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -4,7 +4,11 @@ import string import pytest from common.utils.file_utils import get_file_sha256_hash -from monkey_island.cc.server_utils.encryption import LockedKeyError, RepositoryEncryptor +from monkey_island.cc.server_utils.encryption import ( + LockedKeyError, + RepositoryEncryptor, + UnlockError, +) PLAINTEXT = b"Hello, Monkey!" SECRET = b"53CR31" @@ -51,6 +55,20 @@ def test_existing_key_reused(encryptor, key_file): assert key_file_hash_1 == key_file_hash_2 +def test_unlock_os_error(encryptor, key_file): + key_file.mkdir() + + with pytest.raises(UnlockError): + encryptor.unlock(SECRET) + + +def test_unlock_wrong_password(encryptor): + encryptor.unlock(SECRET) + + with pytest.raises(UnlockError): + encryptor.unlock(b"WRONG_PASSWORD") + + def test_use_locked_encryptor__encrypt(encryptor): with pytest.raises(LockedKeyError): encryptor.encrypt(PLAINTEXT) From e362875201ecd54ae826fa1e6af3abd89867629d Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 11:38:23 -0400 Subject: [PATCH 08/11] Island: Fix incomplete logic in RepositoryEncrypto.reset_key() --- .../cc/server_utils/encryption/repository_encryptor.py | 3 +++ .../server_utils/encryption/test_repository_encryptor.py | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py index 2c845fe43..d45c760d5 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -52,6 +52,9 @@ class RepositoryEncryptor(ILockableEncryptor): if self._key_file.is_file(): self._key_file.unlink() + self._password_based_encryptor = None + self._key_based_encryptor = None + def encrypt(self, plaintext: bytes) -> bytes: if self._key_based_encryptor is None: raise LockedKeyError("Cannot encrypt while the encryptor is locked)") diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py index d1592219a..ff07efc5f 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -99,6 +99,14 @@ def test_reset(encryptor, key_file): assert key_file_hash_1 != key_file_hash_2 +def test_encrypt_after_reset(encryptor, key_file): + encryptor.unlock(SECRET) + encryptor.reset_key() + + with pytest.raises(LockedKeyError): + encryptor.encrypt(PLAINTEXT) + + def test_reset_before_unlock(encryptor): # Test will fail if an exception is raised encryptor.reset_key() From faf9cba1823d22fbcad8ff5cc011cfdd23e4fe60 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 11:53:07 -0400 Subject: [PATCH 09/11] Island: Add ResetKeyError --- .../cc/server_utils/encryption/__init__.py | 2 +- .../encryption/i_lockable_encryptor.py | 10 ++++++++++ .../encryption/repository_encryptor.py | 9 ++++++--- .../encryption/test_repository_encryptor.py | 14 ++++++++++++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/__init__.py b/monkey/monkey_island/cc/server_utils/encryption/__init__.py index 592d42381..9443fa4ab 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/__init__.py +++ b/monkey/monkey_island/cc/server_utils/encryption/__init__.py @@ -7,7 +7,7 @@ from .password_based_bytes_encryptor import ( InvalidCredentialsError, InvalidCiphertextError, ) -from .i_lockable_encryptor import ILockableEncryptor, LockedKeyError, UnlockError +from .i_lockable_encryptor import ILockableEncryptor, LockedKeyError, UnlockError, ResetKeyError from .repository_encryptor import RepositoryEncryptor from .data_store_encryptor import ( get_datastore_encryptor, diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py index 231af0a16..f31024655 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py @@ -27,6 +27,12 @@ class UnlockError(Exception): """ +class ResetKeyError(Exception): + """ + Raised if an error occurs while attempting to reset an ILockableEncryptor's key + """ + + class ILockableEncryptor(IEncryptor): """ An encryptor that can be locked or unlocked. @@ -54,6 +60,10 @@ class ILockableEncryptor(IEncryptor): def reset_key(self): """ Reset the encryptor's key + + Remove the existing key material so that it can never be used again. + + :raises ResetKeyError: If an error occurred while attemping to reset the key """ @abstractmethod diff --git a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py index d45c760d5..73e054289 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -3,7 +3,7 @@ from pathlib import Path from monkey_island.cc.server_utils.file_utils import open_new_securely_permissioned_file -from . import ILockableEncryptor, LockedKeyError, UnlockError +from . import ILockableEncryptor, LockedKeyError, ResetKeyError, UnlockError from .key_based_encryptor import KeyBasedEncryptor from .password_based_bytes_encryptor import PasswordBasedBytesEncryptor @@ -49,8 +49,11 @@ class RepositoryEncryptor(ILockableEncryptor): self._key_based_encryptor = None def reset_key(self): - if self._key_file.is_file(): - self._key_file.unlink() + try: + if self._key_file.is_file(): + self._key_file.unlink() + except Exception as err: + raise ResetKeyError(err) self._password_based_encryptor = None self._key_based_encryptor = None diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py index ff07efc5f..0dcb7cdcf 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -7,6 +7,7 @@ from common.utils.file_utils import get_file_sha256_hash from monkey_island.cc.server_utils.encryption import ( LockedKeyError, RepositoryEncryptor, + ResetKeyError, UnlockError, ) @@ -110,3 +111,16 @@ def test_encrypt_after_reset(encryptor, key_file): def test_reset_before_unlock(encryptor): # Test will fail if an exception is raised encryptor.reset_key() + + +def test_reset_key_error(key_file): + class UnlinkErrorWrapper(key_file.__class__): + def unlink(self): + raise OSError("Can't delete file") + + encryptor = RepositoryEncryptor(UnlinkErrorWrapper(key_file)) + encryptor.unlock(SECRET) + encryptor.lock() + + with pytest.raises(ResetKeyError): + encryptor.reset_key() From d6655a8e2c373eebab59c9f0f6e4fc23b2eacc1c Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 12:27:19 -0400 Subject: [PATCH 10/11] UT: Mark RepositoryEncryptor tests as slow --- .../cc/server_utils/encryption/test_repository_encryptor.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py index 0dcb7cdcf..2555d733a 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_repository_encryptor.py @@ -11,6 +11,9 @@ from monkey_island.cc.server_utils.encryption import ( UnlockError, ) +# Mark all tests in this module as slow +pytestmark = pytest.mark.slow + PLAINTEXT = b"Hello, Monkey!" SECRET = b"53CR31" From 75f3fb02ee0d7e2f3dae8e40f8a67298cb81c87c Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 11 Jul 2022 15:07:16 -0400 Subject: [PATCH 11/11] Island: Remove dependency from ILockableEncryptor -> IEncryptor ILockableEncryptor adds additional constraints on when encrypt() or decrypt() can be used. If ILockableEncryptor inherits from IEncryptor, it will violate the Liskov Substitution Principle --- .../cc/server_utils/encryption/i_lockable_encryptor.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py index f31024655..e092fba63 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/i_lockable_encryptor.py @@ -1,6 +1,4 @@ -from abc import abstractmethod - -from . import IEncryptor +from abc import ABC, abstractmethod # NOTE: The ILockableEncryptor introduces temporal coupling, that is, you must first unlock the # encryptor before you can use it. This is because the key material used to encrypt repository @@ -33,7 +31,7 @@ class ResetKeyError(Exception): """ -class ILockableEncryptor(IEncryptor): +class ILockableEncryptor(ABC): """ An encryptor that can be locked or unlocked.