improve lighthouse addrMap correctness for multi-address certs

go.mod

@@ -31,7 +31,7 @@ require (
 	golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2
 	golang.zx2c4.com/wireguard v0.0.0-20230325221338-052af4a8072b
 	golang.zx2c4.com/wireguard/windows v1.0.1
-	google.golang.org/protobuf v1.36.12
+	google.golang.org/protobuf v1.36.11
 	gopkg.in/yaml.v3 v3.0.1
 	gvisor.dev/gvisor v0.0.0-20240423190808-9d7a357edefe
 )

go.sum

@@ -234,8 +234,8 @@ google.golang.org/protobuf v1.20.1-0.20200309200217-e05f789c0967/go.mod h1:A+miE
 google.golang.org/protobuf v1.21.0/go.mod h1:47Nbq4nVaFHyn7ilMalzfO3qCViNmqZ2kzikPIcrTAo=
 google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU=
 google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
-google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc=
-google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco=
+google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE=
+google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

handshake_manager.go

@@ -807,7 +807,7 @@ func (hm *HandshakeManager) beginHandshake(via ViaSender, packet []byte, h *head
 	}
 
 	hm.sendHandshakeResponse(via, response, hostinfo, false)
-	hostinfo.remotes.RefreshFromHandshake(vpnAddrs)
+	hostinfo.remotes.RefreshFromHandshake(vpnAddrs, remoteCert.Certificate.Version())
 
 	// Don't wait for UpdateWorker
 	if f.lightHouse.IsAnyLighthouseAddr(vpnAddrs) {
@@ -995,7 +995,7 @@ func (hm *HandshakeManager) continueHandshake(via ViaSender, hh *HandshakeHostIn
 		f.cachedPacketMetrics.sent.Inc(int64(len(hh.packetStore)))
 	}
 
-	hostinfo.remotes.RefreshFromHandshake(vpnAddrs)
+	hostinfo.remotes.RefreshFromHandshake(vpnAddrs, remoteCert.Certificate.Version())
 	f.metricHandshakes.Update(duration)
 
 	// Don't wait for UpdateWorker

lighthouse.go

@@ -34,9 +34,7 @@ type LightHouse struct {
 
 	myVpnNetworks      []netip.Prefix
 	myVpnNetworksTable *bart.Lite
-	// myVpnAddrsTable contains our overlay host addrs, as opposed to the overlay networks
-	myVpnAddrsTable *bart.Lite
-	punchy          *Punchy
+	punchy             *Punchy
 
 	// localAddrsFn enumerates the underlay addresses we advertise. It is a field so tests can supply simulated
 	// addresses rather than whatever this machine's NICs happen to be. Set it before Start.
@@ -106,7 +104,6 @@ func NewLightHouseFromConfig(ctx context.Context, l *slog.Logger, c *config.C, c
 		amLighthouse:       amLighthouse,
 		myVpnNetworks:      cs.myVpnNetworks,
 		myVpnNetworksTable: cs.myVpnNetworksTable,
-		myVpnAddrsTable:    cs.myVpnAddrsTable,
 		addrMap:            make(map[netip.Addr]*RemoteList),
 		nebulaPort:         nebulaPort,
 		punchy:             p,
@@ -516,17 +513,15 @@ func (lh *LightHouse) QueryServer(vpnAddr netip.Addr) {
 }
 
 func (lh *LightHouse) QueryCache(vpnAddrs []netip.Addr) *RemoteList {
-	lh.RLock()
-	if v, ok := lh.addrMap[vpnAddrs[0]]; ok {
-		lh.RUnlock()
-		return v
+	rl, ok := lh.findRemoteList(vpnAddrs)
+	if ok {
+		return rl
 	}
-	lh.RUnlock()
 
 	lh.Lock()
 	defer lh.Unlock()
 	// Add an entry if we don't already have one
-	return lh.unlockedGetRemoteList(vpnAddrs) //todo CERT-V2 this contains addrmap lookups we could potentially skip
+	return lh.unlockedGetRemoteList(vpnAddrs) //todo this re-calls unlockedFindRemoteList
 }
 
 // queryAndPrepMessage is a lock helper on RemoteList, assisting the caller to build a lighthouse message containing
@@ -672,23 +667,58 @@ func (lh *LightHouse) addCalculatedRemotes(vpnAddr netip.Addr) bool {
 	return len(calculatedV4) > 0 || len(calculatedV6) > 0
 }
 
+func (lh *LightHouse) findRemoteList(vpnAddrs []netip.Addr) (*RemoteList, bool) {
+	lh.RLock()
+	defer lh.RUnlock()
+	return lh.unlockedFindRemoteList(vpnAddrs)
+}
+
+// unlockedFindRemoteList checks addrMap for each of vpnAddrs. It returns the first RemoteList found,
+// and true if that RemoteList is present for all vpnAddrs.
+// If false, it means the addrMap, and possibly the RemoteList, need to be corrected.
+func (lh *LightHouse) unlockedFindRemoteList(vpnAddrs []netip.Addr) (*RemoteList, bool) {
+	var am *RemoteList
+	//todo: if a host with addresses A and B is "split", so it only has address A and a new host has only address B
+	//todo: we don't handly that correctly, I'm pretty sure.
+	missingOrDifferent := false
+	for _, addr := range vpnAddrs {
+		found, ok := lh.addrMap[addr]
+		if !ok {
+			missingOrDifferent = true
+		} else if am == nil {
+			am = found //the first list we find wins
+		} else if am != found {
+			missingOrDifferent = true
+		}
+	}
+	return am, !missingOrDifferent
+}
+
 // unlockedGetRemoteList assumes you have the lh lock
 func (lh *LightHouse) unlockedGetRemoteList(allAddrs []netip.Addr) *RemoteList {
 	// before we go and make a new remotelist, we need to make sure we don't have one for any of this set of vpnaddrs yet
-	for i, addr := range allAddrs {
-		am, ok := lh.addrMap[addr]
-		if ok {
-			if i != 0 {
-				lh.addrMap[allAddrs[0]] = am
-			}
-			return am
+	am, ok := lh.unlockedFindRemoteList(allAddrs)
+
+	// we failed to find any RemoteLists: make a new one, fill out the addrMap
+	if am == nil {
+		am = NewRemoteList(allAddrs, lh.shouldAdd)
+		for _, addr := range allAddrs {
+			lh.addrMap[addr] = am
+		}
+		return am
+	}
+
+	// we found one! Do we need to fix it?
+	if !ok {
+		am.Lock()
+		am.vpnAddrs = make([]netip.Addr, len(allAddrs))
+		copy(am.vpnAddrs, allAddrs)
+		am.Unlock()
+		for _, addr := range allAddrs {
+			lh.addrMap[addr] = am
 		}
 	}
 
-	am := NewRemoteList(allAddrs, lh.shouldAdd)
-	for _, addr := range allAddrs {
-		lh.addrMap[addr] = am
-	}
 	return am
 }
 
@@ -1161,17 +1191,6 @@ func (lhh *LightHouseHandler) handleHostQuery(n *NebulaMeta, fromVpnAddrs []neti
 		return
 	}
 
-	// Don't respond to requests for us.
-	if lhh.lh.myVpnAddrsTable.Contains(queryVpnAddr) {
-		if lhh.l.Enabled(context.Background(), slog.LevelDebug) {
-			lhh.l.Debug("Ignoring HostQuery for one of my own addresses",
-				"fromVpnAddrs", fromVpnAddrs,
-				"queryVpnAddr", queryVpnAddr,
-			)
-		}
-		return
-	}
-
 	found, ln, err := lhh.lh.queryAndPrepMessage(queryVpnAddr, func(c *cache) (int, error) {
 		n = lhh.resetMeta()
 		n.Type = NebulaMeta_HostQueryReply

lighthouse_test.go

@@ -27,27 +27,15 @@ func TestOldIPv4Only(t *testing.T) {
 	assert.Equal(t, binary.BigEndian.Uint32(bp[:]), m.GetAddr())
 }
 
-func testCertState(networks ...netip.Prefix) *CertState {
-	cs := &CertState{
-		myVpnNetworks:      networks,
-		myVpnNetworksTable: new(bart.Lite),
-		myVpnAddrs:         make([]netip.Addr, 0, len(networks)),
-		myVpnAddrsTable:    new(bart.Lite),
-	}
-
-	for _, n := range networks {
-		cs.myVpnNetworksTable.Insert(n)
-		cs.myVpnAddrs = append(cs.myVpnAddrs, n.Addr())
-		cs.myVpnAddrsTable.Insert(netip.PrefixFrom(n.Addr(), n.Addr().BitLen()))
-	}
-
-	return cs
-}
-
 func Test_lhStaticMapping(t *testing.T) {
 	l := test.NewLogger()
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/16")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 	lh1 := "10.128.0.2"
 
 	c := config.NewC(l)
@@ -67,7 +55,12 @@ func Test_lhStaticMapping(t *testing.T) {
 func TestReloadLighthouseInterval(t *testing.T) {
 	l := test.NewLogger()
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/16")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 	lh1 := "10.128.0.2"
 
 	c := config.NewC(l)
@@ -97,7 +90,12 @@ func TestReloadLighthouseInterval(t *testing.T) {
 func BenchmarkLighthouseHandleRequest(b *testing.B) {
 	l := test.NewLogger()
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/0")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 
 	c := config.NewC(l)
 	lh, err := NewLightHouseFromConfig(b.Context(), l, c, cs, nil, nil)
@@ -197,7 +195,12 @@ func TestLighthouse_Memory(t *testing.T) {
 	c.Settings["listen"] = map[string]any{"port": 4242}
 
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
 	lh.ifce = &mockEncWriter{}
 	require.NoError(t, err)
@@ -277,7 +280,12 @@ func TestLighthouse_reload(t *testing.T) {
 	c.Settings["listen"] = map[string]any{"port": 4242}
 
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 
 	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
 	require.NoError(t, err)
@@ -307,7 +315,12 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) {
 	}
 
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 
 	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
 	require.NoError(t, err)
@@ -416,9 +429,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) {
 	assert.Equal(t, []netip.AddrPort{netip.MustParseAddrPort("3.3.3.3:4242")}, rl.CopyAddrs([]netip.Prefix{}))
 }
 
-// sendLHHostRequest delivers a HostQuery to lhh and hands back the writer that
-// captured what it emitted. Pass a nil filter to see every message.
-func sendLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr, lhh *LightHouseHandler, filter *NebulaMeta_MessageType) *testEncWriter {
+func newLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr, lhh *LightHouseHandler) testLhReply {
 	req := &NebulaMeta{
 		Type:    NebulaMeta_HostQuery,
 		Details: &NebulaMetaDetails{},
@@ -436,59 +447,12 @@ func sendLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr,
 		panic(err)
 	}
 
-	w := &testEncWriter{metaFilter: filter}
-	lhh.HandleRequest(fromAddr, []netip.Addr{myVpnIp}, b, w)
-	return w
-}
-
-func newLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr, lhh *LightHouseHandler) testLhReply {
 	filter := NebulaMeta_HostQueryReply
-	return sendLHHostRequest(fromAddr, myVpnIp, queryVpnIp, lhh, &filter).lastReply
-}
-
-func TestLighthouse_IgnoresHostQueryForItself(t *testing.T) {
-	// Validate that we don't answer host queries for our own address.
-	l := test.NewLogger()
-
-	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	myVpnIp := myVpnNet.Addr()
-
-	c := config.NewC(l)
-	c.Settings["lighthouse"] = map[string]any{"am_lighthouse": true}
-	c.Settings["listen"] = map[string]any{"port": 4242}
-	// Add a static_host_map entry for ourselves, so our address
-	// is in the addrMap.
-	c.Settings["static_host_map"] = map[string]any{
-		myVpnIp.String(): []any{"192.168.100.1:4242"},
+	w := &testEncWriter{
+		metaFilter: &filter,
 	}
-
-	lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(myVpnNet), nil, nil)
-	require.NoError(t, err)
-	lh.ifce = &mockEncWriter{}
-	lhh := lh.NewRequestHandler()
-
-	peerVpnIp := netip.MustParseAddr("10.128.0.2")
-	peerUdpAddr := netip.MustParseAddrPort("10.0.0.2:4242")
-	otherVpnIp := netip.MustParseAddr("10.128.0.3")
-	otherUdpAddr := netip.MustParseAddrPort("10.0.0.3:4242")
-
-	newLHHostUpdate(peerUdpAddr, peerVpnIp, []netip.AddrPort{peerUdpAddr}, lhh)
-	newLHHostUpdate(otherUdpAddr, otherVpnIp, []netip.AddrPort{otherUdpAddr}, lhh)
-
-	// Control: a query about a real peer is still answered, and still ends with
-	// the punch notification aimed at the host that was asked about.
-	w := sendLHHostRequest(peerUdpAddr, peerVpnIp, otherVpnIp, lhh, nil)
-	require.NotNil(t, w.lastReply.msg)
-	assert.Equal(t, NebulaMeta_HostPunchNotification, w.lastReply.msg.Type)
-	assert.Equal(t, otherVpnIp, w.lastReply.vpnIp)
-
-	// Now validate that we don't send to ourselves.
-	found, _, err := lh.queryAndPrepMessage(myVpnIp, func(*cache) (int, error) { return 0, nil })
-	require.NoError(t, err)
-	require.True(t, found, "the lighthouse should hold a cache entry for its own address")
-
-	w = sendLHHostRequest(peerUdpAddr, peerVpnIp, myVpnIp, lhh, nil)
-	assert.Nil(t, w.lastReply.msg, "a query about our own address must produce no reply and no punch notification")
+	lhh.HandleRequest(fromAddr, []netip.Addr{myVpnIp}, b, w)
+	return w.lastReply
 }
 
 func newLHHostUpdate(fromAddr netip.AddrPort, vpnIp netip.Addr, addrs []netip.AddrPort, lhh *LightHouseHandler) {
@@ -678,7 +642,12 @@ func TestLighthouse_Dont_Delete_Static_Hosts(t *testing.T) {
 	}
 
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
 	require.NoError(t, err)
 	lh.ifce = &mockEncWriter{}
@@ -739,7 +708,12 @@ func TestLighthouse_DeletesWork(t *testing.T) {
 	}
 
 	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
-	cs := testCertState(myVpnNet)
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet},
+		myVpnNetworksTable: nt,
+	}
 	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
 	require.NoError(t, err)
 	lh.ifce = &mockEncWriter{}
@@ -764,3 +738,123 @@ func TestLighthouse_DeletesWork(t *testing.T) {
 	out = lh.Query(testHost)
 	assert.Nil(t, out)
 }
+
+// newLHHostUpdateV2 sends a v2-style HostUpdateNotification where the sending tunnel carries
+// multiple vpn addrs (a dual-stack v2 cert). Details.VpnAddr is left blank like SendUpdate does.
+func newLHHostUpdateV2(fromAddr netip.AddrPort, vpnAddrs []netip.Addr, addrs []netip.AddrPort, lhh *LightHouseHandler) {
+	req := &NebulaMeta{
+		Type:    NebulaMeta_HostUpdateNotification,
+		Details: &NebulaMetaDetails{},
+	}
+	for _, v := range addrs {
+		if v.Addr().Is4() {
+			req.Details.V4AddrPorts = append(req.Details.V4AddrPorts, netAddrToProtoV4AddrPort(v.Addr(), v.Port()))
+		} else {
+			req.Details.V6AddrPorts = append(req.Details.V6AddrPorts, netAddrToProtoV6AddrPort(v.Addr(), v.Port()))
+		}
+	}
+	b, err := req.Marshal()
+	if err != nil {
+		panic(err)
+	}
+	lhh.HandleRequest(fromAddr, vpnAddrs, b, &testEncWriter{})
+}
+
+func newIssue1868Lighthouse(t *testing.T) (*LightHouse, *LightHouseHandler) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+	c.Settings["lighthouse"] = map[string]any{"am_lighthouse": true}
+	c.Settings["listen"] = map[string]any{"port": 4242}
+
+	myVpnNet4 := netip.MustParsePrefix("10.128.0.1/24")
+	myVpnNet6 := netip.MustParsePrefix("fd00::1/64")
+	nt := new(bart.Lite)
+	nt.Insert(myVpnNet4)
+	nt.Insert(myVpnNet6)
+	cs := &CertState{
+		myVpnNetworks:      []netip.Prefix{myVpnNet4, myVpnNet6},
+		myVpnNetworksTable: nt,
+	}
+	lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil)
+	require.NoError(t, err)
+	lh.ifce = &mockEncWriter{}
+	return lh, lh.NewRequestHandler()
+}
+
+// Scenario A: host registers via v2 (both addrs), then a v1 handshake with the same host completes on
+// the lighthouse (rehandshake after cert renewal, relay-initiated handshake, traffic to the LH's v4
+// addr...). handshake_manager does QueryCache(vpnAddrs) + RefreshFromHandshake(vpnAddrs) with the v1
+// cert's single address, which truncates RemoteList.vpnAddrs.
+func TestLighthouse_Issue1868_V1HandshakeTruncatesVpnAddrs(t *testing.T) {
+	lh, lhh := newIssue1868Lighthouse(t)
+
+	hostV4 := netip.MustParseAddr("10.128.0.3")
+	hostV6 := netip.MustParseAddr("fd00::3")
+	hostUdp := netip.MustParseAddrPort("192.0.2.3:4242")
+	hostLan := netip.MustParseAddrPort("10.0.0.3:4242")
+
+	askerV4 := netip.MustParseAddr("10.128.0.2")
+	askerUdp := netip.MustParseAddrPort("192.0.2.2:4242")
+
+	// Boot: host handshakes with the LH using its v2 cert and sends an update
+	newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh)
+
+	// Both addresses resolve
+	r := newLHHostRequest(askerUdp, askerV4, hostV4, lhh)
+	require.NotNil(t, r.msg, "v4 query should be answered")
+	assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+
+	r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh)
+	require.NotNil(t, r.msg, "v6 query should be answered before the v1 handshake")
+	assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+
+	// Later: a v1 handshake with the same host completes on the LH. This is exactly what
+	// handshake_manager.go does on completion, with the v1 cert's single vpn addr.
+	rl := lh.QueryCache([]netip.Addr{hostV4})
+	rl.RefreshFromHandshake([]netip.Addr{hostV4}, cert.Version1)
+
+	// The host keeps sending v2 updates over its v2 tunnel too
+	newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh)
+
+	r = newLHHostRequest(askerUdp, askerV4, hostV4, lhh)
+	require.NotNil(t, r.msg, "v4 query should still be answered")
+	assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+
+	r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh)
+	if assert.NotNil(t, r.msg, "BUG: v6 query is silently dropped after a v1 handshake truncated RemoteList.vpnAddrs") {
+		assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+	}
+}
+
+// Scenario B: the LH first creates a RemoteList for the host keyed only by its v4 addr (a pending
+// LH-initiated v1 handshake does QueryCache([v4]) in handleOutbound), then the host arrives with v2.
+// unlockedGetRemoteList/QueryCache hit on allAddrs[0] and never add the v6 key to addrMap.
+func TestLighthouse_Issue1868_V4OnlyListNeverGainsV6Key(t *testing.T) {
+	lh, lhh := newIssue1868Lighthouse(t)
+
+	hostV4 := netip.MustParseAddr("10.128.0.3")
+	hostV6 := netip.MustParseAddr("fd00::3")
+	hostUdp := netip.MustParseAddrPort("192.0.2.3:4242")
+	hostLan := netip.MustParseAddrPort("10.0.0.3:4242")
+
+	askerV4 := netip.MustParseAddr("10.128.0.2")
+	askerUdp := netip.MustParseAddrPort("192.0.2.2:4242")
+
+	// LH is a relay and someone asked it to relay to hostV4 while the host was offline:
+	// StartHandshake(hostV4) -> handleOutbound -> QueryCache([hostV4]) creates a v4-only list.
+	_ = lh.QueryCache([]netip.Addr{hostV4})
+
+	// Host boots and handshakes v2 with the LH (responder path does QueryCache + RefreshFromHandshake)
+	rl := lh.QueryCache([]netip.Addr{hostV4, hostV6})
+	rl.RefreshFromHandshake([]netip.Addr{hostV4, hostV6}, cert.Version2)
+	newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh)
+
+	r := newLHHostRequest(askerUdp, askerV4, hostV4, lhh)
+	require.NotNil(t, r.msg, "v4 query should be answered")
+	assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+
+	r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh)
+	if assert.NotNil(t, r.msg, "BUG: v6 query is silently dropped, addrMap never got the v6 key") {
+		assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan)
+	}
+}

remote_list.go

@@ -11,6 +11,8 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+
+	"github.com/slackhq/nebula/cert"
 )
 
 // forEachFunc is used to benefit folks that want to do work inside the lock
@@ -408,11 +410,15 @@ func (r *RemoteList) CopyBlockedRemotes() []netip.AddrPort {
 }
 
 // RefreshFromHandshake locks and updates the RemoteList to account for data learned upon a completed handshake
-func (r *RemoteList) RefreshFromHandshake(vpnAddrs []netip.Addr) {
+func (r *RemoteList) RefreshFromHandshake(vpnAddrs []netip.Addr, v cert.Version) {
 	r.Lock()
 	r.badRemotes = nil
-	r.vpnAddrs = make([]netip.Addr, len(vpnAddrs))
-	copy(r.vpnAddrs, vpnAddrs)
+	if v != cert.Version1 {
+		// a handshake from a v1 cert can never expand our knowledge of the number of addresses a host has,
+		// and, because v2 certs exist, it can also never contract it. So, only update this for non-v1 certs.
+		r.vpnAddrs = make([]netip.Addr, len(vpnAddrs))
+		copy(r.vpnAddrs, vpnAddrs)
+	}
 	r.Unlock()
 }