From b833be283cad6e640e2515d21b970303e3ba6395 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 13:38:04 +0000 Subject: [PATCH] types, auth, noise: let one request wait on each auth session One verdict per session, so one waiter: the newest. A displaced one returns at once (register error, SSH re-delegate), not at session end. A waiter keeps only the follow-up URL and the cached registration. Fixes ANT-2026-NM50ZP4N --- CHANGELOG.md | 3 +- hscontrol/auth.go | 35 +++++--- hscontrol/auth_test.go | 182 ++++++++++++++++++++++++++++++++++++-- hscontrol/noise.go | 25 +++++- hscontrol/noise_test.go | 97 ++++++++++++++++++++ hscontrol/types/common.go | 20 +++++ 6 files changed, 342 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c1c4f8f9..daac7c3cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,10 +160,11 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - Refuse a key expiry outside the years 0 to 9999 in UTC on registration, on the OIDC token-expiry path and in `headscale nodes expire` / `POST /api/v1/node/{nodeId}/expire` (400); the latest accepted expiry no longer fails node listings on PostgreSQL in a zone east of UTC - Keep the first 256 endpoints and 4096 announced routes from client reports; oversized hostnames, requested tags or other metadata return a registration error or HTTP 413 on map updates. Zoned IPv6 endpoints are refused - At startup, trim stored hostnames, endpoints and routes to the client-report limits and remove zoned endpoints. Hostinfo exceeding its size or tag limit is dropped until the next valid report -- Pending interactive registrations keep only the hostname, OS and requested tags; other device details arrive with the first map request. Reauthentication preserves routes, services and endpoints +- Pending interactive registrations and follow-up waits keep only the hostname, OS and requested tags; other device details arrive with the first map request. Reauthentication preserves routes, services and endpoints - Web reauthentication preserves the discovery key so peers can still reach the device - A stalled request body, or a Noise tunnel with no open request, is closed after 30 seconds - A registration follow-up from a machine other than the one that started the registration is refused before it waits +- Only the newest follow-up waits on a pending registration or SSH check; older ones are answered at once ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/auth.go b/hscontrol/auth.go index bc169ad4b..489b71e82 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -133,7 +133,7 @@ func (h *Headscale) handleRegister( // node has already started the registration process and we should wait for // it to finish the original registration. if req.Followup != "" { - return h.waitForFollowup(ctx, req, machineKey) + return h.waitForFollowup(ctx, req.Followup, registrationDataFromRequest(req, machineKey)) } // Pre authenticated keys are handled slightly different than interactive @@ -308,12 +308,18 @@ func nodeToRegisterResponse(node types.NodeView) *tailcfg.RegisterResponse { return resp } +const maxFollowupURLLength = 2048 + func (h *Headscale) waitForFollowup( ctx context.Context, - req tailcfg.RegisterRequest, - machineKey key.MachinePublic, + followup string, + regData *types.RegistrationData, ) (*tailcfg.RegisterResponse, error) { - fu, err := url.Parse(req.Followup) + if len(followup) > maxFollowupURLLength { + return nil, NewHTTPError(http.StatusUnauthorized, "followup URL too long", nil) + } + + fu, err := url.Parse(followup) if err != nil { return nil, NewHTTPError(http.StatusUnauthorized, "invalid followup URL", err) } @@ -332,7 +338,7 @@ func (h *Headscale) waitForFollowup( // [State.HandleNodeFromAuthPath] builds the node from this cached // [types.RegistrationData.MachineKey]. [Headscale.handleRegister] // and [Headscale.handleLogout] apply the same check. - if !reg.IsRegistration() || reg.RegistrationData().MachineKey != machineKey { + if !reg.IsRegistration() || reg.RegistrationData().MachineKey != regData.MachineKey { return nil, NewHTTPError( http.StatusUnauthorized, "registration was opened by a different machine key", @@ -340,6 +346,8 @@ func (h *Headscale) waitForFollowup( ) } + superseded := reg.Park() + var verdict types.AuthVerdict select { // Prefer a completed registration even if the context has also @@ -351,6 +359,12 @@ func (h *Headscale) waitForFollowup( select { case <-ctx.Done(): return nil, NewHTTPError(http.StatusUnauthorized, "registration timed out", ctx.Err()) + case <-superseded: + // tailscaled keeps one follow-up in flight, so only a request + // it already abandoned is displaced. + return nil, NewHTTPError( + http.StatusConflict, "superseded by a newer followup request", nil, + ) case verdict = <-reg.WaitForAuth(): } } @@ -358,7 +372,7 @@ func (h *Headscale) waitForFollowup( if verdict.Accept() { if !verdict.Node.Valid() { // registration is expired in the cache, instruct the client to try a new registration - return h.reqToNewRegisterResponse(req, machineKey) + return h.reqToNewRegisterResponse(regData) } return nodeToRegisterResponse(verdict.Node), nil @@ -366,24 +380,21 @@ func (h *Headscale) waitForFollowup( } // if the follow-up registration isn't found anymore, instruct the client to try a new registration - return h.reqToNewRegisterResponse(req, machineKey) + return h.reqToNewRegisterResponse(regData) } // reqToNewRegisterResponse refreshes the registration flow by creating a new // registration ID and returning the corresponding [tailcfg.RegisterResponse.AuthURL] // so the client can restart the authentication process. func (h *Headscale) reqToNewRegisterResponse( - req tailcfg.RegisterRequest, - machineKey key.MachinePublic, + regData *types.RegistrationData, ) (*tailcfg.RegisterResponse, error) { newAuthID, err := types.NewAuthID() if err != nil { return nil, NewHTTPError(http.StatusInternalServerError, "failed to generate registration ID", err) } - authRegReq := types.NewRegisterAuthRequest( - registrationDataFromRequest(req, machineKey), - ) + authRegReq := types.NewRegisterAuthRequest(regData) log.Info().Msgf("new followup node registration using auth id: %s", newAuthID) h.state.SetAuthCacheEntry(newAuthID, authRegReq) diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 636388328..60f6b8fcc 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/netip" "net/url" + "strconv" "strings" "testing" "time" @@ -4596,12 +4597,10 @@ func TestFollowupWaitPrefersCompletedAuthOverExpiredContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - req := tailcfg.RegisterRequest{ - Followup: fmt.Sprintf("http://localhost:8080/register/%s", regID), - NodeKey: nodeKey, - } + regData := &types.RegistrationData{MachineKey: machineKey, NodeKey: nodeKey} + followup := fmt.Sprintf("http://localhost:8080/register/%s", regID) - resp, err := app.waitForFollowup(ctx, req, machineKey) + resp, err := app.waitForFollowup(ctx, followup, regData) switch { case err != nil: timeouts++ @@ -4698,3 +4697,176 @@ func TestPendingRegistrationKeepsAuthMetadata(t *testing.T) { assert.Equal(t, []string{"tag:router"}, entry.RegistrationData().Hostinfo.RequestTags) assert.Len(t, hostinfo.RoutableIPs, 1, "the request itself must be left alone") } + +// TestFollowupParksOneRequestPerRegistration pins that a pending registration +// keeps one waiting follow-up, the newest. The session hands out one verdict, +// so older waiters are answered at once instead of staying parked until the +// session ends and then each opening a new one. +func TestFollowupParksOneRequestPerRegistration(t *testing.T) { + app := createTestApp(t) + + machineKey := key.NewMachine().Public() + authID := types.MustAuthID() + reg := types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: machineKey, + Hostname: "followup-park-node", + }) + app.state.SetAuthCacheEntry(authID, reg) + + regData := &types.RegistrationData{ + MachineKey: machineKey, + NodeKey: key.NewNode().Public(), + } + followup := fmt.Sprintf("http://localhost:8080/register/%s", authID) + + const followups = 50 + + type answer struct { + resp *tailcfg.RegisterResponse + err error + } + + answered := make(chan answer, followups) + + for range followups { + go func() { + resp, err := app.waitForFollowup(t.Context(), followup, regData) + answered <- answer{resp, err} + }() + } + + next := func(msg string) answer { + t.Helper() + + select { + case a := <-answered: + return a + case <-time.After(5 * time.Second): + require.FailNow(t, msg) + + return answer{} + } + } + + for range followups - 1 { + var httpErr HTTPError + require.ErrorAs(t, + next("more than one follow-up is parked on the registration").err, + &httpErr) + assert.Equal(t, http.StatusConflict, httpErr.Code) + } + + select { + case a := <-answered: + require.FailNowf(t, "the newest follow-up must still be waiting", "got %+v", a) + case <-time.After(50 * time.Millisecond): + } + + cacheLen := func() int { + n, _ := app.state.DebugRegistrationCache()["current_len"].(int) + + return n + } + before := cacheLen() + + user := app.state.CreateUserForTest("followup-park-user") + node := app.state.CreateNodeForTest(user, "followup-park-node") + node.MachineKey = machineKey + node.User = user + reg.FinishAuth(types.AuthVerdict{Node: node.View()}) + + last := next("the parked follow-up did not collect the verdict") + require.NoError(t, last.err) + assert.True(t, last.resp.MachineAuthorized) + assert.LessOrEqual(t, cacheLen(), before+1, + "the verdict must not open a session per waiter") +} + +func TestFollowupURLLength(t *testing.T) { + t.Parallel() + + for _, length := range []int{maxFollowupURLLength, maxFollowupURLLength + 1} { + t.Run(strconv.Itoa(length), func(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + followup := fmt.Sprintf("http://localhost:8080/register/%s?padding=", types.MustAuthID()) + followup += strings.Repeat("x", length-len(followup)) + regData := &types.RegistrationData{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "followup-length-node", + } + + resp, err := app.waitForFollowup(t.Context(), followup, regData) + + if length > maxFollowupURLLength { + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusUnauthorized, httpErr.Code) + assert.Equal(t, "followup URL too long", httpErr.Msg) + assert.Nil(t, resp) + assert.Equal(t, 0, app.state.DebugRegistrationCache()["current_len"]) + + return + } + + require.NoError(t, err) + require.NotNil(t, resp) + assert.NotEmpty(t, resp.AuthURL) + }) + } +} + +func TestFollowupRefreshKeepsAuthMetadata(t *testing.T) { + t.Parallel() + + for _, completed := range []bool{false, true} { + t.Run(fmt.Sprintf("completed=%t", completed), func(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + machineKey := key.NewMachine().Public() + authID := types.MustAuthID() + + if completed { + reg := types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: machineKey, Hostname: "previous-request", + }) + app.state.SetAuthCacheEntry(authID, reg) + reg.FinishAuth(types.AuthVerdict{}) + } + + expiry := time.Now().UTC().Add(time.Hour) + req := tailcfg.RegisterRequest{ + Followup: fmt.Sprintf("http://localhost:8080/register/%s", authID), + NodeKey: key.NewNode().Public(), + Expiry: expiry, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "refreshed-node", OS: "linux", OSVersion: "6.12", + RequestTags: []string{"tag:router"}, + Services: []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 41641}}, + }, + } + resp, err := app.handleRegister(t.Context(), req, machineKey) + require.NoError(t, err) + require.NotNil(t, resp) + newAuthID, err := extractRegistrationIDFromAuthURL(resp.AuthURL) + require.NoError(t, err) + assert.NotEqual(t, authID, newAuthID) + entry, ok := app.state.GetAuthCacheEntry(newAuthID) + require.True(t, ok) + assert.Equal(t, &types.RegistrationData{ + MachineKey: machineKey, + NodeKey: req.NodeKey, + Hostname: "refreshed-node", + Expiry: &expiry, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "refreshed-node", OS: "linux", RequestTags: []string{"tag:router"}, + }, + }, entry.RegistrationData()) + req.Hostinfo.RequestTags[0] = "tag:changed" + assert.Equal(t, []string{"tag:router"}, entry.RegistrationData().Hostinfo.RequestTags) + }) + } +} diff --git a/hscontrol/noise.go b/hscontrol/noise.go index d12af3dbb..7d8b4c52b 100644 --- a/hscontrol/noise.go +++ b/hscontrol/noise.go @@ -599,8 +599,9 @@ func (ns *noiseServer) sshActionHoldAndDelegate( } // sshActionFollowUp handles follow-up requests where the client -// provides an auth_id. It blocks until the auth session resolves or -// the request context is cancelled (e.g. the client disconnects). +// provides an auth_id. It blocks until the auth session resolves, the +// request context is cancelled (e.g. the client disconnects), or a newer +// follow-up parks on the session. func (ns *noiseServer) sshActionFollowUp( ctx context.Context, reqLog zerolog.Logger, @@ -664,6 +665,8 @@ func (ns *noiseServer) sshActionFollowUp( reqLog.Trace().Caller().Msg("SSH action follow-up") + superseded := auth.Park() + var ( verdict types.AuthVerdict verdictOK bool @@ -681,6 +684,10 @@ func (ns *noiseServer) sshActionFollowUp( "ssh action follow-up cancelled", ctx.Err(), ) + case <-superseded: + // The newer request collects the verdict; this one re-delegates, as + // it would on a consumed verdict, without waiting for it. + return sessionGone("SSH check follow-up superseded; re-delegating") case verdict, verdictOK = <-auth.WaitForAuth(): } @@ -957,6 +964,20 @@ func (ns *noiseServer) RegistrationHandler( decodeErr := json.NewDecoder(req.Body).Decode(®isterRequest) if decodeErr == nil { + if registerRequest.Followup != "" { + // Keep only fields used by logout checks or a refreshed registration. + var auth *tailcfg.RegisterResponseAuth + if registerRequest.Auth != nil { + auth = &tailcfg.RegisterResponseAuth{} + } + + registerRequest = tailcfg.RegisterRequest{ + Version: registerRequest.Version, NodeKey: registerRequest.NodeKey, + Followup: registerRequest.Followup, Expiry: registerRequest.Expiry, + Auth: auth, Hostinfo: registrationDataFromRequest(registerRequest, ns.machineKey).Hostinfo, + } + } + if hi := registerRequest.Hostinfo; hi != nil { if len(hi.RoutableIPs) > types.MaxRoutableIPs { log.Warn().Str(zf.MachineKey, ns.machineKey.ShortString()). diff --git a/hscontrol/noise_test.go b/hscontrol/noise_test.go index 00b0dca58..788d398d9 100644 --- a/hscontrol/noise_test.go +++ b/hscontrol/noise_test.go @@ -1975,6 +1975,35 @@ func TestRegistrationHandler_ValidatesClientReport(t *testing.T) { }, entry.RegistrationData().Hostinfo) }) + t.Run("followup discards unused request data", func(t *testing.T) { + t.Parallel() + app := createTestApp(t) + ns := &noiseServer{headscale: app, machineKey: key.NewMachine().Public()} + rec := serveRegister(t, ns, tailcfg.RegisterRequest{ + Version: tailcfg.CurrentCapabilityVersion, NodeKey: key.NewNode().Public(), + Followup: "http://localhost:8080/register/" + types.MustAuthID().String(), + Auth: &tailcfg.RegisterResponseAuth{AuthKey: "unused-followup-key"}, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "followup-node", OS: "linux", RequestTags: []string{"tag:test"}, + OSVersion: strings.Repeat("v", types.MaxHostinfoSize), + }, + DeviceCert: []byte("unused"), Signature: []byte("unused"), Tailnet: "unused", + }) + require.Equal(t, http.StatusOK, rec.Code) + + var resp tailcfg.RegisterResponse + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + require.Empty(t, resp.Error) + authID, err := extractRegistrationIDFromAuthURL(resp.AuthURL) + require.NoError(t, err) + + entry, ok := app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + assert.Equal(t, &tailcfg.Hostinfo{ + Hostname: "followup-node", OS: "linux", RequestTags: []string{"tag:test"}, + }, entry.RegistrationData().Hostinfo) + }) + t.Run("oversized report", func(t *testing.T) { t.Parallel() @@ -2219,3 +2248,71 @@ func TestNoiseHTTPServerTimeouts(t *testing.T) { } }) } + +// TestSSHActionFollowUp_ParksOneRequestPerSession pins that a pending SSH +// check keeps one waiting follow-up, the newest. Older waiters re-delegate at +// once instead of staying parked until the session ends. +func TestSSHActionFollowUp_ParksOneRequestPerSession(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + nodes := putSSHCheckNodes(t, app, "ssh-park-user", "src-node", "dst-node") + src, dst := nodes[0], nodes[1] + + authID := types.MustAuthID() + auth := types.NewSSHCheckAuthRequest(src.ID, dst.ID) + app.state.SetAuthCacheEntry(authID, auth) + + ns := &noiseServer{headscale: app, machineKey: dst.MachineKey} + + const followups = 50 + + type answer struct { + action *tailcfg.SSHAction + err error + } + + answered := make(chan answer, followups) + + for range followups { + go func() { + action, err := ns.sshActionFollowUp( + t.Context(), zerolog.Nop(), &tailcfg.SSHAction{}, + authID.String(), src.ID, dst.ID, sshTestLocalUser, + ) + answered <- answer{action, err} + }() + } + + next := func(msg string) answer { + t.Helper() + + select { + case a := <-answered: + return a + case <-time.After(5 * time.Second): + require.FailNow(t, msg) + + return answer{} + } + } + + for range followups - 1 { + a := next("more than one follow-up is parked on the session") + require.NoError(t, a.err) + assert.NotEqual(t, authID, requireSSHHold(t, *a.action), + "re-delegation must mint a new session") + } + + select { + case a := <-answered: + require.FailNowf(t, "the newest follow-up must still be waiting", "got %+v", a) + case <-time.After(50 * time.Millisecond): + } + + auth.FinishAuth(types.AuthVerdict{}) + + last := next("the parked follow-up did not collect the verdict") + require.NoError(t, last.err) + assert.True(t, last.action.Accept, "accept, got %+v", last.action) +} diff --git a/hscontrol/types/common.go b/hscontrol/types/common.go index 69cd1279e..05edd706f 100644 --- a/hscontrol/types/common.go +++ b/hscontrol/types/common.go @@ -143,6 +143,10 @@ type AuthRequest struct { finished chan AuthVerdict closed *atomic.Bool + + // waiter is closed once a newer request parks on this session; see + // [AuthRequest.Park]. + waiter atomic.Pointer[chan struct{}] } // NewAuthRequest creates a pending auth request with no payload, suitable @@ -245,6 +249,22 @@ func (rn *AuthRequest) WaitForAuth() <-chan AuthVerdict { return rn.finished } +// Park makes the caller the one request waiting on [AuthRequest.WaitForAuth] +// and returns a channel that is closed once a newer caller takes its place. +// [AuthRequest.FinishAuth] hands out one verdict, so a session needs one +// waiter. The newest wins, so a client that reconnected is not kept out by +// its own dead request. Parked requests are then bounded by the sessions in +// the auth cache, not by how many requests clients send. +func (rn *AuthRequest) Park() <-chan struct{} { + waiter := make(chan struct{}) + + if prev := rn.waiter.Swap(&waiter); prev != nil { + close(*prev) + } + + return waiter +} + type AuthVerdict struct { // Err is the error that occurred during the authentication process, if any. // If Err is nil, the authentication process has succeeded.