Conversation
| MONKEY_ISLAND_ABS_PATH, "cc", "island_logger_default_config.json" | ||
| ) | ||
| | ||
| DEFAULT_DATA_DIR = os.path.join(MONKEY_ISLAND_ABS_PATH, "cc") |
There was a problem hiding this comment.
Term "Data" is misleading. This is a path to island codebase, that's what I'd call this.
There was a problem hiding this comment.
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.
monkey/monkey_island/cc/encryptor.py Outdated
| | ||
| | ||
| encryptor = Encryptor() | ||
| def encryptor(): |
There was a problem hiding this comment.
Nice application of singleton pattern, but I'd still prefer method names to be verbs. So maybe get_encryptor()?
| 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") |
There was a problem hiding this comment.
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
monkey/monkey_island/cc/encryptor.py Outdated
| | ||
| def __init__(self): | ||
| self._load_key() | ||
| def __init__(self, data_dir): |
There was a problem hiding this comment.
Data_dir is too abstract. I'd rename this to something like password_file_dir
monkey/monkey_island/cc/encryptor.py Outdated
| return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode()) | ||
| | ||
| | ||
| def initialize_encryptor(data_dir): |
There was a problem hiding this comment.
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.
| 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 = ( |
There was a problem hiding this comment.
Var name should be more explicit if possible
| config_dict = { | ||
| "server_config": self.server_config, | ||
| "deployment": self.deployment, | ||
| "data_dir": self.data_dir, |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I waffled back and forth on this. I'll add it.
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 left a comment
VakarisZ left a comment There was a problem hiding this comment.
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.
2 participants
What does this PR do?
Adds a
data_dirfield toserver_config.jsonso that Infection Monkey can store runtime artifacts in a location that is configurable at runtime.The
mongo_key.binis now stored indata_dir. Unit tests have been added forEncryptor.PR Checklist
Testing Checklist