Support for imperative config adding and maco-model standard #101
Conversation
psrok1
force-pushed
the
feature/maco-model-support
branch
from
cd6497e
to
4508e36
Compare
psrok1
force-pushed
the
refactor/malduck-extractor
branch
from
1a8482b
to
ed8c86f
Compare
psrok1
force-pushed
the
feature/maco-model-support
branch
from
49d9936
to
675e579
Compare
| max_size=max_size, | ||
| usage=usage, | ||
| ) | ||
| return self.push_config(dict(http=[http])) |
There was a problem hiding this comment.
not compatible with ioc_extractor - should we add support to ioc_extractor for this, or ensure mwdb generates configs understood by ioc_extractor?
| port=port, | ||
| usage=usage, | ||
| ) | ||
| return self.push_config(dict(ssh=[ssh])) |
There was a problem hiding this comment.
won't repeat myself everywhere, but we can think about supporting all useful fields from here in mwdb_iocextract (or supporting mwdb_iocextract from here).
|
|
||
| def add_ftp( | ||
| self, | ||
| username: Optional[str] = None, |
There was a problem hiding this comment.
is everything really optional? (here and below) at least hostname sound important i guess
There was a problem hiding this comment.
I blindly folllowed Maco scheme there, but yeah, if we don't have alternative like ip vs domain, we should treat hostname as required.
malduck/extractor/config_builder.py
Outdated
|
|
||
| :param others: Dictionary with other configuration fields | ||
| """ | ||
| return self.push_config(dict(others=others)) |
There was a problem hiding this comment.
Why do we add a special magic key called others? Would this work if we just did this instead:
| return self.push_config(dict(others=others)) | |
| return self.push_config(others) |
(of course there are some edge cases, like add_other({"tcp": 3}), but maybe it's a good tradeoff vs magic others key).
There was a problem hiding this comment.
Actually it's support for other collection that is meant to contain anything extra, not included in the scheme itself: https://github.com/CybercentreCanada/Maco/blob/master/maco/model/model.py#L200.
And yeah, I see my mistake: the key is other, not others
Maco explicitly bans extra fields: https://github.com/CybercentreCanada/Maco/blob/master/maco/model/model.py#L12
| json.dumps(config) | ||
| except (TypeError, OverflowError) as e: | ||
| log.debug("Config is not JSON-encodable (%s): %s", str(e), repr(config)) | ||
| raise RuntimeError("Config must be JSON-encodable") | ||
|
|
||
| config = sanitize_config(config) |
There was a problem hiding this comment.
what happens if jsonable is false and config was never json-encoded here? Won't this just crash later? 🤔
There was a problem hiding this comment.
That jsonable field is added because I need to somehow support the binaries that contain bytes. CCCS also noted that it's not JSON-compatible https://github.com/CybercentreCanada/Maco/blob/master/maco/model/model.py#L214.
jsonable is only checked for configuration part, so it won't crash later in Extractor itself but may crash during further processing of configurations (e.g. malduck extract prints JSONs so we need some extra processing there)
jsonable sanity check was made to correct mistakes done by extractor writers. But if we follow the Maco scheme, we can trust that type-checking will help in spotting that kind of errors and bytes will be only under binaries key that can be treated specially.
There was a problem hiding this comment.
In Assemblyline, we've base64'd any bytes using a custom encoder (Base64Encoder) when dropping files to be shown in the analysis.
Not sure if you guys want to go the same route here, but thought I should mention it 😅
No description provided.