Apply remote_allow_list before capping addrs at MaxRemotes (#1985)

unlockedSetV4/V6 truncated the reported list to MaxRemotes and only then
ran the allow list check. A host reporting more than MaxRemotes addrs
(docker bridges, temporary v6 addrs, etc) could have every allowed addr
discarded if the denied ones sorted first. Filter the full list and stop
once MaxRemotes allowed addrs are collected. This affects lighthouses
processing HostUpdateNotification and clients processing HostQueryReply.

remote_list.go

@@ -460,10 +460,14 @@ func (r *RemoteList) unlockedSetV4(ownerVpnIp, vpnIp netip.Addr, to []*V4AddrPor
 	// Reset the slice
 	c.reported = c.reported[:0]
 
-	// We can't take their array but we can take their pointers
-	for _, v := range to[:minInt(len(to), MaxRemotes)] {
+	// We can't take their array but we can take their pointers.
+	// Filter before capping so denied addrs do not crowd out allowed ones reported later in the list.
+	for _, v := range to {
 		if check(vpnIp, v) {
 			c.reported = append(c.reported, v)
+			if len(c.reported) == MaxRemotes {
+				break
+			}
 		}
 	}
 }
@@ -508,10 +512,14 @@ func (r *RemoteList) unlockedSetV6(ownerVpnIp, vpnIp netip.Addr, to []*V6AddrPor
 	// Reset the slice
 	c.reported = c.reported[:0]
 
-	// We can't take their array but we can take their pointers
-	for _, v := range to[:minInt(len(to), MaxRemotes)] {
+	// We can't take their array but we can take their pointers.
+	// Filter before capping so denied addrs do not crowd out allowed ones reported later in the list.
+	for _, v := range to {
 		if check(vpnIp, v) {
 			c.reported = append(c.reported, v)
+			if len(c.reported) == MaxRemotes {
+				break
+			}
 		}
 	}
 }

remote_list_test.go

@@ -2,6 +2,7 @@ package nebula
 
 import (
 	"encoding/binary"
+	"fmt"
 	"net/netip"
 	"testing"
 
@@ -346,3 +347,38 @@ func newIp6AndPortFromString(s string) *V6AddrPort {
 		Port: uint32(a.Port()),
 	}
 }
+
+func TestRemoteList_SetFiltersBeforeCap(t *testing.T) {
+	owner := netip.MustParseAddr("0.0.0.0")
+	rl := NewRemoteList([]netip.Addr{owner}, nil)
+
+	// MaxRemotes denied addrs first, then the only allowed one, then enough allowed to exceed the cap
+	var v4 []*V4AddrPort
+	for i := 0; i < MaxRemotes; i++ {
+		v4 = append(v4, newIp4AndPortFromString(fmt.Sprintf("172.17.0.%d:4242", i+1)))
+	}
+	for i := 0; i < MaxRemotes+1; i++ {
+		v4 = append(v4, newIp4AndPortFromString(fmt.Sprintf("70.199.182.%d:4242", i+1)))
+	}
+	rl.unlockedSetV4(owner, owner, v4, func(_ netip.Addr, a *V4AddrPort) bool {
+		return protoV4AddrPortToNetAddrPort(a).Addr().As4()[0] != 172
+	})
+
+	var v6 []*V6AddrPort
+	for i := 0; i < MaxRemotes; i++ {
+		v6 = append(v6, newIp6AndPortFromString(fmt.Sprintf("[fd00::%d]:4242", i+1)))
+	}
+	for i := 0; i < MaxRemotes+1; i++ {
+		v6 = append(v6, newIp6AndPortFromString(fmt.Sprintf("[2001:db8::%d]:4242", i+1)))
+	}
+	rl.unlockedSetV6(owner, owner, v6, func(_ netip.Addr, a *V6AddrPort) bool {
+		return protoV6AddrPortToNetAddrPort(a).Addr().As16()[0] != 0xfd
+	})
+
+	rl.Rebuild([]netip.Prefix{})
+	assert.Len(t, rl.addrs, MaxRemotes*2)
+	for _, a := range rl.addrs {
+		assert.NotEqual(t, byte(172), a.Addr().As16()[12], "denied v4 addr %v was kept", a)
+		assert.NotEqual(t, byte(0xfd), a.Addr().As16()[0], "denied v6 addr %v was kept", a)
+	}
+}