diff --git a/CHANGELOG.md b/CHANGELOG.md index ed089185..4f27bc82 100644 --- a/CHANGELOG.md +++ b/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 ` 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. diff --git a/connection_manager.go b/connection_manager.go index 83d2b0ca..4ec9f614 100644 --- a/connection_manager.go +++ b/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) diff --git a/handshake_manager.go b/handshake_manager.go index fa9ae154..ff80693c 100644 --- a/handshake_manager.go +++ b/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 } diff --git a/handshake_manager_test.go b/handshake_manager_test.go index 03483d87..acf13539 100644 --- a/handshake_manager_test.go +++ b/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) +} diff --git a/lighthouse.go b/lighthouse.go index 21937bcd..837a8df4 100644 --- a/lighthouse.go +++ b/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 diff --git a/lighthouse_test.go b/lighthouse_test.go index c39ffa16..bb588429 100644 --- a/lighthouse_test.go +++ b/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") + } +} diff --git a/outside.go b/outside.go index c28fe1ea..12f2160c 100644 --- a/outside.go +++ b/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 diff --git a/ssh.go b/ssh.go index 3863b5ec..2a440d98 100644 --- a/ssh.go +++ b/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") }