Feature/support subnet in config #94
Conversation
itaymmguardicore
added
monkey
Feature
network now scans several range classes according to config
There was a problem hiding this comment.
Few notes:
I think I want the common code as a separate PR. Reason is if we're doing this there's more code that can easily be lifted out (network/info, can also pull config for a server side validation of config!) and I really don't like what it does to the .spec file.
Didn't we also want to avoid naming the file range.py?
Otherwise, mostly LGTM.
common/network/range.py
Outdated
| self._shuffle = shuffle | ||
|
|
||
| def get_range(self): | ||
| return [x for x in self._get_range() if (x & 0xFF != 0)] # remove broadcast ips |
There was a problem hiding this comment.
That's broadcast only if we're in a class C subnet. If we're in any other network, this is the wrong lookup.
If we're working with a CidrRange, we should use the ipaddress module to know if this is a broadcast, otherwise we can't know.
There was a problem hiding this comment.
I'm removing this and adding a filter to CidrRange.
common/network/range.py
Outdated
| @@ -0,0 +1,113 @@ | |||
| import random | |||
There was a problem hiding this comment.
Didn't we want to change the name of the file anyway?
common/network/range.py
Outdated
| class CidrRange(NetworkRange): | ||
| def __init__(self, cidr_range, shuffle=True): | ||
| super(CidrRange, self).__init__(shuffle=shuffle) | ||
| self._cidr_range = cidr_range.strip() |
There was a problem hiding this comment.
We already stripped the string in get_range_object or are you assuming someone might explicitly instance CidrRange?
There was a problem hiding this comment.
Someone might explicitly instance CidrRange
common/network/range.py
Outdated
| import struct | ||
| from abc import ABCMeta, abstractmethod | ||
|
|
||
| import ipaddress |
There was a problem hiding this comment.
For some reason, ipaddress does not show up in https://github.com/guardicore/monkey/blob/feature/support-subnet-in-config/infection_monkey/requirements.txt
common/network/range.py
Outdated
| yield self._number_to_ip(x) | ||
|
|
||
| @abstractmethod | ||
| def is_in_range(self, ip_address): |
There was a problem hiding this comment.
Currently this is unused, any particular reason we want this?
There was a problem hiding this comment.
Will be in use in detect-cross-segment-traffic
infection_monkey/network/info.py
Outdated
| ip_interface = ipaddress.ip_interface(u"%s/24" % address_str) | ||
| addrs = [str(addr) for addr in ip_interface.network.hosts() if addr != host_address] | ||
| res.extend(addrs) | ||
| res.append(CidrRange(cidr_range="%s/24" % (address_str, ))) |
There was a problem hiding this comment.
Any reason we want to keep the class C limit?
There was a problem hiding this comment.
Was there previously. Logic says more than that is too much.
Your call whether to remove the limit.
There was a problem hiding this comment.
I'd rather remove these limits soon, or make it configurable. But that depends on what we think is realistic. @ofriz ?
# Conflicts: # monkey_island/cc/services/report.py
Feature
Have you added an explanation of what your changes do and why you'd like to include them?
Have you successfully tested your changes locally?
Example screenshot/log transcript of the feature working
2018-02-20 16:13:21,108 [19636:DEBUG] network_scanner.get_victim_machines.52: Scanning for potential victims in the network <FixedRange 192.168.1.34/30,192.168.2.50-192.168.2.54>Changes