From bba34d7920e4e215e648e496504d1839e7dc52b9 Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Tue, 29 Aug 2023 20:20:17 -0500 Subject: [PATCH 01/10] Some suggestions for better IPv6 support --- zeekclient/brokertypes.py | 9 ++++++++- zeekclient/types.py | 9 ++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/zeekclient/brokertypes.py b/zeekclient/brokertypes.py index a5e7672..7c9ea31 100644 --- a/zeekclient/brokertypes.py +++ b/zeekclient/brokertypes.py @@ -512,7 +512,14 @@ def __lt__(self, other): res = super().__lt__(other) if res != NotImplemented: return res - return self._addr < other._addr + if (type(self._addr) == type(other._addr)): + return self._addr < other._addr + else: + # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address + if type(self._addr) == ipaddress.IPv4Address: + return True + else: + return False def __hash__(self): return hash(self._value) diff --git a/zeekclient/types.py b/zeekclient/types.py index 07f6302..ba45aa4 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -582,11 +582,14 @@ def from_config_parser(cls, cfp, _section=None): config.instances.append(Instance(key)) else: hostport = val - parts = hostport.split(':', 1) - if len(parts) != 2 or not parts[0] or not parts[1]: + parts = hostport.rpartition(':') + if(parts[0] == '' or parts[1] == ''): LOG.error('invalid spec for instance "%s": "%s" should be :', key, val) return None - config.instances.append(Instance(key, parts[0].strip(), parts[1].strip())) + # remove brackets to support [ipv6]:port formats + host = parts[0].strip("[]") + port = parts[2].strip() + config.instances.append(Instance(key, host, port)) continue # All keys for sections other than "instances" need to have a value. From 456e76823085820f0a3b78cb6d3bf90fe011f869 Mon Sep 17 00:00:00 2001 From: dopheide-esnet Date: Wed, 30 Aug 2023 11:53:09 -0500 Subject: [PATCH 02/10] Update zeekclient/brokertypes.py Taking Arne's suggestion, much nicer. Co-authored-by: Arne Welzel --- zeekclient/brokertypes.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/zeekclient/brokertypes.py b/zeekclient/brokertypes.py index 7c9ea31..719c68c 100644 --- a/zeekclient/brokertypes.py +++ b/zeekclient/brokertypes.py @@ -512,7 +512,11 @@ def __lt__(self, other): res = super().__lt__(other) if res != NotImplemented: return res - if (type(self._addr) == type(other._addr)): + if self._addr.version == other._addr.version: + return self._addr < other._addr + + # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address + return self._addr.version < other._addr.version; return self._addr < other._addr else: # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address From 3898f300072a80d44385aa94f1df67454478426e Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Wed, 30 Aug 2023 11:54:56 -0500 Subject: [PATCH 03/10] cleanup after github suggestion commit --- zeekclient/brokertypes.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/zeekclient/brokertypes.py b/zeekclient/brokertypes.py index 719c68c..42e41f4 100644 --- a/zeekclient/brokertypes.py +++ b/zeekclient/brokertypes.py @@ -517,13 +517,6 @@ def __lt__(self, other): # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address return self._addr.version < other._addr.version; - return self._addr < other._addr - else: - # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address - if type(self._addr) == ipaddress.IPv4Address: - return True - else: - return False def __hash__(self): return hash(self._value) From 8ee9794fdbb8296d18d56a4a7fa33239c42adb5f Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Wed, 30 Aug 2023 13:59:34 -0500 Subject: [PATCH 04/10] Cleanup based on Benjamin's suggestions to make this more readable and fix a bug. --- zeekclient/types.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/zeekclient/types.py b/zeekclient/types.py index ba45aa4..07c14f9 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -582,13 +582,13 @@ def from_config_parser(cls, cfp, _section=None): config.instances.append(Instance(key)) else: hostport = val - parts = hostport.rpartition(':') - if(parts[0] == '' or parts[1] == ''): - LOG.error('invalid spec for instance "%s": "%s" should be :', key, val) + host, _, port = hostport.rpartition(':') + if(host == '' or port == ''): + LOG.error('invalid spec for instance "%s": "%s" should be :', host, port) return None # remove brackets to support [ipv6]:port formats - host = parts[0].strip("[]") - port = parts[2].strip() + host = host.strip("[]") + port = port.strip() config.instances.append(Instance(key, host, port)) continue From bac032abb4040d9e9382d059951875a845b7db12 Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Tue, 5 Sep 2023 18:01:34 -0500 Subject: [PATCH 05/10] Also strip whitespace from the IP --- zeekclient/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeekclient/types.py b/zeekclient/types.py index 07c14f9..4299de5 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -587,7 +587,7 @@ def from_config_parser(cls, cfp, _section=None): LOG.error('invalid spec for instance "%s": "%s" should be :', host, port) return None # remove brackets to support [ipv6]:port formats - host = host.strip("[]") + host = host.strip("[] ") port = port.strip() config.instances.append(Instance(key, host, port)) continue From 354e33f634fb640fe2a9d32618a2cf95beebbdfa Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Wed, 6 Sep 2023 13:06:49 -0500 Subject: [PATCH 06/10] Change error message back to key/val to match test output --- zeekclient/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zeekclient/types.py b/zeekclient/types.py index 4299de5..6bee6fb 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -584,7 +584,7 @@ def from_config_parser(cls, cfp, _section=None): hostport = val host, _, port = hostport.rpartition(':') if(host == '' or port == ''): - LOG.error('invalid spec for instance "%s": "%s" should be :', host, port) + LOG.error('invalid spec for instance "%s": "%s" should be :', key, val) return None # remove brackets to support [ipv6]:port formats host = host.strip("[] ") From 0342c87db1581a822c23d63ce273a34942f25a11 Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Wed, 6 Sep 2023 13:30:14 -0500 Subject: [PATCH 07/10] add .DS_Store to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 749a792..6e439f6 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ __pycache__ *,cover dist zeek_client.egg-info +.DS_Store From b2daac94b99492bf42180b898dffeb8f0d29a3c6 Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Thu, 7 Sep 2023 09:34:34 -0500 Subject: [PATCH 08/10] Instance constructor now checks if the IP addr is valid. Tests check for ValueError if not. --- tests/test_config_io.py | 64 +++++++++++++++++++++++++++++++++++++++++ zeekclient/types.py | 7 +++-- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/tests/test_config_io.py b/tests/test_config_io.py index 5cbe86a..00c065e 100755 --- a/tests/test_config_io.py +++ b/tests/test_config_io.py @@ -234,6 +234,70 @@ def test_config_addl_key(self): self.logbuf.getvalue(), 'warning: ignoring unexpected keys: also_not_a_key, not_a_key') + def test_config_ipv4_ipv6_instances(self): + # This test creates a Configuration from an INI file with various IP addresses + # and ports specified for the agents. + + ini_input = """[instances] +agent = 127.0.0.1:2151 +agent2 = ::1:2151 +agent3 = [::2]:2151 + +[manager] +instance = agent +role = MANAGER +port = 5000 +""" + ini_expected = """[instances] +agent = 127.0.0.1:2151 +agent2 = ::1:2151 +agent3 = ::2:2151 + +[manager] +instance = agent +role = MANAGER +port = 5000 +""" + cfp = self.parserFromString(ini_input) + config = zeekclient.types.Configuration.from_config_parser(cfp) + self.assertTrue(config is not None) + + # Turning that back into a config parser should have expected content: + cfp = config.to_config_parser() + with io.StringIO() as buf: + cfp.write(buf) + self.assertEqualStripped(buf.getvalue(), ini_expected) + + def test_config_invalid_ipv4_instance(self): + # This test creates a Configuration with an invalid IPv4 address + + ini_input = """[instances] +agent = 127.0.0.1.1:2151 + +[manager] +instance = agent +role = MANAGER +port = 5000 +""" + cfp = self.parserFromString(ini_input) + with self.assertRaisesRegex(ValueError, "'127.0.0.1.1' does not appear to be an IPv4 or IPv6 address"): + zeekclient.types.Configuration.from_config_parser(cfp) + + def test_config_invalid_ipv6_instance(self): + # This test creates a Configuration with an invalid IPv6 address + + ini_input = """[instances] +agent = ::2/128:2151 + +[manager] +instance = agent +role = MANAGER +port = 5000 +""" + cfp = self.parserFromString(ini_input) + with self.assertRaisesRegex(ValueError,"'::2/128' does not appear to be an IPv4 or IPv6 address"): + zeekclient.types.Configuration.from_config_parser(cfp) + def test_config_invalid_instances(self): ini_input = """ [instances] diff --git a/zeekclient/types.py b/zeekclient/types.py index 6bee6fb..ab7cb69 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -3,6 +3,7 @@ import enum import shlex import socket +from ipaddress import ip_address from . import brokertypes as bt from .utils import make_uuid @@ -225,8 +226,10 @@ def __init__(self, name, addr=None, port=None): self.name = name # This is a workaround until we've resolved addresses in instances self.host = '0.0.0.0' # XXX needs proper optionality - if addr is not None: - self.host = str(addr) + + # If addr isn't a valid address, the ipaddress module will raise a ValueError + if addr is not None and ip_address(addr): + self.host = str(addr) self.port = port # None or integer value; we always mean TCP def __lt__(self, other): From 7e96fce2eae8dcf8645be748a8b57f96a0edd24a Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Mon, 18 Sep 2023 12:02:24 -0500 Subject: [PATCH 09/10] Code formatting --- tests/test_config_io.py | 8 ++++++-- zeekclient/brokertypes.py | 2 +- zeekclient/types.py | 16 ++++++++++------ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/test_config_io.py b/tests/test_config_io.py index 6383450..0d80f7c 100755 --- a/tests/test_config_io.py +++ b/tests/test_config_io.py @@ -284,7 +284,9 @@ def test_config_invalid_ipv4_instance(self): port = 5000 """ cfp = self.parserFromString(ini_input) - with self.assertRaisesRegex(ValueError, "'127.0.0.1.1' does not appear to be an IPv4 or IPv6 address"): + with self.assertRaisesRegex( + ValueError, "'127.0.0.1.1' does not appear to be an IPv4 or IPv6 address" + ): zeekclient.types.Configuration.from_config_parser(cfp) def test_config_invalid_ipv6_instance(self): @@ -299,7 +301,9 @@ def test_config_invalid_ipv6_instance(self): port = 5000 """ cfp = self.parserFromString(ini_input) - with self.assertRaisesRegex(ValueError,"'::2/128' does not appear to be an IPv4 or IPv6 address"): + with self.assertRaisesRegex( + ValueError, "'::2/128' does not appear to be an IPv4 or IPv6 address" + ): zeekclient.types.Configuration.from_config_parser(cfp) def test_config_invalid_instances(self): diff --git a/zeekclient/brokertypes.py b/zeekclient/brokertypes.py index 78f68f5..bb25944 100644 --- a/zeekclient/brokertypes.py +++ b/zeekclient/brokertypes.py @@ -529,7 +529,7 @@ def __lt__(self, other): return self._addr < other._addr # Make a blanket assumption that an IPv4 address is "less than" an IPv6 address - return self._addr.version < other._addr.version; + return self._addr.version < other._addr.version def __hash__(self): return hash(self._value) diff --git a/zeekclient/types.py b/zeekclient/types.py index 9cbd613..b313890 100644 --- a/zeekclient/types.py +++ b/zeekclient/types.py @@ -238,12 +238,12 @@ class Instance(ZeekType): def __init__(self, name, addr=None, port=None): self.name = name # This is a workaround until we've resolved addresses in instances - self.host = '0.0.0.0' # XXX needs proper optionality + self.host = "0.0.0.0" # XXX needs proper optionality # If addr isn't a valid address, the ipaddress module will raise a ValueError if addr is not None and ip_address(addr): - self.host = str(addr) - self.port = port # None or integer value; we always mean TCP + self.host = str(addr) + self.port = port # None or integer value; we always mean TCP def __lt__(self, other): return self.name < other.name @@ -653,9 +653,13 @@ def from_config_parser(cls, cfp, _section=None): config.instances.append(Instance(key)) else: hostport = val - host, _, port = hostport.rpartition(':') - if(host == '' or port == ''): - LOG.error('invalid spec for instance "%s": "%s" should be :', key, val) + host, _, port = hostport.rpartition(":") + if host == "" or port == "": + LOG.error( + 'invalid spec for instance "%s": "%s" should be :', + key, + val, + ) return None # remove brackets to support [ipv6]:port formats host = host.strip("[] ") From 8dbd41da7168a9f4d73f5462592f1b56c2604bdb Mon Sep 17 00:00:00 2001 From: Michael Dopheide Date: Mon, 18 Sep 2023 12:43:01 -0500 Subject: [PATCH 10/10] switch to parser_from_string --- tests/test_config_io.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_config_io.py b/tests/test_config_io.py index 0d80f7c..1e4877f 100755 --- a/tests/test_config_io.py +++ b/tests/test_config_io.py @@ -262,7 +262,7 @@ def test_config_ipv4_ipv6_instances(self): role = MANAGER port = 5000 """ - cfp = self.parserFromString(ini_input) + cfp = self.parser_from_string(ini_input) config = zeekclient.types.Configuration.from_config_parser(cfp) self.assertTrue(config is not None) @@ -283,7 +283,7 @@ def test_config_invalid_ipv4_instance(self): role = MANAGER port = 5000 """ - cfp = self.parserFromString(ini_input) + cfp = self.parser_from_string(ini_input) with self.assertRaisesRegex( ValueError, "'127.0.0.1.1' does not appear to be an IPv4 or IPv6 address" ): @@ -300,7 +300,7 @@ def test_config_invalid_ipv6_instance(self): role = MANAGER port = 5000 """ - cfp = self.parserFromString(ini_input) + cfp = self.parser_from_string(ini_input) with self.assertRaisesRegex( ValueError, "'::2/128' does not appear to be an IPv4 or IPv6 address" ):