Conversation
Joerger
force-pushed
the
joerger/add-second-factors
branch
from
3e32b81 to
d536667
Compare
Joerger
commented
Oct 4, 2024
Joerger
force-pushed
the
joerger/add-second-factors
branch
2 times, most recently
from
cd71433 to
10c8b33
Compare
Closed
zmb3
added
the
release-notes
tigrato
reviewed
Oct 7, 2024
codingllama
reviewed
Oct 8, 2024
Contributor
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Thanks for the separate PR, Brian! Apologies for the delay.
| default: | ||
| slog.WarnContext(context.Background(), "Found unknown second_factor setting", "second_factor", sf) | ||
| return "" // Unsure, say nothing. | ||
| return nil |
Contributor
There was a problem hiding this comment.
Should this error instead?
Contributor
Author
There was a problem hiding this comment.
I think we should just validate SecondFactor in CheckAndSetDefaults.
Contributor
There was a problem hiding this comment.
Would erroring here make for a clearer error?
Joerger
force-pushed
the
joerger/add-second-factors
branch
from
a170f2f to
653babe
Compare
codingllama
reviewed
Oct 8, 2024
codingllama
approved these changes
Oct 8, 2024
Joerger
force-pushed
the
joerger/add-second-factors
branch
2 times, most recently
from
a543532 to
06ce4f1
Compare
Contributor
|
🤖 Vercel preview here: https://docs-4jw07zxys-goteleport.vercel.app/docs/ver/preview |
codingllama
reviewed
Oct 9, 2024
| return trace.BadParameter("missing required Webauthn configuration for headless=true") | ||
| } | ||
|
|
||
| // Prevent accidental local lockout by disabling local second factor methods, (likely leaving only sso enabled). |
Contributor
There was a problem hiding this comment.
Could we get test coverage for this?
rosstimothy
reviewed
Oct 9, 2024
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…t when SSO is the only enabled MFA method; Ensure SecondFactors=[] is disallowed.
Joerger
force-pushed
the
joerger/add-second-factors
branch
from
4f4e9dd to
5e2c711
Compare
Contributor
|
🤖 Vercel preview here: https://docs-6xrjugnkq-goteleport.vercel.app/docs/ver/preview |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Oct 11, 2024
* Add proto. * Add decoding logic for SecondFactorType. * Update auth preference methods to use and prefer SecondFactors. * Add fileconf and warning logs. * Fix tests. * Address comments. * Address comments. * Validate SecondFactor; Disallow SecondFactor and SecondFactors to both be set. * Address comments. * Treat second factor SSO as SecondFactor=on; Prevent local user lockout when SSO is the only enabled MFA method; Ensure SecondFactors=[] is disallowed. * Upate terraform schema, docs, crds. * Address comments. * Address comments. * Fix lint, fix test.
mvbrock
pushed a commit
that referenced
this pull request
Oct 16, 2024
* Add proto. * Add decoding logic for SecondFactorType. * Update auth preference methods to use and prefer SecondFactors. * Add fileconf and warning logs. * Fix tests. * Address comments. * Address comments. * Validate SecondFactor; Disallow SecondFactor and SecondFactors to both be set. * Address comments. * Treat second factor SSO as SecondFactor=on; Prevent local user lockout when SSO is the only enabled MFA method; Ensure SecondFactors=[] is disallowed. * Upate terraform schema, docs, crds. * Address comments. * Address comments. * Fix lint, fix test.
Collaborator
|
@Joerger did we ever got docs updates for this one? |
Contributor
Author
No not yet, it's on my TODO list, along with SSO MFA docs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
second_factorsand prefer it oversecond_factor.We don't currently plan on removing second_factor, as this would require a more complicated migration process. Instead we will just derive
second_factorsfromsecond_factor(and vice versa) and output a warning log whensecond_factoris set.There is no plan to deprecate
second_factorcompletely. Whensecond_factoris set andsecond_factorsis not, or vice versa, we convert from one to the other.In a follow up PR I will update as much logic as possible to use
second_factorsinstead ofsecond_factor, as they are two sources of the same information.In this PR I've also added the SSO second_factor type. It is currently completely unused, but we'd rather get the proto changes into v17 rather than waiting until SSO MFA is fully released in a minor version.
Follow up TODO: Update docs.
Changelog: Add new second_factors field to cluster auth preference for more clarity and granularity over which 2fa methods are enabled in a cluster.
Depends on #47230