From 2b245b34cba7045b53c015601a1eb86d95e364db Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 14 Jul 2022 13:10:24 -0400 Subject: [PATCH 01/31] Island: Simplify Credentials Storing a sequence of identities and secrets in Credentials objects added a lot of complication. From now on, a Credentials object consists of one identity and one secret. See #2072. --- monkey/common/credentials/credentials.py | 40 ++------- .../data_for_tests/propagation_credentials.py | 21 +---- .../common/credentials/test_credentials.py | 89 +++++++++++-------- 3 files changed, 67 insertions(+), 83 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index 4916a44e4..5483e9ff8 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -1,7 +1,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Any, Mapping, MutableMapping, Sequence, Tuple +from typing import Any, Mapping, MutableMapping, Sequence from marshmallow import Schema, fields, post_load, pre_dump from marshmallow.exceptions import MarshmallowError @@ -42,27 +42,15 @@ CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA = { class CredentialsSchema(Schema): - # Use fields.List instead of fields.Tuple because marshmallow requires fields.Tuple to have a - # fixed length. - identities = fields.List(fields.Mapping()) - secrets = fields.List(fields.Mapping()) + identity = fields.Mapping() + secret = fields.Mapping() @post_load def _make_credentials( self, data: MutableMapping, **kwargs: Mapping[str, Any] ) -> Mapping[str, Sequence[Mapping[str, Any]]]: - data["identities"] = tuple( - [ - CredentialsSchema._build_credential_component(component) - for component in data["identities"] - ] - ) - data["secrets"] = tuple( - [ - CredentialsSchema._build_credential_component(component) - for component in data["secrets"] - ] - ) + data["identity"] = CredentialsSchema._build_credential_component(data["identity"]) + data["secret"] = CredentialsSchema._build_credential_component(data["secret"]) return data @@ -89,18 +77,8 @@ class CredentialsSchema(Schema): ) -> Mapping[str, Sequence[Mapping[str, Any]]]: data = {} - data["identities"] = tuple( - [ - CredentialsSchema._serialize_credential_component(component) - for component in credentials.identities - ] - ) - data["secrets"] = tuple( - [ - CredentialsSchema._serialize_credential_component(component) - for component in credentials.secrets - ] - ) + data["identity"] = CredentialsSchema._serialize_credential_component(credentials.identity) + data["secret"] = CredentialsSchema._serialize_credential_component(credentials.secret) return data @@ -117,8 +95,8 @@ class CredentialsSchema(Schema): @dataclass(frozen=True) class Credentials(IJSONSerializable): - identities: Tuple[ICredentialComponent] - secrets: Tuple[ICredentialComponent] + identity: ICredentialComponent + secret: ICredentialComponent @staticmethod def from_mapping(credentials: Mapping) -> Credentials: diff --git a/monkey/tests/data_for_tests/propagation_credentials.py b/monkey/tests/data_for_tests/propagation_credentials.py index df6c213cf..47862b2bc 100644 --- a/monkey/tests/data_for_tests/propagation_credentials.py +++ b/monkey/tests/data_for_tests/propagation_credentials.py @@ -6,21 +6,8 @@ nt_hash = "C1C58F96CDF212B50837BC11A00BE47C" lm_hash = "299BD128C1101FD6299BD128C1101FD6" password_1 = "trytostealthis" password_2 = "password" -password_3 = "12345678" -PROPAGATION_CREDENTIALS_1 = Credentials( - identities=(Username(username),), - secrets=(NTHash(nt_hash), LMHash(lm_hash), Password(password_1)), -) -PROPAGATION_CREDENTIALS_2 = Credentials( - identities=(Username(username), Username(special_username)), - secrets=(Password(password_1), Password(password_2), Password(password_3)), -) -PROPAGATION_CREDENTIALS_3 = Credentials( - identities=(Username(username),), - secrets=(Password(password_1),), -) -PROPAGATION_CREDENTIALS_4 = Credentials( - identities=(Username(username),), - secrets=(Password(password_2),), -) +PROPAGATION_CREDENTIALS_1 = Credentials(identity=Username(username), secret=Password(password_1)) +PROPAGATION_CREDENTIALS_2 = Credentials(identity=Username(special_username), secret=LMHash(lm_hash)) +PROPAGATION_CREDENTIALS_3 = Credentials(identity=Username(username), secret=NTHash(nt_hash)) +PROPAGATION_CREDENTIALS_4 = Credentials(identity=Username(username), secret=Password(password_2)) diff --git a/monkey/tests/unit_tests/common/credentials/test_credentials.py b/monkey/tests/unit_tests/common/credentials/test_credentials.py index 0f5fc519a..cb5fb76fc 100644 --- a/monkey/tests/unit_tests/common/credentials/test_credentials.py +++ b/monkey/tests/unit_tests/common/credentials/test_credentials.py @@ -1,3 +1,4 @@ +import copy import json import pytest @@ -14,7 +15,6 @@ from common.credentials import ( ) USER1 = "test_user_1" -USER2 = "test_user_2" PASSWORD = "12435" LM_HASH = "AEBD4DE384C7EC43AAD3B435B51404EE" NT_HASH = "7A21990FCD3D759941E45C490F143D5F" @@ -22,74 +22,93 @@ PUBLIC_KEY = "MY_PUBLIC_KEY" PRIVATE_KEY = "MY_PRIVATE_KEY" CREDENTIALS_DICT = { - "identities": [ - {"credential_type": "USERNAME", "username": USER1}, - {"credential_type": "USERNAME", "username": USER2}, - ], - "secrets": [ - {"credential_type": "PASSWORD", "password": PASSWORD}, - {"credential_type": "LM_HASH", "lm_hash": LM_HASH}, - {"credential_type": "NT_HASH", "nt_hash": NT_HASH}, - { - "credential_type": "SSH_KEYPAIR", - "public_key": PUBLIC_KEY, - "private_key": PRIVATE_KEY, - }, - ], + "identity": {"credential_type": "USERNAME", "username": USER1}, + "secret": {}, } -CREDENTIALS_JSON = json.dumps(CREDENTIALS_DICT) - -IDENTITIES = (Username(USER1), Username(USER2)) +IDENTITY = Username(USER1) SECRETS = ( Password(PASSWORD), LMHash(LM_HASH), NTHash(NT_HASH), SSHKeypair(PRIVATE_KEY, PUBLIC_KEY), ) -CREDENTIALS_OBJECT = Credentials(IDENTITIES, SECRETS) + +SECRETS_DICTS = [ + {"credential_type": "PASSWORD", "password": PASSWORD}, + {"credential_type": "LM_HASH", "lm_hash": LM_HASH}, + {"credential_type": "NT_HASH", "nt_hash": NT_HASH}, + { + "credential_type": "SSH_KEYPAIR", + "public_key": PUBLIC_KEY, + "private_key": PRIVATE_KEY, + }, +] -def test_credentials_serialization_json(): - serialized_credentials = Credentials.to_json(CREDENTIALS_OBJECT) +@pytest.mark.parametrize("secret, expected_secret", zip(SECRETS, SECRETS_DICTS)) +def test_credentials_serialization_json(secret, expected_secret): + expected_credentials = copy.copy(CREDENTIALS_DICT) + expected_credentials["secret"] = expected_secret + c = Credentials(IDENTITY, secret) - assert json.loads(serialized_credentials) == CREDENTIALS_DICT + serialized_credentials = Credentials.to_json(c) + + assert json.loads(serialized_credentials) == expected_credentials -def test_credentials_serialization_mapping(): - serialized_credentials = Credentials.to_mapping(CREDENTIALS_OBJECT) +@pytest.mark.parametrize("secret, expected_secret", zip(SECRETS, SECRETS_DICTS)) +def test_credentials_serialization_mapping(secret, expected_secret): + expected_credentials = copy.copy(CREDENTIALS_DICT) + expected_credentials["secret"] = expected_secret + c = Credentials(IDENTITY, secret) - assert serialized_credentials == CREDENTIALS_DICT + serialized_credentials = Credentials.to_mapping(c) + + assert serialized_credentials == expected_credentials -def test_credentials_deserialization__from_mapping(): - deserialized_credentials = Credentials.from_mapping(CREDENTIALS_DICT) +@pytest.mark.parametrize("secret, secret_dict", zip(SECRETS, SECRETS_DICTS)) +def test_credentials_deserialization__from_mapping(secret, secret_dict): + expected_credentials = Credentials(IDENTITY, secret) + credentials_dict = copy.copy(CREDENTIALS_DICT) + credentials_dict["secret"] = secret_dict - assert deserialized_credentials == CREDENTIALS_OBJECT + deserialized_credentials = Credentials.from_mapping(credentials_dict) + + assert deserialized_credentials == expected_credentials -def test_credentials_deserialization__from_json(): - deserialized_credentials = Credentials.from_json(CREDENTIALS_JSON) +@pytest.mark.parametrize("secret, secret_dict", zip(SECRETS, SECRETS_DICTS)) +def test_credentials_deserialization__from_json(secret, secret_dict): + expected_credentials = Credentials(IDENTITY, secret) + credentials_dict = copy.copy(CREDENTIALS_DICT) + credentials_dict["secret"] = secret_dict - assert deserialized_credentials == CREDENTIALS_OBJECT + deserialized_credentials = Credentials.from_json(json.dumps(credentials_dict)) + + assert deserialized_credentials == expected_credentials def test_credentials_deserialization__invalid_credentials(): - invalid_data = {"secrets": [], "unknown_key": []} + invalid_data = {"secret": SECRETS_DICTS[0], "unknown_key": []} with pytest.raises(InvalidCredentialsError): Credentials.from_mapping(invalid_data) def test_credentials_deserialization__invalid_component_type(): - invalid_data = {"secrets": [], "identities": [{"credential_type": "FAKE", "username": "user1"}]} + invalid_data = { + "secret": SECRETS_DICTS[0], + "identity": {"credential_type": "FAKE", "username": "user1"}, + } with pytest.raises(InvalidCredentialsError): Credentials.from_mapping(invalid_data) def test_credentials_deserialization__invalid_component(): invalid_data = { - "secrets": [], - "identities": [{"credential_type": "USERNAME", "unknown_field": "user1"}], + "secret": SECRETS_DICTS[0], + "identity": {"credential_type": "USERNAME", "unknown_field": "user1"}, } with pytest.raises(InvalidCredentialComponentError): Credentials.from_mapping(invalid_data) From 04d72c0d365f738d883355b07e9b6b335dcd04f7 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 14 Jul 2022 13:11:45 -0400 Subject: [PATCH 02/31] UT: Use new Credentials object in test_credential_telem_send() --- .../telemetry/test_credentials_telem.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py b/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py index 701071fbb..1ceb9f4e5 100644 --- a/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py +++ b/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py @@ -2,7 +2,7 @@ import json import pytest -from common.credentials import Credentials, Password, SSHKeypair, Username +from common.credentials import Credentials, Password, Username from infection_monkey.telemetry.credentials_telem import CredentialsTelem USERNAME = "m0nkey" @@ -14,24 +14,15 @@ PRIVATE_KEY = "priv_key" @pytest.fixture def credentials_for_test(): - return Credentials( - [Username(USERNAME)], [Password(PASSWORD), SSHKeypair(PRIVATE_KEY, PUBLIC_KEY)] - ) + return Credentials(Username(USERNAME), Password(PASSWORD)) def test_credential_telem_send(spy_send_telemetry, credentials_for_test): expected_data = [ { - "identities": [{"username": USERNAME, "credential_type": "USERNAME"}], - "secrets": [ - {"password": PASSWORD, "credential_type": "PASSWORD"}, - { - "private_key": PRIVATE_KEY, - "public_key": PUBLIC_KEY, - "credential_type": "SSH_KEYPAIR", - }, - ], + "identity": {"username": USERNAME, "credential_type": "USERNAME"}, + "secret": {"password": PASSWORD, "credential_type": "PASSWORD"}, } ] From add6ca3941bfba188a76da6d8b26315a18d3bd32 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 14 Jul 2022 13:32:16 -0400 Subject: [PATCH 03/31] Island: Use new Credentials object in MongoCredentialsRepository --- .../mongo_credentials_repository.py | 29 +++++++-------- .../data_for_tests/propagation_credentials.py | 15 +++++++- .../test_mongo_credentials_repository.py | 36 +++++++++---------- 3 files changed, 45 insertions(+), 35 deletions(-) diff --git a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py index 722aa3463..3da05611f 100644 --- a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py +++ b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py @@ -80,30 +80,27 @@ class MongoCredentialsRepository(ICredentialsRepository): 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()) + for secret_or_identity, credentials_component in mapping.items(): + encrypted_component = {} + for key, value in credentials_component.items(): + encrypted_component[key] = self._repository_encryptor.encrypt(value.encode()) - encrypted_mapping[secret_or_identity].append(encrypted_component) + encrypted_mapping[secret_or_identity] = encrypted_component return encrypted_mapping def _decrypt_credentials_mapping(self, mapping: Mapping[str, Any]) -> Mapping[str, Any]: - encrypted_mapping: Dict[str, Any] = {} + decrypted_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() + for secret_or_identity, credentials_component in mapping.items(): + decrypted_mapping[secret_or_identity] = [] + decrypted_component = {} + for key, value in credentials_component.items(): + decrypted_component[key] = self._repository_encryptor.decrypt(value).decode() - encrypted_mapping[secret_or_identity].append(encrypted_component) + decrypted_mapping[secret_or_identity] = decrypted_component - return encrypted_mapping + return decrypted_mapping @staticmethod def _remove_credentials_fom_collection(collection): diff --git a/monkey/tests/data_for_tests/propagation_credentials.py b/monkey/tests/data_for_tests/propagation_credentials.py index 47862b2bc..cea619ca7 100644 --- a/monkey/tests/data_for_tests/propagation_credentials.py +++ b/monkey/tests/data_for_tests/propagation_credentials.py @@ -1,4 +1,4 @@ -from common.credentials import Credentials, LMHash, NTHash, Password, Username +from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username username = "m0nk3y_user" special_username = "m0nk3y.user" @@ -6,8 +6,21 @@ nt_hash = "C1C58F96CDF212B50837BC11A00BE47C" lm_hash = "299BD128C1101FD6299BD128C1101FD6" password_1 = "trytostealthis" password_2 = "password" +PUBLIC_KEY = "MY_PUBLIC_KEY" +PRIVATE_KEY = "MY_PRIVATE_KEY" PROPAGATION_CREDENTIALS_1 = Credentials(identity=Username(username), secret=Password(password_1)) PROPAGATION_CREDENTIALS_2 = Credentials(identity=Username(special_username), secret=LMHash(lm_hash)) PROPAGATION_CREDENTIALS_3 = Credentials(identity=Username(username), secret=NTHash(nt_hash)) PROPAGATION_CREDENTIALS_4 = Credentials(identity=Username(username), secret=Password(password_2)) +PROPAGATION_CREDENTIALS_5 = Credentials( + identity=Username(username), secret=SSHKeypair(PRIVATE_KEY, PUBLIC_KEY) +) + +PROPAGATION_CREDENTIALS = [ + PROPAGATION_CREDENTIALS_1, + PROPAGATION_CREDENTIALS_2, + PROPAGATION_CREDENTIALS_3, + PROPAGATION_CREDENTIALS_4, + PROPAGATION_CREDENTIALS_5, +] 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 c3d55940d..0d1b90801 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 @@ -2,6 +2,7 @@ from unittest.mock import MagicMock import mongomock import pytest +from tests.data_for_tests.propagation_credentials import PROPAGATION_CREDENTIALS from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username from monkey_island.cc.repository import MongoCredentialsRepository @@ -32,9 +33,9 @@ SECRETS_2 = (Password(PASSWORD2), Password(PASSWORD3)) CREDENTIALS_OBJECT_2 = Credentials(IDENTITIES_2, SECRETS_2) -CONFIGURED_CREDENTIALS = [CREDENTIALS_OBJECT_1] +CONFIGURED_CREDENTIALS = PROPAGATION_CREDENTIALS[0:3] -STOLEN_CREDENTIALS = [CREDENTIALS_OBJECT_2] +STOLEN_CREDENTIALS = PROPAGATION_CREDENTIALS[3:6] CREDENTIALS_LIST = [CREDENTIALS_OBJECT_1, CREDENTIALS_OBJECT_2] @@ -81,9 +82,9 @@ def test_mongo_repository_get_all(mongo_repository): def test_mongo_repository_configured(mongo_repository): - mongo_repository.save_configured_credentials(CREDENTIALS_LIST) + mongo_repository.save_configured_credentials(PROPAGATION_CREDENTIALS) actual_configured_credentials = mongo_repository.get_configured_credentials() - assert actual_configured_credentials == CREDENTIALS_LIST + assert actual_configured_credentials == PROPAGATION_CREDENTIALS mongo_repository.remove_configured_credentials() actual_configured_credentials = mongo_repository.get_configured_credentials() @@ -93,7 +94,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 sorted(actual_stolen_credentials) == sorted(STOLEN_CREDENTIALS) + assert actual_stolen_credentials == STOLEN_CREDENTIALS mongo_repository.remove_stolen_credentials() actual_stolen_credentials = mongo_repository.get_stolen_credentials() @@ -104,7 +105,7 @@ 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 + assert actual_credentials == PROPAGATION_CREDENTIALS mongo_repository.remove_all_credentials() @@ -116,26 +117,25 @@ def test_mongo_repository_all(mongo_repository): # 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) +@pytest.mark.parametrize("credentials", PROPAGATION_CREDENTIALS) +def test_configured_secrets_encrypted(mongo_repository, mongo_client, credentials): + mongo_repository.save_configured_credentials([credentials]) + check_if_stored_credentials_encrypted(mongo_client, credentials) -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) +@pytest.mark.parametrize("credentials", PROPAGATION_CREDENTIALS) +def test_stolen_secrets_encrypted(mongo_repository, mongo_client, credentials): + mongo_repository.save_stolen_credentials([credentials]) + check_if_stored_credentials_encrypted(mongo_client, credentials) 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 - ) + for identity_or_secret, credentials_component in rc.items(): + for key, value in credentials_component.items(): + assert original_credentials_mapping[identity_or_secret][key] != value def get_all_credentials_in_mongo(mongo_client): From 0687b010ff3c609430680c388441c50e0a88282c Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 14 Jul 2022 14:29:06 -0400 Subject: [PATCH 04/31] Island: Improve code quality of credentials encryption/decryption --- .../mongo_credentials_repository.py | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py index 3da05611f..aeaa5b884 100644 --- a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py +++ b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py @@ -71,19 +71,16 @@ class MongoCredentialsRepository(ICredentialsRepository): 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. - # - # If possible, implement the encryption/decryption as a decorator so it can be reused with + # TODO: 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] = {} for secret_or_identity, credentials_component in mapping.items(): - encrypted_component = {} - for key, value in credentials_component.items(): - encrypted_component[key] = self._repository_encryptor.encrypt(value.encode()) + encrypted_component = { + key: self._repository_encryptor.encrypt(value.encode()) + for key, value in credentials_component.items() + } encrypted_mapping[secret_or_identity] = encrypted_component @@ -93,10 +90,10 @@ class MongoCredentialsRepository(ICredentialsRepository): decrypted_mapping: Dict[str, Any] = {} for secret_or_identity, credentials_component in mapping.items(): - decrypted_mapping[secret_or_identity] = [] - decrypted_component = {} - for key, value in credentials_component.items(): - decrypted_component[key] = self._repository_encryptor.decrypt(value).decode() + decrypted_component = { + key: self._repository_encryptor.decrypt(value).decode() + for key, value in credentials_component.items() + } decrypted_mapping[secret_or_identity] = decrypted_component From e9dc8d88e739520ff390da0123005dedd6eb09b4 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 14 Jul 2022 14:29:24 -0400 Subject: [PATCH 05/31] UT: Improve code quality of credentials encryption/decryption tests --- .../test_mongo_credentials_repository.py | 52 ++++++++++++++----- 1 file changed, 40 insertions(+), 12 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 0d1b90801..56453e4e2 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,7 +1,11 @@ +from typing import Any, Iterable, List, Mapping, Sequence from unittest.mock import MagicMock import mongomock import pytest +from pymongo import MongoClient +from pymongo.collection import Collection +from pymongo.database import Database from tests.data_for_tests.propagation_credentials import PROPAGATION_CREDENTIALS from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username @@ -118,36 +122,60 @@ def test_mongo_repository_all(mongo_repository): # them now, we can revisit them when we resolve #2072. Resolving #2072 will make it easier to # simplify these tests. @pytest.mark.parametrize("credentials", PROPAGATION_CREDENTIALS) -def test_configured_secrets_encrypted(mongo_repository, mongo_client, credentials): +def test_configured_secrets_encrypted( + mongo_repository: MongoCredentialsRepository, + mongo_client: MongoClient, + credentials: Sequence[Credentials], +): mongo_repository.save_configured_credentials([credentials]) check_if_stored_credentials_encrypted(mongo_client, credentials) @pytest.mark.parametrize("credentials", PROPAGATION_CREDENTIALS) -def test_stolen_secrets_encrypted(mongo_repository, mongo_client, credentials): +def test_stolen_secrets_encrypted(mongo_repository, mongo_client, credentials: Credentials): mongo_repository.save_stolen_credentials([credentials]) check_if_stored_credentials_encrypted(mongo_client, credentials) -def check_if_stored_credentials_encrypted(mongo_client, original_credentials): - raw_credentials = get_all_credentials_in_mongo(mongo_client) +def check_if_stored_credentials_encrypted(mongo_client: MongoClient, original_credentials): original_credentials_mapping = Credentials.to_mapping(original_credentials) + raw_credentials = get_all_credentials_in_mongo(mongo_client) + for rc in raw_credentials: for identity_or_secret, credentials_component in rc.items(): for key, value in credentials_component.items(): - assert original_credentials_mapping[identity_or_secret][key] != value + assert original_credentials_mapping[identity_or_secret][key] != value.decode() -def get_all_credentials_in_mongo(mongo_client): +def get_all_credentials_in_mongo( + mongo_client: MongoClient, +) -> Iterable[Mapping[str, Mapping[str, Any]]]: 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) + for collection in get_all_collections_in_mongo(mongo_client): + mongo_credentials = collection.find({}) + for mc in mongo_credentials: + del mc["_id"] + encrypted_credentials.append(mc) return encrypted_credentials + + +def get_all_collections_in_mongo(mongo_client: MongoClient) -> Iterable[Collection]: + collections: List[Collection] = [] + + databases = get_all_databases_in_mongo(mongo_client) + for db in databases: + collections.extend(get_all_collections_in_database(db)) + + return collections + + +def get_all_databases_in_mongo(mongo_client) -> Iterable[Database]: + return (mongo_client[db_name] for db_name in mongo_client.list_database_names()) + + +def get_all_collections_in_database(db: Database) -> Iterable[Collection]: + return (db[collection_name] for collection_name in db.list_collection_names()) From 7bf80946ba3a6efca90f4987cc7a4d7cc663e4a1 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 09:08:57 -0400 Subject: [PATCH 06/31] UT: Rename test propagation credentials --- .../data_for_tests/propagation_credentials.py | 20 ++++---- .../resources/test_propagation_credentials.py | 46 +++++++++---------- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/monkey/tests/data_for_tests/propagation_credentials.py b/monkey/tests/data_for_tests/propagation_credentials.py index cea619ca7..15351da59 100644 --- a/monkey/tests/data_for_tests/propagation_credentials.py +++ b/monkey/tests/data_for_tests/propagation_credentials.py @@ -9,18 +9,18 @@ password_2 = "password" PUBLIC_KEY = "MY_PUBLIC_KEY" PRIVATE_KEY = "MY_PRIVATE_KEY" -PROPAGATION_CREDENTIALS_1 = Credentials(identity=Username(username), secret=Password(password_1)) -PROPAGATION_CREDENTIALS_2 = Credentials(identity=Username(special_username), secret=LMHash(lm_hash)) -PROPAGATION_CREDENTIALS_3 = Credentials(identity=Username(username), secret=NTHash(nt_hash)) -PROPAGATION_CREDENTIALS_4 = Credentials(identity=Username(username), secret=Password(password_2)) -PROPAGATION_CREDENTIALS_5 = Credentials( +PASSWORD_CREDENTIALS_1 = Credentials(identity=Username(username), secret=Password(password_1)) +PASSWORD_CREDENTIALS_2 = Credentials(identity=Username(username), secret=Password(password_2)) +LM_HASH_CREDENTIALS = Credentials(identity=Username(special_username), secret=LMHash(lm_hash)) +NT_HASH_CREDENTIALS = Credentials(identity=Username(username), secret=NTHash(nt_hash)) +SSH_KEY_CREDENTIALS = Credentials( identity=Username(username), secret=SSHKeypair(PRIVATE_KEY, PUBLIC_KEY) ) PROPAGATION_CREDENTIALS = [ - PROPAGATION_CREDENTIALS_1, - PROPAGATION_CREDENTIALS_2, - PROPAGATION_CREDENTIALS_3, - PROPAGATION_CREDENTIALS_4, - PROPAGATION_CREDENTIALS_5, + PASSWORD_CREDENTIALS_1, + LM_HASH_CREDENTIALS, + NT_HASH_CREDENTIALS, + PASSWORD_CREDENTIALS_2, + SSH_KEY_CREDENTIALS, ] diff --git a/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py b/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py index 93e82a223..e613d85b5 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py +++ b/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py @@ -6,10 +6,10 @@ from urllib.parse import urljoin import pytest from tests.common import StubDIContainer from tests.data_for_tests.propagation_credentials import ( - PROPAGATION_CREDENTIALS_1, - PROPAGATION_CREDENTIALS_2, - PROPAGATION_CREDENTIALS_3, - PROPAGATION_CREDENTIALS_4, + LM_HASH_CREDENTIALS, + NT_HASH_CREDENTIALS, + PASSWORD_CREDENTIALS_1, + PASSWORD_CREDENTIALS_2, ) from tests.monkey_island import InMemoryCredentialsRepository @@ -43,21 +43,19 @@ def flask_client(build_flask_client, credentials_repository): def test_propagation_credentials_endpoint_get(flask_client, credentials_repository): credentials_repository.save_configured_credentials( - [PROPAGATION_CREDENTIALS_1, PROPAGATION_CREDENTIALS_3] - ) - credentials_repository.save_stolen_credentials( - [PROPAGATION_CREDENTIALS_2, PROPAGATION_CREDENTIALS_4] + [PASSWORD_CREDENTIALS_1, NT_HASH_CREDENTIALS] ) + credentials_repository.save_stolen_credentials([LM_HASH_CREDENTIALS, PASSWORD_CREDENTIALS_2]) resp = flask_client.get(ALL_CREDENTIALS_URL) actual_propagation_credentials = [Credentials.from_mapping(creds) for creds in resp.json] assert resp.status_code == HTTPStatus.OK assert len(actual_propagation_credentials) == 4 - assert PROPAGATION_CREDENTIALS_1 in actual_propagation_credentials - assert PROPAGATION_CREDENTIALS_2 in actual_propagation_credentials - assert PROPAGATION_CREDENTIALS_3 in actual_propagation_credentials - assert PROPAGATION_CREDENTIALS_4 in actual_propagation_credentials + assert PASSWORD_CREDENTIALS_1 in actual_propagation_credentials + assert LM_HASH_CREDENTIALS in actual_propagation_credentials + assert NT_HASH_CREDENTIALS in actual_propagation_credentials + assert PASSWORD_CREDENTIALS_2 in actual_propagation_credentials def pre_populate_repository( @@ -72,7 +70,7 @@ def pre_populate_repository( @pytest.mark.parametrize("url", [CONFIGURED_CREDENTIALS_URL, STOLEN_CREDENTIALS_URL]) def test_propagation_credentials_endpoint__get_stolen(flask_client, credentials_repository, url): pre_populate_repository( - url, credentials_repository, [PROPAGATION_CREDENTIALS_1, PROPAGATION_CREDENTIALS_2] + url, credentials_repository, [PASSWORD_CREDENTIALS_1, LM_HASH_CREDENTIALS] ) resp = flask_client.get(url) @@ -80,19 +78,19 @@ def test_propagation_credentials_endpoint__get_stolen(flask_client, credentials_ assert resp.status_code == HTTPStatus.OK assert len(actual_propagation_credentials) == 2 - assert actual_propagation_credentials[0] == PROPAGATION_CREDENTIALS_1 - assert actual_propagation_credentials[1] == PROPAGATION_CREDENTIALS_2 + assert actual_propagation_credentials[0] == PASSWORD_CREDENTIALS_1 + assert actual_propagation_credentials[1] == LM_HASH_CREDENTIALS @pytest.mark.parametrize("url", [CONFIGURED_CREDENTIALS_URL, STOLEN_CREDENTIALS_URL]) def test_propagation_credentials_endpoint__post_stolen(flask_client, credentials_repository, url): - pre_populate_repository(url, credentials_repository, [PROPAGATION_CREDENTIALS_1]) + pre_populate_repository(url, credentials_repository, [PASSWORD_CREDENTIALS_1]) resp = flask_client.post( url, json=[ - Credentials.to_json(PROPAGATION_CREDENTIALS_2), - Credentials.to_json(PROPAGATION_CREDENTIALS_3), + Credentials.to_json(LM_HASH_CREDENTIALS), + Credentials.to_json(NT_HASH_CREDENTIALS), ], ) assert resp.status_code == HTTPStatus.NO_CONTENT @@ -102,15 +100,15 @@ def test_propagation_credentials_endpoint__post_stolen(flask_client, credentials assert resp.status_code == HTTPStatus.OK assert len(retrieved_propagation_credentials) == 3 - assert PROPAGATION_CREDENTIALS_1 in retrieved_propagation_credentials - assert PROPAGATION_CREDENTIALS_2 in retrieved_propagation_credentials - assert PROPAGATION_CREDENTIALS_3 in retrieved_propagation_credentials + assert PASSWORD_CREDENTIALS_1 in retrieved_propagation_credentials + assert LM_HASH_CREDENTIALS in retrieved_propagation_credentials + assert NT_HASH_CREDENTIALS in retrieved_propagation_credentials @pytest.mark.parametrize("url", [CONFIGURED_CREDENTIALS_URL, STOLEN_CREDENTIALS_URL]) def test_stolen_propagation_credentials_endpoint_delete(flask_client, credentials_repository, url): pre_populate_repository( - url, credentials_repository, [PROPAGATION_CREDENTIALS_1, PROPAGATION_CREDENTIALS_2] + url, credentials_repository, [PASSWORD_CREDENTIALS_1, LM_HASH_CREDENTIALS] ) resp = flask_client.delete(url) assert resp.status_code == HTTPStatus.NO_CONTENT @@ -136,8 +134,8 @@ def test_propagation_credentials_endpoint__post_not_found(flask_client): resp = flask_client.post( NON_EXISTENT_COLLECTION_URL, json=[ - Credentials.to_json(PROPAGATION_CREDENTIALS_2), - Credentials.to_json(PROPAGATION_CREDENTIALS_3), + Credentials.to_json(LM_HASH_CREDENTIALS), + Credentials.to_json(NT_HASH_CREDENTIALS), ], ) assert resp.status_code == HTTPStatus.NOT_FOUND From 424022d58a4fe9c242d0d7545a96c82dd17f89e7 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 09:12:21 -0400 Subject: [PATCH 07/31] UT: Remove unused constants from test_mongo_credentials_repository --- .../test_mongo_credentials_repository.py | 30 +------------------ 1 file changed, 1 insertion(+), 29 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 56453e4e2..56ba4e7f8 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 @@ -8,41 +8,13 @@ from pymongo.collection import Collection from pymongo.database import Database from tests.data_for_tests.propagation_credentials import PROPAGATION_CREDENTIALS -from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username +from common.credentials import Credentials from monkey_island.cc.repository import MongoCredentialsRepository from monkey_island.cc.server_utils.encryption import ILockableEncryptor -USER1 = "test_user_1" -USER2 = "test_user_2" -USER3 = "test_user_3" -PASSWORD = "12435" -PASSWORD2 = "password" -PASSWORD3 = "lozinka" -LM_HASH = "AEBD4DE384C7EC43AAD3B435B51404EE" -NT_HASH = "7A21990FCD3D759941E45C490F143D5F" -PUBLIC_KEY = "MY_PUBLIC_KEY" -PRIVATE_KEY = "MY_PRIVATE_KEY" - -IDENTITIES_1 = (Username(USER1), Username(USER2)) -SECRETS_1 = ( - Password(PASSWORD), - LMHash(LM_HASH), - NTHash(NT_HASH), - SSHKeypair(PRIVATE_KEY, PUBLIC_KEY), -) -CREDENTIALS_OBJECT_1 = Credentials(IDENTITIES_1, SECRETS_1) - -IDENTITIES_2 = (Username(USER3),) -SECRETS_2 = (Password(PASSWORD2), Password(PASSWORD3)) -CREDENTIALS_OBJECT_2 = Credentials(IDENTITIES_2, SECRETS_2) - - CONFIGURED_CREDENTIALS = PROPAGATION_CREDENTIALS[0:3] - STOLEN_CREDENTIALS = PROPAGATION_CREDENTIALS[3:6] -CREDENTIALS_LIST = [CREDENTIALS_OBJECT_1, CREDENTIALS_OBJECT_2] - def reverse(data: bytes) -> bytes: return bytes(reversed(data)) From febec2ecef98c2821796f88bf5c401ca5ad9cc58 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 09:30:05 -0400 Subject: [PATCH 08/31] UT: Use all caps for constants in propagation_credentials.py --- .../data_for_tests/propagation_credentials.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/monkey/tests/data_for_tests/propagation_credentials.py b/monkey/tests/data_for_tests/propagation_credentials.py index 15351da59..5c869e8c0 100644 --- a/monkey/tests/data_for_tests/propagation_credentials.py +++ b/monkey/tests/data_for_tests/propagation_credentials.py @@ -1,20 +1,20 @@ from common.credentials import Credentials, LMHash, NTHash, Password, SSHKeypair, Username -username = "m0nk3y_user" -special_username = "m0nk3y.user" -nt_hash = "C1C58F96CDF212B50837BC11A00BE47C" -lm_hash = "299BD128C1101FD6299BD128C1101FD6" -password_1 = "trytostealthis" -password_2 = "password" +USERNAME = "m0nk3y_user" +SPECIAL_USERNAME = "m0nk3y.user" +NT_HASH = "C1C58F96CDF212B50837BC11A00BE47C" +LM_HASH = "299BD128C1101FD6299BD128C1101FD6" +PASSWORD_1 = "trytostealthis" +PASSWORD_2 = "password!" PUBLIC_KEY = "MY_PUBLIC_KEY" PRIVATE_KEY = "MY_PRIVATE_KEY" -PASSWORD_CREDENTIALS_1 = Credentials(identity=Username(username), secret=Password(password_1)) -PASSWORD_CREDENTIALS_2 = Credentials(identity=Username(username), secret=Password(password_2)) -LM_HASH_CREDENTIALS = Credentials(identity=Username(special_username), secret=LMHash(lm_hash)) -NT_HASH_CREDENTIALS = Credentials(identity=Username(username), secret=NTHash(nt_hash)) +PASSWORD_CREDENTIALS_1 = Credentials(identity=Username(USERNAME), secret=Password(PASSWORD_1)) +PASSWORD_CREDENTIALS_2 = Credentials(identity=Username(USERNAME), secret=Password(PASSWORD_2)) +LM_HASH_CREDENTIALS = Credentials(identity=Username(SPECIAL_USERNAME), secret=LMHash(LM_HASH)) +NT_HASH_CREDENTIALS = Credentials(identity=Username(USERNAME), secret=NTHash(NT_HASH)) SSH_KEY_CREDENTIALS = Credentials( - identity=Username(username), secret=SSHKeypair(PRIVATE_KEY, PUBLIC_KEY) + identity=Username(USERNAME), secret=SSHKeypair(PRIVATE_KEY, PUBLIC_KEY) ) PROPAGATION_CREDENTIALS = [ From bd0425beb83043cfdc91bb3aabc9e3b0436cf2aa Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 09:31:25 -0400 Subject: [PATCH 09/31] UT: Add missing __init__.py to tests/data_for_tests/ --- monkey/tests/data_for_tests/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 monkey/tests/data_for_tests/__init__.py diff --git a/monkey/tests/data_for_tests/__init__.py b/monkey/tests/data_for_tests/__init__.py new file mode 100644 index 000000000..e69de29bb From 3f20b71d256c55943b53b1abf803d0cef95a1393 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 09:42:50 -0400 Subject: [PATCH 10/31] UT: Simplify Credentials tests --- .../common/credentials/test_credentials.py | 81 ++++++++++--------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/monkey/tests/unit_tests/common/credentials/test_credentials.py b/monkey/tests/unit_tests/common/credentials/test_credentials.py index cb5fb76fc..2c3718fee 100644 --- a/monkey/tests/unit_tests/common/credentials/test_credentials.py +++ b/monkey/tests/unit_tests/common/credentials/test_credentials.py @@ -2,6 +2,14 @@ import copy import json import pytest +from tests.data_for_tests.propagation_credentials import ( + LM_HASH, + NT_HASH, + PASSWORD_1, + PRIVATE_KEY, + PUBLIC_KEY, + USERNAME, +) from common.credentials import ( Credentials, @@ -14,28 +22,21 @@ from common.credentials import ( Username, ) -USER1 = "test_user_1" -PASSWORD = "12435" -LM_HASH = "AEBD4DE384C7EC43AAD3B435B51404EE" -NT_HASH = "7A21990FCD3D759941E45C490F143D5F" -PUBLIC_KEY = "MY_PUBLIC_KEY" -PRIVATE_KEY = "MY_PRIVATE_KEY" - -CREDENTIALS_DICT = { - "identity": {"credential_type": "USERNAME", "username": USER1}, +CREDENTIALS_DICT_TEMPLATE = { + "identity": {"credential_type": "USERNAME", "username": USERNAME}, "secret": {}, } -IDENTITY = Username(USER1) +IDENTITY = Username(USERNAME) SECRETS = ( - Password(PASSWORD), + Password(PASSWORD_1), LMHash(LM_HASH), NTHash(NT_HASH), SSHKeypair(PRIVATE_KEY, PUBLIC_KEY), ) SECRETS_DICTS = [ - {"credential_type": "PASSWORD", "password": PASSWORD}, + {"credential_type": "PASSWORD", "password": PASSWORD_1}, {"credential_type": "LM_HASH", "lm_hash": LM_HASH}, {"credential_type": "NT_HASH", "nt_hash": NT_HASH}, { @@ -45,46 +46,46 @@ SECRETS_DICTS = [ }, ] +CREDENTIALS_DICTS = [] +for secret in SECRETS_DICTS: + credentials_dict = copy.copy(CREDENTIALS_DICT_TEMPLATE) + credentials_dict["secret"] = secret + CREDENTIALS_DICTS.append(credentials_dict) -@pytest.mark.parametrize("secret, expected_secret", zip(SECRETS, SECRETS_DICTS)) -def test_credentials_serialization_json(secret, expected_secret): - expected_credentials = copy.copy(CREDENTIALS_DICT) - expected_credentials["secret"] = expected_secret - c = Credentials(IDENTITY, secret) - - serialized_credentials = Credentials.to_json(c) - - assert json.loads(serialized_credentials) == expected_credentials +CREDENTIALS = [Credentials(IDENTITY, secret) for secret in SECRETS] -@pytest.mark.parametrize("secret, expected_secret", zip(SECRETS, SECRETS_DICTS)) -def test_credentials_serialization_mapping(secret, expected_secret): - expected_credentials = copy.copy(CREDENTIALS_DICT) - expected_credentials["secret"] = expected_secret - c = Credentials(IDENTITY, secret) +@pytest.mark.parametrize( + "credentials, expected_credentials_dict", zip(CREDENTIALS, CREDENTIALS_DICTS) +) +def test_credentials_serialization_json(credentials, expected_credentials_dict): + serialized_credentials = Credentials.to_json(credentials) - serialized_credentials = Credentials.to_mapping(c) - - assert serialized_credentials == expected_credentials + assert json.loads(serialized_credentials) == expected_credentials_dict -@pytest.mark.parametrize("secret, secret_dict", zip(SECRETS, SECRETS_DICTS)) -def test_credentials_deserialization__from_mapping(secret, secret_dict): - expected_credentials = Credentials(IDENTITY, secret) - credentials_dict = copy.copy(CREDENTIALS_DICT) - credentials_dict["secret"] = secret_dict +@pytest.mark.parametrize( + "credentials, expected_credentials_dict", zip(CREDENTIALS, CREDENTIALS_DICTS) +) +def test_credentials_serialization_mapping(credentials, expected_credentials_dict): + serialized_credentials = Credentials.to_mapping(credentials) + assert serialized_credentials == expected_credentials_dict + + +@pytest.mark.parametrize( + "expected_credentials, credentials_dict", zip(CREDENTIALS, CREDENTIALS_DICTS) +) +def test_credentials_deserialization__from_mapping(expected_credentials, credentials_dict): deserialized_credentials = Credentials.from_mapping(credentials_dict) assert deserialized_credentials == expected_credentials -@pytest.mark.parametrize("secret, secret_dict", zip(SECRETS, SECRETS_DICTS)) -def test_credentials_deserialization__from_json(secret, secret_dict): - expected_credentials = Credentials(IDENTITY, secret) - credentials_dict = copy.copy(CREDENTIALS_DICT) - credentials_dict["secret"] = secret_dict - +@pytest.mark.parametrize( + "expected_credentials, credentials_dict", zip(CREDENTIALS, CREDENTIALS_DICTS) +) +def test_credentials_deserialization__from_json(expected_credentials, credentials_dict): deserialized_credentials = Credentials.from_json(json.dumps(credentials_dict)) assert deserialized_credentials == expected_credentials From fb11c292086f7791f0bfe2da2120ca7fef3ab02c Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 10:05:17 -0400 Subject: [PATCH 11/31] UT: Use nested comprehension in get_all_collections_in_mongo() --- .../test_mongo_credentials_repository.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 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 56ba4e7f8..460f20da9 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,4 +1,4 @@ -from typing import Any, Iterable, List, Mapping, Sequence +from typing import Any, Iterable, Mapping, Sequence from unittest.mock import MagicMock import mongomock @@ -22,6 +22,7 @@ def reverse(data: bytes) -> bytes: @pytest.fixture def repository_encryptor(): + # NOTE: Tests will fail if any inputs to this mock encryptor are palindromes. repository_encryptor = MagicMock(spec=ILockableEncryptor) repository_encryptor.encrypt = MagicMock(side_effect=reverse) repository_encryptor.decrypt = MagicMock(side_effect=reverse) @@ -136,12 +137,13 @@ def get_all_credentials_in_mongo( def get_all_collections_in_mongo(mongo_client: MongoClient) -> Iterable[Collection]: - collections: List[Collection] = [] - - databases = get_all_databases_in_mongo(mongo_client) - for db in databases: - collections.extend(get_all_collections_in_database(db)) + collections = [ + collection + for db in get_all_databases_in_mongo(mongo_client) + for collection in get_all_collections_in_database(db) + ] + assert len(collections) > 0 return collections From 63731b83341b0fc24d7d7eb9e39c90b1695f7526 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 10:26:56 -0400 Subject: [PATCH 12/31] UT: Test identity/password combos in test_credentials.py --- .../common/credentials/test_credentials.py | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/monkey/tests/unit_tests/common/credentials/test_credentials.py b/monkey/tests/unit_tests/common/credentials/test_credentials.py index 2c3718fee..fe3e919ae 100644 --- a/monkey/tests/unit_tests/common/credentials/test_credentials.py +++ b/monkey/tests/unit_tests/common/credentials/test_credentials.py @@ -1,5 +1,5 @@ -import copy import json +from itertools import product import pytest from tests.data_for_tests.propagation_credentials import ( @@ -22,20 +22,16 @@ from common.credentials import ( Username, ) -CREDENTIALS_DICT_TEMPLATE = { - "identity": {"credential_type": "USERNAME", "username": USERNAME}, - "secret": {}, -} +IDENTITIES = [Username(USERNAME)] +IDENTITY_DICTS = [{"credential_type": "USERNAME", "username": USERNAME}] -IDENTITY = Username(USERNAME) SECRETS = ( Password(PASSWORD_1), LMHash(LM_HASH), NTHash(NT_HASH), SSHKeypair(PRIVATE_KEY, PUBLIC_KEY), ) - -SECRETS_DICTS = [ +SECRET_DICTS = [ {"credential_type": "PASSWORD", "password": PASSWORD_1}, {"credential_type": "LM_HASH", "lm_hash": LM_HASH}, {"credential_type": "NT_HASH", "nt_hash": NT_HASH}, @@ -46,13 +42,12 @@ SECRETS_DICTS = [ }, ] -CREDENTIALS_DICTS = [] -for secret in SECRETS_DICTS: - credentials_dict = copy.copy(CREDENTIALS_DICT_TEMPLATE) - credentials_dict["secret"] = secret - CREDENTIALS_DICTS.append(credentials_dict) +CREDENTIALS = [Credentials(identity, secret) for identity, secret in product(IDENTITIES, SECRETS)] -CREDENTIALS = [Credentials(IDENTITY, secret) for secret in SECRETS] +CREDENTIALS_DICTS = [ + {"identity": identity, "secret": secret} + for identity, secret in product(IDENTITY_DICTS, SECRET_DICTS) +] @pytest.mark.parametrize( @@ -92,14 +87,14 @@ def test_credentials_deserialization__from_json(expected_credentials, credential def test_credentials_deserialization__invalid_credentials(): - invalid_data = {"secret": SECRETS_DICTS[0], "unknown_key": []} + invalid_data = {"secret": SECRET_DICTS[0], "unknown_key": []} with pytest.raises(InvalidCredentialsError): Credentials.from_mapping(invalid_data) def test_credentials_deserialization__invalid_component_type(): invalid_data = { - "secret": SECRETS_DICTS[0], + "secret": SECRET_DICTS[0], "identity": {"credential_type": "FAKE", "username": "user1"}, } with pytest.raises(InvalidCredentialsError): @@ -108,7 +103,7 @@ def test_credentials_deserialization__invalid_component_type(): def test_credentials_deserialization__invalid_component(): invalid_data = { - "secret": SECRETS_DICTS[0], + "secret": SECRET_DICTS[0], "identity": {"credential_type": "USERNAME", "unknown_field": "user1"}, } with pytest.raises(InvalidCredentialComponentError): From e3b23993faffced13de3369349cb9572b89b60ae Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 10:43:03 -0400 Subject: [PATCH 13/31] Common: Add type hints to dicts in credentials.py --- monkey/common/credentials/credentials.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index 5483e9ff8..f1c08beb1 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -1,7 +1,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Any, Mapping, MutableMapping, Sequence +from typing import Any, Mapping, MutableMapping, Sequence, Type from marshmallow import Schema, fields, post_load, pre_dump from marshmallow.exceptions import MarshmallowError @@ -24,7 +24,7 @@ from .password import PasswordSchema from .ssh_keypair import SSHKeypairSchema from .username import UsernameSchema -CREDENTIAL_COMPONENT_TYPE_TO_CLASS = { +CREDENTIAL_COMPONENT_TYPE_TO_CLASS: Mapping[CredentialComponentType, Type[ICredentialComponent]] = { CredentialComponentType.LM_HASH: LMHash, CredentialComponentType.NT_HASH: NTHash, CredentialComponentType.PASSWORD: Password, @@ -32,7 +32,7 @@ CREDENTIAL_COMPONENT_TYPE_TO_CLASS = { CredentialComponentType.USERNAME: Username, } -CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA = { +CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA: Mapping[CredentialComponentType, Schema] = { CredentialComponentType.LM_HASH: LMHashSchema(), CredentialComponentType.NT_HASH: NTHashSchema(), CredentialComponentType.PASSWORD: PasswordSchema(), From 2af713dabdfeda04606c48a37134a93ae2630cab Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 10:50:25 -0400 Subject: [PATCH 14/31] Common: Allow identities or secrets to be None It's possible that credentials are stolen and an identity/secret association can not be made. For example, a list of usernames can be acquired by `ls /home`, but no passwords will be retrieved this way. Credentials(identity=Username("username"), secret=None) will represent this case. --- monkey/common/credentials/credentials.py | 32 +++++++++++++------ .../common/credentials/test_credentials.py | 13 ++++++-- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index f1c08beb1..676e77275 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -1,7 +1,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Any, Mapping, MutableMapping, Sequence, Type +from typing import Any, Mapping, MutableMapping, Optional, Sequence, Type from marshmallow import Schema, fields, post_load, pre_dump from marshmallow.exceptions import MarshmallowError @@ -42,8 +42,8 @@ CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA: Mapping[CredentialComponentType, Sche class CredentialsSchema(Schema): - identity = fields.Mapping() - secret = fields.Mapping() + identity = fields.Mapping(allow_none=True) + secret = fields.Mapping(allow_none=True) @post_load def _make_credentials( @@ -55,9 +55,16 @@ class CredentialsSchema(Schema): return data @staticmethod - def _build_credential_component(data: Mapping[str, Any]) -> ICredentialComponent: + def _build_credential_component( + credential_component: Optional[Mapping[str, Any]] + ) -> Optional[ICredentialComponent]: + if credential_component is None: + return None + try: - credential_component_type = CredentialComponentType[data["credential_type"]] + credential_component_type = CredentialComponentType[ + credential_component["credential_type"] + ] except KeyError as err: raise InvalidCredentialsError(f"Unknown credential component type {err}") @@ -67,7 +74,9 @@ class CredentialsSchema(Schema): ] try: - return credential_component_class(**credential_component_schema.load(data)) + return credential_component_class( + **credential_component_schema.load(credential_component) + ) except MarshmallowError as err: raise InvalidCredentialComponentError(credential_component_class, str(err)) @@ -84,8 +93,11 @@ class CredentialsSchema(Schema): @staticmethod def _serialize_credential_component( - credential_component: ICredentialComponent, - ) -> Mapping[str, Any]: + credential_component: Optional[ICredentialComponent], + ) -> Optional[Mapping[str, Any]]: + if credential_component is None: + return None + credential_component_schema = CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA[ credential_component.credential_type ] @@ -95,8 +107,8 @@ class CredentialsSchema(Schema): @dataclass(frozen=True) class Credentials(IJSONSerializable): - identity: ICredentialComponent - secret: ICredentialComponent + identity: Optional[ICredentialComponent] + secret: Optional[ICredentialComponent] @staticmethod def from_mapping(credentials: Mapping) -> Credentials: diff --git a/monkey/tests/unit_tests/common/credentials/test_credentials.py b/monkey/tests/unit_tests/common/credentials/test_credentials.py index fe3e919ae..d26cde8fa 100644 --- a/monkey/tests/unit_tests/common/credentials/test_credentials.py +++ b/monkey/tests/unit_tests/common/credentials/test_credentials.py @@ -22,14 +22,15 @@ from common.credentials import ( Username, ) -IDENTITIES = [Username(USERNAME)] -IDENTITY_DICTS = [{"credential_type": "USERNAME", "username": USERNAME}] +IDENTITIES = [Username(USERNAME), None] +IDENTITY_DICTS = [{"credential_type": "USERNAME", "username": USERNAME}, None] SECRETS = ( Password(PASSWORD_1), LMHash(LM_HASH), NTHash(NT_HASH), SSHKeypair(PRIVATE_KEY, PUBLIC_KEY), + None, ) SECRET_DICTS = [ {"credential_type": "PASSWORD", "password": PASSWORD_1}, @@ -40,13 +41,19 @@ SECRET_DICTS = [ "public_key": PUBLIC_KEY, "private_key": PRIVATE_KEY, }, + None, ] -CREDENTIALS = [Credentials(identity, secret) for identity, secret in product(IDENTITIES, SECRETS)] +CREDENTIALS = [ + Credentials(identity, secret) + for identity, secret in product(IDENTITIES, SECRETS) + if not (identity is None and secret is None) +] CREDENTIALS_DICTS = [ {"identity": identity, "secret": secret} for identity, secret in product(IDENTITY_DICTS, SECRET_DICTS) + if not (identity is None and secret is None) ] From 19a720898ec0cdefae6f3463921aa0ff22d27a8b Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 11:33:31 -0400 Subject: [PATCH 15/31] Island: Handle encryption/decryption of None credential components --- .../mongo_credentials_repository.py | 22 ++++++++++++------- .../data_for_tests/propagation_credentials.py | 5 +++++ .../test_mongo_credentials_repository.py | 12 +++++----- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py index aeaa5b884..5d58d510a 100644 --- a/monkey/monkey_island/cc/repository/mongo_credentials_repository.py +++ b/monkey/monkey_island/cc/repository/mongo_credentials_repository.py @@ -77,10 +77,13 @@ class MongoCredentialsRepository(ICredentialsRepository): encrypted_mapping: Dict[str, Any] = {} for secret_or_identity, credentials_component in mapping.items(): - encrypted_component = { - key: self._repository_encryptor.encrypt(value.encode()) - for key, value in credentials_component.items() - } + if credentials_component is None: + encrypted_component = None + else: + encrypted_component = { + key: self._repository_encryptor.encrypt(value.encode()) + for key, value in credentials_component.items() + } encrypted_mapping[secret_or_identity] = encrypted_component @@ -90,10 +93,13 @@ class MongoCredentialsRepository(ICredentialsRepository): decrypted_mapping: Dict[str, Any] = {} for secret_or_identity, credentials_component in mapping.items(): - decrypted_component = { - key: self._repository_encryptor.decrypt(value).decode() - for key, value in credentials_component.items() - } + if credentials_component is None: + decrypted_component = None + else: + decrypted_component = { + key: self._repository_encryptor.decrypt(value).decode() + for key, value in credentials_component.items() + } decrypted_mapping[secret_or_identity] = decrypted_component diff --git a/monkey/tests/data_for_tests/propagation_credentials.py b/monkey/tests/data_for_tests/propagation_credentials.py index 5c869e8c0..6efe9b7af 100644 --- a/monkey/tests/data_for_tests/propagation_credentials.py +++ b/monkey/tests/data_for_tests/propagation_credentials.py @@ -6,6 +6,7 @@ NT_HASH = "C1C58F96CDF212B50837BC11A00BE47C" LM_HASH = "299BD128C1101FD6299BD128C1101FD6" PASSWORD_1 = "trytostealthis" PASSWORD_2 = "password!" +PASSWORD_3 = "rubberbabybuggybumpers" PUBLIC_KEY = "MY_PUBLIC_KEY" PRIVATE_KEY = "MY_PRIVATE_KEY" @@ -16,6 +17,8 @@ NT_HASH_CREDENTIALS = Credentials(identity=Username(USERNAME), secret=NTHash(NT_ SSH_KEY_CREDENTIALS = Credentials( identity=Username(USERNAME), secret=SSHKeypair(PRIVATE_KEY, PUBLIC_KEY) ) +EMPTY_SECRET_CREDENTIALS = Credentials(identity=Username(USERNAME), secret=None) +EMPTY_IDENTITY_CREDENTIALS = Credentials(identity=None, secret=Password(PASSWORD_3)) PROPAGATION_CREDENTIALS = [ PASSWORD_CREDENTIALS_1, @@ -23,4 +26,6 @@ PROPAGATION_CREDENTIALS = [ NT_HASH_CREDENTIALS, PASSWORD_CREDENTIALS_2, SSH_KEY_CREDENTIALS, + EMPTY_SECRET_CREDENTIALS, + EMPTY_IDENTITY_CREDENTIALS, ] 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 460f20da9..010f26b8f 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 @@ -13,7 +13,7 @@ from monkey_island.cc.repository import MongoCredentialsRepository from monkey_island.cc.server_utils.encryption import ILockableEncryptor CONFIGURED_CREDENTIALS = PROPAGATION_CREDENTIALS[0:3] -STOLEN_CREDENTIALS = PROPAGATION_CREDENTIALS[3:6] +STOLEN_CREDENTIALS = PROPAGATION_CREDENTIALS[3:] def reverse(data: bytes) -> bytes: @@ -91,9 +91,6 @@ def test_mongo_repository_all(mongo_repository): 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. @pytest.mark.parametrize("credentials", PROPAGATION_CREDENTIALS) def test_configured_secrets_encrypted( mongo_repository: MongoCredentialsRepository, @@ -116,8 +113,11 @@ def check_if_stored_credentials_encrypted(mongo_client: MongoClient, original_cr for rc in raw_credentials: for identity_or_secret, credentials_component in rc.items(): - for key, value in credentials_component.items(): - assert original_credentials_mapping[identity_or_secret][key] != value.decode() + if original_credentials_mapping[identity_or_secret] is None: + assert credentials_component is None + else: + for key, value in credentials_component.items(): + assert original_credentials_mapping[identity_or_secret][key] != value.decode() def get_all_credentials_in_mongo( From 62ce91b59b0eb83d0cea4693e4adf935392c1473 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 12:19:12 -0400 Subject: [PATCH 16/31] Common: Prevent invalid Credentials objects from being constructed --- monkey/common/credentials/credentials.py | 14 ++++++++++++++ .../common/credentials/test_credentials.py | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index 676e77275..c65ffab55 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -49,6 +49,9 @@ class CredentialsSchema(Schema): def _make_credentials( self, data: MutableMapping, **kwargs: Mapping[str, Any] ) -> Mapping[str, Sequence[Mapping[str, Any]]]: + if not any(data.values()): + raise InvalidCredentialsError("At least one credentials component must be defined") + data["identity"] = CredentialsSchema._build_credential_component(data["identity"]) data["secret"] = CredentialsSchema._build_credential_component(data["secret"]) @@ -110,6 +113,17 @@ class Credentials(IJSONSerializable): identity: Optional[ICredentialComponent] secret: Optional[ICredentialComponent] + def __post_init__(self): + schema = CredentialsSchema() + try: + serialized_data = schema.dump(self) + + # This will raise an exception if the object is invalid. Calling this in __post__init() + # makes it impossible to construct an invalid object + schema.load(serialized_data) + except Exception as err: + raise InvalidCredentialsError(err) + @staticmethod def from_mapping(credentials: Mapping) -> Credentials: """ diff --git a/monkey/tests/unit_tests/common/credentials/test_credentials.py b/monkey/tests/unit_tests/common/credentials/test_credentials.py index d26cde8fa..4eeb8647f 100644 --- a/monkey/tests/unit_tests/common/credentials/test_credentials.py +++ b/monkey/tests/unit_tests/common/credentials/test_credentials.py @@ -115,3 +115,8 @@ def test_credentials_deserialization__invalid_component(): } with pytest.raises(InvalidCredentialComponentError): Credentials.from_mapping(invalid_data) + + +def test_create_credentials__none_none(): + with pytest.raises(InvalidCredentialsError): + Credentials(None, None) From 213b161d1af982a73d78144deeb4f590094a51d5 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Fri, 15 Jul 2022 12:34:42 -0400 Subject: [PATCH 17/31] Common: Fix type hints in credentials.py --- monkey/common/credentials/credentials.py | 33 ++++++++++++++---------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index c65ffab55..b879816a8 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -1,7 +1,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Any, Mapping, MutableMapping, Optional, Sequence, Type +from typing import Any, Mapping, Optional, Type from marshmallow import Schema, fields, post_load, pre_dump from marshmallow.exceptions import MarshmallowError @@ -40,6 +40,9 @@ CREDENTIAL_COMPONENT_TYPE_TO_CLASS_SCHEMA: Mapping[CredentialComponentType, Sche CredentialComponentType.USERNAME: UsernameSchema(), } +CredentialComponentMapping = Optional[Mapping[str, Any]] +CredentialsMapping = Mapping[str, CredentialComponentMapping] + class CredentialsSchema(Schema): identity = fields.Mapping(allow_none=True) @@ -47,19 +50,23 @@ class CredentialsSchema(Schema): @post_load def _make_credentials( - self, data: MutableMapping, **kwargs: Mapping[str, Any] - ) -> Mapping[str, Sequence[Mapping[str, Any]]]: - if not any(data.values()): + self, + credentials: CredentialsMapping, + **kwargs: Mapping[str, Any], + ) -> Mapping[str, Optional[ICredentialComponent]]: + if not any(credentials.values()): raise InvalidCredentialsError("At least one credentials component must be defined") - data["identity"] = CredentialsSchema._build_credential_component(data["identity"]) - data["secret"] = CredentialsSchema._build_credential_component(data["secret"]) + parsed_credentials = { + key: CredentialsSchema._build_credential_component(credential_component_mapping) + for key, credential_component_mapping in credentials.items() + } - return data + return parsed_credentials @staticmethod def _build_credential_component( - credential_component: Optional[Mapping[str, Any]] + credential_component: CredentialComponentMapping, ) -> Optional[ICredentialComponent]: if credential_component is None: return None @@ -84,9 +91,7 @@ class CredentialsSchema(Schema): raise InvalidCredentialComponentError(credential_component_class, str(err)) @pre_dump - def _serialize_credentials( - self, credentials: Credentials, **kwargs - ) -> Mapping[str, Sequence[Mapping[str, Any]]]: + def _serialize_credentials(self, credentials: Credentials, **kwargs) -> CredentialsMapping: data = {} data["identity"] = CredentialsSchema._serialize_credential_component(credentials.identity) @@ -97,7 +102,7 @@ class CredentialsSchema(Schema): @staticmethod def _serialize_credential_component( credential_component: Optional[ICredentialComponent], - ) -> Optional[Mapping[str, Any]]: + ) -> CredentialComponentMapping: if credential_component is None: return None @@ -125,7 +130,7 @@ class Credentials(IJSONSerializable): raise InvalidCredentialsError(err) @staticmethod - def from_mapping(credentials: Mapping) -> Credentials: + def from_mapping(credentials: CredentialsMapping) -> Credentials: """ Construct a Credentials object from a Mapping @@ -167,7 +172,7 @@ class Credentials(IJSONSerializable): raise InvalidCredentialsError(str(err)) @staticmethod - def to_mapping(credentials: Credentials) -> Mapping: + def to_mapping(credentials: Credentials) -> CredentialsMapping: """ Serialize a Credentials object to a Mapping From f421f42604e0e2eca9d93a68d530f58d5d1942af Mon Sep 17 00:00:00 2001 From: Ilija Lazoroski Date: Mon, 18 Jul 2022 11:28:15 +0200 Subject: [PATCH 18/31] Agent: Simplify credentials in ssh credentials collector --- .../ssh_collector/ssh_credential_collector.py | 16 +++++++--------- .../test_ssh_credentials_collector.py | 6 +++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py b/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py index a5f15a637..5c5b05467 100644 --- a/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py @@ -29,11 +29,11 @@ class SSHCredentialCollector(ICredentialCollector): ssh_credentials = [] for info in ssh_info: - identities = [] - secrets = [] + identity = None + secret = None if info.get("name", ""): - identities.append(Username(info["name"])) + identity = Username(info["name"]) ssh_keypair = {} for key in ["public_key", "private_key"]: @@ -41,13 +41,11 @@ class SSHCredentialCollector(ICredentialCollector): ssh_keypair[key] = info[key] if len(ssh_keypair): - secrets.append( - SSHKeypair( - ssh_keypair.get("private_key", ""), ssh_keypair.get("public_key", "") - ) + secret = SSHKeypair( + ssh_keypair.get("private_key", ""), ssh_keypair.get("public_key", "") ) - if identities != [] or secrets != []: - ssh_credentials.append(Credentials(identities, secrets)) + if identity is not None: + ssh_credentials.append(Credentials(identity, secret)) return ssh_credentials diff --git a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py index 17ca7ddc7..c092ed1fb 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py @@ -55,9 +55,9 @@ def test_ssh_info_result_parsing(monkeypatch, patch_telemetry_messenger): ssh_keypair2 = SSHKeypair("", "AnotherPublicKey") expected = [ - Credentials(identities=[username], secrets=[ssh_keypair1]), - Credentials(identities=[username2], secrets=[ssh_keypair2]), - Credentials(identities=[username3], secrets=[]), + Credentials(identity=username, secret=ssh_keypair1), + Credentials(identity=username2, secret=ssh_keypair2), + Credentials(identity=username3, secret=None), ] collected = SSHCredentialCollector(patch_telemetry_messenger).collect_credentials() assert expected == collected From 0f2fc0902fcd0cc12c1a69d188982fb278be8db4 Mon Sep 17 00:00:00 2001 From: Ilija Lazoroski Date: Mon, 18 Jul 2022 11:29:52 +0200 Subject: [PATCH 19/31] Agent: Simplify credentials object in aggregating credentials store --- .../aggregating_credentials_store.py | 46 +++++++++---------- .../test_aggregating_credentials_store.py | 21 ++++----- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py index 6ff6bb288..dacfbdcee 100644 --- a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py +++ b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py @@ -26,31 +26,29 @@ class AggregatingCredentialsStore(ICredentialsStore): def add_credentials(self, credentials_to_add: Iterable[Credentials]): for credentials in credentials_to_add: - usernames = { - identity.username - for identity in credentials.identities - if identity.credential_type is CredentialComponentType.USERNAME - } - self._stored_credentials.setdefault("exploit_user_list", set()).update(usernames) + self._stored_credentials.setdefault("exploit_user_list", set()).add( + credentials.identity.username + ) - for secret in credentials.secrets: - if secret.credential_type is CredentialComponentType.PASSWORD: - self._stored_credentials.setdefault("exploit_password_list", set()).add( - secret.password - ) - elif secret.credential_type is CredentialComponentType.LM_HASH: - self._stored_credentials.setdefault("exploit_lm_hash_list", set()).add( - secret.lm_hash - ) - elif secret.credential_type is CredentialComponentType.NT_HASH: - self._stored_credentials.setdefault("exploit_ntlm_hash_list", set()).add( - secret.nt_hash - ) - elif secret.credential_type is CredentialComponentType.SSH_KEYPAIR: - self._set_attribute( - "exploit_ssh_keys", - [{"public_key": secret.public_key, "private_key": secret.private_key}], - ) + secret = credentials.secret + + if secret.credential_type is CredentialComponentType.PASSWORD: + self._stored_credentials.setdefault("exploit_password_list", set()).add( + secret.password + ) + elif secret.credential_type is CredentialComponentType.LM_HASH: + self._stored_credentials.setdefault("exploit_lm_hash_list", set()).add( + secret.lm_hash + ) + elif secret.credential_type is CredentialComponentType.NT_HASH: + self._stored_credentials.setdefault("exploit_ntlm_hash_list", set()).add( + secret.nt_hash + ) + elif secret.credential_type is CredentialComponentType.SSH_KEYPAIR: + self._set_attribute( + "exploit_ssh_keys", + [{"public_key": secret.public_key, "private_key": secret.private_key}], + ) def get_credentials(self) -> PropagationCredentials: try: diff --git a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py index baff7df65..54d43f59c 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py @@ -30,21 +30,20 @@ EMPTY_CHANNEL_CREDENTIALS = { TEST_CREDENTIALS = [ Credentials( - [Username("user1"), Username("user3")], - [ - Password("abcdefg"), - Password("root"), - SSHKeypair(public_key="some_public_key_1", private_key="some_private_key_1"), - ], - ) + identity=Username("user1"), + secret=Password("root"), + ), + Credentials(identity=Username("user1"), secret=Password("abcdefg")), + Credentials( + identity=Username("user3"), + secret=SSHKeypair(public_key="some_public_key_1", private_key="some_private_key_1"), + ), ] SSH_KEYS_CREDENTIALS = [ Credentials( - [Username("root")], - [ - SSHKeypair(public_key="some_public_key", private_key="some_private_key"), - ], + Username("root"), + SSHKeypair(public_key="some_public_key", private_key="some_private_key"), ) ] From 2cb6c6086669d0b93a79af94a161fd4e951719bf Mon Sep 17 00:00:00 2001 From: Ilija Lazoroski Date: Mon, 18 Jul 2022 11:47:49 +0200 Subject: [PATCH 20/31] UT: Fix credentials intercepting telemetry messenger tests --- ...st_credentials_intercepting_telemetry_messenger.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/monkey/tests/unit_tests/infection_monkey/telemetry/messengers/test_credentials_intercepting_telemetry_messenger.py b/monkey/tests/unit_tests/infection_monkey/telemetry/messengers/test_credentials_intercepting_telemetry_messenger.py index e516baab8..eda8954d7 100644 --- a/monkey/tests/unit_tests/infection_monkey/telemetry/messengers/test_credentials_intercepting_telemetry_messenger.py +++ b/monkey/tests/unit_tests/infection_monkey/telemetry/messengers/test_credentials_intercepting_telemetry_messenger.py @@ -8,13 +8,10 @@ from infection_monkey.telemetry.messengers.credentials_intercepting_telemetry_me TELEM_CREDENTIALS = [ Credentials( - [Username("user1"), Username("user3")], - [ - Password("abcdefg"), - Password("root"), - SSHKeypair(public_key="some_public_key", private_key="some_private_key"), - ], - ) + Username("user1"), + SSHKeypair(public_key="some_public_key", private_key="some_private_key"), + ), + Credentials(Username("root"), Password("password")), ] From 575fff0cdbe0eccb0c375eaf5ef5147b5803306e Mon Sep 17 00:00:00 2001 From: Ilija Lazoroski Date: Mon, 18 Jul 2022 11:49:44 +0200 Subject: [PATCH 21/31] Agent: Simplify credentials object in MimikatzCredentialCollector --- .../mimikatz_credential_collector.py | 14 +++++--------- .../test_mimikatz_collector.py | 14 +++++++------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py index 3404479a4..bc56efea2 100644 --- a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py @@ -22,28 +22,24 @@ class MimikatzCredentialCollector(ICredentialCollector): def _to_credentials(win_creds: Sequence[WindowsCredentials]) -> [Credentials]: all_creds = [] for win_cred in win_creds: - identities = [] - secrets = [] + identity = None # Mimikatz picks up users created by the Monkey even if they're successfully deleted # since it picks up creds from the registry. The newly created users are not removed # from the registry until a reboot of the system, hence this check. if win_cred.username and not win_cred.username.startswith(USERNAME_PREFIX): identity = Username(win_cred.username) - identities.append(identity) if win_cred.password: password = Password(win_cred.password) - secrets.append(password) + all_creds.append(Credentials(identity, password)) if win_cred.lm_hash: lm_hash = LMHash(lm_hash=win_cred.lm_hash) - secrets.append(lm_hash) + all_creds.append(Credentials(identity, lm_hash)) if win_cred.ntlm_hash: - lm_hash = NTHash(nt_hash=win_cred.ntlm_hash) - secrets.append(lm_hash) + ntlm_hash = NTHash(nt_hash=win_cred.ntlm_hash) + all_creds.append(Credentials(identity, ntlm_hash)) - if identities != [] or secrets != []: - all_creds.append(Credentials(identities, secrets)) return all_creds diff --git a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py index 62142f6e9..47a6ead49 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py @@ -36,7 +36,7 @@ def test_pypykatz_result_parsing(monkeypatch): username = Username("user") password = Password("secret") - expected_credentials = Credentials([username], [password]) + expected_credentials = Credentials(username, password) collected_credentials = collect_credentials() assert len(collected_credentials) == 1 @@ -66,11 +66,11 @@ def test_pypykatz_result_parsing_defaults(monkeypatch): username = Username("user2") password = Password("secret2") lm_hash = LMHash("0182BD0BD4444BF8FC83B5D9042EED2E") - expected_credentials = Credentials([username], [password, lm_hash]) + expected_credentials = [Credentials(username, password), Credentials(username, lm_hash)] collected_credentials = collect_credentials() - assert len(collected_credentials) == 1 - assert collected_credentials[0] == expected_credentials + assert len(collected_credentials) == 2 + assert collected_credentials == expected_credentials def test_pypykatz_result_parsing_no_identities(monkeypatch): @@ -86,8 +86,8 @@ def test_pypykatz_result_parsing_no_identities(monkeypatch): lm_hash = LMHash("0182BD0BD4444BF8FC83B5D9042EED2E") nt_hash = NTHash("E9F85516721DDC218359AD5280DB4450") - expected_credentials = Credentials([], [lm_hash, nt_hash]) + expected_credentials = [Credentials(None, lm_hash), Credentials(None, nt_hash)] collected_credentials = collect_credentials() - assert len(collected_credentials) == 1 - assert collected_credentials[0] == expected_credentials + assert len(collected_credentials) == 2 + assert collected_credentials == expected_credentials From 9e7963afc07ad5fa961cc5ce12dd87a14cd8c91d Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:13:09 -0400 Subject: [PATCH 22/31] Common: Simplify _serialize_credentials() --- monkey/common/credentials/credentials.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index b879816a8..64dbf7bcc 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -92,12 +92,10 @@ class CredentialsSchema(Schema): @pre_dump def _serialize_credentials(self, credentials: Credentials, **kwargs) -> CredentialsMapping: - data = {} - - data["identity"] = CredentialsSchema._serialize_credential_component(credentials.identity) - data["secret"] = CredentialsSchema._serialize_credential_component(credentials.secret) - - return data + return { + "identity": CredentialsSchema._serialize_credential_component(credentials.identity), + "secret": CredentialsSchema._serialize_credential_component(credentials.secret), + } @staticmethod def _serialize_credential_component( From acf12c2de1e3a67d6d8d12bd81502134eb236adf Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:14:01 -0400 Subject: [PATCH 23/31] Common: Simplify _make_credentials() --- monkey/common/credentials/credentials.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/monkey/common/credentials/credentials.py b/monkey/common/credentials/credentials.py index 64dbf7bcc..93390f212 100644 --- a/monkey/common/credentials/credentials.py +++ b/monkey/common/credentials/credentials.py @@ -57,13 +57,11 @@ class CredentialsSchema(Schema): if not any(credentials.values()): raise InvalidCredentialsError("At least one credentials component must be defined") - parsed_credentials = { + return { key: CredentialsSchema._build_credential_component(credential_component_mapping) for key, credential_component_mapping in credentials.items() } - return parsed_credentials - @staticmethod def _build_credential_component( credential_component: CredentialComponentMapping, From 302803b779f2396075348126d5ed87ebaa44dbd8 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:26:49 -0400 Subject: [PATCH 24/31] Agent: Improve variable names in MimikatzCredentialCollector --- .../mimikatz_credential_collector.py | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py index bc56efea2..b0299a0c8 100644 --- a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py @@ -14,32 +14,32 @@ logger = logging.getLogger(__name__) class MimikatzCredentialCollector(ICredentialCollector): def collect_credentials(self, options=None) -> Sequence[Credentials]: logger.info("Attempting to collect windows credentials with pypykatz.") - creds = pypykatz_handler.get_windows_creds() - logger.info(f"Pypykatz gathered {len(creds)} credentials.") - return MimikatzCredentialCollector._to_credentials(creds) + windows_credentials = pypykatz_handler.get_windows_creds() + logger.info(f"Pypykatz gathered {len(windows_credentials)} credentials.") + return MimikatzCredentialCollector._to_credentials(windows_credentials) @staticmethod - def _to_credentials(win_creds: Sequence[WindowsCredentials]) -> [Credentials]: - all_creds = [] - for win_cred in win_creds: + def _to_credentials(windows_credentials: Sequence[WindowsCredentials]) -> [Credentials]: + credentials = [] + for wc in windows_credentials: identity = None # Mimikatz picks up users created by the Monkey even if they're successfully deleted # since it picks up creds from the registry. The newly created users are not removed # from the registry until a reboot of the system, hence this check. - if win_cred.username and not win_cred.username.startswith(USERNAME_PREFIX): - identity = Username(win_cred.username) + if wc.username and not wc.username.startswith(USERNAME_PREFIX): + identity = Username(wc.username) - if win_cred.password: - password = Password(win_cred.password) - all_creds.append(Credentials(identity, password)) + if wc.password: + password = Password(wc.password) + credentials.append(Credentials(identity, password)) - if win_cred.lm_hash: - lm_hash = LMHash(lm_hash=win_cred.lm_hash) - all_creds.append(Credentials(identity, lm_hash)) + if wc.lm_hash: + lm_hash = LMHash(lm_hash=wc.lm_hash) + credentials.append(Credentials(identity, lm_hash)) - if win_cred.ntlm_hash: - ntlm_hash = NTHash(nt_hash=win_cred.ntlm_hash) - all_creds.append(Credentials(identity, ntlm_hash)) + if wc.ntlm_hash: + ntlm_hash = NTHash(nt_hash=wc.ntlm_hash) + credentials.append(Credentials(identity, ntlm_hash)) - return all_creds + return credentials From cb9f43d24288f1a30e11795cd9f62aaa9b982c95 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:27:32 -0400 Subject: [PATCH 25/31] Agent: Fix type hint in MimikatzCredentialCollector --- .../mimikatz_collector/mimikatz_credential_collector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py index b0299a0c8..8ed805ee7 100644 --- a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py @@ -19,7 +19,7 @@ class MimikatzCredentialCollector(ICredentialCollector): return MimikatzCredentialCollector._to_credentials(windows_credentials) @staticmethod - def _to_credentials(windows_credentials: Sequence[WindowsCredentials]) -> [Credentials]: + def _to_credentials(windows_credentials: Sequence[WindowsCredentials]) -> Sequence[Credentials]: credentials = [] for wc in windows_credentials: identity = None From c144ad9e64a6e5ad9dfee4940a5f4dcdc2a1a7fd Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:42:31 -0400 Subject: [PATCH 26/31] Agent: Fix "new user" logic in MimikatzCredentialCollector Neither Passwords nor hashes should be included for and users that Infection Monkey creates. --- .../mimikatz_collector/mimikatz_credential_collector.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py index 8ed805ee7..a2a1d0423 100644 --- a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py @@ -22,12 +22,15 @@ class MimikatzCredentialCollector(ICredentialCollector): def _to_credentials(windows_credentials: Sequence[WindowsCredentials]) -> Sequence[Credentials]: credentials = [] for wc in windows_credentials: - identity = None - # Mimikatz picks up users created by the Monkey even if they're successfully deleted # since it picks up creds from the registry. The newly created users are not removed # from the registry until a reboot of the system, hence this check. - if wc.username and not wc.username.startswith(USERNAME_PREFIX): + if wc.username and wc.username.startswith(USERNAME_PREFIX): + continue + + identity = None + + if wc.username: identity = Username(wc.username) if wc.password: From d5a125d985a87a7f3fa3e90464ffd8ce7b232e10 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:46:07 -0400 Subject: [PATCH 27/31] Agent: Capture username even if no secrets are associated --- .../mimikatz_credential_collector.py | 3 +++ .../test_mimikatz_collector.py | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py index a2a1d0423..e6bc04a31 100644 --- a/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py @@ -45,4 +45,7 @@ class MimikatzCredentialCollector(ICredentialCollector): ntlm_hash = NTHash(nt_hash=wc.ntlm_hash) credentials.append(Credentials(identity, ntlm_hash)) + if len(credentials) == 0 and identity is not None: + credentials.append(Credentials(identity, None)) + return credentials diff --git a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py index 47a6ead49..40d5728dd 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_mimikatz_collector.py @@ -91,3 +91,22 @@ def test_pypykatz_result_parsing_no_identities(monkeypatch): collected_credentials = collect_credentials() assert len(collected_credentials) == 2 assert collected_credentials == expected_credentials + + +def test_pypykatz_result_parsing_no_secrets(monkeypatch): + username = "user3" + win_creds = [ + WindowsCredentials( + username=username, + password="", + ntlm_hash="", + lm_hash="", + ), + ] + patch_pypykatz(win_creds, monkeypatch) + + expected_credentials = [Credentials(Username(username), None)] + + collected_credentials = collect_credentials() + assert len(collected_credentials) == 1 + assert collected_credentials == expected_credentials From 9edfe6979b3aa541f26fe75f541b3ef937034193 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 08:51:58 -0400 Subject: [PATCH 28/31] Agent: Capture secrets if missing username in SSHCredentialCollector --- .../ssh_collector/ssh_credential_collector.py | 2 +- .../test_ssh_credentials_collector.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py b/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py index 5c5b05467..cf18a8efc 100644 --- a/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py +++ b/monkey/infection_monkey/credential_collectors/ssh_collector/ssh_credential_collector.py @@ -45,7 +45,7 @@ class SSHCredentialCollector(ICredentialCollector): ssh_keypair.get("private_key", ""), ssh_keypair.get("public_key", "") ) - if identity is not None: + if any([identity, secret]): ssh_credentials.append(Credentials(identity, secret)) return ssh_credentials diff --git a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py index c092ed1fb..c6d2a869d 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_collectors/test_ssh_credentials_collector.py @@ -43,6 +43,12 @@ def test_ssh_info_result_parsing(monkeypatch, patch_telemetry_messenger): "private_key": None, }, {"name": "guest", "home_dir": "/", "public_key": None, "private_key": None}, + { + "name": "", + "home_dir": "/home/mcus", + "public_key": "PubKey", + "private_key": "PrivKey", + }, ] patch_ssh_handler(ssh_creds, monkeypatch) @@ -53,11 +59,13 @@ def test_ssh_info_result_parsing(monkeypatch, patch_telemetry_messenger): ssh_keypair1 = SSHKeypair("ExtremelyGoodPrivateKey", "SomePublicKeyUbuntu") ssh_keypair2 = SSHKeypair("", "AnotherPublicKey") + ssh_keypair3 = SSHKeypair("PrivKey", "PubKey") expected = [ Credentials(identity=username, secret=ssh_keypair1), Credentials(identity=username2, secret=ssh_keypair2), Credentials(identity=username3, secret=None), + Credentials(identity=None, secret=ssh_keypair3), ] collected = SSHCredentialCollector(patch_telemetry_messenger).collect_credentials() assert expected == collected From 7c920cced358dfc20c7d4f39c04d95141757edbe Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 09:07:11 -0400 Subject: [PATCH 29/31] Agent: Fix identity logic in AggregatingCredentialsStore --- .../credential_store/aggregating_credentials_store.py | 8 +++++--- .../test_aggregating_credentials_store.py | 5 +++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py index dacfbdcee..23ad7ee19 100644 --- a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py +++ b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py @@ -26,9 +26,11 @@ class AggregatingCredentialsStore(ICredentialsStore): def add_credentials(self, credentials_to_add: Iterable[Credentials]): for credentials in credentials_to_add: - self._stored_credentials.setdefault("exploit_user_list", set()).add( - credentials.identity.username - ) + identity = credentials.identity + if identity and identity.credential_type is CredentialComponentType.USERNAME: + self._stored_credentials.setdefault("exploit_user_list", set()).add( + identity.username + ) secret = credentials.secret diff --git a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py index 54d43f59c..871f0db44 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py @@ -38,6 +38,10 @@ TEST_CREDENTIALS = [ identity=Username("user3"), secret=SSHKeypair(public_key="some_public_key_1", private_key="some_private_key_1"), ), + Credentials( + identity=None, + secret=Password("super_secret"), + ), ] SSH_KEYS_CREDENTIALS = [ @@ -93,6 +97,7 @@ def test_add_credentials_to_store(aggregating_credentials_store): "abcdefg", "password", "root", + "super_secret", ] ) From 068dbbe963d1cf8d3b22f68aba997f74b4bbb960 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 09:14:51 -0400 Subject: [PATCH 30/31] Agent: Extract methods to clean up AggregatingCredentialsStore --- .../aggregating_credentials_store.py | 45 +++++++++---------- .../test_aggregating_credentials_store.py | 2 + 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py index 23ad7ee19..bfa69117f 100644 --- a/monkey/infection_monkey/credential_store/aggregating_credentials_store.py +++ b/monkey/infection_monkey/credential_store/aggregating_credentials_store.py @@ -1,7 +1,7 @@ import logging from typing import Any, Iterable, Mapping -from common.credentials import CredentialComponentType, Credentials +from common.credentials import CredentialComponentType, Credentials, ICredentialComponent from infection_monkey.custom_types import PropagationCredentials from infection_monkey.i_control_channel import IControlChannel from infection_monkey.utils.decorators import request_cache @@ -26,31 +26,28 @@ class AggregatingCredentialsStore(ICredentialsStore): def add_credentials(self, credentials_to_add: Iterable[Credentials]): for credentials in credentials_to_add: - identity = credentials.identity - if identity and identity.credential_type is CredentialComponentType.USERNAME: - self._stored_credentials.setdefault("exploit_user_list", set()).add( - identity.username - ) + if credentials.identity: + self._add_identity(credentials.identity) - secret = credentials.secret + if credentials.secret: + self._add_secret(credentials.secret) - if secret.credential_type is CredentialComponentType.PASSWORD: - self._stored_credentials.setdefault("exploit_password_list", set()).add( - secret.password - ) - elif secret.credential_type is CredentialComponentType.LM_HASH: - self._stored_credentials.setdefault("exploit_lm_hash_list", set()).add( - secret.lm_hash - ) - elif secret.credential_type is CredentialComponentType.NT_HASH: - self._stored_credentials.setdefault("exploit_ntlm_hash_list", set()).add( - secret.nt_hash - ) - elif secret.credential_type is CredentialComponentType.SSH_KEYPAIR: - self._set_attribute( - "exploit_ssh_keys", - [{"public_key": secret.public_key, "private_key": secret.private_key}], - ) + def _add_identity(self, identity: ICredentialComponent): + if identity.credential_type is CredentialComponentType.USERNAME: + self._stored_credentials.setdefault("exploit_user_list", set()).add(identity.username) + + def _add_secret(self, secret: ICredentialComponent): + if secret.credential_type is CredentialComponentType.PASSWORD: + self._stored_credentials.setdefault("exploit_password_list", set()).add(secret.password) + elif secret.credential_type is CredentialComponentType.LM_HASH: + self._stored_credentials.setdefault("exploit_lm_hash_list", set()).add(secret.lm_hash) + elif secret.credential_type is CredentialComponentType.NT_HASH: + self._stored_credentials.setdefault("exploit_ntlm_hash_list", set()).add(secret.nt_hash) + elif secret.credential_type is CredentialComponentType.SSH_KEYPAIR: + self._set_attribute( + "exploit_ssh_keys", + [{"public_key": secret.public_key, "private_key": secret.private_key}], + ) def get_credentials(self) -> PropagationCredentials: try: diff --git a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py index 871f0db44..e5ddccfe2 100644 --- a/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py +++ b/monkey/tests/unit_tests/infection_monkey/credential_store/test_aggregating_credentials_store.py @@ -42,6 +42,7 @@ TEST_CREDENTIALS = [ identity=None, secret=Password("super_secret"), ), + Credentials(identity=Username("user4"), secret=None), ] SSH_KEYS_CREDENTIALS = [ @@ -88,6 +89,7 @@ def test_add_credentials_to_store(aggregating_credentials_store): "root", "user1", "user3", + "user4", ] ) assert actual_stored_credentials["exploit_password_list"] == set( From e5d3271b74a911b5c360ef921ef668b064ef3569 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 18 Jul 2022 09:23:21 -0400 Subject: [PATCH 31/31] UT: Use Credentials.to_mapping() in test_credential_telem_send() --- .../infection_monkey/telemetry/test_credentials_telem.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py b/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py index 1ceb9f4e5..07a29ef95 100644 --- a/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py +++ b/monkey/tests/unit_tests/infection_monkey/telemetry/test_credentials_telem.py @@ -19,12 +19,7 @@ def credentials_for_test(): def test_credential_telem_send(spy_send_telemetry, credentials_for_test): - expected_data = [ - { - "identity": {"username": USERNAME, "credential_type": "USERNAME"}, - "secret": {"password": PASSWORD, "credential_type": "PASSWORD"}, - } - ] + expected_data = [Credentials.to_mapping(credentials_for_test)] telem = CredentialsTelem([credentials_for_test]) telem.send()