Skip to content

Commit f78e7ce

Browse files
daalldaall
authored
[acl-loader] Improve input validation for acl_loader (sonic-net#1479)
Signed-off-by: Danny Allen <[email protected]>
1 parent 748dbbf commit f78e7ce

7 files changed

+147
-6
lines changed

‎acl_loader/main.py

+16-1
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,19 @@ def convert_input_interface(self, table_name, rule_idx, rule):
559559

560560
return rule_props
561561

562+
def validate_rule_fields(self, rule_props):
563+
protocol = rule_props.get("IP_PROTOCOL")
564+
565+
if protocol:
566+
if "TCP_FLAGS" in rule_props and protocol != 6:
567+
raise AclLoaderException("IP_PROTOCOL={} is not TCP, but TCP flags were provided".format(protocol))
568+
569+
if ("ICMP_TYPE" in rule_props or "ICMP_CODE" in rule_props) and protocol != 1:
570+
raise AclLoaderException("IP_PROTOCOL={} is not ICMP, but ICMP fields were provided".format(protocol))
571+
572+
if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58:
573+
raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol))
574+
562575
def convert_rule_to_db_schema(self, table_name, rule):
563576
"""
564577
Convert rules format from openconfig ACL to Config DB schema
@@ -579,6 +592,8 @@ def convert_rule_to_db_schema(self, table_name, rule):
579592
deep_update(rule_props, self.convert_transport(table_name, rule_idx, rule))
580593
deep_update(rule_props, self.convert_input_interface(table_name, rule_idx, rule))
581594

595+
self.validate_rule_fields(rule_props)
596+
582597
return rule_data
583598

584599
def deny_rule(self, table_name):
@@ -591,7 +606,7 @@ def deny_rule(self, table_name):
591606
rule_data = {(table_name, "DEFAULT_RULE"): rule_props}
592607
rule_props["PRIORITY"] = str(self.min_priority)
593608
rule_props["PACKET_ACTION"] = "DROP"
594-
if 'v6' in table_name.lower():
609+
if self.is_table_ipv6(table_name):
595610
rule_props["IP_TYPE"] = "IPV6ANY" # ETHERTYPE is not supported for DATAACLV6
596611
else:
597612
rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"])

‎tests/acl_input/acl1.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@
193193
}
194194
}
195195
},
196-
"DATAACLV6": {
196+
"DATAACL_2": {
197197
"acl-entries": {
198198
"acl-entry": {
199199
"1": {

‎tests/acl_input/icmp_bad_protocol_number.json

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
{
2+
"acl": {
3+
"acl-sets": {
4+
"acl-set": {
5+
"DATAACL": {
6+
"acl-entries": {
7+
"acl-entry": {
8+
"1": {
9+
"config": {
10+
"sequence-id": 1
11+
},
12+
"actions": {
13+
"config": {
14+
"forwarding-action": "ACCEPT"
15+
}
16+
},
17+
"ip": {
18+
"config": {
19+
"protocol": "IP_UDP",
20+
"source-ip-address": "20.0.0.2/32",
21+
"destination-ip-address": "30.0.0.3/32"
22+
}
23+
},
24+
"icmp": {
25+
"config": {
26+
"type": "3",
27+
"code": "0"
28+
}
29+
}
30+
}
31+
}
32+
}
33+
}
34+
}
35+
}
36+
}
37+
}

‎tests/acl_input/icmpv6_bad_protocol_number.json

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
{
2+
"acl": {
3+
"acl-sets": {
4+
"acl-set": {
5+
"DATAACL_2": {
6+
"acl-entries": {
7+
"acl-entry": {
8+
"1": {
9+
"config": {
10+
"sequence-id": 1
11+
},
12+
"actions": {
13+
"config": {
14+
"forwarding-action": "ACCEPT"
15+
}
16+
},
17+
"ip": {
18+
"config": {
19+
"protocol": "IP_UDP",
20+
"source-ip-address": "20.0.0.2/32",
21+
"destination-ip-address": "30.0.0.3/32"
22+
}
23+
},
24+
"icmp": {
25+
"config": {
26+
"type": "3",
27+
"code": "0"
28+
}
29+
}
30+
}
31+
}
32+
}
33+
}
34+
}
35+
}
36+
}
37+
}

‎tests/acl_input/tcp_bad_protocol_number.json

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
{
2+
"acl": {
3+
"acl-sets": {
4+
"acl-set": {
5+
"DATAACL": {
6+
"acl-entries": {
7+
"acl-entry": {
8+
"1": {
9+
"config": {
10+
"sequence-id": 1
11+
},
12+
"actions": {
13+
"config": {
14+
"forwarding-action": "ACCEPT"
15+
}
16+
},
17+
"ip": {
18+
"config": {
19+
"protocol": "IP_UDP",
20+
"source-ip-address": "20.0.0.2/32",
21+
"destination-ip-address": "30.0.0.3/32"
22+
}
23+
},
24+
"transport": {
25+
"config": {
26+
"tcp-flags": ["TCP_ACK"]
27+
}
28+
}
29+
}
30+
}
31+
}
32+
}
33+
}
34+
}
35+
}
36+
}

‎tests/acl_loader_test.py

+18-2
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ def test_icmpv6_translation(self, acl_loader):
100100
acl_loader.rules_info = {}
101101
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl1.json'))
102102
print(acl_loader.rules_info)
103-
assert acl_loader.rules_info[("DATAACLV6", "RULE_1")] == {
103+
assert acl_loader.rules_info[("DATAACL_2", "RULE_1")] == {
104104
"ICMPV6_TYPE": 1,
105105
"ICMPV6_CODE": 0,
106106
"IP_PROTOCOL": 58,
@@ -109,7 +109,7 @@ def test_icmpv6_translation(self, acl_loader):
109109
"PACKET_ACTION": "FORWARD",
110110
"PRIORITY": "9999"
111111
}
112-
assert acl_loader.rules_info[("DATAACLV6", "RULE_100")] == {
112+
assert acl_loader.rules_info[("DATAACL_2", "RULE_100")] == {
113113
"ICMPV6_TYPE": 128,
114114
"IP_PROTOCOL": 58,
115115
"SRC_IPV6": "::1/128",
@@ -147,3 +147,19 @@ def test_icmp_code_not_a_number(self, acl_loader):
147147
with pytest.raises(ValueError):
148148
acl_loader.rules_info = {}
149149
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/illegal_icmp_code_nan.json'))
150+
151+
def test_icmp_fields_with_non_icmp_protocol(self, acl_loader):
152+
acl_loader.rules_info = {}
153+
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmp_bad_protocol_number.json'))
154+
assert not acl_loader.rules_info.get("RULE_1")
155+
156+
def ttest_icmp_fields_with_non_icmpv6_protocol(self, acl_loader):
157+
acl_loader.rules_info = {}
158+
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmpv6_bad_protocol_number.json'))
159+
assert not acl_loader.rules_info.get("RULE_1")
160+
161+
162+
def test_icmp_fields_with_non_tcp_protocol(self, acl_loader):
163+
acl_loader.rules_info = {}
164+
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/tcp_bad_protocol_number.json'))
165+
assert not acl_loader.rules_info.get("RULE_1")

‎tests/mock_tables/config_db.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -400,8 +400,8 @@
400400
"ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124",
401401
"type": "L3"
402402
},
403-
"ACL_TABLE|DATAACLV6": {
404-
"policy_desc": "DATAACLV6",
403+
"ACL_TABLE|DATAACL_2": {
404+
"policy_desc": "DATAACL_2",
405405
"ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124",
406406
"type": "L3V6"
407407
},

0 commit comments

Comments
 (0)