Report refactoring #1068
Report refactoring #1068
Conversation
…ts report component
| class ExploiterReportInfo: | ||
| machine: str | ||
| ip_address: str | ||
| type: str |
There was a problem hiding this comment.
Some processors extend this class, maybe should define separate dataclasses for each extension
There was a problem hiding this comment.
Dataclasses can be inherited freely so why not :)
DeepCode failed to analyze this pull requestSomething went wrong despite trying multiple times, sorry about that. |
| SMB = ExploiterDescriptor('SmbExploiter', 'SMB Exploiter', CredExploitProcessor) | ||
| WMI = ExploiterDescriptor('WmiExploiter', 'WMI Exploiter', CredExploitProcessor) | ||
| SSH = ExploiterDescriptor('SSHExploiter', 'SSH Exploiter', CredExploitProcessor) | ||
| SAMBACRY = ExploiterDescriptor('SambaCryExploiter', 'SambaCry Exploiter', CredExploitProcessor) | ||
| ELASTIC = ExploiterDescriptor('ElasticGroovyExploiter', 'Elastic Groovy Exploiter', ExploitProcessor) | ||
| MS08_067 = ExploiterDescriptor('Ms08_067_Exploiter', 'Conficker Exploiter', ExploitProcessor) | ||
| SHELLSHOCK = ExploiterDescriptor('ShellShockExploiter', 'ShellShock Exploiter', ShellShockExploitProcessor) | ||
| STRUTS2 = ExploiterDescriptor('Struts2Exploiter', 'Struts2 Exploiter', ExploitProcessor) | ||
| WEBLOGIC = ExploiterDescriptor('WebLogicExploiter', 'Oracle WebLogic Exploiter', ExploitProcessor) | ||
| HADOOP = ExploiterDescriptor('HadoopExploiter', 'Hadoop/Yarn Exploiter', ExploitProcessor) | ||
| MSSQL = ExploiterDescriptor('MSSQLExploiter', 'MSSQL Exploiter', ExploitProcessor) | ||
| VSFTPD = ExploiterDescriptor('VSFTPDExploiter', 'VSFTPD Backdoor Exploiter', CredExploitProcessor) | ||
| DRUPAL = ExploiterDescriptor('DrupalExploiter', 'Drupal Server Exploiter', ExploitProcessor) |
There was a problem hiding this comment.
The reason this isn't an actual link is because you don't want to include Monkey code in Island? Or why?
There was a problem hiding this comment.
Yes, because the island doesn't contain the agent's code.
| processor: Type[ExploitProcessor] | ||
|
|
||
|
|
||
| class ExploiterDescriptorEnum(Enum): |
There was a problem hiding this comment.
Why is this an enum? What functionality of enum are you using?
There was a problem hiding this comment.
Why not? What do you think would be a better data structure? I like enums over dicts, because when you want to get a specific value from dict, you need to make sure that your key is correct, because key is a hardcoded string.
| def build_exploiter_descriptor_dict() -> Dict[str, ExploiterDescriptor]: | ||
| descriptor_dict = {} | ||
| for descriptor in ExploiterDescriptorEnum: | ||
| descriptor_dict[descriptor.value.class_name] = descriptor | ||
| return descriptor_dict |
There was a problem hiding this comment.
Shouldn't this be in the enum class file? It's tightly coupled to it
| } | ||
| for exploited_node in exploited] | ||
|
|
||
| logger.info('Exploited nodes generated for reporting') | ||
|
|
||
| return exploited | ||
|
|
||
| @staticmethod | ||
| def get_exploits_used_on_node(node: dict) -> List[str]: | ||
| return list(set([exploit['exploiter'] for exploit in node['exploits'] if exploit['result']])) |
There was a problem hiding this comment.
Why the cast from list to set to list? Do you just want a cleaned up list? Do you really need it to be a list and not Iterable?
There was a problem hiding this comment.
Not sure why it was made this way, but I'd guess to remove duplicates in the list.
| @@ -677,8 +489,8 @@ def get_config_exploits(): | |||
| if exploits == default_exploits: | |||
| return ['default'] | |||
|
|
|||
| return [ReportService.EXPLOIT_DISPLAY_DICT[exploit] for exploit in | |||
| exploits] | |||
| # TODO investigate strange code | |||
| # TODO check if this actually works, because stolen passwords get added to config | ||
| # so any password will be in config. We need to separate stolen passwords from initial | ||
| # passwords in config. |
There was a problem hiding this comment.
This requires you to not just have a clean "setup" config but also a "pre run" config.
Wouldn't it be better to split the stolen creds from user creds and in the monkey have a property that merges them?
There was a problem hiding this comment.
Monkey probably registers stolen credentials when they are stolen. I tested and it works
| @staticmethod | ||
| def _is_weak_credential_issue(issue: dict, config_usernames: List[str], config_passwords: List[str]) -> bool: | ||
| # Only credential exploiter issues have 'credential_type' | ||
| return 'credential_type' in issue and \ |
There was a problem hiding this comment.
Wouldn't make more sense to clean this according to the type?
There was a problem hiding this comment.
What 'type'? You mean have a list of credential_type issues? I think that "if issue has credentials, then it's a credential issue" logic is good enough for this PR
| @@ -274,7 +274,7 @@ def get_exploits(): | |||
| for exploit in mongo.db.telemetry.aggregate(query): | |||
| new_exploit = ReportService.process_exploit(exploit) | |||
| if new_exploit not in exploits: | |||
| exploits.append(new_exploit) | |||
| exploits.append(new_exploit.__dict__) | |||
There was a problem hiding this comment.
Why? You have a wrapper function in dataclasses to cast to dict
There was a problem hiding this comment.
AFAIK that wrapper only calls factory function, which in our case is dict anyways. I see no reason to use that wrapper
| @@ -1,4 +1,5 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Shouldn't this entire commit be squashed into the original one?
There was a problem hiding this comment.
Why do you think that it should?
… exploit processors could add
What does this PR do?
Improves modularity in report issues and improves readability in report UI and back end.
Eases the process of new issue incorporation.
PR Checklist
Testing Checklist