From fba84ca2328330780433ce0ed45fa90e92559eac Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Wed, 22 Jul 2026 03:34:29 +0900 Subject: [PATCH] auth: check machine key on the followup registration path waitForFollowup returns nodeToRegisterResponse for a completed registration without checking that the Noise session polling for the result was started with the machine key that opened the registration. That response carries the registering user's User and Login, so the auth ID in the followup URL is the only thing protecting it. handleRegister and handleLogout both call machineKeyMismatch before handing back a node, so this is the one path of the three that does not. The key is already available: HandleNodeFromAuthPath resolves the node from the MachineKey cached in RegistrationData, so on the normal path the node and the session agree and the check is a no-op. The auth ID is 96 bits of randomness and is not guessable, so this is not reachable by brute force. It is logged at info level when a registration is created, which makes log access the realistic way to obtain one. The existing followup_registration_success case built its node with CreateNodeForTest, which picks a random machine key that no real registration would produce. Set the registering machine key so the fixture matches the production path. Signed-off-by: Arpit Jain (cherry picked from commit 0ce3356b8995f6691d12424b566ac93a0388c867) --- hscontrol/auth.go | 24 +++++++++++++ hscontrol/auth_test.go | 82 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/hscontrol/auth.go b/hscontrol/auth.go index a6f39666..c4f5e609 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -23,6 +23,18 @@ type AuthProvider interface { AuthURL(authID types.AuthID) string } +// machineKeyMismatch fails closed when a node looked up by NodeKey was started +// in a Noise session with a different machine key. Without this anyone holding a +// target's NodeKey could open a session with a throwaway machine key and act on +// the owner's node. Returns a 401 [HTTPError] on mismatch, nil otherwise. +func machineKeyMismatch(node types.NodeView, machineKey key.MachinePublic) error { + if node.MachineKey() != machineKey { + return NewHTTPError(http.StatusUnauthorized, "node exists with a different machine key", nil) + } + + return nil +} + func (h *Headscale) handleRegister( ctx context.Context, req tailcfg.RegisterRequest, @@ -301,6 +313,18 @@ func (h *Headscale) waitForFollowup( return h.reqToNewRegisterResponse(req, machineKey) } + // The followup poll is only authenticated by the auth ID in the + // URL, so fail closed unless the Noise session asking for the + // result was started with the same machine key that opened the + // registration. [State.HandleNodeFromAuthPath] resolves the node + // from the cached [types.RegistrationData.MachineKey], so the two + // match on the normal path. [Headscale.handleRegister] and + // [Headscale.handleLogout] apply the same check. + err := machineKeyMismatch(verdict.Node, machineKey) + if err != nil { + return nil, err + } + return nodeToRegisterResponse(verdict.Node), nil } } diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index ed294f3c..d12e5ca1 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/http" "net/url" "strings" "testing" @@ -692,6 +693,11 @@ func TestAuthenticationFlows(t *testing.T) { user := app.state.CreateUserForTest("followup-user") node := app.state.CreateNodeForTest(user, "followup-success-node") + // [State.HandleNodeFromAuthPath] resolves the node from the + // machine key cached when the registration was opened, so on + // the real path the node carries the polling session's + // machine key. CreateNodeForTest picks a random one. + node.MachineKey = machineKey1.Public() nodeToRegister.FinishAuth(types.AuthVerdict{Node: node.View()}) }() @@ -4101,3 +4107,79 @@ func TestHandleNodeFromAuthPath_OldUserNil_NoPanic(t *testing.T) { assert.NotEqual(t, types.NodeID(99002), node.ID(), "new node, not orphan") assert.Equal(t, userB.ID, node.UserID().Get(), "new node belongs to userB") } + +// TestWaitForFollowupMachineKeyMismatch covers the followup poll in +// [Headscale.waitForFollowup]. That poll is authenticated only by the auth ID +// embedded in the followup URL, so without a machine-key check anyone who +// learns an ID gets the registering user's User/Login back in the +// [tailcfg.RegisterResponse]. +// +// [Headscale.handleRegister] and [Headscale.handleLogout] already fail closed +// here; see the "existing_node_machine_key_mismatch" case in +// [TestAuthenticationFlows] for the equivalent assertion on that path. +// +// The nodes are given the registering session's machine key because that is +// what production produces: [State.HandleNodeFromAuthPath] resolves the node +// from the machine key cached in [types.RegistrationData] when the +// registration was opened. +func TestWaitForFollowupMachineKeyMismatch(t *testing.T) { + app := createTestApp(t) + + victimMachineKey := key.NewMachine() + attackerMachineKey := key.NewMachine() + + // Park a completed registration in the auth cache, as a node that is + // already polling for its verdict would see it. + newPendingFollowup := func(hostname string) string { + authID := types.MustAuthID() + regEntry := types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: victimMachineKey.Public(), + NodeKey: key.NewNode().Public(), + Hostname: hostname, + }) + app.state.SetAuthCacheEntry(authID, regEntry) + + user := app.state.CreateUserForTest(hostname + "-user") + node := app.state.CreateNodeForTest(user, hostname) + node.MachineKey = victimMachineKey.Public() + // CreateNodeForTest only sets UserID, but nodeToRegisterResponse reads + // the owner, and the owner's identity is exactly what must not leak. + node.User = user + regEntry.FinishAuth(types.AuthVerdict{Node: node.View()}) + + return fmt.Sprintf("http://localhost:8080/register/%s", authID) + } + + followup := func(url string, machineKey key.MachinePublic) (*tailcfg.RegisterResponse, error) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + return app.handleRegister(ctx, tailcfg.RegisterRequest{ + Followup: url, + NodeKey: key.NewNode().Public(), + }, machineKey) + } + + t.Run("mismatched machine key is rejected", func(t *testing.T) { + resp, err := followup(newPendingFollowup("followup-mismatch"), attackerMachineKey.Public()) + + require.Error(t, err, "followup with a foreign machine key must not succeed") + assert.Nil(t, resp, "no registration details should be returned") + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusUnauthorized, httpErr.Code) + }) + + // Positive control. Without it a regression that stops the poll from + // finding the cache entry at all would still pass the case above, because + // waitForFollowup falls back to handing out a fresh AuthURL. + t.Run("matching machine key still completes", func(t *testing.T) { + resp, err := followup(newPendingFollowup("followup-match"), victimMachineKey.Public()) + + require.NoError(t, err) + require.NotNil(t, resp) + assert.True(t, resp.MachineAuthorized) + assert.NotEmpty(t, resp.User.DisplayName, "the owner's identity is returned on the legitimate path") + }) +}