From ffd696d75fc8c4b6941dc0d2dedd437d3da33649 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 12 Aug 2026 17:41:08 +0200 Subject: [PATCH] Preserve pre-existing ipsets and roll back partial route ACL installs --- client/firewall/iptables/acl_linux.go | 10 ++++- client/firewall/iptables/router_linux.go | 20 +++++++++ client/firewall/iptables/router_linux_test.go | 42 +++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/client/firewall/iptables/acl_linux.go b/client/firewall/iptables/acl_linux.go index 124fa2d56..4c56aaf78 100644 --- a/client/firewall/iptables/acl_linux.go +++ b/client/firewall/iptables/acl_linux.go @@ -92,6 +92,10 @@ func (m *aclManager) AddPeerFiltering( return m.addPeerRule(ip, protocol, sPort, dPort, action, "") } + // A set that is already in the store backs rules installed earlier, so it must + // survive this call's failure. + _, preexisting := m.ipsetStore.ipset(ipsetName) + rules, err := m.addPeerRule(ip, protocol, sPort, dPort, action, ipsetName) if err == nil { return rules, nil @@ -102,10 +106,12 @@ func (m *aclManager) AddPeerFiltering( return nil, err } - // The set could not be created or matched. Drop whatever was created and + // The set could not be created or matched. Drop the one this call 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) + if !preexisting { + m.discardIPSet(ipsetName) + } rules, retryErr := m.addPeerRule(ip, protocol, sPort, dPort, action, "") if retryErr != nil { diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index 4c61b85b3..4713e5399 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -174,6 +174,7 @@ func (r *router) AddRouteFiltering( r.removeRouteRules(string(ruleKey)) if retryErr := r.installRouteRules(string(ruleKey), params, sources, false); retryErr != nil { + r.removeRouteRules(string(ruleKey)) return nil, fmt.Errorf("add route rule (ipset: %w): %w", unusable.cause, retryErr) } @@ -182,6 +183,11 @@ func (r *router) AddRouteFiltering( } if err != nil { + // Leave nothing half-installed: a later call finding the rule key would + // report success while some sources were never installed, which for a + // drop rule would leave them unblocked. + r.removeRouteRules(string(ruleKey)) + return nil, fmt.Errorf("add route rule: %w", err) } @@ -221,6 +227,20 @@ func (r *router) genRouteRuleSpecs(params routeFilteringRuleParams, sources []ne return nil, fmt.Errorf("apply network -d: %w", err) } + specs, err := r.genSourceRules(params, sources, useIPSet, destExp) + if err != nil { + // The destination match may have taken a set reference already. + if decErr := r.decrementSetCounter(destExp); decErr != nil { + log.Debugf("release destination set after failed rule generation: %v", decErr) + } + + return nil, err + } + + return specs, nil +} + +func (r *router) genSourceRules(params routeFilteringRuleParams, sources []netip.Prefix, useIPSet bool, destExp []string) ([][]string, error) { if useIPSet || len(sources) <= 1 { sourceExp, err := r.applyNetwork("-s", sourceNetwork(sources), sources) if err != nil { diff --git a/client/firewall/iptables/router_linux_test.go b/client/firewall/iptables/router_linux_test.go index 6d69b1088..d009dd6b7 100644 --- a/client/firewall/iptables/router_linux_test.go +++ b/client/firewall/iptables/router_linux_test.go @@ -17,6 +17,7 @@ import ( firewall "github.com/netbirdio/netbird/client/firewall/manager" "github.com/netbirdio/netbird/client/firewall/test" "github.com/netbirdio/netbird/client/iface" + nbid "github.com/netbirdio/netbird/client/internal/acl/id" nbnet "github.com/netbirdio/netbird/client/net" "github.com/netbirdio/netbird/shared/management/domain" ) @@ -532,6 +533,47 @@ func TestRouter_DestinationSetRequiresIPSet(t *testing.T) { require.ErrorContains(t, err, "requires ipset") } +// TestRouter_RouteFilteringRollsBackPartialInstall covers a fallback ACL whose +// second rule cannot be installed. Nothing may be left behind: if the rule key +// survived, a later call would short-circuit on it and report success while some +// sources were never installed, leaving them unblocked for a drop rule. +func TestRouter_RouteFilteringRollsBackPartialInstall(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()) + }) + + // The v6 prefix is rejected by the v4 iptables binary, so the second rule of + // the expansion fails after the first has been installed. + good := netip.MustParsePrefix("172.16.0.0/16") + sources := []netip.Prefix{good, netip.MustParsePrefix("2001:db8::/32")} + destination := firewall.Network{Prefix: netip.MustParsePrefix("10.0.0.0/8")} + + _, err = r.AddRouteFiltering(nil, sources, destination, firewall.ProtocolALL, nil, nil, firewall.ActionDrop) + require.Error(t, err, "a source that iptables rejects must fail the whole ACL") + + ruleKey := nbid.GenerateRouteRuleKey(sources, destination, firewall.ProtocolALL, nil, nil, firewall.ActionDrop) + require.Empty(t, routeRuleSpecs(t, r, string(ruleKey)), "no rule may stay recorded") + + // The rule that did get installed must be gone from the chain. + installed := []string{"-s", good.String(), "-d", "10.0.0.0/8", "-j", "DROP"} + exists, err := iptablesClient.Exists(tableFilter, chainRTFWDIN, installed...) + require.NoError(t, err) + require.False(t, exists, "the already-installed rule must be rolled back") +} + // 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 {