Skip to content

Commit

Permalink
[acl-loader] Improve input validation for acl_loader (sonic-net#1479)
Browse files Browse the repository at this point in the history
Signed-off-by: Danny Allen <daall@microsoft.com>
  • Loading branch information
daall authored Mar 5, 2021
1 parent 748dbbf commit f78e7ce
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 6 deletions.
17 changes: 16 additions & 1 deletion acl_loader/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,19 @@ def convert_input_interface(self, table_name, rule_idx, rule):

return rule_props

def validate_rule_fields(self, rule_props):
protocol = rule_props.get("IP_PROTOCOL")

if protocol:
if "TCP_FLAGS" in rule_props and protocol != 6:
raise AclLoaderException("IP_PROTOCOL={} is not TCP, but TCP flags were provided".format(protocol))

if ("ICMP_TYPE" in rule_props or "ICMP_CODE" in rule_props) and protocol != 1:
raise AclLoaderException("IP_PROTOCOL={} is not ICMP, but ICMP fields were provided".format(protocol))

if ("ICMPV6_TYPE" in rule_props or "ICMPV6_CODE" in rule_props) and protocol != 58:
raise AclLoaderException("IP_PROTOCOL={} is not ICMPV6, but ICMPV6 fields were provided".format(protocol))

def convert_rule_to_db_schema(self, table_name, rule):
"""
Convert rules format from openconfig ACL to Config DB schema
Expand All @@ -579,6 +592,8 @@ def convert_rule_to_db_schema(self, table_name, rule):
deep_update(rule_props, self.convert_transport(table_name, rule_idx, rule))
deep_update(rule_props, self.convert_input_interface(table_name, rule_idx, rule))

self.validate_rule_fields(rule_props)

return rule_data

def deny_rule(self, table_name):
Expand All @@ -591,7 +606,7 @@ def deny_rule(self, table_name):
rule_data = {(table_name, "DEFAULT_RULE"): rule_props}
rule_props["PRIORITY"] = str(self.min_priority)
rule_props["PACKET_ACTION"] = "DROP"
if 'v6' in table_name.lower():
if self.is_table_ipv6(table_name):
rule_props["IP_TYPE"] = "IPV6ANY" # ETHERTYPE is not supported for DATAACLV6
else:
rule_props["ETHER_TYPE"] = str(self.ethertype_map["ETHERTYPE_IPV4"])
Expand Down
2 changes: 1 addition & 1 deletion tests/acl_input/acl1.json
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@
}
}
},
"DATAACLV6": {
"DATAACL_2": {
"acl-entries": {
"acl-entry": {
"1": {
Expand Down
37 changes: 37 additions & 0 deletions tests/acl_input/icmp_bad_protocol_number.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"acl": {
"acl-sets": {
"acl-set": {
"DATAACL": {
"acl-entries": {
"acl-entry": {
"1": {
"config": {
"sequence-id": 1
},
"actions": {
"config": {
"forwarding-action": "ACCEPT"
}
},
"ip": {
"config": {
"protocol": "IP_UDP",
"source-ip-address": "20.0.0.2/32",
"destination-ip-address": "30.0.0.3/32"
}
},
"icmp": {
"config": {
"type": "3",
"code": "0"
}
}
}
}
}
}
}
}
}
}
37 changes: 37 additions & 0 deletions tests/acl_input/icmpv6_bad_protocol_number.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"acl": {
"acl-sets": {
"acl-set": {
"DATAACL_2": {
"acl-entries": {
"acl-entry": {
"1": {
"config": {
"sequence-id": 1
},
"actions": {
"config": {
"forwarding-action": "ACCEPT"
}
},
"ip": {
"config": {
"protocol": "IP_UDP",
"source-ip-address": "20.0.0.2/32",
"destination-ip-address": "30.0.0.3/32"
}
},
"icmp": {
"config": {
"type": "3",
"code": "0"
}
}
}
}
}
}
}
}
}
}
36 changes: 36 additions & 0 deletions tests/acl_input/tcp_bad_protocol_number.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"acl": {
"acl-sets": {
"acl-set": {
"DATAACL": {
"acl-entries": {
"acl-entry": {
"1": {
"config": {
"sequence-id": 1
},
"actions": {
"config": {
"forwarding-action": "ACCEPT"
}
},
"ip": {
"config": {
"protocol": "IP_UDP",
"source-ip-address": "20.0.0.2/32",
"destination-ip-address": "30.0.0.3/32"
}
},
"transport": {
"config": {
"tcp-flags": ["TCP_ACK"]
}
}
}
}
}
}
}
}
}
}
20 changes: 18 additions & 2 deletions tests/acl_loader_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def test_icmpv6_translation(self, acl_loader):
acl_loader.rules_info = {}
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/acl1.json'))
print(acl_loader.rules_info)
assert acl_loader.rules_info[("DATAACLV6", "RULE_1")] == {
assert acl_loader.rules_info[("DATAACL_2", "RULE_1")] == {
"ICMPV6_TYPE": 1,
"ICMPV6_CODE": 0,
"IP_PROTOCOL": 58,
Expand All @@ -109,7 +109,7 @@ def test_icmpv6_translation(self, acl_loader):
"PACKET_ACTION": "FORWARD",
"PRIORITY": "9999"
}
assert acl_loader.rules_info[("DATAACLV6", "RULE_100")] == {
assert acl_loader.rules_info[("DATAACL_2", "RULE_100")] == {
"ICMPV6_TYPE": 128,
"IP_PROTOCOL": 58,
"SRC_IPV6": "::1/128",
Expand Down Expand Up @@ -147,3 +147,19 @@ def test_icmp_code_not_a_number(self, acl_loader):
with pytest.raises(ValueError):
acl_loader.rules_info = {}
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/illegal_icmp_code_nan.json'))

def test_icmp_fields_with_non_icmp_protocol(self, acl_loader):
acl_loader.rules_info = {}
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmp_bad_protocol_number.json'))
assert not acl_loader.rules_info.get("RULE_1")

def ttest_icmp_fields_with_non_icmpv6_protocol(self, acl_loader):
acl_loader.rules_info = {}
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/icmpv6_bad_protocol_number.json'))
assert not acl_loader.rules_info.get("RULE_1")


def test_icmp_fields_with_non_tcp_protocol(self, acl_loader):
acl_loader.rules_info = {}
acl_loader.load_rules_from_file(os.path.join(test_path, 'acl_input/tcp_bad_protocol_number.json'))
assert not acl_loader.rules_info.get("RULE_1")
4 changes: 2 additions & 2 deletions tests/mock_tables/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,8 @@
"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",
"type": "L3"
},
"ACL_TABLE|DATAACLV6": {
"policy_desc": "DATAACLV6",
"ACL_TABLE|DATAACL_2": {
"policy_desc": "DATAACL_2",
"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",
"type": "L3V6"
},
Expand Down

0 comments on commit f78e7ce

Please sign in to comment.