Attach a pending handshake's RemoteList when the handshake starts (#1909)

StartHandshake returned a hostinfo with no RemoteList; handleOutbound attached
one lazily on its first tick. Anything that touched the pending host before
then, notably create-tunnel -address, dereferenced nil and took the daemon
down. StartHandshake now attaches the lighthouse's list as it builds the
hostinfo. To keep the surrounding locking sound, a lighthouse now resolves a
punch target before taking the querier's RemoteList lock, and closeTunnel
deletes the host and evicts its RemoteList under a single hostmap write lock so
a completing handshake cannot land in between. The lock order, now documented
on the handshake manager mutex, is hostmap, then handshake manager, then
lighthouse, then RemoteList; no RemoteList lock is ever held across a hostmap
lookup.

CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- `create-tunnel -address <ip:port>` panicked on a nil RemoteList and, over the ssh debug server, took nebula
+  down with it. A hostinfo returned by StartHandshake had no RemoteList until the handshake loop's first
+  pass attached one, and `create-tunnel -address` set the remote before that. The list is now attached when
+  the handshake starts, so a pending hostinfo never exists without one.
+
 ## [1.11.1] - 2026-08-21
 
 See the [v1.11.1](https://github.com/slackhq/nebula/milestone/30?closed=1) milestone for a complete list of changes.

connection_manager.go

@@ -174,10 +174,8 @@ func (cm *connectionManager) doTrafficCheck(localIndex uint32, p, nb, out []byte
 
 	switch decision {
 	case deleteTunnel:
-		if cm.hostMap.DeleteHostInfo(hostinfo) {
-			// Only clearing the lighthouse cache if this is the last hostinfo for this vpn ip in the hostmap
-			cm.intf.lightHouse.DeleteVpnAddrs(hostinfo.vpnAddrs)
-		}
+		// Local delete only, clearing the lighthouse cache if this was the last hostinfo for the vpn addr
+		cm.intf.closeTunnel(hostinfo)
 
 	case closeTunnel:
 		cm.intf.sendCloseTunnel(hostinfo)

handshake_manager.go

@@ -54,7 +54,8 @@ type HandshakeConfig struct {
 }
 
 type HandshakeManager struct {
-	// Mutex for interacting with the vpnIps and indexes maps
+	// Mutex for interacting with the vpnIps and indexes maps.
+	// Lock order is hostmap, then this lock, then the lighthouse, then a RemoteList.
 	sync.RWMutex
 
 	vpnIps  map[netip.Addr]*HandshakeHostInfo
@@ -259,14 +260,6 @@ func (hm *HandshakeManager) handleOutbound(vpnIp netip.Addr, lighthouseTriggered
 		"style": header.SubTypeName(header.Handshake, hh.machine.Subtype()),
 	}
 
-	// Get a remotes object if we don't already have one.
-	// This is mainly to protect us as this should never be the case
-	// NB ^ This comment doesn't jive. It's how the thing gets initialized.
-	// It's the common path. Should it update every time, in case a future LH query/queries give us more info?
-	if hostinfo.remotes == nil {
-		hostinfo.remotes = hm.lightHouse.QueryCache([]netip.Addr{vpnIp})
-	}
-
 	remotes := hostinfo.remotes.CopyAddrs(hm.mainHostMap.GetPreferredRanges())
 	remotesHaveChanged := !slices.Equal(remotes, hh.lastRemotes)
 
@@ -371,6 +364,9 @@ func (hm *HandshakeManager) StartHandshake(vpnAddr netip.Addr, cacheCb func(*Han
 	hostinfo := &HostInfo{
 		vpnAddrs:        []netip.Addr{vpnAddr},
 		HandshakePacket: make(map[uint8][]byte, 0),
+		// Attached here so a pending hostinfo never exists without one: SetRemote dereferences
+		// it, and create-tunnel -address can reach SetRemote before the handshake loop runs.
+		remotes: hm.lightHouse.QueryCache([]netip.Addr{vpnAddr}),
 		relayState: RelayState{
 			relays:         nil,
 			relayForByAddr: map[netip.Addr]*Relay{},
@@ -946,13 +942,14 @@ func (hm *HandshakeManager) continueHandshake(via ViaSender, hh *HandshakeHostIn
 
 		hm.DeleteHostInfo(hostinfo)
 		hm.StartHandshake(hostinfo.vpnAddrs[0], func(newHH *HandshakeHostInfo) {
-			newHH.hostinfo.remotes = hostinfo.remotes
 			newHH.hostinfo.remotes.BlockRemote(via)
 			newHH.packetStore = hh.packetStore
 			hh.packetStore = []*cachedPacket{}
-			hostinfo.vpnAddrs = vpnAddrs
-			f.sendCloseTunnel(hostinfo)
 		})
+		// sendCloseTunnel may take the hostmap lock, so it must not run under the handshake
+		// manager lock that StartHandshake holds around the callback.
+		hostinfo.vpnAddrs = vpnAddrs
+		f.sendCloseTunnel(hostinfo)
 		return
 	}
 

handshake_manager_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/slackhq/nebula/test"
 	"github.com/slackhq/nebula/udp"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_NewHandshakeManagerVpnIp(t *testing.T) {
@@ -42,8 +43,6 @@ func Test_NewHandshakeManagerVpnIp(t *testing.T) {
 	i2 := blah.StartHandshake(ip, nil)
 	assert.Same(t, i, i2)
 
-	i.remotes = NewRemoteList([]netip.Addr{}, nil)
-
 	// Adding something to pending should not affect the main hostmap
 	assert.Empty(t, mainHM.Hosts)
 
@@ -235,3 +234,70 @@ func TestHandleIncomingDispatch(t *testing.T) {
 		assert.Empty(t, hm.indexes, "orphan stage-2 must not create state")
 	})
 }
+
+// Test_PendingHandshakeHasRemotes covers create-tunnel -address: a pending hostinfo must already
+// carry its RemoteList when StartHandshake runs its callback, so SetRemote there, before the
+// handshake loop has run, is safe and the address becomes one the handshake is sent to.
+func Test_PendingHandshakeHasRemotes(t *testing.T) {
+	l := test.NewLogger()
+	mainHM := newHostMap(l)
+	preferredRanges := []netip.Prefix{}
+	mainHM.preferredRanges.Store(&preferredRanges)
+
+	hm := NewHandshakeManager(l, mainHM, newTestLighthouse(), &udp.NoopConn{}, defaultHandshakeConfig)
+
+	vpnIp := netip.MustParseAddr("172.1.1.2")
+	remote := netip.MustParseAddrPort("192.168.1.9:4242")
+
+	hi := hm.StartHandshake(vpnIp, func(hh *HandshakeHostInfo) {
+		require.NotNil(t, hh.hostinfo.remotes, "a pending handshake must never be without its RemoteList")
+		hh.hostinfo.SetRemote(remote)
+	})
+	assert.Contains(t, hi.remotes.CopyAddrs(preferredRanges), remote, "the handshake must be sent to the supplied remote")
+	assert.Equal(t, remote, hi.GetRemote())
+
+	// A RemoteList keeps one learned address per host, so a second remote replaces the
+	// first — the same behaviour change-remote has on an established tunnel.
+	other := netip.MustParseAddrPort("192.168.1.10:4242")
+	hi.SetRemote(other)
+	assert.Equal(t, []netip.AddrPort{other}, hi.remotes.CopyAddrs(preferredRanges))
+	assert.Equal(t, other, hi.GetRemote())
+}
+
+// newRemotesRaceFixture builds a handshake manager with an established tunnel to vpnAddr whose
+// RemoteList is the lighthouse's cached list, the state every teardown-during-handshake race starts from.
+func newRemotesRaceFixture(t *testing.T, vpnAddr netip.Addr) (*HandshakeManager, *Interface, *HostInfo) {
+	t.Helper()
+	l := test.NewLogger()
+	mainHM := newHostMap(l)
+	preferredRanges := []netip.Prefix{}
+	mainHM.preferredRanges.Store(&preferredRanges)
+
+	lh := newTestLighthouse()
+	hm := NewHandshakeManager(l, mainHM, lh, &udp.NoopConn{}, defaultHandshakeConfig)
+	f := &Interface{hostMap: mainHM, lightHouse: lh, handshakeManager: hm, l: l}
+	hm.f = f
+
+	established := &HostInfo{
+		vpnAddrs:        []netip.Addr{vpnAddr},
+		localIndexId:    1,
+		remoteIndexId:   101,
+		remotes:         lh.QueryCache([]netip.Addr{vpnAddr}),
+		HandshakePacket: map[uint8][]byte{},
+	}
+	mainHM.Lock()
+	mainHM.unlockedAddHostInfo(established, f)
+	mainHM.Unlock()
+
+	return hm, f, established
+}
+
+// With no handshake pending, tearing down the last tunnel still evicts the list.
+func Test_CloseTunnelEvictsWithoutPendingHandshake(t *testing.T) {
+	vpnAddr := netip.MustParseAddr("172.1.1.2")
+	_, f, established := newRemotesRaceFixture(t, vpnAddr)
+
+	f.closeTunnel(established)
+	assert.NotContains(t, f.hostMap.Hosts, vpnAddr)
+	assert.NotContains(t, f.lightHouse.addrMap, vpnAddr)
+}

lighthouse.go

@@ -1244,10 +1244,13 @@ func (lhh *LightHouseHandler) handleHostQuery(n *NebulaMeta, fromVpnAddrs []neti
 // sendHostPunchNotification signals the other side to punch some zero byte udp packets
 func (lhh *LightHouseHandler) sendHostPunchNotification(n *NebulaMeta, fromVpnAddrs []netip.Addr, punchNotifDest netip.Addr, w EncWriter) {
 	whereToPunch := fromVpnAddrs[0]
+	// Resolve the target before taking the querier's RemoteList lock below. GetHostInfo takes the
+	// hostmap lock, and the hostmap is taken before the lighthouse, which is taken before a
+	// RemoteList, so looking it up inside the callback would deadlock.
+	targetHI := lhh.lh.ifce.GetHostInfo(punchNotifDest)
 	found, ln, err := lhh.lh.queryAndPrepMessage(whereToPunch, func(c *cache) (int, error) {
 		n = lhh.resetMeta()
 		n.Type = NebulaMeta_HostPunchNotification
-		targetHI := lhh.lh.ifce.GetHostInfo(punchNotifDest)
 		var useVersion cert.Version
 		if targetHI == nil {
 			useVersion = lhh.lh.ifce.GetCertState().initiatingVersion

lighthouse_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/slackhq/nebula/config"
 	"github.com/slackhq/nebula/header"
 	"github.com/slackhq/nebula/test"
+	"github.com/slackhq/nebula/udp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.yaml.in/yaml/v3"
@@ -849,3 +850,108 @@ func TestLighthouse_QueryServerDoesNotBlock(t *testing.T) {
 	}
 	assert.EqualValues(t, 1, lh.queryDropped.Count()-dropped)
 }
+
+// hostmapEncWriter answers GetHostInfo from a real hostmap, the way Interface does.
+type hostmapEncWriter struct {
+	mockEncWriter
+	hm *HostMap
+}
+
+func (w *hostmapEncWriter) GetHostInfo(a netip.Addr) *HostInfo { return w.hm.QueryVpnAddr(a) }
+func (w *hostmapEncWriter) GetCertState() *CertState {
+	return &CertState{initiatingVersion: cert.Version1}
+}
+
+// A lighthouse runs four paths that each hold one lock while waiting for the next: CheckAndComplete
+// holds the hostmap and takes the handshake manager, StartHandshake holds the handshake manager and
+// takes the lighthouse, a host update holds the lighthouse and takes the sender's RemoteList, and a
+// punch notification holds the querier's RemoteList while it resolves the target. If that last step
+// reaches the hostmap under the RemoteList lock, the four deadlock. Drive them all at once.
+func TestLighthouse_PunchNotificationDoesNotCloseLockCycle(t *testing.T) {
+	l := test.NewLogger()
+	myVpnNet := netip.MustParsePrefix("10.128.0.1/24")
+
+	c := config.NewC(l)
+	c.Settings["lighthouse"] = map[string]any{"am_lighthouse": true}
+	c.Settings["listen"] = map[string]any{"port": 4242}
+	lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(myVpnNet), nil, nil)
+	require.NoError(t, err)
+
+	mainHM := newHostMap(l)
+	pr := []netip.Prefix{}
+	mainHM.preferredRanges.Store(&pr)
+	lh.ifce = &hostmapEncWriter{hm: mainHM}
+
+	hm := NewHandshakeManager(l, mainHM, lh, &udp.NoopConn{}, defaultHandshakeConfig)
+	f := &Interface{hostMap: mainHM, lightHouse: lh, handshakeManager: hm, l: l}
+	hm.f = f
+
+	peer := netip.MustParseAddr("10.128.0.2")
+	peerUdp := netip.MustParseAddrPort("10.0.0.2:4242")
+	target := netip.MustParseAddr("10.128.0.3")
+	targetUdp := netip.MustParseAddrPort("10.0.0.3:4242")
+	outbound := netip.MustParseAddr("10.128.0.4")
+	inbound := netip.MustParseAddr("10.128.0.5")
+
+	seed := lh.NewRequestHandler()
+	newLHHostUpdate(peerUdp, peer, []netip.AddrPort{peerUdp}, seed)
+	newLHHostUpdate(targetUdp, target, []netip.AddrPort{targetUdp}, seed)
+
+	stop := make(chan struct{})
+	var wg sync.WaitGroup
+	loop := func(body func()) {
+		defer wg.Done()
+		for {
+			select {
+			case <-stop:
+				return
+			default:
+				body()
+			}
+		}
+	}
+	wg.Add(4)
+
+	// Outbound handshakes keep starting and being dropped.
+	go loop(func() {
+		hm.DeleteHostInfo(hm.StartHandshake(outbound, nil))
+	})
+
+	// Inbound handshakes keep completing and being torn down.
+	var idx uint32
+	go loop(func() {
+		idx++
+		hi := &HostInfo{
+			vpnAddrs:        []netip.Addr{inbound},
+			localIndexId:    idx,
+			remoteIndexId:   idx,
+			remotes:         lh.QueryCache([]netip.Addr{inbound}),
+			HandshakePacket: map[uint8][]byte{handshakePacketStage0: {byte(idx)}},
+		}
+		if _, err := hm.CheckAndComplete(hi, handshakePacketStage0, f); err == nil {
+			mainHM.DeleteHostInfo(hi)
+		}
+	})
+
+	// The peer keeps sending host updates.
+	updates := lh.NewRequestHandler()
+	go loop(func() {
+		newLHHostUpdate(peerUdp, peer, []netip.AddrPort{peerUdp}, updates)
+	})
+
+	// The same peer keeps querying for the target, which ends in a punch notification.
+	queries := lh.NewRequestHandler()
+	go loop(func() {
+		sendLHHostRequest(peerUdp, peer, target, queries, nil)
+	})
+
+	time.Sleep(time.Second)
+	close(stop)
+	done := make(chan struct{})
+	go func() { wg.Wait(); close(done) }()
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("deadlock: the lighthouse paths never finished after stop was signalled")
+	}
+}

outside.go

@@ -263,11 +263,16 @@ func (f *Interface) handleOutsideRelayPacket(hostinfo *HostInfo, via ViaSender,
 
 // closeTunnel closes a tunnel locally, it does not send a closeTunnel packet to the remote
 func (f *Interface) closeTunnel(hostInfo *HostInfo) {
-	final := f.hostMap.DeleteHostInfo(hostInfo)
-	if final {
-		// We no longer have any tunnels with this vpn addr, clear learned lighthouse state to lower memory usage
-		f.lightHouse.DeleteVpnAddrs(hostInfo.vpnAddrs)
+	// Hold the hostmap lock across both the delete and the lighthouse eviction,
+	// to avoid a race with handshake completion.
+	f.hostMap.Lock()
+	defer f.hostMap.Unlock()
+	if !f.hostMap.unlockedDeleteHostInfo(hostInfo) {
+		return
 	}
+
+	// We no longer have any tunnels with this vpn addr, clear learned lighthouse state to lower memory usage
+	f.lightHouse.DeleteVpnAddrs(hostInfo.vpnAddrs)
 }
 
 // sendCloseTunnel is a helper function to send a proper close tunnel packet to a remote

ssh.go

@@ -684,10 +684,14 @@ func sshCreateTunnel(ifce *Interface, fs any, a []string, w sshd.StringWriter) e
 		}
 	}
 
-	hostInfo = ifce.handshakeManager.StartHandshake(vpnAddr, nil)
-	if addr.IsValid() {
-		hostInfo.SetRemote(addr)
-	}
+	// Set the remote from the callback, which StartHandshake runs under the handshake manager lock
+	// before the hostinfo reaches the handshake loop or the static-host trigger, so the address is
+	// in the RemoteList before the first handshake is sent.
+	ifce.handshakeManager.StartHandshake(vpnAddr, func(hh *HandshakeHostInfo) {
+		if addr.IsValid() {
+			hh.hostinfo.SetRemote(addr)
+		}
+	})
 
 	return w.WriteLine("Created")
 }