Fix rule ordering issue, add test to make sure I dont do that again

NHAS · May 2, 2023 · f65e6cb · f65e6cb
1 parent 6b2b734
commit f65e6cb
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 2 deletions.
diff --git a/internal/config/test_port_based_rules.json b/internal/config/test_port_based_rules.json
@@ -36,6 +36,22 @@
                     "6.6.6.6 100-150/tcp",
                     "7.7.7.7 icmp"
                 ]
+            },
+            "tester": {
+                "Allow": [
+                    "8.8.8.8 icmp 8080/any",
+                    "9.9.9.9 8081/tcp 80/udp",
+                    "10.10.10.10 8081-9000/tcp icmp",
+                    "11.11.11.11 7777-8888/tcp 90/any"
+                ]
+            },
+            "randomthingappliedtoall": {
+                "Allow": [
+                    "8.8.8.8 8080/any icmp",
+                    "9.9.9.9 80/udp 8081/tcp",
+                    "10.10.10.10 icmp 8081-9000/tcp",
+                    "11.11.11.11 90/any 7777-8888/tcp "
+                ]
             }
         }
     }

diff --git a/internal/config/test_route_restriction_preference.json b/internal/config/test_route_restriction_preference.json
@@ -53,6 +53,11 @@
                 "Mfa": [
                     "1.1.2.3/32"
                 ]
+            },
+            "test2": {
+                "Allow": [
+                    "1.1.2.0/24"
+                ]
             }
         }
     }

diff --git a/internal/router/bpf_bpfeb.o b/internal/router/bpf_bpfeb.o
diff --git a/internal/router/bpf_bpfel.o b/internal/router/bpf_bpfel.o
diff --git a/internal/router/ebpf_test.go b/internal/router/ebpf_test.go
@@ -1050,6 +1050,87 @@ func TestPortRestrictions(t *testing.T) {
 
 }
 
+func TestAgnosticRuleOrdering(t *testing.T) {
+	if err := setup("../config/test_port_based_rules.json"); err != nil {
+		t.Fatal(err)
+	}
+	defer xdpObjects.Close()
+
+	out, err := addDevices()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var packets [][]byte
+
+	for _, user := range out {
+		acl := config.GetEffectiveAcl(user.Username)
+
+		rules, err := routetypes.ParseRules(routetypes.PUBLIC, acl.Allow)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Populate expected
+		for _, rule := range rules {
+
+			for _, policy := range rule.Values {
+				if policy.Is(routetypes.STOP) {
+					break
+				}
+
+				// If we've got an any single port rule e.g 55/any, make sure that the proto is something that has ports otherwise the test fails
+				successProto := policy.Proto
+				if policy.Proto == routetypes.ANY && policy.LowerPort != routetypes.ANY {
+					successProto = routetypes.UDP
+				}
+
+				// Add matching/passing packet
+				packets = append(packets, createPacket(net.ParseIP(user.Address), net.IP(rule.Keys[0].IP[:]), int(successProto), int(policy.LowerPort)))
+			}
+		}
+
+	}
+	// We check that for both users, that they all pass. This effectively enables us to check that reordered rules are equal
+	for i := range packets {
+
+		packet := packets[i]
+
+		value, _, err := xdpObjects.bpfPrograms.XdpWagFirewall.Test(packet)
+		if err != nil {
+			t.Fatalf("program failed %s", err)
+		}
+
+		var iphdr ipv4.Header
+		err = iphdr.Parse(packet)
+		if err != nil {
+			t.Fatal("packet didnt parse as an IP header: ", err)
+		}
+		packet = packet[20:]
+
+		var pkt pkthdr
+		pkt.pktType = "unknown"
+
+		switch iphdr.Protocol {
+		case routetypes.UDP:
+			pkt.UnpackUdp(packet)
+		case routetypes.TCP:
+			pkt.UnpackTcp(packet)
+		case routetypes.ICMP:
+			pkt.UnpackIcmp(packet)
+		case routetypes.ANY:
+			pkt.UnpackAny(packet)
+
+		}
+		t.Log(iphdr.Src.String(), " -> ", iphdr.Dst.String(), ", proto "+pkt.String())
+
+		if value != XDP_PASS {
+
+			t.Fatalf("program did not XDP_PASS packet instead did: %s", result(value))
+		}
+	}
+}
+
 func TestLookupDifferentKeyTypesInMap(t *testing.T) {
 	if err := setup("../config/test_port_based_rules.json"); err != nil {
 		t.Fatal(err)

diff --git a/internal/router/xdp.c b/internal/router/xdp.c
@@ -546,12 +546,18 @@ static __always_inline int conntrack(struct ip *ip_info)
 
             if (policy.policy_type & SINGLE)
             {
-                return (policy.lower_port == 0 || policy.lower_port == port);
+                if (policy.lower_port == 0 || policy.lower_port == port)
+                {
+                    return 1;
+                }
             }
 
             if (policy.policy_type & RANGE)
             {
-                return (policy.lower_port <= port && policy.upper_port >= port);
+                if (policy.lower_port <= port && policy.upper_port >= port)
+                {
+                    return 1;
+                }
             }
         }
     }