[client] Unify peer and route ACL filtering with multi-source rules (#6322)

* Unify peer and route ACL filtering with multi-source peer rules

* Remove partial userspace firewall mode and open foreign chains via a table-less allower

* Snapshot iptables rule maps before persisting state

* Scope userspace firewall wildcard source rules per address family

* Install nftables peer filter and mangle rules in a single transaction

* Share the iptables jump rule spec between install and cleanup

* Fix legacy ACL source wildcard and keep rollback tracking on delete failure

* Fix CI: recognize multi-value port set lookups in tests and correct PeerIP lint suppression

* Fall back to per-prefix filter rules when ipset is unavailable

* Annotate legacy PeerIP usages in ACL tests and fix import formatting

* Keep firewall rule bookkeeping in step with the kernel on replace and teardown

* Release the routing reference when the route manager shuts down

* Keep set references and rule tracking consistent when a routing rule fails

client/internal/acl/dispatch_test.go

@@ -0,0 +1,190 @@
+package acl
+
+import (
+	"net/netip"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	"github.com/netbirdio/netbird/client/firewall"
+	fwmgr "github.com/netbirdio/netbird/client/firewall/manager"
+	"github.com/netbirdio/netbird/client/iface"
+	"github.com/netbirdio/netbird/client/iface/wgaddr"
+	"github.com/netbirdio/netbird/client/internal/acl/mocks"
+	mgmProto "github.com/netbirdio/netbird/shared/management/proto"
+)
+
+// TestNetworkZeroPrefixIsRoute guards the route-vs-peer dispatch
+// invariant: the backends classify a rule as a peer rule purely by the
+// absence of a destination (neither prefix nor set). A default route
+// (0.0.0.0/0 or ::/0) is a valid prefix and must therefore classify as
+// a route, not collapse into the peer path.
+func TestNetworkZeroPrefixIsRoute(t *testing.T) {
+	for _, p := range []string{"0.0.0.0/0", "::/0", "10.0.0.0/8"} {
+		n := fwmgr.Network{Prefix: netip.MustParsePrefix(p)}
+		assert.True(t, n.IsPrefix(), "%s must report IsPrefix", p)
+		assert.True(t, n.IsPrefix() || n.IsSet(), "%s must classify as a route", p)
+	}
+
+	// A zero-value Network is the only peer-rule shape.
+	var empty fwmgr.Network
+	assert.False(t, empty.IsPrefix(), "zero Network must not be a prefix")
+	assert.False(t, empty.IsSet(), "zero Network must not be a set")
+}
+
+// TestDetermineDestinationAlwaysRoute verifies determineDestination
+// never yields an empty Network for a valid route rule: every branch
+// (static prefix, default route, dynamic with/without domains, with and
+// without a local resolver) produces a destination that classifies as a
+// route. If this regresses, a route rule would be dispatched down the
+// peer path, which matches on source only.
+func TestDetermineDestinationAlwaysRoute(t *testing.T) {
+	v4 := []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}
+	v6 := []netip.Prefix{netip.MustParsePrefix("2001:db8::/48")}
+
+	cases := []struct {
+		name     string
+		rule     *mgmProto.RouteFirewallRule
+		resolver bool
+		sources  []netip.Prefix
+	}{
+		{"static prefix", &mgmProto.RouteFirewallRule{Destination: "192.168.0.0/16"}, false, v4},
+		{"static default route", &mgmProto.RouteFirewallRule{Destination: "0.0.0.0/0"}, false, v4},
+		{"dynamic with domains + resolver", &mgmProto.RouteFirewallRule{IsDynamic: true, Domains: []string{"example.com"}}, true, v4},
+		{"dynamic no domains + resolver (v4)", &mgmProto.RouteFirewallRule{IsDynamic: true}, true, v4},
+		{"dynamic no domains + resolver (v6)", &mgmProto.RouteFirewallRule{IsDynamic: true}, true, v6},
+		{"dynamic + no local resolver (v4)", &mgmProto.RouteFirewallRule{IsDynamic: true}, false, v4},
+		{"dynamic + no local resolver (v6)", &mgmProto.RouteFirewallRule{IsDynamic: true}, false, v6},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			dest, err := determineDestination(tc.rule, tc.resolver, tc.sources)
+			require.NoError(t, err)
+			assert.True(t, dest.IsPrefix() || dest.IsSet(),
+				"destination must classify as a route, got empty Network")
+		})
+	}
+}
+
+// countingFirewall wraps a real firewall.Manager and counts filter-rule
+// add/delete calls so a test can assert how many backing rules the acl
+// manager actually creates and tears down.
+type countingFirewall struct {
+	fwmgr.Manager
+	mu       sync.Mutex
+	addCalls int
+	dels     int
+	ruleIDs  map[fwmgr.RuleID]struct{}
+}
+
+// distinctRules returns the number of distinct backing rules the
+// backend produced. Because the backend dedups identical content,
+// repeated AddFilterRule calls for the same rule resolve to one id.
+func (f *countingFirewall) distinctRules() int {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return len(f.ruleIDs)
+}
+
+func (f *countingFirewall) AddFilterRule(id []byte, sources []netip.Prefix, destination fwmgr.Network, proto fwmgr.Protocol, sPort, dPort *fwmgr.Port, action fwmgr.Action) (fwmgr.Rule, error) {
+	rule, err := f.Manager.AddFilterRule(id, sources, destination, proto, sPort, dPort, action)
+	if err == nil {
+		f.mu.Lock()
+		f.addCalls++
+		if f.ruleIDs == nil {
+			f.ruleIDs = make(map[fwmgr.RuleID]struct{})
+		}
+		if rule != nil {
+			f.ruleIDs[rule.ID()] = struct{}{}
+		}
+		f.mu.Unlock()
+	}
+	return rule, err
+}
+
+func (f *countingFirewall) DeleteFilterRule(r fwmgr.Rule) error {
+	err := f.Manager.DeleteFilterRule(r)
+	if err == nil {
+		f.mu.Lock()
+		f.dels++
+		delete(f.ruleIDs, r.ID())
+		f.mu.Unlock()
+	}
+	return err
+}
+
+func newCountingACL(t *testing.T) (*DefaultManager, *countingFirewall, func()) {
+	t.Helper()
+	t.Setenv("NB_WG_KERNEL_DISABLED", "true")
+	t.Setenv(firewall.EnvForceUserspaceFirewall, "true")
+
+	ctrl := gomock.NewController(t)
+	ifaceMock := mocks.NewMockIFaceMapper(ctrl)
+	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
+	ifaceMock.EXPECT().SetFilter(gomock.Any())
+	network := netip.MustParsePrefix("172.0.0.1/32")
+	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
+	ifaceMock.EXPECT().Address().Return(wgaddr.Address{IP: network.Addr(), Network: network}).AnyTimes()
+	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
+
+	realFW, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+
+	fw := &countingFirewall{Manager: realFW}
+	cleanup := func() {
+		require.NoError(t, realFW.Close(nil))
+		ctrl.Finish()
+	}
+	return NewDefaultManager(fw), fw, cleanup
+}
+
+// TestDuplicateContentPoliciesShareOneRule verifies the dedup contract
+// the backends rely on: two policies that authorize an identical flow
+// (same selector and sources) collapse to a single backing firewall
+// rule, and that rule survives until BOTH policies are gone. This is
+// why the backend can dedup on add without refcounting on delete: the
+// acl manager's pair key matches the backend's content key, so add and
+// delete stay balanced per content key across full-state reapplies.
+func TestDuplicateContentPoliciesShareOneRule(t *testing.T) {
+	acl, fw, cleanup := newCountingACL(t)
+	defer cleanup()
+
+	ruleA := &mgmProto.FirewallRule{
+		PolicyID:  []byte("policy-A"),
+		PeerIP:    "10.0.0.1", //nolint:staticcheck
+		Direction: mgmProto.RuleDirection_IN,
+		Action:    mgmProto.RuleAction_ACCEPT,
+		Protocol:  mgmProto.RuleProtocol_TCP,
+		Port:      "443",
+	}
+	ruleB := &mgmProto.FirewallRule{
+		PolicyID:  []byte("policy-B"),
+		PeerIP:    "10.0.0.1", //nolint:staticcheck
+		Direction: mgmProto.RuleDirection_IN,
+		Action:    mgmProto.RuleAction_ACCEPT,
+		Protocol:  mgmProto.RuleProtocol_TCP,
+		Port:      "443",
+	}
+
+	// Both policies present: identical content collapses to one rule.
+	acl.ApplyFiltering(&mgmProto.NetworkMap{FirewallRules: []*mgmProto.FirewallRule{ruleA, ruleB}, FirewallRulesIsEmpty: false}, false)
+	assert.Equal(t, 1, fw.distinctRules(), "identical-content policies must produce one backing rule")
+	assert.Equal(t, 1, len(acl.peerRulesPairs), "one content key, one pair")
+
+	// Drop policy A only: the shared rule is still authorized by B, so
+	// nothing is deleted.
+	acl.ApplyFiltering(&mgmProto.NetworkMap{FirewallRules: []*mgmProto.FirewallRule{ruleB}, FirewallRulesIsEmpty: false}, false)
+	assert.Equal(t, 1, fw.distinctRules(), "no new backing rule on reapply")
+	assert.Equal(t, 0, fw.dels, "rule must survive while any policy still authorizes it")
+	assert.Equal(t, 1, len(acl.peerRulesPairs))
+
+	// Drop policy B too: now the content key has no authorizer and the
+	// single backing rule is removed exactly once.
+	acl.ApplyFiltering(&mgmProto.NetworkMap{FirewallRules: nil, FirewallRulesIsEmpty: true}, false)
+	assert.Equal(t, 1, fw.dels, "rule removed once when last policy is gone")
+	assert.Equal(t, 0, len(acl.peerRulesPairs))
+}

client/internal/acl/grouping_test.go

@@ -0,0 +1,318 @@
+package acl
+
+import (
+	"errors"
+	"net/netip"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	"github.com/netbirdio/netbird/client/firewall"
+	fwmgr "github.com/netbirdio/netbird/client/firewall/manager"
+	"github.com/netbirdio/netbird/client/iface"
+	"github.com/netbirdio/netbird/client/iface/wgaddr"
+	"github.com/netbirdio/netbird/client/internal/acl/mocks"
+	mgmProto "github.com/netbirdio/netbird/shared/management/proto"
+	"github.com/netbirdio/netbird/shared/netiputil"
+)
+
+// TestGroupPeerRulesPolicyIDSeparates verifies that two FirewallRules
+// with identical selectors but different PolicyIDs do NOT get merged
+// into one group, so each policy's sources merge under its own
+// attribution id. (Identical-content groups may still dedup to one
+// backing rule at the backend; see TestDuplicateContentPoliciesShareOneRule.)
+func TestGroupPeerRulesPolicyIDSeparates(t *testing.T) {
+	rules := []*mgmProto.FirewallRule{
+		{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    "10.0.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+		{
+			PolicyID:  []byte("policy-B"),
+			PeerIP:    "10.0.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+	}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2, "rules with different PolicyIDs must produce separate groups")
+}
+
+// TestGroupPeerRulesFamilySeparates verifies that v4 and v6 rules
+// belonging to the same policy don't merge.
+func TestGroupPeerRulesFamilySeparates(t *testing.T) {
+	rules := []*mgmProto.FirewallRule{
+		{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    "10.0.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+		{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    "2001:db8::1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+	}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2, "rules of different families must produce separate groups")
+
+	var sawV4, sawV6 bool
+	for _, g := range groups {
+		require.Len(t, g.sources, 1)
+		if g.sources[0].Addr().Is4() {
+			sawV4 = true
+		}
+		if g.sources[0].Addr().Is6() {
+			sawV6 = true
+		}
+	}
+	assert.True(t, sawV4 && sawV6)
+}
+
+// TestGroupPeerRulesSplitsMixedFamilySingleRule verifies that a single
+// FirewallRule carrying both v4 and v6 source prefixes is split into one
+// group per family. Each backend keys a rule to a single family, so a
+// group whose sources span families would mismatch the other family's
+// sources. mgmt normally emits one rule per family; this guards against
+// a mixed-family rule slipping through.
+func TestGroupPeerRulesSplitsMixedFamilySingleRule(t *testing.T) {
+	srcs := [][]byte{
+		netiputil.EncodeAddr(netip.MustParseAddr("10.0.0.1")),
+		netiputil.EncodeAddr(netip.MustParseAddr("2001:db8::1")),
+		netiputil.EncodeAddr(netip.MustParseAddr("10.0.0.2")),
+		netiputil.EncodeAddr(netip.MustParseAddr("2001:db8::2")),
+	}
+	rules := []*mgmProto.FirewallRule{
+		{
+			PolicyID:       []byte("policy-A"),
+			SourcePrefixes: srcs,
+			Direction:      mgmProto.RuleDirection_IN,
+			Action:         mgmProto.RuleAction_ACCEPT,
+			Protocol:       mgmProto.RuleProtocol_TCP,
+			Port:           "443",
+		},
+	}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2, "mixed-family sources in one rule must split into two groups")
+
+	for _, g := range groups {
+		require.Len(t, g.sources, 2)
+		v6 := prefixIsV6(g.sources[0])
+		for _, s := range g.sources {
+			assert.Equal(t, v6, prefixIsV6(s), "every source in a group must share one family")
+		}
+	}
+}
+
+// TestGroupPeerRulesMergesSameSelector verifies that rules sharing
+// every distinguishing field (policy, family, direction, action,
+// proto, port) collapse into a single multi-source group.
+func TestGroupPeerRulesMergesSameSelector(t *testing.T) {
+	mk := func(peerIP string) *mgmProto.FirewallRule {
+		return &mgmProto.FirewallRule{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    peerIP, //nolint:staticcheck
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		}
+	}
+	rules := []*mgmProto.FirewallRule{mk("10.0.0.1"), mk("10.0.0.2"), mk("10.0.0.3")}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 1)
+	require.Len(t, groups[0].sources, 3)
+}
+
+// TestGroupPeerRulesPortSeparates verifies that PortInfo is part of the
+// selector key: rules differing only in port must not merge, and a
+// single port must not merge with a range. A regression dropping the
+// port from the key would collapse rules for different ports into one.
+func TestGroupPeerRulesPortSeparates(t *testing.T) {
+	mkPort := func(peerIP string, port uint32) *mgmProto.FirewallRule {
+		return &mgmProto.FirewallRule{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    peerIP, //nolint:staticcheck
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			PortInfo:  &mgmProto.PortInfo{PortSelection: &mgmProto.PortInfo_Port{Port: port}},
+		}
+	}
+
+	groups, denyErr, err := groupPeerRules([]*mgmProto.FirewallRule{
+		mkPort("10.0.0.1", 80), mkPort("10.0.0.2", 80), mkPort("10.0.0.3", 443),
+	})
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2, "rules on different ports must not merge")
+
+	rangeRule := &mgmProto.FirewallRule{
+		PolicyID:  []byte("policy-A"),
+		PeerIP:    "10.0.0.4", //nolint:staticcheck
+		Direction: mgmProto.RuleDirection_IN,
+		Action:    mgmProto.RuleAction_ACCEPT,
+		Protocol:  mgmProto.RuleProtocol_TCP,
+		PortInfo:  &mgmProto.PortInfo{PortSelection: &mgmProto.PortInfo_Range_{Range: &mgmProto.PortInfo_Range{Start: 80, End: 90}}},
+	}
+	groups, denyErr, err = groupPeerRules([]*mgmProto.FirewallRule{mkPort("10.0.0.1", 80), rangeRule})
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2, "a single port and a range must not merge")
+}
+
+// TestGroupPeerRulesUsesSourcePrefixesWhenPresent verifies that the
+// new sourcePrefixes wire field is consumed and produces a
+// multi-source group in one shot (no client-side merging needed).
+func TestGroupPeerRulesUsesSourcePrefixesWhenPresent(t *testing.T) {
+	srcs := [][]byte{
+		netiputil.EncodeAddr(netip.MustParseAddr("10.0.0.1")),
+		netiputil.EncodeAddr(netip.MustParseAddr("10.0.0.2")),
+		netiputil.EncodeAddr(netip.MustParseAddr("10.0.0.3")),
+	}
+	rules := []*mgmProto.FirewallRule{
+		{
+			PolicyID:       []byte("policy-A"),
+			SourcePrefixes: srcs,
+			Direction:      mgmProto.RuleDirection_IN,
+			Action:         mgmProto.RuleAction_ACCEPT,
+			Protocol:       mgmProto.RuleProtocol_TCP,
+			Port:           "443",
+		},
+	}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 1)
+	require.Len(t, groups[0].sources, 3)
+}
+
+// TestGroupPeerRulesActionSeparates verifies the obvious: accept
+// and drop rules with the same selector don't merge.
+func TestGroupPeerRulesActionSeparates(t *testing.T) {
+	rules := []*mgmProto.FirewallRule{
+		{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    "10.0.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_ACCEPT,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+		{
+			PolicyID:  []byte("policy-A"),
+			PeerIP:    "10.0.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_DROP,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "443",
+		},
+	}
+
+	groups, denyErr, err := groupPeerRules(rules)
+	require.NoError(t, denyErr)
+	require.NoError(t, err)
+	require.Len(t, groups, 2)
+}
+
+// failingDeleteFirewall wraps a real firewall.Manager and forces the
+// next N DeleteFilterRule calls to fail. Used to verify that the acl
+// manager retains rules whose deletion was rejected by the backend,
+// so they get retried on the next ApplyFiltering pass instead of
+// becoming orphans.
+type failingDeleteFirewall struct {
+	fwmgr.Manager
+	failCount int
+}
+
+func (f *failingDeleteFirewall) DeleteFilterRule(r fwmgr.Rule) error {
+	if f.failCount > 0 {
+		f.failCount--
+		return errors.New("simulated delete failure")
+	}
+	return f.Manager.DeleteFilterRule(r)
+}
+
+// TestApplyFilteringRetainsRulesOnDeleteFailure verifies that a
+// transient DeleteFilterRule error doesn't make the acl manager
+// forget about a rule. The rule must remain in peerRulesPairs so the
+// next ApplyFiltering pass attempts the delete again.
+func TestApplyFilteringRetainsRulesOnDeleteFailure(t *testing.T) {
+	t.Setenv("NB_WG_KERNEL_DISABLED", "true")
+	t.Setenv(firewall.EnvForceUserspaceFirewall, "true")
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	ifaceMock := mocks.NewMockIFaceMapper(ctrl)
+	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
+	ifaceMock.EXPECT().SetFilter(gomock.Any())
+	network := netip.MustParsePrefix("172.0.0.1/32")
+	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
+	ifaceMock.EXPECT().Address().Return(wgaddr.Address{IP: network.Addr(), Network: network}).AnyTimes()
+	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
+
+	realFW, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() { require.NoError(t, realFW.Close(nil)) }()
+
+	fw := &failingDeleteFirewall{Manager: realFW}
+	acl := NewDefaultManager(fw)
+
+	// First pass: install a rule.
+	netmap1 := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PolicyID:  []byte("policy-A"),
+				PeerIP:    "10.0.0.1",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_DROP,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "22",
+			},
+		},
+		FirewallRulesIsEmpty: false,
+	}
+	acl.ApplyFiltering(netmap1, false)
+	require.Equal(t, 1, len(acl.peerRulesPairs), "rule should be installed")
+
+	// Second pass: remove the rule from the map. The backend will
+	// fail the delete; the acl manager must retain the rule.
+	fw.failCount = 1
+	netmap2 := &mgmProto.NetworkMap{FirewallRules: nil, FirewallRulesIsEmpty: true}
+	acl.ApplyFiltering(netmap2, false)
+	require.Equal(t, 1, len(acl.peerRulesPairs),
+		"rule must be retained when DeleteFilterRule fails so it gets retried")
+
+	// Third pass: same map, backend no longer fails. The rule
+	// should now succeed in being removed.
+	acl.ApplyFiltering(netmap2, false)
+	require.Equal(t, 0, len(acl.peerRulesPairs), "retry should succeed")
+}

client/internal/acl/id/id.go

@@ -5,18 +5,18 @@ import (
 	"encoding/hex"
 	"fmt"
 	"net/netip"
+	"slices"
 	"strconv"
 
 	"github.com/netbirdio/netbird/client/firewall/manager"
 )
 
-type RuleID string
+// RuleID aliases manager.RuleID so existing nbid.RuleID references
+// keep working while the canonical type lives in the firewall package.
+type RuleID = manager.RuleID
 
-func (r RuleID) ID() string {
-	return string(r)
-}
-
-func GenerateRouteRuleKey(
+// GenerateRuleID returns a deterministic content hash identifying a filter rule.
+func GenerateRuleID(
 	sources []netip.Prefix,
 	destination manager.Network,
 	proto manager.Protocol,
@@ -24,6 +24,7 @@ func GenerateRouteRuleKey(
 	dPort *manager.Port,
 	action manager.Action,
 ) RuleID {
+	sources = slices.Clone(sources)
 	manager.SortPrefixes(sources)
 
 	h := sha256.New()

client/internal/acl/legacy_fallback_test.go

@@ -0,0 +1,75 @@
+package acl
+
+import (
+	"net/netip"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	"github.com/netbirdio/netbird/client/firewall"
+	fwmgr "github.com/netbirdio/netbird/client/firewall/manager"
+	"github.com/netbirdio/netbird/client/iface"
+	"github.com/netbirdio/netbird/client/iface/wgaddr"
+	"github.com/netbirdio/netbird/client/internal/acl/mocks"
+	mgmProto "github.com/netbirdio/netbird/shared/management/proto"
+)
+
+// sourcesRecordingFirewall wraps a real firewall.Manager and records
+// the source prefixes of every AddFilterRule call.
+type sourcesRecordingFirewall struct {
+	fwmgr.Manager
+	mu      sync.Mutex
+	sources [][]netip.Prefix
+}
+
+func (f *sourcesRecordingFirewall) AddFilterRule(id []byte, sources []netip.Prefix, destination fwmgr.Network, proto fwmgr.Protocol, sPort, dPort *fwmgr.Port, action fwmgr.Action) (fwmgr.Rule, error) {
+	f.mu.Lock()
+	f.sources = append(f.sources, sources)
+	f.mu.Unlock()
+	return f.Manager.AddFilterRule(id, sources, destination, proto, sPort, dPort, action)
+}
+
+// TestLegacyManagementFallbackUsesMatchAnySources verifies the
+// allow-all fallback for old management servers (empty FirewallRules
+// without the FirewallRulesIsEmpty flag) reaches the firewall as /0
+// match-any sources. The fallback rule carries PeerIP 0.0.0.0; if that
+// were converted to a host prefix (0.0.0.0/32) it would match nothing
+// and all peer traffic would be dropped.
+func TestLegacyManagementFallbackUsesMatchAnySources(t *testing.T) {
+	t.Setenv("NB_WG_KERNEL_DISABLED", "true")
+	t.Setenv(firewall.EnvForceUserspaceFirewall, "true")
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	ifaceMock := mocks.NewMockIFaceMapper(ctrl)
+	ifaceMock.EXPECT().IsUserspaceBind().Return(true).AnyTimes()
+	ifaceMock.EXPECT().SetFilter(gomock.Any())
+	network := netip.MustParsePrefix("172.0.0.1/32")
+	ifaceMock.EXPECT().Name().Return("lo").AnyTimes()
+	ifaceMock.EXPECT().Address().Return(wgaddr.Address{IP: network.Addr(), Network: network}).AnyTimes()
+	ifaceMock.EXPECT().GetWGDevice().Return(nil).AnyTimes()
+
+	realFW, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() { require.NoError(t, realFW.Close(nil)) }()
+
+	fw := &sourcesRecordingFirewall{Manager: realFW}
+	acl := NewDefaultManager(fw)
+
+	// Old management: no rules and no FirewallRulesIsEmpty flag.
+	acl.ApplyFiltering(&mgmProto.NetworkMap{FirewallRules: nil, FirewallRulesIsEmpty: false}, false)
+
+	fw.mu.Lock()
+	defer fw.mu.Unlock()
+	require.NotEmpty(t, fw.sources, "legacy fallback must install at least one allow-all rule")
+	for _, sources := range fw.sources {
+		require.NotEmpty(t, sources)
+		for _, p := range sources {
+			assert.Equal(t, 0, p.Bits(), "legacy fallback source %s must be a /0 match-any prefix", p)
+		}
+	}
+}

client/internal/acl/manager.go

@@ -1,8 +1,6 @@
 package acl
 
 import (
-	"crypto/md5"
-	"encoding/hex"
 	"errors"
 	"fmt"
 	"net/netip"
@@ -24,6 +22,10 @@ import (
 
 var ErrSourceRangesEmpty = errors.New("sources range is empty")
 
+// ErrNoRuleReturned is returned when the firewall backend reports success
+// from AddFilterRule but yields no rule to track.
+var ErrNoRuleReturned = errors.New("backend returned no rule")
+
 // Manager is a ACL rules manager
 type Manager interface {
 	ApplyFiltering(networkMap *mgmProto.NetworkMap, dnsRouteFeatureFlag bool)
@@ -32,19 +34,48 @@ type Manager interface {
 // DefaultManager uses firewall manager to handle
 type DefaultManager struct {
 	firewall           firewall.Manager
-	ipsetCounter       int
 	peerRulesPairs     map[id.RuleID][]firewall.Rule
-	routeRules         map[id.RuleID]struct{}
+	routeRules         map[id.RuleID]firewall.Rule
 	previousConfigHash uint64
 	hasAppliedConfig   bool
 	mutex              sync.Mutex
 }
 
+// peerRuleGroup collapses a set of single-source FirewallRules sharing
+// the same selector into one multi-source rule to push to the backend.
+type peerRuleGroup struct {
+	direction mgmProto.RuleDirection
+	action    mgmProto.RuleAction
+	protocol  mgmProto.RuleProtocol
+	port      *mgmProto.PortInfo
+	// legacyPort is used only when PortInfo is empty (old management).
+	legacyPort string
+	policyID   []byte
+	sources    []netip.Prefix
+}
+
+// peerRuleKey is the comparable selector that decides which single-source
+// rules merge into one group. Rules with an equal key collapse into one
+// multi-source backend rule. PortInfo is flattened into its scalar fields
+// so the key compares by value; policyID keeps policies separate so two
+// policies authorizing different peers don't merge under one attribution.
+type peerRuleKey struct {
+	v6         bool
+	policyID   string
+	direction  mgmProto.RuleDirection
+	action     mgmProto.RuleAction
+	protocol   mgmProto.RuleProtocol
+	legacyPort string
+	port       uint16
+	rangeStart uint16
+	rangeEnd   uint16
+}
+
 func NewDefaultManager(fm firewall.Manager) *DefaultManager {
 	return &DefaultManager{
 		firewall:       fm,
 		peerRulesPairs: make(map[id.RuleID][]firewall.Rule),
-		routeRules:     make(map[id.RuleID]struct{}),
+		routeRules:     make(map[id.RuleID]firewall.Rule),
 	}
 }
 
@@ -88,11 +119,14 @@ func (d *DefaultManager) ApplyFiltering(networkMap *mgmProto.NetworkMap, dnsRout
 			time.Since(start), total)
 	}()
 
-	d.applyPeerACLs(networkMap)
+	peerErr := d.applyPeerACLs(networkMap)
+	if peerErr != nil {
+		log.Errorf("apply peer ACLs: %v", peerErr)
+	}
 
 	routeErr := d.applyRouteACLs(networkMap.RoutesFirewallRules, dnsRouteFeatureFlag)
 	if routeErr != nil {
-		log.Errorf("Failed to apply route ACLs: %v", routeErr)
+		log.Errorf("apply route ACLs: %v", routeErr)
 	}
 
 	flushErr := d.firewall.Flush()
@@ -104,7 +138,7 @@ func (d *DefaultManager) ApplyFiltering(networkMap *mgmProto.NetworkMap, dnsRout
 	// If applying or flushing failed, leave the previous hash untouched so the
 	// next (possibly identical) update is not skipped and gets a chance to
 	// reconcile the firewall state.
-	if err == nil && routeErr == nil && flushErr == nil {
+	if err == nil && peerErr == nil && routeErr == nil && flushErr == nil {
 		d.previousConfigHash = hash
 		d.hasAppliedConfig = true
 	} else {
@@ -135,7 +169,7 @@ func (d *DefaultManager) firewallConfigHash(networkMap *mgmProto.NetworkMap, dns
 	})
 }
 
-func (d *DefaultManager) applyPeerACLs(networkMap *mgmProto.NetworkMap) {
+func (d *DefaultManager) applyPeerACLs(networkMap *mgmProto.NetworkMap) error {
 	rules := networkMap.FirewallRules
 
 	// if we got empty rules list but management not set networkMap.FirewallRulesIsEmpty flag
@@ -158,59 +192,167 @@ func (d *DefaultManager) applyPeerACLs(networkMap *mgmProto.NetworkMap) {
 		)
 	}
 
-	newRulePairs := make(map[id.RuleID][]firewall.Rule)
-	ipsetByRuleSelectors := make(map[string]string)
+	// Group incoming single-source rules from management by their
+	// (direction, action, proto, port) selector and merge sources.
+	// One call to the firewall backend per merged rule.
+	// A deny we cannot decode would leave its traffic unblocked, so skip
+	// the whole pass and keep existing rules until the next sync.
+	groups, denyErr, err := groupPeerRules(rules)
+	if denyErr != nil {
+		return fmt.Errorf("decode deny rule sources: %w", denyErr)
+	}
 
-	// TODO: deny rules should be fatal: if a deny rule fails to apply, we must
-	// roll back all allow rules to avoid a fail-open where allowed traffic bypasses
-	// the missing deny. Currently we accumulate errors and continue.
+	newRulePairs := make(map[id.RuleID][]firewall.Rule)
 	var merr *multierror.Error
-	for _, r := range rules {
-		// if this rule is member of rule selection with more than DefaultIPsCountForSet
-		// it's IP address can be used in the ipset for firewall manager which supports it
-		selector := d.getRuleGroupingSelector(r)
-		ipsetName, ok := ipsetByRuleSelectors[selector]
-		if !ok {
-			d.ipsetCounter++
-			ipsetName = fmt.Sprintf("nb%07d", d.ipsetCounter)
-			ipsetByRuleSelectors[selector] = ipsetName
-		}
-		pairID, rulePair, err := d.protoRuleToFirewallRule(r, ipsetName)
-		if err != nil {
-			merr = multierror.Append(merr, fmt.Errorf("apply firewall rule: %w", err))
+	if err != nil {
+		merr = multierror.Append(merr, err)
+	}
+
+	// Apply denies first. A deny that fails to install is a security
+	// failure (fail-open), so if any deny errors we roll back the
+	// denies we already installed in this pass and bail out without
+	// installing any accept. Pre-existing rules stay untouched until
+	// the next successful pass clears them.
+	denies, accepts := splitDenyAccept(groups)
+	if err := d.installPeerGroups(denies, newRulePairs, true); err != nil {
+		return fmt.Errorf("install deny rules: %w", err)
+	}
+
+	if err := d.installPeerGroups(accepts, newRulePairs, false); err != nil {
+		merr = multierror.Append(merr, err)
+	}
+
+	// Tear down rules that disappeared from the networkmap. Any rule
+	// the backend refuses to delete stays in our tracking so it gets
+	// retried on the next ApplyFiltering. Otherwise a transient
+	// delete failure would leak the rule in the firewall until the
+	// process exits.
+	for pairID, rules := range d.peerRulesPairs {
+		if _, ok := newRulePairs[pairID]; ok {
 			continue
 		}
-		if len(rulePair) > 0 {
-			d.peerRulesPairs[pairID] = rulePair
-			newRulePairs[pairID] = rulePair
-		}
-	}
-
-	if merr != nil {
-		log.Errorf("failed to apply %d peer ACL rule(s): %v", merr.Len(), nberrors.FormatErrorOrNil(merr))
-	}
-
-	for pairID, rules := range d.peerRulesPairs {
-		if _, ok := newRulePairs[pairID]; !ok {
-			for _, rule := range rules {
-				if err := d.firewall.DeletePeerRule(rule); err != nil {
-					log.Errorf("failed to delete peer firewall rule: %v", err)
-					continue
-				}
+		var remaining []firewall.Rule
+		for _, rule := range rules {
+			if err := d.firewall.DeleteFilterRule(rule); err != nil {
+				merr = multierror.Append(merr, fmt.Errorf("delete peer rule, will retry: %w", err))
+				remaining = append(remaining, rule)
 			}
-			delete(d.peerRulesPairs, pairID)
+		}
+		if len(remaining) > 0 {
+			newRulePairs[pairID] = remaining
 		}
 	}
 	d.peerRulesPairs = newRulePairs
+
+	return nberrors.FormatErrorOrNil(merr)
+}
+
+// installPeerGroups applies each group and records the resulting rule
+// pairs in newRulePairs. With atomic set (deny rules), a single failure
+// rolls back every rule installed in this call and returns, leaving the
+// firewall exactly as before: denies are fail-closed and must be applied
+// all-or-nothing. With atomic unset (accept rules), failures are
+// accumulated and the remaining groups still install, so one malformed
+// allow cannot drop every other legitimate allow in the pass.
+func (d *DefaultManager) installPeerGroups(groups []*peerRuleGroup, newRulePairs map[id.RuleID][]firewall.Rule, atomic bool) error {
+	var freshlyInstalled []id.RuleID
+	var merr *multierror.Error
+	for _, g := range groups {
+		pairID, rulePair, err := d.applyPeerGroup(g)
+		if err != nil {
+			if atomic {
+				d.rollbackInstalled(freshlyInstalled)
+				return fmt.Errorf("apply firewall rule: %w", err)
+			}
+			merr = multierror.Append(merr, fmt.Errorf("apply firewall rule: %w", err))
+			continue
+		}
+		if len(rulePair) == 0 {
+			continue
+		}
+		if _, existed := d.peerRulesPairs[pairID]; !existed {
+			freshlyInstalled = append(freshlyInstalled, pairID)
+		}
+		d.peerRulesPairs[pairID] = rulePair
+		newRulePairs[pairID] = rulePair
+	}
+	return nberrors.FormatErrorOrNil(merr)
+}
+
+func (d *DefaultManager) rollbackInstalled(pairIDs []id.RuleID) {
+	var merr *multierror.Error
+	for _, pairID := range pairIDs {
+		// Keep any rule the backend refuses to delete tracked so it is
+		// retried on the next ApplyFiltering instead of leaking in the
+		// firewall with no tracking left to remove it.
+		var remaining []firewall.Rule
+		for _, rule := range d.peerRulesPairs[pairID] {
+			if err := d.firewall.DeleteFilterRule(rule); err != nil {
+				merr = multierror.Append(merr, fmt.Errorf("rule %s: %w", pairID, err))
+				remaining = append(remaining, rule)
+			}
+		}
+		if len(remaining) > 0 {
+			d.peerRulesPairs[pairID] = remaining
+		} else {
+			delete(d.peerRulesPairs, pairID)
+		}
+	}
+	if err := nberrors.FormatErrorOrNil(merr); err != nil {
+		log.Errorf("rollback peer rules: %v", err)
+	}
+}
+
+func (d *DefaultManager) applyPeerGroup(g *peerRuleGroup) (id.RuleID, []firewall.Rule, error) {
+	protocol, err := ConvertToFirewallProtocol(g.protocol)
+	if err != nil {
+		return "", nil, fmt.Errorf("skipping firewall rule: %w", err)
+	}
+	action, err := convertFirewallAction(g.action)
+	if err != nil {
+		return "", nil, fmt.Errorf("skipping firewall rule: %w", err)
+	}
+	port, err := resolveGroupPort(g)
+	if err != nil {
+		return "", nil, err
+	}
+
+	var fwRule firewall.Rule
+	switch g.direction {
+	case mgmProto.RuleDirection_IN:
+		fwRule, err = d.firewall.AddFilterRule(g.policyID, g.sources, firewall.Network{}, protocol, nil, port, action)
+	case mgmProto.RuleDirection_OUT:
+		if d.firewall.IsStateful() {
+			return "", nil, nil
+		}
+		if shouldSkipInvertedRule(protocol, port) {
+			return "", nil, nil
+		}
+		fwRule, err = d.firewall.AddFilterRule(g.policyID, g.sources, firewall.Network{}, protocol, port, nil, action)
+	default:
+		return "", nil, errors.New("invalid direction")
+	}
+
+	if err != nil {
+		return "", nil, fmt.Errorf("add firewall rule: %w", err)
+	}
+	if fwRule == nil {
+		return "", nil, fmt.Errorf("add firewall rule: %w", ErrNoRuleReturned)
+	}
+
+	// Derive the pair id from the backend rule, like the route path:
+	// the backend dedups identical content, so two policies authorizing
+	// the same flow resolve to the same id and a single backing rule.
+	return fwRule.ID(), []firewall.Rule{fwRule}, nil
 }
 
 func (d *DefaultManager) applyRouteACLs(rules []*mgmProto.RouteFirewallRule, dynamicResolver bool) error {
-	newRouteRules := make(map[id.RuleID]struct{}, len(rules))
+	newRouteRules := make(map[id.RuleID]firewall.Rule, len(rules))
 	var merr *multierror.Error
 
-	// Apply new rules - firewall manager will return existing rule ID if already present
+	// Apply new rules - firewall manager will return the existing rule if already present
 	for _, rule := range rules {
-		id, err := d.applyRouteACL(rule, dynamicResolver)
+		addedRule, err := d.applyRouteACL(rule, dynamicResolver)
 		if err != nil {
 			if errors.Is(err, ErrSourceRangesEmpty) {
 				log.Debugf("skipping empty sources rule with destination %s: %v", rule.Destination, err)
@@ -219,16 +361,18 @@ func (d *DefaultManager) applyRouteACLs(rules []*mgmProto.RouteFirewallRule, dyn
 			}
 			continue
 		}
-		newRouteRules[id] = struct{}{}
+		newRouteRules[addedRule.ID()] = addedRule
 	}
 
-	// Clean up old firewall rules
-	for id := range d.routeRules {
-		if _, exists := newRouteRules[id]; !exists {
-			if err := d.firewall.DeleteRouteRule(id); err != nil {
-				merr = multierror.Append(merr, fmt.Errorf("delete route rule: %w", err))
-			}
-			// implicitly deleted from the map
+	// Tear down old route rules; retain ones the backend refused so a
+	// transient failure doesn't leave orphaned rules in the firewall.
+	for ruleID, rule := range d.routeRules {
+		if _, exists := newRouteRules[ruleID]; exists {
+			continue
+		}
+		if err := d.firewall.DeleteFilterRule(rule); err != nil {
+			merr = multierror.Append(merr, fmt.Errorf("delete route rule, will retry: %w", err))
+			newRouteRules[ruleID] = rule
 		}
 	}
 
@@ -236,102 +380,202 @@ func (d *DefaultManager) applyRouteACLs(rules []*mgmProto.RouteFirewallRule, dyn
 	return nberrors.FormatErrorOrNil(merr)
 }
 
-func (d *DefaultManager) applyRouteACL(rule *mgmProto.RouteFirewallRule, dynamicResolver bool) (id.RuleID, error) {
+func (d *DefaultManager) applyRouteACL(rule *mgmProto.RouteFirewallRule, dynamicResolver bool) (firewall.Rule, error) {
 	if len(rule.SourceRanges) == 0 {
-		return "", ErrSourceRangesEmpty
+		return nil, ErrSourceRangesEmpty
 	}
 
 	var sources []netip.Prefix
 	for _, sourceRange := range rule.SourceRanges {
 		source, err := netip.ParsePrefix(sourceRange)
 		if err != nil {
-			return "", fmt.Errorf("parse source range: %w", err)
+			return nil, fmt.Errorf("parse source range: %w", err)
 		}
-		sources = append(sources, source)
+		sources = append(sources, firewall.UnmapPrefix(source))
 	}
 
 	destination, err := determineDestination(rule, dynamicResolver, sources)
 	if err != nil {
-		return "", fmt.Errorf("determine destination: %w", err)
+		return nil, fmt.Errorf("determine destination: %w", err)
 	}
 
-	protocol, err := convertToFirewallProtocol(rule.Protocol)
+	protocol, err := ConvertToFirewallProtocol(rule.Protocol)
 	if err != nil {
-		return "", fmt.Errorf("invalid protocol: %w", err)
+		return nil, fmt.Errorf("invalid protocol: %w", err)
 	}
 
 	action, err := convertFirewallAction(rule.Action)
 	if err != nil {
-		return "", fmt.Errorf("invalid action: %w", err)
+		return nil, fmt.Errorf("invalid action: %w", err)
 	}
 
 	dPorts := convertPortInfo(rule.PortInfo)
 
-	addedRule, err := d.firewall.AddRouteFiltering(rule.PolicyID, sources, destination, protocol, nil, dPorts, action)
+	addedRule, err := d.firewall.AddFilterRule(rule.PolicyID, sources, destination, protocol, nil, dPorts, action)
 	if err != nil {
-		return "", fmt.Errorf("add route rule: %w", err)
+		return nil, fmt.Errorf("add route rule: %w", err)
+	}
+	if addedRule == nil {
+		return nil, fmt.Errorf("add route rule: %w", ErrNoRuleReturned)
 	}
 
-	return id.RuleID(addedRule.ID()), nil
+	return addedRule, nil
 }
 
-func (d *DefaultManager) protoRuleToFirewallRule(
-	r *mgmProto.FirewallRule,
-	ipsetName string,
-) (id.RuleID, []firewall.Rule, error) {
-	ip, err := extractRuleIP(r)
-	if err != nil {
-		return "", nil, err
+// splitDenyAccept partitions groups by action so denies can be
+// applied before accepts. Order within each bucket is preserved.
+func splitDenyAccept(groups []*peerRuleGroup) (denies, accepts []*peerRuleGroup) {
+	for _, g := range groups {
+		if g.action == mgmProto.RuleAction_DROP {
+			denies = append(denies, g)
+		} else {
+			accepts = append(accepts, g)
+		}
+	}
+	return denies, accepts
+}
+
+// groupPeerRules merges single-source rules sharing a selector into
+// multi-source groups. It splits source-decode failures by action:
+// denyErr is non-nil when a deny rule could not be decoded, which is a
+// fail-open risk the caller must treat as fatal for the pass; err
+// carries the tolerable accept-rule failures the caller can log and
+// continue past.
+func groupPeerRules(rules []*mgmProto.FirewallRule) (groups []*peerRuleGroup, denyErr error, err error) {
+	var denyMerr, acceptMerr *multierror.Error
+	byKey := make(map[peerRuleKey]*peerRuleGroup)
+	order := make([]peerRuleKey, 0)
+
+	for _, r := range rules {
+		srcs, decErr := extractRuleSources(r)
+		if decErr != nil {
+			if r.Action == mgmProto.RuleAction_DROP {
+				denyMerr = multierror.Append(denyMerr, decErr)
+			} else {
+				acceptMerr = multierror.Append(acceptMerr, decErr)
+			}
+			continue
+		}
+		// A single FirewallRule normally carries one address family, but
+		// split by family defensively: each backend keys a rule to one
+		// family and would mismatch sources of the other, so a group's
+		// sources must never span families.
+		v4, v6 := splitPrefixesByFamily(srcs)
+		for _, sub := range []struct {
+			isV6    bool
+			sources []netip.Prefix
+		}{{false, v4}, {true, v6}} {
+			if len(sub.sources) == 0 {
+				continue
+			}
+			key := ruleGroupKey(r, sub.isV6)
+			g, ok := byKey[key]
+			if !ok {
+				g = &peerRuleGroup{
+					direction:  r.Direction,
+					action:     r.Action,
+					protocol:   r.Protocol,
+					port:       r.PortInfo,
+					legacyPort: r.Port,
+					policyID:   r.PolicyID,
+				}
+				byKey[key] = g
+				order = append(order, key)
+			}
+			g.sources = append(g.sources, sub.sources...)
+		}
 	}
 
-	protocol, err := convertToFirewallProtocol(r.Protocol)
-	if err != nil {
-		return "", nil, fmt.Errorf("skipping firewall rule: %s", err)
+	out := make([]*peerRuleGroup, 0, len(order))
+	for _, k := range order {
+		out = append(out, byKey[k])
+	}
+	return out, nberrors.FormatErrorOrNil(denyMerr), nberrors.FormatErrorOrNil(acceptMerr)
+}
+
+func prefixIsV6(p netip.Prefix) bool {
+	return p.Addr().Is6() && !p.Addr().Is4In6()
+}
+
+// splitPrefixesByFamily partitions prefixes into IPv4 and IPv6 groups.
+func splitPrefixesByFamily(prefixes []netip.Prefix) (v4, v6 []netip.Prefix) {
+	for _, p := range prefixes {
+		if prefixIsV6(p) {
+			v6 = append(v6, p)
+		} else {
+			v4 = append(v4, p)
+		}
+	}
+	return v4, v6
+}
+
+// ruleGroupKey builds the selector key for a rule. v6 must reflect the
+// rule's source family: mgmt emits one rule per family and mixing them
+// would break ICMP-variant selection in uspfilter.
+func ruleGroupKey(r *mgmProto.FirewallRule, v6 bool) peerRuleKey {
+	k := peerRuleKey{
+		v6:         v6,
+		policyID:   string(r.PolicyID),
+		direction:  r.Direction,
+		action:     r.Action,
+		protocol:   r.Protocol,
+		legacyPort: r.Port,
+	}
+	if pi := r.PortInfo; pi != nil {
+		k.port = uint16(pi.GetPort())
+		if rng := pi.GetRange(); rng != nil {
+			k.rangeStart = uint16(rng.GetStart())
+			k.rangeEnd = uint16(rng.GetEnd())
+		}
+	}
+	return k
+}
+
+// extractRuleSources returns all source prefixes the rule applies to.
+// New management populates sourcePrefixes; older management sets PeerIP.
+func extractRuleSources(r *mgmProto.FirewallRule) ([]netip.Prefix, error) {
+	if len(r.SourcePrefixes) > 0 {
+		out := make([]netip.Prefix, 0, len(r.SourcePrefixes))
+		for _, raw := range r.SourcePrefixes {
+			addr, err := netiputil.DecodeAddr(raw)
+			if err != nil {
+				return nil, fmt.Errorf("decode source prefix: %w", err)
+			}
+			out = append(out, netip.PrefixFrom(addr.Unmap(), addr.Unmap().BitLen()))
+		}
+		return out, nil
 	}
 
-	action, err := convertFirewallAction(r.Action)
+	peerIP := r.PeerIP //nolint:staticcheck // PeerIP is the legacy source field for old management servers
+	addr, err := netip.ParseAddr(peerIP)
 	if err != nil {
-		return "", nil, fmt.Errorf("skipping firewall rule: %s", err)
+		return nil, fmt.Errorf("parse peer IP %q: %w", peerIP, err)
 	}
+	addr = addr.Unmap()
+	// An unspecified PeerIP means "any peer" (legacy management
+	// allow-all fallback); only a /0 prefix matches any source in the
+	// backends, a full-length prefix would match nothing.
+	if addr.IsUnspecified() {
+		return []netip.Prefix{netip.PrefixFrom(addr, 0)}, nil
+	}
+	return []netip.Prefix{netip.PrefixFrom(addr, addr.BitLen())}, nil
+}
 
-	var port *firewall.Port
-	if !portInfoEmpty(r.PortInfo) {
-		port = convertPortInfo(r.PortInfo)
-	} else if r.Port != "" {
-		// old version of management, single port
-		value, err := strconv.Atoi(r.Port)
+func resolveGroupPort(g *peerRuleGroup) (*firewall.Port, error) {
+	if !portInfoEmpty(g.port) {
+		return convertPortInfo(g.port), nil
+	}
+	if g.legacyPort != "" {
+		value, err := strconv.ParseUint(g.legacyPort, 10, 16)
 		if err != nil {
-			return "", nil, fmt.Errorf("invalid port: %w", err)
+			return nil, fmt.Errorf("invalid port: %w", err)
 		}
-		port = &firewall.Port{
+		return &firewall.Port{
 			Values: []uint16{uint16(value)},
-		}
+		}, nil
 	}
-
-	ruleID := d.getPeerRuleID(ip, protocol, int(r.Direction), port, action)
-	if rulesPair, ok := d.peerRulesPairs[ruleID]; ok {
-		return ruleID, rulesPair, nil
-	}
-
-	var rules []firewall.Rule
-	switch r.Direction {
-	case mgmProto.RuleDirection_IN:
-		rules, err = d.addInRules(r.PolicyID, ip, protocol, port, action, ipsetName)
-	case mgmProto.RuleDirection_OUT:
-		if d.firewall.IsStateful() {
-			return "", nil, nil
-		}
-		// return traffic for outbound connections if firewall is stateless
-		rules, err = d.addOutRules(r.PolicyID, ip, protocol, port, action, ipsetName)
-	default:
-		return "", nil, fmt.Errorf("invalid direction, skipping firewall rule")
-	}
-
-	if err != nil {
-		return "", nil, err
-	}
-
-	return ruleID, rules, nil
+	// nolint:nilnil // a nil port legitimately means "no port restriction"
+	return nil, nil
 }
 
 func portInfoEmpty(portInfo *mgmProto.PortInfo) bool {
@@ -350,84 +594,9 @@ func portInfoEmpty(portInfo *mgmProto.PortInfo) bool {
 	}
 }
 
-func (d *DefaultManager) addInRules(
-	id []byte,
-	ip netip.Addr,
-	protocol firewall.Protocol,
-	port *firewall.Port,
-	action firewall.Action,
-	ipsetName string,
-) ([]firewall.Rule, error) {
-	rule, err := d.firewall.AddPeerFiltering(id, ip.AsSlice(), protocol, nil, port, action, ipsetName)
-	if err != nil {
-		return nil, fmt.Errorf("add firewall rule: %w", err)
-	}
-
-	return rule, nil
-}
-
-func (d *DefaultManager) addOutRules(
-	id []byte,
-	ip netip.Addr,
-	protocol firewall.Protocol,
-	port *firewall.Port,
-	action firewall.Action,
-	ipsetName string,
-) ([]firewall.Rule, error) {
-	if shouldSkipInvertedRule(protocol, port) {
-		return nil, nil
-	}
-
-	rule, err := d.firewall.AddPeerFiltering(id, ip.AsSlice(), protocol, port, nil, action, ipsetName)
-	if err != nil {
-		return nil, fmt.Errorf("add firewall rule: %w", err)
-	}
-
-	return rule, nil
-}
-
-// getPeerRuleID returns unique ID for the rule based on its parameters.
-func (d *DefaultManager) getPeerRuleID(
-	ip netip.Addr,
-	proto firewall.Protocol,
-	direction int,
-	port *firewall.Port,
-	action firewall.Action,
-) id.RuleID {
-	idStr := ip.String() + string(proto) + strconv.Itoa(direction) + strconv.Itoa(int(action))
-	if port != nil {
-		idStr += port.String()
-	}
-
-	return id.RuleID(hex.EncodeToString(md5.New().Sum([]byte(idStr))))
-}
-
-// getRuleGroupingSelector takes all rule properties except IP address to build selector
-func (d *DefaultManager) getRuleGroupingSelector(rule *mgmProto.FirewallRule) string {
-	return fmt.Sprintf("%v:%v:%v:%s:%v", strconv.Itoa(int(rule.Direction)), rule.Action, rule.Protocol, rule.Port, rule.PortInfo)
-}
-
-// extractRuleIP extracts the peer IP from a firewall rule.
-// If sourcePrefixes is populated (new management), decode the first entry and use its address.
-// Otherwise fall back to the deprecated PeerIP string field (old management).
-func extractRuleIP(r *mgmProto.FirewallRule) (netip.Addr, error) {
-	if len(r.SourcePrefixes) > 0 {
-		addr, err := netiputil.DecodeAddr(r.SourcePrefixes[0])
-		if err != nil {
-			return netip.Addr{}, fmt.Errorf("decode source prefix: %w", err)
-		}
-		return addr.Unmap(), nil
-	}
-
-	//nolint:staticcheck // PeerIP used for backward compatibility with old management
-	addr, err := netip.ParseAddr(r.PeerIP)
-	if err != nil {
-		return netip.Addr{}, fmt.Errorf("invalid IP address, skipping firewall rule")
-	}
-	return addr.Unmap(), nil
-}
-
-func convertToFirewallProtocol(protocol mgmProto.RuleProtocol) (firewall.Protocol, error) {
+// ConvertToFirewallProtocol maps a management rule protocol to the
+// firewall protocol type.
+func ConvertToFirewallProtocol(protocol mgmProto.RuleProtocol) (firewall.Protocol, error) {
 	switch protocol {
 	case mgmProto.RuleProtocol_TCP:
 		return firewall.ProtocolTCP, nil

client/internal/acl/manager_test.go

@@ -10,6 +10,7 @@ import (
 	"go.uber.org/mock/gomock"
 
 	"github.com/netbirdio/netbird/client/firewall"
+	fwmanager "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/iface"
 	"github.com/netbirdio/netbird/client/iface/wgaddr"
 	"github.com/netbirdio/netbird/client/internal/acl/mocks"
@@ -77,9 +78,9 @@ func TestDefaultManager(t *testing.T) {
 	})
 
 	t.Run("add extra rules", func(t *testing.T) {
-		existedPairs := map[string]struct{}{}
+		existedPairs := map[fwmanager.RuleID]struct{}{}
 		for id := range acl.peerRulesPairs {
-			existedPairs[id.ID()] = struct{}{}
+			existedPairs[id] = struct{}{}
 		}
 
 		// remove first rule
@@ -106,7 +107,7 @@ func TestDefaultManager(t *testing.T) {
 		// check that old rule was removed
 		previousCount := 0
 		for id := range acl.peerRulesPairs {
-			if _, ok := existedPairs[id.ID()]; ok {
+			if _, ok := existedPairs[id]; ok {
 				previousCount++
 			}
 		}

client/internal/dns/server_test.go

@@ -423,7 +423,7 @@ func createWgInterfaceWithBind(t *testing.T) (*iface.WGIface, error) {
 		return nil, err
 	}
 
-	pf, err := uspfilter.Create(wgIface, false, flowLogger, iface.DefaultMTU)
+	pf, err := uspfilter.Create(uspfilter.Config{IFace: wgIface, FlowLogger: flowLogger, MTU: iface.DefaultMTU})
 	if err != nil {
 		t.Fatalf("failed to create uspfilter: %v", err)
 		return nil, err

client/internal/dnsfwd/manager.go

@@ -3,7 +3,6 @@ package dnsfwd
 import (
 	"context"
 	"fmt"
-	"net"
 	"net/netip"
 	"os"
 	"strconv"
@@ -160,12 +159,13 @@ func (m *Manager) allowDNSFirewall() error {
 		return nil
 	}
 
-	dnsRules, err := m.firewall.AddPeerFiltering(nil, net.IP{0, 0, 0, 0}, firewall.ProtocolUDP, nil, dport, firewall.ActionAccept, "")
+	anyV4 := []netip.Prefix{netip.PrefixFrom(netip.IPv4Unspecified(), 0)}
+	dnsRule, err := m.firewall.AddFilterRule(nil, anyV4, firewall.Network{}, firewall.ProtocolUDP, nil, dport, firewall.ActionAccept)
 	if err != nil {
 		return fmt.Errorf("add udp firewall rule: %w", err)
 	}
 
-	tcpRules, err := m.firewall.AddPeerFiltering(nil, net.IP{0, 0, 0, 0}, firewall.ProtocolTCP, nil, dport, firewall.ActionAccept, "")
+	tcpRule, err := m.firewall.AddFilterRule(nil, anyV4, firewall.Network{}, firewall.ProtocolTCP, nil, dport, firewall.ActionAccept)
 	if err != nil {
 		return fmt.Errorf("add tcp firewall rule: %w", err)
 	}
@@ -174,8 +174,12 @@ func (m *Manager) allowDNSFirewall() error {
 		return fmt.Errorf("flush: %w", err)
 	}
 
-	m.fwRules = dnsRules
-	m.tcpRules = tcpRules
+	if dnsRule != nil {
+		m.fwRules = []firewall.Rule{dnsRule}
+	}
+	if tcpRule != nil {
+		m.tcpRules = []firewall.Rule{tcpRule}
+	}
 
 	m.registerNetstackServices()
 
@@ -209,12 +213,12 @@ func (m *Manager) unregisterNetstackServices() {
 func (m *Manager) dropDNSFirewall() error {
 	var mErr *multierror.Error
 	for _, rule := range m.fwRules {
-		if err := m.firewall.DeletePeerRule(rule); err != nil {
+		if err := m.firewall.DeleteFilterRule(rule); err != nil {
 			mErr = multierror.Append(mErr, fmt.Errorf("failed to delete DNS router rules, err: %v", err))
 		}
 	}
 	for _, rule := range m.tcpRules {
-		if err := m.firewall.DeletePeerRule(rule); err != nil {
+		if err := m.firewall.DeleteFilterRule(rule); err != nil {
 			mErr = multierror.Append(mErr, fmt.Errorf("failed to delete DNS router rules, err: %v", err))
 		}
 	}

client/internal/engine.go

@@ -745,6 +745,11 @@ func (e *Engine) initFirewall() error {
 		return fmt.Errorf("set firewall: %w", err)
 	}
 
+	// TODO: the firewall backends dedup filter rules by content, so a
+	// management route ACL with identical content would collapse onto the
+	// untracked drop rules installed here, and a later management delete
+	// could remove them. Needs backend refcounting or per-consumer key
+	// namespacing.
 	if e.config.BlockLANAccess {
 		e.blockLanAccess()
 	}
@@ -757,14 +762,14 @@ func (e *Engine) initFirewall() error {
 	port := firewallManager.Port{Values: []uint16{uint16(rosenpassPort)}}
 
 	// IPv4-only: rosenpass peers connect via AllowedIps[0] which is always v4.
-	if _, err := e.firewall.AddPeerFiltering(
+	if _, err := e.firewall.AddFilterRule(
 		nil,
-		net.IP{0, 0, 0, 0},
+		[]netip.Prefix{netip.PrefixFrom(netip.IPv4Unspecified(), 0)},
+		firewallManager.Network{},
 		firewallManager.ProtocolUDP,
 		nil,
 		&port,
 		firewallManager.ActionAccept,
-		"",
 	); err != nil {
 		log.Errorf("failed to allow rosenpass interface traffic: %v", err)
 		return nil
@@ -814,7 +819,7 @@ func (e *Engine) blockLanAccess() {
 		if network.Addr().Is6() {
 			source = v6
 		}
-		if _, err := e.firewall.AddRouteFiltering(
+		if _, err := e.firewall.AddFilterRule(
 			nil,
 			[]netip.Prefix{source},
 			firewallManager.Network{Prefix: network},
@@ -2658,7 +2663,7 @@ func (e *Engine) updateForwardRules(rules []*mgmProto.ForwardingRule) ([]firewal
 	var merr *multierror.Error
 	forwardingRules := make([]firewallManager.ForwardRule, 0, len(rules))
 	for _, rule := range rules {
-		proto, err := convertToFirewallProtocol(rule.GetProtocol())
+		proto, err := acl.ConvertToFirewallProtocol(rule.GetProtocol())
 		if err != nil {
 			merr = multierror.Append(merr, fmt.Errorf("failed to convert protocol '%s': %w", rule.GetProtocol(), err))
 			continue

client/internal/ingressgw/manager.go

@@ -24,14 +24,14 @@ type RulePair struct {
 type Manager struct {
 	dnatFirewall DNATFirewall
 
-	rules   map[string]RulePair // keys is the ID of the ForwardRule
+	rules   map[firewall.RuleID]RulePair
 	rulesMu sync.Mutex
 }
 
 func NewManager(dnatFirewall DNATFirewall) *Manager {
 	return &Manager{
 		dnatFirewall: dnatFirewall,
-		rules:        make(map[string]RulePair),
+		rules:        make(map[firewall.RuleID]RulePair),
 	}
 }
 
@@ -41,7 +41,7 @@ func (h *Manager) Update(forwardRules []firewall.ForwardRule) error {
 
 	var mErr *multierror.Error
 
-	toDelete := make(map[string]RulePair, len(h.rules))
+	toDelete := make(map[firewall.RuleID]RulePair, len(h.rules))
 	for id, r := range h.rules {
 		toDelete[id] = r
 	}
@@ -59,6 +59,10 @@ func (h *Manager) Update(forwardRules []firewall.ForwardRule) error {
 			mErr = multierror.Append(mErr, fmt.Errorf("add forward rule '%s': %v", fwdRule.String(), err))
 			continue
 		}
+		if rule == nil {
+			mErr = multierror.Append(mErr, fmt.Errorf("add forward rule '%s': backend returned no rule", fwdRule.String()))
+			continue
+		}
 		log.Infof("forward rule has been added '%s'", fwdRule)
 		h.rules[id] = RulePair{
 			ForwardRule: fwdRule,
@@ -90,7 +94,7 @@ func (h *Manager) Close() error {
 		}
 	}
 
-	h.rules = make(map[string]RulePair)
+	h.rules = make(map[firewall.RuleID]RulePair)
 	return nberrors.FormatErrorOrNil(mErr)
 }
 

client/internal/ingressgw/manager_test.go

@@ -14,11 +14,11 @@ var (
 )
 
 type MocFwRule struct {
-	id string
+	id firewall.RuleID
 }
 
-func (m *MocFwRule) ID() string {
-	return string(m.id)
+func (m *MocFwRule) ID() firewall.RuleID {
+	return m.id
 }
 
 type MockDNATFirewall struct {

client/internal/message_convert.go

@@ -10,21 +10,6 @@ import (
 	mgmProto "github.com/netbirdio/netbird/shared/management/proto"
 )
 
-func convertToFirewallProtocol(protocol mgmProto.RuleProtocol) (firewallManager.Protocol, error) {
-	switch protocol {
-	case mgmProto.RuleProtocol_TCP:
-		return firewallManager.ProtocolTCP, nil
-	case mgmProto.RuleProtocol_UDP:
-		return firewallManager.ProtocolUDP, nil
-	case mgmProto.RuleProtocol_ICMP:
-		return firewallManager.ProtocolICMP, nil
-	case mgmProto.RuleProtocol_ALL:
-		return firewallManager.ProtocolALL, nil
-	default:
-		return "", fmt.Errorf("invalid protocol type: %s", protocol.String())
-	}
-}
-
 func convertPortInfo(portInfo *mgmProto.PortInfo) (*firewallManager.Port, error) {
 	if portInfo == nil {
 		return nil, errors.New("portInfo cannot be nil")

client/internal/routemanager/server/server.go

@@ -135,6 +135,14 @@ func (r *Router) CleanUp() {
 		}
 	}
 
+	// Give back the routing reference taken in UpdateRoutes, after the routes
+	// are gone as above. Without this the sysctls enabling it changed (IPv6
+	// forwarding and the accept_ra values that keep RA handling alive next to
+	// it) stay applied once the client stops.
+	if err := r.firewall.DisableRouting(); err != nil {
+		log.Errorf("Failed to disable routing: %v", err)
+	}
+
 	r.statusRecorder.CleanLocalPeerStateRoutes()
 }
 

client/internal/routemanager/server/server_test.go

@@ -0,0 +1,66 @@
+package server
+
+import (
+	"context"
+	"net/netip"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	firewall "github.com/netbirdio/netbird/client/firewall/manager"
+	"github.com/netbirdio/netbird/client/internal/peer"
+	"github.com/netbirdio/netbird/route"
+)
+
+// routingFirewall records the routing lifecycle calls the router makes. The
+// embedded interface covers the methods this test never reaches.
+type routingFirewall struct {
+	firewall.Manager
+
+	removed  []firewall.RouterPair
+	enabled  int
+	disabled int
+}
+
+func (f *routingFirewall) RemoveNatRule(pair firewall.RouterPair) error {
+	f.removed = append(f.removed, pair)
+	return nil
+}
+
+func (f *routingFirewall) EnableRouting() error {
+	f.enabled++
+	return nil
+}
+
+func (f *routingFirewall) DisableRouting() error {
+	f.disabled++
+	return nil
+}
+
+// TestRouterCleanUpReleasesRouting covers the shutdown path: the router holds a
+// routing reference for as long as it serves routes, and CleanUp has to give it
+// back. Without that the sysctls the reference enabled (IPv6 forwarding and the
+// accept_ra values that keep RA handling working alongside it) stay applied
+// after the client stops, leaving the host configured as a router.
+func TestRouterCleanUpReleasesRouting(t *testing.T) {
+	fw := &routingFirewall{}
+	r := &Router{
+		ctx:            context.Background(),
+		firewall:       fw,
+		statusRecorder: peer.NewRecorder("https://mgm"),
+		routes: map[route.ID]*route.Route{
+			"route-1": {
+				ID:          "route-1",
+				Network:     netip.MustParsePrefix("192.168.55.0/24"),
+				NetworkType: route.IPv4Network,
+				Masquerade:  true,
+			},
+		},
+	}
+
+	r.CleanUp()
+
+	require.Len(t, fw.removed, 1, "the route's NAT rule must be removed")
+	assert.Equal(t, 1, fw.disabled, "CleanUp must release the routing reference")
+}