Merge remote-tracking branch 'origin/main' into benvin/dnat-implied-accept

# Conflicts: # internal/nftables/compiler.go # internal/nftables/compiler_test.go
2026-10-04 15:22:41 +11:00
parent ac63f65f2f 599c483cd8
commit 87be6bc6af
2 changed files with 267 additions and 90 deletions
@@ -126,40 +126,23 @@ func (c *Compiler) compileDHCP(state *FirewallState) {
 			continue
 		}
 		name := iface.PhysicalName()
-		// Allow DHCPv4 client traffic (bootpc:68 → bootps:67)
-		state.Rules["input"] = append(state.Rules["input"], ManagedRule{
-			Chain: "input",
-			Exprs: append(append(append(
-				matchIfaceName(true, name),
-				matchProtoNum(unix.IPPROTO_UDP)...),
-				matchSPort(68)...),
-				matchDPort(67)...,
-			),
-			Tag: fmt.Sprintf("dhcp:in:%s", iface.Interface),
-		})
-		// Allow DHCPv4 server → client replies
-		state.Rules["input"] = append(state.Rules["input"], ManagedRule{
-			Chain: "input",
-			Exprs: append(append(append(append(
-				matchIfaceName(true, name),
-				matchProtoNum(unix.IPPROTO_UDP)...),
-				matchSPort(67)...),
-				matchDPort(68)...),
-				&expr.Verdict{Kind: expr.VerdictAccept},
-			),
-			Tag: fmt.Sprintf("dhcp:reply:%s", iface.Interface),
-		})
-		state.Rules["output"] = append(state.Rules["output"], ManagedRule{
-			Chain: "output",
-			Exprs: append(append(append(append(
-				matchIfaceName(false, name),
-				matchProtoNum(unix.IPPROTO_UDP)...),
-				matchSPort(68)...),
-				matchDPort(67)...),
-				&expr.Verdict{Kind: expr.VerdictAccept},
-			),
-			Tag: fmt.Sprintf("dhcp:out:%s", iface.Interface),
-		})
+		dhcp := func(chain, dir string, ifaceMatch []expr.Any) {
+			state.Rules[chain] = append(state.Rules[chain], ManagedRule{
+				Chain: chain,
+				Exprs: append(append(append(append(ifaceMatch,
+					matchNFProto(unix.NFPROTO_IPV4)...),
+					matchProtoNum(unix.IPPROTO_UDP)...),
+					matchDPortRange(67, 68)...),
+					&expr.Verdict{Kind: expr.VerdictAccept}),
+				Tag: fmt.Sprintf("dhcp:%s:%s", dir, iface.Interface),
+			})
+		}
+		// shorewall: udp dport 67:68 both ways between fw and iface, forwarded back out a bridge
+		dhcp("input", "in", matchIfaceName(true, name))
+		dhcp("output", "out", matchIfaceName(false, name))
+		if iface.Options.Bridge {
+			dhcp("forward", "fwd", append(matchIfaceName(true, name), matchIfaceName(false, name)...))
+		}
 	}
 }

@@ -381,7 +364,7 @@ func (c *Compiler) compileRules(state *FirewallState) error {
 			if err != nil {
 				return fmt.Errorf("rule[%d]: %w", i, err)
 			}
-			if len(matches)*c.specCount(rule.Source, rule.Dest, rule.OrigDest, rule.Action) > 1 {
+			if len(matches)*c.specCount(rule.Source, rule.Dest, rule.OrigDest, fwZone, rule.Action) > 1 {
 				return fmt.Errorf("rule[%d]: ratelimit/connlimit cannot be combined with proto, port, zone or address lists (each expanded rule would get its own limiter)", i)
 			}
 		}
@@ -467,39 +450,32 @@ 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 {

-	isDNAT := action == config.RuleDNAT || action == config.RuleRedirect
-	srcs := c.zoneSpecs(srcSpec)
-	if isDNAT {
-		srcs = c.dnatSourceSpecs(srcSpec, fwZone)
-	}
-	for _, src := range srcs {
-		for _, srcAddr := range splitAddrs(src.Addr) {
-			if isDNAT {
-				if dnatSkipsIntrazone(srcSpec, src.Zone, dstSpec) {
-					continue
-				}
+	if action == config.RuleDNAT || action == config.RuleRedirect {
+		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 isDNAT {
+				for _, od := range splitAddrs(origDest) {
 					if err := c.compileDNATRule(state, tag, src.Zone, srcAddr, od, dstSpec, proto, dports, sports, action, logLevel); err != nil {
 						return err
 					}
-					continue
 				}
-				for _, dst := range c.zoneSpecs(dstSpec) {
-					// 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
-					}
-					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
 					}
 				}
 			}
@@ -562,18 +538,39 @@ func (c *Compiler) compileDNATAccept(state *FirewallState, tag, srcZone, srcAddr
 }

 // specCount is how many zone/address combinations compileOneRule expands src and dst into.
-func (c *Compiler) specCount(srcSpec, dstSpec, origDest string, action config.RuleAction) int {
-	count := func(spec string) (n int) {
-		for _, z := range c.zoneSpecs(spec) {
-			n += len(splitAddrs(z.Addr))
-		}
-		return n
-	}
-	n := count(srcSpec) * len(splitAddrs(origDest))
+func (c *Compiler) specCount(srcSpec, dstSpec, origDest, fwZone string, action config.RuleAction) int {
+	n := 0
 	if action == config.RuleDNAT || action == config.RuleRedirect {
-		return n
+		for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) {
+			n += len(splitAddrs(src.Addr))
+		}
+		return n * len(splitAddrs(origDest))
 	}
-	return n * count(dstSpec)
+	for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) {
+		n += len(splitAddrs(p[0].Addr)) * len(splitAddrs(p[1].Addr))
+	}
+	return n * len(splitAddrs(origDest))
+}
+
+// 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})
+		}
+	}
+	return out
 }

 // zoneSpecs expands a comma zone list; "all"/"any" stay global and "all!x,y" becomes every zone but x and y.
@@ -592,6 +589,26 @@ func (c *Compiler) zoneSpecs(spec string) []config.ZoneSpec {
 	return out
 }

+// 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, bool) {
+	out, global := specs, false
+	for _, s := range specs {
+		if fwZone != "" && isGlobalZone(s.Zone) {
+			global = true
+			if fw := (config.ZoneSpec{Zone: fwZone, Addr: s.Addr}); !slices.Contains(out, fw) {
+				out = append(out, fw)
+			}
+		}
+	}
+	return out, global
+}
+
+func isGlobalZone(spec string) bool {
+	zone, _ := splitZoneSpec(spec)
+	base := strings.TrimSuffix(zone, "+")
+	return base == "all" || base == "any"
+}
+
 func isZoneExclusion(spec string) bool {
 	base, _, ok := strings.Cut(spec, "!")
 	base = strings.TrimSuffix(base, "+")
@@ -1620,10 +1637,14 @@ func matchOrigDest(addr string) ([]expr.Any, error) {
 	if err != nil {
 		return nil, err
 	}
-	return append([]expr.Any{
+	return append(matchNFProto(proto), dst...), nil
+}
+
+func matchNFProto(proto byte) []expr.Any {
+	return []expr.Any{
 		&expr.Meta{Key: expr.MetaKeyNFPROTO, Register: 1},
 		&expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{proto}},
-	}, dst...), nil
+	}
 }

 func matchAddrCIDR(cidr string, isSrc bool) ([]expr.Any, error) {
@@ -1013,38 +1013,84 @@ func TestCompile_DHCP(t *testing.T) {
 		Zones: map[string]config.Zone{
 			"fw":  {Type: config.ZoneFirewall},
 			"net": {Type: config.ZoneIP},
+			"loc": {Type: config.ZoneIP},
 		},
 		Interfaces: []config.Interface{
 			{Zone: "net", Interface: "eth0", Options: config.InterfaceOptions{DHCP: true}},
+			{Zone: "loc", Interface: "br0", Options: config.InterfaceOptions{DHCP: true, Bridge: true}},
 		},
 		Policy: []config.Policy{
 			{Source: "all", Dest: "all", Action: config.PolicyDrop},
 		},
 		PortGroups: make(map[string]config.PortGroup),
 	}
-	c := NewCompiler(cfg)
-	state, err := c.Compile()
+	state, err := NewCompiler(cfg).Compile()
 	if err != nil {
 		t.Fatalf("Compile() error: %v", err)
 	}

-	foundIn := false
-	foundOut := false
-	for _, r := range state.Rules["input"] {
-		if r.Tag == "dhcp:in:eth0" || r.Tag == "dhcp:reply:eth0" {
-			foundIn = true
+	find := func(chain, tag string) *ManagedRule {
+		for i, r := range state.Rules[chain] {
+			if r.Tag == tag {
+				return &state.Rules[chain][i]
+			}
+		}
+		return nil
+	}
+	for _, want := range []struct{ chain, tag, iif, oif string }{
+		{"input", "dhcp:in:eth0", "eth0", ""},
+		{"output", "dhcp:out:eth0", "", "eth0"},
+		{"input", "dhcp:in:br0", "br0", ""},
+		{"output", "dhcp:out:br0", "", "br0"},
+		{"forward", "dhcp:fwd:br0", "br0", "br0"},
+	} {
+		r := find(want.chain, want.tag)
+		if r == nil {
+			t.Errorf("%s: no rule %s", want.chain, want.tag)
+			continue
+		}
+		metas := map[expr.MetaKey][]byte{}
+		for i := 0; i+1 < len(r.Exprs); i++ {
+			if m, ok := r.Exprs[i].(*expr.Meta); ok {
+				if c, ok := r.Exprs[i+1].(*expr.Cmp); ok && c.Op == expr.CmpOpEq {
+					metas[m.Key] = c.Data
+				}
+			}
+		}
+		if got := metas[expr.MetaKeyNFPROTO]; !bytes.Equal(got, []byte{unix.NFPROTO_IPV4}) {
+			t.Errorf("%s: nfproto %v, want ipv4 guard", want.tag, got)
+		}
+		if got := metas[expr.MetaKeyL4PROTO]; !bytes.Equal(got, []byte{unix.IPPROTO_UDP}) {
+			t.Errorf("%s: l4proto %v, want udp", want.tag, got)
+		}
+		for key, name := range map[expr.MetaKey]string{expr.MetaKeyIIFNAME: want.iif, expr.MetaKeyOIFNAME: want.oif} {
+			got, ok := metas[key]
+			if name == "" {
+				if ok {
+					t.Errorf("%s: unexpected meta %v match %q", want.tag, key, got)
+				}
+			} else if string(got) != name+"\x00" {
+				t.Errorf("%s: meta %v %q, want %q", want.tag, key, got, name)
+			}
+		}
+		v, ok := r.Exprs[len(r.Exprs)-1].(*expr.Verdict)
+		if !ok || v.Kind != expr.VerdictAccept {
+			t.Errorf("%s: last expr %#v, want accept verdict", want.tag, r.Exprs[len(r.Exprs)-1])
+		}
+		var lo, hi []byte
+		for _, e := range r.Exprs {
+			if c, ok := e.(*expr.Cmp); ok && c.Op == expr.CmpOpGte {
+				lo = c.Data
+			} else if ok && c.Op == expr.CmpOpLte {
+				hi = c.Data
+			}
+		}
+		if !bytes.Equal(lo, []byte{0, 67}) || !bytes.Equal(hi, []byte{0, 68}) {
+			t.Errorf("%s: dport range %v-%v, want 67-68", want.tag, lo, hi)
 		}
 	}
-	for _, r := range state.Rules["output"] {
-		if r.Tag == "dhcp:out:eth0" {
-			foundOut = true
-		}
-	}
-	if !foundIn {
-		t.Error("no DHCP input rule found for eth0")
-	}
-	if !foundOut {
-		t.Error("no DHCP output rule found for eth0")
+	if find("forward", "dhcp:fwd:eth0") != nil {
+		t.Error("non-bridge eth0 must not forward DHCP")
 	}
 }

@@ -2372,6 +2418,10 @@ func TestCompile_CommaZoneListLimitErrors(t *testing.T) {
 		{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"},
+		{Action: config.RuleAccept, Source: "net,all", Dest: "fw", RateLimit: "10/sec"},
 	} {
 		t.Run(r.Source+">"+r.Dest, func(t *testing.T) {
 			cfg := &config.Config{
@@ -2949,3 +2999,109 @@ func TestCompile_ConntrackHelperZones(t *testing.T) {
 		})
 	}
 }
+
+func TestCompile_AllIncludesFirewallMatches(t *testing.T) {
+	cases := []struct {
+		name string
+		rule config.Rule
+		want map[string][]string
+	}{
+		{
+			name: "all address kept on added fw rules",
+			rule: config.Rule{Action: config.RuleAccept, Source: "all:192.0.2.5", Dest: "all"},
+			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 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 {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &config.Config{
+				Settings:   config.Settings{TableName: "test", AddressFamily: config.FamilyINET},
+				Zones:      map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}},
+				Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}},
+				Rules:      []config.Rule{tc.rule},
+				PortGroups: map[string]config.PortGroup{},
+			}
+			state := mustCompile(t, cfg)
+			got := map[string][]string{}
+			for _, chain := range []string{"prerouting", "input", "output", "forward"} {
+				for _, r := range taggedRules(state, chain, "rule:0") {
+					got[chain] = append(got[chain], describeRule(r))
+				}
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("rules = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestCompile_AllIncludesFirewall(t *testing.T) {
+	cases := []struct {
+		src, dst string
+		want     map[string]int
+	}{
+		{"all", "all", map[string]int{"input": 1, "output": 1, "forward": 1}},
+		{"net", "all", map[string]int{"input": 1, "output": 0, "forward": 1}},
+		{"all", "net", map[string]int{"input": 0, "output": 1, "forward": 1}},
+		{"all", "fw", map[string]int{"input": 1, "output": 0, "forward": 0}},
+		{"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) {
+			cfg := &config.Config{
+				Settings: config.Settings{TableName: "test", AddressFamily: config.FamilyINET},
+				Zones: map[string]config.Zone{
+					"fw":  {Type: config.ZoneFirewall},
+					"net": {Type: config.ZoneIP},
+					"loc": {Type: config.ZoneIP},
+				},
+				Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}, {Zone: "loc", Interface: "eth1"}},
+				Rules: []config.Rule{
+					{Action: config.RuleAccept, Source: tc.src, Dest: tc.dst, Proto: "icmp", DPort: config.PortSpec{"8"}},
+				},
+				PortGroups: map[string]config.PortGroup{},
+			}
+			state, err := NewCompiler(cfg).Compile()
+			if err != nil {
+				t.Fatalf("Compile() error: %v", err)
+			}
+			for chain, want := range tc.want {
+				got := 0
+				for _, r := range state.Rules[chain] {
+					if r.Tag == "rule:0" {
+						got++
+					}
+				}
+				if got != want {
+					t.Errorf("%s: got %d rule:0 entries, want %d", chain, got, want)
+				}
+			}
+		})
+	}
+}
+
+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)
+		}
+	}
+}