Retry filter rules per prefix when a set fails and latch ipset off once confirmed

This commit is contained in:
Viktor Liu
2026-08-20 20:08:07 +02:00
parent 4b70772a4a
commit 8f5f9216f5
7 changed files with 322 additions and 77 deletions

View File

@@ -112,10 +112,11 @@ 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
// ipsetSupport is shared by the families of both address families,
// so a kernel without usable ipset support degrades them together.
// When latched off, multi-source rules expand to one rule per
// source prefix.
ipsetSupport *ipsetSupport
// rules holds NAT, jump, and MSS-clamping rules (auxiliary
// plumbing that isn't a filter rule).
@@ -129,7 +130,7 @@ type family struct {
stateManager *statemanager.Manager
}
func newFamily(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint16) (*family, error) {
func newFamily(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint16, ipsetSupport *ipsetSupport) (*family, error) {
r := &family{
iptablesClient: iptablesClient,
wgIface: wgIface,
@@ -140,6 +141,7 @@ func newFamily(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint1
rules: make(routeRules),
mtu: mtu,
ipFwdState: ipfwdstate.NewIPForwardingState(wgIface.Name()),
ipsetSupport: ipsetSupport,
}
r.ipsetCounter = refcounter.New(
@@ -159,8 +161,6 @@ 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)
}

View File

@@ -3,6 +3,7 @@
package iptables
import (
"errors"
"fmt"
"net/netip"
"slices"
@@ -38,7 +39,21 @@ func (r *family) AddFilterRule(
return existing, nil
}
rule, err := r.installFilterRules(ruleID, sources, destination, proto, sPort, dPort, action, r.ipsetSupported)
rule, err := r.installFilterRules(ruleID, sources, destination, proto, sPort, dPort, action, r.ipsetSupport.supported())
var unusable *ipsetUnusableError
if errors.As(err, &unusable) {
// The set could not be created or matched. Retry matching each source
// prefix on its own so the rule lands either way, and latch the
// capability off only once a probe confirms the kernel is the reason.
rule, err = r.installFilterRules(ruleID, sources, destination, proto, sPort, dPort, action, false)
if err != nil {
return nil, fmt.Errorf("add filter rule (ipset: %w): %w", unusable.cause, err)
}
if !r.ipsetUsable(filterChain(destination)) {
r.ipsetSupport.markUnsupported(unusable.cause)
}
}
if err != nil {
return nil, err
}
@@ -194,7 +209,7 @@ func (r *family) applySourceMatch(network firewall.Network, prefixes []netip.Pre
}
name := r.ipsetName(network.Set.HashedName())
if _, err := r.ipsetCounter.Increment(name, prefixes); err != nil {
return nil, fmt.Errorf("ipset increment %s: %w", name, err)
return nil, ipsetUnusable(fmt.Errorf("ipset increment %s: %w", name, err))
}
return []string{"-m", "set", matchSet, name, "src"}, nil
case network.IsPrefix():
@@ -262,11 +277,7 @@ func (r *family) installFilterRule(
}
}
matchSpecs := filterMatchSpecs(proto, sPort, dPort)
chain := chainACLInput
if isRoute {
chain = chainRTFwdIn
}
chain := filterChain(destination)
var installed []filterSpecs
for _, srcMatch := range srcMatches {
@@ -291,7 +302,16 @@ func (r *family) installFilterRule(
// 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)
err = fmt.Errorf("install filter rule on %s: %w", chain, err)
// A failure on a rule whose source carries a set match may mean
// iptables cannot match against sets (xt_set); report it as such
// so the caller can retry the rule in its per-prefix form. A
// destination set cannot be expanded per prefix, so its failures
// are not retryable.
if len(findSets(srcMatch)) > 0 {
err = ipsetUnusable(err)
}
return nil, err
}
// The mangle redirect-mark rule is best effort: the filter rule itself
@@ -317,6 +337,16 @@ func (r *family) installFilterRule(
}, nil
}
// filterChain returns the ACL chain a filter rule belongs in: a rule with a
// destination filters routed traffic, one without filters traffic addressed to
// this peer.
func filterChain(destination firewall.Network) string {
if destination.IsZero() {
return chainACLInput
}
return chainRTFwdIn
}
// 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
@@ -362,7 +392,7 @@ func (r *family) applyNetwork(flag string, network firewall.Network, prefixes []
// source set it cannot be expanded into per-prefix rules. Without
// ipset such a rule is not expressible; report it instead of
// installing something broader than the policy allows.
if flag == "-d" && !r.ipsetSupported {
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())
}

View File

@@ -6,7 +6,6 @@ import (
"fmt"
"net/netip"
"github.com/google/uuid"
"github.com/hashicorp/go-multierror"
"github.com/lrh3321/ipset-go"
log "github.com/sirupsen/logrus"
@@ -15,32 +14,6 @@ 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)

View File

@@ -0,0 +1,105 @@
//go:build !android
package iptables
import (
"sync"
"github.com/google/uuid"
log "github.com/sirupsen/logrus"
)
// ipsetSupport tracks whether ipset-backed firewall rules can be installed.
//
// It starts optimistic and latches to unsupported once the kernel has proven
// 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-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 families 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-prefix 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)
}
// ipsetUsable reports whether the kernel can create a hash:net set and match it
// from an iptables rule in the given chain. It confirms a suspected ipset
// failure before the capability is latched off: a rule can fail for reasons
// that say nothing about the kernel's ipset support (a set name already taken
// by an incompatible type, a transient xtables lock), and latching on one of
// those would drop set matching for the rest of the process lifetime, including
// for the dynamic destination sets that have no per-prefix form.
func (r *family) ipsetUsable(chain string) bool {
// A short unique name so concurrent processes don't collide and we only
// ever destroy the set we created ourselves. ipset names are limited to
// 31 characters.
name := "nb-probe-" + uuid.New().String()[:8]
if err := r.createIPSet(name); err != nil {
log.Debugf("ipset probe: create %s: %v", name, err)
return false
}
defer func() {
if err := r.destroyIPSet(name); err != nil {
log.Debugf("ipset probe: destroy %s: %v", name, err)
}
}()
// Match-only rule with no target: the set is empty, so while it is
// installed it matches nothing and reaches no verdict.
specs := []string{"-m", "set", matchSet, name, "src"}
if err := r.iptablesClient.Insert(tableFilter, chain, 1, specs...); err != nil {
log.Debugf("ipset probe: match a set from %s: %v", chain, err)
return false
}
if err := r.iptablesClient.DeleteIfExists(tableFilter, chain, specs...); err != nil {
log.Errorf("remove ipset probe rule from %s: %v", chain, err)
}
return true
}
// ipsetUnusableError marks a failure attributable to ipset, so the caller can
// retry the same rule in its per-prefix 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}
}

View File

@@ -29,6 +29,10 @@ type Manager struct {
family4 *family
rawSupported bool
// ipsetSupport is shared by both families, so a kernel without
// usable ipset support degrades them together.
ipsetSupport *ipsetSupport
// IPv6 counterparts, nil when no v6 overlay
ipv6Client *iptables.IPTables
family6 *family
@@ -48,11 +52,12 @@ func Create(wgIface iFaceMapper, mtu uint16) (*Manager, error) {
}
m := &Manager{
wgIface: wgIface,
ipv4Client: iptablesClient,
wgIface: wgIface,
ipv4Client: iptablesClient,
ipsetSupport: newIPSetSupport(),
}
m.family4, err = newFamily(iptablesClient, wgIface, mtu)
m.family4, err = newFamily(iptablesClient, wgIface, mtu, m.ipsetSupport)
if err != nil {
return nil, fmt.Errorf("create family: %w", err)
}
@@ -72,7 +77,7 @@ func (m *Manager) createIPv6Components(wgIface iFaceMapper, mtu uint16) error {
return fmt.Errorf("init ip6tables: %w", err)
}
family6, err := newFamily(ip6Client, wgIface, mtu)
family6, err := newFamily(ip6Client, wgIface, mtu, m.ipsetSupport)
if err != nil {
return fmt.Errorf("create v6 family: %w", err)
}

View File

@@ -3,6 +3,7 @@
package iptables
import (
"errors"
"fmt"
"net/netip"
"slices"
@@ -237,6 +238,10 @@ func TestIptablesManagerIPSet(t *testing.T) {
require.NotNil(t, multi, "multi-source rule must produce one iptables rule")
sets := findSets(multi.(*Rule).specs)
require.Len(t, sets, 1, "multi-source rule must reference exactly one ipset")
// Guard the default: a regression that reported ipset as unusable
// would silently move every Linux client to per-prefix rules.
require.True(t, manager.ipsetSupport.supported(),
"ipset must not be latched off on a healthy kernel")
require.NoError(t, manager.DeleteFilterRule(multi))
})
@@ -264,7 +269,7 @@ func TestIptablesFilterIPSetFallback(t *testing.T) {
}()
// Simulate a kernel without the ipset hash module.
manager.family4.ipsetSupported = false
manager.ipsetSupport.markUnsupported(errors.New("test: pretend the kernel has no ipset"))
sources := []netip.Prefix{
netip.MustParsePrefix("10.20.0.42/32"),
@@ -298,30 +303,6 @@ func TestIptablesFilterIPSetFallback(t *testing.T) {
}
}
// TestIptablesFilterDestinationSetRequiresIPSet 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 TestIptablesFilterDestinationSetRequiresIPSet(t *testing.T) {
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
destination := fw.Network{Set: fw.NewDomainSet(domain.List{"example.com"})}
_, err = manager.AddFilterRule(nil, []netip.Prefix{netip.MustParsePrefix("172.16.0.0/16")},
destination, fw.ProtocolALL, nil, nil, fw.ActionAccept)
require.Error(t, err, "a domain destination is not expressible without ipset")
require.ErrorContains(t, err, "requires ipset")
}
// TestIptablesNatRuleDropsSourceSetOnDestinationFailure covers a marking rule
// whose source set is created but whose destination set is not: the source
// reference has to go back, or the set it created stays in the kernel with a
@@ -417,7 +398,7 @@ func TestIptablesRouteFilterIPSetFallback(t *testing.T) {
require.NoError(t, manager.Close(nil))
}()
manager.family4.ipsetSupported = false
manager.ipsetSupport.markUnsupported(errors.New("test: pretend the kernel has no ipset"))
sources := []netip.Prefix{
netip.MustParsePrefix("172.16.0.0/16"),
@@ -449,6 +430,157 @@ func TestIptablesRouteFilterIPSetFallback(t *testing.T) {
}
}
// TestIptablesFilterFallsBackOnSetFailure drives the real failure path: a set
// with the rule's name already exists with an incompatible type, so the kernel
// rejects the hash:net creation. The rule must still land in the chain,
// matching each prefix directly; without the fallback it was dropped and the
// catch-all DROP silently blocked traffic the policy permits. The capability
// must survive, because this kernel does support ipset and a latch would also
// take down the destination sets that have no per-prefix form.
func TestIptablesFilterFallsBackOnSetFailure(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))
}()
sources := []netip.Prefix{
netip.MustParsePrefix("10.20.0.42/32"),
netip.MustParsePrefix("10.20.0.43/32"),
}
// Poison the name the rule's set would get so hash:net creation fails.
poisoned := fw.NewPrefixSet(sources).HashedName()
require.NoError(t, ipset.Create(poisoned, ipset.TypeHashIP, ipset.CreateOptions{}))
t.Cleanup(func() {
if err := ipset.Destroy(poisoned); err != nil {
t.Logf("destroy poisoned set %s: %v", poisoned, err)
}
})
port := &fw.Port{Values: []uint16{22}}
rule, err := manager.AddFilterRule(nil, sources, fw.Network{}, "tcp", nil, port, fw.ActionAccept)
require.NoError(t, err, "AddFilterRule must succeed by falling back")
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 the source prefix")
require.NotContains(t, joined, matchSet)
checkRuleSpecs(t, ipv4Client, rr.chain, true, fs.specs...)
}
require.True(t, manager.ipsetSupport.supported(),
"a failure specific to one set must not latch ipset off on a kernel that supports it")
// A subsequent multi-source rule still gets a set.
next, err := manager.AddFilterRule(nil, []netip.Prefix{
netip.MustParsePrefix("10.20.0.44/32"),
netip.MustParsePrefix("10.20.0.45/32"),
}, fw.Network{}, "tcp", nil, port, fw.ActionAccept)
require.NoError(t, err)
require.Len(t, findSets(next.(*Rule).specs), 1, "later rules must keep using ipset")
require.NoError(t, manager.DeleteFilterRule(next))
}
// TestIptablesIPSetProbe covers the confirmation step that decides whether a
// suspected ipset failure latches the capability off. On a kernel with the
// modules it must report ipset as usable and leave nothing behind: a probe rule
// or set left in place would be untracked.
func TestIptablesIPSetProbe(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))
}()
before, err := ipv4Client.List(tableFilter, chainACLInput)
require.NoError(t, err, "list acl chain")
require.True(t, manager.family4.ipsetUsable(chainACLInput), "ipset must be usable on this kernel")
after, err := ipv4Client.List(tableFilter, chainACLInput)
require.NoError(t, err, "list acl chain")
require.Equal(t, before, after, "the probe must leave the chain as it found it")
sets, err := ipset.ListAll()
require.NoError(t, err, "list sets")
for _, s := range sets {
require.NotContains(t, s.SetName, "nb-probe-", "the probe must destroy its set")
}
}
// TestIptablesFilterDestinationSetRequiresIPSet 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 TestIptablesFilterDestinationSetRequiresIPSet(t *testing.T) {
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.ipsetSupport.markUnsupported(errors.New("test: pretend the kernel has no ipset"))
destination := fw.Network{Set: fw.NewDomainSet(domain.List{"example.com"})}
_, err = manager.AddFilterRule(nil, []netip.Prefix{netip.MustParsePrefix("172.16.0.0/16")},
destination, fw.ProtocolALL, nil, nil, fw.ActionAccept)
require.Error(t, err, "a domain destination is not expressible without ipset")
require.ErrorContains(t, err, "requires ipset")
}
// TestIptablesFilterRollsBackPartialInstall covers a fallback rule whose second
// expanded rule cannot be installed. Nothing may be left behind: if the rule
// were tracked, a later call would short-circuit on it and report success while
// some sources were never installed, and an untracked leftover rule would keep
// matching with no way to remove it.
func TestIptablesFilterRollsBackPartialInstall(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.ipsetSupport.markUnsupported(errors.New("test: pretend the kernel has no ipset"))
// The v6 prefix is rejected by the v4 iptables binary, so the second rule
// of the expansion fails after the first has been installed. Call the
// family directly since the manager dispatches by the first source.
good := netip.MustParsePrefix("172.16.0.0/16")
sources := []netip.Prefix{good, netip.MustParsePrefix("2001:db8::/32")}
destination := fw.Network{Prefix: netip.MustParsePrefix("10.0.0.0/8")}
_, err = manager.family4.AddFilterRule(nil, sources, destination, fw.ProtocolALL, nil, nil, fw.ActionDrop)
require.Error(t, err, "a source that iptables rejects must fail the whole rule")
require.Empty(t, manager.family4.filters, "no rule may stay tracked")
installed := []string{"-s", good.String(), "-d", "10.0.0.0/8", "-j", "DROP"}
checkRuleSpecs(t, ipv4Client, chainRTFwdIn, false, installed...)
}
// TestIptablesCloseRemovesAllState exercises a spread of rule kinds and then
// asserts Close puts every table it touches back exactly as it found it. A
// leaked chain, jump, or ipset survives the daemon and nothing can remove it

View File

@@ -31,7 +31,7 @@ func TestIptablesManager_RestoreOrCreateContainers(t *testing.T) {
iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
require.NoError(t, err, "failed to init iptables client")
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU)
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport())
require.NoError(t, err, "should return a valid iptables manager")
require.NoError(t, manager.init(nil))
@@ -84,7 +84,7 @@ func TestIptablesManager_AddNatRule(t *testing.T) {
iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
require.NoError(t, err, "failed to init iptables client")
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU)
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport())
require.NoError(t, err, "shouldn't return error")
require.NoError(t, manager.init(nil))
@@ -157,7 +157,7 @@ func TestIptablesManager_RemoveNatRule(t *testing.T) {
t.Run(testCase.Name, func(t *testing.T) {
iptablesClient, _ := iptables.NewWithProtocol(iptables.ProtocolIPv4)
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU)
manager, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport())
require.NoError(t, err, "shouldn't return error")
require.NoError(t, manager.init(nil))
defer func() {
@@ -219,7 +219,7 @@ func TestRouter_AddRouteFiltering(t *testing.T) {
iptablesClient, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
require.NoError(t, err, "Failed to create iptables client")
r, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU)
r, err := newFamily(iptablesClient, ifaceMock, iface.DefaultMTU, newIPSetSupport())
require.NoError(t, err, "Failed to create family manager")
require.NoError(t, r.init(nil))