[Nmap] [NetBlock] ✨ introduce Nmap IP range scanning #58
Conversation
|
|
||
| for host in parsed.hosts: | ||
| yield from get_ports_and_service(host) | ||
| logging.info(f"Parsing {len(raw)} Nmap-xml(s) for {network}.") |
There was a problem hiding this comment.
Please use logging format instead (logging.info("Parsing %d ... for %s", len(raw), network) to be consistent with the rest of the codebase. (https://github.com/globality-corp/flake8-logging-format#violations-detected)
There was a problem hiding this comment.
Can you elaborate on this? G004 in your reference only holds for non-Python3.6+. Besides, I think the pre-commit setup currently requires flynt
There was a problem hiding this comment.
G004mentions that the first argument should not be pre-formattedflyntwill only warn about old-style formatting, it doesn't relate to the log functions using a format string as a first argument and (optionally) positional and keyword arguments to supply the format string- I doubted whether to post this link or not because lacks proper reasoning and motivation, but in general it's safer to do it this way and (sometimes, depending on configurations such as log levels) better performing (due to lazy loading instead of pre-formatting)
There was a problem hiding this comment.
So your proposal is to use:
logging.info("Parsing {n_xmls} Nmap-xml(s) for {network}.", extra={"n_xmls": len(raw), "network": network})?
I don't mind changing it, though can you motivate why this (or using the old-style formatting) would be safer?
There was a problem hiding this comment.
Yes, even simpler:
logging.info("Parsing %d Nmap-xml(s) for %s.", len(raw), network)I'd like to refer to https://bugs.python.org/issue46200. But what I find equally important is consistency and lazy loading/ delayed execution (e.g. there is no point of formatting a string if it's never rendered due to e.g. configuration). It also simplifies logging filters (a more advanced logging feature) if they have access to the log record's arguments, whereas it would be much more difficult if it had to work on an already formatted string.
There was a problem hiding this comment.
Well, I am not really convinced after reading through that issue. I think I will stay in the f-string camp for a while. Nevertheless, I did update the line with your suggestion :)
There was a problem hiding this comment.
I'm a huge fan of f-strings and not discouraging you to use them. But in this specific example, where safety is not the main argument, it's better to use the log formatting option rather than f-strings.
|
I couldn't find instances for |
I was also referring to the package name in the |
|
Fixed! I noticed however that IPs that are found by this boefje do not get the scan level of the IP range they are from, (or scan level -1 for that matter). The scan level seems to be 0 for IPs in a range that is indemnified up to scan level 2. I think it should be 2 or 1 in this case. |
@Lisser do you know more about this? |
We should adapt the Octopoes model to store the Netblock - IPAddress relation and assign a scanlevel cascade rule for this. Created this issue for this: |
This pull request proposes to introduce a boefje that is able to scan IP ranges and store IPs in those ranges with open ports in the graph. IPs in the ranges where no scanned ports are open will not be added to the graph. This also holds now for other boefjes utilizing the Nmap normalizer as the boefje uses the usual (modified) Nmap normalizer. It also updates the normalizer to be agnostic of
Networkobject.A side-effect is that IPs in a range with ports available will be scanned twice (once by this boefje, and then by the usual boefjes as soon as the individual IP is added to the graph). Therefore, this boefje does not use the Nmap service detection, as that is already covered in the usual Nmap boefje.
The ports that will be scanned can be set separately for UDP and TCP by using their respective
TOP_PORTS_<PROTOCOL>settings (which correspond to the Nmap--top-portsargument). Setting to0will skip the scanning for that protocol.Related discussion for which this pull request implements a solution: minvws/nl-kat-coordination#21