Merge pull request 'Emit the implied ACCEPT for DNAT and REDIRECT rules' (#29) from benvin/dnat-implied-accept into main

Reviewed-on: #29
2026-10-04 15:28:05 +11:00
parent 599c483cd8 87be6bc6af
commit 1e32bde555
2 changed files with 229 additions and 15 deletions
@@ -451,10 +451,16 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	dnatDest, origDest string, fwZone string, section config.RuleSection) error {

 	if action == config.RuleDNAT || action == config.RuleRedirect {
-		for _, src := range c.zoneSpecs(srcSpec) {
+		for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) {
+			if dnatSkipsIntrazone(srcSpec, src.Zone, dstSpec) {
+				continue
+			}
 			for _, srcAddr := range splitAddrs(src.Addr) {
+				if err := c.compileDNATAccept(state, tag+":accept", src.Zone, srcAddr, dstSpec, proto, dports, sports, action, fwZone, section); err != nil {
+					return err
+				}
 				for _, od := range splitAddrs(origDest) {
-					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, action, logLevel); err != nil {
+					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, sports, action, logLevel); err != nil {
 						return err
 					}
 				}
@@ -478,11 +484,64 @@ func (c *Compiler) compileOneRule(state *FirewallState, tag, srcSpec, dstSpec, p
 	return nil
 }

+// dnatSourceSpecs hooks DNAT per source zone like shorewall: all/any expand to every zone but fw (prerouting never sees fw traffic).
+func (c *Compiler) dnatSourceSpecs(spec, fwZone string) []config.ZoneSpec {
+	zone, addr := splitZoneSpec(spec)
+	base, _, _ := strings.Cut(zone, "!")
+	if base = strings.TrimSuffix(base, "+"); base != "all" && base != "any" {
+		return c.zoneSpecs(spec)
+	}
+	var out []config.ZoneSpec
+	for _, z := range c.expandZoneRef(zone) {
+		if z != fwZone {
+			out = append(out, config.ZoneSpec{Zone: z, Addr: addr})
+		}
+	}
+	return out
+}
+
+// dnatSkipsIntrazone mirrors shorewall: a zone list or all/any source never pairs a zone with itself unless marked "+".
+func dnatSkipsIntrazone(srcSpec, srcZone, dstSpec string) bool {
+	zones, _, _ := strings.Cut(srcSpec, ":")
+	base, _, _ := strings.Cut(zones, "!")
+	wild := base == "all" || base == "any" || strings.Contains(base, ",")
+	dstZone, _, _ := strings.Cut(dstSpec, ":")
+	return wild && srcZone == dstZone
+}
+
+// compileDNATAccept emits the filter ACCEPT implied by DNAT/REDIRECT (shorewall's DNAT-/REDIRECT- omit it) for the translated flow.
+func (c *Compiler) compileDNATAccept(state *FirewallState, tag, srcZone, srcAddr, dstSpec, proto string,
+	dports, sports config.PortSpec, action config.RuleAction, fwZone string, section config.RuleSection) error {
+	parts := strings.SplitN(dstSpec, ":", 3)
+	if len(parts) < 2 {
+		return fmt.Errorf("DNAT dest must be zone:address or zone:address:port")
+	}
+	dstZone, dstAddr := parts[0], parts[1]
+	if action == config.RuleRedirect {
+		dstZone, dstAddr = fwZone, ""
+	}
+	if len(parts) == 3 {
+		dports = config.PortSpec{parts[2]}
+	}
+	chain := c.selectChain(srcZone, dstZone, fwZone)
+	n := len(state.Rules[chain])
+	if err := c.compileZonePair(state, tag, srcZone, srcAddr, dstZone, dstAddr, "", proto,
+		dports, sports, config.RuleAccept, "", fwZone, section); err != nil {
+		return err
+	}
+	for i := n; i < len(state.Rules[chain]); i++ {
+		e := state.Rules[chain][i].Exprs
+		last := len(e) - 1
+		state.Rules[chain][i].Exprs = append(append(e[:last:last], matchCtBits(expr.CtKeySTATUS, ctStatusDNAT)...), e[last])
+	}
+	return nil
+}
+
 // specCount is how many zone/address combinations compileOneRule expands src and dst into.
 func (c *Compiler) specCount(srcSpec, dstSpec, origDest, fwZone string, action config.RuleAction) int {
 	n := 0
 	if action == config.RuleDNAT || action == config.RuleRedirect {
-		for _, src := range c.zoneSpecs(srcSpec) {
+		for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) {
 			n += len(splitAddrs(src.Addr))
 		}
 		return n * len(splitAddrs(origDest))
@@ -619,7 +678,7 @@ func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr,
 }

 func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, origDest, dstSpec, proto string,
-	dports config.PortSpec, action config.RuleAction, logLevel string) error {
+	dports, sports config.PortSpec, action config.RuleAction, logLevel string) error {
 	chain := "prerouting"

 	parts := strings.SplitN(dstSpec, ":", 3)
@@ -647,7 +706,7 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr,
 		}
 	}

-	matches, err := l4Matches(proto, dports, nil)
+	matches, err := l4Matches(proto, dports, sports)
 	if err != nil {
 		return err
 	}
@@ -1673,13 +1732,18 @@ const (
 	ctStateRelated     = 4
 	ctStateNew         = 8
 	ctStateUntracked   = 64
+	ctStatusDNAT       = 32
 )

 func matchCtState(stateMask uint32) []expr.Any {
+	return matchCtBits(expr.CtKeySTATE, stateMask)
+}
+
+func matchCtBits(key expr.CtKey, stateMask uint32) []expr.Any {
 	stateBytes := make([]byte, 4)
 	binary.NativeEndian.PutUint32(stateBytes, stateMask)
 	return []expr.Any{
-		&expr.Ct{Key: expr.CtKeySTATE, Register: 1},
+		&expr.Ct{Key: key, Register: 1},
 		&expr.Bitwise{
 			SourceRegister: 1,
 			DestRegister:   1,
@@ -1964,12 +1964,58 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 		{
 			name: "dnat source list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
-			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth1"}},
+			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth1"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat source list skips the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "net,svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}, "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat lone source zone may equal the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth2"}, "forward": {"iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat exclusion source skips the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all!fw,anycast", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth1", "iif=eth0"},
+				"forward": {"iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat intrazone exclusion source keeps the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all+!fw,anycast,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth2"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat all source expands per zone and skips fw and the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0"},
+				"forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat any+ source keeps the target zone",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "any+", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0", "iif=eth2"},
+				"forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}},
+		},
+		{
+			name: "dnat to fw accepts in input",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.1", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}, "input": {"iif=eth0 daddr=192.0.2.1"}},
+		},
+		{
+			name: "redirect accepts in input without daddr",
+			rule: config.Rule{Action: config.RuleRedirect, Source: "lan", Dest: "fw:192.0.2.1:3128", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth1"}, "input": {"iif=eth1"}},
 		},
 		{
 			name: "dnat source address list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net:192.0.2.5,198.51.100.5", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
-			want: map[string][]string{"prerouting": {"iif=eth0 saddr=192.0.2.5", "iif=eth0 saddr=198.51.100.5"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 saddr=192.0.2.5", "iif=eth0 saddr=198.51.100.5"},
+				"forward": {"iif=eth0 oif=eth2 saddr=192.0.2.5 daddr=192.0.2.10", "iif=eth0 oif=eth2 saddr=198.51.100.5 daddr=192.0.2.10"}},
 		},
 		{
 			name: "negated address list stays one AND-ed rule",
@@ -2004,17 +2050,20 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 		{
 			name: "dnat origdest",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5"},
-			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "dnat origdest list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5,203.0.113.6"},
-			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5", "iif=eth0 daddr=203.0.113.6"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 daddr=203.0.113.5", "iif=eth0 daddr=203.0.113.6"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "dnat negated origdest list",
 			rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "!203.0.113.5,203.0.113.6"},
-			want: map[string][]string{"prerouting": {"iif=eth0 !daddr=203.0.113.5 !daddr=203.0.113.6"}},
+			want: map[string][]string{"prerouting": {"iif=eth0 !daddr=203.0.113.5 !daddr=203.0.113.6"},
+				"forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}},
 		},
 		{
 			name: "accept origdest",
@@ -2065,7 +2114,7 @@ func TestCompile_CommaZoneLists(t *testing.T) {
 			got := map[string][]string{}
 			for chain, rules := range state.Rules {
 				for _, r := range rules {
-					if r.Tag == tag {
+					if r.Tag == tag || r.Tag == tag+":accept" {
 						got[chain] = append(got[chain], describeRule(r))
 					}
 				}
@@ -2263,11 +2312,112 @@ func TestCompile_DNATGetsNoRuleExtras(t *testing.T) {
 	}
 }

+func TestCompile_DNATImpliedAccept(t *testing.T) {
+	state, err := NewCompiler(listCfg(func(c *config.Config) {
+		c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+		c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+		c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17:8080", Proto: "tcp", DPort: config.PortSpec{"80"}}}
+	})).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	fwd := taggedRules(state, "forward", "rule:0:accept")
+	if len(fwd) != 1 {
+		t.Fatalf("got %d forward accepts, want 1", len(fwd))
+	}
+	want := append(append(append(append(append(matchIfaceName(true, "eth0"), matchIfaceName(false, "eth2")...),
+		mustExprs(t)(matchDestCIDR("192.0.2.17"))...), mustExprs(t)(l4Exprs("tcp", "8080"))...),
+		dnatStatusExprs...), &expr.Verdict{Kind: expr.VerdictAccept})
+	if !reflect.DeepEqual(fwd[0].Exprs, want) {
+		t.Errorf("forward accept = %#v, want %#v", fwd[0].Exprs, want)
+	}
+}
+
+// IPS_DST_NAT = 1<<5, hard-coded so a wrong ctStatusDNAT or ct key fails here.
+var dnatStatusExprs = []expr.Any{
+	&expr.Ct{Key: expr.CtKeySTATUS, Register: 1},
+	&expr.Bitwise{SourceRegister: 1, DestRegister: 1, Len: 4, Mask: binary.NativeEndian.AppendUint32(nil, 32), Xor: []byte{0, 0, 0, 0}},
+	&expr.Cmp{Op: expr.CmpOpNeq, Register: 1, Data: []byte{0, 0, 0, 0}},
+}
+
+func TestCompile_DNATImpliedAcceptGetsNoRuleExtras(t *testing.T) {
+	compile := func(r config.Rule) *FirewallState {
+		state, err := NewCompiler(listCfg(func(c *config.Config) {
+			c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+			c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+			c.Rules = []config.Rule{r}
+		})).Compile()
+		if err != nil {
+			t.Fatalf("Compile() error: %v", err)
+		}
+		return state
+	}
+	plain := config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}}
+	extras := plain
+	extras.RateLimit, extras.Mark, extras.User = "10/sec:5", "0x1", "root"
+	want, got := compile(plain), compile(extras)
+	if len(taggedRules(got, "forward", "rule:0:accept")) != 1 {
+		t.Fatalf("want one forward accept, got %v", got.Rules["forward"])
+	}
+	if !reflect.DeepEqual(got.Rules, want.Rules) {
+		t.Errorf("ratelimit/mark/user changed the DNAT rules:\ngot  %#v\nwant %#v", got.Rules, want.Rules)
+	}
+}
+
+func TestCompile_DNATMatchesSport(t *testing.T) {
+	state, err := NewCompiler(listCfg(func(c *config.Config) {
+		c.Zones["svr"] = config.Zone{Type: config.ZoneIP}
+		c.Interfaces = append(c.Interfaces, config.Interface{Zone: "svr", Interface: "eth2"})
+		c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp",
+			DPort: config.PortSpec{"80"}, SPort: config.PortSpec{"1024"}}}
+	})).Compile()
+	if err != nil {
+		t.Fatalf("Compile() error: %v", err)
+	}
+	m, err := l4Matches("tcp", config.PortSpec{"80"}, config.PortSpec{"1024"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range append(taggedRules(state, "prerouting", "rule:0"), taggedRules(state, "forward", "rule:0:accept")...) {
+		if !containsExprs(r.Exprs, m[0].exprs) {
+			t.Errorf("%s rule lacks the sport match: %#v", r.Chain, r.Exprs)
+		}
+	}
+}
+
+func containsExprs(haystack, needle []expr.Any) bool {
+	for i := 0; i+len(needle) <= len(haystack); i++ {
+		if reflect.DeepEqual(haystack[i:i+len(needle)], needle) {
+			return true
+		}
+	}
+	return false
+}
+
+func mustExprs(t *testing.T) func([]expr.Any, error) []expr.Any {
+	return func(e []expr.Any, err error) []expr.Any {
+		t.Helper()
+		if err != nil {
+			t.Fatal(err)
+		}
+		return e
+	}
+}
+
+func l4Exprs(proto, port string) ([]expr.Any, error) {
+	m, err := l4Matches(proto, config.PortSpec{port}, nil)
+	if err != nil {
+		return nil, err
+	}
+	return m[0].exprs, nil
+}
+
 func TestCompile_CommaZoneListLimitErrors(t *testing.T) {
 	for _, r := range []config.Rule{
 		{Action: config.RuleAccept, Source: "net", Dest: "fw,lan", RateLimit: "10/sec:5"},
 		{Action: config.RuleAccept, Source: "net,lan", Dest: "fw", ConnLimit: "10"},
 		{Action: config.RuleAccept, Source: "net", Dest: "fw:192.0.2.1,198.51.100.1", RateLimit: "10/sec"},
+		{Action: config.RuleDNAT, Source: "net,lan", Dest: "fw:192.0.2.1", RateLimit: "10/sec"},
 		{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"},
@@ -2862,9 +3012,9 @@ func TestCompile_AllIncludesFirewallMatches(t *testing.T) {
 			want: map[string][]string{"input": {"saddr=192.0.2.5"}, "output": {"saddr=192.0.2.5"}, "forward": {"saddr=192.0.2.5"}},
 		},
 		{
-			name: "dnat with all source unchanged",
-			rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "net:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
-			want: map[string][]string{"prerouting": {""}},
+			name: "dnat with all source skips fw",
+			rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "fw:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}},
+			want: map[string][]string{"prerouting": {"iif=eth0"}},
 		},
 	}
 	for _, tc := range cases {