Mimikatz dll to pypykatz refactor #678
Conversation
… and docs updated
Codecov Report
@@ Coverage Diff @@
## develop #678 +/- ##
===========================================
- Coverage 56.62% 56.49% -0.14%
===========================================
Files 120 135 +15
Lines 4044 4381 +337
===========================================
+ Hits 2290 2475 +185
- Misses 1754 1906 +152
Continue to review full report at Codecov.
|
| <div style={{position: 'relative' /*, height: '80vh'*/}}> | ||
| {/*Disable PTH map until we fix it | ||
| this.generateReportPthMap()*/} | ||
| </div> |
There was a problem hiding this comment.
No need to comment out code. Delete, git remembers
There was a problem hiding this comment.
I'm still not sure what's the best routine for this type of code. Just deleting and forcing future developer to search for this isn't the best solution. When future developer will try to implement pth map, he might not even be aware that this code existed.
| def get_mimikatz_info(self): | ||
| mimikatz_collector = MimikatzCollector() | ||
| mimikatz_info = mimikatz_collector.get_logon_info() | ||
| if mimikatz_info: | ||
| if "credentials" in self.info: | ||
| self.info["credentials"].update(mimikatz_info) | ||
| self.info["mimikatz"] = mimikatz_collector.get_mimikatz_text() | ||
| LOG.info('Mimikatz info gathered successfully') | ||
| else: | ||
| LOG.info('No mimikatz info was gathered') | ||
| LOG.info("Gathering mimikatz info") | ||
| try: | ||
| credentials = WindowsCredentialCollector.get_creds() | ||
| if credentials: | ||
| if "credentials" in self.info: | ||
| self.info["credentials"].update(credentials) | ||
| self.info["mimikatz"] = credentials | ||
| LOG.info('Mimikatz info gathered successfully') | ||
| else: | ||
| LOG.info('No mimikatz info was gathered') | ||
| except Exception as e: | ||
| LOG.info(f"Pypykatz failed: {e}") |
There was a problem hiding this comment.
Replace Mimikatz with pypykatz (or with a generic name that's not tied to a specific implementation) in the name, log, and info field name :)
There was a problem hiding this comment.
Improving naming consistency is a valid point. Regarding renaming mimikatz to pypykatz - on high level it makes most sense to keep mimikatz, because it's well known and it's purpose is clear. On caller level (actual commented code) we don't care how mimikatz cred gathering is implemented, via dll or some kind of python module.
| from typing import Dict | ||
|
|
||
|
|
||
| class WindowsCredential: |
There was a problem hiding this comment.
Credentials in considered a mass noun, so I think this class should be renamed to WindowsCredentialsSet (and WindowsCredentialCollector -> WindowsCredential**s**Collector)
There was a problem hiding this comment.
Hmmm... Credential (singular) is a valid word (not like scissors for example). In terms of English, I agree. In terms of programming, a class or an object should be in singular form, plural implies an iterable.
| for test_dict in test_dicts: | ||
| self.assertTrue(test_dict in results) |
There was a problem hiding this comment.
I might be mistaken but I think you can do this with assert set(test_dicts) <= set(results) which might be more efficient. Not critical at all, just a nice Pythonic way to do it 🐍
There was a problem hiding this comment.
Dicts are not hashable, so native dict can't be used in set. I converted it to even more pythonic way though :)
| def cred_list_to_cred_dict(creds: List[WindowsCredential]): | ||
| cred_dict = {} | ||
| for cred in creds: | ||
| # Lets not use "." and "$" in keys, because it will confuse mongo. |
There was a problem hiding this comment.
Is this still true?
starting in MongoDB 3.6, the server permits storage of field names that contain dots (i.e. .) and dollar signs (i.e. $)
From Mongo reference
What is this?
Fixes #471 #583
Replaces mimikatz binary with pypykatz python module.
Hides credential map until we fix it.
Checklist