1960 deserialize config #2043

mssalvatore · 2022-06-24T18:26:58Z

What does this PR do?

Use the AgentConfiguration object in the agent.

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 unit tests
Tested by running ETE tests
~~if applicable, add screenshots or log transcripts of the feature working~~

Creating AgentConfiguration object from dictionary makes sense because it doesn't couple the configuration to any specific serialization methods. Also, the json sent from the island doesn't match the config structure because it stores config in a dict under "config" key.

"supported_os" was removed from the schema in d079d74

The old config scheme stored timeouts as milliseconds, whereas the new one uses seconds. Seconds are more convenient because most python methods expecting timeouts are expecting floating-point seconds.

The variable name "os" conflicts with the name of Python's `os` library.

…tion Agent configuration construction

monkey/infection_monkey/master/automated_master.py

shreyamalviya · 2022-06-24T22:08:15Z

monkey/common/utils/exceptions.py

+# TODO: This has been replaced by common.configuration.InvalidConfigurationError. Use that error
+#       instead and remove this one.


Not in this PR?

No, it's out of scope. Other components are still using the old ConfigService and other components that still use this error. The TODO is here because vulture can't tell which one is being used, so once everything is switched over to the new error, we don't want to forget to remove this one.

shreyamalviya · 2022-06-24T22:23:12Z

monkey/common/configuration/__init__.py

+from .agent_sub_configurations import (
+    CustomPBAConfiguration,
+    PluginConfiguration,
+    ScanTargetConfiguration,
+    ICMPScanConfiguration,
+    TCPScanConfiguration,
+    NetworkScanConfiguration,
+    ExploitationOptionsConfiguration,
+    ExploiterConfiguration,
+    ExploitationConfiguration,
+    PropagationConfiguration,


Why do we have both PluginConfiguration and ExploiterConfiguration dataclasses which have the same member variables (link to file with definitions)?

Good question! ExploiterConfiguration had supported_os in it, until that was removed by #2038. When supported_os was removed, everything that used ExploiterConfiguration should have been changed to PluginConfiguration. I'll add a task to #1960.

Typehint was Mapping, when it was using and calling other methods with CustomPBAConfiguration

VakarisZ · 2022-06-27T08:06:36Z

monkey/common/configuration/agent_configuration.py

+        # makes it impossible to construct an invalid object
+        try:
+            AgentConfigurationSchema().dump(self)
+        except Exception as err:


Are we expecting anything but MarshmallowError?

That I'm aware, it can raise MarshmallowError, TypeError, or ValueError. It may raise others as well, I'm not sure.

A better question is, "Is there any case where AgentConfigurationSchema().dump() could raise an exception for any reason other than an invalid exception?" To my knowledge, there's not.

VakarisZ · 2022-06-27T08:15:57Z

monkey/common/configuration/default_agent_configuration.py

@@ -204,5 +204,4 @@


 def build_default_agent_configuration() -> AgentConfiguration:
-    schema = AgentConfigurationSchema()
-    return schema.loads(DEFAULT_AGENT_CONFIGURATION_JSON)
+    return AgentConfiguration.from_json(DEFAULT_AGENT_CONFIGURATION_JSON)


We should probably specify the defaults in the marshmallow schema. We'll do it with validation, right?

Yeah, #2004.

Move line 60 to f formatting from the old %s style

VakarisZ · 2022-06-27T08:34:46Z

monkey/infection_monkey/master/propagator.py

+        self, target_config: ScanTargetConfiguration
+    ) -> List[NetworkAddress]:
+        ranges_to_scan = target_config.subnets
+        inaccessible_subnets = target_config.inaccessible_subnets


What is the purpose of these variables? I'd rather we just use keyword arguments in compile_scan_target_list call and pass them directly

It keeps _scan_network() a little shorter and easier to read. That was probably more important when we were using a dict. I could go either way. Feel free to fix it.

monkey/tests/data_for_tests/monkey_configs/default_config.py

VakarisZ · 2022-06-27T09:13:12Z

monkey/tests/unit_tests/common/configuration/test_agent_configuration.py

+        AgentConfiguration.from_json(json.dumps(invalid_dict))
+
+
+def test_to_json():


Maybe it's worth considering merging the serialization tests. Not only because from_json and to_json will be used in tandem, but also to avoid the maintenance of default configuration in json format

I added DEFAULT_AGENT_CONFIGURATION and removed DEFAULT_AGENT_CONFIGURATION_JSON

VakarisZ

Approved, but left some comments

It's easier to maintain object than a JSON string for the default configuration.

VakarisZ and others added 30 commits June 23, 2022 14:47

Agent: Fix configuration retrieval in _run_simulation

d8ac441

UT: Add a new fixture for config object

e83995d

Agent: Change configuration to object in control channel

6b406ef

Agent: Change scanners to use the config object

ffe8c34

Agent: Use deserialized config in automated_master.py

095e49b

Agent: Use deserialized in exploiter.py and propagator.py

9286e86

Agent: Usa agent config object instead of dict in option_parsing.py

86ed174

Agent: Usa agent config object instead of dict automated_master.py

ab67853

Agent: Usa agent config object instead of dict should_propagate

0f848eb

Agent: Remove redundant call to control_channel.get_config()

aff5423

Agent: Add missing return type hint to _try_communicate_with_island()

bba7139

UT: Remove supported_os from default_config.py

6e951ed

"supported_os" was removed from the schema in d079d74

Agent: Remove print() that was added by mistake

81d3300

UT: Fix erroneously abbreviated fixture

bff92ed

Agent: Remove unnecessary parameter

5a95aef

UT: Use AgentConfiguration in test_propagation.py

afeca66

Agent: Decouple should_propagate() and AgentConfiguration

ad0f694

Agent: Rename should_propagate -> maximum_depth_reached

05f640d

Island: Return config timeouts in seconds

6d156b8

The old config scheme stored timeouts as milliseconds, whereas the new one uses seconds. Seconds are more convenient because most python methods expecting timeouts are expecting floating-point seconds.

Agent: Fix running of payloads

2ff2e5f

Agent: Use == to compare OperatingSystems enum

f9445a2

Agent: Use OperatingSystems in CachingAgentRepository

d59dd81

Agent: Use OperatingSystems.value for urllib.parse.quote

fb67586

Agent: Use == to compare OperatingSystems enum in Log4Shell

b605f16

Agent: Use OperatingSystem.WINDOWS in Powershell

e1d5d25

Agent: Move enum to string conversion to _download_binary_from_island()

ffd3464

Agent: Rename os -> operating_system in caching_agent_repository

858eb23

The variable name "os" conflicts with the name of Python's `os` library.

Agent: Use operating_system.value in _download_binary_from_island()

2eb1691

Common: Add a docstring to OperatingSystems

a3db414

mssalvatore added 2 commits June 24, 2022 13:30

Common: Add docstrings to static methods in AgentConfiguration

568eb4f

Merge pull request #2041 from guardicore/agent-configuration-construc…

93ed7cf

…tion Agent configuration construction

mssalvatore requested a review from ilija-lazoroski June 24, 2022 18:26

mssalvatore added 6 commits June 24, 2022 14:47

UT: Remove unused imports from conftest.py

84fc78c

UT: Fix line that was too long

b219ca0

Agent: Log configuration when it's received from the Island

33ec4f7

Agent: Use Iterable instead of List in type hint

dc9b91d

Agent: Remove unnecessary local variables

8886ebc

Agent: Use PluginConfiguration in _run_payload()

4f7d8be

shreyamalviya reviewed Jun 24, 2022

View reviewed changes

mssalvatore added 2 commits June 24, 2022 19:19

Agent: Use Sequence instead of List for type hints

503a0a8

Agent: Use Mapping instead of Dict

fefd2da

mssalvatore requested a review from VakarisZ June 25, 2022 00:33

VakarisZ added 3 commits June 27, 2022 09:36

Agent: Fix typehints in _run_pbas of automated_master.py

4b7ab05

Typehint was Mapping, when it was using and calling other methods with CustomPBAConfiguration

Agent: Fix typehints in clear_command_history.py

7179f91

Agent: Fix _filter_none_values to be a static method

c080f03

VakarisZ reviewed Jun 27, 2022

View reviewed changes

Agent: Fix string formatting in http_tools.py

232d6ba

Move line 60 to f formatting from the old %s style

VakarisZ reviewed Jun 27, 2022

View reviewed changes

monkey/tests/data_for_tests/monkey_configs/default_config.py Outdated Show resolved Hide resolved

VakarisZ reviewed Jun 27, 2022

View reviewed changes

VakarisZ approved these changes Jun 27, 2022

View reviewed changes

mssalvatore added 2 commits June 27, 2022 06:56

UT: Remove disused DEFAULT_CONFIG

bf1d360

UT: Remove dependency on DEFAULT_AGENT_CONFIGURATION_JSON

90259c1

VakarisZ mentioned this pull request Jun 27, 2022

1960 ExploiterConfig to PluginConfig #2044

Merged

6 tasks

Common: Remove DEFAULT_AGENT_CONFIGURATION_JSON

e6d3854

It's easier to maintain object than a JSON string for the default configuration.

VakarisZ approved these changes Jun 27, 2022

View reviewed changes

mssalvatore merged commit 44a6197 into 1960-configuration-object Jun 27, 2022

mssalvatore deleted the 1960-deserialize-config branch June 27, 2022 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1960 deserialize config #2043

1960 deserialize config #2043

mssalvatore commented Jun 24, 2022 •

edited

Loading

shreyamalviya Jun 24, 2022

mssalvatore Jun 24, 2022

shreyamalviya Jun 24, 2022

mssalvatore Jun 24, 2022

VakarisZ Jun 27, 2022

mssalvatore Jun 27, 2022

mssalvatore Jun 27, 2022

VakarisZ Jun 27, 2022

mssalvatore Jun 27, 2022

VakarisZ Jun 27, 2022

mssalvatore Jun 27, 2022

VakarisZ Jun 27, 2022

mssalvatore Jun 27, 2022

VakarisZ left a comment

		# TODO: This has been replaced by common.configuration.InvalidConfigurationError. Use that error
		# instead and remove this one.

		AgentConfiguration.from_json(json.dumps(invalid_dict))


		def test_to_json():

1960 deserialize config #2043

1960 deserialize config #2043

Conversation

mssalvatore commented Jun 24, 2022 • edited Loading

What does this PR do?

PR Checklist

Testing Checklist

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

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

mssalvatore commented Jun 24, 2022 •

edited

Loading