Conversation
github-actions
Bot
added
size/md
tctl
Joerger
force-pushed
the
joerger/deprectate-password-login-endpoint
branch
from
515cd9b to
20d569d
Compare
Joerger
force-pushed
the
joerger/second-factors
branch
2 times, most recently
from
525ff1e to
2425453
Compare
…actor from SecondFactors for WebUI ping response.
Joerger
force-pushed
the
joerger/second-factors
branch
from
f8db35d to
04fd4a4
Compare
Joerger
force-pushed
the
joerger/second-factors
branch
from
04fd4a4 to
9dd277c
Compare
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Taking a quick look, I'm not certain we should deprecate second_factor (see comment threads).
If we do go that way, I think we should make this PR only about adding second_factors, and follow up with a deprecation and widespread changes separately. It helps greatly in bounding the review.
| } | ||
|
|
||
| // SecondFactorType is the type of 2FA authentication. | ||
| // Deprecated: Use types.SecondFactorType |
There was a problem hiding this comment.
Will this cause problems on e/ ?
(I'm not sure deprecating this is wise, see other comment threads.)
| // The current default value is "legacy". This field is not yet fully supported. | ||
| SignatureAlgorithmSuite signature_algorithm_suite = 20; | ||
|
|
||
| // SecondFactors is a list of supported second factor types. |
There was a problem hiding this comment.
| // SecondFactors is a list of supported second factor types. | |
| // SecondFactors is a list of supported second factor types, in ascending | |
| // order of preference (first item is preferred). |
There was a problem hiding this comment.
and deprecate second_factor
Do we have to? Every config out there uses it, plus I see little harm in keeping it around.
| rcConf.DataDir = t.TempDir() | ||
| rcConf.Auth.Enabled = true | ||
| rcConf.Auth.Preference.SetSecondFactor("off") | ||
| rcConf.Auth.Preference.SetSecondFactor(constants.SecondFactorOff) |
There was a problem hiding this comment.
Taking the PR as-is, isn't this introducing a new usage of a deprecated constant?
| // decodeEnum decodes a protobuf enum from a representational value, usually a bool, | ||
| // string, or from the actual enum (int32) value. If the value is valid, it is saved | ||
| // in the given enum pointer. | ||
| func decodeEnum[T ~int32](p *T, val any, representationMap map[any]T, enumMap map[int32]string) error { |
There was a problem hiding this comment.
I'd be tempted to punt on this change for this PR, or move it to a separate isolated PR that could be backported without conflict. There is a decent amount going on in this PR and the additional updates to start using this helper add extra noise.
We don't plan on removing
I will try to split this into a couple PRs. |
|
Thanks, Brian! |
3 participants
Add
second_factorsand deprecatesecond_factorWe don't currently plan on removing
second_factor, as this would require a more complicated migration process. Instead we will just derivesecond_factorsfromsecond_factorand output a warning log whensecond_factoris set.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.
Note: I have not propogated the
second_factorschange to the Teleport Connect or WebUI, meaning those still rely on thesecond_factorfrom proxy ping response. I will do this in a follow up PR.Based off #47153 due to reduced dependence on second_factor from proxy ping response.
TODO: I expect some test breakages, will fix after opening. Also cleanup.
Follow up TODO: Update docs.
Changelog: Add new
second_factorsfield to cluster auth preference for more clarity and granularity over which 2fa methods are enabled in a cluster.