From a48618c074d2d072114ea463a72dba66c84094d1 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Tue, 14 Jul 2026 12:12:37 +0200 Subject: [PATCH] [client] Fix forwarder peers never excluded from lazy connections (#6674) * [client] Extract peerRoutesAddr helper in toExcludedLazyPeers Refactor: pull the AllowedIPs match into a named peerRoutesAddr helper and document why forward-target peers are excluded from lazy connections. No behavior change; the existing address match is preserved as-is. * [client] Add failing test for lazy-conn forward-target exclusion toExcludedLazyPeers compares AllowedIPs (CIDR) against the unmasked TranslatedAddress, so forward-target peers are never excluded. This test asserts the peer is excluded and fails on the current behavior; the fix follows. * [client] Fix lazy-conn exclusion for ingress forward peers peerRoutesAddr compared AllowedIPs (CIDR, e.g. a peer's overlay IP as /32) against the unmasked TranslatedAddress string, so the match never fired and forward-target peers were never excluded from lazy connections. Use prefix containment so a routed address matches the peer's AllowedIP * [client] Reuse parsed AllowedIPs from peerStore in lazy exclusion Instead of re-parsing the network map AllowedIPs strings, look up the already-parsed []netip.Prefix from peerStore.AllowedIPs (the same typed value the lazy manager itself consumes). A down/lazy peer still has its conn in the store, so exclusion is unaffected by connection state. Extract a pure prefixesContain helper and unit-test it. --- client/internal/engine.go | 32 ++++++-- client/internal/engine_lazy_exclude_test.go | 87 +++++++++++++++++++++ 2 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 client/internal/engine_lazy_exclude_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index 7b2fc7b26..1d00ed0d2 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -2605,13 +2605,14 @@ func (e *Engine) updateForwardRules(rules []*mgmProto.ForwardingRule) ([]firewal func (e *Engine) toExcludedLazyPeers(rules []firewallManager.ForwardRule, peers []*mgmProto.RemotePeerConfig) map[string]bool { excludedPeers := make(map[string]bool) + + // Ingress forward targets: inbound forwarded traffic is initiated remotely and + // cannot wake a lazy connection, so the peer routing the target must stay + // permanently connected. AllowedIPs are already parsed on the peer conn, so + // reuse those typed prefixes instead of re-parsing the network map strings. for _, r := range rules { - ip := r.TranslatedAddress for _, p := range peers { - for _, allowedIP := range p.GetAllowedIps() { - if allowedIP != ip.String() { - continue - } + if e.peerRoutesAddr(p, r.TranslatedAddress) { log.Infof("exclude forwarder peer from lazy connection: %s", p.GetWgPubKey()) excludedPeers[p.GetWgPubKey()] = true } @@ -2621,6 +2622,27 @@ func (e *Engine) toExcludedLazyPeers(rules []firewallManager.ForwardRule, peers return excludedPeers } +// peerRoutesAddr reports whether the peer is a router for addr, matched against +// the peer's already-parsed AllowedIPs from the store (the same typed value the +// lazy manager consumes) rather than re-parsing the network map strings. +func (e *Engine) peerRoutesAddr(p *mgmProto.RemotePeerConfig, addr netip.Addr) bool { + prefixes, ok := e.peerStore.AllowedIPs(p.GetWgPubKey()) + if !ok { + return false + } + return prefixesContain(prefixes, addr) +} + +// prefixesContain reports whether addr falls within any of the prefixes. +func prefixesContain(prefixes []netip.Prefix, addr netip.Addr) bool { + for _, prefix := range prefixes { + if prefix.Contains(addr) { + return true + } + } + return false +} + // isChecksEqual checks if two slices of checks are equal. func isChecksEqual(checks1, checks2 []*mgmProto.Checks) bool { normalize := func(checks []*mgmProto.Checks) []string { diff --git a/client/internal/engine_lazy_exclude_test.go b/client/internal/engine_lazy_exclude_test.go new file mode 100644 index 000000000..b5ef16c3b --- /dev/null +++ b/client/internal/engine_lazy_exclude_test.go @@ -0,0 +1,87 @@ +package internal + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/require" + + firewallManager "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/client/internal/peerstore" + mgmProto "github.com/netbirdio/netbird/shared/management/proto" +) + +func TestPrefixesContain(t *testing.T) { + tests := []struct { + name string + prefixes []string + addr string + want bool + }{ + {name: "own overlay /32 matches", prefixes: []string{"100.110.8.145/32"}, addr: "100.110.8.145", want: true}, + {name: "addr inside routed subnet", prefixes: []string{"10.121.0.0/16"}, addr: "10.121.208.4", want: true}, + {name: "addr outside subnet", prefixes: []string{"10.121.0.0/16"}, addr: "10.122.0.1", want: false}, + {name: "different /32", prefixes: []string{"100.110.8.145/32"}, addr: "100.110.8.146", want: false}, + {name: "ipv6 /128 matches", prefixes: []string{"fd00::1/128"}, addr: "fd00::1", want: true}, + {name: "no prefixes", prefixes: nil, addr: "10.121.208.4", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prefixes := make([]netip.Prefix, 0, len(tt.prefixes)) + for _, p := range tt.prefixes { + prefixes = append(prefixes, netip.MustParsePrefix(p)) + } + require.Equal(t, tt.want, prefixesContain(prefixes, netip.MustParseAddr(tt.addr))) + }) + } +} + +// TestToExcludedLazyPeers_ForwardTarget guards a regression: the forward-target +// peer (the peer routing a ForwardRule.TranslatedAddress) must be excluded from +// lazy connections, matched via the peer's already-parsed AllowedIPs. +func TestToExcludedLazyPeers_ForwardTarget(t *testing.T) { + const targetPeerKey = "cccccccccccccccccccccccccccccccccccccccccc0=" + const otherPeerKey = "dddddddddddddddddddddddddddddddddddddddddd0=" + + store := peerstore.NewConnStore() + store.AddPeerConn(targetPeerKey, newTestConn(t, targetPeerKey, "100.110.8.145/32")) + store.AddPeerConn(otherPeerKey, newTestConn(t, otherPeerKey, "100.110.9.10/32")) + + e := &Engine{peerStore: store} + + peers := []*mgmProto.RemotePeerConfig{ + {WgPubKey: targetPeerKey, AllowedIps: []string{"100.110.8.145/32"}}, + {WgPubKey: otherPeerKey, AllowedIps: []string{"100.110.9.10/32"}}, + } + rules := []firewallManager.ForwardRule{ + {TranslatedAddress: netip.MustParseAddr("100.110.8.145")}, + } + + excluded := e.toExcludedLazyPeers(rules, peers) + + require.True(t, excluded[targetPeerKey], "forward-target peer must be excluded from lazy connections") + require.False(t, excluded[otherPeerKey], "non-target peer must not be excluded") + require.Len(t, excluded, 1) +} + +func TestToExcludedLazyPeers_NoRules(t *testing.T) { + e := &Engine{peerStore: peerstore.NewConnStore()} + + peers := []*mgmProto.RemotePeerConfig{ + {WgPubKey: "peer-a", AllowedIps: []string{"100.110.8.145/32"}}, + } + + require.Empty(t, e.toExcludedLazyPeers(nil, peers)) +} + +func newTestConn(t *testing.T, key, allowedIP string) *peer.Conn { + t.Helper() + conn, err := peer.NewConn(peer.ConnConfig{ + Key: key, + WgConfig: peer.WgConfig{AllowedIps: []netip.Prefix{netip.MustParsePrefix(allowedIP)}}, + }, peer.ServiceDependencies{}) + require.NoError(t, err) + return conn +}