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
2 changes: 1 addition & 1 deletion lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -4314,7 +4314,7 @@ func (a *Server) CreateAuthenticateChallenge(ctx context.Context, req *proto.Cre
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)
browserMFAReq, err := a.GetMFASession(ctx, req.BrowserMFARequestID)
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")
Expand Down
7 changes: 4 additions & 3 deletions lib/auth/browser_mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import (
func (a *Server) CompleteBrowserMFAChallenge(ctx context.Context, requestID string, webauthnResponse *webauthnpb.CredentialAssertionResponse) (string, error) {
const notFoundErrMsg = "mfa session data not found"
// Retrieve the MFA session
mfaSession, err := a.GetSSOMFASession(ctx, requestID)
mfaSession, err := a.GetMFASession(ctx, requestID)
if trace.IsNotFound(err) {
return "", trace.AccessDenied("%s", notFoundErrMsg)
} else if err != nil {
Expand Down Expand Up @@ -88,8 +88,9 @@ func (a *Server) BeginBrowserMFAChallenge(ctx context.Context, params mfatypes.B
ConnectorType: constants.BrowserMFA,
TSHRedirectURL: params.BrowserMFATSHRedirectURL,
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: params.Ext.Scope,
AllowReuse: params.Ext.AllowReuse,
Scope: params.Ext.Scope,
AllowReuse: params.Ext.AllowReuse,
UserVerificationRequirement: params.Ext.UserVerificationRequirement,
},
SourceCluster: params.SourceCluster,
TargetCluster: params.TargetCluster,
Expand Down
112 changes: 109 additions & 3 deletions lib/auth/browser_mfa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func TestCompleteBrowserMFAChallenge(t *testing.T) {
setupSession: func(t *testing.T) string {
requestID := uuid.NewString()
redirectURL := "http://127.0.0.1:62972/callback?secret_key=" + secretKey.String()
session := &services.SSOMFASessionData{
session := &services.MFASessionData{
RequestID: requestID,
Username: "other-user", // mismatch username
TSHRedirectURL: redirectURL,
Expand All @@ -309,7 +309,7 @@ func TestCompleteBrowserMFAChallenge(t *testing.T) {
name: "NOK invalid redirect URL in session",
setupSession: func(t *testing.T) string {
requestID := uuid.NewString()
session := &services.SSOMFASessionData{
session := &services.MFASessionData{
RequestID: requestID,
Username: username,
TSHRedirectURL: "://invalid-url",
Expand All @@ -332,7 +332,7 @@ func TestCompleteBrowserMFAChallenge(t *testing.T) {
setupSession: func(t *testing.T) string {
requestID := uuid.NewString()
redirectURL := "http://127.0.0.1:62972/callback?secret_key=" + secretKey.String()
session := &services.SSOMFASessionData{
session := &services.MFASessionData{
RequestID: requestID,
Username: username,
TSHRedirectURL: redirectURL,
Expand Down Expand Up @@ -713,6 +713,7 @@ func TestBrowserMFAChallengeCreation(t *testing.T) {
if tt.setup != nil {
tt.setup(t)
}

chal, err := userClient.CreateAuthenticateChallenge(ctx, tt.challengeRequest)

if tt.checkError != nil {
Expand All @@ -729,3 +730,108 @@ func TestBrowserMFAChallengeCreation(t *testing.T) {
})
}
}

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

env := newBrowserMFATestEnv(t)
a := env.auth

for _, tt := range []struct {
name string
sd *services.MFASessionData
requestID string
checkError func(t *testing.T, err error)
assertValidation func(t *testing.T, sd *services.MFASessionData)
}{
{
name: "NOK session data not found",
sd: nil,
requestID: "nonexistent-request",
checkError: func(t *testing.T, err error) {
require.Error(t, err, "should fail when session data not found")
},
},
{
name: "OK session data retrieved correctly",
sd: &services.MFASessionData{
RequestID: "request1",
Username: env.webauthnUser.GetName(),
ConnectorID: constants.BrowserMFA,
ConnectorType: constants.BrowserMFA,
TSHRedirectURL: browserMFARedirectURL,
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN,
},
},
requestID: "request1",
assertValidation: func(t *testing.T, sd *services.MFASessionData) {
require.NotNil(t, sd)
assert.Equal(t, "request1", sd.RequestID)
assert.Equal(t, env.webauthnUser.GetName(), sd.Username)
assert.Equal(t, constants.BrowserMFA, sd.ConnectorID)
assert.Equal(t, constants.BrowserMFA, sd.ConnectorType)
assert.Equal(t, browserMFARedirectURL, sd.TSHRedirectURL)
assert.Equal(t, mfav1.ChallengeScope_CHALLENGE_SCOPE_LOGIN, sd.ChallengeExtensions.Scope)
},
},
{
name: "OK session data with allow reuse",
sd: &services.MFASessionData{
RequestID: "request2",
Username: env.webauthnUser.GetName(),
ConnectorID: constants.BrowserMFA,
ConnectorType: constants.BrowserMFA,
TSHRedirectURL: browserMFARedirectURL,
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION,
AllowReuse: mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES,
},
},
requestID: "request2",
assertValidation: func(t *testing.T, sd *services.MFASessionData) {
require.NotNil(t, sd)
assert.Equal(t, mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES, sd.ChallengeExtensions.AllowReuse)
},
},
{
name: "OK session data with admin action scope",
sd: &services.MFASessionData{
RequestID: "request3",
Username: env.webauthnUser.GetName(),
ConnectorID: constants.BrowserMFA,
ConnectorType: constants.BrowserMFA,
TSHRedirectURL: browserMFARedirectURL,
ChallengeExtensions: &mfatypes.ChallengeExtensions{
Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_ADMIN_ACTION,
},
},
requestID: "request3",
assertValidation: func(t *testing.T, sd *services.MFASessionData) {
require.NotNil(t, sd)
assert.Equal(t, mfav1.ChallengeScope_CHALLENGE_SCOPE_ADMIN_ACTION, sd.ChallengeExtensions.Scope)
},
},
} {
t.Run(tt.name, func(t *testing.T) {
if tt.sd != nil {
err := a.UpsertMFASessionData(ctx, tt.sd)
require.NoError(t, err)
}

sd, err := a.GetSSOMFASessionData(ctx, tt.requestID)

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

require.NoError(t, err)
if tt.assertValidation != nil {
tt.assertValidation(t, sd)
}
})
}
}
6 changes: 3 additions & 3 deletions lib/client/mfa/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,12 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
slog.DebugContext(ctx, "Disabling SSO MFA: SSO MFA ceremony not available (this is likely a bug)")
}

if state.promptBrowser && (!c.cfg.WebauthnSupported || c.cfg.MFACeremony == nil) {
if state.promptBrowser && (chal.WebauthnChallenge == nil || c.cfg.MFACeremony == nil) {
state.promptBrowser = false
Comment thread
danielashare marked this conversation as resolved.
slog.DebugContext(
ctx,
"Disabling Browser MFA: cluster needs to support Webauthn and client needs to support SSO MFA Ceremony",
"webauthn_supported", c.cfg.WebauthnSupported,
"Disabling Browser MFA: user needs at least one webauthn device and client needs to support SSO MFA Ceremony",
"webauthn_available", chal.WebauthnChallenge != nil,
"mfa_ceremony_available (if false, this is a bug)", c.cfg.MFACeremony != nil,
)
}
Expand Down
95 changes: 76 additions & 19 deletions lib/client/sso/ceremony.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import (

"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/mfa"
apiutils "github.com/gravitational/teleport/api/utils"
"github.com/gravitational/teleport/lib/auth/authclient"
wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
)

// Ceremony is a customizable SSO login ceremony.
Expand Down Expand Up @@ -107,6 +109,7 @@ type MFACeremony struct {
ProxyAddress string
HandleRedirect func(ctx context.Context, redirectURL string) error
GetCallbackMFAToken func(ctx context.Context) (string, error)
GetCallbackWebauthn func(ctx context.Context) (*wantypes.CredentialAssertionResponse, error)
}

// GetClientCallbackURL returns the client callback URL.
Expand All @@ -119,30 +122,60 @@ func (m *MFACeremony) GetProxyAddress() string {
return m.ProxyAddress
}

// Run the SSO MFA ceremony.
// Run the SSO/Browser MFA ceremony.
func (m *MFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
// TODO(danielashare): Remove when Browser MFA challenge handling is implemented
if chal.SSOChallenge == nil {
return nil, trace.BadParameter("no SSO challenge provided")
}
// The proxy will only ever return one of SSO or Browser challenge. However,
// check for SSO challenge first as it takes priority over browser MFA.
switch {
Comment thread
danielashare marked this conversation as resolved.
case chal.SSOChallenge != nil:
if err := m.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil {
return nil, trace.Wrap(err)
}

if err := m.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil {
return nil, trace.Wrap(err)
}
mfaToken, err := m.GetCallbackMFAToken(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

mfaToken, err := m.GetCallbackMFAToken(ctx)
if err != nil {
return nil, trace.Wrap(err)
}
return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_SSO{
SSO: &proto.SSOResponse{
RequestId: chal.SSOChallenge.RequestId,
Token: mfaToken,
},
},
}, nil
case chal.BrowserMFAChallenge != nil:
redirectURL, err := apiutils.ParseURL(m.ProxyAddress)
if err != nil {
return nil, trace.Wrap(err)
}
if redirectURL == nil {
return nil, trace.BadParameter("proxy address is required for browser MFA")
}
redirectURL.Scheme = "https"
redirectURL.Path = WebBrowserMFAPath + chal.BrowserMFAChallenge.RequestId

return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_SSO{
SSO: &proto.SSOResponse{
RequestId: chal.SSOChallenge.RequestId,
Token: mfaToken,
if err := m.HandleRedirect(ctx, redirectURL.String()); err != nil {
return nil, trace.Wrap(err)
}

webauthnResp, err := m.GetCallbackWebauthn(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_Browser{
Browser: &proto.BrowserMFAResponse{
RequestId: chal.BrowserMFAChallenge.RequestId,
WebauthnResponse: wantypes.CredentialAssertionResponseToProto(webauthnResp),
},
},
},
}, nil
}, nil
default:
return nil, trace.BadParameter("no SSO or Browser challenge provided")
}
}

// Close closes resources associated with the SSO MFA ceremony.
Expand Down Expand Up @@ -172,6 +205,18 @@ func NewCLIMFACeremony(rd *Redirector) *MFACeremony {

return loginResp.MFAToken, nil
},
GetCallbackWebauthn: func(ctx context.Context) (*wantypes.CredentialAssertionResponse, error) {
loginResp, err := rd.WaitForResponse(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

if loginResp.BrowserMFAWebauthnResponse == nil {
return nil, trace.BadParameter("login response for Browser MFA flow missing WebAuthn response")
Comment thread
danielashare marked this conversation as resolved.
}

return loginResp.BrowserMFAWebauthnResponse, nil
},
}
}

Expand All @@ -197,5 +242,17 @@ func NewConnectMFACeremony(rd *Redirector) mfa.CallbackCeremony {

return loginResp.MFAToken, nil
},
GetCallbackWebauthn: func(ctx context.Context) (*wantypes.CredentialAssertionResponse, error) {
loginResp, err := rd.WaitForResponse(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

if loginResp.BrowserMFAWebauthnResponse == nil {
return nil, trace.BadParameter("login response for Browser MFA flow missing WebAuthn response")
}

return loginResp.BrowserMFAWebauthnResponse, nil
},
}
}
6 changes: 5 additions & 1 deletion lib/client/sso/ceremony_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ const postform = `

func TestCLICeremony_MFA(t *testing.T) {
const token = "sso-mfa-token"
const requestID = "soo-mfa-request-id"
const requestID = "sso-mfa-request-id"

ctx := context.Background()
mockProxy := newMockProxy(t)
Expand Down Expand Up @@ -297,3 +297,7 @@ func TestCLICeremony_MFA(t *testing.T) {
assert.Equal(t, token, mfaResponse.GetSSO().Token)
assert.Equal(t, requestID, mfaResponse.GetSSO().RequestId)
}

// TODO(danielashare): Add full Browser MFA ceremony test once server-side
// Browser MFA functions have been merged. Test similar to above that tests
// CLI output, redirect handling, and MFA response.
Loading
Loading