diff --git a/client/firewall/iptables/acl_linux.go b/client/firewall/iptables/acl_linux.go index 89d1ebf7c..124fa2d56 100644 --- a/client/firewall/iptables/acl_linux.go +++ b/client/firewall/iptables/acl_linux.go @@ -42,12 +42,12 @@ type aclManager struct { optionalEntries map[string][]entry ipsetStore *ipsetStore v6 bool - ipsetSupported bool + ipsetSupport *ipsetSupport stateManager *statemanager.Manager } -func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper) (*aclManager, error) { +func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper, ipsetSupport *ipsetSupport) (*aclManager, error) { return &aclManager{ iptablesClient: iptablesClient, wgIface: wgIface, @@ -55,14 +55,13 @@ func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper) (*acl optionalEntries: make(map[string][]entry), ipsetStore: newIpsetStore(), v6: iptablesClient.Proto() == iptables.ProtocolIPv6, + ipsetSupport: ipsetSupport, }, nil } func (m *aclManager) init(stateManager *statemanager.Manager) error { m.stateManager = stateManager - m.ipsetSupported = m.probeIPSetSupport() - m.seedInitialEntries() m.seedInitialOptionalEntries() @@ -88,18 +87,71 @@ func (m *aclManager) AddPeerFiltering( action firewall.Action, ipsetName string, ) ([]firewall.Rule, error) { - chain := chainNameInputRules + ipsetName = m.resolveIPSetName(ipsetName, sPort, dPort, action) + if ipsetName == "" { + return m.addPeerRule(ip, protocol, sPort, dPort, action, "") + } + rules, err := m.addPeerRule(ip, protocol, sPort, dPort, action, ipsetName) + if err == nil { + return rules, nil + } + + var unusable *ipsetUnusableError + if !errors.As(err, &unusable) { + return nil, err + } + + // The set could not be created or matched. Drop whatever was created and + // retry the rule matching the IP directly; only if that succeeds do we know + // ipset was to blame and latch it off for subsequent rules. + m.discardIPSet(ipsetName) + + rules, retryErr := m.addPeerRule(ip, protocol, sPort, dPort, action, "") + if retryErr != nil { + return nil, fmt.Errorf("add peer rule (ipset: %w): %w", unusable.cause, retryErr) + } + + m.ipsetSupport.markUnsupported(unusable.cause) + + return rules, nil +} + +// resolveIPSetName derives the ipset name for a rule, returning "" when the rule +// must match the IP directly: either no set was requested or ipset is unusable. +func (m *aclManager) resolveIPSetName(ipsetName string, sPort, dPort *firewall.Port, action firewall.Action) string { ipsetName = transformIPsetName(ipsetName, sPort, dPort, action) - if m.v6 && ipsetName != "" { + if ipsetName == "" || !m.ipsetSupport.supported() { + return "" + } + + if m.v6 { ipsetName += "-v6" } - // When the kernel lacks the required ipset hash module, fall back to - // per-IP iptables rules (pre-0.68 behavior) so ACLs keep working instead - // of silently leaving the chain empty. - if ipsetName != "" && !m.ipsetSupported { - ipsetName = "" + + return ipsetName +} + +// discardIPSet removes a set that turned out to be unusable, so a later rule +// does not find it in the store and assume it works. +func (m *aclManager) discardIPSet(ipsetName string) { + m.ipsetStore.deleteIpset(ipsetName) + + if err := m.destroyIPSet(ipsetName); err != nil { + log.Debugf("destroy unusable ipset %s: %v", ipsetName, err) } +} + +func (m *aclManager) addPeerRule( + ip net.IP, + protocol firewall.Protocol, + sPort *firewall.Port, + dPort *firewall.Port, + action firewall.Action, + ipsetName string, +) ([]firewall.Rule, error) { + chain := chainNameInputRules + proto := protoForFamily(protocol, m.v6) specs := filterRuleSpecs(ip, proto, sPort, dPort, action, ipsetName) @@ -114,7 +166,7 @@ func (m *aclManager) AddPeerFiltering( if ipsetName != "" { if ipList, ipsetExists := m.ipsetStore.ipset(ipsetName); ipsetExists { if err := m.addToIPSet(ipsetName, ip); err != nil { - return nil, fmt.Errorf("add IP to ipset: %w", err) + return nil, ipsetUnusable(fmt.Errorf("add IP to ipset: %w", err)) } // if ruleset already exists it means we already have the firewall rule // so we need to update IPs in the ruleset and return new fw.Rule object for ACL manager. @@ -137,10 +189,10 @@ func (m *aclManager) AddPeerFiltering( } } if err := m.createIPSet(ipsetName); err != nil { - return nil, fmt.Errorf("create ipset: %w", err) + return nil, ipsetUnusable(fmt.Errorf("create ipset: %w", err)) } if err := m.addToIPSet(ipsetName, ip); err != nil { - return nil, fmt.Errorf("add IP to ipset: %w", err) + return nil, ipsetUnusable(fmt.Errorf("add IP to ipset: %w", err)) } ipList := newIpList(ip.String()) @@ -149,7 +201,7 @@ func (m *aclManager) AddPeerFiltering( ok, err := m.iptablesClient.Exists(tableFilter, chain, specs...) if err != nil { - return nil, fmt.Errorf("failed to check rule: %w", err) + return nil, maybeIPSetUnusable(ipsetName, fmt.Errorf("check rule: %w", err)) } if ok { return nil, fmt.Errorf("rule already exists") @@ -163,7 +215,7 @@ func (m *aclManager) AddPeerFiltering( err = m.iptablesClient.Append(tableFilter, chain, specs...) } if err != nil { - return nil, err + return nil, maybeIPSetUnusable(ipsetName, err) } if err := m.iptablesClient.Append(tableMangle, chainRTPRE, mangleSpecs...); err != nil { @@ -507,40 +559,6 @@ func transformIPsetName(ipsetName string, sPort, dPort *firewall.Port, action fi } } -// probeIPSetSupport checks whether the kernel can create the ipset type used for -// ACL rules. On kernels lacking the required ipset hash module, ipset creation -// fails (e.g. "invalid argument"), which would otherwise leave the ACL chain -// empty and silently drop all policy-permitted inbound traffic. When unsupported, -// the manager falls back to per-IP iptables rules. -func (m *aclManager) 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] - - opts := ipset.CreateOptions{ - Replace: true, - } - if m.v6 { - opts.Family = ipset.FamilyIPV6 - } - - if err := ipset.Create(probeName, ipset.TypeHashNet, opts); 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 - } - - defer func() { - if err := ipset.Destroy(probeName); err != nil { - log.Debugf("destroy ipset probe set %q: %v", probeName, err) - } - }() - - return true -} - func (m *aclManager) createIPSet(name string) error { opts := ipset.CreateOptions{ Replace: true, diff --git a/client/firewall/iptables/ipset_support_linux.go b/client/firewall/iptables/ipset_support_linux.go new file mode 100644 index 000000000..8a0700955 --- /dev/null +++ b/client/firewall/iptables/ipset_support_linux.go @@ -0,0 +1,75 @@ +package iptables + +import ( + "sync" + + log "github.com/sirupsen/logrus" +) + +// ipsetSupport tracks whether ipset-backed firewall rules can be installed. +// +// It starts optimistic and latches to unsupported the first time the kernel +// proves otherwise: either the hash:net set type is missing (ip_set_hash_net) or +// iptables cannot match against a set (xt_set). Callers then emit per-IP and +// per-prefix rules instead. Without the fallback, a rule referencing an unusable +// set is never installed and the catch-all DROP silently blocks traffic the +// policy permits. +// +// One instance is shared by the ACL managers and routers of both address +// families, because ipset availability is a property of the kernel rather than +// of any single table. +type ipsetSupport struct { + mu sync.RWMutex + unsupported bool +} + +func newIPSetSupport() *ipsetSupport { + return &ipsetSupport{} +} + +func (s *ipsetSupport) supported() bool { + s.mu.RLock() + defer s.mu.RUnlock() + + return !s.unsupported +} + +// markUnsupported records that ipset cannot be used, logging the reason once. +func (s *ipsetSupport) markUnsupported(cause error) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.unsupported { + return + } + s.unsupported = true + + log.Warnf("ipset is unavailable (%v); falling back to per-IP firewall rules. "+ + "Ensure the kernel provides ip_set_hash_net and xt_set; without them rule "+ + "sets are larger and slower to converge on networks with many peers", cause) +} + +// ipsetUnusableError marks a failure attributable to ipset, so the caller can +// retry the same rule in its per-IP form before latching the capability off. +type ipsetUnusableError struct { + cause error +} + +func (e *ipsetUnusableError) Error() string { return e.cause.Error() } + +func (e *ipsetUnusableError) Unwrap() error { return e.cause } + +func ipsetUnusable(cause error) error { + return &ipsetUnusableError{cause: cause} +} + +// maybeIPSetUnusable marks an iptables failure as ipset-attributable only when the +// rule actually carried a set match, since the same call can fail for unrelated +// reasons on a rule that matches addresses directly. +func maybeIPSetUnusable(ipsetName string, err error) error { + if ipsetName == "" { + return err + } + + return ipsetUnusable(err) +} diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index aa052d933..e49ac7ebe 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -33,6 +33,10 @@ type Manager struct { router *router rawSupported bool + // ipsetSupport is shared by the ACL managers and routers of both families, + // so a kernel without usable ipset support degrades them together. + ipsetSupport *ipsetSupport + // IPv6 counterparts, nil when no v6 overlay ipv6Client *iptables.IPTables aclMgr6 *aclManager @@ -53,16 +57,17 @@ func Create(wgIface iFaceMapper, mtu uint16) (*Manager, error) { } m := &Manager{ - wgIface: wgIface, - ipv4Client: iptablesClient, + wgIface: wgIface, + ipv4Client: iptablesClient, + ipsetSupport: newIPSetSupport(), } - m.router, err = newRouter(iptablesClient, wgIface, mtu) + m.router, err = newRouter(iptablesClient, wgIface, mtu, m.ipsetSupport) if err != nil { return nil, fmt.Errorf("create router: %w", err) } - m.aclMgr, err = newAclManager(iptablesClient, wgIface) + m.aclMgr, err = newAclManager(iptablesClient, wgIface, m.ipsetSupport) if err != nil { return nil, fmt.Errorf("create acl manager: %w", err) } @@ -83,7 +88,7 @@ func (m *Manager) createIPv6Components(wgIface iFaceMapper, mtu uint16) error { } m.ipv6Client = ip6Client - m.router6, err = newRouter(ip6Client, wgIface, mtu) + m.router6, err = newRouter(ip6Client, wgIface, mtu, m.ipsetSupport) if err != nil { return fmt.Errorf("create v6 router: %w", err) } @@ -92,7 +97,7 @@ func (m *Manager) createIPv6Components(wgIface iFaceMapper, mtu uint16) error { // Forwarding refcounter is per-family but shared between v4 and v6 routers. m.router6.ipFwdState = m.router.ipFwdState - m.aclMgr6, err = newAclManager(ip6Client, wgIface) + m.aclMgr6, err = newAclManager(ip6Client, wgIface, m.ipsetSupport) if err != nil { return fmt.Errorf("create v6 acl manager: %w", err) } diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index 2c3c1a08e..f89732855 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -292,39 +292,97 @@ func TestIptablesCreatePerformance(t *testing.T) { } } -// TestIptablesACLIPSetFallback verifies that when the kernel lacks ipset support, -// the ACL manager falls back to per-IP iptables rules (-s ) instead of -// silently leaving the chain empty. See discussion #6125. -func TestIptablesACLIPSetFallback(t *testing.T) { - ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) - require.NoError(t, err) +// newACLTestManager returns a started manager. Create()/Init() is used so the +// router-owned chains (chainRTFWDIN/OUT) exist before the ACL manager's +// createDefaultChains() references them. +func newACLTestManager(t *testing.T) *Manager { + t.Helper() - // Use Create()/Init() so the router-owned chains (chainRTFWDIN/OUT) are - // created before the ACL manager's createDefaultChains() references them. manager, err := Create(ifaceMock, iface.DefaultMTU) require.NoError(t, err) require.NoError(t, manager.Init(nil)) - - aclMgr := manager.aclMgr - // Simulate a kernel without the ipset hash module. - aclMgr.ipsetSupported = false - - defer func() { + t.Cleanup(func() { require.NoError(t, manager.Close(nil)) - }() + }) + + return manager +} + +// TestIptablesACLUsesIPSetOnHealthyKernel guards the default: on a kernel that +// does have ipset, rules must keep matching a set. A regression that reported +// ipset as unusable would silently move every Linux client to per-IP rules. +func TestIptablesACLUsesIPSetOnHealthyKernel(t *testing.T) { + ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + manager := newACLTestManager(t) ip := netip.MustParseAddr("10.20.0.42") port := &fw.Port{Values: []uint16{22}} - rules, err := aclMgr.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, "nb0000001") - require.NoError(t, err, "AddPeerFiltering should succeed via fallback") + rules, err := manager.aclMgr.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, "nb0000001") + require.NoError(t, err) + require.NotEmpty(t, rules) + + rule := rules[0].(*Rule) + require.Equal(t, "nb0000001-dport", rule.ipsetName, "healthy kernel must use an ipset") + require.Contains(t, rule.specs, "--match-set") + require.True(t, manager.ipsetSupport.supported(), "ipset must not be latched off on a healthy kernel") + + checkRuleSpecs(t, ipv4Client, rule.chain, true, rule.specs...) +} + +// TestIptablesACLFallsBackWhenIPSetUnusable drives the real failure path: an +// oversized set name is rejected by the kernel, which stands in for a kernel +// without ip_set_hash_net or xt_set. The rule must still land in the chain, +// matching the IP directly, and the capability must latch off so later rules skip +// ipset. Before the fallback existed, the rule was dropped and the catch-all DROP +// silently blocked traffic the policy permits. +func TestIptablesACLFallsBackWhenIPSetUnusable(t *testing.T) { + ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + manager := newACLTestManager(t) + + // ipset names are limited to 31 characters, so creating this set fails. + unusableName := strings.Repeat("a", 40) + + ip := netip.MustParseAddr("10.20.0.42") + port := &fw.Port{Values: []uint16{22}} + + rules, err := manager.aclMgr.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, unusableName) + require.NoError(t, err, "AddPeerFiltering must succeed by falling back") require.NotEmpty(t, rules) rule := rules[0].(*Rule) require.Empty(t, rule.ipsetName, "fallback rule must not reference an ipset") - require.Contains(t, strings.Join(rule.specs, " "), "-s 10.20.0.42", "fallback rule must match by source IP") - require.NotContains(t, strings.Join(rule.specs, " "), "--match-set", "fallback rule must not use ipset matching") + require.Contains(t, strings.Join(rule.specs, " "), "-s 10.20.0.42", "fallback rule must match the source IP") + require.NotContains(t, strings.Join(rule.specs, " "), "--match-set") - // The rule must actually be present in the ACL chain (not silently dropped). + // The rule must actually be present, not silently missing. checkRuleSpecs(t, ipv4Client, rule.chain, true, rule.specs...) + + require.False(t, manager.ipsetSupport.supported(), "failure must latch ipset off") + + // A subsequent rule with a perfectly valid set name now skips ipset too. + next, err := manager.aclMgr.AddPeerFiltering(nil, netip.MustParseAddr("10.20.0.43").AsSlice(), "tcp", nil, port, fw.ActionAccept, "nb0000001") + require.NoError(t, err) + require.NotEmpty(t, next) + require.Empty(t, next[0].(*Rule).ipsetName, "later rules must skip ipset once latched") +} + +// TestIptablesACLLeavesNoIPSetAfterFallback verifies the set created before the +// failure is destroyed, so a later rule does not find a half-built set and assume +// ipset works. +func TestIptablesACLLeavesNoIPSetAfterFallback(t *testing.T) { + manager := newACLTestManager(t) + + port := &fw.Port{Values: []uint16{22}} + ip := netip.MustParseAddr("10.20.0.42") + + _, err := manager.aclMgr.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, strings.Repeat("a", 40)) + require.NoError(t, err) + + _, exists := manager.aclMgr.ipsetStore.ipset(strings.Repeat("a", 40) + "-dport") + require.False(t, exists, "unusable set must not stay in the store") } diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index 01b18570c..4c61b85b3 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -3,6 +3,7 @@ package iptables import ( + "errors" "fmt" "maps" "net/netip" @@ -51,6 +52,10 @@ const ( markManglePost = "mark-mangle-post" matchSet = "--match-set" + // routeSourceSuffix names the extra rules a route ACL needs when ipset is + // unusable and each source prefix has to be matched by its own rule. + routeSourceSuffix = "_src" + dnatSuffix = "_dnat" snatSuffix = "_snat" fwdSuffix = "_fwd" @@ -68,7 +73,6 @@ type ruleInfo struct { } type routeFilteringRuleParams struct { - Source firewall.Network Destination firewall.Network Proto firewall.Protocol SPort *firewall.Port @@ -90,12 +94,13 @@ type router struct { legacyManagement bool mtu uint16 v6 bool + ipsetSupport *ipsetSupport stateManager *statemanager.Manager ipFwdState *ipfwdstate.IPForwardingState } -func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint16) (*router, error) { +func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint16, ipsetSupport *ipsetSupport) (*router, error) { r := &router{ iptablesClient: iptablesClient, rules: make(map[string][]string), @@ -103,6 +108,7 @@ func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint1 mtu: mtu, v6: iptablesClient.Proto() == iptables.ProtocolIPv6, ipFwdState: ipfwdstate.NewIPForwardingState(wgIface.Name()), + ipsetSupport: ipsetSupport, } r.ipsetCounter = refcounter.New( @@ -151,15 +157,7 @@ func (r *router) AddRouteFiltering( return ruleKey, nil } - var source firewall.Network - if len(sources) > 1 { - source.Set = firewall.NewPrefixSet(sources) - } else if len(sources) > 0 { - source.Prefix = sources[0] - } - params := routeFilteringRuleParams{ - Source: source, Destination: destination, Proto: proto, SPort: sPort, @@ -167,30 +165,136 @@ func (r *router) AddRouteFiltering( Action: action, } - rule, err := r.genRouteRuleSpec(params, sources) - if err != nil { - return nil, fmt.Errorf("generate route rule spec: %w", err) - } + err := r.installRouteRules(string(ruleKey), params, sources, r.ipsetSupport.supported()) - // Insert DROP rules at the beginning, append ACCEPT rules at the end - if action == firewall.ActionDrop { - // after the established rule - err = r.iptablesClient.Insert(tableFilter, chainRTFWDIN, 2, rule...) - } else { - err = r.iptablesClient.Append(tableFilter, chainRTFWDIN, rule...) + var unusable *ipsetUnusableError + if errors.As(err, &unusable) { + // The set could not be created or matched. Retry matching each source + // prefix on its own; only if that works do we know ipset was to blame. + r.removeRouteRules(string(ruleKey)) + + if retryErr := r.installRouteRules(string(ruleKey), params, sources, false); retryErr != nil { + return nil, fmt.Errorf("add route rule (ipset: %w): %w", unusable.cause, retryErr) + } + + r.ipsetSupport.markUnsupported(unusable.cause) + err = nil } if err != nil { - return nil, fmt.Errorf("add route rule: %v", err) + return nil, fmt.Errorf("add route rule: %w", err) } - r.rules[string(ruleKey)] = rule - r.updateState() return ruleKey, nil } +// installRouteRules installs every rule needed for one route ACL and records them +// under ruleKey. It is more than one rule only when useIPSet is false and the +// sources have to be matched one prefix at a time. +func (r *router) installRouteRules(ruleKey string, params routeFilteringRuleParams, sources []netip.Prefix, useIPSet bool) error { + specs, err := r.genRouteRuleSpecs(params, sources, useIPSet) + if err != nil { + return fmt.Errorf("generate route rule spec: %w", err) + } + + for i, spec := range specs { + if err := r.insertRouteRule(params.Action, spec); err != nil { + if len(r.findSets(spec)) > 0 { + return ipsetUnusable(err) + } + return err + } + r.rules[routeRuleKey(ruleKey, i)] = spec + } + + return nil +} + +// genRouteRuleSpecs builds the rules for one route ACL. With ipset available that +// is a single rule matching a set of sources; without it, one rule per source +// prefix, which is the only form a stripped kernel can express. +func (r *router) genRouteRuleSpecs(params routeFilteringRuleParams, sources []netip.Prefix, useIPSet bool) ([][]string, error) { + destExp, err := r.applyNetwork("-d", params.Destination, nil) + if err != nil { + return nil, fmt.Errorf("apply network -d: %w", err) + } + + if useIPSet || len(sources) <= 1 { + sourceExp, err := r.applyNetwork("-s", sourceNetwork(sources), sources) + if err != nil { + return nil, fmt.Errorf("apply network -s: %w", err) + } + + return [][]string{assembleRouteRule(sourceExp, destExp, params, r.v6)}, nil + } + + specs := make([][]string, 0, len(sources)) + for _, source := range sources { + sourceExp, err := r.applyNetwork("-s", firewall.Network{Prefix: source}, nil) + if err != nil { + return nil, fmt.Errorf("apply network -s: %w", err) + } + + specs = append(specs, assembleRouteRule(sourceExp, destExp, params, r.v6)) + } + + return specs, nil +} + +func sourceNetwork(sources []netip.Prefix) firewall.Network { + var source firewall.Network + if len(sources) > 1 { + source.Set = firewall.NewPrefixSet(sources) + } else if len(sources) > 0 { + source.Prefix = sources[0] + } + + return source +} + +func (r *router) insertRouteRule(action firewall.Action, spec []string) error { + // Insert DROP rules at the beginning, append ACCEPT rules at the end + if action == firewall.ActionDrop { + // after the established rule + return r.iptablesClient.Insert(tableFilter, chainRTFWDIN, 2, spec...) + } + + return r.iptablesClient.Append(tableFilter, chainRTFWDIN, spec...) +} + +// removeRouteRules deletes the rules recorded for ruleKey, used to undo a partial +// install before retrying without ipset. +func (r *router) removeRouteRules(ruleKey string) { + for i := 0; ; i++ { + key := routeRuleKey(ruleKey, i) + spec, exists := r.rules[key] + if !exists { + return + } + + if err := r.iptablesClient.DeleteIfExists(tableFilter, chainRTFWDIN, spec...); err != nil { + log.Debugf("delete partial route rule %s: %v", key, err) + } + delete(r.rules, key) + + if err := r.decrementSetCounter(spec); err != nil { + log.Debugf("decrement ipset counter for %s: %v", key, err) + } + } +} + +// routeRuleKey names the i-th rule of a route ACL. The first keeps the plain rule +// key so single-rule ACLs, which is every ACL when ipset works, are unaffected. +func routeRuleKey(ruleKey string, i int) string { + if i == 0 { + return ruleKey + } + + return fmt.Sprintf("%s%s%d", ruleKey, routeSourceSuffix, i) +} + func (r *router) hasRule(id string) bool { _, ok := r.rules[id] return ok @@ -199,17 +303,29 @@ func (r *router) hasRule(id string) bool { func (r *router) DeleteRouteRule(rule firewall.Rule) error { ruleKey := rule.ID() - if rule, exists := r.rules[ruleKey]; exists { + if _, exists := r.rules[ruleKey]; !exists { + log.Debugf("route rule %s not found", ruleKey) + r.updateState() + + return nil + } + + // In the ipset fallback one ACL is several rules, one per source prefix. + for i := 0; ; i++ { + key := routeRuleKey(ruleKey, i) + rule, exists := r.rules[key] + if !exists { + break + } + if err := r.iptablesClient.Delete(tableFilter, chainRTFWDIN, rule...); err != nil { return fmt.Errorf("delete route rule: %v", err) } - delete(r.rules, ruleKey) + delete(r.rules, key) if err := r.decrementSetCounter(rule); err != nil { return fmt.Errorf("decrement ipset counter: %w", err) } - } else { - log.Debugf("route rule %s not found", ruleKey) } r.updateState() @@ -927,31 +1043,23 @@ func (r *router) DeleteDNATRule(rule firewall.Rule) error { return nberrors.FormatErrorOrNil(merr) } -func (r *router) genRouteRuleSpec(params routeFilteringRuleParams, sources []netip.Prefix) ([]string, error) { +// assembleRouteRule joins the pre-built source and destination matches with the +// protocol, ports and target of a route ACL. +func assembleRouteRule(sourceExp, destExp []string, params routeFilteringRuleParams, v6 bool) []string { var rule []string - sourceExp, err := r.applyNetwork("-s", params.Source, sources) - if err != nil { - return nil, fmt.Errorf("apply network -s: %w", err) - - } - destExp, err := r.applyNetwork("-d", params.Destination, nil) - if err != nil { - return nil, fmt.Errorf("apply network -d: %w", err) - } - rule = append(rule, sourceExp...) rule = append(rule, destExp...) if params.Proto != firewall.ProtocolALL { - rule = append(rule, "-p", strings.ToLower(protoForFamily(params.Proto, r.v6))) + rule = append(rule, "-p", strings.ToLower(protoForFamily(params.Proto, v6))) rule = append(rule, applyPort("--sport", params.SPort)...) rule = append(rule, applyPort("--dport", params.DPort)...) } rule = append(rule, "-j", actionToStr(params.Action)) - return rule, nil + return rule } func (r *router) applyNetwork(flag string, network firewall.Network, prefixes []netip.Prefix) ([]string, error) { @@ -961,9 +1069,17 @@ func (r *router) applyNetwork(flag string, network firewall.Network, prefixes [] } if network.IsSet() { + // A destination set is populated later from DNS results, so unlike a + // source set it cannot be expanded into per-prefix rules here. Without + // ipset such a rule is not expressible; report it instead of installing + // something broader than the policy allows. + if flag == "-d" && !r.ipsetSupport.supported() { + return nil, fmt.Errorf("destination set %s requires ipset (ip_set_hash_net and xt_set)", network.Set.HashedName()) + } + name := r.ipsetName(network.Set.HashedName()) if _, err := r.ipsetCounter.Increment(name, prefixes); err != nil { - return nil, fmt.Errorf("create or get ipset: %w", err) + return nil, ipsetUnusable(fmt.Errorf("create or get ipset: %w", err)) } return []string{"-m", "set", matchSet, name, direction}, nil diff --git a/client/firewall/iptables/router_linux_test.go b/client/firewall/iptables/router_linux_test.go index 9ca6b9f7e..6d69b1088 100644 --- a/client/firewall/iptables/router_linux_test.go +++ b/client/firewall/iptables/router_linux_test.go @@ -3,9 +3,11 @@ package iptables import ( + "errors" "fmt" "net/netip" "os/exec" + "strings" "testing" "github.com/coreos/go-iptables/iptables" @@ -16,6 +18,7 @@ import ( "github.com/netbirdio/netbird/client/firewall/test" "github.com/netbirdio/netbird/client/iface" nbnet "github.com/netbirdio/netbird/client/net" + "github.com/netbirdio/netbird/shared/management/domain" ) func isIptablesSupported() bool { @@ -31,7 +34,7 @@ func TestIptablesManager_RestoreOrCreateContainers(t *testing.T) { iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) require.NoError(t, err, "failed to init iptables client") - manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU) + manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport()) require.NoError(t, err, "should return a valid iptables manager") require.NoError(t, manager.init(nil)) @@ -84,7 +87,7 @@ func TestIptablesManager_AddNatRule(t *testing.T) { iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) require.NoError(t, err, "failed to init iptables client") - manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU) + manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport()) require.NoError(t, err, "shouldn't return error") require.NoError(t, manager.init(nil)) @@ -157,7 +160,7 @@ func TestIptablesManager_RemoveNatRule(t *testing.T) { t.Run(testCase.Name, func(t *testing.T) { iptablesClient, _ := iptables.NewWithProtocol(iptables.ProtocolIPv4) - manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU) + manager, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport()) require.NoError(t, err, "shouldn't return error") require.NoError(t, manager.init(nil)) defer func() { @@ -219,7 +222,7 @@ func TestRouter_AddRouteFiltering(t *testing.T) { iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) require.NoError(t, err, "Failed to create iptables client") - r, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU) + r, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport()) require.NoError(t, err, "Failed to create router manager") require.NoError(t, r.init(nil)) @@ -337,27 +340,26 @@ func TestRouter_AddRouteFiltering(t *testing.T) { ruleKey, err := r.AddRouteFiltering(nil, tt.sources, firewall.Network{Prefix: tt.destination}, tt.proto, tt.sPort, tt.dPort, tt.action) require.NoError(t, err, "AddRouteFiltering failed") - // Check if the rule is in the internal map - rule, ok := r.rules[ruleKey.ID()] - assert.True(t, ok, "Rule not found in internal map") + // A kernel without usable ipset splits a multi-source ACL into one + // rule per source, so compare against whichever form is in effect. + useIPSet := r.ipsetSupport.supported() - // Log the internal rule - t.Logf("Internal rule: %v", rule) + // Check if the rules are in the internal map + rules := routeRuleSpecs(t, r, ruleKey.ID()) + require.NotEmpty(t, rules, "Rule not found in internal map") - // Check if the rule exists in iptables - exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, rule...) - assert.NoError(t, err, "Failed to check rule existence") - assert.True(t, exists, "Rule not found in iptables") + // Log the internal rules + t.Logf("Internal rules: %v", rules) - var source firewall.Network - if len(tt.sources) > 1 { - source.Set = firewall.NewPrefixSet(tt.sources) - } else if len(tt.sources) > 0 { - source.Prefix = tt.sources[0] + // Check if the rules exist in iptables + for _, rule := range rules { + exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, rule...) + assert.NoError(t, err, "Failed to check rule existence") + assert.True(t, exists, "Rule not found in iptables") } + // Verify rule content params := routeFilteringRuleParams{ - Source: source, Destination: firewall.Network{Prefix: tt.destination}, Proto: tt.proto, SPort: tt.sPort, @@ -365,20 +367,18 @@ func TestRouter_AddRouteFiltering(t *testing.T) { Action: tt.action, } - expectedRule, err := r.genRouteRuleSpec(params, nil) + expectedRules, err := r.genRouteRuleSpecs(params, tt.sources, useIPSet) require.NoError(t, err, "Failed to generate expected rule spec") - if tt.expectSet { + if tt.expectSet && useIPSet { setName := firewall.NewPrefixSet(tt.sources).HashedName() - expectedRule, err = r.genRouteRuleSpec(params, nil) - require.NoError(t, err, "Failed to generate expected rule spec with set") // Check if the set was created _, exists := r.ipsetCounter.Get(setName) assert.True(t, exists, "IPSet not created") } - assert.Equal(t, expectedRule, rule, "Rule content mismatch") + assert.Equal(t, expectedRules, rules, "Rule content mismatch") // Clean up err = r.DeleteRouteRule(ruleKey) @@ -445,3 +445,104 @@ func TestFindSetNameInRule(t *testing.T) { }) } } + +// TestRouter_AddRouteFilteringIPSetFallback covers a kernel that cannot use ipset: +// a multi-source route ACL must become one rule per source prefix, all present in +// the chain, and deleting the ACL must remove every one of them. Without the +// fallback the rule was never installed and the interface-wide DROP in FORWARD +// silently dropped routed traffic. +func TestRouter_AddRouteFilteringIPSetFallback(t *testing.T) { + if !isIptablesSupported() { + t.Skip("iptables not supported on this system") + } + + iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + support := newIPSetSupport() + support.markUnsupported(errors.New("test: pretend the kernel has no ipset")) + + r, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, support) + require.NoError(t, err) + require.NoError(t, r.init(nil)) + t.Cleanup(func() { + require.NoError(t, r.Reset()) + }) + + sources := []netip.Prefix{ + netip.MustParsePrefix("172.16.0.0/16"), + netip.MustParsePrefix("192.168.0.0/16"), + } + destination := firewall.Network{Prefix: netip.MustParsePrefix("10.0.0.0/8")} + + rule, err := r.AddRouteFiltering(nil, sources, destination, firewall.ProtocolTCP, nil, + &firewall.Port{Values: []uint16{443}}, firewall.ActionAccept) + require.NoError(t, err, "route ACL must install without ipset") + + specs := routeRuleSpecs(t, r, rule.ID()) + require.Len(t, specs, len(sources), "each source prefix needs its own rule") + + for i, spec := range specs { + joined := strings.Join(spec, " ") + require.Contains(t, joined, "-s "+sources[i].String(), "rule must match the source prefix directly") + require.NotContains(t, joined, matchSet, "fallback rule must not reference a set") + + exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, spec...) + require.NoError(t, err) + require.True(t, exists, "rule %d must be present in %s", i, chainRTFWDIN) + } + + require.NoError(t, r.DeleteRouteRule(rule)) + + for i, spec := range specs { + exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, spec...) + require.NoError(t, err) + require.False(t, exists, "rule %d must be removed", i) + } + require.Empty(t, routeRuleSpecs(t, r, rule.ID()), "no rule may be left recorded") +} + +// TestRouter_DestinationSetRequiresIPSet documents that a dynamic (domain) +// destination cannot be expressed without ipset: its prefixes are only known +// after DNS resolution, so there is nothing to expand into per-prefix rules. The +// call must report that rather than install a broader rule than the policy allows. +func TestRouter_DestinationSetRequiresIPSet(t *testing.T) { + if !isIptablesSupported() { + t.Skip("iptables not supported on this system") + } + + iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + support := newIPSetSupport() + support.markUnsupported(errors.New("test: pretend the kernel has no ipset")) + + r, err := newRouter(iptablesClient, ifaceMock, iface.DefaultMTU, support) + require.NoError(t, err) + require.NoError(t, r.init(nil)) + t.Cleanup(func() { + require.NoError(t, r.Reset()) + }) + + destination := firewall.Network{Set: firewall.NewDomainSet(domain.List{"example.com"})} + + _, err = r.AddRouteFiltering(nil, []netip.Prefix{netip.MustParsePrefix("172.16.0.0/16")}, + destination, firewall.ProtocolALL, nil, nil, firewall.ActionAccept) + require.Error(t, err, "a domain destination is not expressible without ipset") + require.ErrorContains(t, err, "requires ipset") +} + +// routeRuleSpecs collects the rules recorded for one route ACL, which is more than +// one when the ipset fallback splits it per source prefix. +func routeRuleSpecs(t *testing.T, r *router, ruleKey string) [][]string { + t.Helper() + + var specs [][]string + for i := 0; ; i++ { + spec, exists := r.rules[routeRuleKey(ruleKey, i)] + if !exists { + return specs + } + specs = append(specs, spec) + } +}