Skip to content

[Browser MFA] Rename browser mfa config name #64980

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.

Sign up for GitHub

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

Merged
danielashare merged 1 commit into from
Mar 26, 2026
Merged

[Browser MFA] Rename browser mfa config name #64980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2743,13 +2743,17 @@ message AuthPreferenceSpecV2 {
// UNIX users.
StableUNIXUserConfig stable_unix_user_config = 22;

// AllowBrowserAuthentication enables/disables browser-based authentication for
reserved 23; // AllowBrowserAuthentication replaced by AllowCLIAuthViaBrowser
reserved "AllowBrowserAuthentication";

// AllowCLIAuthViaBrowser enables/disables browser-based authentication for
// authenticating CLI sessions.
// When set to false, authentication flows that require a browser will be disabled.
// Defaults to true.
BoolValue AllowBrowserAuthentication = 23 [
// Defaults to true if the Webauthn is configured, defaults to false
// otherwise.
BoolValue AllowCLIAuthViaBrowser = 24 [
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 25, 2026

Choose a reason for hiding this comment

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

P1 Badge Keep AuthPreference protobuf tag stable for browser auth

Changing AllowCLIAuthViaBrowser to protobuf field 24 (while reserving 23) breaks binary compatibility for mixed-version clients and servers that still send AllowBrowserAuthentication on tag 23 through clusterconfigv1 gRPC (Get/Update/UpsertAuthPreference use types.AuthPreferenceV2). Fresh evidence: this commit now reserves tag 23, so legacy values can no longer populate a known field and GetAllowCLIAuthViaBrowser() falls back to the WebAuthn-derived default, which can silently re-enable browser-based CLI auth after upgrade.

Useful? React with 👍 / 👎.

(gogoproto.nullable) = true,
(gogoproto.jsontag) = "allow_browser_authentication,omitempty",
(gogoproto.jsontag) = "allow_cli_auth_via_browser,omitempty",
Comment thread
danielashare marked this conversation as resolved.
(gogoproto.customtype) = "BoolOption"
];
}
Expand Down
26 changes: 13 additions & 13 deletions api/types/authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@ type AuthPreference interface {
// SetAllowHeadless sets the value of the allow headless setting.
SetAllowHeadless(b bool)

// GetAllowBrowserAuthentication returns if browser authentication is allowed by cluster settings.
GetAllowBrowserAuthentication() bool
// SetAllowBrowserAuthentication sets the value of the allow browser authentication setting.
SetAllowBrowserAuthentication(b bool)
// GetAllowCLIAuthViaBrowser returns if cli auth via browser is allowed by cluster settings.
GetAllowCLIAuthViaBrowser() bool
// SetAllowCLIAuthViaBrowser sets the value of the allow cli auth via browser setting.
SetAllowCLIAuthViaBrowser(b bool)

// SetRequireMFAType sets the type of MFA requirement enforced for this cluster.
SetRequireMFAType(RequireMFAType)
Expand Down Expand Up @@ -472,20 +472,20 @@ func (c *AuthPreferenceV2) SetAllowHeadless(b bool) {
c.Spec.AllowHeadless = NewBoolOption(b)
}

// GetAllowBrowserAuthentication returns whether browser authentication is
// GetAllowCLIAuthViaBrowser returns whether cli auth via browser is
// allowed. If it's not explicitly configured, it defaults to true when
// WebAuthn is enabled as a second factor.
func (c *AuthPreferenceV2) GetAllowBrowserAuthentication() bool {
if c.Spec.AllowBrowserAuthentication != nil {
return c.Spec.AllowBrowserAuthentication.Value
func (c *AuthPreferenceV2) GetAllowCLIAuthViaBrowser() bool {
if c.Spec.AllowCLIAuthViaBrowser != nil {
return c.Spec.AllowCLIAuthViaBrowser.Value
Comment on lines +479 to +480
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 24, 2026

Choose a reason for hiding this comment

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

P1 Badge Read deprecated browser-auth field before defaulting

AllowBrowserAuthentication is still accepted on the wire (field 23 in types.proto) and still populated by compatibility paths (for example Terraform copies into obj.AllowBrowserAuthentication), but GetAllowCLIAuthViaBrowser now checks only Spec.AllowCLIAuthViaBrowser. Any legacy payload that sets only the deprecated field will be silently ignored here, so behavior falls back to the default (IsSecondFactorWebauthnAllowed()), which can re-enable browser-based CLI auth after upgrade even when legacy config explicitly disabled it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@bernardjkim bernardjkim Mar 24, 2026

Choose a reason for hiding this comment

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

^ Do we want to deprecate AllowCLIAuthViaBrowser but continue to support it as a legacy option, or do we want to fully replace it. If we want to fully replace it here, we should remove AllowBrowserAuthentication from the AuthPreferenceV2 spec.

Copy link
Copy Markdown
Contributor Author

@danielashare danielashare Mar 25, 2026

Choose a reason for hiding this comment

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

Ideally, I would've fully replaced AllowBrowserAuthentication with AllowCLIAuthViaBrowser, but AllowBrowserAuthentication is already in master so to follow best practise I've deprecated one and created another. However, AllowBrowserAuthentication has only been in master for a couple weeks and nothing/no one is using it. So I didn't think it was worth supporting it as a legacy option. WDYT?

Copy link
Copy Markdown
Contributor

@codingllama codingllama Mar 25, 2026

Choose a reason for hiding this comment

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

If this wasn't part of an active release we can delete the old field and "replace" it with the new one.

If it were part of an active release then it's a different conversation.

}

// Default to enabled when WebAuthn is enabled.
return c.IsSecondFactorWebauthnAllowed()
}

func (c *AuthPreferenceV2) SetAllowBrowserAuthentication(b bool) {
c.Spec.AllowBrowserAuthentication = NewBoolOption(b)
func (c *AuthPreferenceV2) SetAllowCLIAuthViaBrowser(b bool) {
c.Spec.AllowCLIAuthViaBrowser = NewBoolOption(b)
}

// SetRequireMFAType sets the type of MFA requirement enforced for this cluster.
Expand Down Expand Up @@ -830,9 +830,9 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
return trace.BadParameter("missing required Webauthn configuration for headless=true")
}

// Validate AllowBrowserAuthentication. WebAuthn is required for browser authentication.
if !hasWebauthn && c.Spec.AllowBrowserAuthentication != nil && c.Spec.AllowBrowserAuthentication.Value {
return trace.BadParameter("missing required Webauthn configuration for allow_browser_authentication=true")
// Validate AllowCLIAuthViaBrowser. WebAuthn is required for cli auth via browser.
if !hasWebauthn && c.Spec.AllowCLIAuthViaBrowser != nil && c.Spec.AllowCLIAuthViaBrowser.Value {
return trace.BadParameter("missing required Webauthn configuration for allow_cli_auth_via_browser=true")
Comment thread
danielashare marked this conversation as resolved.
}

// Prevent local lockout by disabling local second factor methods.
Expand Down
54 changes: 27 additions & 27 deletions api/types/authentication_authpreference_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,81 +632,81 @@ func TestAuthPreferenceV2_CheckAndSetDefaults_secondFactor(t *testing.T) {
},
wantErr: "invalid local connector",
},
// AllowBrowserAuthentication
// AllowCLIAuthViaBrowser
{
name: "OK AllowBrowserAuthentication defaults to false without Webauthn",
name: "OK AllowCLIAuthViaBrowser defaults to false without Webauthn",
secondFactors: []constants.SecondFactorType{
constants.SecondFactorOff,
constants.SecondFactorOTP,
},
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
AllowBrowserAuthentication: nil, // aka unset
Type: constants.Local,
AllowCLIAuthViaBrowser: nil, // aka unset
},
assertFn: func(t *testing.T, cap *types.AuthPreferenceV2) {
assert.False(t, cap.GetAllowBrowserAuthentication(), "AllowBrowserAuthentication")
assert.False(t, cap.GetAllowCLIAuthViaBrowser(), "AllowCLIAuthViaBrowser")
},
},
{
name: "OK AllowBrowserAuthentication=false without Webauthn",
name: "OK AllowCLIAuthViaBrowser=false without Webauthn",
secondFactors: []constants.SecondFactorType{
constants.SecondFactorOff,
constants.SecondFactorOTP,
},
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
AllowBrowserAuthentication: types.NewBoolOption(false),
Type: constants.Local,
AllowCLIAuthViaBrowser: types.NewBoolOption(false),
},
assertFn: func(t *testing.T, cap *types.AuthPreferenceV2) {
assert.False(t, cap.GetAllowBrowserAuthentication(), "AllowBrowserAuthentication")
assert.False(t, cap.GetAllowCLIAuthViaBrowser(), "AllowCLIAuthViaBrowser")
},
},
{
name: "NOK AllowBrowserAuthentication=true without Webauthn",
name: "NOK AllowCLIAuthViaBrowser=true without Webauthn",
secondFactors: []constants.SecondFactorType{
constants.SecondFactorOff,
constants.SecondFactorOTP,
},
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
AllowBrowserAuthentication: types.NewBoolOption(true),
Type: constants.Local,
AllowCLIAuthViaBrowser: types.NewBoolOption(true),
},
wantErr: "required Webauthn",
},
{
name: "OK AllowBrowserAuthentication defaults to true with Webauthn",
name: "OK AllowCLIAuthViaBrowser defaults to true with Webauthn",
secondFactors: secondFactorWebActive,
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
Webauthn: minimalWeb,
AllowBrowserAuthentication: nil, // aka unset
Type: constants.Local,
Webauthn: minimalWeb,
AllowCLIAuthViaBrowser: nil, // aka unset
},
assertFn: func(t *testing.T, cap *types.AuthPreferenceV2) {
assert.True(t, cap.GetAllowBrowserAuthentication(), "AllowBrowserAuthentication")
assert.True(t, cap.GetAllowCLIAuthViaBrowser(), "AllowCLIAuthViaBrowser")
},
},
{
name: "OK AllowBrowserAuthentication=false with Webauthn",
name: "OK AllowCLIAuthViaBrowser=false with Webauthn",
secondFactors: secondFactorWebActive,
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
Webauthn: minimalWeb,
AllowBrowserAuthentication: types.NewBoolOption(false),
Type: constants.Local,
Webauthn: minimalWeb,
AllowCLIAuthViaBrowser: types.NewBoolOption(false),
},
assertFn: func(t *testing.T, cap *types.AuthPreferenceV2) {
assert.False(t, cap.GetAllowBrowserAuthentication(), "AllowBrowserAuthentication")
assert.False(t, cap.GetAllowCLIAuthViaBrowser(), "AllowCLIAuthViaBrowser")
},
},
{
name: "OK AllowBrowserAuthentication=true with Webauthn",
name: "OK AllowCLIAuthViaBrowser=true with Webauthn",
secondFactors: secondFactorWebActive,
spec: types.AuthPreferenceSpecV2{
Type: constants.Local,
Webauthn: minimalWeb,
AllowBrowserAuthentication: types.NewBoolOption(true),
Type: constants.Local,
Webauthn: minimalWeb,
AllowCLIAuthViaBrowser: types.NewBoolOption(true),
},
assertFn: func(t *testing.T, cap *types.AuthPreferenceV2) {
assert.True(t, cap.GetAllowBrowserAuthentication(), "AllowBrowserAuthentication")
assert.True(t, cap.GetAllowCLIAuthViaBrowser(), "AllowCLIAuthViaBrowser")
},
},
}
Expand Down
Loading
Loading