diff --git a/apf_linux.go b/apf_linux.go index 760f16c..a92b93e 100644 --- a/apf_linux.go +++ b/apf_linux.go @@ -486,7 +486,7 @@ func (f *APF) ParseIPList(filePath string, action Action) (rules []*Rule, err er rules = append(rules, rule) } else { // Try to parse IP. - fam, ok := csfAddrFamily(trimmed) + fam, ok := parseAddrFamily(trimmed) if !ok { continue } @@ -543,9 +543,10 @@ func (f *APF) ParsePorts(val string, proto Protocol, out bool) (rules []*Rule) { // features apf's native config cannot express. func (f *APF) hook() *hookScript { return &hookScript{ - rulePrefix: f.rulePrefix, - hookPath: APFHook, - hookPerm: 0750, + rulePrefix: f.rulePrefix, + hookPath: APFHook, + hookPerm: 0750, + ipv6Enabled: f.ipv6Enabled, } } @@ -956,37 +957,14 @@ func (f *APF) addrField(addr string) string { return addr } -// MarshalAdvRule encodes a rule as an apf advanced allow/deny line. An apf -// advanced rule is tcp/udp only and must carry a source or destination address. -func (f *APF) MarshalAdvRule(r *Rule) (string, error) { - if r.Proto.IsICMP() { - return "", fmt.Errorf("apf advanced rules do not support icmp") - } - if r.Source == "" && r.Destination == "" { - return "", fmt.Errorf("an apf advanced rule requires a source or destination address") - } - // apf's advanced rule holds a single address field, so a rule matching both a - // source and a destination cannot be expressed; reject it rather than silently - // dropping the destination (which would install a broader rule than asked and - // leave the rule unremovable). Mirrors the dual-port guard below. - if r.Source != "" && r.Destination != "" { - return "", fmt.Errorf("apf advanced rules cannot match both a source and destination address") - } - // apf's port field takes a single port or an underscore range (no comma list), - // and there is a single port-flow field, so a rule cannot match both a source - // and a destination port at once. - if len(r.PortSpecs()) > 1 || len(r.SourcePortSpecs()) > 1 { - return "", fmt.Errorf("apf advanced rules do not support a port list in this model") - } - if r.HasPorts() && r.HasSourcePorts() { - return "", fmt.Errorf("apf advanced rules cannot match both a source and destination port") - } - +// MarshalAdvRule encodes a rule as an apf advanced allow/deny line: an optional +// protocol field, a direction, one port-flow field (a source or a destination port) +// and one address field, joined by ":". It validates nothing; addRule/RemoveRule +// route every shape the line cannot carry elsewhere first (see needsHook). +func (f *APF) MarshalAdvRule(r *Rule) string { // apf's advanced rule carries an optional protocol field, and treats a line // without one as both transports: its trust parser emits a `-p tcp` rule and a - // `-p udp` rule for it. So TCPUDP is written by omitting the field, and no other - // protocol may be: ProtocolAny would mean every IP protocol, which the omitted - // field does not express. + // `-p udp` rule for it. So TCPUDP is written by omitting the field. var parts []string switch r.Proto { case TCP: @@ -995,15 +973,15 @@ func (f *APF) MarshalAdvRule(r *Rule) (string, error) { parts = append(parts, "udp") case TCPUDP: // No protocol field: apf reads that as tcp plus udp. - default: - return "", fmt.Errorf("an apf advanced rule carries tcp, udp or both: %w", ErrUnsupported) } if r.IsOutput() { parts = append(parts, "out") } else { parts = append(parts, "in") } - // The port-flow field: a source port or a destination port. + // The port-flow field: a source port or a destination port. apf's field holds a + // single port or one underscore range; needsHook routes a multi-port list to the + // hook, so only one spec ever arrives. if specs := r.SourcePortSpecs(); len(specs) == 1 { parts = append(parts, "s="+f.portToken(specs[0])) } else if specs := r.PortSpecs(); len(specs) == 1 { @@ -1014,10 +992,99 @@ func (f *APF) MarshalAdvRule(r *Rule) (string, error) { } else if r.Destination != "" { parts = append(parts, "d="+f.addrField(r.Destination)) } - return strings.Join(parts, ":"), nil + return strings.Join(parts, ":") } -// EditIPList adds or removes a rule in an apf allow_hosts/deny_hosts file, rewriting it in place. +// parseListLine parses one apf allow_hosts/deny_hosts rule line into the rule it +// holds: an advanced rule, or a plain address line, which is a single bidirectional +// DirAny rule matching every protocol. It returns nil for a line that is neither, +// which the caller passes through untouched. The action comes from the file +// (allow_hosts is accept, deny_hosts takes the drop/reject action conf.apf sets); +// the line encodes none of its own. +func (f *APF) parseListLine(line string, action Action) *Rule { + if strings.Contains(line, "=") { + return f.ParseAdvRule(line, action) + } + fam, ok := parseAddrFamily(line) + if !ok { + return nil + } + return &Rule{Direction: DirAny, Family: fam, Source: line, Action: action} +} + +// listRows returns the allow_hosts/deny_hosts rows a rule materializes into, in write +// order, or none for a shape the trust files cannot hold. The rule must already carry +// the action its file implies (see EditIPList's match), since each row's read-back +// form is compared against lines stamped with it. +// +// A rule never fans out across transports: apf's trust parser reads a protocol-less +// advanced line as a `-p tcp` rule plus a `-p udp` rule, so MarshalAdvRule writes a +// TCPUDP rule as a single line. A port-only deny does fan out per family: apf requires +// an address in the advanced line's field position, and the "any" network placeholder +// it uses there is family-specific, so a family-neutral rule would otherwise silently +// become IPv4-only. The fan-out covers the families apf actually enforces (see +// writeFamilyRows; with conf.apf's USE_IPV6 off that is IPv4 alone). parseAddr +// normalizes the placeholder back to an empty address, so each row reads back as the +// address-less rule it stands for. +func (f *APF) listRows(action Action, match *Rule) []listRow { + hasIP := match.Source != "" || match.Destination != "" + var rows []listRow + switch { + case hasIP && (match.HasPorts() || match.HasSourcePorts()): + // A port rule with an address is an advanced rule. + rows = append(rows, listRow{line: f.MarshalAdvRule(match), read: match}) + case hasIP: + // A bare all-protocol host allow/deny: a single address matching every + // protocol. apf's trust files hold no other portless address shape — a + // concrete-protocol host or a source+destination pair — so AddRule diverts + // those to the raw-iptables hook (hostNeedsHook) and never reaches here with + // one. A direct caller of this exported writer that supplies such a shape gets + // a best-effort single-address write, not a guard. + addr := match.Source + if addr == "" { + addr = match.Destination + } + // The plain line is bidirectional and names its address as the source, which is + // the frame the scan reads it back in. + read := *match + read.Direction = DirAny + read.Source, read.Destination = addr, "" + rows = append(rows, listRow{line: addr, read: &read}) + case action != Accept && match.HasPorts(): + specs := match.PortSpecs() + for _, fam := range writeFamilyRows(f.ipv6Enabled, match) { + placeholder := "0.0.0.0/0" + if fam.impliedFamily() == IPv6 { + // MarshalAdvRule brackets an IPv6 literal for the colon-separated format. + placeholder = "::/0" + } + // Pin the row to the one shape apf's single port-flow field holds: one + // destination port. needsHook routes a multi-port list and a source-port match + // to the hook before AddRule reaches here, so this only bounds a direct caller + // of this exported writer — without it MarshalAdvRule would emit a portless + // line (denying the whole protocol) for a list, or prefer the source port over + // the destination one. + read := *fam + read.Port, read.Ports = 0, specs[:1] + read.SourcePort, read.SourcePorts = 0, nil + // The row reads back address-less; only the line carries the placeholder. + row := read + if row.IsOutput() { + row.Destination = placeholder + } else { + row.Source = placeholder + } + rows = append(rows, listRow{line: f.MarshalAdvRule(&row), read: &read}) + } + } + return rows +} + +// EditIPList adds or removes a rule in an apf allow_hosts/deny_hosts file, rewriting +// it in place. An add expands the rule into the rows it materializes into (see +// listRows), notes which of them the file already holds, and appends only the rest, +// so a rule that fans out across families is completed rather than duplicated on +// every reconcile. A removal drops every line the target covers. func (f *APF) EditIPList(ctx context.Context, filePath string, action Action, r *Rule, remove bool) error { // Read the allow_hosts/deny_hosts rule list. fd, err := os.Open(filePath) @@ -1034,13 +1101,20 @@ func (f *APF) EditIPList(ctx context.Context, filePath string, action Action, r defer af.Abort() scanner := bufio.NewScanner(fd) - exists := false // A deny_hosts entry takes on the drop/reject action set in apf.conf (and // allow_hosts is accept), so a rule read back is stamped with the file's // action. Resolve the incoming rule's action the same way via resolveAction // before matching. match := *r match.Action = f.resolveAction(action, r.Proto) + // The rows an add must end up with, and which of them the scan finds already in + // the file. A removal wants no rows: it matches the target against each line + // directly, since a line it must drop need not be one this library would write. + var rows []listRow + if !remove { + rows = f.listRows(action, &match) + } + present := make([]bool, len(rows)) // pending holds the full-line comments seen immediately above a rule, so they // can be dropped together with a removed rule (they are its comment) or written @@ -1089,149 +1163,60 @@ func (f *APF) EditIPList(ctx context.Context, filePath string, action Action, r continue } - if strings.Contains(line, "=") { - rule := f.ParseAdvRule(line, action) - if rule == nil { - flush() - _, _ = fmt.Fprintln(af, orig) - continue - } - // Match family-aware: the address-less "port-only deny" form parses to - // Source="" for both families, so a bare EqualBase would let an IPv4 line - // stand in for its IPv6 twin. EqualForDedup/EqualForRemoval fold in the - // family coverage the add and remove paths respectively need. - var famMatch bool - if remove { - famMatch = rule.EqualForRemoval(&match, true) - } else { - famMatch = rule.EqualForDedup(&match, true) - } - if famMatch { - exists = true - if !remove { - flush() - _, _ = fmt.Fprintln(af, orig) - } else { - drop() - f.ConfigChanged = true - } - } else { - flush() - _, _ = fmt.Fprintln(af, orig) - } - } else { - // Try to parse IP. - fam, ok := csfAddrFamily(line) - if !ok { - flush() - _, _ = fmt.Fprintln(af, orig) - continue - } + // A line neither form parses is not a rule; pass it through untouched. + cur := f.parseListLine(line, action) + if cur == nil { + flush() + _, _ = fmt.Fprintln(af, orig) + continue + } - // A plain IP line is one bidirectional DirAny rule; match the target - // against it in the inbound frame (canonicalMatch), so a DirAny or a - // concrete-direction input/output target that names this host lines up. - plainRule := &Rule{ - Direction: DirAny, - Family: fam, - Source: line, - Action: action, + // A removal drops every line the target covers, along with its comment. A + // family-neutral target touches each of the concrete lines it was written as; + // that coverage is folded into EqualForRemoval. + if remove { + if cur.EqualForRemoval(&match, true) { + drop() + f.ConfigChanged = true + continue } - if plainRule.EqualBase(match.canonicalMatch(), false) { - exists = true - if !remove { - flush() - _, _ = fmt.Fprintln(af, orig) - } else { - drop() - f.ConfigChanged = true - } - } else { - flush() - _, _ = fmt.Fprintln(af, orig) + flush() + _, _ = fmt.Fprintln(af, orig) + continue + } + + // An add keeps every line and only notes which wanted rows the file already + // covers, so the tail writes the rest. Coverage rather than a text compare, so + // a row is satisfied by an existing line that spans it (a protocol-less TCPUDP + // line absorbing a tcp row) and by one spelled differently but meaning the same. + for i := range rows { + if !present[i] && cur.EqualForDedup(rows[i].read, true) { + present[i] = true } } + flush() + _, _ = fmt.Fprintln(af, orig) } // Write any trailing comments that followed the last rule. flush() - // If not exists and not remove, try adding the rule. - if !exists && !remove { - writeComment := func() { - if c := combineComment(f.rulePrefix, r.Comment); c != "" { - _, _ = fmt.Fprintln(af, "# "+c) - } + // Append the wanted rows the file does not already hold. A rule that fans out is + // completed row by row: when only a subset is present (the IPv4 line but not its + // IPv6 twin, from a prior single-family add or a manual edit) the missing rows + // must still be written, or that family stays open while the library reports the + // rule in force. + writeComment := func() { + if c := combineComment(f.rulePrefix, r.Comment); c != "" { + _, _ = fmt.Fprintln(af, "# "+c) } - hasIP := r.Source != "" || r.Destination != "" - switch { - case hasIP && (r.HasPorts() || r.HasSourcePorts()): - // A port rule with an address is an advanced rule. - line, err := f.MarshalAdvRule(r) - if err != nil { - _ = fd.Close() - return err - } - f.ConfigChanged = true - writeComment() - _, _ = fmt.Fprintln(af, line) - case hasIP: - // A bare all-protocol host allow/deny: a single address matching every - // protocol. apf's trust files hold no other portless address shape — a - // concrete-protocol host or a source+destination pair — so AddRule diverts - // those to the raw-iptables hook (hostNeedsHook) and never reaches here - // with one. A direct caller of this exported writer that supplies such a - // shape gets a best-effort single-address write, not a guard. - f.ConfigChanged = true - writeComment() - if r.Source != "" { - _, _ = fmt.Fprintln(af, r.Source) - } else { - _, _ = fmt.Fprintln(af, r.Destination) - } - case action != Accept && r.HasPorts(): - // A port-only deny is written as an advanced rule against any address, - // which apf requires in the field position. apf's port field holds a - // single port or underscore range only; a multi-port list has no advanced- - // rule form, so AddRule diverts it to the hook (needsHook) and this - // branch only sees a single port/range. A direct caller supplying a list - // gets a best-effort single-port write. - specs := r.PortSpecs() - // apf requires an address in the field position, so a port-only deny - // uses the "any" network as a placeholder. That literal is family- - // specific (0.0.0.0/0 vs ::/0) and read back with its family, so emit - // the placeholder matching the rule's family. A family-neutral rule has - // no single literal, so emit a line per family rather than let the rule - // silently become IPv4-only; each line reads back as its own rule. - writeAny := func(placeholder string) { - var tokens []string - if r.Proto != TCPUDP { - tokens = append(tokens, r.Proto.String()) - } - if r.IsOutput() { - tokens = append(tokens, "out") - } else { - tokens = append(tokens, "in") - } - tokens = append(tokens, "d="+f.portToken(specs[0])) - if r.IsOutput() { - tokens = append(tokens, "d="+placeholder) - } else { - tokens = append(tokens, "s="+placeholder) - } - writeComment() - _, _ = fmt.Fprintln(af, strings.Join(tokens, ":")) - } - switch r.impliedFamily() { - case IPv6: - writeAny("[::/0]") - case IPv4: - writeAny("0.0.0.0/0") - default: - writeAny("0.0.0.0/0") - writeAny("[::/0]") - } - f.ConfigChanged = true + } + for i, row := range rows { + if present[i] { + continue } + f.ConfigChanged = true + writeComment() + _, _ = fmt.Fprintln(af, row.line) } _ = fd.Close() @@ -1258,9 +1243,9 @@ func (f *APF) isConfRule(r *Rule) bool { // nativeICMPv6 reports whether an ICMPv6 rule can be carried by apf's native // IG_ICMPV6_TYPES/EG_ICMPV6_TYPES lists (an address-less accept, optionally typed) // and so belongs in conf.apf rather than the raw-iptables hook. The shared -// ruleNeedsHook diverts every ICMPv6 rule to the hook — correct for csf, which has -// no native v6 type list — so apf overrides that only for the rules its config can -// actually express, leaving an ICMPv6 rule that also needs state/interface/log/ +// ruleNeedsHook diverts every ICMPv6 rule to the hook, since not every backend it +// serves has a native v6 type list; apf overrides that only for the rules its config +// can actually express, leaving an ICMPv6 rule that also needs state/interface/log/ // rate matching (which conf.apf cannot carry) on the hook path. func (f *APF) nativeICMPv6(r *Rule) bool { return r.Proto == ICMPv6 && r.State == 0 && r.InInterface == "" && r.OutInterface == "" && @@ -1281,8 +1266,8 @@ func (f *APF) barePortAccept(r *Rule) bool { // dualStackPortNeedsHook reports whether a bare tcp/udp port accept pinned to a // single family must be injected through the hook. apf's IG_*_CPORTS/EG_*_CPORTS // lists are dual-stack — one list applied to both the ip and ip6 tables — so they -// express only a FamilyAny port (unlike csf, whose TCP_IN/TCP6_IN split the -// families). A single-family one is written per-family through the hook instead, +// express only a FamilyAny port; the lists have no per-family form to pin one to. +// A single-family port accept is written per-family through the hook instead, // whose iptables (or ip6tables) rule carries just that family; removing one family // of a FamilyAny CPORTS entry splits it (see removeDualStackPort). ICMP keeps its // concrete family (its type lists are per-family), so this gates on a port match. @@ -1295,10 +1280,10 @@ func (f *APF) dualStackPortNeedsHook(r *Rule) bool { // gate between the hook path and apf's config files: everything it rejects (returns // true) is written to the hook, everything it accepts (returns false) maps onto // conf.apf or the allow_hosts/deny_hosts trust files. The shared shapes -// (ruleNeedsHook, bareHostOneWay, hostNeedsHook) and the two apf shapes RemoveRule -// reuses to route a split (dualStackPortNeedsHook, nativeICMPv6) keep their own -// predicates; the apf-only, single-use port/source-port/connlimit/icmp tests are -// inlined here as their sole caller. +// (ruleNeedsHook, bareHostOneWay, hostNeedsHook, advRuleNeedsHook) and the two apf +// shapes RemoveRule reuses to route a split (dualStackPortNeedsHook, nativeICMPv6) +// keep their own predicates; the apf-only, single-use port/source-port/connlimit/icmp +// tests are inlined here as their sole caller. func (f *APF) needsHook(r *Rule) bool { // Features apf's native config cannot model — connection state, per-rule // interface, logging, rate limiting, forward-chain routing, ICMPv6, or a @@ -1314,6 +1299,13 @@ func (f *APF) needsHook(r *Rule) bool { if bareHostOneWay(r) || hostNeedsHook(r) { return true } + // A ported source+destination pair, a source port matched with a destination port, + // and an address-less source-port match all overflow the advanced line's single + // address and single port-flow field (see advRuleNeedsHook); iptables matches each + // directly, so they go to the hook. + if advRuleNeedsHook(r) { + return true + } // A multi-port tcp/udp list apf's config cannot carry: its advanced rule holds a // single port or one underscore range, and the only native multi-port shape is an // address-less accept (isConfRule, carried by the IG_*_CPORTS comma lists); @@ -1322,12 +1314,6 @@ func (f *APF) needsHook(r *Rule) bool { (len(r.PortSpecs()) > 1 || len(r.SourcePortSpecs()) > 1) && !f.isConfRule(r) { return true } - // A source-port match with no address has no advanced-rule form (advanced rules - // require an address); iptables matches --sport directly, so it goes to the hook. - if r.HasSourcePorts() && r.Source == "" && r.Destination == "" && - onProtocolAxis(r.Proto) { - return true - } // A connection limit conf.apf's IG_*_CLIMIT cannot express — anything but a // per-source cap on a single address-less inbound tcp/udp port rejecting the // excess (isConnLimitRule) — goes to the hook's `-m connlimit` match. @@ -1381,10 +1367,16 @@ func (f *APF) addRule(ctx context.Context, zoneName string, r *Rule, enforceIPv6 return nil } + // Verify the rule is valid with iptables. + if err := iptablesRuleValid(r); err != nil { + return fmt.Errorf("%v: %w", err, ErrUnsupported) + } + // Any shape apf's native config cannot express (a stateful/interface/logged/ // rate-limited rule, a one-way or concrete-protocol host, a source+destination - // pair, a multi-port list, an address-less source-port match, a non-native - // connection limit, a non-native ICMPv4 rule, or a single-family port accept) is + // pair, a source-and-destination port match, a multi-port list, an address-less + // source-port match, a non-native connection limit, a non-native ICMPv4 rule, or + // a single-family port accept) is // injected as a raw iptables rule through the apf pre-hook. See needsHook for // each clause; everything past this gate maps onto apf's own config files. if f.needsHook(r) { @@ -1392,9 +1384,6 @@ func (f *APF) addRule(ctx context.Context, zoneName string, r *Rule, enforceIPv6 f.ConfigChanged = f.ConfigChanged || changed return err } - if err := iptablesRuleValid(r); err != nil { - return fmt.Errorf("%v: %w", err, ErrUnsupported) - } // A native connection-limit rule maps onto conf.apf's IG_*_CLIMIT lists (a // non-native one was diverted to the hook above by needsHook). diff --git a/apf_linux_test.go b/apf_linux_test.go index 5e9fe7c..e934a41 100644 --- a/apf_linux_test.go +++ b/apf_linux_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -23,8 +24,7 @@ func TestAPFAdvRules(t *testing.T) { {&Rule{Direction: DirOutput, Proto: UDP, Port: 53, Destination: "192.0.2.1", Family: IPv4, Action: Accept}, "udp:out:d=53:d=192.0.2.1"}, } for _, c := range cases { - got, err := fw.MarshalAdvRule(c.rule) - require.NoError(t, err, "failed to marshal %+v", *c.rule) + got := fw.MarshalAdvRule(c.rule) require.Equal(t, c.want, got, "marshal %+v", *c.rule) parsed := fw.ParseAdvRule(got, c.rule.Action) @@ -40,16 +40,18 @@ func TestAPFAdvRules(t *testing.T) { require.Equal(t, IPv6, r.Family) require.EqualValues(t, 443, r.Port) - // MarshalAdvRule rejects rules apf cannot express as advanced rules. - bad := []*Rule{ + // The shapes an advanced line cannot hold never reach MarshalAdvRule: needsHook + // routes an addressed ICMP rule and a multi-port list to the raw-iptables hook, + // and an address-less port accept lands in conf.apf's CPORTS lists instead. + routed := []*Rule{ {Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "1.2.3.4", Action: Accept}, - {Proto: TCP, Port: 80, Action: Accept}, {Proto: TCP, Ports: []PortRange{{Start: 80}, {Start: 443}}, Source: "1.2.3.4", Action: Accept}, } - for _, r := range bad { - _, err := fw.MarshalAdvRule(r) - require.Error(t, err, "expected error marshalling %+v", *r) + for _, r := range routed { + require.True(t, fw.needsHook(r), "expected %+v to be routed to the hook, not marshalled", *r) } + require.True(t, fw.isConfRule(&Rule{Proto: TCP, Port: 80, Action: Accept}), + "an address-less port accept belongs in conf.apf, not an advanced line") } // A port-only deny whose action is Drop (conf.apf's default ALL_STOP=DROP) must @@ -85,8 +87,7 @@ func TestAPFSourcePorts(t *testing.T) { {&Rule{Proto: UDP, SourcePorts: []PortRange{{Start: 1024, End: 2048}}, Source: "192.0.2.1", Family: IPv4, Action: Accept}, "udp:in:s=1024_2048:s=192.0.2.1"}, } for _, c := range cases { - got, err := fw.MarshalAdvRule(c.rule) - require.NoError(t, err, "failed to marshal %+v", *c.rule) + got := fw.MarshalAdvRule(c.rule) require.Equal(t, c.want, got, "marshal %+v", *c.rule) parsed := fw.ParseAdvRule(got, c.rule.Action) @@ -95,12 +96,14 @@ func TestAPFSourcePorts(t *testing.T) { "round-trip mismatch: input %+v, line %q, output %+v", *c.rule, got, parsed) } - // Matching both a source and a destination port is not representable. - _, err := fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 22, SourcePort: 1234, Source: "1.2.3.4", Action: Accept}) - require.Error(t, err, "expected error matching both source and destination ports") + // The single port-flow field holds one port match, so a source port matched with + // a destination port has no advanced-line form and routes to the hook instead. + require.True(t, fw.needsHook(&Rule{Proto: TCP, Port: 22, SourcePort: 1234, Source: "1.2.3.4", Action: Accept}), + "a dual-port rule must route to the hook") - // A source-port rule with no address has no advanced-rule form, so it routes to - // the hook; one carrying an address stays on the native advanced-rule path. + // A source-port rule with no address has no advanced-rule form either (an advanced + // line requires an address), so it routes to the hook; one carrying an address + // stays on the native advanced-rule path. require.True(t, fw.needsHook(&Rule{Proto: TCP, SourcePort: 1234, Action: Accept}), "an address-less source-port rule must route to the hook") require.False(t, fw.needsHook(&Rule{Proto: TCP, SourcePort: 1234, Source: "1.2.3.4", Action: Accept})) @@ -588,14 +591,13 @@ func TestAPFDropRuleRemovable(t *testing.T) { } // A ported apf advanced rule that matches both a source and a destination address -// cannot be expressed (apf's advanced rule holds a single address field) and must -// be rejected rather than silently dropping the destination. The portless bare -// form is not tested here: AddRule diverts it to the raw-iptables hook -// (hostNeedsHook), so the writer never sees it. -func TestAPFDualAddressRejected(t *testing.T) { +// cannot be expressed (apf's advanced rule holds a single address field), so AddRule +// routes it to the raw-iptables hook rather than silently dropping the destination. +// The portless bare form takes the same path one predicate earlier (hostNeedsHook). +func TestAPFDualAddressRouted(t *testing.T) { fw := new(APF) - _, err := fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 22, Source: "1.2.3.4", Destination: "5.6.7.8", Action: Accept}) - require.Error(t, err, "a dual-address apf advanced rule must be rejected") + require.True(t, fw.needsHook(&Rule{Proto: TCP, Port: 22, Source: "1.2.3.4", Destination: "5.6.7.8", Action: Accept}), + "a ported dual-address rule must be routed to the hook, never marshalled as an advanced line") } // APF's IG_*_CPORTS lists are dual-stack (one list applied to both v4 and v6), so @@ -628,11 +630,11 @@ func TestAPFConnLimitFamilyIsAny(t *testing.T) { "FamilyAny connlimit must equal the APF read-back or Sync churns") } -// TestAPFBareProtocolRoutesToHook is the apf analogue of the csf case: a bare -// protocol match (no port, no address) has no native apf construct but iptables -// expresses it directly, so AddRule diverts it to the pre-hook via needsHook -// rather than rejecting it. A native address-less ICMP accept stays out of the -// hook (it lives in conf.apf's type lists). +// TestAPFBareProtocolRoutesToHook covers a bare protocol match (no port, no +// address): it has no native apf construct but iptables expresses it directly, so +// AddRule diverts it to the pre-hook via needsHook rather than rejecting it. A +// native address-less ICMP accept stays out of the hook (it lives in conf.apf's +// type lists). func TestAPFBareProtocolRoutesToHook(t *testing.T) { fw := new(APF) for _, r := range []*Rule{ @@ -660,7 +662,8 @@ func TestAPFBareProtocolRoutesToHook(t *testing.T) { // to here since no real conf.apf exists in this test environment. Using an action // apf would not actually apply to this entry would make EditIPList reject it. func TestAPFPortOnlyRejectFamily(t *testing.T) { - fw := new(APF) + // IPv6 enabled, so a family-neutral deny fans out to both families (see writeFamilyRows). + fw := &APF{ipv6Enabled: true} ctx := context.Background() for _, rule := range []*Rule{ {Action: Drop, Proto: TCP, Port: 80}, @@ -693,6 +696,48 @@ func TestAPFPortOnlyRejectFamily(t *testing.T) { } } +// A port-only deny fans out across family, but the file may already hold a subset of +// those lines — a prior single-family add, a manual edit, or the same rule added +// twice by a reconcile. The add must note each fan-out line present and write only +// the rest. Regression for the single-"exists" gate: no one line covers a +// family-neutral target, so re-adding it duplicated the whole fan-out, and a target +// whose IPv4 line already existed had its IPv6 twin left unwritten. +func TestAPFPortOnlyDenyHealsAndDoesNotDuplicate(t *testing.T) { + ctx := context.Background() + // IPv6 enabled, so the deny fans out to an IPv4 and an IPv6 line. + fw := &APF{ipv6Enabled: true} + dir := t.TempDir() + + // Adding the same family-neutral deny twice must leave one line per family. + path := filepath.Join(dir, "deny_hosts.rules") + require.NoError(t, os.WriteFile(path, nil, 0o644)) + deny := &Rule{Family: FamilyAny, Proto: TCP, Port: 80, Action: Drop} + require.NoError(t, fw.EditIPList(ctx, path, Drop, deny, false)) + require.NoError(t, fw.EditIPList(ctx, path, Drop, deny, false)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + text := string(data) + require.Equal(t, 1, strings.Count(text, "tcp:in:d=80:s=0.0.0.0/0"), + "re-adding the rule must not duplicate the IPv4 line") + require.Equal(t, 1, strings.Count(text, "tcp:in:d=80:s=[::/0]"), + "re-adding the rule must not duplicate the IPv6 line") + + // A file holding only the IPv4 line must gain the missing IPv6 one, so IPv6:80 is + // actually blocked rather than reported blocked while open. + path2 := filepath.Join(dir, "deny_hosts2.rules") + require.NoError(t, os.WriteFile(path2, []byte("tcp:in:d=80:s=0.0.0.0/0\n"), 0o644)) + require.NoError(t, fw.EditIPList(ctx, path2, Drop, deny, false)) + + data2, err := os.ReadFile(path2) + require.NoError(t, err) + text2 := string(data2) + require.Equal(t, 1, strings.Count(text2, "tcp:in:d=80:s=0.0.0.0/0"), + "the pre-existing IPv4 line must be preserved, not duplicated") + require.Equal(t, 1, strings.Count(text2, "tcp:in:d=80:s=[::/0]"), + "the missing IPv6 line must be added") +} + // A bare all-protocol host rule (address, no port) is the one portless address // shape apf's trust files express, written as the plain address line. The // inexpressible shapes — a concrete-protocol host or a source+destination pair — @@ -894,8 +939,7 @@ func TestAPFTCPUDPAdvRule(t *testing.T) { fw := new(APF) r := &Rule{Proto: TCPUDP, Port: 80, Source: "192.0.2.1", Action: Accept, Direction: DirInput} - line, err := fw.MarshalAdvRule(r) - require.NoError(t, err) + line := fw.MarshalAdvRule(r) require.Equal(t, "in:d=80:s=192.0.2.1", line, "the protocol field is omitted for both transports") back := fw.ParseAdvRule(line, Accept) @@ -904,15 +948,18 @@ func TestAPFTCPUDPAdvRule(t *testing.T) { require.True(t, back.EqualBase(r, true)) // A concrete transport names itself and round-trips unchanged. - line, err = fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 80, Source: "192.0.2.1", Action: Accept}) - require.NoError(t, err) + line = fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 80, Source: "192.0.2.1", Action: Accept}) require.Equal(t, "tcp:in:d=80:s=192.0.2.1", line) require.Equal(t, TCP, fw.ParseAdvRule(line, Accept).Proto) // ProtocolAny has no advanced-rule form: the omitted field means tcp+udp, not - // every protocol, so emitting it would under-apply the rule. - _, err = fw.MarshalAdvRule(&Rule{Proto: ProtocolAny, Port: 80, Source: "192.0.2.1", Action: Accept}) - require.ErrorIs(t, err, ErrUnsupported) + // every protocol, so emitting it would under-apply the rule. It never reaches the + // marshaller — a port on ProtocolAny is inexpressible in iptables too, so addRule + // rejects it (iptablesRuleValid) before EditIPList runs, and needsHook leaves it + // alone rather than sending an unmarshallable rule to the hook. + anyProto := &Rule{Proto: ProtocolAny, Port: 80, Source: "192.0.2.1", Action: Accept} + require.False(t, fw.needsHook(anyProto), "a port on ProtocolAny must not reach the hook") + require.Error(t, iptablesRuleValid(anyProto), "a port on ProtocolAny must be rejected before marshalling") } // TestAPFTCPUDPRouting pins where a both-transports rule goes. A FamilyAny bare port @@ -937,3 +984,42 @@ func TestAPFTCPUDPRouting(t *testing.T) { // A portless tcpudp host has no plain-line form (a plain line is all-protocol). require.True(t, hostNeedsHook(&Rule{Proto: TCPUDP, Source: "192.0.2.1", Action: Accept})) } + +// With conf.apf's USE_IPV6 off, apf installs no IPv6 rule from its config, so a +// family-neutral port-only deny must be written as the IPv4 line alone (see +// writeFamilyRows). Removal still sweeps both families. +func TestAPFPortOnlyDenyIPv6DisabledWritesV4Only(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + off := new(APF) + + path := filepath.Join(dir, "deny_hosts.rules") + require.NoError(t, os.WriteFile(path, nil, 0o644)) + deny := &Rule{Family: FamilyAny, Proto: TCP, Port: 80, Action: Drop} + require.NoError(t, off.EditIPList(ctx, path, Drop, deny, false)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + require.Contains(t, string(data), "tcp:in:d=80:s=0.0.0.0/0", "the IPv4 line must be written") + require.NotContains(t, string(data), "[::/0]", + "no IPv6 line may be written while apf's IPv6 handling is off") + + got, err := off.ParseIPList(path, Drop) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, IPv4, got[0].impliedFamily()) + + // Switching IPv6 off must not strand the v6 line written while it was on. + on := &APF{ipv6Enabled: true} + bothPath := filepath.Join(dir, "deny_hosts.both.rules") + require.NoError(t, os.WriteFile(bothPath, nil, 0o644)) + require.NoError(t, on.EditIPList(ctx, bothPath, Drop, deny, false)) + data, err = os.ReadFile(bothPath) + require.NoError(t, err) + require.Contains(t, string(data), "[::/0]") + require.NoError(t, off.EditIPList(ctx, bothPath, Drop, deny, true)) + data, err = os.ReadFile(bothPath) + require.NoError(t, err) + require.NotContains(t, string(data), "d=80", + "removal must sweep the stale IPv6 line even with IPv6 off") +} diff --git a/csf_linux.go b/csf_linux.go index 4663fd2..283b175 100644 --- a/csf_linux.go +++ b/csf_linux.go @@ -165,27 +165,13 @@ func (f *CSF) ParseConnLimit(val string) (rules []*Rule) { return } -// csfAddrFamily parses an address (IP or CIDR) and reports its family, or false -// when the value is not a valid address. -func csfAddrFamily(v string) (Family, bool) { - cidrIP, _, err := net.ParseCIDR(v) - ip := net.ParseIP(v) - if err != nil && ip == nil { - return FamilyAny, false - } - if (cidrIP != nil && cidrIP.To4() == nil) || (ip != nil && ip.To4() == nil) { - return IPv6, true - } - return IPv4, true -} - // parseAddr classifies a csf advanced-rule address field. It returns the // address, its family, and whether the value is an address at all (a non-address // value is a port list or ICMP type). A zero "any" network (0.0.0.0/0 or ::/0) // is normalized to an empty address so a port-only rule written with the "any" // placeholder round-trips against a rule that carries no address. func (f *CSF) parseAddr(v string) (addr string, fam Family, ok bool) { - family, ok := csfAddrFamily(v) + family, ok := parseAddrFamily(v) if !ok { return "", FamilyAny, false } @@ -368,7 +354,7 @@ func (f *CSF) ParseIPList(filePath string, action Action) (rules []*Rule, err er rules = append(rules, rule) } else { // Try to parse IP. - family, ok := csfAddrFamily(trimmed) + family, ok := parseAddrFamily(trimmed) if !ok { continue } @@ -469,9 +455,10 @@ func (f *CSF) dropActions() (dropIn, dropOut Action) { // features csf's native config cannot express. func (f *CSF) hook() *hookScript { return &hookScript{ - rulePrefix: f.rulePrefix, - hookPath: CSFHook, - hookPerm: 0700, + rulePrefix: f.rulePrefix, + hookPath: CSFHook, + hookPerm: 0700, + ipv6Enabled: f.ipv6Enabled, } } @@ -796,53 +783,11 @@ func (f *CSF) advPortValue(specs []PortRange) string { return strings.Join(parts, ",") } -// MarshalAdvRule encodes a rule as a csf advanced allow/deny line. A csf -// advanced rule must carry a source or destination address. -func (f *CSF) MarshalAdvRule(r *Rule) (string, error) { - if r.Proto == ICMPv6 { - return "", fmt.Errorf("csf advanced rules do not support icmpv6") - } - if r.Source == "" && r.Destination == "" { - return "", fmt.Errorf("a csf advanced rule requires a source or destination address") - } - // A csf advanced rule holds a single address field, so a rule matching both a - // source and a destination cannot be expressed; reject it rather than silently - // dropping the destination (installing a broader rule and leaving it - // unremovable). Mirrors the dual-port guard below. - if r.Source != "" && r.Destination != "" { - return "", fmt.Errorf("csf advanced rules cannot match both a source and destination address") - } - // A csf advanced rule carries a single port-flow field, so a rule cannot match - // both a source and a destination port at once. - if r.HasPorts() && r.HasSourcePorts() { - return "", fmt.Errorf("csf advanced rules cannot match both a source and destination port") - } - // An ICMP advanced rule needs a concrete type for the port-flow field: without - // one the address would land there and csf would parse it as `--icmp-type ` - // and drop the rule (see checkICMP). Refuse to emit such a line. - if r.Proto == ICMP && r.ICMPType == nil { - return "", fmt.Errorf("a csf icmp advanced rule requires a concrete icmp type") - } - // A single advanced line carries a single protocol token, so a TCPUDP port rule - // with an address cannot be expressed here: a protocol-less line is defaulted to - // `-p tcp` by csf.pl's linefilter, applying the rule to TCP only while the - // library reads it back as both transports — leaving UDP open on a deny (or - // unallowed on an accept). The address-less form is fanned into a tcp and a udp - // line by the caller (portOnlyDenyLines); one line cannot fan, so reject it here - // rather than under-apply it. - if r.Proto == TCPUDP { - return "", fmt.Errorf("a csf advanced rule cannot carry both transports; expand it to tcp and udp lines: %w", ErrUnsupported) - } - // csf.pl's linefilter reads an advanced line by fixed field position, not by - // tag: it always looks for the port-flow field (d=/s=) before the address - // field. A tcp/udp rule with an address but no port shifts the address into - // the port-flow slot instead, where it is parsed as a garbage --sport/--dport - // value and the address field is left empty — linefilter then requires both - // an address and a port to install anything, so the rule is silently dropped. - if r.Proto != ICMP && !r.HasPorts() && !r.HasSourcePorts() { - return "", fmt.Errorf("a csf tcp/udp advanced rule with an address requires a port: %w", ErrUnsupported) - } - +// MarshalAdvRule encodes a rule as a csf advanced allow/deny line: a protocol +// token, a direction, one port-flow field (an icmp type, a source port or a +// destination port) and one address field, joined by "|". It validates nothing; +// addRule/RemoveRule route every shape the line cannot carry elsewhere first. +func (f *CSF) MarshalAdvRule(r *Rule) string { var parts []string switch r.Proto { case TCP: @@ -877,66 +822,94 @@ func (f *CSF) MarshalAdvRule(r *Rule) (string, error) { parts = append(parts, "d="+r.Destination) } - return strings.Join(parts, "|"), nil + return strings.Join(parts, "|") } -// advMatch reports whether a parsed advanced-rule line (the existing row) matches -// target. A TCPUDP port rule is written as a separate tcp line and udp line, so a -// TCPUDP target matches either concrete-transport line; that coverage, like the -// family and direction coverage the add and remove paths need, is folded into -// EqualForDedup / EqualForRemoval. -func (f *CSF) advMatch(parsed, target *Rule, remove bool) bool { - if remove { - return parsed.EqualForRemoval(target, true) +// parseListLine parses one csf.allow/csf.deny rule line into the rule it holds: an +// advanced rule, or a plain address line, which is a single bidirectional DirAny +// rule matching every protocol. It returns nil for a line that is neither, which the +// caller passes through untouched. The action comes from the file (csf.allow is +// accept, csf.deny is a deny); the line encodes none of its own. +func (f *CSF) parseListLine(line string, action Action) *Rule { + if strings.Contains(line, "|") { + return f.ParseAdvRule(line, action) } - return parsed.EqualForDedup(target, true) + family, ok := parseAddrFamily(line) + if !ok { + return nil + } + return &Rule{Direction: DirAny, Family: family, Source: line, Action: action} } -// portOnlyDenyLines returns the advanced csf.deny lines a port-only deny (no -// address) fans out to: one per transport (tcp and udp for a TCPUDP rule) and per -// family placeholder (0.0.0.0/0 for IPv4, ::/0 for IPv6, both for a family-neutral -// rule). It mirrors the emit path so the add logic can compare against the lines -// already in the file and write only the missing ones — a single "exists" flag -// would skip the whole fan-out when just one family/protocol line was already -// present, leaving the other family/protocol open. -func (f *CSF) portOnlyDenyLines(r *Rule) []string { - dir := "in" - if r.IsOutput() { - dir = "out" - } - port := f.advPortValue(r.PortSpecs()) - var protos []string - for _, sub := range expandProtocols(r) { - protos = append(protos, sub.Proto.String()) - } - var placeholders []string - switch r.impliedFamily() { - case IPv6: - placeholders = []string{"::/0"} - case IPv4: - placeholders = []string{"0.0.0.0/0"} - default: - placeholders = []string{"0.0.0.0/0", "::/0"} - } - var lines []string - for _, ph := range placeholders { - for _, p := range protos { - tokens := []string{p, dir, "d=" + port} - if r.IsOutput() { - tokens = append(tokens, "d="+ph) - } else { - tokens = append(tokens, "s="+ph) +// listRows returns the csf.allow/csf.deny rows a rule materializes into, in write +// order, or none for a shape the lists cannot hold. The rule must already carry the +// action its file implies (see EditIPList's match), since each row's read-back form +// is compared against lines stamped with it. +// +// csf has no both-transports line anywhere — csf.pl's linefilter silently reads a +// protocol-less advanced line as `-p tcp` — so a TCPUDP rule fans out into a tcp row +// and a udp row. A port-only deny additionally carries no address of its own, and +// csf's advanced-rule handler only emits an iptables rule for a line that has one, so +// each of its rows takes the "any" network as a placeholder address. That literal is +// family-specific, so a family-neutral rule fans out per family too rather than +// silently becoming IPv4-only — across the families csf actually enforces (see +// writeFamilyRows; with csf.conf's IPV6 off that is IPv4 alone). parseAddr normalizes +// the placeholder back to an empty address, so each row reads back as the address-less +// rule it stands for. +func (f *CSF) listRows(action Action, match *Rule) []listRow { + hasIP := match.Source != "" || match.Destination != "" + var rows []listRow + switch { + case hasIP && (match.HasPorts() || match.HasSourcePorts() || match.Proto.IsICMP()): + // A port/ICMP rule with an address is an advanced rule. + for _, sub := range expandProtocols(match) { + rows = append(rows, listRow{line: f.MarshalAdvRule(sub), read: sub}) + } + case hasIP: + // A bare all-protocol host allow/deny: a single address matching every + // protocol. csf.allow/csf.deny hold no other portless address shape — a + // concrete-protocol host or a source+destination pair — so AddRule diverts + // those to the raw-iptables hook (hostNeedsHook) and never reaches here with + // one. A direct caller of this exported writer that supplies such a shape gets + // a best-effort single-address write, not a guard. + addr := match.Source + if addr == "" { + addr = match.Destination + } + // The plain line is bidirectional and names its address as the source, which is + // the frame the scan reads it back in. + read := *match + read.Direction = DirAny + read.Source, read.Destination = addr, "" + rows = append(rows, listRow{line: addr, read: &read}) + case action != Accept && match.HasPorts(): + for _, fam := range writeFamilyRows(f.ipv6Enabled, match) { + placeholder := "0.0.0.0/0" + if fam.impliedFamily() == IPv6 { + placeholder = "::/0" + } + for _, sub := range expandProtocols(fam) { + // The row reads back address-less; only the line carries the placeholder. + // advRuleNeedsHook has already routed an address-less source-port match to + // the hook, so the address field being filled in here is free. + row := *sub + if row.IsOutput() { + row.Destination = placeholder + } else { + row.Source = placeholder + } + rows = append(rows, listRow{line: f.MarshalAdvRule(&row), read: sub}) } - lines = append(lines, strings.Join(tokens, "|")) } } - return lines + return rows } // EditIPList adds or removes a rule in a csf.allow/csf.deny list, rewriting it in -// place. A port-only deny fans out per transport and family placeholder, so a -// TCPUDP rule reads back as the rule that was written rather than being re-added on -// every reconcile and never matched for removal. +// place. An add expands the rule into the rows it materializes into (see listRows), +// notes which of them the file already holds, and appends only the rest, so a rule +// that fans out across families or transports is completed rather than duplicated on +// every reconcile. A removal drops every line the target covers. func (f *CSF) EditIPList(ctx context.Context, filePath string, action Action, r *Rule, remove bool) error { // Read the allow/deny IP rule list. fd, err := os.Open(filePath) @@ -953,20 +926,6 @@ func (f *CSF) EditIPList(ctx context.Context, filePath string, action Action, r defer af.Abort() scanner := bufio.NewScanner(fd) - exists := false - // A port-only deny (no address) fans out into several csf.deny lines across - // family and protocol. Track which of those lines are already in the file so the - // add path can write only the missing ones: the single "exists" flag below marks - // the whole rule present as soon as any one fan-out line matches, which would - // otherwise leave the other family/protocol open. wantDeny is empty for every - // other rule shape, so this tracking is inert unless the fan-out applies. - wantDeny := map[string]bool{} - if !remove && action != Accept && r.HasPorts() && r.Source == "" && r.Destination == "" { - for _, l := range f.portOnlyDenyLines(r) { - wantDeny[l] = true - } - } - presentDeny := map[string]bool{} // csf.allow/csf.deny encode no action of their own — the file decides it // (csf.allow is accept, csf.deny is a deny). A rule read from a file is stamped // with that file's action, so match an incoming rule with its action coerced @@ -974,6 +933,14 @@ func (f *CSF) EditIPList(ctx context.Context, filePath string, action Action, r // as the deny action) could never be found and removed. match := *r match.Action = action + // The rows an add must end up with, and which of them the scan finds already in + // the file. A removal wants no rows: it matches the target against each line + // directly, since a line it must drop need not be one this library would write. + var rows []listRow + if !remove { + rows = f.listRows(action, &match) + } + present := make([]bool, len(rows)) // Full-line comments immediately above a rule are held back so they can be // dropped together with a removed rule (they are its comment) and written // ahead of a kept one. A blank line detaches them. @@ -1019,129 +986,58 @@ func (f *CSF) EditIPList(ctx context.Context, filePath string, action Action, r continue } - // Note a fan-out line that is already present so the add path below skips it - // and writes only the missing family/protocol lines. Every such line is - // preserved (written back) by the pass-through branches below. - if wantDeny[line] { - presentDeny[line] = true + // A line neither form parses is not a rule; pass it through untouched. + cur := f.parseListLine(line, action) + if cur == nil { + flush() + _, _ = fmt.Fprintln(af, orig) + continue } - if strings.Contains(line, "|") { - rule := f.ParseAdvRule(line, action) - if rule == nil { - flush() - _, _ = fmt.Fprintln(af, orig) - continue - } - if f.advMatch(rule, &match, remove) { - exists = true - if !remove { - flush() - _, _ = fmt.Fprintln(af, orig) - } else { - drop() - } - } else { - flush() - _, _ = fmt.Fprintln(af, orig) - } - } else { - // Try to parse IP. - family, ok := csfAddrFamily(line) - if !ok { - flush() - _, _ = fmt.Fprintln(af, orig) + // A removal drops every line the target covers, along with its comment. A + // TCPUDP or family-neutral target touches each of the concrete lines it was + // written as; that coverage is folded into EqualForRemoval. + if remove { + if cur.EqualForRemoval(&match, true) { + drop() continue } + flush() + _, _ = fmt.Fprintln(af, orig) + continue + } - // A plain IP line is one bidirectional DirAny rule; match the target - // against it in the inbound frame (canonicalMatch), so a DirAny or a - // concrete-direction input/output target that names this host lines up. - plainRule := &Rule{ - Direction: DirAny, - Family: family, - Source: line, - Action: action, - } - if plainRule.EqualBase(match.canonicalMatch(), false) { - exists = true - if !remove { - flush() - _, _ = fmt.Fprintln(af, orig) - } else { - drop() - } - } else { - flush() - _, _ = fmt.Fprintln(af, orig) + // An add keeps every line and only notes which wanted rows the file already + // covers, so the tail writes the rest. Coverage rather than a text compare, so + // a row is satisfied by an existing line that spans it (a TCPUDP line absorbing + // a tcp row) and by one spelled differently but meaning the same. + for i := range rows { + if !present[i] && cur.EqualForDedup(rows[i].read, true) { + present[i] = true } } + flush() + _, _ = fmt.Fprintln(af, orig) } // Write any trailing comments that followed the last rule. flush() - // If not exists and not remove, try adding the rule. - if !exists && !remove { - writeComment := func() { - if c := combineComment(f.rulePrefix, r.Comment); c != "" { - _, _ = fmt.Fprintln(af, "# "+c) - } - } - hasIP := r.Source != "" || r.Destination != "" - switch { - case hasIP && (r.HasPorts() || r.HasSourcePorts() || r.Proto.IsICMP()): - // A port/ICMP rule with an address is an advanced rule. - line, err := f.MarshalAdvRule(r) - if err != nil { - _ = fd.Close() - return err - } - writeComment() - _, _ = fmt.Fprintln(af, line) - case hasIP: - // A bare all-protocol host allow/deny: a single address matching every - // protocol. csf.allow/csf.deny hold no other portless address shape — a - // concrete-protocol host or a source+destination pair — so AddRule diverts - // those to the raw-iptables hook (hostNeedsHook) and never reaches here - // with one. A direct caller of this exported writer that supplies such a - // shape gets a best-effort single-address write, not a guard. - writeComment() - if r.Source != "" { - _, _ = fmt.Fprintln(af, r.Source) - } else { - _, _ = fmt.Fprintln(af, r.Destination) - } + // Append the wanted rows the file does not already hold. A rule that fans out is + // completed row by row: when only a subset is present (the IPv4 line but not its + // IPv6 twin, from a prior single-family add or a manual edit) the missing rows + // must still be written, or that family stays open while the library reports the + // rule in force. + writeComment := func() { + if c := combineComment(f.rulePrefix, r.Comment); c != "" { + _, _ = fmt.Fprintln(af, "# "+c) } } - - // A port-only deny (no address) fans out into a csf.deny line per family and - // protocol. Unlike the single-line cases above it must NOT be gated on the - // whole-rule "exists" flag: when only a subset of the fan-out lines is already - // present (e.g. the IPv4 line but not the IPv6 one, from a prior single-family - // add or a manual edit), the missing lines must still be written or that - // family/protocol stays open while the library reports the port blocked. Emit - // only the lines not already present (presentDeny, noted during the scan). - // - // csf's advanced-rule handler only emits an iptables rule when the line carries - // an address, so each fan-out line uses the "any" network placeholder (0.0.0.0/0 - // or ::/0) as the address; parseAddr normalizes it back to an empty address on - // read, so each family's line reads back as its own rule. The transport is named - // explicitly (a - // protocol-less line defaults to tcp in csf's linefilter), and a TCPUDP deny fans - // to both tcp and udp. - if len(wantDeny) > 0 { - writeComment := func() { - if c := combineComment(f.rulePrefix, r.Comment); c != "" { - _, _ = fmt.Fprintln(af, "# "+c) - } - } - for _, line := range f.portOnlyDenyLines(r) { - if presentDeny[line] { - continue - } - writeComment() - _, _ = fmt.Fprintln(af, line) + for i, row := range rows { + if present[i] { + continue } + writeComment() + _, _ = fmt.Fprintln(af, row.line) } _ = fd.Close() @@ -1155,62 +1051,62 @@ func (f *CSF) EditIPList(ctx context.Context, filePath string, action Action, r return af.Commit() } -// checkConnLimit rejects a connection-limit request csf's CONNLIMIT cannot -// express, so an inexpressible one is reported rather than silently dropped. -func (f *CSF) checkConnLimit(r *Rule) error { - if r.ConnLimit == nil || f.isConnLimitRule(r) { - return nil - } - return fmt.Errorf("csf connection limiting (CONNLIMIT) applies only to a single inbound tcp port with no address, per source, rejecting the excess with a tcp reset: %w", ErrUnsupportedConnLimit) +// nativeICMP reports whether an ICMPv4 rule maps onto a csf advanced rule and so +// belongs in csf.allow/csf.deny rather than the raw-iptables hook. An advanced line +// requires an address, and its single port-flow field carries the icmp type, so only +// a rule with exactly one address and a concrete type has a faithful form: csf.pl's +// linefilter reads that field by position, and an address with no type would land the +// address there (`--icmp-type `), which csf fails to parse and drops silently. A +// source+destination pair overflows the single address field and is routed by +// advRuleNeedsHook. ICMPv6 never reaches this test — ruleNeedsHook sends it to the +// hook, or the IPv6 gate rejects it. +func (f *CSF) nativeICMP(r *Rule) bool { + return r.Proto == ICMP && r.ICMPType != nil && (r.Source != "") != (r.Destination != "") } -// checkICMP rejects ICMP rules csf cannot express: csf advanced rules are -// built on iptables ICMP (IPv4) and require an address, and must also carry a -// concrete type: csf's linefilter treats the single port-flow field as the -// icmp-type, so an address with no type would put the address in that field -// (`--icmp-type `), which csf then fails to parse and drops silently (csf.pl -// linefilter). There is no csf advanced-rule encoding for "any icmp type from an -// address" — a bare host rule already covers all protocols — so reject it rather -// than emit a dropped line. ICMPv6 never reaches this check: ruleNeedsHook routes it -// to the pre-hook (or the IPv6 gate rejects it) before addRule/RemoveRule call checkICMP. -func (f *CSF) checkICMP(r *Rule) error { - if r.Proto == ICMP { - if r.Source == "" && r.Destination == "" { - return fmt.Errorf("a csf icmp rule requires a source or destination address: %w", ErrUnsupported) - } - if r.ICMPType == nil { - return fmt.Errorf("a csf icmp rule with an address requires a concrete icmp type: %w", ErrUnsupported) - } +// needsHook reports whether a rule must be injected through the csf pre-hook as a +// raw iptables rule because csf's native config cannot express it. It is the single +// gate between the hook path and csf's config files: everything it rejects (returns +// true) is written to the hook, everything it accepts (returns false) maps onto +// csf.conf or the csf.allow/csf.deny lists. The shared shapes (ruleNeedsHook, +// bareHostOneWay, hostNeedsHook, advRuleNeedsHook, bareProtoNeedsHook) keep their own +// predicates, since RemoveRule routes on ruleNeedsHook and bareHostOneWay directly. +func (f *CSF) needsHook(r *Rule) bool { + // Features csf's native config cannot express (connection state, per-rule + // interface, logging, rate limiting, forward-chain routing, icmpv6, a transport + // csf does not carry, an address set) go to the hook. + if ruleNeedsHook(r) { + return true } - return nil -} - -// checkPortProto rejects a port match on a protocol csf cannot express as a port. -// csf's port lists are TCP_IN/UDP_IN only, so a port on a concrete non-tcp/udp -// protocol (e.g. sctp) would otherwise be wrongly written into BOTH lists, and a -// port on ProtocolAny — which matches every IP protocol, not just the two csf can -// carry — has no faithful form at all. TCPUDP is allowed: it fans out to both lists -// on an address-less accept, and to a tcp and a udp csf.deny advanced line -// otherwise. -func (f *CSF) checkPortProto(r *Rule) error { - switch r.Proto { - case TCP, UDP, TCPUDP: - return nil + // A one-way bare host has no csf.allow/csf.deny form (a plain line is + // bidirectional, an advanced rule needs a port), and a concrete-protocol host or a + // source+destination pair likewise has none (see hostNeedsHook); all go to the hook. + if bareHostOneWay(r) || hostNeedsHook(r) { + return true } - if r.HasPorts() || r.HasSourcePorts() { - return fmt.Errorf("csf requires a tcp or udp protocol for a port match: %w", ErrUnsupported) + // A ported source+destination pair, a source port matched with a destination port, + // and an address-less source-port match all overflow the advanced line's single + // address and single port-flow field (see advRuleNeedsHook); iptables matches each + // directly, so they go to the hook. + if advRuleNeedsHook(r) { + return true } - return nil -} - -// checkSourcePort rejects a source-port match csf cannot express. csf source -// ports live in an advanced rule, which requires an address, so a source-port -// rule without one has nowhere to go. -func (f *CSF) checkSourcePort(r *Rule) error { - if r.HasSourcePorts() && r.Source == "" && r.Destination == "" { - return fmt.Errorf("a csf source-port rule requires a source or destination address: %w", ErrUnsupportedSourcePort) + // A connection limit csf.conf's CONNLIMIT cannot express — anything but a + // per-source cap on a single address-less inbound tcp port rejecting the excess + // (isConnLimitRule) — goes to the hook's `-m connlimit` match. + if r.ConnLimit != nil && !f.isConnLimitRule(r) { + return true } - return nil + // An ICMPv4 rule csf's advanced-rule format cannot carry (see nativeICMP) goes to + // the hook's `iptables -p icmp` match, which needs neither an address nor a type. + if r.Proto == ICMP && !f.nativeICMP(r) { + return true + } + // A bare protocol match with no address and no port has no native csf construct — + // csf.conf keys on a port, an advanced rule on address+port, and csf.allow/csf.deny + // on an address — but iptables expresses it directly, so it goes to the hook (see + // bareProtoNeedsHook). ICMP is excluded there and routed above. + return bareProtoNeedsHook(r) } // denyAction returns the action a csf.deny entry takes in the given direction, @@ -1251,12 +1147,11 @@ func (f *CSF) addRule(ctx context.Context, zoneName string, r *Rule, enforceIPv6 return nil } - // csf has no both-transports construct: its port lists are a TCP list and a UDP - // list, and csf.pl's linefilter silently reads a protocol-less advanced line as - // `-p tcp`. So a TCPUDP rule fans out into a tcp rule and a udp rule, each routed - // independently, and each reads back as its own rule. Unlike apf — whose trust - // parser derives both transports from one protocol-less line — csf cannot express - // the pair in a single line anywhere. + // csf has no both-transports construct anywhere: its port lists are a TCP list and + // a UDP list, and csf.pl's linefilter silently reads a protocol-less advanced line + // as `-p tcp` rather than as both transports. So a TCPUDP rule fans out into a tcp + // rule and a udp rule, each routed independently, and each reads back as its own + // rule. if r.Proto == TCPUDP { for _, sub := range expandProtocols(r) { if err := f.addRule(ctx, zoneName, sub, enforceIPv6Gate); err != nil { @@ -1266,49 +1161,23 @@ func (f *CSF) addRule(ctx context.Context, zoneName string, r *Rule, enforceIPv6 return nil } - // Features csf's native config cannot express (connection-state, per-rule - // interface, logging, rate limiting, icmpv6) are injected as iptables rules - // through the csf pre-hook. - if ruleNeedsHook(r) { - _, err := f.hook().edit(r, false) - return err - } - // A one-way bare host allow/deny cannot be a plain csf line (bidirectional) nor - // an advanced rule (needs a port), so it is written to the hook. A DirAny bare - // host takes the plain-line path below. - if bareHostOneWay(r) { - _, err := f.hook().edit(r, false) - return err - } - // A concrete-protocol host or a source+destination pair likewise has no csf.allow/ - // csf.deny form (see hostNeedsHook), so it too is injected as a raw iptables rule. - if hostNeedsHook(r) { - _, err := f.hook().edit(r, false) - return err - } - // A bare protocol match with no address and no port has no native csf construct — - // csf.conf keys on a port, an advanced rule on address+port, and csf.allow/csf.deny - // on an address — but iptables expresses it directly, so it is injected through the - // pre-hook (see bareProtoNeedsHook). ICMP keeps its own handling (checkICMP) and is - // excluded there. - if bareProtoNeedsHook(r) { - _, err := f.hook().edit(r, false) - return err - } - if err := f.checkSourcePort(r); err != nil { - return err - } - if err := f.checkConnLimit(r); err != nil { - return err - } - if err := f.checkICMP(r); err != nil { - return err - } - if err := f.checkPortProto(r); err != nil { - return err + // Verify the rule is valid with iptables. + if err := iptablesRuleValid(r); err != nil { + return fmt.Errorf("%v: %w", err, ErrUnsupported) } - // A connection-limit rule maps onto the csf.conf CONNLIMIT list. + // Any shape csf's native config cannot express (a stateful/interface/logged/ + // rate-limited rule, a one-way or concrete-protocol host, a source+destination + // pair, a source-and-destination port match, an address-less source-port match, a + // non-native connection limit, a non-native ICMPv4 rule, or a bare protocol match) + // is injected as a raw iptables rule through the csf pre-hook. See needsHook for + // each clause; everything past this gate maps onto csf's own config files. + if f.needsHook(r) { + _, err := f.hook().edit(r, false) + return err + } + // A native connection-limit rule maps onto the csf.conf CONNLIMIT list (a + // non-native one was diverted to the hook above by needsHook). if r.ConnLimit != nil { return f.EditConf(ctx, r, false) } @@ -1432,56 +1301,44 @@ func (f *CSF) RemoveRule(ctx context.Context, zoneName string, r *Rule) error { return nil } - // Hook-injected rules (see AddRule) are removed from the managed script. - if ruleNeedsHook(r) { - _, err := f.hook().edit(r, true) + // Clear any hook copy of the rule first, no matter how csf stores it. A rule csf + // carries only in the hook (see needsHook) lives nowhere else, so this is its + // entire removal; a natively-expressible rule may still have a stray hook copy — + // the library's own (a deny whose action differs from csf.conf's is stored there, + // see AddRule) or one a customer added by hand for a shape csf can also express + // natively — that must be cleared before the native entry below. DirAny is expanded + // so both one-way hook lines are matched; a rule with no hook copy makes this a + // harmless no-op. + var err error + for _, sub := range expandDirections(r) { + if _, e := f.hook().edit(sub, true); e != nil { + err = e + break + } + } + // A rule csf carries only in the hook has no native entry to fall through to, so + // return once its hook copy is cleared (or on any hook error). Returning here also + // keeps such a rule out of the plain-line split scan below, which could wrongly + // split an unrelated coexisting native entry. + if ruleNeedsHook(r) || err != nil { return err } - // A one-way bare host rule is stored either as its own hook rule or as one - // direction of a bidirectional plain line; removing it may need to split the + // A one-way bare host rule is stored either as its own hook rule (cleared above) or + // as one direction of a bidirectional plain line; removing it may need to split the // plain line (see removeBareHostOneWay). if bareHostOneWay(r) { return f.removeBareHostOneWay(ctx, zoneName, r) } - // A concrete-protocol host or source+destination pair is stored only as its own - // hook rule (it has no plain-line form to split), so remove it directly. - if hostNeedsHook(r) { - _, err := f.hook().edit(r, true) - return err + // Every other shape csf's native config cannot express (see needsHook) has already + // had its hook copy cleared above and has no native entry to split, so it is done. + if f.needsHook(r) { + return nil } - // A bare protocol match is stored only as its own hook rule (see AddRule), so - // remove it from the managed script directly. ICMP keeps its own handling below. - if bareProtoNeedsHook(r) { - _, err := f.hook().edit(r, true) - return err - } - if err := f.checkSourcePort(r); err != nil { - return err - } - if err := f.checkConnLimit(r); err != nil { - return err - } - if err := f.checkICMP(r); err != nil { - return err - } - if err := f.checkPortProto(r); err != nil { - return err + if err := iptablesRuleValid(r); err != nil { + return fmt.Errorf("%v: %w", err, ErrUnsupported) } - // This rule is natively expressible, but a copy of it may nonetheless live in the - // hook — the library's own (a deny whose action differs from csf.conf's is stored - // there, see AddRule) or one a customer added by hand for a shape csf can also - // express natively. Clear any hook copy before removing the native entry, so a rule - // present in both csf's config and the hook is removed from both and a hook-only - // deny is fully removed here. DirAny is expanded so both one-way hook lines are - // matched; a rule with no hook copy makes this a harmless no-op. - for _, sub := range expandDirections(r) { - if _, err := f.hook().edit(sub, true); err != nil { - return err - } - } - - // A connection-limit rule maps onto the csf.conf CONNLIMIT list. + // A native connection-limit rule maps onto the csf.conf CONNLIMIT list. if r.ConnLimit != nil { return f.EditConf(ctx, r, true) } @@ -1494,22 +1351,21 @@ func (f *CSF) RemoveRule(ctx context.Context, zoneName string, r *Rule) error { } } - // Edit csf.allow if accept is the action, otherwise edit csf.deny. A deny whose - // action differs from csf.conf's action for its direction was only ever in the hook - // (cleared above), so return rather than match it against csf.deny — where it could - // coincide with a genuine matching-action entry. A matching-action deny lives in - // csf.deny. + // Edit csf.allow if accept is the action, otherwise edit csf.deny. A csf.deny entry + // carries no action of its own — csf applies csf.conf's action by direction — so the + // deny of an address is a single entry there, and it is removed whatever action the + // caller named: asking to stop denying something means the entry goes, or RemoveRule + // would report success while csf kept enforcing it. EditIPList coerces the target's + // action to the file's, so a differing-action deny still matches the line. The hook + // copy such a deny was added as (see AddRule) was already cleared above, exactly as + // the hook copy of a matching-action deny is, so both backings are swept either way. if r.Action == Accept { err := f.EditIPList(ctx, CSFAllow, Accept, r, true) if err != nil { return err } } else { - denyAction := f.denyAction(r.IsOutput()) - if r.Action != denyAction { - return nil - } - err := f.EditIPList(ctx, CSFDeny, denyAction, r, true) + err := f.EditIPList(ctx, CSFDeny, f.denyAction(r.IsOutput()), r, true) if err != nil { return err } @@ -1541,7 +1397,7 @@ func (f *CSF) UnmarshalNATRule(line string) *NATRule { return nil } if ipx != "*" && ipx != "" { - if _, ok := csfAddrFamily(ipx); !ok { + if _, ok := parseAddrFamily(ipx); !ok { return nil } r.Destination = ipx @@ -1563,7 +1419,7 @@ func (f *CSF) UnmarshalNATRule(line string) *NATRule { return nil } } else { - fam, ok := csfAddrFamily(ipy) + fam, ok := parseAddrFamily(ipy) if !ok { return nil } diff --git a/csf_linux_test.go b/csf_linux_test.go index 0c03d5c..9bbadfa 100644 --- a/csf_linux_test.go +++ b/csf_linux_test.go @@ -110,7 +110,8 @@ func TestCSFTCPUDPPortDenyRoundTrip(t *testing.T) { // gate that skipped the whole fan-out. func TestCSFPortOnlyDenyHealsMissingFamily(t *testing.T) { ctx := context.Background() - fw := new(CSF) + // IPv6 enabled, so the deny fans out across families and the missing v6 line heals. + fw := &CSF{ipv6Enabled: true} dir := t.TempDir() path := filepath.Join(dir, "csf.deny") @@ -143,41 +144,44 @@ func TestCSFPortOnlyDenyHealsMissingFamily(t *testing.T) { "the missing tcp line must be added so tcp:53 is actually blocked") } -// A csf advanced rule with an address, a port, and TCPUDP cannot be -// expressed as a single line: csf.pl defaults a protocol-less line to tcp, so -// udp would be silently left open. MarshalAdvRule must reject it rather than -// under-apply it (the port-only form fans out instead, but that path has no -// address to place). -func TestCSFAdvRuleTCPUDPWithAddressRejected(t *testing.T) { +// A csf advanced rule with an address, a port, and TCPUDP cannot be expressed as a +// single line: csf.pl defaults a protocol-less line to tcp, so udp would be silently +// left open. AddRule must therefore fan the rule into a tcp rule and a udp rule +// before it reaches MarshalAdvRule, each of which marshals to its own line. +func TestCSFAdvRuleTCPUDPWithAddressFansOut(t *testing.T) { fw := new(CSF) - _, err := fw.MarshalAdvRule(&Rule{Family: IPv4, Proto: TCPUDP, Port: 443, Source: "192.0.2.10", Action: Drop}) - require.ErrorIs(t, err, ErrUnsupported, - "an address+port rule with TCPUDP must be rejected, not written tcp-only") + both := &Rule{Family: IPv4, Proto: TCPUDP, Port: 443, Source: "192.0.2.10", Action: Drop} + subs := expandProtocols(both) + require.Len(t, subs, 2, "a TCPUDP rule must fan out before it is marshalled") - // A concrete protocol on the same shape is fine. - _, err = fw.MarshalAdvRule(&Rule{Family: IPv4, Proto: TCP, Port: 443, Source: "192.0.2.10", Action: Drop}) - require.NoError(t, err) + var lines []string + for _, sub := range subs { + lines = append(lines, fw.MarshalAdvRule(sub)) + } + require.Equal(t, []string{"tcp|in|d=443|s=192.0.2.10", "udp|in|d=443|s=192.0.2.10"}, lines, + "each transport must get its own line so udp is not left open") } // A csf tcp/udp advanced rule with an address but no port cannot be expressed: // csf.pl's linefilter reads the port-flow field by position before the address // field, so the address shifts into the port slot, gets parsed as a garbage // --sport/--dport value, and the rule is silently dropped (linefilter requires -// both an address and a port match to install anything). MarshalAdvRule must -// reject this shape rather than emit an unenforceable line. -func TestCSFAdvRuleAddressWithoutPortRejected(t *testing.T) { +// both an address and a port match to install anything). AddRule must route this +// shape to the raw-iptables hook rather than let MarshalAdvRule emit an +// unenforceable line. +func TestCSFAdvRuleAddressWithoutPortRouted(t *testing.T) { fw := new(CSF) - _, err := fw.MarshalAdvRule(&Rule{Family: IPv4, Proto: TCP, Source: "192.0.2.10", Action: Drop}) - require.ErrorIs(t, err, ErrUnsupported, - "a tcp advanced rule with an address and no port must be rejected") - _, err = fw.MarshalAdvRule(&Rule{Family: IPv4, Proto: UDP, Destination: "192.0.2.10", Action: Accept}) - require.ErrorIs(t, err, ErrUnsupported, - "a udp advanced rule with an address and no port must be rejected") + require.True(t, hostNeedsHook(&Rule{Family: IPv4, Proto: TCP, Source: "192.0.2.10", Action: Drop}), + "a tcp host with no port must go to the hook, not an advanced line") + require.True(t, hostNeedsHook(&Rule{Family: IPv4, Proto: UDP, Destination: "192.0.2.10", Action: Accept}), + "a udp host with no port must go to the hook, not an advanced line") - // An ICMP rule with an address and no port is unaffected by this guard (it - // already requires a concrete ICMP type via a separate check above). - _, err = fw.MarshalAdvRule(&Rule{Family: IPv4, Proto: ICMP, Source: "192.0.2.10", Action: Accept}) - require.Error(t, err, "expected the existing icmp-without-type guard to fire, not this one") + // An ICMP rule with an address and no type takes its own route: csf's linefilter + // would consume the address as the icmp-type field, so it goes to the hook, whose + // iptables rule needs no type. + icmp := &Rule{Family: IPv4, Proto: ICMP, Source: "192.0.2.10", Action: Accept} + require.False(t, hostNeedsHook(icmp), "icmp keeps its own handling") + require.True(t, fw.needsHook(icmp), "a typeless icmp host must be routed to the hook") } func TestCSFParseAdvRuleIPv6(t *testing.T) { @@ -224,8 +228,7 @@ func TestCSFFeatureRules(t *testing.T) { {&Rule{Direction: DirOutput, Proto: UDP, Port: 53, Destination: "192.0.2.1", Family: IPv4, Action: Accept}, "udp|out|d=53|d=192.0.2.1"}, } for _, c := range cases { - got, err := fw.MarshalAdvRule(c.rule) - require.NoError(t, err, "failed to marshal %+v", *c.rule) + got := fw.MarshalAdvRule(c.rule) require.Equal(t, c.want, got, "marshal %+v", *c.rule) parsed := fw.ParseAdvRule(got, c.rule.Action) @@ -268,11 +271,12 @@ func TestCSFFeatureRules(t *testing.T) { &Rule{Proto: TCP, Ports: []PortRange{{Start: 2000, End: 3000}}, Action: Accept}, false), "unexpected csf.conf port edit") - // MarshalAdvRule rejects rules csf cannot express as advanced rules. - _, err := fw.MarshalAdvRule(&Rule{Proto: ICMPv6, Source: "2001:db8::1", Action: Accept}) - require.Error(t, err, "expected error marshalling an icmpv6 advanced rule") - _, err = fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 80, Action: Accept}) - require.Error(t, err, "expected error marshalling an advanced rule without an address") + // An ICMPv6 rule has no advanced-line form at all and never reaches the + // marshaller: ruleNeedsHook routes it to the raw-iptables hook. (An address-less + // port rule has no advanced form either; it lands in a csf.conf port list or the + // csf.deny fan-out, covered by TestCSFTCPUDPRejectFansOut.) + require.True(t, ruleNeedsHook(&Rule{Proto: ICMPv6, Source: "2001:db8::1", Action: Accept}), + "an icmpv6 rule must be routed to the hook, never marshalled as an advanced line") } func TestCSFSourcePorts(t *testing.T) { @@ -289,8 +293,7 @@ func TestCSFSourcePorts(t *testing.T) { {&Rule{Proto: TCP, SourcePorts: []PortRange{{Start: 2000, End: 3000}}, Source: "1.2.3.4", Family: IPv4, Action: Accept}, "tcp|in|s=2000_3000|s=1.2.3.4"}, } for _, c := range cases { - got, err := fw.MarshalAdvRule(c.rule) - require.NoError(t, err, "failed to marshal %+v", *c.rule) + got := fw.MarshalAdvRule(c.rule) require.Equal(t, c.want, got, "marshal %+v", *c.rule) parsed := fw.ParseAdvRule(got, c.rule.Action) @@ -299,38 +302,51 @@ func TestCSFSourcePorts(t *testing.T) { "round-trip mismatch: input %+v, line %q, output %+v", *c.rule, got, parsed) } - // Matching both a source and a destination port is not representable. - _, err := fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 22, SourcePort: 1234, Source: "1.2.3.4", Action: Accept}) - require.Error(t, err, "expected error matching both source and destination ports") - - // A source-port rule with no address has nowhere to go. - require.Error(t, fw.checkSourcePort(&Rule{Proto: TCP, SourcePort: 1234, Action: Accept})) + // The single port-flow field holds one port match, so a source port matched with + // a destination port has no advanced-line form; neither does an address-less + // source port, since an advanced line requires an address. AddRule routes both to + // the raw-iptables hook rather than marshalling them. + require.True(t, advRuleNeedsHook(&Rule{Proto: TCP, Port: 22, SourcePort: 1234, Source: "1.2.3.4", Action: Accept}), + "a dual-port rule must be routed to the hook") + require.True(t, advRuleNeedsHook(&Rule{Proto: TCP, SourcePort: 1234, Action: Accept}), + "an address-less source-port rule must be routed to the hook") } -// An ICMP advanced rule that carries an address but no concrete type must be -// rejected: csf's linefilter would consume the address as the icmp-type field -// and silently drop the rule, while the library reported it enforced. -func TestCSFICMPRequiresType(t *testing.T) { +// Only an ICMP rule with exactly one address and a concrete type is a csf advanced +// rule; every other ICMP shape is routed to the raw-iptables hook, which needs +// neither. csf's linefilter would otherwise consume the address as the icmp-type +// field and silently drop the rule, while the library reported it enforced. +func TestCSFICMPRouting(t *testing.T) { fw := new(CSF) - // Address but no type: rejected by both the validator and the marshaller. - noType := &Rule{Proto: ICMP, Source: "1.2.3.4", Action: Accept} - require.Error(t, fw.checkICMP(noType)) - _, err := fw.MarshalAdvRule(noType) - require.Error(t, err, "must not emit an ICMP advanced line with the address in the type field") - - // With a concrete type it is a valid advanced rule and round-trips. + // With one address and a concrete type it is a valid advanced rule and round-trips. typed := &Rule{Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "1.2.3.4", Action: Accept} - require.NoError(t, fw.checkICMP(typed)) - line, err := fw.MarshalAdvRule(typed) - require.NoError(t, err) + require.True(t, fw.nativeICMP(typed)) + require.False(t, fw.needsHook(typed), "a typed icmp host is a native advanced rule") + line := fw.MarshalAdvRule(typed) require.Equal(t, "icmp|in|d=8|s=1.2.3.4", line) parsed := fw.ParseAdvRule(line, Accept) require.NotNil(t, parsed) require.True(t, parsed.Equal(typed, true), "round-trip mismatch: %q -> %+v", line, parsed) - // An address-less ICMP rule is still rejected (advanced rules need an address). - require.Error(t, fw.checkICMP(&Rule{Proto: ICMP, ICMPType: Ptr[uint8](8), Action: Accept})) + // Every other ICMP shape has no advanced-line form, so it is written to the hook + // rather than rejected: iptables expresses each directly. + hooked := []*Rule{ + {Proto: ICMP, Source: "1.2.3.4", Action: Accept}, // address, no type + {Proto: ICMP, ICMPType: Ptr[uint8](8), Action: Accept}, // type, no address + {Proto: ICMP, Action: Drop}, // neither + {Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "1.2.3.4", Destination: "5.6.7.8", Action: Accept}, // both addresses + } + h := &hookScript{} + for _, r := range hooked { + require.False(t, fw.nativeICMP(r), "expected non-native icmp for %+v", *r) + require.True(t, fw.needsHook(r), "expected hook routing for %+v", *r) + lines, err := h.rulesToLines(r) + require.NoError(t, err, "the hook must be able to render %+v", *r) + require.Len(t, lines, 1) + require.Contains(t, lines[0], "iptables -A ") + require.Contains(t, lines[0], "-p icmp") + } } func TestCSFConnLimit(t *testing.T) { @@ -353,17 +369,31 @@ func TestCSFConnLimit(t *testing.T) { require.Equal(t, `CONNLIMIT = "80;20"`, fw.editConnLimit("22;5,80;20", 22, 5, true)) require.Equal(t, `CONNLIMIT = "80;50"`, fw.editConnLimit("80;20", 80, 50, false)) - // Only a single inbound tcp port, address-less, per-source, reject rule maps. - require.True(t, fw.isConnLimitRule(&Rule{Proto: TCP, Port: 80, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}})) - bad := []*Rule{ + // Only a single inbound tcp port, address-less, per-source, reject rule maps onto + // CONNLIMIT. Every other connection limit is written to the hook's `-m connlimit` + // match instead of being rejected, since iptables expresses each directly. + native := &Rule{Proto: TCP, Port: 80, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}} + require.True(t, fw.isConnLimitRule(native)) + require.False(t, fw.needsHook(native), "a native connlimit belongs in csf.conf") + + hooked := []*Rule{ {Proto: TCP, Port: 80, Action: Drop, ConnLimit: &ConnLimit{Count: 5, PerSource: true}}, // wrong action (csf rejects, not drops) {Proto: UDP, Port: 80, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}}, // udp {Proto: TCP, Port: 80, Source: "1.2.3.4", Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}}, // address {Proto: TCP, Ports: []PortRange{{Start: 80, End: 90}}, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}}, // range {Proto: TCP, Port: 80, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: false}}, // global + {Proto: TCP, Port: 80, Direction: DirOutput, Action: Reject, ConnLimit: &ConnLimit{Count: 5, PerSource: true}}, // outbound } - for _, r := range bad { - require.Error(t, fw.checkConnLimit(r), "expected reject for %+v", *r) + h := &hookScript{} + for _, r := range hooked { + require.False(t, fw.isConnLimitRule(r), "expected non-native connlimit for %+v", *r) + require.True(t, fw.needsHook(r), "expected hook routing for %+v", *r) + lines, err := h.rulesToLines(r) + require.NoError(t, err, "the hook must be able to render %+v", *r) + require.NotEmpty(t, lines) + for _, line := range lines { + require.Contains(t, line, "-m connlimit --connlimit-above 5") + } } } @@ -546,27 +576,41 @@ func TestCSFDropRuleRemovable(t *testing.T) { } // A ported csf advanced rule that matches both a source and a destination address -// cannot be expressed (a csf advanced rule holds a single address field) and must -// be rejected rather than silently dropping the destination. The portless bare -// form is not tested here: AddRule diverts it to the raw-iptables hook -// (hostNeedsHook), so the writer never sees it. -func TestCSFDualAddressRejected(t *testing.T) { - fw := new(CSF) - _, err := fw.MarshalAdvRule(&Rule{Proto: TCP, Port: 80, Source: "1.2.3.4", Destination: "5.6.7.8", Action: Accept}) - require.Error(t, err, "a dual-address csf advanced rule must be rejected") +// cannot be expressed (a csf advanced rule holds a single address field), so AddRule +// routes it to the raw-iptables hook rather than silently dropping the destination. +// The portless bare form takes the same path one predicate earlier (hostNeedsHook). +func TestCSFDualAddressRouted(t *testing.T) { + require.True(t, advRuleNeedsHook(&Rule{Proto: TCP, Port: 80, Source: "1.2.3.4", Destination: "5.6.7.8", Action: Accept}), + "a ported dual-address rule must be routed to the hook, never marshalled as an advanced line") } -// csf's port lists are TCP_IN/UDP_IN; a concrete non-tcp/udp protocol (sctp) would -// wrongly be written into both, so it must error. ProtocolAny carries a port on no -// protocol csf can express — it matches every IP protocol — so it errors too. Only -// TCPUDP maps to both lists. +// csf's port lists are TCP_IN/UDP_IN. A port on a concrete protocol they cannot hold +// (sctp) is written to the hook, whose iptables rule matches it directly. A port on +// ProtocolAny is expressible nowhere — it matches every IP protocol, and iptables has +// no protocol-less port match — so it is the one shape csf still rejects. func TestCSFPortProtoGuard(t *testing.T) { fw := new(CSF) - require.Error(t, fw.checkPortProto(&Rule{Proto: SCTP, Port: 80, Action: Accept}), "sctp port must be rejected") - require.Error(t, fw.checkPortProto(&Rule{Port: 80, Action: Accept}), "an any-protocol port must be rejected") - require.NoError(t, fw.checkPortProto(&Rule{Proto: TCPUDP, Port: 80, Action: Accept}), "tcpudp port fans to both lists") - require.NoError(t, fw.checkPortProto(&Rule{Proto: TCP, Port: 80, Action: Accept})) - require.NoError(t, fw.checkPortProto(&Rule{Proto: ICMP, ICMPType: Ptr[uint8](8), Action: Accept})) + + // An sctp port goes to the hook rather than into both csf.conf lists. + sctp := &Rule{Proto: SCTP, Port: 80, Action: Accept} + require.True(t, fw.needsHook(sctp), "an sctp port must be routed to the hook") + lines, err := (&hookScript{}).rulesToLines(sctp) + require.NoError(t, err) + for _, line := range lines { + require.Contains(t, line, "-p sctp") + require.Contains(t, line, "--dport 80") + } + + // An any-protocol port has no form in csf's config nor in iptables, so the native + // path rejects it as unsupported. + anyProto := &Rule{Port: 80, Action: Accept} + require.False(t, fw.needsHook(anyProto), "an any-protocol port has no hook form either") + require.ErrorIs(t, fw.AddRule(context.Background(), "", anyProto), ErrUnsupported, + "an any-protocol port must be rejected") + + // The shapes csf's own config carries stay off the hook. + require.False(t, fw.needsHook(&Rule{Proto: TCP, Port: 80, Action: Accept})) + require.False(t, fw.needsHook(&Rule{Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "1.2.3.4", Action: Accept})) } // csf.conf's CONNLIMIT is a single dual-stack config key (it caps both v4 and v6 @@ -574,7 +618,7 @@ func TestCSFPortProtoGuard(t *testing.T) { // from it must be FamilyAny — not IPv4 — or a FamilyAny desired connlimit rule // (the natural shape: no address, so no family is implied) never matches its own // read-back and Sync removes-and-re-adds it every reconcile, firing csf -r each -// time. This mirrors the APF behavior above. +// time. func TestCSFConnLimitFamilyIsAny(t *testing.T) { // csf.pl only installs the ip6tables CONNLIMIT rule when csf.conf's IPV6 is // enabled; only then does a dual-stack FamilyAny read-back (and the @@ -651,7 +695,8 @@ func TestCSFBareProtocolRoutesToHook(t *testing.T) { // normalizes the "any" network back to an empty address, and a family-neutral rule // writes one line per family, which cover it between them. func TestCSFPortOnlyRejectRoundTrip(t *testing.T) { - fw := new(CSF) + // IPv6 enabled, so a family-neutral reject writes a line per family. + fw := &CSF{ipv6Enabled: true} ctx := context.Background() for _, rule := range []*Rule{ @@ -711,9 +756,9 @@ func TestCSFBareHostWritten(t *testing.T) { // A port-only "any"-source deny is written to csf.deny as a family-specific // placeholder line (0.0.0.0/0 for IPv4, ::/0 for IPv6). The two lines cover // different families, so adding the IPv6 twin while the IPv4 line already exists -// must write it — advMatch matches with EqualForDedup, so without the family -// coverage gate the IPv6 add was silently dropped as a false duplicate, leaving -// IPv6 open and making Sync churn forever. +// must write it — EditIPList matches an existing line with EqualForDedup, so without +// the family coverage gate the IPv6 add was silently dropped as a false duplicate, +// leaving IPv6 open and making Sync churn forever. func TestCSFCrossFamilyAdvDenyBothWritten(t *testing.T) { ctx := context.Background() fw := new(CSF) @@ -744,11 +789,12 @@ func TestCSFCrossFamilyAdvDenyBothWritten(t *testing.T) { } // A FamilyAny port-only deny writes both placeholder lines and must still be -// idempotent on re-add and fully removable — the EqualForDedup/EqualForRemoval -// gate in advMatch must not disturb the FamilyAny case. +// idempotent on re-add and fully removable — EditIPList's EqualForDedup/ +// EqualForRemoval gate must not disturb the FamilyAny case. func TestCSFFamilyAnyAdvDenyRoundTrip(t *testing.T) { ctx := context.Background() - fw := new(CSF) + // IPv6 enabled, so a FamilyAny deny fans out to both placeholder lines. + fw := &CSF{ipv6Enabled: true} dir := t.TempDir() path := filepath.Join(dir, "csf.deny") require.NoError(t, os.WriteFile(path, nil, 0644)) @@ -827,3 +873,55 @@ func TestCSFHasPrefixFlag(t *testing.T) { require.Equal(t, "hand-added by an admin", rules[1].Comment) require.False(t, rules[1].HasPrefix, "a comment without the prefix is not flagged") } + +// With csf.conf's IPV6 off, csf installs no IPv6 rule from its config, so a +// family-neutral port-only deny must be written as the IPv4 line alone. An IPv6 +// placeholder line would sit inert in csf.deny and read back as an IPv6 rule csf does +// not enforce and AddRule would reject. Removal still matches the target against every +// line, so a v6 line written while IPv6 was on is swept regardless. +func TestCSFPortOnlyDenyIPv6DisabledWritesV4Only(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + off := new(CSF) + + path := filepath.Join(dir, "csf.deny") + require.NoError(t, os.WriteFile(path, nil, 0644)) + deny := &Rule{Family: FamilyAny, Proto: TCP, Port: 80, Action: Drop} + require.NoError(t, off.EditIPList(ctx, path, Drop, deny, false)) + + data, err := os.ReadFile(path) + require.NoError(t, err) + require.Contains(t, string(data), "tcp|in|d=80|s=0.0.0.0/0", "the IPv4 line must be written") + require.NotContains(t, string(data), "::/0", + "no IPv6 line may be written while csf's IPv6 handling is off") + + // The rows read back cover the rule, and only for the family csf enforces. + got, err := off.ParseIPList(path, Drop) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, IPv4, got[0].impliedFamily()) + + // A rule pinned to IPv6 still writes its line: the ipv6Unavailable gate stops a + // fresh add, and Restore bypasses that gate on purpose to reproduce a snapshot. + v6path := filepath.Join(dir, "csf.deny.v6") + require.NoError(t, os.WriteFile(v6path, nil, 0644)) + v6 := &Rule{Family: IPv6, Proto: TCP, Port: 80, Action: Drop} + require.NoError(t, off.EditIPList(ctx, v6path, Drop, v6, false)) + data, err = os.ReadFile(v6path) + require.NoError(t, err) + require.Contains(t, string(data), "tcp|in|d=80|s=::/0") + + // Switching IPv6 off must not strand the v6 line written while it was on. + on := &CSF{ipv6Enabled: true} + bothPath := filepath.Join(dir, "csf.deny.both") + require.NoError(t, os.WriteFile(bothPath, nil, 0644)) + require.NoError(t, on.EditIPList(ctx, bothPath, Drop, deny, false)) + data, err = os.ReadFile(bothPath) + require.NoError(t, err) + require.Contains(t, string(data), "::/0") + require.NoError(t, off.EditIPList(ctx, bothPath, Drop, deny, true)) + data, err = os.ReadFile(bothPath) + require.NoError(t, err) + require.NotContains(t, string(data), "d=80", + "removal must sweep the stale IPv6 line even with IPv6 off") +} diff --git a/hooks_linux.go b/hooks_linux.go index 59a9866..a845ce8 100644 --- a/hooks_linux.go +++ b/hooks_linux.go @@ -2,6 +2,7 @@ package firewall import ( "fmt" + "net" "os" "strings" @@ -33,6 +34,10 @@ type hookScript struct { // hookPerm is the mode the hook file must carry to be executed (0700 for CSF, // 0750 for APF). hookPerm os.FileMode + // ipv6Enabled mirrors the backend's own IPv6 handling (csf.conf IPV6, conf.apf + // USE_IPV6). With it off, a family-agnostic rule is written for IPv4 only (see + // writeFamilies). + ipv6Enabled bool } // ruleNeedsHook reports whether a rule requires a feature that CSF/APF cannot @@ -64,6 +69,66 @@ func ipv6Unavailable(ipv6Enabled bool, r *Rule) bool { return !ipv6Enabled && r.impliedFamily() == IPv6 } +// parseAddrFamily parses an address (IP or CIDR) and reports its family, or false +// when the value is not a valid address. The boolean is what distinguishes it from +// familyOfAddr, which reports FamilyAny for both an unset and an unparseable address: +// a config line is classified by whether it is an address at all. Shared by CSF and +// APF. +func parseAddrFamily(v string) (Family, bool) { + cidrIP, _, err := net.ParseCIDR(v) + ip := net.ParseIP(v) + if err != nil && ip == nil { + return FamilyAny, false + } + if (cidrIP != nil && cidrIP.To4() == nil) || (ip != nil && ip.To4() == nil) { + return IPv6, true + } + return IPv4, true +} + +// listRow is one line a rule materializes into in a csf.allow/csf.deny or apf +// allow_hosts/deny_hosts file, paired with the rule that line reads back as. A rule +// spanning a family or transport axis the native line cannot carry has no single +// form, so it fans out into one row per cell. EditIPList marks the rows the file +// already holds as it scans and writes only the rest, so a partially present fan-out +// — one family written by an earlier single-family add, or a line lost to a manual +// edit — is completed rather than left half open or duplicated wholesale. Shared by +// CSF and APF. +type listRow struct { + // line is the exact text written to the list file. + line string + // read is the rule that line parses back to, which is what an existing line in + // the file is compared against to decide whether the row is already present. + read *Rule +} + +// writeFamilyRows returns the concrete-family rows a rule is written as in a csf/apf +// list file, narrowed to the families the backend enforces. With its own IPv6 handling +// off, neither backend installs any IPv6 rule from its config, so a family-agnostic +// rule is written for IPv4 only: the IPv6 row would sit inert in the file and read back +// as an IPv6 rule the backend does not enforce and AddRule would reject. It is the +// list-file analog of hookScript.writeFamilies, and enabling IPv6 later heals the +// missing row (EditIPList completes a partial fan-out). +// +// A row already pinned to a concrete family keeps it, IPv6 included: a fresh +// concrete-IPv6 add is stopped earlier by the ipv6Unavailable gate, and Restore +// deliberately bypasses that gate to reproduce a snapshot's entries verbatim. Removal +// needs no such narrowing — it matches each line against the target directly, so a +// family-agnostic target already sweeps both rows. Shared by CSF and APF. +func writeFamilyRows(ipv6Enabled bool, r *Rule) []*Rule { + rows := expandFamilies(r) + if ipv6Enabled || len(rows) == 1 { + return rows + } + kept := make([]*Rule, 0, 1) + for _, row := range rows { + if row.Family != IPv6 { + kept = append(kept, row) + } + } + return kept +} + // bareHostShape reports whether a rule has the shape a plain csf.allow/apf // allow_hosts line expresses: exactly one source or destination address, no ports, // and the any-protocol match. Its direction is not considered — a DirAny bare host @@ -106,8 +171,8 @@ func dirAnyPlainLine(r *Rule) bool { // advanced rule requires a port). Both are expressed natively as an iptables // rule. A single-address all-protocol host is not covered here — a one-way one is // routed by bareHostOneWay, a bidirectional one is a native plain line — and ICMP -// keeps its own handling (checkICMP/apfCheckICMP), so it is excluded. Shared by -// CSF and APF. +// keeps its own handling (CSF.nativeICMP, APF.isConfRule), so it is excluded. Shared +// by CSF and APF. func hostNeedsHook(r *Rule) bool { if r.Proto.IsICMP() || r.HasPorts() || r.HasSourcePorts() { return false @@ -125,13 +190,52 @@ func hostNeedsHook(r *Rule) bool { return false } +// advRuleNeedsHook reports whether a rule that would otherwise be written as a +// csf/apf advanced rule must instead be injected through the raw-iptables hook, +// because the advanced-line format cannot carry its shape. An advanced line holds +// exactly one address field and exactly one port-flow field, and requires an +// address, so three shapes overflow it: a source+destination pair (the second +// address has nowhere to go), a source port matched together with a destination +// port, and an address-less source-port match. iptables expresses all three directly +// (`-s`/`-d`, `--sport`/`--dport`), so they are hooked rather than rejected — the +// alternative is failing on a rule the firewall can enforce. +// +// hostNeedsHook covers the portless address shapes and bows out once a rule carries +// a port, so this predicate is what routes their ported counterparts. Only the +// protocols an iptables port or icmp match accepts are routed here: a port on +// ProtocolAny is inexpressible in iptables too, so it stays on the native path and is +// rejected there by iptablesRuleValid rather than reaching the hook and failing there. +// Shared by CSF and APF. +func advRuleNeedsHook(r *Rule) bool { + if !onProtocolAxis(r.Proto) && !r.Proto.IsICMP() { + return false + } + // One port-flow field: a source port and a destination port cannot share it. + if r.HasPorts() && r.HasSourcePorts() { + return true + } + // An advanced rule requires an address, so a bare source-port match has no + // advanced form at all; iptables matches --sport on its own. + if r.HasSourcePorts() && r.Source == "" && r.Destination == "" { + return true + } + // One address field: a source+destination pair cannot share it. Only the advanced + // form is at stake, so a rule with neither a port nor an icmp match — which never + // reaches MarshalAdvRule — is left to hostNeedsHook. + if r.Source != "" && r.Destination != "" { + return r.HasPorts() || r.HasSourcePorts() || r.Proto.IsICMP() + } + return false +} + // bareProtoNeedsHook reports whether a rule is a bare protocol match — a non-ICMP // transport with no address and no port — that CSF/APF cannot express in their // native config (the trust files key on an address and the conf lists key on a // port or icmp type) but iptables applies directly (`-p tcp -j ACCEPT`, or a bare // `-j ACCEPT`). Such a rule is injected through the raw-iptables hook rather than -// rejected. ICMP/ICMPv6 keep their own native/hook handling (checkICMP/apfCheckICMP, -// nativeICMPv6, ruleNeedsHook), so they are excluded here. Shared by CSF and APF. +// rejected. ICMP/ICMPv6 keep their own native/hook handling (CSF.nativeICMP, +// APF.isConfRule, APF.nativeICMPv6, ruleNeedsHook), so they are excluded here. Shared +// by CSF and APF. func bareProtoNeedsHook(r *Rule) bool { return r.Source == "" && r.Destination == "" && !r.HasPorts() && !r.HasSourcePorts() && !r.Proto.IsICMP() } @@ -158,9 +262,11 @@ func hookRuleProtos(r *Rule) []Protocol { return protos } -// hookRuleFamilies lists the address families a rule is written for: a rule -// pinned to a family (by address or an ICMP protocol) touches only that command, -// a family-agnostic rule (e.g. a bare state match) is written for both v4 and v6. +// hookRuleFamilies lists every address family a rule's hook lines can occupy: a +// rule pinned to a family (by address or an ICMP protocol) touches only that +// command, a family-agnostic rule (e.g. a bare state match) spans both v4 and v6. +// It is the full set, which is what removal must sweep; writeFamilies narrows it to +// the families the backend actually enforces. func hookRuleFamilies(r *Rule) []Family { switch r.impliedFamily() { case IPv4: @@ -172,6 +278,26 @@ func hookRuleFamilies(r *Rule) []Family { } } +// writeFamilies lists the address families a rule's hook lines are written for. +// With the backend's own IPv6 handling off, a family-agnostic rule is written for +// IPv4 only: the pre-hook runs on every (re)load regardless, but neither csf nor apf +// flushes ip6tables while IPv6 is disabled (csf.pl guards the v6 flush behind IPV6; +// apf's ipt6() is a no-op unless USE_IPV6=1), so an injected ip6tables line would be +// re-appended on each reload and would outlive its own removal from the hook. That is +// the same hazard ipv6Unavailable rejects a concrete-IPv6 rule for; a family-agnostic +// rule is not rejected, it is simply written for the family the backend enforces. +// +// A rule already pinned to a concrete family keeps it, IPv6 included: a fresh +// concrete-IPv6 add is stopped earlier by the ipv6Unavailable gate, and Restore +// deliberately bypasses that gate to reproduce a snapshot's entries verbatim. +func (h *hookScript) writeFamilies(r *Rule) []Family { + fams := hookRuleFamilies(r) + if h.ipv6Enabled || len(fams) == 1 { + return fams + } + return []Family{IPv4} +} + // hookCommand returns the iptables command name for a family. func hookCommand(fam Family) string { if fam == IPv6 { @@ -204,15 +330,29 @@ func shellSafeToken(tok string) string { return "'" + strings.ReplaceAll(tok, "'", `'\''`) + "'" } -// rulesToLines encodes a rule as the raw command line(s) to inject: one iptables -// (or ip6tables) command per underlying iptables line and per family. A logged -// rule yields a LOG line followed by its action line, as with the iptables +// rulesToLines encodes a rule as the raw command line(s) to inject, for the +// families the backend enforces (see writeFamilies). +func (h *hookScript) rulesToLines(r *Rule) ([]string, error) { + return h.linesForFamilies(r, h.writeFamilies(r)) +} + +// removalLines encodes every hook line a rule could occupy, across both families +// regardless of the backend's IPv6 setting. Removal sweeps the wider set so an +// ip6tables line written while IPv6 was enabled — or added by hand — is still +// cleared once it is switched off, rather than stranded in the hook. +func (h *hookScript) removalLines(r *Rule) ([]string, error) { + return h.linesForFamilies(r, hookRuleFamilies(r)) +} + +// linesForFamilies encodes a rule as the raw command line(s) to inject: one iptables +// (or ip6tables) command per underlying iptables line and per requested family. A +// logged rule yields a LOG line followed by its action line, as with the iptables // backend. Each marshalled line is re-tokenized and re-quoted shell-safely, // because the hook script is sourced by /bin/sh rather than exec'd argv-style. -func (h *hookScript) rulesToLines(r *Rule) ([]string, error) { +func (h *hookScript) linesForFamilies(r *Rule, fams []Family) ([]string, error) { var out []string for _, proto := range hookRuleProtos(r) { - for _, fam := range hookRuleFamilies(r) { + for _, fam := range fams { rc := *r rc.Proto = proto rc.Family = fam @@ -362,7 +502,13 @@ func (h *hookScript) writeHook(lines []string, existed bool) error { // line — user-authored shell and rules alike — and reports whether the hook // changed. Adding to an absent hook creates it; removing from one is a no-op. func (h *hookScript) edit(r *Rule, remove bool) (bool, error) { - desired, err := h.rulesToLines(r) + // An add writes only the families the backend enforces; a removal sweeps both, so + // a stale ip6tables line does not outlive an IPv6 switch-off (see writeFamilies). + linesFor := h.rulesToLines + if remove { + linesFor = h.removalLines + } + desired, err := linesFor(r) if err != nil { return false, err } diff --git a/hooks_linux_test.go b/hooks_linux_test.go index 009f56e..b823cec 100644 --- a/hooks_linux_test.go +++ b/hooks_linux_test.go @@ -40,6 +40,51 @@ func TestRuleNeedsHook(t *testing.T) { } } +// A csf/apf advanced line holds one address field and one port-flow field. A rule +// that overflows either must reach the raw-iptables hook, which matches -s/-d and +// --sport/--dport together: before advRuleNeedsHook existed, AddRule fell through to +// MarshalAdvRule and failed on a rule the firewall can enforce. +func TestAdvRuleNeedsHook(t *testing.T) { + // Shapes the single address / single port-flow field cannot hold. + needs := []*Rule{ + // A source+destination pair carrying a port: hostNeedsHook bows out once a + // rule has a port, so this predicate is what routes it. + {Proto: TCP, Port: 80, Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + {Proto: TCPUDP, Port: 80, Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + {Proto: TCP, SourcePort: 1234, Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + // A source+destination pair on an ICMP match, typed or not. + {Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + {Proto: ICMP, Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + // A source port matched together with a destination port, addressed or not. + {Proto: TCP, Port: 80, SourcePort: 1234, Source: "192.0.2.1", Action: Accept}, + {Proto: UDP, Port: 53, SourcePort: 53, Destination: "192.0.2.1", Action: Accept}, + {Proto: TCP, Port: 80, SourcePort: 1234, Action: Accept}, + // An advanced rule requires an address, so a bare source-port match has no + // advanced form of any kind. + {Proto: TCP, SourcePort: 1234, Action: Accept}, + {Proto: TCPUDP, SourcePort: 1234, Action: Drop}, + } + for _, r := range needs { + require.True(t, advRuleNeedsHook(r), "expected %+v to need the hook", *r) + } + // Shapes the advanced line holds, or that another predicate routes. + native := []*Rule{ + // One address, one port-flow field: the advanced rule itself. + {Proto: TCP, Port: 22, Source: "192.0.2.1", Action: Accept}, + {Proto: TCP, SourcePort: 1234, Source: "192.0.2.1", Action: Accept}, + {Proto: ICMP, ICMPType: Ptr[uint8](8), Source: "192.0.2.1", Action: Accept}, + // A portless source+destination pair belongs to hostNeedsHook. + {Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + // A port on ProtocolAny is inexpressible in iptables too, so it must keep its + // own rejection rather than reach the hook and fail there. + {Proto: ProtocolAny, Port: 80, SourcePort: 1234, Source: "192.0.2.1", Action: Accept}, + {Proto: ProtocolAny, Port: 80, Source: "192.0.2.1", Destination: "198.51.100.1", Action: Accept}, + } + for _, r := range native { + require.False(t, advRuleNeedsHook(r), "expected %+v not to route to the hook", *r) + } +} + // With csf.conf's IPV6 or conf.apf's USE_IPV6 off (the shipped default in both), // neither backend enforces any IPv6, and the raw-iptables hook is no escape hatch: // neither firewall flushes ip6tables on reload, so a hook-injected v6 line is @@ -260,12 +305,14 @@ func TestHookScriptRemoveIgnoresComment(t *testing.T) { func TestHookScriptRoundTrip(t *testing.T) { dir := t.TempDir() h := &hookScript{ - rulePrefix: "go_firewall", - hookPath: filepath.Join(dir, "csfpre.sh"), - hookPerm: 0700, + rulePrefix: "go_firewall", + hookPath: filepath.Join(dir, "csfpre.sh"), + hookPerm: 0700, + ipv6Enabled: true, } - // A family-agnostic rule is injected for both v4 and v6. + // A family-agnostic rule is injected for both v4 and v6 when the backend enforces + // IPv6. lines, err := h.rulesToLines(&Rule{Proto: TCP, Port: 8080, Action: Accept, State: StateNew}) require.NoError(t, err) require.Len(t, lines, 2) @@ -623,3 +670,70 @@ func TestHookAddressSetEntryEdits(t *testing.T) { _, err = h.editAddressSetEntry("missing", "1.2.3.4", false) require.Error(t, err, "editing an entry in a set that does not exist must fail") } + +// With the backend's own IPv6 handling off, a family-agnostic rule must be injected +// as an IPv4 line only. The pre-hook runs on every (re)load regardless, but csf/apf +// never flush ip6tables while IPv6 is disabled, so an injected ip6tables line would +// be re-appended on each reload and would outlive its removal from the hook. The +// ipv6Unavailable gate only stops a *concrete* IPv6 rule; a FamilyAny rule reaches +// the hook and must be narrowed here instead. +func TestHookScriptIPv6DisabledSkipsV6Family(t *testing.T) { + dir := t.TempDir() + h := &hookScript{ + rulePrefix: "go_firewall", + hookPath: filepath.Join(dir, "csfpre.sh"), + hookPerm: 0700, + } + + anyFam := &Rule{Proto: TCP, Port: 8080, Action: Accept, State: StateNew} + lines, err := h.rulesToLines(anyFam) + require.NoError(t, err) + require.Len(t, lines, 1, "a family-agnostic rule must not be written for ipv6 when ipv6 is off") + require.True(t, strings.HasPrefix(lines[0], "iptables "), "want an iptables line, got %q", lines[0]) + + // It is written to the hook the same way, so no ip6tables command is ever injected. + changed, err := h.edit(anyFam, false) + require.NoError(t, err) + require.True(t, changed) + data, err := os.ReadFile(h.hookPath) + require.NoError(t, err) + require.NotContains(t, string(data), "ip6tables ", + "an ip6tables line csf/apf never flush must not be injected while ipv6 is off") + + // A rule pinned to a concrete family keeps it: the ipv6Unavailable gate stops a + // fresh concrete-IPv6 add, and Restore bypasses that gate on purpose to reproduce a + // snapshot's entries verbatim, so the hook must still be able to render one. + v6 := &Rule{Family: IPv6, Proto: TCP, Port: 8080, Action: Accept, State: StateNew} + lines, err = h.rulesToLines(v6) + require.NoError(t, err) + require.Len(t, lines, 1) + require.True(t, strings.HasPrefix(lines[0], "ip6tables "), "want an ip6tables line, got %q", lines[0]) +} + +// Switching IPv6 off must not strand the ip6tables lines written while it was on: +// removal sweeps both families even though an add only writes the enforced one. +func TestHookScriptRemoveSweepsV6AfterIPv6Disabled(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "csfpre.sh") + + // Written while the backend's IPv6 handling was on: an iptables and an ip6tables line. + on := &hookScript{rulePrefix: "go_firewall", hookPath: path, hookPerm: 0700, ipv6Enabled: true} + rule := &Rule{Proto: TCP, Port: 8080, Action: Accept, State: StateNew} + changed, err := on.edit(rule, false) + require.NoError(t, err) + require.True(t, changed) + data, err := os.ReadFile(path) + require.NoError(t, err) + require.Contains(t, string(data), "ip6tables ") + + // IPv6 is now off. Removing the same rule must still clear the stale ip6tables line. + off := &hookScript{rulePrefix: "go_firewall", hookPath: path, hookPerm: 0700} + changed, err = off.edit(rule, true) + require.NoError(t, err) + require.True(t, changed) + data, err = os.ReadFile(path) + require.NoError(t, err) + require.NotContains(t, string(data), "ip6tables ", + "a stale ip6tables line must be swept on removal, not stranded in the hook") + require.NotContains(t, string(data), "iptables -A ") +} diff --git a/integration_test.go b/integration_test.go index 184332a..a272c8c 100644 --- a/integration_test.go +++ b/integration_test.go @@ -227,6 +227,30 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Ports: []PortRange{{Start: 6001, End: 6001}, {Start: 6002, End: 6002}}, Source: "192.0.2.43/32", Action: Accept}) }) + t.Run("advhookoverflow", func(t *testing.T) { + // A csf/apf advanced line holds one address field and one port-flow field, so a + // ported source+destination pair and a source-plus-destination port match both + // overflow it. Neither backend may reject them: iptables matches -s with -d and + // --sport with --dport directly, so AddRule routes both to the raw-iptables hook + // (advRuleNeedsHook). Gated to csf/apf; the chain backends express these + // natively and cover them elsewhere. + switch mgr.Type() { + case CSFType, APFType: + default: + t.Skip("only csf/apf route these shapes to the hook") + } + // A source+destination pair carrying a destination port. hostNeedsHook stops at + // the port, so this is the shape that used to reach MarshalAdvRule and fail. + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 8443, Source: "192.0.2.50/32", Destination: "198.51.100.50/32", Action: Accept}) + // The same pair on a source port, and on a typed ICMP match. + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: UDP, SourcePort: 5353, Source: "192.0.2.51/32", Destination: "198.51.100.51/32", Action: Accept}) + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: ICMP, ICMPType: Ptr[uint8](13), Source: "192.0.2.52/32", Destination: "198.51.100.52/32", Action: Accept}) + // A source port matched together with a destination port, with and without an + // address. Avoid port 22 in either field so an SSH-driven run keeps its session. + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 8444, SourcePort: 5354, Source: "192.0.2.53/32", Action: Accept}) + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 8445, SourcePort: 5355, Action: Accept}) + }) + t.Run("negatedaddress", func(t *testing.T) { // ufw's tuple grammar cannot negate an address, so a negated source is routed // to the before.rules raw path (iptables `! -s`) rather than rejected, and @@ -921,11 +945,13 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context t.Run("icmp", func(t *testing.T) { // ICMP has genuinely different shapes per backend: nft/iptables/ufw accept a - // bare rule; apf models ICMP as a list of allowed types so it needs a type - // (and forbids an address); csf builds ICMP on host-based rules so it needs - // an address AND a type (its advanced-rule format carries the type in the - // single port-flow field, so an address with no type is silently dropped by - // csf). Offer all forms and use the first the backend accepts. + // bare rule; apf models ICMP as a list of allowed types, so a rule with an + // address or a non-accept action goes to its hook; csf builds native ICMP on + // host-based advanced rules needing an address AND a type (the advanced-rule + // format carries the type in the single port-flow field), so every other shape + // goes to its hook. Both hook forms are plain iptables rules, so the bare + // variant is what csf and apf match here. Offer all forms and use the first the + // backend accepts. roundTripVariants(t, ctx, mgr, zone, &Rule{Family: IPv4, Proto: ICMP, Action: Accept}, &Rule{Family: IPv4, Proto: ICMP, ICMPType: Ptr[uint8](8), Action: Accept}, @@ -970,17 +996,29 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context }) t.Run("icmptype", func(t *testing.T) { - // A specific ICMP type, with an address fallback for csf (which requires one - // on any ICMP rule). Use type 13 (Timestamp) rather than the more common - // echo-request type 8 to avoid colliding with Windows' built-in echo-request - // rules, which cannot be removed and would make the round-trip assertions - // match the wrong rule. + // A specific ICMP type, with an addressed fallback for a backend whose native + // type list cannot carry the bare form. Use type 13 (Timestamp) rather than the + // more common echo-request type 8 to avoid colliding with Windows' built-in + // echo-request rules, which cannot be removed and would make the round-trip + // assertions match the wrong rule. roundTripVariants(t, ctx, mgr, zone, &Rule{Family: IPv4, Proto: ICMP, ICMPType: Ptr[uint8](13), Action: Accept}, &Rule{Family: IPv4, Proto: ICMP, ICMPType: Ptr[uint8](13), Source: "192.0.2.0/24", Action: Accept}, ) }) + t.Run("csfnativeicmp", func(t *testing.T) { + // csf's only native ICMP form is an advanced rule carrying exactly one address + // and a concrete type; every other shape routes to the hook (nativeICMP), so + // the shared icmp/icmptype probes match csf on its bare hook form first. + // Round-trip the native form here to keep the csf.allow advanced-rule path + // covered. Gated to csf; type 13 (Timestamp) avoids echo-request collisions. + if mgr.Type() != CSFType { + t.Skip("csf-specific advanced-rule ICMP path") + } + roundTripRule(t, ctx, mgr, zone, &Rule{Family: IPv4, Proto: ICMP, ICMPType: Ptr[uint8](13), Source: "192.0.2.71/32", Action: Accept}) + }) + t.Run("apficmphook", func(t *testing.T) { // apf carries ICMP as a zone-wide allowed-type list (IG_ICMP_TYPES), so an // ICMP rule with an address or a non-accept action has no native form and @@ -1018,8 +1056,9 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context t.Run("sourceport", func(t *testing.T) { // Source-port alone where supported (note firewalld rich rules can match a // source port but not together with a destination port, so the probe never - // combines the two); apf requires an address on a source-port rule, so fall - // back to a form carrying a source address. + // combines the two). csf/apf have no address-less advanced-rule form, so they + // route this through the raw-iptables hook; the addressed fallback remains for + // any backend that needs an address on a source-port rule. roundTripVariants(t, ctx, mgr, zone, &Rule{Proto: TCP, SourcePort: 1234, Action: Accept}, &Rule{Proto: TCP, SourcePort: 1234, Source: "192.0.2.0/24", Action: Accept}, @@ -1054,10 +1093,11 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context t.Run("connlimit", func(t *testing.T) { requireCap(t, caps.ConnLimit) // Connection limiting splits across backends: nft expresses only a global - // cap; apf a per-source one that rejects the excess; csf a per-source one - // that drops it; pf a per-source one only on an accept rule (single inbound - // tcp port, no address). iptables does the global form. Try each and - // round-trip the first the backend accepts. + // cap; pf a per-source one only on an accept rule (single inbound tcp port, no + // address); iptables does the global form. csf and apf carry one native shape + // each in their config and route every other shape to their hook, whose + // iptables rule does the global form too. Try each and round-trip the first the + // backend accepts. roundTripVariants(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 80, Action: Drop, ConnLimit: &ConnLimit{Count: 20}}, &Rule{Proto: TCP, Port: 80, Action: Reject, ConnLimit: &ConnLimit{Count: 20, PerSource: true}}, @@ -1079,6 +1119,46 @@ func runManagerSuite(t *testing.T, mgr Manager, reconstruct func(context.Context roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 8081, Action: Reject, ConnLimit: &ConnLimit{Count: 15, PerSource: true}}) }) + t.Run("csfdenyremovedanyaction", func(t *testing.T) { + // A csf.deny entry carries no action of its own — csf applies csf.conf's action + // by direction — so the deny of an address is a single entry there and must be + // removable whatever action the caller names, as apf's deny_hosts entry is. + // Otherwise RemoveRule reports success while csf keeps enforcing the entry. + // Gated to csf; the stock csf.conf DROP is "DROP", so Drop is the native inbound + // deny action and Reject is the differing one. + if mgr.Type() != CSFType { + t.Skip("csf-specific csf.deny action-agnostic removal") + } + host := "192.0.2.72/32" + added := &Rule{Family: IPv4, Proto: TCP, Port: 8084, Source: host, Action: Drop} + require.NoError(t, mgr.AddRule(ctx, zone, added)) + t.Cleanup(func() { _ = mgr.RemoveRule(ctx, zone, added) }) + require.True(t, containsRule(rulesOf(t, ctx, mgr, zone), added, mgr.Capabilities().Output), + "the native deny must be present before removal") + + // The same rule, named with the other deny action, must still clear the entry. + differing := *added + differing.Action = Reject + require.NoError(t, mgr.RemoveRule(ctx, zone, &differing)) + for _, r := range rulesOf(t, ctx, mgr, zone) { + require.Falsef(t, addrEqual(r.Source, host) && r.Port == 8084, + "the csf.deny entry must be removed whatever action the target names: %+v", r) + } + }) + + t.Run("csfnativeconnlimit", func(t *testing.T) { + // csf writes a native connection limit — a single inbound tcp port, no address, + // per-source, rejecting the excess — to csf.conf's CONNLIMIT list rather than + // the hook (isConnLimitRule). Every other connection-limit shape now routes to + // the hook (needsHook), so the shared connlimit probe matches csf on its global + // hook form first; round-trip the native form here to keep the CONNLIMIT path + // covered. Gated to csf. + if mgr.Type() != CSFType { + t.Skip("csf-specific csf.conf CONNLIMIT path") + } + roundTripRule(t, ctx, mgr, zone, &Rule{Proto: TCP, Port: 8082, Action: Reject, ConnLimit: &ConnLimit{Count: 15, PerSource: true}}) + }) + t.Run("comment", func(t *testing.T) { requireCap(t, caps.Comments) // Most backends carry a comment on a bare port rule; CSF and APF can only