From e3130b6c3b132026de3340dc3bc2bd9bd7542d96 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 4 Oct 2026 15:10:49 +1100 Subject: [PATCH] Expand all/any firewall pairs per zone and dedupe fw --- internal/nftables/compiler.go | 78 ++++++++++++++++-------------- internal/nftables/compiler_test.go | 20 ++++++++ 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/internal/nftables/compiler.go b/internal/nftables/compiler.go index 12d2940..0308049 100644 --- a/internal/nftables/compiler.go +++ b/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 { diff --git a/internal/nftables/compiler_test.go b/internal/nftables/compiler_test.go index 776e1c5..a83cd22 100644 --- a/internal/nftables/compiler_test.go +++ b/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) + } + } +}