-
Notifications
You must be signed in to change notification settings - Fork 792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data dir #994
Data dir #994
Changes from 4 commits
a09f286
bc14136
c99349f
d04e5f3
2d1b84c
4c9cd3e
64b9431
c900694
90acc46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,50 +5,61 @@ | |
from Crypto import Random # noqa: DOU133 | ||
from Crypto.Cipher import AES # noqa: DOU133 | ||
|
||
from monkey_island.cc.consts import MONKEY_ISLAND_ABS_PATH | ||
|
||
__author__ = "itay.mizeretz" | ||
|
||
_encryptor = None | ||
|
||
|
||
class Encryptor: | ||
_BLOCK_SIZE = 32 | ||
_DB_PASSWORD_FILENAME = os.path.join(MONKEY_ISLAND_ABS_PATH, 'cc/mongo_key.bin') | ||
_PASSWORD_FILENAME = "mongo_key.bin" | ||
|
||
def __init__(self): | ||
self._load_key() | ||
def __init__(self, data_dir): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Data_dir is too abstract. I'd rename this to something like |
||
password_file = os.path.expanduser( | ||
os.path.join(data_dir, self._PASSWORD_FILENAME) | ||
) | ||
|
||
def _init_key(self): | ||
if os.path.exists(password_file): | ||
self._load_existing_key(password_file) | ||
else: | ||
self._init_key(password_file) | ||
|
||
def _init_key(self, password_file): | ||
self._cipher_key = Random.new().read(self._BLOCK_SIZE) | ||
with open(self._DB_PASSWORD_FILENAME, 'wb') as f: | ||
with open(password_file, "wb") as f: | ||
f.write(self._cipher_key) | ||
|
||
def _load_existing_key(self): | ||
with open(self._DB_PASSWORD_FILENAME, 'rb') as f: | ||
def _load_existing_key(self, password_file): | ||
with open(password_file, "rb") as f: | ||
self._cipher_key = f.read() | ||
|
||
def _load_key(self): | ||
if os.path.exists(self._DB_PASSWORD_FILENAME): | ||
self._load_existing_key() | ||
else: | ||
self._init_key() | ||
|
||
def _pad(self, message): | ||
return message + (self._BLOCK_SIZE - (len(message) % self._BLOCK_SIZE)) * chr( | ||
self._BLOCK_SIZE - (len(message) % self._BLOCK_SIZE)) | ||
self._BLOCK_SIZE - (len(message) % self._BLOCK_SIZE) | ||
) | ||
|
||
def _unpad(self, message: str): | ||
return message[0:-ord(message[len(message) - 1])] | ||
return message[0 : -ord(message[len(message) - 1])] | ||
|
||
def enc(self, message: str): | ||
cipher_iv = Random.new().read(AES.block_size) | ||
cipher = AES.new(self._cipher_key, AES.MODE_CBC, cipher_iv) | ||
return base64.b64encode(cipher_iv + cipher.encrypt(self._pad(message).encode())).decode() | ||
return base64.b64encode( | ||
cipher_iv + cipher.encrypt(self._pad(message).encode()) | ||
).decode() | ||
|
||
def dec(self, enc_message): | ||
enc_message = base64.b64decode(enc_message) | ||
cipher_iv = enc_message[0:AES.block_size] | ||
cipher_iv = enc_message[0 : AES.block_size] | ||
cipher = AES.new(self._cipher_key, AES.MODE_CBC, cipher_iv) | ||
return self._unpad(cipher.decrypt(enc_message[AES.block_size:]).decode()) | ||
return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode()) | ||
|
||
|
||
def initialize_encryptor(data_dir): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once again, this is not |
||
global _encryptor | ||
|
||
_encryptor = Encryptor(data_dir) | ||
|
||
|
||
encryptor = Encryptor() | ||
def encryptor(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice application of singleton pattern, but I'd still prefer method names to be verbs. So maybe |
||
return _encryptor |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
from typing import Dict, List | ||
|
||
import monkey_island.cc.environment.server_config_generator as server_config_generator | ||
from monkey_island.cc.consts import DEFAULT_DATA_DIR | ||
from monkey_island.cc.environment.user_creds import UserCreds | ||
from monkey_island.cc.resources.auth.auth_user import User | ||
from monkey_island.cc.resources.auth.user_store import UserStore | ||
|
@@ -18,6 +19,7 @@ def __init__(self, file_path): | |
self.deployment = None | ||
self.user_creds = None | ||
self.aws = None | ||
self.data_dir = None | ||
|
||
self._load_from_file(self._server_config_path) | ||
|
||
|
@@ -26,7 +28,7 @@ def _load_from_file(self, file_path): | |
|
||
if not Path(file_path).is_file(): | ||
server_config_generator.create_default_config_file(file_path) | ||
with open(file_path, 'r') as f: | ||
with open(file_path, "r") as f: | ||
config_content = f.read() | ||
|
||
self._load_from_json(config_content) | ||
|
@@ -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 = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Var name should be more explicit if possible |
||
dict_data["data_dir"] if "data_dir" in dict_data else DEFAULT_DATA_DIR | ||
) | ||
|
||
self.server_config = dict_data['server_config'] | ||
self.deployment = dict_data['deployment'] | ||
self.server_config = dict_data["server_config"] | ||
self.deployment = dict_data["deployment"] | ||
self.user_creds = user_creds | ||
self.aws = aws | ||
self.data_dir = data_dir | ||
|
||
def save_to_file(self): | ||
with open(self._server_config_path, 'w') as f: | ||
with open(self._server_config_path, "w") as f: | ||
f.write(json.dumps(self.to_dict(), indent=2)) | ||
|
||
def to_dict(self) -> Dict: | ||
config_dict = {'server_config': self.server_config, | ||
'deployment': self.deployment} | ||
config_dict = { | ||
"server_config": self.server_config, | ||
"deployment": self.deployment, | ||
"data_dir": self.data_dir, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same complaint, this should be more specific. In the context of environment config, this could be something like |
||
} | ||
if self.aws: | ||
config_dict.update({'aws': self.aws}) | ||
config_dict.update({"aws": self.aws}) | ||
config_dict.update(self.user_creds.to_dict()) | ||
return config_dict | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
import pytest | ||
|
||
from monkey_island.cc.consts import MONKEY_ISLAND_ABS_PATH | ||
from monkey_island.cc.consts import DEFAULT_DATA_DIR, MONKEY_ISLAND_ABS_PATH | ||
from monkey_island.cc.environment.environment_config import EnvironmentConfig | ||
from monkey_island.cc.environment.user_creds import UserCreds | ||
|
||
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a path to a file |
||
|
||
|
||
@pytest.fixture | ||
|
@@ -32,28 +33,31 @@ def config_file(tmpdir): | |
def test_get_with_credentials(): | ||
config_dict = EnvironmentConfig(WITH_CREDENTIALS).to_dict() | ||
|
||
assert len(config_dict.keys()) == 4 | ||
assert len(config_dict.keys()) == 5 | ||
assert config_dict["server_config"] == "password" | ||
assert config_dict["deployment"] == "develop" | ||
assert config_dict["user"] == "test" | ||
assert config_dict["password_hash"] == "abcdef" | ||
assert config_dict["data_dir"] == DEFAULT_DATA_DIR | ||
|
||
|
||
def test_get_with_no_credentials(): | ||
config_dict = EnvironmentConfig(NO_CREDENTIALS).to_dict() | ||
|
||
assert len(config_dict.keys()) == 2 | ||
assert len(config_dict.keys()) == 3 | ||
assert config_dict["server_config"] == "password" | ||
assert config_dict["deployment"] == "develop" | ||
assert config_dict["data_dir"] == DEFAULT_DATA_DIR | ||
|
||
|
||
def test_get_with_partial_credentials(): | ||
config_dict = EnvironmentConfig(PARTIAL_CREDENTIALS).to_dict() | ||
|
||
assert len(config_dict.keys()) == 3 | ||
assert len(config_dict.keys()) == 4 | ||
assert config_dict["server_config"] == "password" | ||
assert config_dict["deployment"] == "develop" | ||
assert config_dict["user"] == "test" | ||
assert config_dict["data_dir"] == DEFAULT_DATA_DIR | ||
|
||
|
||
def test_save_to_file(config_file): | ||
|
@@ -66,12 +70,13 @@ def test_save_to_file(config_file): | |
with open(config_file, "r") as f: | ||
from_file = json.load(f) | ||
|
||
assert len(from_file.keys()) == 5 | ||
assert len(from_file.keys()) == 6 | ||
assert from_file["server_config"] == "standard" | ||
assert from_file["deployment"] == "develop" | ||
assert from_file["user"] == "test" | ||
assert from_file["password_hash"] == "abcdef" | ||
assert from_file["aws"] == "test_aws" | ||
assert from_file["data_dir"] == DEFAULT_DATA_DIR | ||
|
||
|
||
def test_add_user(config_file): | ||
|
@@ -87,7 +92,7 @@ def test_add_user(config_file): | |
with open(config_file, "r") as f: | ||
from_file = json.load(f) | ||
|
||
assert len(from_file.keys()) == 4 | ||
assert len(from_file.keys()) == 5 | ||
assert from_file["user"] == new_user | ||
assert from_file["password_hash"] == new_password_hash | ||
|
||
|
@@ -112,3 +117,9 @@ def test_generate_default_file(config_file): | |
assert environment_config.user_creds.username == "" | ||
assert environment_config.user_creds.password_hash == "" | ||
assert environment_config.aws is None | ||
assert environment_config.data_dir == DEFAULT_DATA_DIR | ||
|
||
|
||
def test_data_dir(): | ||
environment_config = EnvironmentConfig(DATA_DIR) | ||
assert environment_config.data_dir == "/test/data/dir" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import os | ||
|
||
from monkey_island.cc.consts import MONKEY_ISLAND_ABS_PATH | ||
from monkey_island.cc.encryptor import initialize_encryptor, encryptor | ||
|
||
|
||
TEST_DATA_DIR = os.path.join(MONKEY_ISLAND_ABS_PATH, "cc", "testing") | ||
PASSWORD_FILENAME = "mongo_key.bin" | ||
|
||
PLAINTEXT = "Hello, Monkey!" | ||
CYPHERTEXT = "vKgvD6SjRyIh1dh2AM/rnTa0NI/vjfwnbZLbMocWtE4e42WJmSUz2ordtbQrH1Fq" | ||
|
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think more important than both of these cases would be to test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I waffled back and forth on this. I'll add it. |
||
|
||
|
||
def test_create_new_password_file(tmpdir): | ||
initialize_encryptor(tmpdir) | ||
|
||
assert os.path.isfile(os.path.join(tmpdir, PASSWORD_FILENAME)) | ||
|
||
|
||
def test_expand_home(monkeypatch, tmpdir): | ||
monkeypatch.setenv("HOME", str(tmpdir)) | ||
|
||
initialize_encryptor("~/") | ||
|
||
assert os.path.isfile(os.path.join(tmpdir, "mongo_key.bin")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.