Rephrase custom PBA file descriptions in configuration by shreyamalviya · Pull Request #1027 · guardicore/monkey

shreyamalviya · 2021-03-09T09:55:05Z

Related: #1020

codecov · 2021-03-09T10:05:08Z

Codecov Report

Merging #1027 (2b4fd9e) into release/1.10.0 (53fa550) will increase coverage by 0.54%.
The diff coverage is n/a.

@@ Coverage Diff @@ ## release/1.10.0 #1027 +/- ## ================================================== + Coverage 27.63% 28.18% +0.54%  ================================================== Files 402 402 Lines 12838 12830 -8 ================================================== + Hits 3548 3616 +68  + Misses 9290 9214 -76

Impacted Files	Coverage Δ
monkey/monkey/infection_monkey/utils/monkey_dir.py	`46.66% <0.00%> (+46.66%)`	⬆️
monkey/monkey/infection_monkey/post_breach/pba.py	`56.81% <0.00%> (+56.81%)`	⬆️
...ion_monkey/post_breach/actions/users_custom_pba.py	`57.37% <0.00%> (+57.37%)`	⬆️
...ey/monkey/infection_monkey/post_breach/__init__.py	`100.00% <0.00%> (+100.00%)`	⬆️

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 53fa550...2b4fd9e. Read the comment docs.

VakarisZ

First, let us resolve the PBA fix PR

ghost · 2021-03-09T13:50:45Z

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

mssalvatore · 2021-03-09T15:18:30Z

monkey/infection_monkey/post_breach/actions/users_custom_pba.py

 elif WormConfiguration.custom_PBA_windows_cmd:
 self.command = WormConfiguration.custom_PBA_windows_cmd

- def _execute_default(self):


Can you explain this change? It think it's not ideal to be downloading files from the island in the constructor if we can avoid it. Also, it makes sense to me that downloading the file is part of execution; I wouldn't expect the initialization of the PBA to fail if there were a network outage or other condition that prevented the download.

Makes sense; fixed

mssalvatore

@shreyamalviya Can you rework and include the tests from #1020?

VakarisZ · 2021-03-10T07:39:08Z

monkey/infection_monkey/post_breach/tests/actions/test_users_custom_pba.py

@@ -0,0 +1,152 @@
+import pytest


Unit tests are nice, but not a silver bullet. There's an antipattern in TDD called the "mockery", when most of the functionality is mocked. I don't suggest refactoring the whole pba's to solve it, but I can see BB tests bringing more value here and still being trivial enough to implement (command execution at least). Other PBA's should be tested with BB's IMO.

VakarisZ · 2021-03-10T07:49:26Z

monkey/monkey_island/cc/services/config_schema/monkey.py

+ "description": "File to be uploaded after breaching. "
+ "If you want the file to be executed, "
+ "specify it in the 'Linux post breach command' field. "


Suggested change

"description": "File to be uploaded after breaching. "

"If you want the file to be executed, "

"specify it in the 'Linux post breach command' field. "

"description": "File will be uploaded after breaching. "

"Use 'Linux post-breach command' field to "

"change permissions, run or delete the file."

"File will be uploaded" doesn't seem right as a description. "File to be uploaded" is what I would expect.

VakarisZ · 2021-03-10T07:54:04Z

monkey/monkey_island/cc/services/config_schema/monkey.py

+ "description": "File to be uploaded after breaching. "
+ "If you want the file to be executed, "
+ "specify it in the 'Windows post breach command' field. "


Suggested change

"description": "File to be uploaded after breaching. "

"If you want the file to be executed, "

"specify it in the 'Windows post breach command' field. "

"description": "File will be uploaded after breaching. "

"Use 'Windows post-breach command' field to "

"run or delete the file."

VakarisZ · 2021-03-10T07:54:12Z

monkey/monkey_island/cc/services/config_schema/monkey.py

@@ -35,9 +35,9 @@
 "title": "Windows post breach file",


Suggested change

"title": "Windows post breach file",

"title": "Windows post-breach file",

VakarisZ · 2021-03-10T07:54:38Z

monkey/monkey_island/cc/services/config_schema/monkey.py

@@ -20,9 +20,9 @@
 "title": "Linux post breach file",


Suggested change

"title": "Linux post breach file",

"title": "Linux post-breach file",

VakarisZ

We also need to change descriptions of PBA command fields. They should also contain examples. Linux description should be something like:
"Use this field to run custom commands or uploaded files on exploited machines. Example command chmod +x ./my_script.sh; ./my_script.sh ; rm ./my_script.sh"

…command

VakarisZ requested changes Mar 9, 2021

View reviewed changes

shreyamalviya force-pushed the rephrasing-config-custom-pba branch from 9399b86 to 4ea6ac1 Compare March 9, 2021 12:47

shreyamalviya requested review from VakarisZ and mssalvatore March 9, 2021 12:48

mssalvatore reviewed Mar 9, 2021

View reviewed changes

shreyamalviya force-pushed the rephrasing-config-custom-pba branch from 6e500f8 to 1020bd3 Compare March 10, 2021 07:16

shreyamalviya requested a review from mssalvatore March 10, 2021 07:20

VakarisZ reviewed Mar 10, 2021

View reviewed changes

VakarisZ requested changes Mar 10, 2021

View reviewed changes

shreyamalviya added 6 commits March 11, 2021 18:33

Rephrase custom PBA file descriptions in configuration

307e1e3

Only upload custom PBA file; execute only if specified in custom PBA …

eeba051

…command

Download custom PBA file during execution, not initialisation

6f134bd

Add unit tests

72a88c8

Unit test modifications

9167aa6

Rephrase custom PBA file config descriptions

4928109

shreyamalviya force-pushed the rephrasing-config-custom-pba branch from 1020bd3 to bf4555e Compare March 11, 2021 13:28

Rephrase custom PBA command config descriptions

2b4fd9e

shreyamalviya force-pushed the rephrasing-config-custom-pba branch from bf4555e to 2b4fd9e Compare March 11, 2021 14:10

mssalvatore merged commit 2a44cf8 into guardicore:release/1.10.0 Mar 11, 2021

mssalvatore mentioned this pull request Mar 11, 2021

Modify logic for user PBA command/file to allow both simultaneously #1020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rephrase custom PBA file descriptions in configuration#1027

Rephrase custom PBA file descriptions in configuration#1027
mssalvatore merged 7 commits intoguardicore:release/1.10.0from
shreyamalviya:rephrasing-config-custom-pba

shreyamalviya commented Mar 9, 2021

codecov bot commented Mar 9, 2021 •

edited

Loading

VakarisZ left a comment

ghost commented Mar 9, 2021 •

edited by ghost

Loading

mssalvatore Mar 9, 2021

shreyamalviya Mar 10, 2021

mssalvatore left a comment •

edited

Loading

VakarisZ Mar 10, 2021

VakarisZ Mar 10, 2021 •

edited

Loading

mssalvatore Mar 10, 2021

VakarisZ Mar 10, 2021

VakarisZ Mar 10, 2021

VakarisZ Mar 10, 2021

VakarisZ left a comment

Labels

3 participants

	"title": "Windows post breach file",
	"title": "Windows post-breach file",

	"title": "Linux post breach file",
	"title": "Linux post-breach file",

Conversation

shreyamalviya commented Mar 9, 2021

codecov bot commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

VakarisZ left a comment

Choose a reason for hiding this comment

ghost commented Mar 9, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepCode failed to analyze this pull request

mssalvatore Mar 9, 2021

Choose a reason for hiding this comment

shreyamalviya Mar 10, 2021

Choose a reason for hiding this comment

mssalvatore left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

VakarisZ Mar 10, 2021

Choose a reason for hiding this comment

VakarisZ Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mssalvatore Mar 10, 2021

Choose a reason for hiding this comment

VakarisZ Mar 10, 2021

Choose a reason for hiding this comment

VakarisZ Mar 10, 2021

Choose a reason for hiding this comment

VakarisZ Mar 10, 2021

Choose a reason for hiding this comment

VakarisZ left a comment

Choose a reason for hiding this comment

Labels

3 participants

codecov bot commented Mar 9, 2021 •

edited

Loading

ghost commented Mar 9, 2021 •

edited by ghost

Loading

mssalvatore left a comment •

edited

Loading

VakarisZ Mar 10, 2021 •

edited

Loading