Merge branch 'main' into embedded-vnc

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/connect_test.go

@@ -5,65 +5,78 @@ import (
 	"testing"
 )
 
-func Test_freePort(t *testing.T) {
-	tests := []struct {
-		name        string
-		port        int
-		want        int
-		shouldMatch bool
-	}{
-		{
-			name:        "when port is 0 use random port",
-			port:        0,
-			want:        0,
-			shouldMatch: false,
-		},
-		{
-			name:        "provided and available",
-			port:        51821,
-			want:        51821,
-			shouldMatch: true,
-		},
-		{
-			name:        "provided and not available",
-			port:        51830,
-			want:        51830,
-			shouldMatch: false,
-		},
-	}
-	c1, err := net.ListenUDP("udp", &net.UDPAddr{Port: 0})
+// probeFreePort asks the OS for a free UDP port and immediately releases it.
+// The returned number is only a hint: nothing stops another process from
+// grabbing the same port before the caller gets a chance to bind it.
+//
+// A hardcoded port number is not an option here: any fixed number can fall
+// inside the ephemeral range and be held by an unrelated process on the test
+// runner.
+func probeFreePort(t *testing.T) int {
+	t.Helper()
+
+	conn, err := net.ListenUDP("udp", &net.UDPAddr{Port: 0})
 	if err != nil {
-		t.Errorf("freePort error = %v", err)
+		t.Fatalf("failed to bind probe port: %v", err)
 	}
-	defer func(c1 *net.UDPConn) {
-		_ = c1.Close()
-	}(c1)
-
-	if tests[1].port == c1.LocalAddr().(*net.UDPAddr).Port {
-		tests[1].port++
-		tests[1].want++
-	}
-
-	tests[2].port = c1.LocalAddr().(*net.UDPAddr).Port
-	tests[2].want = c1.LocalAddr().(*net.UDPAddr).Port
-
-	for _, tt := range tests {
-
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := freePort(tt.port)
-
-			if err != nil {
-				t.Errorf("got an error while getting free port: %v", err)
-			}
-
-			if tt.shouldMatch && got != tt.want {
-				t.Errorf("got a different port %v, want %v", got, tt.want)
-			}
-
-			if !tt.shouldMatch && got == tt.want {
-				t.Errorf("got the same port %v, want a different port", tt.want)
-			}
-		})
-
+	port := conn.LocalAddr().(*net.UDPAddr).Port
+	if err := conn.Close(); err != nil {
+		t.Fatalf("failed to close probe port: %v", err)
 	}
+	return port
+}
+
+func Test_freePort(t *testing.T) {
+	t.Run("when port is 0 use random port", func(t *testing.T) {
+		got, err := freePort(0)
+		if err != nil {
+			t.Fatalf("got an error while getting free port: %v", err)
+		}
+		if got == 0 {
+			t.Errorf("got port 0, want a non-zero random port")
+		}
+	})
+
+	t.Run("provided and available", func(t *testing.T) {
+		const maxAttempts = 5
+
+		// The probed port is released before freePort binds it, so an
+		// unrelated process on the test runner can grab it in between,
+		// making freePort fall back to a different port. Retry with a
+		// freshly probed port instead of failing on a lost race.
+		for attempt := 1; attempt <= maxAttempts; attempt++ {
+			candidate := probeFreePort(t)
+
+			got, err := freePort(candidate)
+			if err != nil {
+				t.Fatalf("got an error while getting free port: %v", err)
+			}
+
+			if got == candidate {
+				return
+			}
+			t.Logf("attempt %d: freePort returned %d instead of the requested %d, retrying", attempt, got, candidate)
+		}
+
+		t.Fatalf("freePort did not return the requested free port after %d attempts", maxAttempts)
+	})
+
+	t.Run("provided and not available", func(t *testing.T) {
+		busy, err := net.ListenUDP("udp", &net.UDPAddr{Port: 0})
+		if err != nil {
+			t.Fatalf("failed to bind busy port: %v", err)
+		}
+		t.Cleanup(func() {
+			_ = busy.Close()
+		})
+		busyPort := busy.LocalAddr().(*net.UDPAddr).Port
+
+		got, err := freePort(busyPort)
+		if err != nil {
+			t.Fatalf("got an error while getting free port: %v", err)
+		}
+		if got == busyPort {
+			t.Errorf("got the same port %v, want a different port", busyPort)
+		}
+	})
 }

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/forwarder.go

@@ -54,12 +54,20 @@ type DNSForwarder struct {
 	ttl            uint32
 	statusRecorder *peer.Status
 
-	dnsServer *dns.Server
-	mux       *dns.ServeMux
-	tcpServer *dns.Server
-	tcpMux    *dns.ServeMux
+	mux    *dns.ServeMux
+	tcpMux *dns.ServeMux
 
-	mutex      sync.RWMutex
+	mutex sync.RWMutex
+	// closed records that Close has run, so a Listen still in flight does not
+	// go on to serve sockets nobody will shut down.
+	closed bool
+	// The sockets are kept alongside the servers because closing them is the
+	// only stop that always works: a server whose ActivateAndServe has not run
+	// yet refuses to shut down, and would otherwise start serving afterwards.
+	udpConn    net.PacketConn
+	tcpLn      net.Listener
+	dnsServer  *dns.Server
+	tcpServer  *dns.Server
 	fwdEntries []*ForwarderEntry
 	firewall   firewaller
 	resolver   resolver
@@ -106,7 +114,7 @@ func (f *DNSForwarder) Listen(entries []*ForwarderEntry) error {
 	mux := dns.NewServeMux()
 	f.mux = mux
 	mux.HandleFunc(".", f.handleDNSQueryUDP)
-	f.dnsServer = &dns.Server{
+	dnsServer := &dns.Server{
 		PacketConn: udpLn,
 		Handler:    mux,
 	}
@@ -114,22 +122,32 @@ func (f *DNSForwarder) Listen(entries []*ForwarderEntry) error {
 	tcpMux := dns.NewServeMux()
 	f.tcpMux = tcpMux
 	tcpMux.HandleFunc(".", f.handleDNSQueryTCP)
-	f.tcpServer = &dns.Server{
+	tcpServer := &dns.Server{
 		Listener: tcpLn,
 		Handler:  tcpMux,
 	}
 
-	f.UpdateDomains(entries)
+	if !f.publish(udpLn, tcpLn, dnsServer, tcpServer, entries) {
+		log.Infof("DNS forwarder on %s was closed before it started serving", addrDesc)
+		if err := udpLn.Close(); err != nil {
+			log.Debugf("close UDP listener of a closed forwarder: %v", err)
+		}
+		if err := tcpLn.Close(); err != nil {
+			log.Debugf("close TCP listener of a closed forwarder: %v", err)
+		}
+		return nil
+	}
+	log.Debugf("DNS forwarder serving %d domains", len(entries))
 
 	errCh := make(chan error, 2)
 
 	go func() {
 		log.Infof("DNS UDP listener running on %s", addrDesc)
-		errCh <- f.dnsServer.ActivateAndServe()
+		errCh <- dnsServer.ActivateAndServe()
 	}()
 	go func() {
 		log.Infof("DNS TCP listener running on %s", addrDesc)
-		errCh <- f.tcpServer.ActivateAndServe()
+		errCh <- tcpServer.ActivateAndServe()
 	}()
 
 	return <-errCh
@@ -151,6 +169,46 @@ func (f *DNSForwarder) createTCPListener(netstackNet *netstack.Net) (net.Listene
 	return net.ListenTCP("tcp", net.TCPAddrFromAddrPort(f.listenAddress))
 }
 
+// publish hands the sockets, servers and entries to the forwarder so Close can
+// reach them and Domains can report them, and says whether serving may begin.
+// Listen runs on its own goroutine, so a Close can arrive before it gets this
+// far; false means the caller must close what it created instead of serving on
+// it.
+//
+// The entries go in under the same lock rather than afterwards. Anything that
+// reads them in between would otherwise see a forwarder that is listening and
+// serves no domain, which for a caller rebuilding one means it comes back
+// refusing every routed query.
+func (f *DNSForwarder) publish(
+	udpConn net.PacketConn,
+	tcpLn net.Listener,
+	dnsServer, tcpServer *dns.Server,
+	entries []*ForwarderEntry,
+) bool {
+	f.mutex.Lock()
+	defer f.mutex.Unlock()
+
+	if f.closed {
+		return false
+	}
+
+	f.udpConn = udpConn
+	f.tcpLn = tcpLn
+	f.dnsServer = dnsServer
+	f.tcpServer = tcpServer
+	f.fwdEntries = entries
+	return true
+}
+
+// Domains returns the entries currently being served. The slice is replaced
+// wholesale by UpdateDomains rather than mutated, so the caller may read it but
+// must not write to it.
+func (f *DNSForwarder) Domains() []*ForwarderEntry {
+	f.mutex.RLock()
+	defer f.mutex.RUnlock()
+	return f.fwdEntries
+}
+
 func (f *DNSForwarder) UpdateDomains(entries []*ForwarderEntry) {
 	f.mutex.Lock()
 	defer f.mutex.Unlock()
@@ -189,19 +247,45 @@ func (f *DNSForwarder) removeStaleCacheEntries(oldEntries, newEntries []*Forward
 }
 
 func (f *DNSForwarder) Close(ctx context.Context) error {
+	// Marked closed under the lock so a Listen that has not published its
+	// servers yet gives up instead of racing this shutdown. The shutdowns
+	// themselves block, so they run outside it.
+	f.mutex.Lock()
+	f.closed = true
+	dnsServer, tcpServer := f.dnsServer, f.tcpServer
+	udpConn, tcpLn := f.udpConn, f.tcpLn
+	f.mutex.Unlock()
+
 	var result *multierror.Error
 
-	if f.dnsServer != nil {
-		if err := f.dnsServer.ShutdownContext(ctx); err != nil {
+	if dnsServer != nil {
+		if err := shutdownServer(ctx, dnsServer); err != nil {
 			result = multierror.Append(result, fmt.Errorf("UDP shutdown: %w", err))
 		}
 	}
-	if f.tcpServer != nil {
-		if err := f.tcpServer.ShutdownContext(ctx); err != nil {
+	if tcpServer != nil {
+		if err := shutdownServer(ctx, tcpServer); err != nil {
 			result = multierror.Append(result, fmt.Errorf("TCP shutdown: %w", err))
 		}
 	}
 
+	// The sockets are closed even when the shutdowns above reported nothing to
+	// do. A server that has been published but has not reached
+	// ActivateAndServe refuses to shut down, and closing what it was about to
+	// serve on is what stops it: the alternative is a listener still answering
+	// on an interface that has gone away. A shutdown that did run has already
+	// closed these, so the second close is expected to fail.
+	if udpConn != nil {
+		if err := udpConn.Close(); err != nil {
+			log.Debugf("close UDP socket of the DNS forwarder: %v", err)
+		}
+	}
+	if tcpLn != nil {
+		if err := tcpLn.Close(); err != nil {
+			log.Debugf("close TCP socket of the DNS forwarder: %v", err)
+		}
+	}
+
 	return nberrors.FormatErrorOrNil(result)
 }
 
@@ -514,3 +598,16 @@ func attachEDE(resp *dns.Msg, code uint16, text string) {
 	}
 	opt.Option = append(opt.Option, &dns.EDNS0_EDE{InfoCode: code, ExtraText: text})
 }
+
+// shutdownServer shuts a server down gracefully, treating "never started" as
+// success. A server that was published but has not reached ActivateAndServe
+// has nothing to wind down, and the caller closes its socket regardless, which
+// is what actually stops it. dns exports no sentinel for this, so the message
+// is all there is to match on.
+func shutdownServer(ctx context.Context, server *dns.Server) error {
+	err := server.ShutdownContext(ctx)
+	if err == nil || strings.Contains(err.Error(), "server not started") {
+		return nil
+	}
+	return err
+}

client/internal/dnsfwd/forwarder_test.go

@@ -1238,3 +1238,55 @@ func TestDNSForwarder_EmptyQuery(t *testing.T) {
 
 	assert.Nil(t, mockWriter.GetLastResponse(), "Should not write response for empty query")
 }
+
+// TestDNSForwarder_ClosedBeforeItServes covers Listen reaching the point of
+// serving after the forwarder has already been closed. Listen runs on its own
+// goroutine, so it can get there late, and a socket it starts serving then is
+// one nothing will ever close: on Android it keeps answering on an interface
+// that has been replaced. The close is sequenced first here rather than raced,
+// which pins the same state deterministically.
+func TestDNSForwarder_ClosedBeforeItServes(t *testing.T) {
+	f := NewDNSForwarder(netip.MustParseAddrPort("127.0.0.1:0"), 60, nil, nil, nil)
+
+	require.NoError(t, f.Close(context.Background()), "closing a forwarder that never started")
+
+	done := make(chan error, 1)
+	go func() { done <- f.Listen(nil) }()
+
+	select {
+	case err := <-done:
+		assert.NoError(t, err, "a closed forwarder should give up quietly, not serve")
+	case <-time.After(5 * time.Second):
+		t.Fatal("Listen went on to serve after the forwarder was closed")
+	}
+}
+
+// TestDNSForwarder_CloseStopsUnactivatedServers covers the window between
+// Listen publishing its servers and reaching ActivateAndServe. A server that
+// has not been activated refuses to shut down, so Close has to close the
+// sockets itself or they are left serving.
+func TestDNSForwarder_CloseStopsUnactivatedServers(t *testing.T) {
+	f := NewDNSForwarder(netip.MustParseAddrPort("127.0.0.1:0"), 60, nil, nil, nil)
+
+	udpConn, err := f.createUDPListener(nil)
+	require.NoError(t, err, "create UDP listener")
+	tcpLn, err := f.createTCPListener(nil)
+	require.NoError(t, err, "create TCP listener")
+
+	// Published but deliberately never activated, which is the state Listen is
+	// in for the moment before it starts serving.
+	require.True(t, f.publish(udpConn, tcpLn, &dns.Server{PacketConn: udpConn}, &dns.Server{Listener: tcpLn}, nil),
+		"publishing to an open forwarder")
+
+	tcpAddr := tcpLn.Addr().String()
+	require.NoError(t, f.Close(context.Background()), "close should report no error for servers it could not shut down")
+
+	_, err = tcpLn.Accept()
+	assert.Error(t, err, "the TCP socket should be closed after Close")
+
+	conn, err := net.DialTimeout("tcp", tcpAddr, time.Second)
+	if err == nil {
+		_ = conn.Close()
+		t.Fatal("the forwarder is still accepting connections after Close")
+	}
+}

client/internal/dnsfwd/manager.go

@@ -3,7 +3,6 @@ package dnsfwd
 import (
 	"context"
 	"fmt"
-	"net"
 	"net/netip"
 	"os"
 	"strconv"
@@ -118,6 +117,16 @@ func (m *Manager) UpdateDomains(entries []*ForwarderEntry) {
 	m.dnsForwarder.UpdateDomains(entries)
 }
 
+// Domains returns the entries currently being served, or nil when the
+// forwarder is not running.
+func (m *Manager) Domains() []*ForwarderEntry {
+	if m.dnsForwarder == nil {
+		return nil
+	}
+
+	return m.dnsForwarder.Domains()
+}
+
 func (m *Manager) Stop(ctx context.Context) error {
 	if m.dnsForwarder == nil {
 		return nil
@@ -160,12 +169,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 +184,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 +223,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

@@ -95,6 +95,13 @@ const (
 	// exec, os.Stat); without this bound a single stuck call freezes handleSync, and
 	// thus syncMsgMux, for as long as the call hangs (observed multi-minute freezes).
 	systemInfoTimeout = 15 * time.Second
+
+	// dnsForwarderStopTimeout bounds how long stopping the DNS forwarder waits
+	// for the queries still in flight. One waiting on an unresponsive upstream
+	// would otherwise hold the stop for the whole upstream timeout, and the
+	// stop runs with syncMsgMux held. The sockets are closed either way, so
+	// giving up costs a query that was already failing.
+	dnsForwarderStopTimeout = 2 * time.Second
 )
 
 var ErrResetConnection = fmt.Errorf("reset connection")
@@ -326,6 +333,10 @@ type localIpUpdater interface {
 	UpdateLocalIPs() error
 }
 
+// overlayRebind rebuilds one subsystem's sockets on the current interface. The
+// error it returns names its own subsystem, since the caller can only log it.
+type overlayRebind func() error
+
 // NewEngine creates a new Connection Engine with probes attached
 func NewEngine(
 	clientCtx context.Context,
@@ -755,6 +766,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()
 	}
@@ -767,14 +783,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
@@ -824,7 +840,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},
@@ -2517,7 +2533,72 @@ func (e *Engine) RenewTun(fd int) error {
 		return fmt.Errorf("wireguard interface not initialized")
 	}
 
-	return wgInterface.RenewTun(fd)
+	if err := wgInterface.RenewTun(fd); err != nil {
+		return err
+	}
+
+	e.rebindOverlayListeners()
+	return nil
+}
+
+// rebindOverlayListeners gives the servers that listen on an overlay address
+// sockets on the interface as it is now.
+//
+// A socket belongs to the interface generation it was created on. Renewing the
+// TUN builds a new interface and moves the overlay addresses to it, which
+// leaves the old sockets in LISTEN with the uspfilter still logging packets
+// arriving for them, while every accept fails with EINVAL for the life of the
+// socket: from the outside the server looks alive and answers nothing. On
+// Android this happens during a normal startup, where the first TUN is
+// established before the routes are known and replaced once they arrive.
+//
+// Rebinding costs whatever those sockets were carrying, which the renewal has
+// already broken. Errors are logged rather than returned: the renewal itself
+// succeeded, and failing it would hand the caller a working interface and an
+// error.
+func (e *Engine) rebindOverlayListeners() {
+	e.syncMsgMux.Lock()
+	defer e.syncMsgMux.Unlock()
+
+	for _, rebind := range e.overlayRebinds() {
+		if err := rebind(); err != nil {
+			log.Errorf("after TUN renewal: %v", err)
+		}
+	}
+}
+
+// overlayRebinds is every subsystem of this engine that holds sockets bound to
+// an overlay address, and how to rebuild each one's.
+//
+// A subsystem that starts listening on an overlay address belongs in this list.
+// Leaving it out costs nothing that review would notice and produces a listener
+// that stays in LISTEN, is logged as receiving packets, and refuses every
+// connection for the life of the process.
+func (e *Engine) overlayRebinds() []overlayRebind {
+	return []overlayRebind{
+		e.restartSSHListeners,
+		e.restartDNSForwarder,
+	}
+}
+
+// restartDNSForwarder rebuilds the DNS forwarder serving the same domains.
+// No-op when it is not running. See Engine.rebindOverlayListeners.
+func (e *Engine) restartDNSForwarder() error {
+	if e.dnsForwardMgr == nil {
+		return nil
+	}
+	// Read from the forwarder before it goes away, so the replacement serves
+	// the domains in force now rather than a copy kept somewhere else.
+	entries := e.dnsForwardMgr.Domains()
+	e.stopDNSForwarder()
+	// Both halves log their own failures, so the only thing left to report is
+	// the outcome: a start that failed left the manager nil, and the forwarder
+	// is now down rather than merely rebound.
+	e.startDNSForwarder(entries)
+	if e.dnsForwardMgr == nil {
+		return errors.New("rebind DNS forwarder: it did not come back up")
+	}
+	return nil
 }
 
 // updateDNSForwarder start or stop the DNS forwarder based on the domains and the feature flag
@@ -2563,7 +2644,14 @@ func (e *Engine) stopDNSForwarder() {
 		return
 	}
 
-	if err := e.dnsForwardMgr.Stop(context.Background()); err != nil {
+	// Bounded because the shutdown waits for queries still in flight, and one
+	// waiting on an unresponsive upstream holds it for as long as that lookup
+	// is allowed to take. This runs with syncMsgMux held, so that wait is one
+	// the whole engine spends.
+	ctx, cancel := context.WithTimeout(context.Background(), dnsForwarderStopTimeout)
+	defer cancel()
+
+	if err := e.dnsForwardMgr.Stop(ctx); err != nil {
 		log.Errorf("failed to stop DNS forward: %v", err)
 	}
 
@@ -2678,7 +2766,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/engine_ssh.go

@@ -24,6 +24,8 @@ type sshServer interface {
 	Stop() error
 	GetStatus() (bool, []sshserver.SessionInfo)
 	UpdateSSHAuth(config *sshauth.Config)
+	JWTConfig() *sshserver.JWTConfig
+	AuthConfig() *sshauth.Config
 }
 
 func (e *Engine) setupSSHPortRedirection() error {
@@ -77,7 +79,7 @@ func (e *Engine) updateSSH(sshConf *mgmProto.SSHConfig) error {
 
 	if e.config.DisableSSHAuth != nil && *e.config.DisableSSHAuth {
 		log.Info("starting SSH server without JWT authentication (authentication disabled by config)")
-		return e.startSSHServer(nil)
+		return e.startSSHServer(nil, nil)
 	}
 
 	if protoJWT := sshConf.GetJwtConfig(); protoJWT != nil {
@@ -95,7 +97,7 @@ func (e *Engine) updateSSH(sshConf *mgmProto.SSHConfig) error {
 			MaxTokenAge:  protoJWT.GetMaxTokenAge(),
 		}
 
-		return e.startSSHServer(jwtConfig)
+		return e.startSSHServer(jwtConfig, nil)
 	}
 
 	return errors.New("SSH server requires valid JWT configuration")
@@ -231,8 +233,33 @@ func (e *Engine) cleanupSSHConfig() {
 	}
 }
 
-// startSSHServer initializes and starts the SSH server with proper configuration.
-func (e *Engine) startSSHServer(jwtConfig *sshserver.JWTConfig) error {
+// restartSSHListeners rebuilds the SSH server so it listens on new sockets, on
+// the same terms it was started with. No-op when it is not running. See
+// Engine.rebindOverlayListeners for why this is needed.
+func (e *Engine) restartSSHListeners() error {
+	if e.sshServer == nil {
+		return nil
+	}
+	// Read from the server before it goes away. A rebuilt one starts with an
+	// empty authorizer, which fails closed, so without carrying the
+	// authorization over every JWT login is refused until the next network map
+	// happens to bring one.
+	jwtConfig, authConfig := e.sshServer.JWTConfig(), e.sshServer.AuthConfig()
+	if err := e.stopSSHServer(); err != nil {
+		return fmt.Errorf("rebind SSH listeners: %w", err)
+	}
+	if err := e.startSSHServer(jwtConfig, authConfig); err != nil {
+		return fmt.Errorf("rebind SSH listeners: %w", err)
+	}
+	return nil
+}
+
+// startSSHServer initializes and starts the SSH server with proper
+// configuration. authConfig is the fine-grained authorization to open with, and
+// is applied before the server accepts anything: a server that starts listening
+// with an empty authorizer refuses the logins that arrive in the meantime.
+// Nil leaves it as management has not sent one yet.
+func (e *Engine) startSSHServer(jwtConfig *sshserver.JWTConfig, authConfig *sshauth.Config) error {
 	if e.wgInterface == nil {
 		return errors.New("wg interface not initialized")
 	}
@@ -241,6 +268,7 @@ func (e *Engine) startSSHServer(jwtConfig *sshserver.JWTConfig) error {
 	serverConfig := &sshserver.Config{
 		HostKeyPEM:        e.config.SSHKey,
 		JWT:               jwtConfig,
+		Auth:              authConfig,
 		NetstackNet:       e.wgInterface.GetNet(),
 		NetworkValidation: wgAddr,
 	}

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/notifier/notifier_android.go

@@ -4,8 +4,6 @@ package notifier
 
 import (
 	"net/netip"
-	"slices"
-	"sort"
 	"sync"
 
 	"github.com/netbirdio/netbird/client/internal/listener"
@@ -75,19 +73,3 @@ func (n *Notifier) notifyLocked() {
 func (n *Notifier) Close() {
 	// unused
 }
-
-func routesToStrings(routes []*route.Route) []string {
-	nets := make([]string, 0, len(routes))
-	for _, r := range routes {
-		nets = append(nets, r.NetString())
-	}
-	return nets
-}
-
-func hasRouteDiff(a []*route.Route, b []*route.Route) bool {
-	as := routesToStrings(a)
-	bs := routesToStrings(b)
-	sort.Strings(as)
-	sort.Strings(bs)
-	return !slices.Equal(as, bs)
-}

client/internal/routemanager/notifier/route_diff.go

@@ -0,0 +1,27 @@
+package notifier
+
+import (
+	"slices"
+	"sort"
+
+	"github.com/netbirdio/netbird/route"
+)
+
+// routePrefixes returns the distinct prefixes a route set covers, sorted.
+// Duplicates are dropped deliberately: an HA group hands us one route per
+// peer serving the same prefix, and the platform is given the prefix, not the
+// candidates. Counting them would report a change every time a peer joins or
+// leaves a group, and on Android each report renews the TUN.
+func routePrefixes(routes []*route.Route) []string {
+	nets := make([]string, 0, len(routes))
+	for _, r := range routes {
+		nets = append(nets, r.NetString())
+	}
+	sort.Strings(nets)
+	return slices.Compact(nets)
+}
+
+// hasRouteDiff reports whether the prefixes the two route sets cover differ.
+func hasRouteDiff(a []*route.Route, b []*route.Route) bool {
+	return !slices.Equal(routePrefixes(a), routePrefixes(b))
+}

client/internal/routemanager/notifier/route_diff_test.go

@@ -0,0 +1,88 @@
+package notifier
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/netbirdio/netbird/route"
+)
+
+func routeFor(id route.ID, prefix string) *route.Route {
+	return &route.Route{
+		ID:      id,
+		NetID:   "net",
+		Network: netip.MustParsePrefix(prefix),
+	}
+}
+
+// TestHasRouteDiff_IgnoresHACandidateCount is the reason the comparison
+// deduplicates. Every notification renews the TUN, and a renewed TUN
+// invalidates the sockets the embedded servers are listening on, so a peer
+// joining or leaving an HA group must not count as a route change when the
+// prefixes the TUN carries are identical.
+func TestHasRouteDiff_IgnoresHACandidateCount(t *testing.T) {
+	onePeer := []*route.Route{routeFor("a", "10.0.0.0/24")}
+	twoPeers := []*route.Route{
+		routeFor("a", "10.0.0.0/24"),
+		routeFor("b", "10.0.0.0/24"),
+	}
+
+	assert.False(t, hasRouteDiff(onePeer, twoPeers),
+		"a second peer serving the same prefix is not a route change")
+	assert.False(t, hasRouteDiff(twoPeers, onePeer),
+		"losing one of two peers serving the same prefix is not a route change")
+}
+
+func TestHasRouteDiff_ReportsRealChanges(t *testing.T) {
+	tests := []struct {
+		name string
+		a    []*route.Route
+		b    []*route.Route
+		want bool
+	}{
+		{
+			name: "added prefix",
+			a:    []*route.Route{routeFor("a", "10.0.0.0/24")},
+			b:    []*route.Route{routeFor("a", "10.0.0.0/24"), routeFor("b", "10.0.1.0/24")},
+			want: true,
+		},
+		{
+			name: "removed prefix",
+			a:    []*route.Route{routeFor("a", "10.0.0.0/24"), routeFor("b", "10.0.1.0/24")},
+			b:    []*route.Route{routeFor("a", "10.0.0.0/24")},
+			want: true,
+		},
+		{
+			name: "replaced prefix",
+			a:    []*route.Route{routeFor("a", "10.0.0.0/24")},
+			b:    []*route.Route{routeFor("a", "10.0.1.0/24")},
+			want: true,
+		},
+		{
+			name: "same prefix, different order",
+			a:    []*route.Route{routeFor("a", "10.0.1.0/24"), routeFor("b", "10.0.0.0/24")},
+			b:    []*route.Route{routeFor("b", "10.0.0.0/24"), routeFor("a", "10.0.1.0/24")},
+			want: false,
+		},
+		{
+			name: "all routes gone",
+			a:    []*route.Route{routeFor("a", "10.0.0.0/24")},
+			b:    nil,
+			want: true,
+		},
+		{
+			name: "both empty",
+			a:    nil,
+			b:    nil,
+			want: false,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, hasRouteDiff(tc.a, tc.b),
+				"route diff for %s", tc.name)
+		})
+	}
+}

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")
+}