[client] Fix uspfilter duplicate firewall rules (#5269)

client/firewall/uspfilter/allow_netbird.go

@@ -3,12 +3,6 @@
 package uspfilter
 
 import (
-	"context"
-	"net/netip"
-	"time"
-
-	log "github.com/sirupsen/logrus"
-
 	"github.com/netbirdio/netbird/client/internal/statemanager"
 )
 
@@ -17,33 +11,7 @@ func (m *Manager) Close(stateManager *statemanager.Manager) error {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()
 
-	m.outgoingRules = make(map[netip.Addr]RuleSet)
+	m.resetState()
-	m.incomingDenyRules = make(map[netip.Addr]RuleSet)
-	m.incomingRules = make(map[netip.Addr]RuleSet)
-
-	if m.udpTracker != nil {
-		m.udpTracker.Close()
-	}
-
-	if m.icmpTracker != nil {
-		m.icmpTracker.Close()
-	}
-
-	if m.tcpTracker != nil {
-		m.tcpTracker.Close()
-	}
-
-	if fwder := m.forwarder.Load(); fwder != nil {
-		fwder.Stop()
-	}
-
-	if m.logger != nil {
-		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
-		defer cancel()
-		if err := m.logger.Stop(ctx); err != nil {
-			log.Errorf("failed to shutdown logger: %v", err)
-		}
-	}
 
 	if m.nativeFirewall != nil {
 		return m.nativeFirewall.Close(stateManager)

client/firewall/uspfilter/allow_netbird_windows.go

@@ -1,12 +1,9 @@
 package uspfilter
 
 import (
-	"context"
 	"fmt"
-	"net/netip"
 	"os/exec"
 	"syscall"
-	"time"
 
 	log "github.com/sirupsen/logrus"
 
@@ -26,33 +23,7 @@ func (m *Manager) Close(*statemanager.Manager) error {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()
 
-	m.outgoingRules = make(map[netip.Addr]RuleSet)
+	m.resetState()
-	m.incomingDenyRules = make(map[netip.Addr]RuleSet)
-	m.incomingRules = make(map[netip.Addr]RuleSet)
-
-	if m.udpTracker != nil {
-		m.udpTracker.Close()
-	}
-
-	if m.icmpTracker != nil {
-		m.icmpTracker.Close()
-	}
-
-	if m.tcpTracker != nil {
-		m.tcpTracker.Close()
-	}
-
-	if fwder := m.forwarder.Load(); fwder != nil {
-		fwder.Stop()
-	}
-
-	if m.logger != nil {
-		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
-		defer cancel()
-		if err := m.logger.Stop(ctx); err != nil {
-			log.Errorf("failed to shutdown logger: %v", err)
-		}
-	}
 
 	if !isWindowsFirewallReachable() {
 		return nil

client/firewall/uspfilter/filter.go

@@ -1,6 +1,7 @@
 package uspfilter
 
 import (
+	"context"
 	"encoding/binary"
 	"errors"
 	"fmt"
@@ -12,11 +13,13 @@ import (
 	"strings"
 	"sync"
 	"sync/atomic"
+	"time"
 
 	"github.com/google/gopacket"
 	"github.com/google/gopacket/layers"
 	"github.com/google/uuid"
 	log "github.com/sirupsen/logrus"
+	"golang.org/x/exp/maps"
 
 	firewall "github.com/netbirdio/netbird/client/firewall/manager"
 	"github.com/netbirdio/netbird/client/firewall/uspfilter/common"
@@ -24,6 +27,7 @@ import (
 	"github.com/netbirdio/netbird/client/firewall/uspfilter/forwarder"
 	nblog "github.com/netbirdio/netbird/client/firewall/uspfilter/log"
 	"github.com/netbirdio/netbird/client/iface/netstack"
+	nbid "github.com/netbirdio/netbird/client/internal/acl/id"
 	nftypes "github.com/netbirdio/netbird/client/internal/netflow/types"
 	"github.com/netbirdio/netbird/client/internal/statemanager"
 )
@@ -89,6 +93,7 @@ type Manager struct {
 	incomingDenyRules map[netip.Addr]RuleSet
 	incomingRules     map[netip.Addr]RuleSet
 	routeRules        RouteRules
+	routeRulesMap     map[nbid.RuleID]*RouteRule
 	decoders          sync.Pool
 	wgIface           common.IFaceMapper
 	nativeFirewall    firewall.Manager
@@ -229,6 +234,7 @@ func create(iface common.IFaceMapper, nativeFirewall firewall.Manager, disableSe
 		flowLogger:          flowLogger,
 		netstack:            netstack.IsEnabled(),
 		localForwarding:     enableLocalForwarding,
+		routeRulesMap:       make(map[nbid.RuleID]*RouteRule),
 		dnatMappings:        make(map[netip.Addr]netip.Addr),
 		portDNATRules:       []portDNATRule{},
 		netstackServices:    make(map[serviceKey]struct{}),
@@ -480,11 +486,15 @@ func (m *Manager) addRouteFiltering(
 		return m.nativeFirewall.AddRouteFiltering(id, sources, destination, proto, sPort, dPort, action)
 	}
 
-	ruleID := uuid.New().String()
+	ruleKey := nbid.GenerateRouteRuleKey(sources, destination, proto, sPort, dPort, action)
+
+	if existingRule, ok := m.routeRulesMap[ruleKey]; ok {
+		return existingRule, nil
+	}
 
 	rule := RouteRule{
 		// TODO: consolidate these IDs
-		id:         ruleID,
+		id:         string(ruleKey),
 		mgmtId:     id,
 		sources:    sources,
 		dstSet:     destination.Set,
@@ -499,6 +509,7 @@ func (m *Manager) addRouteFiltering(
 
 	m.routeRules = append(m.routeRules, &rule)
 	m.routeRules.Sort()
+	m.routeRulesMap[ruleKey] = &rule
 
 	return &rule, nil
 }
@@ -515,15 +526,20 @@ func (m *Manager) deleteRouteRule(rule firewall.Rule) error {
 		return m.nativeFirewall.DeleteRouteRule(rule)
 	}
 
-	ruleID := rule.ID()
+	ruleKey := nbid.RuleID(rule.ID())
+	if _, ok := m.routeRulesMap[ruleKey]; !ok {
+		return fmt.Errorf("route rule not found: %s", ruleKey)
+	}
+
 	idx := slices.IndexFunc(m.routeRules, func(r *RouteRule) bool {
-		return r.id == ruleID
+		return r.id == string(ruleKey)
 	})
 	if idx < 0 {
-		return fmt.Errorf("route rule not found: %s", ruleID)
+		return fmt.Errorf("route rule not found in slice: %s", ruleKey)
 	}
 
 	m.routeRules = slices.Delete(m.routeRules, idx, idx+1)
+	delete(m.routeRulesMap, ruleKey)
 	return nil
 }
 
@@ -570,6 +586,40 @@ func (m *Manager) SetLegacyManagement(isLegacy bool) error {
 // Flush doesn't need to be implemented for this manager
 func (m *Manager) Flush() error { return nil }
 
+// resetState clears all firewall rules and closes connection trackers.
+// Must be called with m.mutex held.
+func (m *Manager) resetState() {
+	maps.Clear(m.outgoingRules)
+	maps.Clear(m.incomingDenyRules)
+	maps.Clear(m.incomingRules)
+	maps.Clear(m.routeRulesMap)
+	m.routeRules = m.routeRules[:0]
+
+	if m.udpTracker != nil {
+		m.udpTracker.Close()
+	}
+
+	if m.icmpTracker != nil {
+		m.icmpTracker.Close()
+	}
+
+	if m.tcpTracker != nil {
+		m.tcpTracker.Close()
+	}
+
+	if fwder := m.forwarder.Load(); fwder != nil {
+		fwder.Stop()
+	}
+
+	if m.logger != nil {
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+		defer cancel()
+		if err := m.logger.Stop(ctx); err != nil {
+			log.Errorf("failed to shutdown logger: %v", err)
+		}
+	}
+}
+
 // SetupEBPFProxyNoTrack creates notrack rules for eBPF proxy loopback traffic.
 func (m *Manager) SetupEBPFProxyNoTrack(proxyPort, wgPort uint16) error {
 	if m.nativeFirewall == nil {

client/firewall/uspfilter/filter_routeacl_test.go

@@ -0,0 +1,376 @@
+package uspfilter
+
+import (
+	"net/netip"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/google/gopacket/layers"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	wgdevice "golang.zx2c4.com/wireguard/device"
+
+	fw "github.com/netbirdio/netbird/client/firewall/manager"
+	"github.com/netbirdio/netbird/client/iface"
+	"github.com/netbirdio/netbird/client/iface/device"
+	"github.com/netbirdio/netbird/client/iface/mocks"
+	"github.com/netbirdio/netbird/client/iface/wgaddr"
+)
+
+// TestAddRouteFilteringReturnsExistingRule verifies that adding the same route
+// filtering rule twice returns the same rule ID (idempotent behavior).
+func TestAddRouteFilteringReturnsExistingRule(t *testing.T) {
+	manager := setupTestManager(t)
+
+	sources := []netip.Prefix{
+		netip.MustParsePrefix("100.64.1.0/24"),
+		netip.MustParsePrefix("100.64.2.0/24"),
+	}
+	destination := fw.Network{Prefix: netip.MustParsePrefix("192.168.1.0/24")}
+
+	// Add rule first time
+	rule1, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		&fw.Port{Values: []uint16{443}},
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+	require.NotNil(t, rule1)
+
+	// Add the same rule again
+	rule2, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		&fw.Port{Values: []uint16{443}},
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+	require.NotNil(t, rule2)
+
+	// These should be the same (idempotent) like nftables/iptables implementations
+	assert.Equal(t, rule1.ID(), rule2.ID(),
+		"Adding the same rule twice should return the same rule ID (idempotent)")
+
+	manager.mutex.RLock()
+	ruleCount := len(manager.routeRules)
+	manager.mutex.RUnlock()
+
+	assert.Equal(t, 2, ruleCount,
+		"Should have exactly 2 rules (1 user rule + 1 block rule)")
+}
+
+// TestAddRouteFilteringDifferentRulesGetDifferentIDs verifies that rules with
+// different parameters get distinct IDs.
+func TestAddRouteFilteringDifferentRulesGetDifferentIDs(t *testing.T) {
+	manager := setupTestManager(t)
+
+	sources := []netip.Prefix{netip.MustParsePrefix("100.64.1.0/24")}
+
+	// Add first rule
+	rule1, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		fw.Network{Prefix: netip.MustParsePrefix("192.168.1.0/24")},
+		fw.ProtocolTCP,
+		nil,
+		&fw.Port{Values: []uint16{443}},
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	// Add different rule (different destination)
+	rule2, err := manager.AddRouteFiltering(
+		[]byte("policy-2"),
+		sources,
+		fw.Network{Prefix: netip.MustParsePrefix("192.168.2.0/24")}, // Different!
+		fw.ProtocolTCP,
+		nil,
+		&fw.Port{Values: []uint16{443}},
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	assert.NotEqual(t, rule1.ID(), rule2.ID(),
+		"Different rules should have different IDs")
+
+	manager.mutex.RLock()
+	ruleCount := len(manager.routeRules)
+	manager.mutex.RUnlock()
+
+	assert.Equal(t, 3, ruleCount, "Should have 3 rules (2 user rules + 1 block rule)")
+}
+
+// TestRouteRuleUpdateDoesNotCauseGap verifies that re-adding the same route
+// rule during a network map update does not disrupt existing traffic.
+func TestRouteRuleUpdateDoesNotCauseGap(t *testing.T) {
+	manager := setupTestManager(t)
+
+	sources := []netip.Prefix{netip.MustParsePrefix("100.64.1.0/24")}
+	destination := fw.Network{Prefix: netip.MustParsePrefix("192.168.1.0/24")}
+
+	rule1, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		nil,
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	srcIP := netip.MustParseAddr("100.64.1.5")
+	dstIP := netip.MustParseAddr("192.168.1.10")
+	_, pass := manager.routeACLsPass(srcIP, dstIP, layers.LayerTypeTCP, 12345, 443)
+	require.True(t, pass, "Traffic should pass with rule in place")
+
+	// Re-add same rule (simulates network map update)
+	rule2, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		nil,
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	// Idempotent IDs mean rule1.ID() == rule2.ID(), so the ACL manager
+	// won't delete rule1 during cleanup. If IDs differed, deleting rule1
+	// would remove the only matching rule and cause a traffic gap.
+	if rule1.ID() != rule2.ID() {
+		err = manager.DeleteRouteRule(rule1)
+		require.NoError(t, err)
+	}
+
+	_, passAfter := manager.routeACLsPass(srcIP, dstIP, layers.LayerTypeTCP, 12345, 443)
+	assert.True(t, passAfter,
+		"Traffic should still pass after rule update - no gap should occur")
+}
+
+// TestBlockInvalidRoutedIdempotent verifies that blockInvalidRouted creates
+// exactly one drop rule for the WireGuard network prefix, and calling it again
+// returns the same rule without duplicating.
+func TestBlockInvalidRoutedIdempotent(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	dev := mocks.NewMockDevice(ctrl)
+	dev.EXPECT().MTU().Return(1500, nil).AnyTimes()
+
+	wgNet := netip.MustParsePrefix("100.64.0.1/16")
+
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+		AddressFunc: func() wgaddr.Address {
+			return wgaddr.Address{
+				IP:      wgNet.Addr(),
+				Network: wgNet,
+			}
+		},
+		GetDeviceFunc: func() *device.FilteredDevice {
+			return &device.FilteredDevice{Device: dev}
+		},
+		GetWGDeviceFunc: func() *wgdevice.Device {
+			return &wgdevice.Device{}
+		},
+	}
+
+	manager, err := Create(ifaceMock, false, flowLogger, iface.DefaultMTU)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, manager.Close(nil))
+	})
+
+	// Call blockInvalidRouted directly multiple times
+	rule1, err := manager.blockInvalidRouted(ifaceMock)
+	require.NoError(t, err)
+	require.NotNil(t, rule1)
+
+	rule2, err := manager.blockInvalidRouted(ifaceMock)
+	require.NoError(t, err)
+	require.NotNil(t, rule2)
+
+	rule3, err := manager.blockInvalidRouted(ifaceMock)
+	require.NoError(t, err)
+	require.NotNil(t, rule3)
+
+	// All should return the same rule
+	assert.Equal(t, rule1.ID(), rule2.ID(), "Second call should return same rule")
+	assert.Equal(t, rule2.ID(), rule3.ID(), "Third call should return same rule")
+
+	// Should have exactly 1 route rule
+	manager.mutex.RLock()
+	ruleCount := len(manager.routeRules)
+	manager.mutex.RUnlock()
+
+	assert.Equal(t, 1, ruleCount, "Should have exactly 1 block rule after 3 calls")
+
+	// Verify the rule blocks traffic to the WG network
+	srcIP := netip.MustParseAddr("10.0.0.1")
+	dstIP := netip.MustParseAddr("100.64.0.50")
+	_, pass := manager.routeACLsPass(srcIP, dstIP, layers.LayerTypeTCP, 12345, 80)
+	assert.False(t, pass, "Block rule should deny traffic to WG prefix")
+}
+
+// TestBlockRuleNotAccumulatedOnRepeatedEnableRouting verifies that calling
+// EnableRouting multiple times (as happens on each route update) does not
+// accumulate duplicate block rules in the routeRules slice.
+func TestBlockRuleNotAccumulatedOnRepeatedEnableRouting(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	dev := mocks.NewMockDevice(ctrl)
+	dev.EXPECT().MTU().Return(1500, nil).AnyTimes()
+
+	wgNet := netip.MustParsePrefix("100.64.0.1/16")
+
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+		AddressFunc: func() wgaddr.Address {
+			return wgaddr.Address{
+				IP:      wgNet.Addr(),
+				Network: wgNet,
+			}
+		},
+		GetDeviceFunc: func() *device.FilteredDevice {
+			return &device.FilteredDevice{Device: dev}
+		},
+		GetWGDeviceFunc: func() *wgdevice.Device {
+			return &wgdevice.Device{}
+		},
+	}
+
+	manager, err := Create(ifaceMock, false, flowLogger, iface.DefaultMTU)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, manager.Close(nil))
+	})
+
+	// Call EnableRouting multiple times (simulating repeated route updates)
+	for i := 0; i < 5; i++ {
+		require.NoError(t, manager.EnableRouting())
+	}
+
+	manager.mutex.RLock()
+	ruleCount := len(manager.routeRules)
+	manager.mutex.RUnlock()
+
+	assert.Equal(t, 1, ruleCount,
+		"Repeated EnableRouting should not accumulate block rules")
+}
+
+// TestRouteRuleCountStableAcrossUpdates verifies that adding the same route
+// rule multiple times does not create duplicate entries.
+func TestRouteRuleCountStableAcrossUpdates(t *testing.T) {
+	manager := setupTestManager(t)
+
+	sources := []netip.Prefix{netip.MustParsePrefix("100.64.1.0/24")}
+	destination := fw.Network{Prefix: netip.MustParsePrefix("192.168.1.0/24")}
+
+	// Simulate 5 network map updates with the same route rule
+	for i := 0; i < 5; i++ {
+		rule, err := manager.AddRouteFiltering(
+			[]byte("policy-1"),
+			sources,
+			destination,
+			fw.ProtocolTCP,
+			nil,
+			&fw.Port{Values: []uint16{443}},
+			fw.ActionAccept,
+		)
+		require.NoError(t, err)
+		require.NotNil(t, rule)
+	}
+
+	manager.mutex.RLock()
+	ruleCount := len(manager.routeRules)
+	manager.mutex.RUnlock()
+
+	assert.Equal(t, 2, ruleCount,
+		"Should have exactly 2 rules (1 user rule + 1 block rule) after 5 updates")
+}
+
+// TestDeleteRouteRuleAfterIdempotentAdd verifies that deleting a route rule
+// after adding it multiple times works correctly.
+func TestDeleteRouteRuleAfterIdempotentAdd(t *testing.T) {
+	manager := setupTestManager(t)
+
+	sources := []netip.Prefix{netip.MustParsePrefix("100.64.1.0/24")}
+	destination := fw.Network{Prefix: netip.MustParsePrefix("192.168.1.0/24")}
+
+	// Add same rule twice
+	rule1, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		nil,
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	rule2, err := manager.AddRouteFiltering(
+		[]byte("policy-1"),
+		sources,
+		destination,
+		fw.ProtocolTCP,
+		nil,
+		nil,
+		fw.ActionAccept,
+	)
+	require.NoError(t, err)
+
+	require.Equal(t, rule1.ID(), rule2.ID(), "Should return same rule ID")
+
+	// Delete using first reference
+	err = manager.DeleteRouteRule(rule1)
+	require.NoError(t, err)
+
+	// Verify traffic no longer passes
+	srcIP := netip.MustParseAddr("100.64.1.5")
+	dstIP := netip.MustParseAddr("192.168.1.10")
+	_, pass := manager.routeACLsPass(srcIP, dstIP, layers.LayerTypeTCP, 12345, 443)
+	assert.False(t, pass, "Traffic should not pass after rule deletion")
+}
+
+func setupTestManager(t *testing.T) *Manager {
+	t.Helper()
+
+	ctrl := gomock.NewController(t)
+	dev := mocks.NewMockDevice(ctrl)
+	dev.EXPECT().MTU().Return(1500, nil).AnyTimes()
+
+	wgNet := netip.MustParsePrefix("100.64.0.1/16")
+
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+		AddressFunc: func() wgaddr.Address {
+			return wgaddr.Address{
+				IP:      wgNet.Addr(),
+				Network: wgNet,
+			}
+		},
+		GetDeviceFunc: func() *device.FilteredDevice {
+			return &device.FilteredDevice{Device: dev}
+		},
+		GetWGDeviceFunc: func() *wgdevice.Device {
+			return &wgdevice.Device{}
+		},
+	}
+
+	manager, err := Create(ifaceMock, false, flowLogger, iface.DefaultMTU)
+	require.NoError(t, err)
+	require.NoError(t, manager.EnableRouting())
+
+	t.Cleanup(func() {
+		require.NoError(t, manager.Close(nil))
+	})
+
+	return manager
+}

client/firewall/uspfilter/filter_test.go

@@ -263,6 +263,158 @@ func TestAddUDPPacketHook(t *testing.T) {
 	}
 }
 
+// TestPeerRuleLifecycleDenyRules verifies that deny rules are correctly added
+// to the deny map and can be cleanly deleted without leaving orphans.
+func TestPeerRuleLifecycleDenyRules(t *testing.T) {
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+	}
+
+	m, err := Create(ifaceMock, false, flowLogger, nbiface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, m.Close(nil))
+	}()
+
+	ip := net.ParseIP("192.168.1.1")
+	addr := netip.MustParseAddr("192.168.1.1")
+
+	// Add multiple deny rules for different ports
+	rule1, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+		&fw.Port{Values: []uint16{22}}, fw.ActionDrop, "")
+	require.NoError(t, err)
+
+	rule2, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+		&fw.Port{Values: []uint16{80}}, fw.ActionDrop, "")
+	require.NoError(t, err)
+
+	m.mutex.RLock()
+	denyCount := len(m.incomingDenyRules[addr])
+	m.mutex.RUnlock()
+	require.Equal(t, 2, denyCount, "Should have exactly 2 deny rules")
+
+	// Delete the first deny rule
+	err = m.DeletePeerRule(rule1[0])
+	require.NoError(t, err)
+
+	m.mutex.RLock()
+	denyCount = len(m.incomingDenyRules[addr])
+	m.mutex.RUnlock()
+	require.Equal(t, 1, denyCount, "Should have 1 deny rule after deleting first")
+
+	// Delete the second deny rule
+	err = m.DeletePeerRule(rule2[0])
+	require.NoError(t, err)
+
+	m.mutex.RLock()
+	_, exists := m.incomingDenyRules[addr]
+	m.mutex.RUnlock()
+	require.False(t, exists, "Deny rules IP entry should be cleaned up when empty")
+}
+
+// TestPeerRuleAddAndDeleteDontLeak verifies that repeatedly adding and deleting
+// peer rules (simulating network map updates) does not leak rules in the maps.
+func TestPeerRuleAddAndDeleteDontLeak(t *testing.T) {
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+	}
+
+	m, err := Create(ifaceMock, false, flowLogger, nbiface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, m.Close(nil))
+	}()
+
+	ip := net.ParseIP("192.168.1.1")
+	addr := netip.MustParseAddr("192.168.1.1")
+
+	// Simulate 10 network map updates: add rule, delete old, add new
+	for i := 0; i < 10; i++ {
+		// Add a deny rule
+		rules, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+			&fw.Port{Values: []uint16{22}}, fw.ActionDrop, "")
+		require.NoError(t, err)
+
+		// Add an allow rule
+		allowRules, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+			&fw.Port{Values: []uint16{80}}, fw.ActionAccept, "")
+		require.NoError(t, err)
+
+		// Delete them (simulating ACL manager cleanup)
+		for _, r := range rules {
+			require.NoError(t, m.DeletePeerRule(r))
+		}
+		for _, r := range allowRules {
+			require.NoError(t, m.DeletePeerRule(r))
+		}
+	}
+
+	m.mutex.RLock()
+	denyCount := len(m.incomingDenyRules[addr])
+	allowCount := len(m.incomingRules[addr])
+	m.mutex.RUnlock()
+
+	require.Equal(t, 0, denyCount, "No deny rules should remain after cleanup")
+	require.Equal(t, 0, allowCount, "No allow rules should remain after cleanup")
+}
+
+// TestMixedAllowDenyRulesSameIP verifies that allow and deny rules for the same
+// IP are stored in separate maps and don't interfere with each other.
+func TestMixedAllowDenyRulesSameIP(t *testing.T) {
+	ifaceMock := &IFaceMock{
+		SetFilterFunc: func(device.PacketFilter) error { return nil },
+	}
+
+	m, err := Create(ifaceMock, false, flowLogger, nbiface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, m.Close(nil))
+	}()
+
+	ip := net.ParseIP("192.168.1.1")
+
+	// Add allow rule for port 80
+	allowRule, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+		&fw.Port{Values: []uint16{80}}, fw.ActionAccept, "")
+	require.NoError(t, err)
+
+	// Add deny rule for port 22
+	denyRule, err := m.AddPeerFiltering(nil, ip, fw.ProtocolTCP, nil,
+		&fw.Port{Values: []uint16{22}}, fw.ActionDrop, "")
+	require.NoError(t, err)
+
+	addr := netip.MustParseAddr("192.168.1.1")
+	m.mutex.RLock()
+	allowCount := len(m.incomingRules[addr])
+	denyCount := len(m.incomingDenyRules[addr])
+	m.mutex.RUnlock()
+
+	require.Equal(t, 1, allowCount, "Should have 1 allow rule")
+	require.Equal(t, 1, denyCount, "Should have 1 deny rule")
+
+	// Delete allow rule should not affect deny rule
+	err = m.DeletePeerRule(allowRule[0])
+	require.NoError(t, err)
+
+	m.mutex.RLock()
+	denyCountAfter := len(m.incomingDenyRules[addr])
+	m.mutex.RUnlock()
+
+	require.Equal(t, 1, denyCountAfter, "Deny rule should still exist after deleting allow rule")
+
+	// Delete deny rule
+	err = m.DeletePeerRule(denyRule[0])
+	require.NoError(t, err)
+
+	m.mutex.RLock()
+	_, denyExists := m.incomingDenyRules[addr]
+	_, allowExists := m.incomingRules[addr]
+	m.mutex.RUnlock()
+
+	require.False(t, denyExists, "Deny rules should be empty")
+	require.False(t, allowExists, "Allow rules should be empty")
+}
+
 func TestManagerReset(t *testing.T) {
 	ifaceMock := &IFaceMock{
 		SetFilterFunc: func(device.PacketFilter) error { return nil },

client/internal/acl/manager_test.go

@@ -189,6 +189,212 @@ func TestDefaultManagerStateless(t *testing.T) {
 	})
 }
 
+// TestDenyRulesNotAccumulatedOnRepeatedApply verifies that applying the same
+// deny rules repeatedly does not accumulate duplicate rules in the uspfilter.
+// This tests the full ACL manager -> uspfilter integration.
+func TestDenyRulesNotAccumulatedOnRepeatedApply(t *testing.T) {
+	t.Setenv("NB_WG_KERNEL_DISABLED", "true")
+
+	networkMap := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PeerIP:    "10.93.0.1",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_DROP,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "22",
+			},
+			{
+				PeerIP:    "10.93.0.2",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_DROP,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "80",
+			},
+			{
+				PeerIP:    "10.93.0.3",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "443",
+			},
+		},
+		FirewallRulesIsEmpty: false,
+	}
+
+	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()
+
+	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, fw.Close(nil))
+	}()
+
+	acl := NewDefaultManager(fw)
+
+	// Apply the same rules 5 times (simulating repeated network map updates)
+	for i := 0; i < 5; i++ {
+		acl.ApplyFiltering(networkMap, false)
+	}
+
+	// The ACL manager should track exactly 3 rule pairs (2 deny + 1 accept inbound)
+	assert.Equal(t, 3, len(acl.peerRulesPairs),
+		"Should have exactly 3 rule pairs after 5 identical updates")
+}
+
+// TestDenyRulesCleanedUpOnRemoval verifies that deny rules are properly cleaned
+// up when they're removed from the network map in a subsequent update.
+func TestDenyRulesCleanedUpOnRemoval(t *testing.T) {
+	t.Setenv("NB_WG_KERNEL_DISABLED", "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()
+
+	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, fw.Close(nil))
+	}()
+
+	acl := NewDefaultManager(fw)
+
+	// First update: add deny and accept rules
+	networkMap1 := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PeerIP:    "10.93.0.1",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_DROP,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "22",
+			},
+			{
+				PeerIP:    "10.93.0.2",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "443",
+			},
+		},
+		FirewallRulesIsEmpty: false,
+	}
+
+	acl.ApplyFiltering(networkMap1, false)
+	assert.Equal(t, 2, len(acl.peerRulesPairs), "Should have 2 rules after first update")
+
+	// Second update: remove the deny rule, keep only accept
+	networkMap2 := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PeerIP:    "10.93.0.2",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "443",
+			},
+		},
+		FirewallRulesIsEmpty: false,
+	}
+
+	acl.ApplyFiltering(networkMap2, false)
+	assert.Equal(t, 1, len(acl.peerRulesPairs),
+		"Should have 1 rule after removing deny rule")
+
+	// Third update: remove all rules
+	networkMap3 := &mgmProto.NetworkMap{
+		FirewallRules:        []*mgmProto.FirewallRule{},
+		FirewallRulesIsEmpty: true,
+	}
+
+	acl.ApplyFiltering(networkMap3, false)
+	assert.Equal(t, 0, len(acl.peerRulesPairs),
+		"Should have 0 rules after removing all rules")
+}
+
+// TestRuleUpdateChangingAction verifies that when a rule's action changes from
+// accept to deny (or vice versa), the old rule is properly removed and the new
+// one added without leaking.
+func TestRuleUpdateChangingAction(t *testing.T) {
+	t.Setenv("NB_WG_KERNEL_DISABLED", "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()
+
+	fw, err := firewall.NewFirewall(ifaceMock, nil, flowLogger, false, iface.DefaultMTU)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, fw.Close(nil))
+	}()
+
+	acl := NewDefaultManager(fw)
+
+	// First update: accept rule
+	networkMap := &mgmProto.NetworkMap{
+		FirewallRules: []*mgmProto.FirewallRule{
+			{
+				PeerIP:    "10.93.0.1",
+				Direction: mgmProto.RuleDirection_IN,
+				Action:    mgmProto.RuleAction_ACCEPT,
+				Protocol:  mgmProto.RuleProtocol_TCP,
+				Port:      "22",
+			},
+		},
+		FirewallRulesIsEmpty: false,
+	}
+	acl.ApplyFiltering(networkMap, false)
+	assert.Equal(t, 1, len(acl.peerRulesPairs))
+
+	// Second update: change to deny (same IP/port/proto, different action)
+	networkMap.FirewallRules = []*mgmProto.FirewallRule{
+		{
+			PeerIP:    "10.93.0.1",
+			Direction: mgmProto.RuleDirection_IN,
+			Action:    mgmProto.RuleAction_DROP,
+			Protocol:  mgmProto.RuleProtocol_TCP,
+			Port:      "22",
+		},
+	}
+	acl.ApplyFiltering(networkMap, false)
+
+	// Should still have exactly 1 rule (the old accept removed, new deny added)
+	assert.Equal(t, 1, len(acl.peerRulesPairs),
+		"Changing action should result in exactly 1 rule, not 2")
+}
+
 func TestPortInfoEmpty(t *testing.T) {
 	tests := []struct {
 		name     string