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