[acl-loader] Improve input validation for acl_loader (sonic-net#1479)

Signed-off-by: Danny Allen <daall@microsoft.com>
carl-nokia · Mar 5, 2021 · f78e7ce · f78e7ce
1 parent 748dbbf
commit f78e7ce
Show file tree

Hide file tree

Showing 7 changed files with 147 additions and 6 deletions.
diff --git a/acl_loader/main.py b/acl_loader/main.py
@@ -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
@@ -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):
@@ -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"])

diff --git a/tests/acl_input/acl1.json b/tests/acl_input/acl1.json
@@ -193,7 +193,7 @@
 						}
 					}
 				},
-				"DATAACLV6": {
+				"DATAACL_2": {
 					"acl-entries": {
 						"acl-entry": {
 							"1": {

diff --git a/tests/acl_input/icmp_bad_protocol_number.json b/tests/acl_input/icmp_bad_protocol_number.json
@@ -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"
+									}
+								}
+							}
+						}
+					}
+				}
+			}
+		}
+	}
+}
diff --git a/tests/acl_input/icmpv6_bad_protocol_number.json b/tests/acl_input/icmpv6_bad_protocol_number.json
@@ -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"
+									}
+								}
+							}
+						}
+					}
+				}
+			}
+		}
+	}
+}
diff --git a/tests/acl_input/tcp_bad_protocol_number.json b/tests/acl_input/tcp_bad_protocol_number.json
@@ -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"]
+									}
+								}
+							}
+						}
+					}
+				}
+			}
+		}
+	}
+}
diff --git a/tests/acl_loader_test.py b/tests/acl_loader_test.py
@@ -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,
@@ -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",
@@ -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")
diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json
@@ -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"
     },