Expand all/any firewall pairs per zone and dedupe fw

internal/nftables/compiler.go

@@ -467,28 +467,26 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	dports, sports config.PortSpec, action config.RuleAction, logLevel string,
 	dnatDest, origDest string, fwZone string, section config.RuleSection) error {
 
-	srcs := c.zoneSpecs(srcSpec)
-	if action != config.RuleDNAT && action != config.RuleRedirect {
-		srcs = withFirewall(srcs, fwZone)
-	}
-	for _, src := range srcs {
-		for _, srcAddr := range splitAddrs(src.Addr) {
-			for _, od := range splitAddrs(origDest) {
-				if action == config.RuleDNAT || action == config.RuleRedirect {
+	if action == config.RuleDNAT || action == config.RuleRedirect {
+		for _, src := range c.zoneSpecs(srcSpec) {
+			for _, srcAddr := range splitAddrs(src.Addr) {
+				for _, od := range splitAddrs(origDest) {
 					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, action, logLevel); err != nil {
 						return err
 					}
-					continue
 				}
-				for _, dst := range withFirewall(c.zoneSpecs(dstSpec), fwZone) {
-					if skipPair(srcSpec, dstSpec, src.Zone, dst.Zone, fwZone) {
-						continue
-					}
-					for _, dstAddr := range splitAddrs(dst.Addr) {
-						if err := c.compileZonePair(state, tag, src.Zone, srcAddr, dst.Zone, dstAddr, od, proto,
-							dports, sports, action, logLevel, fwZone, section); err != nil {
-							return err
-						}
+			}
+		}
+		return nil
+	}
+	for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) {
+		src, dst := p[0], p[1]
+		for _, srcAddr := range splitAddrs(src.Addr) {
+			for _, od := range splitAddrs(origDest) {
+				for _, dstAddr := range splitAddrs(dst.Addr) {
+					if err := c.compileZonePair(state, tag, src.Zone, srcAddr, dst.Zone, dstAddr, od, proto,
+						dports, sports, action, logLevel, fwZone, section); err != nil {
+						return err
 					}
 				}
 			}
@@ -506,24 +504,31 @@ func (c *Compiler) specCount(srcSpec, dstSpec, origDest, fwZone string, action c
 		}
 		return n * len(splitAddrs(origDest))
 	}
-	for _, src := range withFirewall(c.zoneSpecs(srcSpec), fwZone) {
-		for _, dst := range withFirewall(c.zoneSpecs(dstSpec), fwZone) {
-			if !skipPair(srcSpec, dstSpec, src.Zone, dst.Zone, fwZone) {
-				n += len(splitAddrs(src.Addr)) * len(splitAddrs(dst.Addr))
-			}
-		}
+	for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) {
+		n += len(splitAddrs(p[0].Addr)) * len(splitAddrs(p[1].Addr))
 	}
 	return n * len(splitAddrs(origDest))
 }
 
-// skipPair reports whether compileOneRule drops a src/dst zone pair from the expansion.
-func skipPair(srcSpec, dstSpec, srcZone, dstZone, fwZone string) bool {
-	if srcZone == fwZone && dstZone == fwZone && (isGlobalZone(srcSpec) || isGlobalZone(dstSpec)) {
-		return true
+// zonePairs is the src/dst zone expansion of a non-DNAT rule, with fw added beside all/any.
+func (c *Compiler) zonePairs(srcSpec, dstSpec, fwZone string) [][2]config.ZoneSpec {
+	srcs, srcGlobal := withFirewall(c.zoneSpecs(srcSpec), fwZone)
+	dsts, dstGlobal := withFirewall(c.zoneSpecs(dstSpec), fwZone)
+	var out [][2]config.ZoneSpec
+	for _, src := range srcs {
+		for _, dst := range dsts {
+			if src.Zone == fwZone && dst.Zone == fwZone && (srcGlobal || dstGlobal) {
+				continue
+			}
+			// Exclusion expansion never pairs fw with itself, and pairs a zone with itself only for "all+".
+			if src.Zone == dst.Zone && (isZoneExclusion(srcSpec) || isZoneExclusion(dstSpec)) &&
+				(src.Zone == fwZone || !strings.Contains(srcSpec, "+!") && !strings.Contains(dstSpec, "+!")) {
+				continue
+			}
+			out = append(out, [2]config.ZoneSpec{src, dst})
+		}
 	}
-	// Exclusion expansion never pairs fw with itself, and pairs a zone with itself only for "all+".
-	return srcZone == dstZone && (isZoneExclusion(srcSpec) || isZoneExclusion(dstSpec)) &&
-		(srcZone == fwZone || !strings.Contains(srcSpec, "+!") && !strings.Contains(dstSpec, "+!"))
+	return out
 }
 
 // zoneSpecs expands a comma zone list; "all"/"any" stay global and "all!x,y" becomes every zone but x and y.
@@ -543,14 +548,17 @@ func (c *Compiler) zoneSpecs(spec string) []config.ZoneSpec {
 }
 
 // withFirewall adds the firewall zone beside a global all/any spec, which otherwise only reaches forward.
-func withFirewall(specs []config.ZoneSpec, fwZone string) []config.ZoneSpec {
-	out := specs
+func withFirewall(specs []config.ZoneSpec, fwZone string) ([]config.ZoneSpec, bool) {
+	out, global := specs, false
 	for _, s := range specs {
 		if fwZone != "" && isGlobalZone(s.Zone) {
-			out = append(out, config.ZoneSpec{Zone: fwZone, Addr: s.Addr})
+			global = true
+			if fw := (config.ZoneSpec{Zone: fwZone, Addr: s.Addr}); !slices.Contains(out, fw) {
+				out = append(out, fw)
+			}
 		}
 	}
-	return out
+	return out, global
 }
 
 func isGlobalZone(spec string) bool {

internal/nftables/compiler_test.go

@@ -2225,6 +2225,7 @@ func TestCompile_CommaZoneListLimitErrors(t *testing.T) {
 		{Action: config.RuleAccept, Source: "all", Dest: "all", RateLimit: "10/sec"},
 		{Action: config.RuleAccept, Source: "net", Dest: "all", ConnLimit: "10"},
 		{Action: config.RuleAccept, Source: "all", Dest: "net", RateLimit: "10/sec"},
+		{Action: config.RuleAccept, Source: "net,all", Dest: "fw", RateLimit: "10/sec"},
 	} {
 		t.Run(r.Source+">"+r.Dest, func(t *testing.T) {
 			cfg := &config.Config{
@@ -2855,6 +2856,8 @@ func TestCompile_AllIncludesFirewall(t *testing.T) {
 		{"all:192.0.2.0/24", "fw", map[string]int{"input": 1, "output": 0, "forward": 0}},
 		{"all!fw", "all!fw", map[string]int{"input": 0, "output": 0, "forward": 2}},
 		{"all+", "all", map[string]int{"input": 1, "output": 1, "forward": 1}},
+		{"net,all", "fw", map[string]int{"input": 2, "output": 0, "forward": 0}},
+		{"fw,all", "net", map[string]int{"input": 0, "output": 1, "forward": 1}},
 	}
 	for _, tc := range cases {
 		t.Run(tc.src+"->"+tc.dst, func(t *testing.T) {
@@ -2889,3 +2892,20 @@ func TestCompile_AllIncludesFirewall(t *testing.T) {
 		})
 	}
 }
+
+func TestSpecCount_CommaAllMatchesExpansion(t *testing.T) {
+	c := NewCompiler(&config.Config{
+		Zones: map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+	})
+	for _, tc := range []struct {
+		src, dst string
+		want     int
+	}{
+		{"net,all", "fw", 2},
+		{"fw,all", "net", 2},
+	} {
+		if got := c.specCount(tc.src, tc.dst, "", "fw", config.RuleAccept); got != tc.want {
+			t.Errorf("specCount(%s, %s) = %d, want %d", tc.src, tc.dst, got, tc.want)
+		}
+	}
+}