From 391221a986813463345eed059d2593802fdcf823 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Mon, 9 Feb 2026 17:14:02 +0800 Subject: [PATCH] [client] Fix uspfilter duplicate firewall rules (#5269) --- client/firewall/uspfilter/allow_netbird.go | 34 +- .../uspfilter/allow_netbird_windows.go | 31 +- client/firewall/uspfilter/filter.go | 60 ++- .../uspfilter/filter_routeacl_test.go | 376 ++++++++++++++++++ client/firewall/uspfilter/filter_test.go | 152 +++++++ client/internal/acl/manager_test.go | 206 ++++++++++ 6 files changed, 791 insertions(+), 68 deletions(-) create mode 100644 client/firewall/uspfilter/filter_routeacl_test.go diff --git a/client/firewall/uspfilter/allow_netbird.go b/client/firewall/uspfilter/allow_netbird.go index 22e6fca1f..6a6533344 100644 --- a/client/firewall/uspfilter/allow_netbird.go +++ b/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.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) - } - } + m.resetState() if m.nativeFirewall != nil { return m.nativeFirewall.Close(stateManager) diff --git a/client/firewall/uspfilter/allow_netbird_windows.go b/client/firewall/uspfilter/allow_netbird_windows.go index 8a56b0862..6aef2ecfd 100644 --- a/client/firewall/uspfilter/allow_netbird_windows.go +++ b/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.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) - } - } + m.resetState() if !isWindowsFirewallReachable() { return nil diff --git a/client/firewall/uspfilter/filter.go b/client/firewall/uspfilter/filter.go index aacc4ca1c..df2e274eb 100644 --- a/client/firewall/uspfilter/filter.go +++ b/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 { diff --git a/client/firewall/uspfilter/filter_routeacl_test.go b/client/firewall/uspfilter/filter_routeacl_test.go new file mode 100644 index 000000000..68572a01c --- /dev/null +++ b/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 +} diff --git a/client/firewall/uspfilter/filter_test.go b/client/firewall/uspfilter/filter_test.go index c6a4ebeb8..55a8e723c 100644 --- a/client/firewall/uspfilter/filter_test.go +++ b/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 }, diff --git a/client/internal/acl/manager_test.go b/client/internal/acl/manager_test.go index 4bc0fd800..bd7adfaef 100644 --- a/client/internal/acl/manager_test.go +++ b/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