From 313572393b595e3d80f7e0f7e9b29d7711fd7b50 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 11:24:32 +0000 Subject: [PATCH] oidc: confirm SSH checks and display registration details Approve SSH checks only after a click and recheck source ownership. Show registration state and requested tags on the confirmation page. --- hscontrol/oidc.go | 278 ++++++++++-------- hscontrol/oidc_test.go | 373 +++++++++++++++++++++++- hscontrol/templates/register_confirm.go | 100 +++++-- hscontrol/types/common.go | 28 +- 4 files changed, 608 insertions(+), 171 deletions(-) diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index e31265899..e8412d990 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -391,93 +391,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( a.h.Change(userChange) - // If this is a registration flow, render the confirmation - // interstitial instead of finalising the registration immediately. - // Without an explicit user click, a single GET to - // /register/{auth_id} could silently complete a registration when - // the IdP allows silent SSO. - if authInfo.Registration { - a.beginRegistrationConfirmation(writer, req, authInfo.AuthID, user, nodeExpiry) - - return - } - - // If this is not a registration callback, then it is an SSH - // check-mode auth callback. Confirm the OIDC identity is the owner - // of the SSH source node before recording approval; without this - // check any tailnet user could approve a check-mode prompt for any - // other user's node, defeating the stolen-key protection that - // check-mode is meant to provide. - - authReq, ok := a.h.state.GetAuthCacheEntry(authInfo.AuthID) - if !ok { - log.Debug().Caller().Str("auth_id", authInfo.AuthID.String()).Msg("auth session expired before authorization completed") - httpUserError(writer, NewHTTPError(http.StatusGone, "login session expired, try again", nil)) - - return - } - - if !authReq.IsSSHCheck() { - log.Warn().Caller(). - Str("auth_id", authInfo.AuthID.String()). - Msg("OIDC callback hit non-registration path with auth request that is not an SSH check binding") - httpUserError(writer, NewHTTPError(http.StatusBadRequest, "auth session is not for SSH check", nil)) - - return - } - - binding := authReq.SSHCheckBinding() - - srcNode, ok := a.h.state.GetNodeByID(binding.SrcNodeID) - if !ok { - log.Warn().Caller(). - Str("auth_id", authInfo.AuthID.String()). - Uint64("src_node_id", binding.SrcNodeID.Uint64()). - Msg("SSH check src node no longer exists") - httpUserError(writer, NewHTTPError(http.StatusGone, "src node no longer exists", nil)) - - return - } - - // Strict identity binding: only the user that owns the src node - // may approve an SSH check for that node. Tagged source nodes are - // rejected because they have no user owner to compare against. - if srcNode.IsTagged() || !srcNode.UserID().Valid() { - log.Warn().Caller(). - Str("auth_id", authInfo.AuthID.String()). - Uint64("src_node_id", binding.SrcNodeID.Uint64()). - Bool("src_is_tagged", srcNode.IsTagged()). - Str("oidc_user", user.Username()). - Msg("SSH check rejected: src node has no user owner") - httpUserError(writer, NewHTTPError(http.StatusForbidden, "src node has no user owner", nil)) - - return - } - - if srcNode.UserID().Get() != user.ID { - log.Warn().Caller(). - Str("auth_id", authInfo.AuthID.String()). - Uint64("src_node_id", binding.SrcNodeID.Uint64()). - Uint("src_owner_id", srcNode.UserID().Get()). - Uint("oidc_user_id", user.ID). - Str("oidc_user", user.Username()). - Msg("SSH check rejected: OIDC user is not the owner of src node") - httpUserError(writer, NewHTTPError(http.StatusForbidden, "OIDC user is not the owner of the SSH source node", nil)) - - return - } - - // Identity verified — record the verdict for the waiting follow-up. - authReq.FinishAuth(types.AuthVerdict{}) - - content := renderAuthSuccessTemplate(user) - - writer.Header().Set("Content-Type", "text/html; charset=utf-8") - writer.WriteHeader(http.StatusOK) - - if _, err := writer.Write(content.Bytes()); err != nil { //nolint:noinlineerr - util.LogErr(err, "Failed to write HTTP response") - } + a.beginConfirmation(writer, req, authInfo.AuthID, authInfo.Registration, user, nodeExpiry) } // mergeEmailClaims keeps each verification flag with the address it describes. @@ -823,7 +737,61 @@ func (a *AuthProviderOIDC) setRegisterConfirmCookie( }) } -// beginRegistrationConfirmation captures the resolved OIDC identity and +func (a *AuthProviderOIDC) sshCheckSource( + authID types.AuthID, + authReq *types.AuthRequest, + user *types.User, +) (types.NodeView, error) { + if !authReq.IsSSHCheck() { + log.Warn().Caller(). + Str("auth_id", authID.String()). + Msg("OIDC callback hit non-registration path with auth request that is not an SSH check binding") + + return types.NodeView{}, NewHTTPError(http.StatusBadRequest, "auth session is not for SSH check", nil) + } + + binding := authReq.SSHCheckBinding() + + srcNode, ok := a.h.state.GetNodeByID(binding.SrcNodeID) + if !ok { + log.Warn().Caller(). + Str("auth_id", authID.String()). + Uint64("src_node_id", binding.SrcNodeID.Uint64()). + Msg("SSH check src node no longer exists") + + return types.NodeView{}, NewHTTPError(http.StatusGone, "src node no longer exists", nil) + } + + // Strict identity binding: only the user that owns the src node + // may approve an SSH check for that node. Tagged source nodes are + // rejected because they have no user owner to compare against. + if srcNode.IsTagged() || !srcNode.UserID().Valid() { + log.Warn().Caller(). + Str("auth_id", authID.String()). + Uint64("src_node_id", binding.SrcNodeID.Uint64()). + Bool("src_is_tagged", srcNode.IsTagged()). + Str("oidc_user", user.Username()). + Msg("SSH check rejected: src node has no user owner") + + return types.NodeView{}, NewHTTPError(http.StatusForbidden, "src node has no user owner", nil) + } + + if srcNode.UserID().Get() != user.ID { + log.Warn().Caller(). + Str("auth_id", authID.String()). + Uint64("src_node_id", binding.SrcNodeID.Uint64()). + Uint("src_owner_id", srcNode.UserID().Get()). + Uint("oidc_user_id", user.ID). + Str("oidc_user", user.Username()). + Msg("SSH check rejected: OIDC user is not the owner of src node") + + return types.NodeView{}, NewHTTPError(http.StatusForbidden, "OIDC user is not the owner of the SSH source node", nil) + } + + return srcNode, nil +} + +// beginConfirmation captures the resolved OIDC identity and // node expiry into the cached [types.AuthRequest], sets the CSRF cookie, and // redirects the browser to the confirmation page. // @@ -835,22 +803,23 @@ func (a *AuthProviderOIDC) setRegisterConfirmCookie( // — re-enters the callback with a spent code and paints an error over the // interstitial. Redirecting keeps the code exchange one-shot and makes the // page the user waits on safe to reload. -func (a *AuthProviderOIDC) beginRegistrationConfirmation( +func (a *AuthProviderOIDC) beginConfirmation( writer http.ResponseWriter, req *http.Request, authID types.AuthID, + registration bool, user *types.User, nodeExpiry *time.Time, ) { authReq, ok := a.h.state.GetAuthCacheEntry(authID) if !ok { - log.Debug().Caller().Str("auth_id", authID.String()).Msg("registration session expired before authorization completed") + log.Debug().Caller().Str("auth_id", authID.String()).Msg("auth session expired before authorization completed") httpUserError(writer, NewHTTPError(http.StatusGone, "login session expired, try again", nil)) return } - if !authReq.IsRegistration() { + if registration && !authReq.IsRegistration() { log.Warn().Caller(). Str("auth_id", authID.String()). Msg("OIDC callback hit registration path with auth request that is not a node registration") @@ -859,6 +828,15 @@ func (a *AuthProviderOIDC) beginRegistrationConfirmation( return } + if !registration { + _, err := a.sshCheckSource(authID, authReq, user) + if err != nil { + httpUserError(writer, err) + + return + } + } + csrf := rands.HexString(32) authReq.SetPendingConfirmation(&types.PendingRegistrationConfirmation{ @@ -875,9 +853,9 @@ func (a *AuthProviderOIDC) beginRegistrationConfirmation( http.Redirect(writer, req, a.registerConfirmURL(authID), http.StatusSeeOther) } -// RegisterConfirmGetHandler renders the OIDC registration confirmation +// RegisterConfirmGetHandler renders the OIDC registration or SSH confirmation // interstitial. It is reached via the redirect that -// [AuthProviderOIDC.beginRegistrationConfirmation] issues from the OIDC +// [AuthProviderOIDC.beginConfirmation] issues from the OIDC // callback, and it is safe to reload: it only reads the pending // confirmation captured on the cached [types.AuthRequest] and never touches // the one-time code exchange. @@ -933,18 +911,46 @@ func (a *AuthProviderOIDC) RegisterConfirmGetHandler( return } - regData := authReq.RegistrationData() - info := templates.RegisterConfirmInfo{ FormAction: a.registerConfirmURL(authID), CSRFTokenName: registerConfirmCSRFCookie, CSRFToken: pending.CSRF, User: user.Display(), - Hostname: regData.Hostname, - MachineKey: regData.MachineKey.ShortString(), } - if regData.Hostinfo != nil { - info.OS = regData.Hostinfo.OS + + var content string + + if authReq.IsSSHCheck() { + src, err := a.sshCheckSource(authID, authReq, user) + if err != nil { + httpUserError(writer, err) + + return + } + + dst, ok := a.h.state.GetNodeByID(authReq.SSHCheckBinding().DstNodeID) + if !ok { + httpUserError(writer, NewHTTPError(http.StatusGone, "destination node no longer exists", nil)) + + return + } + + content = templates.SSHCheckConfirm(info, src.GivenName(), dst.GivenName()).Render() + } else { + regData := authReq.RegistrationData() + info.Hostname = regData.Hostname + info.MachineKey = regData.MachineKey.ShortString() + + if node, ok := a.h.state.LoginNode(regData.MachineKey, types.UserID(user.ID)); ok { + info.ExistingNode = node.GivenName() + } + + if regData.Hostinfo != nil { + info.OS = regData.Hostinfo.OS + info.Tags = regData.Hostinfo.RequestTags + } + + content = templates.RegisterConfirm(info).Render() } // The page carries the token that finalises the registration, so no @@ -953,15 +959,14 @@ func (a *AuthProviderOIDC) RegisterConfirmGetHandler( writer.Header().Set("Content-Type", "text/html; charset=utf-8") writer.WriteHeader(http.StatusOK) - if _, err := writer.Write([]byte(templates.RegisterConfirm(info).Render())); err != nil { //nolint:noinlineerr + if _, err := writer.Write([]byte(content)); err != nil { //nolint:noinlineerr util.LogErr(err, "Failed to write HTTP response") } } // RegisterConfirmHandler is the POST endpoint behind the OIDC -// registration confirmation interstitial. It validates the CSRF cookie -// against the form-submitted token, finalises the registration via -// [AuthProviderOIDC.handleRegistration], and renders the success page. +// confirmation interstitial. It validates the CSRF cookie against the +// form-submitted token, then finalises registration or SSH approval. func (a *AuthProviderOIDC) RegisterConfirmHandler( writer http.ResponseWriter, req *http.Request, @@ -1031,29 +1036,49 @@ func (a *AuthProviderOIDC) RegisterConfirmHandler( return } - newNode, err := a.handleRegistration(user, authID, pending.NodeExpiry) - if err != nil { - if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) { - httpUserError(writer, newHTTPUserError( - http.StatusGone, - registrationLinkSpentMsg, - registrationLinkSpentUserMsg, - err, - )) + var content *bytes.Buffer + + if authReq.IsSSHCheck() { + _, err := a.sshCheckSource(authID, authReq, user) + if err != nil { + httpUserError(writer, err) return } - httpUserError(writer, err) + authReq.FinishAuth(types.AuthVerdict{}) - return + content = renderAuthSuccessTemplate(user) + } else { + _, reauth := a.h.state.LoginNode( + authReq.RegistrationData().MachineKey, + types.UserID(user.ID), + ) + + node, err := a.handleRegistration(user, authID, pending.NodeExpiry) + if err != nil { + if errors.Is(err, db.ErrNodeNotFoundRegistrationCache) { + httpUserError(writer, newHTTPUserError( + http.StatusGone, + registrationLinkSpentMsg, + registrationLinkSpentUserMsg, + err, + )) + + return + } + + httpUserError(writer, err) + + return + } + + content = renderRegistrationSuccessTemplate(user, node, reauth) } - // Clear the CSRF cookie now that the registration is final. + // Clear the CSRF cookie now that the request is final. a.setRegisterConfirmCookie(writer, req, authID, "", -1) - content := renderRegistrationSuccessTemplate(user, newNode) - writer.Header().Set("Content-Type", "text/html; charset=utf-8") writer.WriteHeader(http.StatusOK) @@ -1069,8 +1094,8 @@ func (a *AuthProviderOIDC) handleRegistration( user *types.User, registrationID types.AuthID, expiry *time.Time, -) (bool, error) { - _, nodeChange, err := a.h.state.HandleNodeFromAuthPath( +) (types.NodeView, error) { + node, nodeChange, err := a.h.state.HandleNodeFromAuthPath( registrationID, types.UserID(user.ID), expiry, @@ -1080,19 +1105,20 @@ func (a *AuthProviderOIDC) handleRegistration( if err != nil { if errors.Is(err, types.ErrHostinfoTooLarge) { - return false, newHTTPUserError(http.StatusBadRequest, "node report too large", + return types.NodeView{}, newHTTPUserError(http.StatusBadRequest, "node report too large", "This device reports too much data. Ask your administrator to reduce its reported metadata, then sign in again.", err) } - return false, fmt.Errorf("registering node: %w", err) + return types.NodeView{}, fmt.Errorf("registering node: %w", err) } - return !nodeChange.IsEmpty(), nil + return node, nil } func renderRegistrationSuccessTemplate( user *types.User, - newNode bool, + node types.NodeView, + reauth bool, ) *bytes.Buffer { result := templates.AuthSuccessResult{ Title: "Headscale - Node Reauthenticated", @@ -1101,7 +1127,11 @@ func renderRegistrationSuccessTemplate( User: user.Display(), Message: "You can now close this window.", } - if newNode { + if node.IsTagged() { + result.User = strings.Join(node.Tags().AsSlice(), ", ") + } + + if !reauth { result.Title = "Headscale - Node Registered" result.Heading = "Node registered" result.Verb = "Registered" diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index 32024f66a..0735fbb83 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -938,15 +938,30 @@ func TestOIDCPendingCallbackPreservesSSHVerdict(t *testing.T) { src := b.app.state.CreateNodeForTest(user, "oidc-ssh-source") b.app.state.PutNodeInStoreForTest(*src) + dst := b.app.state.CreateNodeForTest(user, "oidc-ssh-destination") + b.app.state.PutNodeInStoreForTest(*dst) + authID := types.MustAuthID() - authReq := types.NewSSHCheckAuthRequest(src.ID, 2) + authReq := types.NewSSHCheckAuthRequest(src.ID, dst.ID) require.NoError(t, b.app.state.SetAuthCacheEntry(authID, authReq)) - status, _, body := b.get(t, a.AuthURL(authID)) + status, landed, body := b.get(t, a.AuthURL(authID)) require.Equal(t, http.StatusOK, status) - assert.Contains(t, body, "You may return to your terminal.") assert.Zero(t, a.authCache.Len()) + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + //nolint:noctx,bodyclose // test response closed below + confirmed, err := b.client.PostForm(landed.String(), url.Values{registerConfirmCSRFCookie: {csrf[1]}}) + require.NoError(t, err) + + confirmedBody, err := io.ReadAll(confirmed.Body) + confirmed.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusOK, confirmed.StatusCode) + assert.Contains(t, string(confirmedBody), "You may return to your terminal.") + cached, ok := b.app.state.GetAuthCacheEntry(authID) require.True(t, ok, "completing OIDC must preserve the SSH verdict for its polling client") assert.Same(t, authReq, cached) @@ -1548,6 +1563,354 @@ func TestOIDCCallbackDispatchesUserIdentityChange(t *testing.T) { }, 3*time.Second, 20*time.Millisecond, "the connected node must receive the updated filter before registration confirmation") } +func (b *oidcBrowser) pendingSSHCheck(t *testing.T) (types.AuthID, types.Nodes) { + t.Helper() + + provider, ok := b.app.authProvider.(*AuthProviderOIDC) + require.True(t, ok) + + owner, _, err := provider.createOrUpdateUserFromClaim(&types.OIDCClaims{ + Iss: b.idp.Issuer(), Sub: "ssh-owner", Username: "ssh-owner", + }) + require.NoError(t, err) + + nodes := make(types.Nodes, 0, 2) + + for _, name := range []string{"ssh-source", "ssh-destination"} { + node := b.app.state.CreateRegisteredNodeForTest(owner, name) + node.User = owner.Clone() + b.app.state.PutNodeInStoreForTest(*node) + nodes = append(nodes, node) + } + + authID := types.MustAuthID() + require.NoError(t, b.app.state.SetAuthCacheEntry(authID, types.NewSSHCheckAuthRequest(nodes[0].ID, nodes[1].ID))) + + return authID, nodes +} + +func TestSSHCheckNeedsConfirmation(t *testing.T) { + for _, change := range []string{"approve", "reowned", "tagged"} { + t.Run(change, func(t *testing.T) { + b := newOIDCBrowser(t) + authID, nodes := b.pendingSSHCheck(t) + src, dst := nodes[0], nodes[1] + auth, ok := b.app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + b.idp.QueueUser(&mockoidc.MockUser{ + Subject: src.User.Name, PreferredUsername: src.User.Name, + }) + + status, landed, body := b.get(t, b.publicURL+"/auth/"+authID.String()) + require.Equal(t, http.StatusOK, status) + + select { + case <-auth.WaitForAuth(): + t.Fatal("opening the check link must not approve the session") + default: + } + + require.Equal(t, "/register/confirm/"+authID.String(), landed.Path) + require.Contains(t, body, "Confirm SSH session") + require.Contains(t, body, src.GivenName) + require.Contains(t, body, dst.GivenName) + require.Contains(t, body, "Approved by") + require.Contains(t, body, src.User.Display()) + require.Contains(t, body, "check period") + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + form := url.Values{registerConfirmCSRFCookie: {csrf[1]}} + withoutCookie := &http.Client{} + //nolint:noctx,bodyclose // test response closed below + refused, err := withoutCookie.PostForm(landed.String(), form) + require.NoError(t, err) + refused.Body.Close() + require.Equal(t, http.StatusForbidden, refused.StatusCode) + + select { + case <-auth.WaitForAuth(): + t.Fatal("a POST without the callback cookie must leave the check pending") + default: + } + + switch change { + case "reowned": + other := b.app.state.CreateUserForTest("other-owner") + updated := src.Clone() + updated.UserID, updated.User = new(other.ID), other.Clone() + b.app.state.PutNodeInStoreForTest(*updated) + case "tagged": + updated := src.Clone() + updated.Tags = []string{"tag:changed"} + b.app.state.PutNodeInStoreForTest(*updated) + } + + //nolint:noctx,bodyclose // test response closed below + confirmed, err := b.client.PostForm(landed.String(), form) + require.NoError(t, err) + + confirmedBody, err := io.ReadAll(confirmed.Body) + confirmed.Body.Close() + require.NoError(t, err) + + if change != "approve" { + require.Equal(t, http.StatusForbidden, confirmed.StatusCode) + + select { + case <-auth.WaitForAuth(): + t.Fatal("a changed source owner must leave the check pending") + default: + } + + status, _, _ = b.get(t, landed.String()) + require.Equal(t, http.StatusForbidden, status) + + return + } + + require.Equal(t, http.StatusOK, confirmed.StatusCode) + require.Contains(t, string(confirmedBody), "SSH session authorized") + + select { + case verdict := <-auth.WaitForAuth(): + require.True(t, verdict.Accept()) + default: + t.Fatal("confirmation must approve the SSH check") + } + }) + } +} + +func TestSSHCheckRejectsOtherUser(t *testing.T) { + b := newOIDCBrowser(t) + authID, _ := b.pendingSSHCheck(t) + auth, ok := b.app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + b.idp.QueueUser(&mockoidc.MockUser{Subject: "other-ssh-user", PreferredUsername: "other"}) + + status, _, _ := b.get(t, b.publicURL+"/auth/"+authID.String()) + require.Equal(t, http.StatusForbidden, status) + require.Nil(t, auth.PendingConfirmation()) + + select { + case <-auth.WaitForAuth(): + t.Fatal("a different user must not approve the SSH check") + default: + } +} + +func TestOIDCConfirmationRejectsWrongRequestKind(t *testing.T) { + for _, path := range []string{"auth", "register"} { + t.Run(path, func(t *testing.T) { + b := newOIDCBrowser(t) + + var authID types.AuthID + if path == "auth" { + authID, _ = b.pendingNode(t) + } else { + authID, _ = b.pendingSSHCheck(t) + } + + status, _, _ := b.get(t, b.publicURL+"/"+path+"/"+authID.String()) + require.Equal(t, http.StatusBadRequest, status) + + auth, ok := b.app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + require.Nil(t, auth.PendingConfirmation()) + }) + } +} + +func TestRegisterConfirmShowsExistingNode(t *testing.T) { + b := newOIDCBrowser(t) + machineKey := key.NewMachine().Public() + existingRow := regexp.MustCompile(`>Existing node]*>([^<]*)`) + + // A tagged node another credential made: a login on its machine adds a + // device instead of taking it. + taggedMachine := key.NewMachine().Public() + tagKey, err := b.app.state.CreatePreAuthKey(nil, true, false, nil, []string{"tag:server"}) + require.NoError(t, err) + _, _, err = b.app.state.HandleNodeFromPreAuthKey(tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: tagKey.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "server"}, + }, taggedMachine) + require.NoError(t, err) + + for _, tt := range []struct { + name string + machineKey key.MachinePublic + existing string + heading string + nodeCount int + }{ + { + name: "first login", machineKey: machineKey, + existing: "None, this is a new device", heading: "Node registered", nodeCount: 2, + }, + { + name: "same machine", machineKey: machineKey, + existing: "laptop", heading: "Node reauthenticated", nodeCount: 2, + }, + { + name: "copied hostname", machineKey: key.NewMachine().Public(), + existing: "None, this is a new device", heading: "Node registered", nodeCount: 3, + }, + { + name: "tagged machine", machineKey: taggedMachine, + existing: "None, this is a new device", heading: "Node registered", nodeCount: 4, + }, + } { + authID := types.MustAuthID() + require.NoError(t, b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: tt.machineKey, + NodeKey: key.NewNode().Public(), + Hostname: "laptop", + }))) + b.idp.QueueUser(&mockoidc.MockUser{ + Subject: "existing-device", PreferredUsername: "device-owner", + }) + + status, landed, body := b.get(t, b.publicURL+"/register/"+authID.String()) + require.Equal(t, http.StatusOK, status) + + row := existingRow.FindStringSubmatch(body) + if assert.Len(t, row, 2, "the page must distinguish an existing node from client-supplied details") { + assert.Equal(t, tt.existing, row[1], tt.name) + } + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + //nolint:noctx,bodyclose // test response closed below + confirmed, err := b.client.PostForm(landed.String(), url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + confirmedBody, err := io.ReadAll(confirmed.Body) + confirmed.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusOK, confirmed.StatusCode) + assert.Contains(t, string(confirmedBody), tt.heading, tt.name) + assert.Equal(t, tt.nodeCount, b.app.state.ListNodes().Len(), tt.name) + } +} + +func TestRegisterConfirmShowsRequestedTags(t *testing.T) { + const ownerName = "tag-owner" + + b := newOIDCBrowser(t) + _, err := b.app.state.SetPolicy([]byte(`{ + "tagOwners": {"tag:prod": ["tag-owner@"]}, + "acls": [{"action":"accept", "src":["*"], "dst":["*:*"]}] + }`)) + require.NoError(t, err) + + machineKey := key.NewMachine().Public() + authID := types.MustAuthID() + require.NoError(t, b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: machineKey, + NodeKey: key.NewNode().Public(), + Hostname: "tagged-laptop", + Hostinfo: &tailcfg.Hostinfo{RequestTags: []string{"tag:prod"}}, + }))) + b.idp.QueueUser(&mockoidc.MockUser{Subject: ownerName, PreferredUsername: ownerName}) + + status, landed, body := b.get(t, b.publicURL+"/register/"+authID.String()) + require.Equal(t, http.StatusOK, status) + + registered := regexp.MustCompile(`>Registered to]*>([^<]*)`).FindStringSubmatch(body) + require.Len(t, registered, 2) + assert.Equal(t, "tag:prod", registered[1]) + + approved := regexp.MustCompile(`>Approved by]*>([^<]*)`).FindStringSubmatch(body) + if assert.Len(t, approved, 2) { + assert.Equal(t, ownerName, approved[1]) + } + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + //nolint:noctx,bodyclose // test response closed below + confirmed, err := b.client.PostForm(landed.String(), url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + + confirmedBody, err := io.ReadAll(confirmed.Body) + confirmed.Body.Close() + require.NoError(t, err) + require.Equal(t, http.StatusOK, confirmed.StatusCode) + + success := regexp.MustCompile(`Registered as ([^<]*)`).FindSubmatch(confirmedBody) + require.Len(t, success, 2) + assert.Equal(t, "tag:prod", string(success[1])) + + nodes := b.app.state.ListNodes() + require.Equal(t, 1, nodes.Len()) + require.True(t, nodes.At(0).IsTagged()) + require.Equal(t, []string{"tag:prod"}, nodes.At(0).Tags().AsSlice()) + + reloginID := types.MustAuthID() + require.NoError(t, b.app.state.SetAuthCacheEntry(reloginID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: machineKey, + NodeKey: key.NewNode().Public(), + Hostname: "different-client-name", + }))) + b.idp.QueueUser(&mockoidc.MockUser{Subject: ownerName, PreferredUsername: ownerName}) + status, _, body = b.get(t, b.publicURL+"/register/"+reloginID.String()) + require.Equal(t, http.StatusOK, status) + + existing := regexp.MustCompile(`>Existing node]*>([^<]*)`).FindStringSubmatch(body) + require.Len(t, existing, 2) + require.Equal(t, "tagged-laptop", existing[1]) + require.True(t, b.app.state.ListNodes().At(0).IsTagged(), "viewing confirmation must not change ownership") +} + +func TestRegisterConfirmEscapesAndRejectsUnownedTags(t *testing.T) { + for _, tt := range []struct { + tag string + display string + }{ + {tag: "tag:unowned", display: "tag:unowned"}, + {tag: "tag:prod", display: "tag:<em>prod</em>"}, + } { + t.Run(tt.tag, func(t *testing.T) { + b := newOIDCBrowser(t) + authID := types.MustAuthID() + require.NoError(t, b.app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: key.NewMachine().Public(), + NodeKey: key.NewNode().Public(), + Hostname: "unowned-tag-node", + Hostinfo: &tailcfg.Hostinfo{RequestTags: []string{tt.tag}}, + }))) + + status, landed, body := b.get(t, b.publicURL+"/register/"+authID.String()) + require.Equal(t, http.StatusOK, status) + + row := regexp.MustCompile(`>Registered to]*>(.*?)`).FindStringSubmatch(body) + require.Len(t, row, 2) + assert.Equal(t, tt.display, row[1]) + + csrf := csrfInputRe.FindStringSubmatch(body) + require.Len(t, csrf, 2) + + //nolint:noctx,bodyclose // test response closed below + confirmed, err := b.client.PostForm(landed.String(), url.Values{ + registerConfirmCSRFCookie: {csrf[1]}, + }) + require.NoError(t, err) + confirmed.Body.Close() + require.NotEqual(t, http.StatusOK, confirmed.StatusCode) + require.Zero(t, b.app.state.ListNodes().Len()) + }) + } +} + func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { t.Parallel() @@ -1570,7 +1933,7 @@ func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { }))) provider := &AuthProviderOIDC{h: app} - changed, err := provider.handleRegistration(user, authID, nil) + registered, err := provider.handleRegistration(user, authID, nil) require.ErrorIs(t, err, types.ErrHostinfoTooLarge) var httpErr HTTPError @@ -1579,7 +1942,7 @@ func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { assert.Contains(t, strings.ToLower(httpErr.UserMsg), "reduce") assert.Contains(t, httpErr.UserMsg, "metadata") assert.Contains(t, httpErr.UserMsg, "sign") - assert.False(t, changed) + assert.False(t, registered.Valid()) after, ok := app.state.GetNodeByID(node.ID) require.True(t, ok) diff --git a/hscontrol/templates/register_confirm.go b/hscontrol/templates/register_confirm.go index a7297017c..aef27511f 100644 --- a/hscontrol/templates/register_confirm.go +++ b/hscontrol/templates/register_confirm.go @@ -2,6 +2,7 @@ package templates import ( "cmp" + "strings" "github.com/chasefleming/elem-go" "github.com/chasefleming/elem-go/attrs" @@ -15,6 +16,7 @@ import ( // detail (hostname, OS, machine-key fingerprint) for the user to // recognise whether the device they are about to claim is in fact // theirs. +// The form and user fields are also used for SSH check confirmations. type RegisterConfirmInfo struct { // FormAction is the absolute or relative URL the confirm form // POSTs to. Typically /register/confirm/{auth_id}. @@ -28,8 +30,7 @@ type RegisterConfirmInfo struct { // the cookie set by the OIDC callback before the POST is honoured. CSRFToken string - // User is the OIDC-authenticated identity the device will be - // registered to if the user confirms. + // User is the OIDC-authenticated identity approving the request. User string // Hostname is the hostname the registering tailscaled instance @@ -43,6 +44,12 @@ type RegisterConfirmInfo struct { // MachineKey is the short fingerprint of the registering machine // key. The full key is intentionally not shown. MachineKey string + + // ExistingNode is the stored node name, empty for a new device. + ExistingNode string + + // Tags are the ownership tags requested by the registering client. + Tags []string } // RegisterConfirm renders an interstitial page that asks the @@ -52,16 +59,70 @@ type RegisterConfirmInfo struct { // silently complete a phishing-style registration when the victim's // IdP allows silent SSO. func RegisterConfirm(info RegisterConfirmInfo) *elem.Element { - deviceList := deviceTable( - []deviceRow{ - {"Hostname", elem.Text(info.Hostname)}, - {"OS", elem.Text(cmp.Or(info.OS, "(unknown)"))}, - {"Machine key", Code(elem.Text(info.MachineKey))}, - {"Registered to", elem.Text(info.User)}, - }, - ) + rows := []deviceRow{ + {"Hostname", elem.Text(info.Hostname)}, + {"OS", elem.Text(cmp.Or(info.OS, "(unknown)"))}, + {"Machine key", Code(elem.Text(info.MachineKey))}, + {"Existing node", elem.Text(cmp.Or(info.ExistingNode, "None, this is a new device"))}, + } + if len(info.Tags) > 0 { + rows = append(rows, + deviceRow{"Registered to", elem.Text(strings.Join(info.Tags, ", "))}, + deviceRow{"Approved by", elem.Text(info.User)}, + ) + } else { + rows = append(rows, deviceRow{"Registered to", elem.Text(info.User)}) + } - form := elem.Form( + deviceList := deviceTable(rows) + + return page( + "Headscale - Confirm node registration", + H2(elem.Text("Confirm node registration")), + P(elem.Text( + "A device is asking to be added to your tailnet. "+ + "Please review the details below and confirm that this device is yours.", + )), + deviceList, + confirmForm(info, "Confirm registration"), + P(elem.Text( + "If you do not recognise this device, close this window. "+ + "The registration request will expire automatically.", + )), + ) +} + +// SSHCheckConfirm asks the owner to approve the source and destination node pair. +func SSHCheckConfirm( + info RegisterConfirmInfo, + source, destination string, +) *elem.Element { + return page( + "Headscale - Confirm SSH session", + H2(elem.Text("Confirm SSH session")), + P(elem.Text( + "An SSH session is waiting for your approval. "+ + "Review the source and destination nodes before continuing.", + )), + deviceTable([]deviceRow{ + {"From", elem.Text(source)}, + {"To", elem.Text(destination)}, + {"Approved by", elem.Text(info.User)}, + }), + P(elem.Text( + "Approval also allows new SSH sessions between these two nodes "+ + "without asking again until the check period ends.", + )), + confirmForm(info, "Approve SSH session"), + P(elem.Text( + "If you do not recognise this request, close this window. "+ + "The request will expire automatically.", + )), + ) +} + +func confirmForm(info RegisterConfirmInfo, buttonText string) *elem.Element { + return elem.Form( attrs.Props{ attrs.Method: "POST", attrs.Action: info.FormAction, @@ -73,24 +134,9 @@ func RegisterConfirm(info RegisterConfirmInfo) *elem.Element { }), elem.Button( attrs.Props{attrs.Type: "submit"}, - elem.Text("Confirm registration"), + elem.Text(buttonText), ), ) - - return page( - "Headscale - Confirm node registration", - H2(elem.Text("Confirm node registration")), - P(elem.Text( - "A device is asking to be added to your tailnet. "+ - "Please review the details below and confirm that this device is yours.", - )), - deviceList, - form, - P(elem.Text( - "If you do not recognise this device, close this window. "+ - "The registration request will expire automatically.", - )), - ) } type deviceRow struct { diff --git a/hscontrol/types/common.go b/hscontrol/types/common.go index d759fa466..a5a6eedc5 100644 --- a/hscontrol/types/common.go +++ b/hscontrol/types/common.go @@ -94,11 +94,10 @@ type SSHCheckBinding struct { } // PendingRegistrationConfirmation captures the server-side state needed -// to finalise a node registration after the user has confirmed it on -// the OIDC interstitial. The OIDC callback resolves the user identity -// and node expiry, stores them on the cached [AuthRequest], and renders -// a confirmation page; only when the user POSTs the confirmation form -// does the actual node registration run. +// to finalise a node registration or SSH check after the user confirms +// it on the OIDC interstitial. The callback resolves the user identity +// and optional node expiry, then stores them on the cached [AuthRequest]. +// Only the confirmation POST registers the node or approves the SSH check. // // CSRF is a one-shot per-session token that the OIDC callback set // both as a cookie and as a hidden form field. The confirm POST @@ -129,7 +128,7 @@ type AuthRequest struct { // sshBinding is populated for SSH check-mode flows. It captures the // (src, dst) node pair the request was minted for so the follow-up - // and OIDC callback can refuse to record a verdict for any other + // and OIDC confirmation handler can refuse a verdict for any other // pair. // // nil for non-SSH-check flows. Use [AuthRequest.SSHCheckBinding] to read it @@ -137,10 +136,10 @@ type AuthRequest struct { sshBinding *SSHCheckBinding // pendingConfirmation is populated by the OIDC callback for the - // node-registration flow once the user identity has been resolved - // but before the user has explicitly confirmed the registration on + // registration or SSH check once the user identity has been resolved + // but before the user has explicitly confirmed the request on // the interstitial. The /register/confirm POST handler reads it to - // finalise the registration without re-running the OIDC flow. + // finalise the request without re-running the OIDC flow. pendingConfirmation *PendingRegistrationConfirmation finished chan AuthVerdict @@ -199,7 +198,7 @@ func (rn *AuthRequest) PrepareForCache() error { // NewSSHCheckAuthRequest creates a pending auth request bound to a // specific (src, dst) SSH check-mode pair. The follow-up handler and -// OIDC callback must verify their incoming request matches this binding +// OIDC confirmation handler must verify the request matches this binding // before recording any verdict. func NewSSHCheckAuthRequest(src, dst NodeID) *AuthRequest { return &AuthRequest{ @@ -265,15 +264,14 @@ func (rn *AuthRequest) IsFinished() bool { } // SetPendingConfirmation marks this [AuthRequest] as having an -// OIDC-resolved user that is waiting to confirm the registration on -// the interstitial. The OIDC callback should call this and then render -// the confirmation page; the /register/confirm POST handler reads the -// stored UserID/NodeExpiry to finish the registration. +// OIDC-resolved user waiting to confirm registration or an SSH check. +// The callback stores this before redirecting to the confirmation page; +// the /register/confirm POST handler reads it to finish the request. func (rn *AuthRequest) SetPendingConfirmation(p *PendingRegistrationConfirmation) { rn.pendingConfirmation = p } -// PendingConfirmation returns the pending OIDC-resolved registration +// PendingConfirmation returns the pending OIDC-resolved confirmation // state captured by [AuthRequest.SetPendingConfirmation], or nil if no OIDC callback // has yet resolved an identity for this [AuthRequest]. func (rn *AuthRequest) PendingConfirmation() *PendingRegistrationConfirmation {