Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -1463,6 +1463,11 @@ type Server struct {
// normally be fetched from the GitHub API. Used for testing.
GithubUserAndTeamsOverride func() (*GithubUserResponse, []GithubTeamResponse, error)

// ObserveBrowserMFAChallengeExtensionsForTesting, if set, is called with the resolved
// challenge extensions when a BrowserMFARequestID is provided to
// CreateAuthenticateChallenge. Used for testing.
ObserveBrowserMFAChallengeExtensionsForTesting func(*mfav1.ChallengeExtensions)

// AWSRolesAnywhereCreateSessionOverride overrides the AWS Roles Anywhere Create Session API wrapper with a mocked one.
// Used for testing.
AWSRolesAnywhereCreateSessionOverride func(ctx context.Context, req createsession.CreateSessionRequest) (*createsession.CreateSessionResponse, error)
Expand Down Expand Up @@ -4254,6 +4259,35 @@ func (a *Server) CreateAuthenticateChallenge(ctx context.Context, req *proto.Cre
return nil
}

// If a BrowserMFARequestID is provided, look up the request and apply its challenge extensions.
if req.BrowserMFARequestID != "" {
Comment thread
Joerger marked this conversation as resolved.
if req.ChallengeExtensions != nil {
return nil, trace.BadParameter("challenge extensions must not be set when a browser MFA request ID is provided")
}

browserMFAReq, err := a.GetSSOMFASession(ctx, req.BrowserMFARequestID)
Comment thread
Joerger marked this conversation as resolved.
if err != nil {
a.logger.WarnContext(ctx, "Failed to read MFA session for browser MFA request", "error", err)
return nil, trace.AccessDenied("invalid browser MFA request")
Comment thread
danielashare marked this conversation as resolved.
}

chalExts := browserMFAReq.ChallengeExtensions
if chalExts == nil {
a.logger.WarnContext(ctx, "MFA session for browser MFA recorded with empty challenge extensions", "request_id", req.BrowserMFARequestID)
return nil, trace.BadParameter("stored session lacks challenge extensions")
}

// Replace the challenge extensions with the ones found in the SSO MFA object.
// These are the ones from the original tsh request.
challengeExtensions = mfatypes.ChallengeExtensionsToProto(chalExts)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make sure that SSO MFA sessions record all ChallengeExtensions fields as well? It appears we do not record UserVerificationRequirement:

ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: ext.Scope,
AllowReuse: ext.AllowReuse,
},

OK if you want to follow up separately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll include this in an upstream PR, thanks

Comment thread
danielashare marked this conversation as resolved.

// Used for testing. If observer function is set, call it with
// challenge extensions from SSO MFA session.
if a.ObserveBrowserMFAChallengeExtensionsForTesting != nil {
a.ObserveBrowserMFAChallengeExtensionsForTesting(challengeExtensions)
}
}

switch req.GetRequest().(type) {
case *proto.CreateAuthenticateChallengeRequest_UserCredentials:
username = req.GetUserCredentials().GetUsername()
Expand Down
141 changes: 140 additions & 1 deletion lib/auth/browser_mfa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/gravitational/teleport/api/client/proto"
mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1"
"github.com/gravitational/teleport/lib/auth/authclient"
"github.com/gravitational/teleport/lib/auth/authtest"
Expand Down Expand Up @@ -221,7 +222,9 @@ func TestCompleteBrowserMFAChallenge(t *testing.T) {
ConnectorID: "test-connector",
ConnectorType: "test",
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
AllowReuse: mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES,
UserVerificationRequirement: "required",
},
}
err := a.UpsertSSOMFASessionData(ctx, session)
Expand Down Expand Up @@ -314,3 +317,139 @@ func TestCompleteBrowserMFAChallenge(t *testing.T) {
})
}
}

func TestCreateAuthenticateChallenge_BrowserMFARequestID(t *testing.T) {
t.Parallel()
ctx := t.Context()

testServer, err := authtest.NewTestServer(authtest.ServerConfig{
Auth: authtest.AuthServerConfig{
Dir: t.TempDir(),
},
})
require.NoError(t, err)
t.Cleanup(func() { assert.NoError(t, testServer.Close()) })

a := testServer.Auth()

userCreds, err := createUserWithSecondFactors(testServer.TLS)
require.NoError(t, err)

userCredsRequest := &proto.CreateAuthenticateChallengeRequest_UserCredentials{
UserCredentials: &proto.UserCredentials{
Username: userCreds.username,
Password: userCreds.password,
},
}

tests := []struct {
name string
setup func(t *testing.T)
request *proto.CreateAuthenticateChallengeRequest
checkError func(t *testing.T, err error)
wantExtensions *mfav1.ChallengeExtensions
}{
{
name: "NOK invalid browser MFA request ID",
request: &proto.CreateAuthenticateChallengeRequest{
Request: userCredsRequest,
BrowserMFARequestID: "non-existent-id",
},
checkError: func(t *testing.T, err error) {
assert.ErrorAs(t, err, new(*trace.AccessDeniedError), "CreateAuthenticateChallenge error mismatch")
assert.ErrorContains(t, err, "invalid browser MFA request")
},
},
{
name: "NOK challenge extensions set with browser MFA request ID",
request: &proto.CreateAuthenticateChallengeRequest{
Request: userCredsRequest,
BrowserMFARequestID: "some-request-id",
ChallengeExtensions: &mfav1.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
},
},
checkError: func(t *testing.T, err error) {
assert.ErrorAs(t, err, new(*trace.BadParameterError), "CreateAuthenticateChallenge error mismatch")
assert.ErrorContains(t, err, "challenge extensions must not be set")
},
},
{
name: "OK browser MFA challenge extensions applied from SSO MFA session",
setup: func(t *testing.T) {
session := &services.SSOMFASessionData{
RequestID: "test-request-1",
Username: userCreds.username,
ConnectorID: "Browser",
ConnectorType: "Browser",
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
AllowReuse: mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO,
UserVerificationRequirement: "required",
},
}
err := a.UpsertSSOMFASessionData(ctx, session)
require.NoError(t, err)
},
request: &proto.CreateAuthenticateChallengeRequest{
Request: userCredsRequest,
BrowserMFARequestID: "test-request-1",
},
wantExtensions: &mfav1.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
AllowReuse: mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO,
UserVerificationRequirement: "required",
},
},
{
name: "NOK nil challenge extensions",
setup: func(t *testing.T) {
session := &services.SSOMFASessionData{
RequestID: "test-request-2",
Username: userCreds.username,
ConnectorID: "Browser",
ConnectorType: "Browser",
ChallengeExtensions: nil,
Comment thread
codingllama marked this conversation as resolved.
}
err := a.UpsertSSOMFASessionData(ctx, session)
require.NoError(t, err)
},
request: &proto.CreateAuthenticateChallengeRequest{
Request: userCredsRequest,
BrowserMFARequestID: "test-request-2",
},
checkError: func(t *testing.T, err error) {
assert.ErrorAs(t, err, new(*trace.BadParameterError), "CreateAuthenticateChallenge error mismatch")
assert.ErrorContains(t, err, "stored session lacks challenge extensions")
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.setup != nil {
tt.setup(t)
}

var gotExtensions *mfav1.ChallengeExtensions
a.ObserveBrowserMFAChallengeExtensionsForTesting = func(ext *mfav1.ChallengeExtensions) {
gotExtensions = ext
}

challenge, err := a.CreateAuthenticateChallenge(ctx, tt.request)

if tt.checkError != nil {
tt.checkError(t, err)
return
}

require.NoError(t, err)
require.NotNil(t, challenge)
Comment thread
danielashare marked this conversation as resolved.
assert.NotNil(t, challenge.WebauthnChallenge, "expected WebAuthn challenge to be present")

if tt.wantExtensions != nil {
require.Equal(t, tt.wantExtensions, gotExtensions)
}
})
}
}
32 changes: 32 additions & 0 deletions lib/auth/mfatypes/proto.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Teleport
// Copyright (C) 2026 Gravitational, Inc.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <http://www.gnu.org/licenses/>.

package mfatypes

import mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1"

// ChallengeExtensionsToProto converts a ChallengeExtensions to its protobuf representation.
func ChallengeExtensionsToProto(chalExts *ChallengeExtensions) *mfav1.ChallengeExtensions {
Comment thread
danielashare marked this conversation as resolved.
if chalExts == nil {
return nil
}

return &mfav1.ChallengeExtensions{
Scope: chalExts.Scope,
AllowReuse: chalExts.AllowReuse,
UserVerificationRequirement: chalExts.UserVerificationRequirement,
}
}
22 changes: 16 additions & 6 deletions lib/web/mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ type CreateAuthenticateChallengeRequest struct {
ChallengeAllowReuse bool `json:"challenge_allow_reuse"`
UserVerificationRequirement string `json:"user_verification_requirement"`
ProxyAddress string `json:"proxy_address"`
BrowserMFARequestID string `json:"browser_mfa_request_id"`
}

// createAuthenticateChallengeHandle creates and returns MFA authentication challenges for the user in context (logged in user).
Expand Down Expand Up @@ -218,18 +219,27 @@ func (h *Handler) createAuthenticateChallengeHandle(w http.ResponseWriter, r *ht
query.Set("channel_id", channelID)
ssoClientRedirectURL.RawQuery = query.Encode()

chal, err := clt.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{
Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{
ContextUser: &proto.ContextUser{},
},
MFARequiredCheck: mfaRequiredCheckProto,
ChallengeExtensions: &mfav1.ChallengeExtensions{
// If BrowserMFARequestID is set, don't set the challenge extensions.
// They will be gotten from the stored MFASession on the backend and
// applied to the challenge.
var challengeExtensions *mfav1.ChallengeExtensions
if req.BrowserMFARequestID == "" {
challengeExtensions = &mfav1.ChallengeExtensions{
Scope: mfav1.ChallengeScope(req.ChallengeScope),
AllowReuse: allowReuse,
UserVerificationRequirement: req.UserVerificationRequirement,
}
}

chal, err := clt.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{
Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{
ContextUser: &proto.ContextUser{},
},
MFARequiredCheck: mfaRequiredCheckProto,
ChallengeExtensions: challengeExtensions,
SSOClientRedirectURL: ssoClientRedirectURL.String(),
ProxyAddress: req.ProxyAddress,
BrowserMFARequestID: req.BrowserMFARequestID,
Comment thread
danielashare marked this conversation as resolved.
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
Loading