diff --git a/client/internal/acl/legacy_fallback_test.go b/client/internal/acl/legacy_fallback_test.go new file mode 100644 index 000000000..b51bf6442 --- /dev/null +++ b/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) + } + } +} diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index 0ff2b5276..58fe330ee 100644 --- a/client/internal/acl/manager.go +++ b/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 } diff --git a/client/internal/engine.go b/client/internal/engine.go index e4ca59ae8..98ab7a216 100644 --- a/client/internal/engine.go +++ b/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() } diff --git a/client/ssh/server/test.go b/client/ssh/server/test.go index e2be0551c..7ca28f034 100644 --- a/client/ssh/server/test.go +++ b/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