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") + }) +}