1996 remove current server #2015

ilija-lazoroski · 2022-06-13T12:28:17Z

What does this PR do?

Fixes part of #1996 .

Add any further explanations here.

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?
Have you checked that you haven't introduced any duplicate code?

Testing Checklist

Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:

Tested by {Running the Monkey locally with relevant config/running Island/...}
If applicable, add screenshots or log transcripts of the feature working

Explain Changes

Are the commit messages enough? If not, elaborate.

codecov · 2022-06-13T12:32:12Z

Codecov Report

Merging #2015 (5be015b) into develop (3598b0d) will increase coverage by 0.02%.
The diff coverage is 47.25%.

❗ Current head 5be015b differs from pull request most recent head 5f8507c. Consider uploading reports for the commit 5f8507c to get more accurate results

@@             Coverage Diff             @@
##           develop    #2015      +/-   ##
===========================================
+ Coverage    56.63%   56.66%   +0.02%     
===========================================
  Files          463      464       +1     
  Lines        12894    12885       -9     
===========================================
- Hits          7303     7301       -2     
+ Misses        5591     5584       -7

Impacted Files	Coverage Δ
monkey/common/config_value_paths.py	`100.00% <ø> (ø)`
monkey/infection_monkey/cc/i_control_channel.py	`81.81% <ø> (ø)`
monkey/infection_monkey/monkey.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/cc/services/config.py	`86.58% <ø> (+0.01%)`	⬆️
...onkey_island/cc/services/config_schema/internal.py	`100.00% <ø> (ø)`
...s/unit_tests/monkey_island/cc/services/conftest.py	`81.81% <ø> (-18.19%)`	⬇️
monkey/infection_monkey/cc/control.py	`32.17% <42.10%> (ø)`
...land/cc/services/attack/technique_reports/T1065.py	`84.61% <50.00%> (+4.61%)`	⬆️
monkey/infection_monkey/cc/control_channel.py	`34.14% <75.00%> (ø)`
...ection_monkey/post_breach/custom_pba/custom_pba.py	`63.33% <75.00%> (ø)`
... and 9 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 3598b0d...5f8507c. Read the comment docs.

mssalvatore

Let's not create a new subpackage (cc) yet. When we merge the communication mechanisms, this subpackage won't provide any value.

monkey/infection_monkey/cc/control.py

monkey/infection_monkey/monkey.py

mssalvatore · 2022-06-13T13:53:36Z

monkey/infection_monkey/monkey.py

@@ -184,7 +180,11 @@ def _setup(self):
    def _build_master(self):
        local_network_interfaces = InfectionMonkey._get_local_network_interfaces()

-        control_channel = ControlChannel(self._default_server, GUID)
+        # TODO control_channel and control_client have same responsibilities, merge them


+1

It will be easier to merge them once we finish stripping out everything unnecessary from ControlClient and decoupling it from WormConfiguration

monkey/tests/unit_tests/infection_monkey/telemetry/conftest.py

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

This is done to refactor ControlClient from a global

* Workaround global class attribute

* It removes whole island_server section from internal config

* leftover from renaming credential_classes

mssalvatore

I've made some more comments, but the PR is too large and has at least two distinct goals. Split it up into 2 or more PRs, each of which accomplishes a single goal.

mssalvatore · 2022-06-13T16:42:19Z

monkey/infection_monkey/control.py

+class ControlClient:
+    # TODO When we have mechanism that support telemetry messenger
+    # with control clients, then this needs to be removed
+    # Ref: infection_monkey.telemetry.base_telem.py


Use a GitHub permalink instead of the file path. The file name or contents could change, which would make this comment less useful.

How do you get that permalink?

Navigate to a file in GitHub. Click a line. SHIFT+click on a line below to select a block. Click the ellipsis and select "copy permalink"

mssalvatore · 2022-06-13T16:49:14Z

monkey/tests/unit_tests/infection_monkey/telemetry/conftest.py

@@ -11,5 +13,6 @@ def _spy_send_telemetry(telem_category, data):

    _spy_send_telemetry.telem_category = None
    _spy_send_telemetry.data = None
-    monkeypatch.setattr(ControlClient, "send_telemetry", _spy_send_telemetry)
+    ControlClient.control_client_object = MagicMock()
+    monkeypatch.setattr(ControlClient.control_client_object, "send_telemetry", _spy_send_telemetry)


Suggested change

monkeypatch.setattr(ControlClient.control_client_object, "send_telemetry", _spy_send_telemetry)

ControlClient.control_client_object.send_telemetry = MagicMock(side_effect=_spy_send_telemetry)

What's the difference? Why do you prefer it that way?

https://stackoverflow.com/a/47101194 Not really some difference between.

That answer doesn't have anything to do with why I prefer using the MagicMock here.

If you monkey patch the object, then the object still has the capability to do all sorts of things. Monkey patching send_telemetry() but leaving all of the other methods intact could result in unexpected behavior down the road if the code decides to try and use any of those methods.

Using a mock object, we're sure it only does the things we've programmed it to do within the confines of what we want to test.

monkey/monkey_island/cc/services/attack/technique_reports/T1065.py

monkey/monkey_island/cc/resources/local_ips.py

VakarisZ · 2022-06-14T08:00:53Z

Let's not create a new subpackage (cc) yet. When we merge the communication mechanisms, this subpackage won't provide any value.

I disagree. It makes it easier to find the files that have the same responsibility until then and it costs nothing to move them back. I'm not sure why you say that it won't provide any value once we merge the control_client and control_channel, because if we do it in extendable way this package will contain more than one file

VakarisZ · 2022-06-14T09:38:55Z

Split up the PR. First part is in #2016

mssalvatore · 2022-06-14T12:06:24Z

Let's not create a new subpackage (cc) yet. When we merge the communication mechanisms, this subpackage won't provide any value.

I disagree. It makes it easier to find the files that have the same responsibility until then and it costs nothing to move them back. I'm not sure why you say that it won't provide any value once we merge the control_client and control_channel, because if we do it in extendable way this package will contain more than one file

@VakarisZ It may contain more than one file, or it may not. As we simplify the API on the Island, the agent should also become simpler. Adding layers in a hierarchy can add difficulty, so unless there's a distinct benefit to do it, it should be avoided.

In this specific case, it introduces a circular dependency between the parent and child packages. infection_monkey depends on infection_monkey.cc since main.py depends on control.py. But infection_monkey.cc.control.py depends on infection_monkey.tunnel, creating a circular dependency between infection_monkey and infection_monkey.cc.

ilija-lazoroski requested review from mssalvatore and VakarisZ June 13, 2022 12:28

mssalvatore requested changes Jun 13, 2022

View reviewed changes

VakarisZ and others added 15 commits June 13, 2022 17:46

Agent: Add control client to the agent initialization

44890b9

Agent: Initialize CustomPBA with a ControlClient object

40703df

This is done to refactor ControlClient from a global

UT: Fix control client and custom pba tests

5539b77

Agent: Add control client proxies to tunnel telem initialization

20a6272

Agent: Add proxies to the initialization of ControlChannel

a797690

Agent: Add control_client_object to ControlClient

e344cf1

* Workaround global class attribute

Agent: Remove current_server and command_servers from WormConfiguration

3248a66

Agent: Remove current_server option from custom_pba

54d78e9

Island: Add local_ips resource

9585973

Island: Remove current_server and command_servers from config

587c590

* It removes whole island_server section from internal config

UI: Use local_ips endpoint to get command_servers

fa37475

UT: Fix island config tests

a566a7f

UT: Use MagicMock instead of ControlClient actual instance

9af0d8b

Agent: Rename cc_client to control_client

25db865

UT: Fix control client location

5631eb8

ilija-lazoroski force-pushed the 1996-remove-current-server branch from 5be015b to 5631eb8 Compare June 13, 2022 15:52

ilija-lazoroski added 3 commits June 13, 2022 17:55

Agent: Fix control_client_object TODOs

cc068e3

UI: Remove testing from InternalConfig

50fdd8a

* leftover from renaming credential_classes

Agent: Remove unused control channel from build_master

5f8507c

mssalvatore requested changes Jun 13, 2022

View reviewed changes

VakarisZ closed this Jun 14, 2022

mssalvatore deleted the 1996-remove-current-server branch June 16, 2022 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1996 remove current server #2015

1996 remove current server #2015

ilija-lazoroski commented Jun 13, 2022

codecov bot commented Jun 13, 2022 •

edited

Loading

mssalvatore left a comment

mssalvatore Jun 13, 2022

mssalvatore left a comment

mssalvatore Jun 13, 2022

VakarisZ Jun 14, 2022

mssalvatore Jun 14, 2022

mssalvatore Jun 13, 2022

VakarisZ Jun 14, 2022

ilija-lazoroski Jun 14, 2022

mssalvatore Jun 14, 2022

VakarisZ commented Jun 14, 2022

VakarisZ commented Jun 14, 2022

mssalvatore commented Jun 14, 2022

	monkeypatch.setattr(ControlClient.control_client_object, "send_telemetry", _spy_send_telemetry)
	ControlClient.control_client_object.send_telemetry = MagicMock(side_effect=_spy_send_telemetry)

1996 remove current server #2015

1996 remove current server #2015

Conversation

ilija-lazoroski commented Jun 13, 2022

What does this PR do?

PR Checklist

Testing Checklist

Explain Changes

codecov bot commented Jun 13, 2022 • edited Loading

Codecov Report

mssalvatore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssalvatore left a 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

Choose a reason for hiding this comment

VakarisZ commented Jun 14, 2022

VakarisZ commented Jun 14, 2022

mssalvatore commented Jun 14, 2022

codecov bot commented Jun 13, 2022 •

edited

Loading