From a282a5358ad1d76611e47ec358269da514bfc1b0 Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Mon, 16 Jan 2023 14:24:36 +0000 Subject: [PATCH] acl: rework source/destination For ACE 'source' and 'destination' is now possible to specify one of: - ipv4 or ipv6 address - ipv4 or ipv6 prefix - name of a prefixlist The validator resolves the src/dst network list, optionally filtering this with the desired 'family' (which defaults to 'any'). Errors are raised if the resulting src/dst network lists do not overlap, that is to say if all src entries are IPv4 but there are no IPv4 dst entries and vise-versa. * Update the example to have a 'trusted' prefixlist. * Update the unit tests to use the new error message(s). --- vppcfg/config/acl.py | 112 ++++++++++++++++---------- vppcfg/config/test_acl.py | 17 ++++ vppcfg/example.yaml | 8 ++ vppcfg/schema.yaml | 10 +-- vppcfg/unittest/yaml/correct-acl.yaml | 13 +++ vppcfg/unittest/yaml/error-acl1.yaml | 22 ----- vppcfg/unittest/yaml/error-acl2.yaml | 33 +++++++- 7 files changed, 144 insertions(+), 71 deletions(-) delete mode 100644 vppcfg/unittest/yaml/error-acl1.yaml diff --git a/vppcfg/config/acl.py b/vppcfg/config/acl.py index d7b477c..9f86b8b 100644 --- a/vppcfg/config/acl.py +++ b/vppcfg/config/acl.py @@ -40,28 +40,12 @@ def get_by_name(yaml, aclname): def hydrate_term(acl_term): """Adds all defaults to an ACL term""" - # Figure out the address family if "family" not in acl_term: - if "source" in acl_term and ":" in acl_term["source"]: - acl_term["family"] = "ipv6" - elif "destination" in acl_term and ":" in acl_term["destination"]: - acl_term["family"] = "ipv6" - elif "source" in acl_term and "." in acl_term["source"]: - acl_term["family"] = "ipv4" - elif "destination" in acl_term and "." in acl_term["destination"]: - acl_term["family"] = "ipv4" - else: - acl_term["family"] = "any" - - # Set source/destionation based on family, if they're omitted - if acl_term["family"] == "ipv4" and "source" not in acl_term: - acl_term["source"] = "0.0.0.0/0" - if acl_term["family"] == "ipv4" and "destination" not in acl_term: - acl_term["destination"] = "0.0.0.0/0" - if acl_term["family"] == "ipv6" and "source" not in acl_term: - acl_term["source"] = "::/0" - if acl_term["family"] == "ipv6" and "destination" not in acl_term: - acl_term["destination"] = "::/0" + acl_term["family"] = "any" + if "source" not in acl_term: + acl_term["source"] = "any" + if "destination" not in acl_term: + acl_term["destination"] = "any" if "protocol" not in acl_term or acl_term["protocol"] == "any": acl_term["protocol"] = 0 @@ -181,6 +165,13 @@ def get_network_list(yaml, network_string, want_ipv4=True, want_ipv6=True): ret = [ipn] return ret + if network_string == "any": + if want_ipv4: + ret.append(ipaddress.ip_network("0.0.0.0/0")) + if want_ipv6: + ret.append(ipaddress.ip_network("::/0")) + return ret + return prefixlist.get_network_list( yaml, network_string, want_ipv4=want_ipv4, want_ipv6=want_ipv6 ) @@ -211,6 +202,16 @@ def get_protocol(protostring): return None +def network_list_has_family(network_list, version): + """Returns True if the given list of ip_network() elements has at least one + element with the specified version, which can be either 4 or 6. Return False + otherwise""" + for m in network_list: + if m.version == version: + return True + return False + + def validate_acls(yaml): """Validate the semantics of all YAML 'acls' entries""" result = True @@ -230,25 +231,53 @@ def validate_acls(yaml): logger.debug( f"acl {aclname} term {terms} orig {orig_acl_term} hydrated {acl_term}" ) - if acl_term["family"] == "any": - if "source" in acl_term: - msgs.append( - f"acl {aclname} term {terms} family any cannot have source" - ) - result = False - if "destination" in acl_term: - msgs.append( - f"acl {aclname} term {terms} family any cannot have destination" - ) - result = False + if acl_term["family"] == "ipv4": + want_ipv4 = True + want_ipv6 = False + elif acl_term["family"] == "ipv6": + want_ipv4 = False + want_ipv6 = True else: - src = ipaddress.ip_network(acl_term["source"]) - dst = ipaddress.ip_network(acl_term["destination"]) - if src.version != dst.version: - msgs.append( - f"acl {aclname} term {terms} source and destination have different address family" - ) - result = False + want_ipv4 = True + want_ipv6 = True + + src_network_list = get_network_list( + yaml, acl_term["source"], want_ipv4=want_ipv4, want_ipv6=want_ipv6 + ) + dst_network_list = get_network_list( + yaml, acl_term["destination"], want_ipv4=want_ipv4, want_ipv6=want_ipv6 + ) + logger.debug( + f"acl {aclname} term {terms} src: {src_network_list} dst: {dst_network_list}" + ) + if len(src_network_list) == 0: + msgs.append( + f"acl {aclname} term {terms} family {acl_term['family']} has no source" + ) + result = False + if len(dst_network_list) == 0: + msgs.append( + f"acl {aclname} term {terms} family {acl_term['family']} has no destination" + ) + result = False + if len(dst_network_list) == 0 or len(src_network_list) == 0: + ## Pointless to continue if there's no src/dst at all + continue + + src_network_has_ipv4 = network_list_has_family(src_network_list, 4) + dst_network_has_ipv4 = network_list_has_family(dst_network_list, 4) + src_network_has_ipv6 = network_list_has_family(src_network_list, 6) + dst_network_has_ipv6 = network_list_has_family(dst_network_list, 6) + + if ( + src_network_has_ipv4 != dst_network_has_ipv4 + and src_network_has_ipv6 != dst_network_has_ipv6 + ): + msgs.append( + f"acl {aclname} term {terms} source and destination family do not overlap" + ) + result = False + continue proto = get_protocol(acl_term["protocol"]) if proto is None: @@ -266,8 +295,7 @@ def validate_acls(yaml): f"acl {aclname} term {terms} destination-port can only be specified for protocol tcp or udp" ) result = False - - if proto in [6, 17]: + else: src_low_port, src_high_port = get_port_low_high(acl_term["source-port"]) dst_low_port, dst_high_port = get_port_low_high( acl_term["destination-port"] @@ -328,7 +356,7 @@ def validate_acls(yaml): f"acl {aclname} term {terms} icmp-type can only be specified for protocol icmp or icmp-ipv6" ) result = False - if proto in [1, 58]: + else: icmp_code_low, icmp_code_high = get_icmp_low_high(acl_term["icmp-code"]) icmp_type_low, icmp_type_high = get_icmp_low_high(acl_term["icmp-type"]) if icmp_code_low > icmp_code_high: diff --git a/vppcfg/config/test_acl.py b/vppcfg/config/test_acl.py index b699daa..6b719a4 100644 --- a/vppcfg/config/test_acl.py +++ b/vppcfg/config/test_acl.py @@ -152,3 +152,20 @@ class TestACLMethods(unittest.TestCase): l = acl.get_network_list(self.cfg, "pl-notexist") self.assertIsInstance(l, list) self.assertEquals(0, len(l)) + + def test_network_list_has_family(self): + l = acl.get_network_list(self.cfg, "trusted") + self.assertTrue(acl.network_list_has_family(l, 4)) + self.assertTrue(acl.network_list_has_family(l, 6)) + + l = acl.get_network_list(self.cfg, "trusted", want_ipv4=False) + self.assertFalse(acl.network_list_has_family(l, 4)) + self.assertTrue(acl.network_list_has_family(l, 6)) + + l = acl.get_network_list(self.cfg, "trusted", want_ipv6=False) + self.assertTrue(acl.network_list_has_family(l, 4)) + self.assertFalse(acl.network_list_has_family(l, 6)) + + l = acl.get_network_list(self.cfg, "trusted", want_ipv4=False, want_ipv6=False) + self.assertFalse(acl.network_list_has_family(l, 4)) + self.assertFalse(acl.network_list_has_family(l, 6)) diff --git a/vppcfg/example.yaml b/vppcfg/example.yaml index b54fdd8..c11813d 100644 --- a/vppcfg/example.yaml +++ b/vppcfg/example.yaml @@ -126,6 +126,7 @@ prefixlists: - 192.0.2.0/24 - 2001:db8::1 - 2001:db8::/64 + - 2001:db8::/48 empty: description: "An empty prefixlist" members: [] @@ -134,6 +135,13 @@ acls: acl01: description: "Test ACL" terms: + - description: "Allow a prefixlist" + action: permit + source: trusted + - description: "Allow a prefixlist for one family only" + family: ipv4 + action: permit + source: trusted - description: "Allow a specific IPv6 TCP flow" action: permit source: 2001:db8::/64 diff --git a/vppcfg/schema.yaml b/vppcfg/schema.yaml index 12d62db..d2fb830 100644 --- a/vppcfg/schema.yaml +++ b/vppcfg/schema.yaml @@ -5,7 +5,7 @@ bridgedomains: map(include('bridgedomain'),key=str(matches='bd[0-9]+'),required= vxlan_tunnels: map(include('vxlan'),key=str(matches='vxlan_tunnel[0-9]+'),required=False) taps: map(include('tap'),key=str(matches='tap[0-9]+'),required=False) prefixlists: map(include('prefixlist'),key=str(matches='[a-z][a-z0-9\-]+',min=1,max=64),required=False) -acls: map(include('acl'),key=str(matches='[a-z][a-z0-9\-]+'),required=False) +acls: map(include('acl'),key=str(matches='[a-z][a-z0-9\-]+',min=1,max=56),required=False) --- vxlan: description: str(exclude='\'"',len=64,required=False) @@ -96,11 +96,11 @@ acl-term: description: str(exclude='\'"',len=64,required=False) action: enum('permit','deny','permit+reflect') family: enum('ipv4','ipv6','any',required=False) - source: any(ip(), ip_interface(),required=False) - destination: any(ip(), ip_interface(),required=False) + source: any(ip(),ip_interface(),str(min=1,max=56),required=False) + destination: any(ip(),ip_interface(),str(min=1,max=56),required=False) protocol: any(int(min=1,max=255),regex('^[a-z][a-z0-9-]*$'),required=False) - source-port: include('acl-term-port-int-range-symbolic', required=False) - destination-port: include('acl-term-port-int-range-symbolic', required=False) + source-port: include('acl-term-port-int-range-symbolic',required=False) + destination-port: include('acl-term-port-int-range-symbolic',required=False) icmp-type: include('acl-term-icmp-int-range',required=False) icmp-code: include('acl-term-icmp-int-range',required=False) --- diff --git a/vppcfg/unittest/yaml/correct-acl.yaml b/vppcfg/unittest/yaml/correct-acl.yaml index 58e0611..9024b5a 100644 --- a/vppcfg/unittest/yaml/correct-acl.yaml +++ b/vppcfg/unittest/yaml/correct-acl.yaml @@ -3,10 +3,23 @@ test: errors: count: 0 --- +prefixlists: + trusted: + description: "Trusted IPv4 nd IPv6 hosts" + members: + - 192.0.2.1 + - 192.0.2.0/24 + - 2001:db8::1 + - 2001:db8::/64 + - 2001:db8::/48 + acls: acl01: description: "Test ACL" terms: + - description: "Allow a prefixlist" + action: permit + source: trusted - description: "Allow a specific IPv6 TCP flow" action: permit source: 2001:db8::/64 diff --git a/vppcfg/unittest/yaml/error-acl1.yaml b/vppcfg/unittest/yaml/error-acl1.yaml deleted file mode 100644 index a758e42..0000000 --- a/vppcfg/unittest/yaml/error-acl1.yaml +++ /dev/null @@ -1,22 +0,0 @@ -test: - description: "Family any precludes source/destination" - errors: - expected: - - "acl .* term .* family any cannot have (source|destination)" - count: 4 ---- -acls: - acl01: - terms: - - family: any - source: 0.0.0.0/0 - action: permit - - family: any - source: ::/0 - action: permit - - family: any - destination: 0.0.0.0/0 - action: permit - - family: any - destination: ::/0 - action: permit diff --git a/vppcfg/unittest/yaml/error-acl2.yaml b/vppcfg/unittest/yaml/error-acl2.yaml index 3f18e00..a9404e4 100644 --- a/vppcfg/unittest/yaml/error-acl2.yaml +++ b/vppcfg/unittest/yaml/error-acl2.yaml @@ -2,9 +2,20 @@ test: description: "Source and Destination must have the same address family" errors: expected: - - "acl .* term .* source and destination have different address family" - count: 4 + - "acl .* term .* source and destination family do not overlap" + count: 6 --- +prefixlists: + v4only: + members: + - 192.0.2.1 + - 192.0.2.0/24 + v6only: + members: + - 2001:db8::1 + - 2001:db8::/64 + - 2001:db8::/48 + acls: acl01: terms: @@ -12,6 +23,14 @@ acls: source: 0.0.0.0/0 destination: ::/0 action: permit + - description: "Error, source prefixlist is IPv4 and destination prefixlist is IPv6" + source: v4only + destination: v6only + action: permit + - description: "Error, source prefixlist is IPv6 and destination is IPv4" + source: v6only + destination: 0.0.0.0/0 + action: permit - description: "Error, source is IPv6 and destination is IPv4" source: ::/0 destination: 192.168.0.1 @@ -32,3 +51,13 @@ acls: source: 192.168.0.1 destination: 10.0.0.0/8 action: permit + - description: "OK" + source: v4only + action: permit + - description: "OK" + source: v6only + action: permit + - description: "OK" + source: v4only + destination: v4only + action: permit