Some structure for the simple STIX to MISP filter - mainly cosmetics :-) #27
Conversation
There was a problem hiding this comment.
Thanks for the PR.
There are quite a few unnecessary stylistic changes here that go against the rest of the project, I won't be accepting on those grounds.
I like to keep a script as being a script - you should be able to read it like, well, a (theatrical) script, top to bottom. This jumps around a lot. Whilst in practice they execute the same, I personally can't stand it.
misp_stix_converter/stix-to-misp.py
Outdated
| MISP_CONFIG_KEY = "MISP" | ||
| SSL_CONFIG_KEY = "SSL" | ||
| BLAST_FROM_PAST_PENALTY_SECS = 1.0 | ||
| args_src = sys.argv |
There was a problem hiding this comment.
I mean... for what reason?
There was a problem hiding this comment.
the filename is bad for testing, as you cannot import it with the hyphens without tricks, but at least offer some testability, where you could import the "script" as module, and depending on test idea, overwrite the args_src module level var and call main with the list (you can always use unittest.mock, but ... or one can directly call parse_args(["some", "list"]) at least with the fix I echo pushed (because I forgot to inject into the parser.parse_args call.
misp_stix_converter/stix-to-misp.py
Outdated
| log.info("Pushing to MISP...") | ||
| MISP.push(pkg) | ||
| log.info("COMPLETE") | ||
| if __name__ == '__main__': |
There was a problem hiding this comment.
This is an executable.
The only time we'd ever run this is when __name__ is "main".
Check not needed.
There was a problem hiding this comment.
Ah, yes, but the coverage is so "low" (of that project) and coveralls even suggested that by adding lines, this has lowered the coverage, so having a shield against exec might come in handy, when executing tests, that import the file content as module ...
There was a problem hiding this comment.
... of course the sys.exit(main)was not my brightest idea, but there came a new commit amending the branch (I generated this before reading the above comments).
misp_stix_converter/stix-to-misp.py
Outdated
| CONFIG_DEFAULT_PATH = "~/.misptostix/misp.login" | ||
| MISP_CONFIG_KEY = "MISP" | ||
| SSL_CONFIG_KEY = "SSL" | ||
| BLAST_FROM_PAST_PENALTY_SECS = 1.0 |
There was a problem hiding this comment.
This tells you absolutely nothing about what its actually used for
There was a problem hiding this comment.
Again, just a suggestion to un-lit(t)er(al) the code, replaced only multiple copies of string literals or that funky 1 as time.sleep argument with the reverse guessed meaning. The config path with the tilde that requires expand user pre-action for usable open file path creation, directly came from my fingers ...
misp_stix_converter/stix-to-misp.py
Outdated
| from misp_stix_converter.converters.convert import load_stix | ||
| from misp_stix_converter.servers import misp | ||
|
|
||
| LOGGER_FORMAT = '%(asctime)s - %(name)s - %(levelname)s - %(message)s' |
There was a problem hiding this comment.
Kinda for lines 15-20
If they're constants, why bother defining them beforehand when they can be passed straight to a function without adding more lines?
It executes the same but still, seems unneeded
There was a problem hiding this comment.
Is of course individual perception, but in my experience, complexity is not in the number of lines. A logger format is some public interface, and might ease adaption and use, if made visible on module level.
misp_stix_converter/stix-to-misp.py
Outdated
| # TO MISP # | ||
| ########################## | ||
|
|
||
| """Wrapper script providing STIX to MISP conversion. """ |
There was a problem hiding this comment.
Why did you change this?
It's consistent throughout the project.
There was a problem hiding this comment.
Ok, interesting, newbie from planet python - thought that one liner would convey more meaning and suit pep8
misp_stix_converter/stix-to-misp.py
Outdated
| MISP.push(pkg) | ||
| log.info("COMPLETE") | ||
| if __name__ == '__main__': | ||
| sys.exit(main) |
There was a problem hiding this comment.
misp_stix_converter/stix-to-misp.py
Outdated
| config[MISP_CONFIG_KEY]["KEY"], | ||
| config[MISP_CONFIG_KEY].get(SSL_CONFIG_KEY, True) | ||
| ) | ||
| planet_misp = misp.MISP(*misp_args) |
There was a problem hiding this comment.
This genuinely makes less sense now
f("a", "b", "c")is understood more quickly than
x = ["a", "b", "c"]
f(*x)There was a problem hiding this comment.
Well, it is just an offering, but I was first irritated by the "defensive" get on the SSL key lookup nearly hidden as third argument, so to me that made it more clear (also as the default of get is explicitly given). That is in your sampl, it would be a more complicated "c"
misp_stix_converter/stix-to-misp.py
Outdated
| return log | ||
|
|
||
|
|
||
| def parse_args(args): |
There was a problem hiding this comment.
argparse does not need to be passed sys.argv
There was a problem hiding this comment.
Cf. above, but it profits for test from receiving an explicit list ...
…Hashes as some hash algos require additional length
|
@FloatingGhost - I think the latest changes accommodate to your review change requests. If not please indicate to me, only if you have time, of course. Thanks. |
|
@FloatingGhost are you cool with the changes? |
Just a first communication attempt - hi :-)
Did not setup a travis / CI chain for the fork ... but maybe you guys find it helpful.