Mongo key encryption #1506

VakarisZ · 2021-09-30T14:19:31Z

What does this PR do?

PR Checklist

Have you added an explanation of what your changes do and why you'd like to include them?
Is the TravisCI build passing?
Was the CHANGELOG.md updated to reflect the changes?
Was the documentation framework updated to reflect the changes?

Testing Checklist

Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:

Tested by Registering, logging in, resetting the user

…nd PasswordBasedByteEncryptor This change allows to encrypt strings and bytes without any additional conversion done on the caller

This change enables the encryption/decryption of mongo key with a custom secret

…ongo key initialization that happens during launch and initialization of key which happens after login or registration

VakarisZ · 2021-09-30T14:22:11Z

monkey/monkey_island/cc/resources/auth/registration.py

@@ -19,21 +21,13 @@ def get(self):
        return {"needs_registration": is_registration_needed}

    def post(self):
-        credentials = _get_user_credentials_from_request(request)
+        # TODO delete the old key here, before creating new one


Is it worth it? I think it is. It will allow us to remove the extra step of delete this mongo key from credential reset procedure

Makes sense, as long as it's appropriately encapsulated.

codecov · 2021-09-30T14:31:13Z

Codecov Report

Merging #1506 (ddff2f0) into develop (f387595) will increase coverage by 0.12%.
The diff coverage is 70.71%.

@@             Coverage Diff             @@
##           develop    #1506      +/-   ##
===========================================
+ Coverage    42.69%   42.82%   +0.12%     
===========================================
  Files          476      478       +2     
  Lines        14202    14242      +40     
===========================================
+ Hits          6064     6099      +35     
- Misses        8138     8143       +5

Impacted Files	Coverage Δ
monkey/monkey_island/cc/app.py	`79.69% <ø> (+0.43%)`	⬆️
...monkey_island/cc/resources/configuration_import.py	`61.29% <0.00%> (ø)`
monkey/monkey_island/cc/server_setup.py	`0.00% <ø> (ø)`
.../server_utils/encryption/encryptors/i_encryptor.py	`100.00% <ø> (ø)`
...utils/encryption/encryptors/key_based_encryptor.py	`100.00% <ø> (ø)`
...attack/technique_reports/technique_report_tools.py	`23.52% <0.00%> (ø)`
monkey/monkey_island/cc/services/database.py	`58.33% <ø> (+2.77%)`	⬆️
monkey/monkey_island/cc/services/initialize.py	`0.00% <0.00%> (ø)`
...island/cc/services/telemetry/processing/exploit.py	`26.92% <0.00%> (ø)`
...nd/cc/services/telemetry/processing/system_info.py	`27.41% <0.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f387595...ddff2f0. Read the comment docs.

VakarisZ · 2021-09-30T14:45:38Z

@mssalvatore take a look at commit 4f17693.
Multiple things are changed there:

Password utils renamed to credential utils
Credential utils moved to a single file to be more reusable between registration and login. Registration page changed to use 'username' instead of 'user', same as login page
Data store encryptor initialization split up into 2 parts: dir initialization and key decryption/setup.

Separating these changes into different commits post-factum seems more work than it's worth. Doing refactoring first requires a lot of forethought. We could separate the things roughly by modified files, but the it will introducuse system-breaking commits.

mssalvatore · 2021-09-30T17:28:16Z

monkey/monkey_island/cc/resources/auth/credential_utils.py

+    return f"{username}:{password}"
+
+
+def get_creds_from_request(_request: Request) -> Tuple[str, str]:


Dnt abbrv.

Or at least abbreviate consistently. This function is get_creds...(), whereas above there is get_user_credentials...() My preference is to use "credentials" in these names.

mssalvatore · 2021-09-30T17:31:33Z

monkey/monkey_island/cc/resources/auth/credential_utils.py

+def get_secret_from_request(_request) -> str:
+    username, password = get_creds_from_request(_request)
+    return f"{username}:{password}"


I think this should be handled in setup_datastore_key(). Pass it the username and password and let it worry about what to do with them.

mssalvatore · 2021-09-30T17:34:33Z

monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py

-        key_file = os.path.join(key_file_dir, self._KEY_FILENAME)
+    def __init__(self, key_file_dir: str):
+        self.key_file_path = os.path.join(key_file_dir, self._KEY_FILENAME)
+        self._key_base_encryptor = None


"_key_based"

mssalvatore · 2021-09-30T17:37:52Z

monkey/monkey_island/cc/server_utils/encryption/password_based_string_encryption.py

+class PasswordBasedStringEncryptor(IEncryptor):
+
+    _BUFFER_SIZE = pyAesCrypt.crypto.bufferSizeDef
+
+    def __init__(self, password: str):
+        self.password = password
+
+    def encrypt(self, plaintext: str) -> str:
+        plaintext_stream = io.BytesIO(plaintext.encode())
+        ciphertext = PasswordBasedByteEncryptor(self.password).encrypt(plaintext_stream)
+
+        return base64.b64encode(ciphertext.getvalue()).decode()
+
+    def decrypt(self, ciphertext: str) -> str:
+        ciphertext = base64.b64decode(ciphertext)
+        ciphertext_stream = io.BytesIO(ciphertext)
+
+        plaintext_stream = PasswordBasedByteEncryptor(self.password).decrypt(ciphertext_stream)
+        return plaintext_stream.getvalue().decode("utf-8")


Suggested change

class PasswordBasedStringEncryptor(IEncryptor):

_BUFFER_SIZE = pyAesCrypt.crypto.bufferSizeDef

def __init__(self, password: str):

self.password = password

def encrypt(self, plaintext: str) -> str:

plaintext_stream = io.BytesIO(plaintext.encode())

ciphertext = PasswordBasedByteEncryptor(self.password).encrypt(plaintext_stream)

return base64.b64encode(ciphertext.getvalue()).decode()

def decrypt(self, ciphertext: str) -> str:

ciphertext = base64.b64decode(ciphertext)

ciphertext_stream = io.BytesIO(ciphertext)

plaintext_stream = PasswordBasedByteEncryptor(self.password).decrypt(ciphertext_stream)

return plaintext_stream.getvalue().decode("utf-8")

class PasswordBasedStringEncryptor(IEncryptor):

_BUFFER_SIZE = pyAesCrypt.crypto.bufferSizeDef

def __init__(self, password: str):

self._bytes_encryptor = PasswordBasedByteEncryptor(password)

def encrypt(self, plaintext: str) -> str:

plaintext_stream = io.BytesIO(plaintext.encode())

ciphertext = self._bytes_encryptor.encrypt(plaintext_stream)

return base64.b64encode(ciphertext.getvalue()).decode()

def decrypt(self, ciphertext: str) -> str:

ciphertext = base64.b64decode(ciphertext)

ciphertext_stream = io.BytesIO(ciphertext)

plaintext_stream = self._bytes_encryptor.decrypt(ciphertext_stream)

return plaintext_stream.getvalue().decode("utf-8")

mssalvatore · 2021-09-30T17:39:01Z

monkey/monkey_island/cc/server_utils/encryption/password_based_byte_encryption.py

@@ -17,36 +17,28 @@
 # Note: password != key


-class PasswordBasedEncryptor(IEncryptor):
+class PasswordBasedByteEncryptor(IEncryptor):


What about "PasswordBasedBytesEncryptor"? "Byte" makes it seem like it encrypts a single byte.

The file name should match the class name.

mssalvatore · 2021-09-30T17:42:28Z

monkey/monkey_island/cc/server_utils/encryption/password_based_byte_encryption.py


    _BUFFER_SIZE = pyAesCrypt.crypto.bufferSizeDef

    def __init__(self, password: str):
        self.password = password

-    def encrypt(self, plaintext: str) -> str:
-        plaintext_stream = io.BytesIO(plaintext.encode())
+    def encrypt(self, plaintext: BytesIO) -> BytesIO:


I think this should take bytes, not BytesIO. Anything that has to use it (such as PasswordBasedStringEncryptor) has to convert first to bytes and then to BytesIO. Anything that already has data as bytes has to then convert it to BytesIO.

The only reason that it needs to be BytesIO is because that's what pyAesCrypt expects. This signature requiring BytesIO leaks implementation details to the callers.

I agree on this. It will also improve readability where we use the functions.

mssalvatore · 2021-09-30T17:49:59Z

monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py

+    def init_key(self, secret: str):
+        if os.path.exists(self.key_file_path):
+            self._load_existing_key(secret)
        else:
-            self._init_key(key_file)
-
-        self._key_base_encryptor = KeyBasedEncryptor(self._cipher_key)
-
-    def _init_key(self, password_file_path: str):
-        self._cipher_key = Random.new().read(self._BLOCK_SIZE)
-        with open_new_securely_permissioned_file(password_file_path, "wb") as f:
-            f.write(self._cipher_key)
-
-    def _load_existing_key(self, key_file):
-        with open(key_file, "rb") as f:
-            self._cipher_key = f.read()
+            self._create_new_key(secret)


I think this two-phased construction approach adds unnecessary complexity to the encryptor. I think we can solve this in a better way by using a factory. We can discuss options on Zoom.

You mean that database initializer is constructed first and the key is initialized later? Or that we either load the key or create one?

mssalvatore · 2021-09-30T17:54:26Z

monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py

 def test_create_new_password_file(tmpdir):
    initialize_datastore_encryptor(tmpdir)
-
-    assert os.path.isfile(os.path.join(tmpdir, PASSWORD_FILENAME))
+    setup_datastore_key(ENCRYPTOR_SECRET)
+    assert os.path.isfile(os.path.join(tmpdir, DataStoreEncryptor._KEY_FILENAME))


While we're here:

def test_create_new_password_file(tmp_path): initialize_datastore_encryptor(tmp_path) setup_datastore_key(ENCRYPTOR_SECRET) assert (tmp_path / DataStoreEncryptor._KEY_FILENAME).is_file()

mssalvatore · 2021-09-30T17:55:48Z

monkey/monkey_island/cc/resources/auth/auth.py

-
-
-def _credentials_match_registered_user(username, password):
+def _credentials_match_registered_user(username: str, password: str):


Suggested change

def _credentials_match_registered_user(username: str, password: str):

def _credentials_match_registered_user(username: str, password: str) -> bool:

mssalvatore · 2021-09-30T18:01:11Z

monkey/monkey_island/cc/server_utils/encryption/password_based_string_encryption.py

+logger = logging.getLogger(__name__)
+
+
+class PasswordBasedStringEncryptor(IEncryptor):


The file name should match the class name.

…ests for data store encryptor Data store key needs to be deleted upon registration to create a new one.

…ptor from credential_utils.py

…hange PasswordBasedBytesEncryptor interface to use bytes instead of io.BytesIO

…e() syntax to check for presence of files

…s credentials.)

…thods into EncryptorFactory This makes encryptor initialization workflow more straight-forward and the files become smaller, easier to read

…equest This better indicates that get_username_password_from_request returns a username/password pair rather than UserCreds structure

mssalvatore · 2021-10-01T12:47:05Z

monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py

+    assert (initialized_key_dir / EncryptorFactory._KEY_FILENAME).isfile()
+
+
+def test_key_removal(initialized_key_dir, monkeypatch):


Suggested change

def test_key_removal(initialized_key_dir, monkeypatch):

def test_key_removal(initialized_key_dir):

mssalvatore · 2021-10-01T12:49:01Z

monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_data_store_encryptor.py

+    initialize_encryptor_factory(tmpdir)
+    assert not (tmpdir / EncryptorFactory._KEY_FILENAME).isfile()
+    initialize_datastore_encryptor(MOCK_USERNAME, MOCK_PASSWORD)
+    assert (tmpdir / EncryptorFactory._KEY_FILENAME).isfile()


We should add an encryption/decryption test.

mssalvatore · 2021-10-01T12:52:43Z

monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py

+    if not factory:
+        raise FactoryNotInitializedError
+    secret = get_secret_from_credentials(username, password)
+    _encryptor = DataStoreEncryptor(factory.key_file_path, secret)


Factories are responsible for constructing objects. We should have factory.set_key_file_path() and factory.set_secret() and then call factory.construct_data_store_encryptor() or similar.

Yes, this is not a factory, it's a class that handles initialization parameters for data store encryptor. I'm not sure what would be a better name, maybe DataStoreEncryptorInitializer? I don't see any benefits in making this an actual factory, since the output object will not be different depending on key file path and secret. What is more, the usage of such factory is less intuitive. The registration resource would have to factory.set_secret() then built_encryptor = factory.construct_data_store_encryptor() and finally set_encryptor(built_encryptor). Now it's only one call - initialize_datastore_encryptor(username, password).

I think we can simplify this way down. I'm going to dig into it a bit.

I'll try to refactor and see how it looks

mssalvatore · 2021-10-01T12:53:07Z

monkey/monkey_island/cc/server_utils/encryption/encryptor_factory.py

+    _KEY_FILENAME = "mongo_key.bin"
+
+    def __init__(self, key_file_dir: str):
+        self.key_file_path = os.path.join(key_file_dir, self._KEY_FILENAME)


Prefer pathlib

… data_store_encryptor.py

mssalvatore · 2021-10-04T00:32:00Z

monkey/monkey_island/cc/server_utils/encryption/data_store_encryptor.py

+    def __init__(self, key_based_encryptor: KeyBasedEncryptor):
+        self._key_based_encryptor = key_based_encryptor


If the DataStoreEncryptor is a thin wrapper around KeyBasedEncryptor with no logic, I'd rather get_datastore_encryptor() return an IEncryptor and just return a concrete KeyBasedEncryptor. We can refactor everything to use encrypt() and decrypt() instead of enc() and dec(), which is better anyway.

This separates encryptor classes from other encryption related infrastructure that we have cc\server_utils\encryption

…e encryptor creation related code to data_store_encryptor.py, move the reponsibility to initialize data store encryptor to AuthenticationService

Database initialization can not be done because island doesn't know the key needed for encrypting collections. Since the key only appears after registration, database setup also should happen only after registration

VakarisZ · 2021-10-04T11:51:50Z

monkey/monkey_island/cc/server_utils/encryption/encryptors/password_based_string_encryptior.py

+from monkey_island.cc.server_utils.encryption.encryptors.password_based_bytes_encryption import (
+    PasswordBasedBytesEncryptor,
+)


can't do from monkey_island.cc.server_utils.encryption import PasswordBasedBytesEncryptor because of circular imports. 🤔

mssalvatore

I think this iteration looks much better. I have some thoughts on cleaning this up a bit further, but if all of the automated and manual tests pass, let's merge it.

VakarisZ added 3 commits September 30, 2021 17:16

Refactor password based encryptor into PasswordBasedStringEncryptor a…

191fbea

…nd PasswordBasedByteEncryptor This change allows to encrypt strings and bytes without any additional conversion done on the caller

Add a secret to datastore encryptor

fd1cb9d

This change enables the encryption/decryption of mongo key with a custom secret

Split up the initialization of mongo_key into 2 parts: directory of m…

4f17693

…ongo key initialization that happens during launch and initialization of key which happens after login or registration

VakarisZ requested review from ilija-lazoroski and mssalvatore September 30, 2021 14:19

VakarisZ commented Sep 30, 2021

View reviewed changes

mssalvatore requested changes Sep 30, 2021

View reviewed changes

VakarisZ added 7 commits October 1, 2021 11:26

Implement data store encryptor key removal on registration and unit t…

f97ec4e

…ests for data store encryptor Data store key needs to be deleted upon registration to create a new one.

Move data store encryptor secret generation into the data store encry…

e280c4f

…ptor from credential_utils.py

Fix typos and rename files/classes related to data store encryptor. C…

4cbed6d

…hange PasswordBasedBytesEncryptor interface to use bytes instead of io.BytesIO

Refactor test_data_store_encryptor.py to use (path / to / file).isfil…

ddae092

…e() syntax to check for presence of files

Add CHANGELOG.md entry for #1463 (Encrypt the database key with user'…

b2bbb62

…s credentials.)

Refactor DataStoreEncryptor by splitting up initialization related me…

da169dd

…thods into EncryptorFactory This makes encryptor initialization workflow more straight-forward and the files become smaller, easier to read

Refactor get_credentials_from_request to get_username_password_from_r…

26ba02a

…equest This better indicates that get_username_password_from_request returns a username/password pair rather than UserCreds structure

VakarisZ marked this pull request as ready for review October 1, 2021 12:41

mssalvatore reviewed Oct 1, 2021

View reviewed changes

Move all encryptor building related code to encryptor_factory.py from…

9d6dc3b

… data_store_encryptor.py

mssalvatore reviewed Oct 4, 2021

View reviewed changes

VakarisZ added 5 commits October 4, 2021 11:09

Move encryptors into a separate folder

34d065c

This separates encryptor classes from other encryption related infrastructure that we have cc\server_utils\encryption

Refactor data store encryptor to IEncryptor interface, move data stor…

3ec26bc

…e encryptor creation related code to data_store_encryptor.py, move the reponsibility to initialize data store encryptor to AuthenticationService

Fix scoutsuite unit test to use updated datastore encryptor interface

ea6fe37

Fix unit tests for data store encryptor

a2b09a9

Remove database initialization during island startup

3b5dd6a

Database initialization can not be done because island doesn't know the key needed for encrypting collections. Since the key only appears after registration, database setup also should happen only after registration

VakarisZ commented Oct 4, 2021

View reviewed changes

Refactor a couple of imports into a shorter import statement

ddff2f0

mssalvatore approved these changes Oct 4, 2021

View reviewed changes

VakarisZ force-pushed the mongo_key_encryption branch from a8f875d to ddff2f0 Compare October 4, 2021 11:59

VakarisZ merged commit af99482 into develop Oct 4, 2021

VakarisZ deleted the mongo_key_encryption branch October 4, 2021 12:10

mssalvatore mentioned this pull request Nov 19, 2021

Remove environments #1620

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mongo key encryption #1506

Mongo key encryption #1506

VakarisZ commented Sep 30, 2021

VakarisZ Sep 30, 2021

mssalvatore Sep 30, 2021

codecov bot commented Sep 30, 2021 •

edited

Loading

VakarisZ commented Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

ilija-lazoroski Oct 1, 2021

mssalvatore Sep 30, 2021

VakarisZ Oct 1, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Sep 30, 2021

mssalvatore Oct 1, 2021

mssalvatore Oct 1, 2021

mssalvatore Oct 1, 2021

VakarisZ Oct 1, 2021

mssalvatore Oct 1, 2021

VakarisZ Oct 1, 2021

mssalvatore Oct 1, 2021

mssalvatore Oct 4, 2021

VakarisZ Oct 4, 2021

mssalvatore left a comment

		return f"{username}:{password}"


		def get_creds_from_request(_request: Request) -> Tuple[str, str]:



		def _credentials_match_registered_user(username, password):
		def _credentials_match_registered_user(username: str, password: str):

		logger = logging.getLogger(__name__)


		class PasswordBasedStringEncryptor(IEncryptor):

		assert (initialized_key_dir / EncryptorFactory._KEY_FILENAME).isfile()


		def test_key_removal(initialized_key_dir, monkeypatch):

		def __init__(self, key_based_encryptor: KeyBasedEncryptor):
		self._key_based_encryptor = key_based_encryptor

Mongo key encryption #1506

Mongo key encryption #1506

Conversation

VakarisZ commented Sep 30, 2021

What does this PR do?

PR Checklist

Testing Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2021 • edited Loading

Codecov Report

VakarisZ commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssalvatore left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2021 •

edited

Loading