Create data directory on Island initialisation #1170

shreyamalviya · 2021-05-14T11:50:35Z

Fixes #1147
~~(Still need to figure out Windows permissions!)~~

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:

Ran monkey_island.py without --server-config and with (passing it valid path/valid file (with "data_dir" field and without), valid path/invalid file, invalid path)
~~If applicable, add screenshots or log transcripts of the feature working~~

ghost · 2021-05-14T11:50:42Z

DeepCode's analysis on #baee74 found:

⚠️ 1 warning 👇
✔️ 1 issue was fixed.

Top issues

Description	Example fixes
Use os.makedirs instead of os.mkdir because the given path may require creating the parent directories. Occurrences: data_dir_generator.py:16	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

codecov · 2021-05-14T11:50:44Z

Codecov Report

Merging #1170 (baee74b) into develop (d8c1e2b) will decrease coverage by 0.14%.
The diff coverage is 20.00%.

@@             Coverage Diff             @@
##           develop    #1170      +/-   ##
===========================================
- Coverage    28.94%   28.80%   -0.15%     
===========================================
  Files          419      424       +5     
  Lines        12797    12863      +66     
===========================================
+ Hits          3704     3705       +1     
- Misses        9093     9158      +65

Impacted Files	Coverage Δ
monkey/monkey_island.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/cc/arg_parser.py	`0.00% <0.00%> (ø)`
...monkey_island/cc/environment/data_dir_generator.py	`0.00% <0.00%> (ø)`
...monkey_island/cc/environment/environment_config.py	`100.00% <ø> (ø)`
...key_island/cc/environment/environment_singleton.py	`44.82% <ø> (-26.15%)`	⬇️
...key_island/cc/environment/server_config_handler.py	`0.00% <0.00%> (ø)`
...onkey_island/cc/environment/windows_permissions.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/cc/main.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/setup/config_setup.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/setup/gevent_setup.py	`0.00% <0.00%> (ø)`
... and 13 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 d8c1e2b...baee74b. Read the comment docs.

monkey/monkey_island.py

monkey/monkey_island/config_loader.py

monkey/monkey_island/cc/server_utils/consts.py

appimage/run_appimage.sh

…and initialisation

…ts, but the dir doesn't exist

…tup.py

permissions to newly created data directory

`set_data_dir_security_to_read_and_write_by_owner`

…_server_config_to_file()` This was causing an error in the unit tests, and is handled in monkey_island.py (commit 3de620e).

VakarisZ · 2021-05-24T06:09:42Z

monkey/monkey_island/cc/environment/server_config_generator.py

-def create_default_config_file(path):
+def create_default_server_config_file() -> str:
+    if not os.path.isfile(DEFAULT_SERVER_CONFIG_PATH):
+        create_data_dir(DEFAULT_DATA_DIR, False)


I'm sorry, but I still don't understand what's wrong with using os.makedirs here. The explanation of $HOME might not be resolved doesn't make sense. It's the responsibility of DEFAULT_DATA_DIR to point to a resolvable path. And if DEFAULT_DATA_DIR can't be resolved, how will not making parent directories help us?

For example, on Linux, if the $HOME env variable doesn't exist, the value of DEFAULT_DATA_DIR will remain "$HOME/.monkey_island" (we're using os.path.expandvars() to resolve the path but in case nothing is resolved, it will simply return the same string, it won't throw any kind of error). Using os.makedirs will create a directory named "$HOME" and the ".monkey_island" directory under that. Creating a directory named "$HOME" is not the expected behaviour. If the $HOME variable can't be resolved, we expect the user to specify the path for the data directory.

If the $HOME variable can't be resolved, we expect the user to specify the path for the data directory.

How is this expressed in the code and where? Users and developers are expected to get os.mkdir error and know that os.mkdir failed because it doesn't create parent folders, so it means that the parent folder of data_dir is not present?
What prevents the user from making the same mistake via cmd arguments?
Wouldn't it be better to

if not os.path.isdir(data_dir_parent): logger.error(f"Can't create a data directory because path {data_dir_parent} doesn't exist. Specify a reachable directory with {data_dir_flag}")

See baee74b7

VakarisZ · 2021-05-24T06:18:57Z

monkey/monkey_island.py


        config = config_loader.load_server_config_from_file(server_config_path)

+        create_data_dir(config["data_dir"], True)


This line only makes sense if island_args.server_config, because otherwise you create the dir in server_config_generator.create_default_server_config_file()

Fixed in #1184

…e file, with server config write/read operations

…f server_config setup by cmd_arguments and setup of server_config using defaults

Data dir on island refactor

…he argparser flag section

…ted ways

… though server path is not specified. Island thinks that server config path was specified.

…tion and failing

…e the entrypoint to the application

…ages import

…imports

… mongodb parameters

monkey/monkey_island.py

shreyamalviya · 2021-05-26T07:43:12Z

monkey/monkey_island/cc/environment/server_config_handler.py

-        add_default_values_to_config(config)

-        return config
-
-
-def add_default_values_to_config(config):
-    config["data_dir"] = os.path.abspath(
-        os.path.expanduser(os.path.expandvars(config.get("data_dir", DEFAULT_DATA_DIR)))
-    )
-
-    config.setdefault("log_level", DEFAULT_LOG_LEVEL)
-
-    return config


Are we deliberately removing this or is this part of the rebase mess?

This commit also removes monkey/tests/unit_tests/monkey_island/cc/environment/test_server_config_handler.py as part of the rebase. Depending on this comment, we need to re-add/modify this file too.

Deliberately. Config is now built in island_config_options.py.

shreyamalviya changed the title ~~Data dir on island init~~ Create data directory on Island initialisation May 14, 2021

VakarisZ reviewed May 14, 2021

View reviewed changes

monkey/monkey_island.py Outdated Show resolved Hide resolved

VakarisZ reviewed May 14, 2021

View reviewed changes

monkey/monkey_island/config_loader.py Outdated Show resolved Hide resolved

VakarisZ reviewed May 14, 2021

View reviewed changes

monkey/monkey_island/cc/server_utils/consts.py Outdated Show resolved Hide resolved

VakarisZ reviewed May 14, 2021

View reviewed changes

monkey/monkey_island/cc/server_utils/consts.py Outdated Show resolved Hide resolved

VakarisZ reviewed May 18, 2021

View reviewed changes

appimage/run_appimage.sh Show resolved Hide resolved

shreyamalviya force-pushed the data-dir-on-island-init branch from 8137313 to 81f6af7 Compare May 19, 2021 11:15

shreyamalviya marked this pull request as ready for review May 19, 2021 13:10

shreyamalviya requested a review from VakarisZ May 19, 2021 13:11

shreyamalviya force-pushed the data-dir-on-island-init branch from 9cc4d23 to b925113 Compare May 20, 2021 11:14

shreyamalviya and others added 18 commits May 21, 2021 14:55

Create data_dir if no --server-config is passed during Monkey Isl…

76d82ce

…and initialisation

Create data dir if --server-config is passed, "data_dir" field exis…

8246341

…ts, but the dir doesn't exist

Create data_dir before logger is set up

808e86d

Move create_data_dir() from monkey_island.py to monkey_island/cc/se…

9eedac4

…tup.py

Restructure code for creating default data directory and server config

805e5e6

Create constant SERVER_CONFIG_FILENAME

d8927a5

Replace missed out function name

af42c01

Remove logic for creating default server config in appimage script

ff1e6bd

Change data_dir permissions on Windows

a1beee9

Move is_windows_os to data_dir_generator.py and add user write

3201672

permissions to newly created data directory

Rename set_data_dir_security_to_read_by_owner() to

e7a26aa

`set_data_dir_security_to_read_and_write_by_owner`

Import Windows specific modules only on Windows

8c575b9

Refactored windows permission handling into a separate file

8ce506a

Refactored code duplication by adding a parameter for create_data_dir

409a3c5

Get default directory depending on OS

2fb77fc

Move is_windows_os() to separate file

37b7815

Fix unit test in test_consts.py

dc129c0

Update CHANGELOG (create data dir on island init)

4640a76

shreyamalviya added 3 commits May 21, 2021 14:56

Import Windows specific modules only on Windows systems

b4708fc

Remove code in environment_config.py which was calling `write_default…

9705c73

…_server_config_to_file()` This was causing an error in the unit tests, and is handled in monkey_island.py (commit 3de620e).

Remove unit test related to code removed in the previous commit

23b0492

shreyamalviya force-pushed the data-dir-on-island-init branch from 378f8ed to 23b0492 Compare May 21, 2021 13:48

Create default server_config.json when running unit tests

43d919a

VakarisZ reviewed May 24, 2021

View reviewed changes

VakarisZ and others added 15 commits May 24, 2021 11:05

Refactored config_loader.py and server_config_handler.py into a singl…

70b9a9f

…e file, with server config write/read operations

Improved monkey_island.py setup readability by extracting workflows o…

8afe937

…f server_config setup by cmd_arguments and setup of server_config using defaults

Moved a comment to a proper place

5f88f6f

Fixed a failing environment setup in unit tests

3098ac1

Use os.path.expandvars() on server config argument

d33c5d6

Merge pull request #1184 from guardicore/data-dir-on-island-refactor

f500ca8

Data dir on island refactor

Merged develop into data-dir-on-island-init

7d1c5dd

Improved readability in the arg_parser.py by extracting defaults to t…

0a7cf1d

…he argparser flag section

Fixed bugs merge bugs where structures are being accessed in an outda…

2b257f0

…ted ways

Fixed bugs in argument parser passing default server config path even…

d273e85

… though server path is not specified. Island thinks that server config path was specified.

Fixed a bug where models were getting imported without mongodb connec…

3a800d9

…tion and failing

Removed the infrastructure from starting main.py - it can no longer b…

9337c68

…e the entrypoint to the application

Rolled back the changes that made default server config on model pack…

057a579

…ages import

Fixed stack-overflow that has been happening due to gevent unpatched …

5b7329b

…imports

Fixed UT's for models by mocking an environment singleton with needed…

17e994c

… mongodb parameters

shreyamalviya commented May 26, 2021

View reviewed changes

monkey/monkey_island.py Outdated Show resolved Hide resolved

Remove unneeded "noqa" statements

a89a62c

shreyamalviya commented May 26, 2021

View reviewed changes

Add exception messages during data directory creation

baee74b

VakarisZ merged commit a211c0f into develop May 26, 2021

VakarisZ deleted the data-dir-on-island-init branch May 26, 2021 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create data directory on Island initialisation #1170

Create data directory on Island initialisation #1170

shreyamalviya commented May 14, 2021 •

edited

Loading

ghost commented May 14, 2021 •

edited by ghost

Loading

codecov bot commented May 14, 2021 •

edited

Loading

VakarisZ May 24, 2021

shreyamalviya May 24, 2021 •

edited

Loading

VakarisZ May 26, 2021

shreyamalviya May 26, 2021

VakarisZ May 24, 2021

shreyamalviya May 24, 2021

shreyamalviya May 26, 2021

shreyamalviya May 26, 2021

VakarisZ May 26, 2021


		config = config_loader.load_server_config_from_file(server_config_path)

		create_data_dir(config["data_dir"], True)

Create data directory on Island initialisation #1170

Create data directory on Island initialisation #1170

Conversation

shreyamalviya commented May 14, 2021 • edited Loading

PR Checklist

Testing Checklist

ghost commented May 14, 2021 • edited by ghost Loading

DeepCode's analysis on #baee74 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

codecov bot commented May 14, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

shreyamalviya May 24, 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

shreyamalviya commented May 14, 2021 •

edited

Loading

ghost commented May 14, 2021 •

edited by ghost

Loading

codecov bot commented May 14, 2021 •

edited

Loading

shreyamalviya May 24, 2021 •

edited

Loading