Data dir #994

mssalvatore · 2021-02-24T15:50:53Z

What does this PR do?

Adds a data_dir field to server_config.json so that Infection Monkey can store runtime artifacts in a location that is configurable at runtime.

The mongo_key.bin is now stored in data_dir. Unit tests have been added for Encryptor.

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 documentation framework updated to reflect the changes?

Testing Checklist

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

Tested by running the monkey locally and building/running an AppImage
If applicable, add screenshots or log transcripts of the feature working

monkey/monkey_island/cc/environment/environment_singleton.py

VakarisZ · 2021-02-25T07:27:10Z

monkey/monkey_island/cc/consts.py

@@ -16,3 +16,5 @@
 DEFAULT_LOGGER_CONFIG_PATH = os.path.join(
    MONKEY_ISLAND_ABS_PATH, "cc", "island_logger_default_config.json"
 )
+
+DEFAULT_DATA_DIR = os.path.join(MONKEY_ISLAND_ABS_PATH, "cc")


Term "Data" is misleading. This is a path to island codebase, that's what I'd call this.

By default it is, only because that's what we're doing today. We need to move away from storing data generated at runtime alongside our code. Since we're so close to a release, I don't want to change that right now, except when running from an AppImage.

VakarisZ · 2021-02-25T08:03:45Z

monkey/monkey_island/cc/encryptor.py



-encryptor = Encryptor()
+def encryptor():


Nice application of singleton pattern, but I'd still prefer method names to be verbs. So maybe get_encryptor()?

VakarisZ · 2021-02-25T08:11:18Z

monkey/monkey_island/cc/environment/test_environment_config.py

@@ -22,6 +22,7 @@
 STANDARD_WITH_CREDENTIALS = os.path.join(
    TEST_RESOURCES_DIR, "server_config_standard_with_credentials.json"
 )
+DATA_DIR = os.path.join(TEST_RESOURCES_DIR, "server_config_data_dir.json")


This is a path to a file server_config_data_dir.json. Name DATA_DIR implies that it's a directory full of some kind of data. Find a more specific name for this variable, that would convey what server_config_data_dir.json is, like PATH_TO_SOMETHING

VakarisZ · 2021-02-25T08:16:28Z

monkey/monkey_island/cc/encryptor.py


-    def __init__(self):
-        self._load_key()
+    def __init__(self, data_dir):


Data_dir is too abstract. I'd rename this to something like password_file_dir

VakarisZ · 2021-02-25T08:22:10Z

monkey/monkey_island/cc/encryptor.py

+        return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode())
+
+
+def initialize_encryptor(data_dir):


Once again, this is not data_dir. In the context of encryptor, this is where password is, so it's password_file_dir_path or something like that.

VakarisZ · 2021-02-25T08:24:40Z

monkey/monkey_island/cc/environment/environment_config.py

@@ -37,22 +39,29 @@ def _load_from_json(self, config_json: str) -> EnvironmentConfig:

    def _load_from_dict(self, dict_data: Dict):
        user_creds = UserCreds.get_from_dict(dict_data)
-        aws = dict_data['aws'] if 'aws' in dict_data else None
+        aws = dict_data["aws"] if "aws" in dict_data else None
+        data_dir = (


Var name should be more explicit if possible

VakarisZ · 2021-02-25T08:26:38Z

monkey/monkey_island/cc/environment/environment_config.py

+        config_dict = {
+            "server_config": self.server_config,
+            "deployment": self.deployment,
+            "data_dir": self.data_dir,


Same complaint, this should be more specific. In the context of environment config, this could be something like path_to_dir_of_config_files or something.

VakarisZ · 2021-02-25T08:31:57Z

monkey/monkey_island/cc/test_encryptor.py

+def test_aes_cbc_encryption():
+    initialize_encryptor(TEST_DATA_DIR)
+
+    assert encryptor().enc(PLAINTEXT) != PLAINTEXT
+
+
+def test_aes_cbc_decryption():
+    initialize_encryptor(TEST_DATA_DIR)
+
+    assert encryptor().dec(CYPHERTEXT) == PLAINTEXT


I think more important than both of these cases would be to test assert encryptor().dec(encryptor().enc(PLAINTEXT)) == PLAINTEXT. Currently, if encryption fails, but still generates something your test would pass.

I waffled back and forth on this. I'll add it.

VakarisZ · 2021-02-25T08:35:34Z

monkey/monkey_island/cc/testing/environment/server_config_data_dir.json

@@ -0,0 +1,7 @@
+{


Name for this file doesn't make sense. If this is our new format of server_config.json then we should move this file to test_common folder (so we can reuse it for all tests that involve server_config.json) and rename it to server_config_test_data.json or something similar.

I renamed it to server_config_with_data_dir.json to help alleviate some ambiguity. As for it's placement, it's currently in the testing directory. It looks like we have testing and test_common. Since we're planning on moving all of our tests under a common directory, we can sort out the finer details of where this file lives when we undertake that endeavor.

VakarisZ

Please review my comments.

I approve this PR, because I haven't found issues big enough to be worth a discussion (feel free to zoom if you think otherwise). Resolve my comments how you best see fit and merge.

….json

mssalvatore added 4 commits February 24, 2021 10:05

cc: add data_dir property to EnvironmentConfig

a09f286

cc: allow encryptor to store key file in variable locations

bc14136

cc: format encryptor.py with black

c99349f

cc: expanduser in data_dir path in Encryptor

d04e5f3

mssalvatore commented Feb 24, 2021

View reviewed changes

monkey/monkey_island/cc/environment/environment_singleton.py Show resolved Hide resolved

mssalvatore requested a review from VakarisZ February 24, 2021 16:14

VakarisZ reviewed Feb 25, 2021

View reviewed changes

VakarisZ approved these changes Feb 25, 2021

View reviewed changes

mssalvatore added 5 commits February 25, 2021 06:36

cc: rename encryptor() -> get_encryptor()

2d1b84c

cc: rename DATA_DIR constant in test to WITH_DATA_DIR

4c9cd3e

cc: rename data_dir parameter in encryptor to password_file_dir

64b9431

cc: add encrypt/decrypt test

c900694

cc: rename server_config_data_dir.json -> server_config_with_data_dir…

90acc46

….json

mssalvatore merged commit 8264b5a into appimage Feb 25, 2021

mssalvatore mentioned this pull request Mar 31, 2021

Build Prototype Monkey Island AppImage #1069

Merged

7 tasks

mssalvatore deleted the data-dir branch April 15, 2021 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data dir #994

Data dir #994

mssalvatore commented Feb 24, 2021

VakarisZ Feb 25, 2021 •

edited

Loading

mssalvatore Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

VakarisZ Feb 25, 2021

mssalvatore Feb 25, 2021

VakarisZ Feb 25, 2021

mssalvatore Feb 25, 2021

VakarisZ left a comment

		return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode())


		def initialize_encryptor(data_dir):

Data dir #994

Data dir #994

Conversation

mssalvatore commented Feb 24, 2021

What does this PR do?

PR Checklist

Testing Checklist

VakarisZ Feb 25, 2021 • edited Loading

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

VakarisZ left a comment

Choose a reason for hiding this comment

VakarisZ Feb 25, 2021 •

edited

Loading