Modify ATT&CK report messages for unscanned techniques #1525
Conversation
| properties = schema["properties"] | ||
| for prop in properties: | ||
| property_type = properties[prop]["title"] | ||
| for tab_name in properties[prop]["properties"]: | ||
| tab = properties[prop]["properties"][tab_name] | ||
| for config_option_name in tab["properties"]: | ||
| config_option = tab["properties"][config_option_name] | ||
| for attack_technique in config_option.get("related_attack_techniques", []): | ||
| # No config values could be a reason that related attack techniques are left | ||
| # unscanned. See https://github.com/guardicore/monkey/issues/1518 for more. | ||
| config_field = f"{config_option['title']} ({tab['title']})" | ||
| _add_config_field_to_reverse_schema( | ||
| property_type, config_field, attack_technique, reverse_schema | ||
| ) |
There was a problem hiding this comment.
So previously we only mapped definitions, but we should also map config properties? What happens if we add one more layer to config, like internal -> monkey -> behavior -> attemp_sudo? Will this break? Can hide the complexity of 4 nested loops at least behind a well-named function call that explains what this does?
There was a problem hiding this comment.
So previously we only mapped definitions, but we should also map config properties?
Yes, as mentioned in #1518, values in the internal config also affect if a technique was tried or not.
What happens if we add one more layer to config, like internal -> monkey -> behavior -> attemp_sudo? Will this break?
It won't break but it also won't show that field in the unscanned reasons in the report.
Can hide the complexity of 4 nested loops at least behind a well-named function call that explains what this does?
Will do.
| _crawl_config_schema_definitions_for_reverse_schema(schema, reverse_schema) | ||
| _crawl_config_schema_properties_for_reverse_schema(schema, reverse_schema) |
There was a problem hiding this comment.
We should avoid functions with output arguments. See Clean Code, page 45, "Output Arguments".
dae3c98 to
5e453d8
Compare
|
After 5e453d8: |
…oit_ntlm_hash_list' and 'exploit_lm_hash_list'
… unscanned attack techniques' reasons
…report generation
…e schema generation
…everse schema so it doesn't break when another level of nesting is added
1d479f8 to
ae6ebcf
Compare
| def __init__(self) -> None: | ||
| self.reverse_schema = {} |
There was a problem hiding this comment.
You don't really need this to be a class, because you can pass the reverse schema to self._crawl_config_schema_definitions_for_reverse_schema and return reverse schema from it. Same with _crawl_config_schema_properties_for_reverse_schema. The pro of that is that the call is more readable, it becomes
get_config_schema_per_attack_technique(SCHEMA) instead of ConfigSchemaPerAttackTechnique().get_config_schema_per_attack_technique(SCHEMA). The downside is obviously that you have to pass reverse_schema around. I think this could be a little bit more readable without the class, because you need to pass self, so why not pass reverse schema? But it's a minor thing, can be left as it is
There was a problem hiding this comment.
That's what I did initially, but as Mike pointed out, we shouldn't.
| # check for "properties" and each property's related techniques recursively; | ||
| # the levels of nesting and where related techniques are declared won't | ||
| # always be fixed in the config schema |
There was a problem hiding this comment.
I would remove this comment. It's obvious that this is a recursive call. I feel like this also expresses that new_config_option might have config options inside it, which explains why we need it.
There was a problem hiding this comment.
I think I'd like to keep it; this also makes it clear that related techniques don't have to be defined at the inner-most level of configuration options, they can be at any level.
What does this PR do?
Fixes #1518
PR Checklist
Was the documentation framework updated to reflect the changes?Testing Checklist