-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[Browser MFA] Add browser MFA path to MFA finish flow #64523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8151,7 +8151,7 @@ func groupByDeviceType(devs []*types.MFADevice) devicesByType { | |
| // Use only for registration purposes. | ||
| func (a *Server) validateMFAAuthResponseForRegister(ctx context.Context, resp *proto.MFAAuthenticateResponse, username string, requiredExtensions *mfav1.ChallengeExtensions) (hasDevices bool, err error) { | ||
| // Let users without a useable device go through registration. | ||
| if resp == nil || (resp.GetTOTP() == nil && resp.GetWebauthn() == nil && resp.GetSSO() == nil) { | ||
| if resp == nil || (resp.GetTOTP() == nil && resp.GetWebauthn() == nil && resp.GetSSO() == nil && resp.GetBrowser() == nil) { | ||
| devices, err := a.Services.GetMFADevices(ctx, username, false /* withSecrets */) | ||
| if err != nil { | ||
| return false, trace.Wrap(err) | ||
|
|
@@ -8170,8 +8170,9 @@ func (a *Server) validateMFAAuthResponseForRegister(ctx context.Context, resp *p | |
| hasTOTP := authPref.IsSecondFactorTOTPAllowed() && devsByType.TOTP | ||
| hasWebAuthn := authPref.IsSecondFactorWebauthnAllowed() && len(devsByType.Webauthn) > 0 | ||
| hasSSO := authPref.IsSecondFactorSSOAllowed() && devsByType.SSO != nil | ||
| hasBrowser := authPref.GetAllowCLIAuthViaBrowser() && devsByType.Browser != nil | ||
|
|
||
| if hasTOTP || hasWebAuthn || hasSSO { | ||
| if hasTOTP || hasWebAuthn || hasSSO || hasBrowser { | ||
| return false, trace.BadParameter("second factor authentication required") | ||
| } | ||
|
|
||
|
|
@@ -8240,6 +8241,7 @@ func (a *Server) ValidateMFAAuthResponse( | |
| auditEvent.Code = events.ValidateMFAAuthResponseCode | ||
| auditEvent.Success = true | ||
| deviceMetadata := mfaDeviceEventMetadata(authData.Device) | ||
| deviceMetadata.MFAViaBrowser = authData.MFAViaBrowser | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love that I set the |
||
| auditEvent.MFADevice = &deviceMetadata | ||
| auditEvent.ChallengeAllowReuse = authData.AllowReuse == mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES | ||
| } | ||
|
|
@@ -8352,6 +8354,9 @@ func (a *Server) validateMFAAuthResponseInternal( | |
| case *proto.MFAAuthenticateResponse_SSO: | ||
| mfaAuthData, err := a.VerifySSOMFASession(ctx, user, res.SSO.RequestId, res.SSO.Token, requiredExtensions) | ||
| return mfaAuthData, trace.Wrap(err) | ||
| case *proto.MFAAuthenticateResponse_Browser: | ||
| mfaAuthData, err := a.VerifyBrowserMFASession(ctx, user, res.Browser.RequestId, res.Browser.WebauthnResponse, requiredExtensions) | ||
|
danielashare marked this conversation as resolved.
|
||
| return mfaAuthData, trace.Wrap(err) | ||
| default: | ||
| return nil, trace.BadParameter("unknown or missing MFAAuthenticateResponse type %T", resp.Response) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,9 +25,11 @@ import ( | |
|
|
||
| "github.com/gravitational/teleport/api/client/proto" | ||
| "github.com/gravitational/teleport/api/constants" | ||
| mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" | ||
| webauthnpb "github.com/gravitational/teleport/api/types/webauthn" | ||
| "github.com/gravitational/teleport/lib/auth/internal/browsermfa" | ||
| "github.com/gravitational/teleport/lib/auth/mfatypes" | ||
| wanlib "github.com/gravitational/teleport/lib/auth/webauthn" | ||
| wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" | ||
| "github.com/gravitational/teleport/lib/authz" | ||
| "github.com/gravitational/teleport/lib/client/sso" | ||
|
|
@@ -105,3 +107,121 @@ func (a *Server) BeginBrowserMFAChallenge(ctx context.Context, params mfatypes.B | |
|
|
||
| return browserChal, nil | ||
| } | ||
|
|
||
| // VerifyBrowserMFASession verifies that the given Browser MFA webauthn response matches an existing MFA session | ||
| // for the user and session ID. It also checks the required extensions, and finishes by deleting | ||
| // the MFA session if reuse is not allowed. | ||
| func (a *Server) VerifyBrowserMFASession(ctx context.Context, username, sessionID string, webauthnResponse *webauthnpb.CredentialAssertionResponse, requiredExtensions *mfav1.ChallengeExtensions) (*authz.MFAAuthData, error) { | ||
| if requiredExtensions == nil { | ||
| return nil, trace.BadParameter("requested challenge extensions must be supplied.") | ||
| } | ||
|
|
||
| if webauthnResponse == nil { | ||
| return nil, trace.BadParameter("webauthn response must be supplied") | ||
| } | ||
|
|
||
| const notFoundErrMsg = "browser MFA session data not found" | ||
| mfaSess, err := a.GetMFASessionData(ctx, sessionID) | ||
| if trace.IsNotFound(err) { | ||
| return nil, trace.NotFound("%s", notFoundErrMsg) | ||
| } else if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
|
|
||
| // Verify the user's name matches. | ||
| if mfaSess.Username != username { | ||
| return nil, trace.NotFound("%s", notFoundErrMsg) | ||
| } | ||
|
danielashare marked this conversation as resolved.
|
||
|
|
||
| // Verify this is a Browser MFA session and not an SSO MFA session. | ||
| if mfaSess.TSHRedirectURL == "" || mfaSess.ConnectorType != constants.BrowserMFA { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add this same check in |
||
| a.logger.WarnContext(ctx, | ||
| "The Browser MFA flow was used to access a SSO MFA session.", | ||
| "request_id", mfaSess.RequestID, | ||
| "connector_type", mfaSess.ConnectorType, | ||
| "username", username, | ||
| ) | ||
| return nil, trace.NotFound("%s", notFoundErrMsg) | ||
| } | ||
|
|
||
| // Check if the MFA session matches the user's Browser MFA settings. | ||
| devs, err := a.Services.GetMFADevices(ctx, username, false /* withSecrets */) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
|
|
||
| // Check the user has a Browser MFA device | ||
| groupedDevs := groupByDeviceType(devs) | ||
| if groupedDevs.Browser == nil { | ||
| if len(groupedDevs.Webauthn) == 0 { | ||
| a.logger.DebugContext(ctx, | ||
| "Browser MFA not available: user has no WebAuthn devices registered", | ||
| "user", username, | ||
| ) | ||
| } | ||
| return nil, trace.AccessDenied("browser MFA not available") | ||
| } | ||
|
|
||
| // Check if the given scope is satisfied by the challenge scope. | ||
| if requiredExtensions.Scope != mfaSess.ChallengeExtensions.Scope { | ||
| return nil, trace.AccessDenied( | ||
| "required scope %q is not satisfied by the given browser MFA session with scope %q", | ||
| requiredExtensions.Scope, | ||
| mfaSess.ChallengeExtensions.Scope, | ||
| ) | ||
| } | ||
|
|
||
| // If this session is reusable, but this context forbids reusable sessions, return an error. | ||
| reuseNotPermitted := requiredExtensions.AllowReuse == mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO | ||
| sessionAllowsReuse := mfaSess.ChallengeExtensions.AllowReuse == mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES | ||
| if reuseNotPermitted && sessionAllowsReuse { | ||
| return nil, trace.AccessDenied("the given browser MFA session allows reuse, but reuse is not permitted in this context") | ||
| } | ||
|
|
||
| // Convert from protobuf type to wantypes | ||
| wanResp := wantypes.CredentialAssertionResponseFromProto(webauthnResponse) | ||
|
|
||
| cap, err := a.GetAuthPreference(ctx) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| waConfig, err := cap.GetWebauthn() | ||
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| u2f, err := cap.GetU2F() | ||
| if err != nil && !trace.IsNotFound(err) { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
|
|
||
| loginFlow := &wanlib.LoginFlow{ | ||
| Webauthn: waConfig, | ||
| U2F: u2f, | ||
| Identity: a.Services, | ||
|
danielashare marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Verify webauthn response | ||
| loginData, err := loginFlow.Finish(ctx, username, wanResp, &mfav1.ChallengeExtensions{ | ||
| Scope: mfaSess.ChallengeExtensions.Scope, | ||
| AllowReuse: mfaSess.ChallengeExtensions.AllowReuse, | ||
| }) | ||
|
danielashare marked this conversation as resolved.
danielashare marked this conversation as resolved.
danielashare marked this conversation as resolved.
danielashare marked this conversation as resolved.
|
||
| if err != nil { | ||
| return nil, trace.AccessDenied("failed to verify WebAuthn response: %v", err) | ||
| } | ||
|
|
||
| if mfaSess.ChallengeExtensions.AllowReuse != mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES { | ||
| if err := a.DeleteMFASessionData(ctx, sessionID); err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| } | ||
|
|
||
| return &authz.MFAAuthData{ | ||
| Device: loginData.Device, | ||
| User: username, | ||
| AllowReuse: mfaSess.ChallengeExtensions.AllowReuse, | ||
| Payload: mfaSess.Payload, | ||
| SourceCluster: mfaSess.SourceCluster, | ||
| TargetCluster: mfaSess.TargetCluster, | ||
| MFAViaBrowser: true, | ||
| }, nil | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.