From bdee3b9d8a2af078a16dd34969cde36b82e305bf Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 12 Jul 2022 11:57:30 -0400 Subject: [PATCH 1/4] UT: Improve formatting of test_mongo_credentials_repository.py --- .../test_mongo_credentials_repository.py | 27 +++---------------- 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py b/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py index 4b2a28849..e8776b440 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py +++ b/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py @@ -44,72 +44,51 @@ def mongo_repository(): def test_mongo_repository_get_configured(mongo_repository): - actual_configured_credentials = mongo_repository.get_configured_credentials() assert actual_configured_credentials == [] def test_mongo_repository_get_stolen(mongo_repository): - actual_stolen_credentials = mongo_repository.get_stolen_credentials() assert actual_stolen_credentials == [] def test_mongo_repository_get_all(mongo_repository): - actual_credentials = mongo_repository.get_all_credentials() assert actual_credentials == [] def test_mongo_repository_configured(mongo_repository): - mongo_repository.save_configured_credentials(CREDENTIALS_LIST) - actual_configured_credentials = mongo_repository.get_configured_credentials() - assert actual_configured_credentials == CREDENTIALS_LIST mongo_repository.remove_configured_credentials() - actual_configured_credentials = mongo_repository.get_configured_credentials() - assert actual_configured_credentials == [] def test_mongo_repository_stolen(mongo_repository): - - mongo_repository.save_configured_credentials(CONFIGURED_CREDENTIALS) mongo_repository.save_stolen_credentials(STOLEN_CREDENTIALS) - actual_stolen_credentials = mongo_repository.get_stolen_credentials() - assert actual_stolen_credentials == STOLEN_CREDENTIALS mongo_repository.remove_stolen_credentials() - actual_stolen_credentials = mongo_repository.get_stolen_credentials() - assert actual_stolen_credentials == [] def test_mongo_repository_all(mongo_repository): - mongo_repository.save_configured_credentials(CONFIGURED_CREDENTIALS) mongo_repository.save_stolen_credentials(STOLEN_CREDENTIALS) - actual_credentials = mongo_repository.get_all_credentials() - assert actual_credentials == CREDENTIALS_LIST mongo_repository.remove_all_credentials() - actual_credentials = mongo_repository.get_all_credentials() - actual_stolen_credentials = mongo_repository.get_stolen_credentials() - actual_configured_credentials = mongo_repository.get_configured_credentials() - - assert actual_credentials == [] - assert actual_stolen_credentials == [] - assert actual_configured_credentials == [] + assert mongo_repository.get_all_credentials() == [] + assert mongo_repository.get_stolen_credentials() == [] + assert mongo_repository.get_configured_credentials() == [] From cee52ab12cb722e6be0c785880f7cb365a4fc83b Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 12 Jul 2022 13:22:22 -0400 Subject: [PATCH 2/4] Island: Encrypt credentials in MongoCredentialsRepository --- .../mongo_credentials_repository.py | 69 +++++++++++++------ .../test_mongo_credentials_repository.py | 69 +++++++++++++++++-- 2 files changed, 111 insertions(+), 27 deletions(-) diff --git a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py index 454196ee8..5dd43a32f 100644 --- a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py +++ b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py @@ -1,10 +1,11 @@ -from typing import Sequence +from typing import Any, Dict, Mapping, Sequence from pymongo import MongoClient from common.credentials import Credentials from monkey_island.cc.repository import RemovalError, RetrievalError, StorageError from monkey_island.cc.repository.i_credentials_repository import ICredentialsRepository +from monkey_island.cc.server_utils.encryption import ILockableEncryptor class MongoCredentialsRepository(ICredentialsRepository): @@ -12,18 +13,15 @@ class MongoCredentialsRepository(ICredentialsRepository): Store credentials in a mongo database that can be used to propagate around the network. """ - def __init__(self, mongo: MongoClient): + def __init__(self, mongo: MongoClient, repository_encryptor: ILockableEncryptor): self._mongo = mongo + self._repository_encryptor = repository_encryptor def get_configured_credentials(self) -> Sequence[Credentials]: - return MongoCredentialsRepository._get_credentials_from_collection( - self._mongo.db.configured_credentials - ) + return self._get_credentials_from_collection(self._mongo.db.configured_credentials) def get_stolen_credentials(self) -> Sequence[Credentials]: - return MongoCredentialsRepository._get_credentials_from_collection( - self._mongo.db.stolen_credentials - ) + return self._get_credentials_from_collection(self._mongo.db.stolen_credentials) def get_all_credentials(self) -> Sequence[Credentials]: configured_credentials = self.get_configured_credentials() @@ -33,14 +31,10 @@ class MongoCredentialsRepository(ICredentialsRepository): def save_configured_credentials(self, credentials: Sequence[Credentials]): # TODO: Fix deduplication of Credentials in mongo - MongoCredentialsRepository._save_credentials_to_collection( - credentials, self._mongo.db.configured_credentials - ) + self._save_credentials_to_collection(credentials, self._mongo.db.configured_credentials) def save_stolen_credentials(self, credentials: Sequence[Credentials]): - MongoCredentialsRepository._save_credentials_to_collection( - credentials, self._mongo.db.stolen_credentials - ) + self._save_credentials_to_collection(credentials, self._mongo.db.stolen_credentials) def remove_configured_credentials(self): MongoCredentialsRepository._remove_credentials_fom_collection( @@ -56,27 +50,58 @@ class MongoCredentialsRepository(ICredentialsRepository): self.remove_configured_credentials() self.remove_stolen_credentials() - @staticmethod - def _get_credentials_from_collection(collection) -> Sequence[Credentials]: + def _get_credentials_from_collection(self, collection) -> Sequence[Credentials]: try: collection_result = [] list_collection_result = list(collection.find({})) - for c in list_collection_result: - del c["_id"] - collection_result.append(Credentials.from_mapping(c)) + for encrypted_credentials in list_collection_result: + del encrypted_credentials["_id"] + plaintext_credentials = self._decrypt_credentials_mapping(encrypted_credentials) + collection_result.append(Credentials.from_mapping(plaintext_credentials)) return collection_result except Exception as err: raise RetrievalError(err) - @staticmethod - def _save_credentials_to_collection(credentials: Sequence[Credentials], collection): + def _save_credentials_to_collection(self, credentials: Sequence[Credentials], collection): try: for c in credentials: - collection.insert_one(Credentials.to_mapping(c)) + encrypted_credentials = self._encrypt_credentials_mapping(Credentials.to_mapping(c)) + collection.insert_one(encrypted_credentials) except Exception as err: raise StorageError(err) + # NOTE: The encryption/decryption is complicated and also full of mostly duplicated code. Rather + # than spend the effort to improve them now, we can revisit them when we resolve #2072. + # Resolving #2072 will make it easier to simplify these methods and remove duplication. + def _encrypt_credentials_mapping(self, mapping: Mapping[str, Any]) -> Mapping[str, Any]: + encrypted_mapping: Dict[str, Any] = {} + + for secret_or_identity, credentials_components in mapping.items(): + encrypted_mapping[secret_or_identity] = [] + for component in credentials_components: + encrypted_component = {} + for key, value in component.items(): + encrypted_component[key] = self._repository_encryptor.encrypt(value.encode()) + + encrypted_mapping[secret_or_identity].append(encrypted_component) + + return encrypted_mapping + + def _decrypt_credentials_mapping(self, mapping: Mapping[str, Any]) -> Mapping[str, Any]: + encrypted_mapping: Dict[str, Any] = {} + + for secret_or_identity, credentials_components in mapping.items(): + encrypted_mapping[secret_or_identity] = [] + for component in credentials_components: + encrypted_component = {} + for key, value in component.items(): + encrypted_component[key] = self._repository_encryptor.decrypt(value).decode() + + encrypted_mapping[secret_or_identity].append(encrypted_component) + + return encrypted_mapping + @staticmethod def _remove_credentials_fom_collection(collection): try: diff --git a/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py b/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py index e8776b440..c3d55940d 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py +++ b/monkey/tests/unit_tests/monkey_island/cc/repository/test_mongo_credentials_repository.py @@ -1,8 +1,11 @@ +from unittest.mock import MagicMock + import mongomock import pytest from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username from monkey_island.cc.repository import MongoCredentialsRepository +from monkey_island.cc.server_utils.encryption import ILockableEncryptor USER1 = "test_user_1" USER2 = "test_user_2" @@ -36,11 +39,27 @@ STOLEN_CREDENTIALS = [CREDENTIALS_OBJECT_2] CREDENTIALS_LIST = [CREDENTIALS_OBJECT_1, CREDENTIALS_OBJECT_2] -@pytest.fixture -def mongo_repository(): - mongo = mongomock.MongoClient() +def reverse(data: bytes) -> bytes: + return bytes(reversed(data)) - return MongoCredentialsRepository(mongo) + +@pytest.fixture +def repository_encryptor(): + repository_encryptor = MagicMock(spec=ILockableEncryptor) + repository_encryptor.encrypt = MagicMock(side_effect=reverse) + repository_encryptor.decrypt = MagicMock(side_effect=reverse) + + return repository_encryptor + + +@pytest.fixture +def mongo_client(): + return mongomock.MongoClient() + + +@pytest.fixture +def mongo_repository(mongo_client, repository_encryptor): + return MongoCredentialsRepository(mongo_client, repository_encryptor) def test_mongo_repository_get_configured(mongo_repository): @@ -74,7 +93,7 @@ def test_mongo_repository_configured(mongo_repository): def test_mongo_repository_stolen(mongo_repository): mongo_repository.save_stolen_credentials(STOLEN_CREDENTIALS) actual_stolen_credentials = mongo_repository.get_stolen_credentials() - assert actual_stolen_credentials == STOLEN_CREDENTIALS + assert sorted(actual_stolen_credentials) == sorted(STOLEN_CREDENTIALS) mongo_repository.remove_stolen_credentials() actual_stolen_credentials = mongo_repository.get_stolen_credentials() @@ -92,3 +111,43 @@ def test_mongo_repository_all(mongo_repository): assert mongo_repository.get_all_credentials() == [] assert mongo_repository.get_stolen_credentials() == [] assert mongo_repository.get_configured_credentials() == [] + + +# NOTE: The following tests are complicated, but they work. Rather than spend the effort to improve +# them now, we can revisit them when we resolve #2072. Resolving #2072 will make it easier to +# simplify these tests. +def test_configured_secrets_encrypted(mongo_repository, mongo_client): + mongo_repository.save_configured_credentials([CREDENTIALS_OBJECT_2]) + check_if_stored_credentials_encrypted(mongo_client, CREDENTIALS_OBJECT_2) + + +def test_stolen_secrets_encrypted(mongo_repository, mongo_client): + mongo_repository.save_stolen_credentials([CREDENTIALS_OBJECT_2]) + check_if_stored_credentials_encrypted(mongo_client, CREDENTIALS_OBJECT_2) + + +def check_if_stored_credentials_encrypted(mongo_client, original_credentials): + raw_credentials = get_all_credentials_in_mongo(mongo_client) + original_credentials_mapping = Credentials.to_mapping(original_credentials) + for rc in raw_credentials: + for identity_or_secret, credentials_components in rc.items(): + for component in credentials_components: + for key, value in component.items(): + assert ( + original_credentials_mapping[identity_or_secret][0].get(key, None) != value + ) + + +def get_all_credentials_in_mongo(mongo_client): + encrypted_credentials = [] + + # Loop through all databases and collections and search for credentials. We don't want the tests + # to assume anything about the internal workings of the repository. + for db in mongo_client.list_database_names(): + for collection in mongo_client[db].list_collection_names(): + mongo_credentials = mongo_client[db][collection].find({}) + for mc in mongo_credentials: + del mc["_id"] + encrypted_credentials.append(mc) + + return encrypted_credentials From fe7188798f000205983011209c24b8a64183a76e Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 12 Jul 2022 13:23:38 -0400 Subject: [PATCH 3/4] Island: Add note about encryption decorator in MongoCredentialsRepository --- .../cc/repository/mongo_credentials_repository.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py index 5dd43a32f..722aa3463 100644 --- a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py +++ b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py @@ -74,6 +74,9 @@ class MongoCredentialsRepository(ICredentialsRepository): # NOTE: The encryption/decryption is complicated and also full of mostly duplicated code. Rather # than spend the effort to improve them now, we can revisit them when we resolve #2072. # Resolving #2072 will make it easier to simplify these methods and remove duplication. + # + # If possible, implement the encryption/decryption as a decorator so it can be reused with + # different ICredentialsRepository implementations def _encrypt_credentials_mapping(self, mapping: Mapping[str, Any]) -> Mapping[str, Any]: encrypted_mapping: Dict[str, Any] = {} From a9799de1ba6fbbb5a1a89a9b6fb4651efa8e8617 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 13 Jul 2022 06:07:08 -0400 Subject: [PATCH 4/4] Island: Remove extra ')' from strings --- .../cc/server_utils/encryption/repository_encryptor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 73e054289..c53d6e38b 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/repository_encryptor.py @@ -60,12 +60,12 @@ class RepositoryEncryptor(ILockableEncryptor): def encrypt(self, plaintext: bytes) -> bytes: if self._key_based_encryptor is None: - raise LockedKeyError("Cannot encrypt while the encryptor is locked)") + 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)") + raise LockedKeyError("Cannot decrypt while the encryptor is locked") return self._key_based_encryptor.decrypt(ciphertext)