-
Notifications
You must be signed in to change notification settings - Fork 786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Agent: Change the Agent to use the modified exploiter config #2807
Agent: Change the Agent to use the modified exploiter config #2807
Conversation
2278b83
to
94e1f51
Compare
@@ -121,16 +117,15 @@ def _exploit_hosts_on_queue( | |||
|
|||
def _run_all_exploiters( | |||
self, | |||
exploiters_to_run: Sequence[PluginConfiguration], | |||
exploiter_configs: Mapping[ExploiterName, Mapping], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure about changing the sequence to Dict, but I think dict is more flexible and it's the responsibility of this method to determine the order based on configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Dict
will preserve order. In this case, I think we should type hint with Dict
instead of Mapping
, as Mappings
may not preserve order. Better yet, we should, perhaps, used OrderedDict
to signal that order matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the exploiters are plugins but don't use PluginConfiguration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PluginConfiguration
will likely go away at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with Dict
, because I ran into issues trying to used OrderedDict type hint. We can make it "ordered" later, we don't need the extra method it provides
94e1f51
to
730ce6b
Compare
for exploiter in exploitation_config.exploiters: | ||
) -> Dict[ExploiterName, Dict]: | ||
extended_configs = {} | ||
for exploiter, plugin_options in exploitation_config.exploiters.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid the word "plugin" in the master. There's really no reason this component needs to be aware of plugins. It receives a config from C&C and orders the puppet to take action. "exploiter_options" would be better.
target_host: TargetHost, | ||
current_depth: int, | ||
servers: Sequence[str], | ||
results_callback: Callback, | ||
stop: Event, | ||
): | ||
|
||
for exploiter in interruptible_iter(exploiters_to_run, stop): | ||
exploiter_name = exploiter.name | ||
for exploiter_name in interruptible_iter(exploiter_configs, stop): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could maybe do
for exploiter_name in interruptible_iter(exploiter_configs, stop): | |
for exploiter_name, exploiter_config in interruptible_iter(exploiter_configs.items(), stop): |
Then you wouldn't need the lookup on line 143. But this is a minor nit.
730ce6b
to
8cfdcb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but let's add a comment stating that order is important and a dict
preserves order.
What does this PR do?
Addresses #2785 (changes agent to be compatible with configuration changes)
Improved naming a bit
PR Checklist
Testing Checklist