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

client/internal/acl/legacy_fallback_test.go

@@ -0,0 +1,75 @@
+package acl
+
+import (
+	"net/netip"
+	"sync"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"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

@@ -225,12 +225,21 @@ func (d *DefaultManager) installPeerGroups(groups []*peerRuleGroup, newRulePairs
 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)
 			}
 		}
-		delete(d.peerRulesPairs, pairID)
+		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)
@@ -264,7 +273,7 @@ func (d *DefaultManager) applyPeerGroup(g *peerRuleGroup) (id.RuleID, []firewall
 		}
 		fwRule, err = d.firewall.AddFilterRule(g.policyID, g.sources, firewall.Network{}, protocol, port, nil, action)
 	default:
-		return "", nil, fmt.Errorf("invalid direction, skipping firewall rule")
+		return "", nil, errors.New("invalid direction")
 	}
 
 	if err != nil {
@@ -483,9 +492,15 @@ func extractRuleSources(r *mgmProto.FirewallRule) ([]netip.Prefix, error) {
 	//nolint:staticcheck // PeerIP used for backward compatibility with old management
 	addr, err := netip.ParseAddr(r.PeerIP)
 	if err != nil {
-		return nil, fmt.Errorf("invalid IP address, skipping firewall rule")
+		return nil, fmt.Errorf("parse peer IP %q: %w", r.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
 }
 

client/internal/engine.go

@@ -240,7 +240,7 @@ type Engine struct {
 	syncStore    syncstore.Store
 	syncStoreDir string
 
-	flowManager         nftypes.FlowManager
+	flowManager nftypes.FlowManager
 
 	// auto-update
 	updateManager *updater.Manager
@@ -639,6 +639,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()
 	}

client/ssh/server/test.go

@@ -1,9 +1,9 @@
 // This file is intentionally named test.go (not test_test.go) so the exported
 // StartTestServer helper is visible to the ssh/proxy and ssh/client external
 // test packages, not just this package's own tests. The //go:build !js tag
-// keeps its "testing" import — and the whole testing/flag/regexp transitive
-// chain it drags in — out of the wasm client, which links ssh/server through
-// the engine but never runs Go tests under GOOS=js.
+// keeps its "testing" import, along with the whole testing/flag/regexp
+// transitive chain it drags in, out of the wasm client, which links
+// ssh/server through the engine but never runs Go tests under GOOS=js.
 //go:build !js
 
 package server