Delay mongo init to after registration #1495

shreyamalviya · 2021-09-28T08:01:35Z

What does this PR do?

#1463

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 running the Island (deleted data dir, deleted DB, deleted server config, deleted creds)
Tested by running Hugo.
If applicable, add screenshots or log transcripts of the feature working

codecov · 2021-09-28T08:08:05Z

Codecov Report

Merging #1495 (349b381) into develop (b791ee1) will decrease coverage by 0.52%.
The diff coverage is 42.85%.

❗ Current head 349b381 differs from pull request most recent head 579ebf4. Consider uploading reports for the commit 579ebf4 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1495      +/-   ##
===========================================
- Coverage    42.61%   42.09%   -0.53%     
===========================================
  Files          476      468       -8     
  Lines        14193    14069     -124     
===========================================
- Hits          6049     5922     -127     
- Misses        8144     8147       +3

Impacted Files	Coverage Δ
monkey/monkey_island/cc/server_setup.py	`0.00% <ø> (ø)`
...ey/monkey_island/cc/resources/auth/registration.py	`44.44% <35.71%> (-3.39%)`	⬇️
monkey/monkey_island/cc/resources/auth/auth.py	`55.76% <57.14%> (+0.21%)`	⬆️
...d/cc/services/attack/technique_reports/__init__.py	`45.07% <0.00%> (-21.26%)`	⬇️
...land/cc/services/attack/technique_reports/T1197.py	`61.53% <0.00%> (-2.75%)`	⬇️
...land/cc/services/attack/technique_reports/T1035.py	`70.00% <0.00%> (-2.73%)`	⬇️
...land/cc/services/attack/technique_reports/T1106.py	`70.00% <0.00%> (-2.73%)`	⬇️
...land/cc/services/attack/technique_reports/T1090.py	`50.00% <0.00%> (-2.64%)`	⬇️
...land/cc/services/attack/technique_reports/T1041.py	`45.00% <0.00%> (-2.62%)`	⬇️
...land/cc/services/attack/technique_reports/T1064.py	`66.66% <0.00%> (-2.57%)`	⬇️
... and 55 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 b791ee1...579ebf4. Read the comment docs.

VakarisZ · 2021-09-28T08:21:58Z

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

@@ -42,6 +45,7 @@ def post(self):

        if _credentials_match_registered_user(username, password):
            access_token = _create_access_token(username)
+            _check_attack_mitigations_in_mongo()


This checks for attack mitigations during login? Why? Shouldn't it happen during registration?

VakarisZ · 2021-09-28T08:26:15Z

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

+def _check_attack_mitigations_in_mongo():
+    if AttackMitigations.COLLECTION_NAME not in mongo.db.list_collection_names():
+        init_collections()


I feel like this function doesn't belong here since it has nothing to do with authentication's resource

VakarisZ · 2021-09-28T08:35:08Z

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

+        is_registration_needed = env_singleton.env.needs_registration()
+        if is_registration_needed:
+            # if registration is required, drop previous user's data (for credentials reset case)
+            _drop_mongo_db()


All this logic should be moved into init_collections method which gets called during registration. This way interaction with mongodb during registration is separated from the logic of resources. In general resources shouldn't interact with database directly.

VakarisZ · 2021-09-28T08:38:14Z

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

+        except Exception as ex:
+            logger.error(
+                "Exception raised during registration; most likely an issue with the "
+                f"mongo collection's initialisation. Exception: {str(ex)}."
+            )


if the error has to do with interaction with mongo we should catch it when interacting with mongo.

VakarisZ · 2021-09-28T08:40:06Z

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

+                "Exception raised during registration; most likely an issue with the "
+                f"mongo collection's initialisation. Exception: {str(ex)}."
+            )
+            return make_response({"error": str(ex)}, 400)


What's the poin of this response? user can already see the error in logs and we don't handle 400 in any special way on the front end.

VakarisZ · 2021-09-28T08:42:00Z

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

+
+
+def _drop_mongo_db():
+    mongo.db.command("dropDatabase")


why not mongo.drop_database('db')? Why are we even dropping the whole database if we're going to import the exact same mitigations?

VakarisZ

Mostly nit-picking on the readability, but some major questions as well.

mssalvatore · 2021-09-28T15:36:05Z

CHANGELOG.md

+- Initialise MongoDB collection of attack mitigations after registration or login (if required)
+  instead of on Island startup. #1495


I think this is too low-level for the changelog.

Maybe a better entry is "Resetting credentials clears the database".

…esetting login credentials also cleans the contents of the database. #1495"

… in DB, add if not

of when registration request is sent The issue with this whole change is that there's a long gap where nothing happens after you click on the log in or register button on the UI. But we don't need to worry about this because we plan on shipping Island's mongodb with attack mitigations already present.

…esetting login credentials also cleans the contents of the database. #1495"

…st is being processed

mssalvatore · 2021-09-29T13:48:08Z

monkey/monkey_island/cc/setup/mongo/database_initializer.py

+def reset_database():
+    Database.reset_db()
+    if Database.is_mitigations_missing():
+        logger.info("Populating Monkey Island with ATT&CK mitigations, this might take a while...")


This happens very quickly now (< 5ms). I think we can remove "this might take a while..."

ilija-lazoroski

Great job!

…he part saying that it will take a while

shreyamalviya requested review from mssalvatore, VakarisZ and ilija-lazoroski September 28, 2021 08:07

VakarisZ reviewed Sep 28, 2021

View reviewed changes

VakarisZ requested changes Sep 28, 2021

View reviewed changes

mssalvatore reviewed Sep 28, 2021

View reviewed changes

VakarisZ added a commit that referenced this pull request Sep 29, 2021

Rename the CHANGELOG.md entry about resetting login credentials to "R…

252c21d

…esetting login credentials also cleans the contents of the database. #1495"

shreyamalviya and others added 10 commits September 29, 2021 16:44

island: Add attack mitigations to mongo upon registration

3cbeb3d

island: On login, check if collection 'attack_mitigations' is present…

194e244

… in DB, add if not

island: Drop mongo db if registration is required

340dd1f

docs: Add warning that DB will be cleared if creds are reset

1e02ab6

docs: Fix spelling mistake

2cbaf95

CHANGELOG: Add entry for delaying mongo init

ab7872d

Rename the CHANGELOG.md entry about resetting login credentials to "R…

b73958d

…esetting login credentials also cleans the contents of the database. #1495"

Move database reset to happen during the registration

c211d51

Alter registration page to show loading icon while registration reque…

579ebf4

…st is being processed

VakarisZ force-pushed the delay-mongo-init branch from 8e425b1 to 579ebf4 Compare September 29, 2021 13:45

VakarisZ approved these changes Sep 29, 2021

View reviewed changes

mssalvatore approved these changes Sep 29, 2021

View reviewed changes

ilija-lazoroski approved these changes Sep 29, 2021

View reviewed changes

Alter the log message talking about storing the mitigations: remove t…

7939ed4

…he part saying that it will take a while

VakarisZ merged commit f387595 into develop Sep 29, 2021

VakarisZ deleted the delay-mongo-init branch September 29, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay mongo init to after registration #1495

Delay mongo init to after registration #1495

shreyamalviya commented Sep 28, 2021

codecov bot commented Sep 28, 2021 •

edited

Loading

VakarisZ Sep 28, 2021

VakarisZ Sep 28, 2021

VakarisZ Sep 28, 2021

VakarisZ Sep 28, 2021

VakarisZ Sep 28, 2021

VakarisZ Sep 28, 2021

VakarisZ left a comment

mssalvatore Sep 28, 2021

mssalvatore Sep 29, 2021

ilija-lazoroski left a comment

		- Initialise MongoDB collection of attack mitigations after registration or login (if required)
		instead of on Island startup. #1495

Delay mongo init to after registration #1495

Delay mongo init to after registration #1495

Conversation

shreyamalviya commented Sep 28, 2021

What does this PR do?

PR Checklist

Testing Checklist

codecov bot commented Sep 28, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilija-lazoroski left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2021 •

edited

Loading