Merge pull request 'Accept DHCP on dhcp interfaces as shorewall does' (#27) from benvin/dhcp-option into main

Reviewed-on: #27
2026-10-04 15:21:51 +11:00
parent 110b109d97 0551120eec
commit 599c483cd8
2 changed files with 86 additions and 53 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)...))
+		}
 	}
 }

@@ -1595,10 +1578,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")
 	}
 }