[Browser MFA] Rename browser mfa config name#64980
Conversation
|
Amplify deployment status
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d190e283
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f2d190e to
3b00c76
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b00c76f40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
3b00c76 to
b5005f9
Compare
b5005f9 to
aebf3d0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aebf3d0c76
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
aebf3d0 to
c8099c5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8099c5a40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c8099c5 to
6edf70c
Compare
| // When set to false, authentication flows that require a browser will be disabled. | ||
| // Defaults to true. | ||
| BoolValue AllowBrowserAuthentication = 23 [ | ||
| deprecated = true, |
There was a problem hiding this comment.
Just a heads up that deprecations can break e/ if the field is used there, due to failing linters. (Not a problem if it's only used in OSS.)
There was a problem hiding this comment.
Thankfully it isn't used by e
| // authenticating CLI sessions. | ||
| // When set to false, authentication flows that require a browser will be disabled. | ||
| // Defaults to true. | ||
| BoolValue AllowBrowserAuthentication = 23 [ |
There was a problem hiding this comment.
Could we delete/reserve this instead?
There was a problem hiding this comment.
+1, I would just remove this completely, and then add
reserve 23;
reserve AllowBrowserAuthentication;
There was a problem hiding this comment.
Like Alan said, we can go a step further and just reuse message 23 for AllowCLIAuthViaBrowser since there is no releases depending on it.
There was a problem hiding this comment.
I would just do the new tag. Reusing means a whole lot of fighting with the breaking changes linter, which is likely not worth it to save a single tag number. When Dan I discussed this out-of-band I suggested him to follow the "normal" deprecation process.
There was a problem hiding this comment.
Thanks guys, I've gone the reserve route to get rid of this field
| if c.Spec.AllowCLIAuthViaBrowser != nil { | ||
| return c.Spec.AllowCLIAuthViaBrowser.Value |
There was a problem hiding this comment.
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.
2baa264 to
b9519c9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9519c906c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| BoolValue AllowBrowserAuthentication = 23 [ | ||
| // Defaults to true if the Webauthn is configured, defaults to false | ||
| // otherwise. | ||
| BoolValue AllowCLIAuthViaBrowser = 24 [ |
There was a problem hiding this comment.
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 👍 / 👎.
b9519c9 to
b0d6bf6
Compare
|
@danielashare See the table below for backport results.
|
[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
[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
Deprecate
AllowBrowserAuthenticationand addAllowCLIAuthViaBrowserto replace it, to make it clearer to the user what is being enabled.Manual Test Plan
Test Environment
Running local Teleport
Test Cases