Skip to content

[Browser MFA] Add protobuf and config#63831

Merged
danielashare merged 1 commit into
masterfrom
danielashare/browser-mfa-proto
Mar 2, 2026
Merged

[Browser MFA] Add protobuf and config#63831
danielashare merged 1 commit into
masterfrom
danielashare/browser-mfa-proto

Conversation

@danielashare
Copy link
Copy Markdown
Contributor

This PR adds protobuf and the configuration for enabling Browser MFA. The RFD for these additions can be found here.

Manual tests:

  • Browser MFA enabled by default when WebAuthn enabled
  • Browser MFA disabled by default when WebAuthn disabled
  • Error emitted when Browser MFA explicitly enabled and WebAuthn disabled

@danielashare danielashare self-assigned this Feb 16, 2026
@danielashare danielashare added the no-changelog Indicates that a PR does not require a changelog entry label Feb 16, 2026
@danielashare danielashare force-pushed the danielashare/browser-mfa-proto branch from 8e9cfa5 to f5df2e4 Compare February 16, 2026 12:12
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
message ValidateBrowserMFAChallengeResponse {
// tsh_redirect_url is the callback URL to tsh's local HTTP server with the encrypted WebAuthn response.
// Format: http://127.0.0.1:[random_port]/callback?response={encrypted_webauthn_response}
string tsh_redirect_url = 1;
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.

I don't think it was mentioned in the RFD, but will there be any protection against creating an open-redirect logic on the web frontend side?

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.

I wasn't planning on adding redirect validation on the web frontend because the redirect url is accepted once from the user at the start of the ceremony and validated by ValidateClientRedirect. The URL is then stored on the backend waiting for the user to MFA in the browser. After doing so, the auth server encrypts the WebAuthn response and adds it to the stored and validated redirect URL for returning to the browser.

Comment on lines +2737 to +2739
// AllowBrowserAuthentication enables/disables browser-based authentication.
// When set to false, authentication flows that require a browser will be disabled.
// Defaults to true.
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.

The way it's phrased suggests that any kind of local login (including regular sign-ins to the web app) would be gated by this option. Is that the case? If not, the documentation should be a bit more clear that we're only talking about the browser MFA flow for tsh.

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.

That is not the case, this is only available to CLI tools. I'll make sure this is clearer in this comment and in documentation.

string proxy_address_for_sso = 4;
// Used to construct the redirect URL for browser-based MFA flows. If the client supports browser MFA, this field
// should be set to the URL where the browser should redirect to tsh after completing the MFA challenge.
string browser_mfa_tsh_redirect_url = 5;
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.

Will this URL be validated by the auth server?

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, this will also be validated by ValidateClientRedirect on the server side

// per-session MFA but we may still need to know that the user has TOTP
// configured as an option.
bool per_session_mfa = 7;
BrowserMFAChallenge browser = 8;
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.

What's the role of this field? I also see it mentioned in the RFD, but I don't understand how this all relates to teleterm.

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.

When a user initiates a request in teleterm that requires MFA, a request is sent by tsh daemon to Teleport and Teleport replies with the available challenges. The daemon will then create this PromptMFARequest message and forward it to teleterm, which will then show a modal for the user to choose their method of MFA. So this field just lets teleterm know that Browser MFA is an MFA option

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 18, 2026

Amplify deployment status

Branch Commit Job ID Status Preview Updated (UTC)
danielashare/browser-mfa-proto 8a5561b 14 ✅SUCCEED danielashare-browser-mfa-proto 2026-03-02 07:45:42

Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread api/proto/teleport/mfa/v1/service.proto
Comment thread api/types/authentication.go Outdated
Comment thread api/types/authentication.go Outdated
Comment thread lib/client/api_login_test.go
Comment thread lib/config/fileconf.go Outdated
Comment thread lib/config/fileconf_test.go
Comment thread lib/config/fileconf_test.go Outdated
Comment thread api/types/authentication.go Outdated
Comment on lines +827 to +832
case c.Spec.AllowBrowserAuthentication == nil:
// Materialize AllowBrowserAuthentication to true if WebAuthn is enabled,
// otherwise leave it nil.
if hasWebauthn {
c.Spec.AllowBrowserAuthentication = NewBoolOption(true)
}
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.

Suggested change
case c.Spec.AllowBrowserAuthentication == nil:
// Materialize AllowBrowserAuthentication to true if WebAuthn is enabled,
// otherwise leave it nil.
if hasWebauthn {
c.Spec.AllowBrowserAuthentication = NewBoolOption(true)
}
case hasWebauthn && c.Spec.AllowBrowserAuthentication == nil:
// Materialize AllowBrowserAuthentication to true if WebAuthn is enabled,
// otherwise leave it nil.
c.Spec.AllowBrowserAuthentication = NewBoolOption(true)

It also occurs to me that we could never materialize and always calculate on the fly. Maybe add a variant of GetAllowBrowserAuthentication() that does that, so the logic is all captured here.

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.

Friendly ping?

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.

Sounds good to me, left a validation check in CheckAndSetDefaults and calculate the rest on the fly in the getter

Comment thread api/types/authentication.go Outdated
Comment thread proto/teleport/lib/teleterm/v1/tshd_events_service.proto Outdated
@codingllama
Copy link
Copy Markdown
Contributor

Looks good, a few minor comments only.

Improve comments

grpc, lint, test fix

Update tf docs

Update tf integration

Fix login test

Address comments

Fix test

Move ValidateBrowserMFAChallenge rpc to MFA service

Fix service test

Address comments

Don't materialise browser authentication

Remove browser auth materialisation in test

Convert validate browser mfa to complete browser mfa
@danielashare danielashare force-pushed the danielashare/browser-mfa-proto branch from 0cb377a to 8a5561b Compare March 2, 2026 07:40
@danielashare danielashare added this pull request to the merge queue Mar 2, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 2, 2026
@danielashare danielashare added this pull request to the merge queue Mar 2, 2026
Merged via the queue into master with commit 36120b3 Mar 2, 2026
47 checks passed
@danielashare danielashare deleted the danielashare/browser-mfa-proto branch March 2, 2026 09:08
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@danielashare See the table below for backport results.

Branch Result
branch/v18 Failed

mmcallister pushed a commit that referenced this pull request Apr 28, 2026
Improve comments

grpc, lint, test fix

Update tf docs

Update tf integration

Fix login test

Address comments

Fix test

Move ValidateBrowserMFAChallenge rpc to MFA service

Fix service test

Address comments

Don't materialise browser authentication

Remove browser auth materialisation in test

Convert validate browser mfa to complete browser mfa
danielashare added a commit that referenced this pull request May 6, 2026
[Browser MFA] Add protobuf and config (#63831)

[Browser MFA] Add proto for Browser MFA feature (#64048)

[Browser MFA] Add CompleteBrowserMFAChallenge gRPC (#63873)

[Browser MFA] Rename browser mfa config name (#64980)

[Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (#63945)

[Browser MFA] Add Browser MFA to challenge request flow (#63936)

[Browser MFA] Add initial requests for browser MFA process to client tools (#64301)

[Browser MFA] Add tsh callback handling for webauthn response (#64461)

[Browser MFA] Add Browser MFA to presence checks (#65052)

[Browser MFA] Add browser MFA path to MFA finish flow (#64523)

[Browser MFA] Add Browser MFA to Connect (#64887)

[Browser MFA] Add Browser MFA UI (#64692)

[Browser MFA] Fix formatting in moderated sessions (#65236)

[Browser MFA] Add Browser MFA ceremony tests
ivan-bax pushed a commit to ivan-bax/teleport that referenced this pull request May 22, 2026
[Browser MFA] Add protobuf and config (gravitational#63831)

[Browser MFA] Add proto for Browser MFA feature (gravitational#64048)

[Browser MFA] Add CompleteBrowserMFAChallenge gRPC (gravitational#63873)

[Browser MFA] Rename browser mfa config name (gravitational#64980)

[Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (gravitational#63945)

[Browser MFA] Add Browser MFA to challenge request flow (gravitational#63936)

[Browser MFA] Add initial requests for browser MFA process to client tools (gravitational#64301)

[Browser MFA] Add tsh callback handling for webauthn response (gravitational#64461)

[Browser MFA] Add Browser MFA to presence checks (gravitational#65052)

[Browser MFA] Add browser MFA path to MFA finish flow (gravitational#64523)

[Browser MFA] Add Browser MFA to Connect (gravitational#64887)

[Browser MFA] Add Browser MFA UI (gravitational#64692)

[Browser MFA] Fix formatting in moderated sessions (gravitational#65236)

[Browser MFA] Add Browser MFA ceremony tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants