diff --git a/client/firewall/iptables/family_linux.go b/client/firewall/iptables/family_linux.go index 873593520..c5ed8cc20 100644 --- a/client/firewall/iptables/family_linux.go +++ b/client/firewall/iptables/family_linux.go @@ -112,6 +112,10 @@ type family struct { // AddFilterRule writes here; DeleteFilterRule looks up by id. filters map[nbid.RuleID]*Rule ipsetCounter *ipsetCounter + // ipsetSupported records whether the kernel can create the hash:net + // sets the source matches rely on; probed once at init. When false, + // multi-source rules expand to one rule per source prefix. + ipsetSupported bool // rules holds NAT, jump, and MSS-clamping rules (auxiliary // plumbing that isn't a filter rule). @@ -155,6 +159,8 @@ func newFamily(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint1 func (r *family) init(stateManager *statemanager.Manager) error { r.stateManager = stateManager + r.ipsetSupported = r.probeIPSetSupport() + if err := r.cleanUpDefaultForwardRules(); err != nil { log.Errorf("failed to clean up rules from FORWARD chain: %s", err) } diff --git a/client/firewall/iptables/filter_linux.go b/client/firewall/iptables/filter_linux.go index 5fc578700..8955d9541 100644 --- a/client/firewall/iptables/filter_linux.go +++ b/client/firewall/iptables/filter_linux.go @@ -38,14 +38,8 @@ func (r *family) AddFilterRule( return existing, nil } - srcMatch, err := r.applySourceMatch(sourceNetwork(sources), sources) + rule, err := r.installFilterRules(ruleID, sources, destination, proto, sPort, dPort, action, r.ipsetSupported) if err != nil { - return nil, fmt.Errorf("apply source match: %w", err) - } - - rule, err := r.installFilterRule(ruleID, srcMatch, destination, proto, sPort, dPort, action) - if err != nil { - r.dropSourceMatch(srcMatch) return nil, err } @@ -54,6 +48,34 @@ func (r *family) AddFilterRule( return rule, nil } +// installFilterRules resolves the source matches and installs one +// iptables rule per match. It is more than one rule only when useIPSet +// is false and a multi-source rule has to be expanded per prefix. +func (r *family) installFilterRules( + ruleID nbid.RuleID, + sources []netip.Prefix, + destination firewall.Network, + proto firewall.Protocol, + sPort *firewall.Port, + dPort *firewall.Port, + action firewall.Action, + useIPSet bool, +) (*Rule, error) { + srcMatches, err := r.applySourceMatches(sources, useIPSet) + if err != nil { + return nil, fmt.Errorf("apply source match: %w", err) + } + + rule, err := r.installFilterRule(ruleID, srcMatches, destination, proto, sPort, dPort, action) + if err != nil { + for _, srcMatch := range srcMatches { + r.dropSourceMatch(srcMatch) + } + return nil, err + } + return rule, nil +} + func (r *family) hasRule(id nbid.RuleID) bool { _, ok := r.filters[id] return ok @@ -80,25 +102,28 @@ func (r *family) DeleteFilterRule(rule firewall.Rule) error { return nil } - // DeleteIfExists keeps both deletes idempotent so a retry after a - // partial failure does not error on the half that was already removed. + // DeleteIfExists keeps the deletes idempotent so a retry after a + // partial failure does not error on the parts already removed. var merr *multierror.Error - if err := r.iptablesClient.DeleteIfExists(tableFilter, pr.chain, pr.specs...); err != nil { - merr = multierror.Append(merr, fmt.Errorf("delete rule from %s: %w", pr.chain, err)) - } - if pr.mangleSpecs != nil { - if err := r.iptablesClient.DeleteIfExists(tableMangle, chainRTPre, pr.mangleSpecs...); err != nil { - merr = multierror.Append(merr, fmt.Errorf("delete mangle rule: %w", err)) + for _, fs := range pr.allSpecs() { + if err := r.iptablesClient.DeleteIfExists(tableFilter, pr.chain, fs.specs...); err != nil { + merr = multierror.Append(merr, fmt.Errorf("delete rule from %s: %w", pr.chain, err)) + } + if fs.mangleSpecs != nil { + if err := r.iptablesClient.DeleteIfExists(tableMangle, chainRTPre, fs.mangleSpecs...); err != nil { + merr = multierror.Append(merr, fmt.Errorf("delete mangle rule: %w", err)) + } } } if merr != nil { - // Leave the rule tracked so the caller retries the remaining half. + // Leave the rule tracked so the caller retries the remaining part. return nberrors.FormatErrorOrNil(merr) } // The rule is gone from iptables, so untrack it regardless of how the // refcount decrement goes, but surface decrement failures so callers - // see the ipset desync. + // see the ipset desync. Only the primary spec can reference sets: the + // per-prefix expansion never uses them. delete(r.filters, ruleID) r.updateState() if err := r.decrementSetCounter(pr.specs); err != nil { @@ -136,6 +161,28 @@ func sourceNetwork(sources []netip.Prefix) firewall.Network { } } +// applySourceMatches returns one source match fragment per iptables +// rule needed for the sources: normally a single fragment (a set match, +// a direct -s match, or nil for match-any), and one -s fragment per +// prefix when a multi-source rule cannot use ipset. Per-prefix rules +// are the only form a kernel without the ipset modules can express. +func (r *family) applySourceMatches(sources []netip.Prefix, useIPSet bool) ([][]string, error) { + network := sourceNetwork(sources) + if !network.IsSet() || useIPSet { + match, err := r.applySourceMatch(network, sources) + if err != nil { + return nil, err + } + return [][]string{match}, nil + } + + matches := make([][]string, 0, len(sources)) + for _, source := range sources { + matches = append(matches, []string{"-s", source.String()}) + } + return matches, nil +} + // applySourceMatch returns the iptables match fragment for the rule's // source. For a Set it increments the shared ipset's refcount; for a // Prefix it emits a direct -s match; for the wildcard it returns nil. @@ -188,14 +235,15 @@ func (r *family) decrementSetCounter(rule []string) error { return nberrors.FormatErrorOrNil(merr) } -// installFilterRule assembles and writes one iptables filter-chain -// rule. With destination empty the rule lands in the peer ACL input -// chain and a paired mangle PREROUTING rule is added for the redirect -// mark. With destination set the rule lands in the route ACL forward -// chain and there is no mangle pairing. +// installFilterRule assembles and writes the iptables filter-chain +// rules for one filter rule, one per source match fragment. With +// destination empty the rules land in the peer ACL input chain and each +// gets a paired mangle PREROUTING rule for the redirect mark. With +// destination set the rules land in the route ACL forward chain and +// there is no mangle pairing. func (r *family) installFilterRule( ruleID nbid.RuleID, - srcMatch []string, + srcMatches [][]string, destination firewall.Network, protocol firewall.Protocol, sPort, dPort *firewall.Port, @@ -205,7 +253,6 @@ func (r *family) installFilterRule( proto := protoForFamily(protocol, r.v6) - specs := slices.Clone(srcMatch) var destExp []string if isRoute { var err error @@ -213,64 +260,93 @@ func (r *family) installFilterRule( if err != nil { return nil, fmt.Errorf("apply network -d: %w", err) } - specs = append(specs, destExp...) } - specs = append(specs, filterMatchSpecs(proto, sPort, dPort)...) - - var mangleSpecs []string - if !isRoute { - mangleSpecs = slices.Clone(specs) - mangleSpecs = append(mangleSpecs, - "-i", r.wgIface.Name(), - "-m", "addrtype", "--dst-type", "LOCAL", - "-j", "MARK", "--set-xmark", fmt.Sprintf("%#x", nbnet.PreroutingFwmarkRedirected), - ) - } - - specs = append(specs, "-j", actionToStr(action)) + matchSpecs := filterMatchSpecs(proto, sPort, dPort) chain := chainACLInput if isRoute { chain = chainRTFwdIn } - // Peer ACL drops are inserted at position 1 so they precede the - // chain's catch-all; route ACL drops are inserted at position 2 - // to sit immediately after the established/related accept rule. - var err error - if action == firewall.ActionDrop { - pos := 1 - if isRoute { - pos = 2 - } - err = r.iptablesClient.Insert(tableFilter, chain, pos, specs...) - } else { - err = r.iptablesClient.Append(tableFilter, chain, specs...) - } - if err != nil { - r.dropSourceMatch(destExp) - return nil, fmt.Errorf("install filter rule on %s: %w", chain, err) - } + var installed []filterSpecs + for _, srcMatch := range srcMatches { + specs := slices.Clone(srcMatch) + specs = append(specs, destExp...) + specs = append(specs, matchSpecs...) - // The mangle redirect-mark rule is best effort: the filter rule itself - // is what enforces the ACL, so a mangle failure must not undo it. Drop - // the spec so teardown does not try to remove a rule that was not added. - if mangleSpecs != nil { - if err := r.iptablesClient.Append(tableMangle, chainRTPre, mangleSpecs...); err != nil { - log.Errorf("add mangle rule: %v", err) - mangleSpecs = nil + var mangleSpecs []string + if !isRoute { + mangleSpecs = slices.Clone(specs) + mangleSpecs = append(mangleSpecs, + "-i", r.wgIface.Name(), + "-m", "addrtype", "--dst-type", "LOCAL", + "-j", "MARK", "--set-xmark", fmt.Sprintf("%#x", nbnet.PreroutingFwmarkRedirected), + ) } + + specs = append(specs, "-j", actionToStr(action)) + + if err := r.insertFilterRule(chain, action, specs); err != nil { + // Leave nothing half-installed: the caller sees an error, so a + // partial rule would silently keep matching without being tracked. + r.removeFilterSpecs(chain, installed) + r.dropSourceMatch(destExp) + return nil, fmt.Errorf("install filter rule on %s: %w", chain, err) + } + + // The mangle redirect-mark rule is best effort: the filter rule itself + // is what enforces the ACL, so a mangle failure must not undo it. Drop + // the spec so teardown does not try to remove a rule that was not added. + if mangleSpecs != nil { + if err := r.iptablesClient.Append(tableMangle, chainRTPre, mangleSpecs...); err != nil { + log.Errorf("add mangle rule: %v", err) + mangleSpecs = nil + } + } + + installed = append(installed, filterSpecs{specs: specs, mangleSpecs: mangleSpecs}) } return &Rule{ id: ruleID, - specs: specs, - mangleSpecs: mangleSpecs, + specs: installed[0].specs, + mangleSpecs: installed[0].mangleSpecs, + extraRules: installed[1:], chain: chain, v6: r.v6, }, nil } +// insertFilterRule writes one assembled rule spec into the given ACL +// chain. Peer ACL drops are inserted at position 1 so they precede the +// chain's catch-all; route ACL drops are inserted at position 2 to sit +// immediately after the established/related accept rule. +func (r *family) insertFilterRule(chain string, action firewall.Action, specs []string) error { + if action == firewall.ActionDrop { + pos := 1 + if chain == chainRTFwdIn { + pos = 2 + } + return r.iptablesClient.Insert(tableFilter, chain, pos, specs...) + } + return r.iptablesClient.Append(tableFilter, chain, specs...) +} + +// removeFilterSpecs deletes the already-installed rules of a partially +// applied filter rule. +func (r *family) removeFilterSpecs(chain string, installed []filterSpecs) { + for _, fs := range installed { + if err := r.iptablesClient.DeleteIfExists(tableFilter, chain, fs.specs...); err != nil { + log.Debugf("delete partial filter rule: %v", err) + } + if fs.mangleSpecs != nil { + if err := r.iptablesClient.DeleteIfExists(tableMangle, chainRTPre, fs.mangleSpecs...); err != nil { + log.Debugf("delete partial mangle rule: %v", err) + } + } + } +} + // applyNetwork resolves a firewall.Network into the iptables match // fragment for the given direction flag (-s or -d). Set networks // increment the shared ipset refcount; prefixes emit a direct match; diff --git a/client/firewall/iptables/ipset_linux.go b/client/firewall/iptables/ipset_linux.go index 0d5f71d69..2a3685af7 100644 --- a/client/firewall/iptables/ipset_linux.go +++ b/client/firewall/iptables/ipset_linux.go @@ -6,6 +6,7 @@ import ( "fmt" "net/netip" + "github.com/google/uuid" "github.com/hashicorp/go-multierror" "github.com/lrh3321/ipset-go" log "github.com/sirupsen/logrus" @@ -14,6 +15,32 @@ import ( firewall "github.com/netbirdio/netbird/client/firewall/manager" ) +// probeIPSetSupport checks whether the kernel can create the ipset type +// used for source and destination matches. On kernels lacking the +// required ipset hash module, set creation fails (e.g. "invalid +// argument"), which would otherwise fail every multi-source rule and +// leave traffic the policy permits blocked by the catch-all drop. When +// unsupported, multi-source rules fall back to one rule per prefix. +func (r *family) probeIPSetSupport() bool { + // Use a unique name so concurrent processes don't collide and we only ever + // destroy the set we created ourselves. ipset names are limited to 31 chars, + // so use a short random suffix. + probeName := "nb-probe-" + uuid.New().String()[:8] + + if err := r.createIPSet(probeName); err != nil { + log.Warnf("ipset is not available (failed to create probe set: %v); "+ + "falling back to per-IP iptables ACL rules. Ensure the kernel provides "+ + "the ipset hash:net module (ip_set_hash_net) for better performance with large rule sets", err) + return false + } + + if err := r.destroyIPSet(probeName); err != nil { + log.Debugf("destroy ipset probe set %q: %v", probeName, err) + } + + return true +} + func (r *family) createIpSet(setName string, sources []netip.Prefix) error { if err := r.createIPSet(setName); err != nil { return fmt.Errorf("create set %s: %w", setName, err) diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index 7e0061c00..98983aeb6 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -247,6 +247,99 @@ func TestIptablesManagerIPSet(t *testing.T) { }) } +// TestIptablesFilterIPSetFallback verifies that when the kernel lacks +// ipset support, a multi-source rule falls back to one iptables rule +// per source prefix instead of silently leaving the chain empty. See +// discussion #6125. +func TestIptablesFilterIPSetFallback(t *testing.T) { + ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + manager, err := Create(ifaceMock, iface.DefaultMTU) + require.NoError(t, err) + require.NoError(t, manager.Init(nil)) + + defer func() { + require.NoError(t, manager.Close(nil)) + }() + + // Simulate a kernel without the ipset hash module. + manager.family4.ipsetSupported = false + + sources := []netip.Prefix{ + netip.MustParsePrefix("10.20.0.42/32"), + netip.MustParsePrefix("10.20.0.43/32"), + } + port := &fw.Port{Values: []uint16{22}} + + rule, err := manager.AddFilterRule(nil, sources, fw.Network{}, "tcp", nil, port, fw.ActionAccept) + require.NoError(t, err, "AddFilterRule should succeed via fallback") + + rr := rule.(*Rule) + all := rr.allSpecs() + require.Len(t, all, len(sources), "each source prefix needs its own rule") + for i, fs := range all { + joined := strings.Join(fs.specs, " ") + require.Contains(t, joined, "-s "+sources[i].String(), "fallback rule must match by source prefix") + require.NotContains(t, joined, matchSet, "fallback rule must not use ipset matching") + + // The rule must actually be present in the ACL chain (not silently dropped). + checkRuleSpecs(t, ipv4Client, rr.chain, true, fs.specs...) + } + + require.NoError(t, manager.DeleteFilterRule(rule), "failed to delete fallback rule") + for _, fs := range all { + checkRuleSpecs(t, ipv4Client, rr.chain, false, fs.specs...) + } +} + +// TestIptablesRouteFilterIPSetFallback covers the route ACL side of the +// fallback: with a destination set, the expanded per-source rules land +// in the route forward chain and are all removed on delete. +func TestIptablesRouteFilterIPSetFallback(t *testing.T) { + ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + manager, err := Create(ifaceMock, iface.DefaultMTU) + require.NoError(t, err) + require.NoError(t, manager.Init(nil)) + + defer func() { + require.NoError(t, manager.Close(nil)) + }() + + manager.family4.ipsetSupported = false + + sources := []netip.Prefix{ + netip.MustParsePrefix("172.16.0.0/16"), + netip.MustParsePrefix("192.168.0.0/16"), + } + destination := fw.Network{Prefix: netip.MustParsePrefix("10.0.0.0/8")} + port := &fw.Port{Values: []uint16{443}} + + rule, err := manager.AddFilterRule(nil, sources, destination, "tcp", nil, port, fw.ActionAccept) + require.NoError(t, err, "route ACL must install without ipset") + + rr := rule.(*Rule) + require.Equal(t, chainRTFwdIn, rr.chain, "route rule must land in the forward chain") + + all := rr.allSpecs() + require.Len(t, all, len(sources), "each source prefix needs its own rule") + for i, fs := range all { + joined := strings.Join(fs.specs, " ") + require.Contains(t, joined, "-s "+sources[i].String(), "fallback rule must match by source prefix") + require.NotContains(t, joined, matchSet, "fallback rule must not use ipset matching") + require.Nil(t, fs.mangleSpecs, "route rules have no mangle pairing") + + checkRuleSpecs(t, ipv4Client, rr.chain, true, fs.specs...) + } + + require.NoError(t, manager.DeleteFilterRule(rule), "failed to delete fallback rule") + for _, fs := range all { + checkRuleSpecs(t, ipv4Client, rr.chain, false, fs.specs...) + } +} + func checkRuleSpecs(t *testing.T, ipv4Client *iptables.IPTables, chainName string, mustExists bool, rulespec ...string) { t.Helper() exists, err := ipv4Client.Exists("filter", chainName, rulespec...) diff --git a/client/firewall/iptables/rule.go b/client/firewall/iptables/rule.go index 23e5846a6..33bcbd1d2 100644 --- a/client/firewall/iptables/rule.go +++ b/client/firewall/iptables/rule.go @@ -10,8 +10,25 @@ type Rule struct { id manager.RuleID specs []string mangleSpecs []string - chain string - v6 bool + // extraRules holds the rules beyond the first when the ipset + // fallback expands a multi-source rule into one rule per prefix. + extraRules []filterSpecs + chain string + v6 bool +} + +// filterSpecs is one installed iptables rule: its filter-table spec and +// the paired mangle redirect-mark spec (nil for route rules or when the +// mangle rule could not be added). +type filterSpecs struct { + specs []string + mangleSpecs []string +} + +// allSpecs returns the spec pairs of every iptables rule backing this +// Rule, the primary one first. +func (r *Rule) allSpecs() []filterSpecs { + return append([]filterSpecs{{specs: r.specs, mangleSpecs: r.mangleSpecs}}, r.extraRules...) } // ID returns the rule id