Skip to content
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.

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

feat: SSO MFA - Add ephemeral SSO MFA device #46704

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Sep 18, 2024

Part of the implementation of SSO MFA

This PR adds an ephemeral SSO MFA device for users that originated from an Auth connector with MFA enabled. This SSO device can be seen with tsh mfa ls or through the WebUI settings page, and will be used as the basis for SSO MFA challenges in a follow up PR.

The SSO MFA devices cannot be deleted or added directly.

Depends on #47451, #47426

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Sep 18, 2024
@Joerger
Copy link
Contributor Author

Joerger commented Sep 18, 2024

@codingllama You mentioned in the RFD that we should add SSO devices to various tests, do you have any in particular in mind?

@codingllama
Copy link
Contributor

@codingllama You mentioned in the RFD that we should add SSO devices to various tests, do you have any in particular in mind?

No specific tests in mind, but I think it's best to cover all the "big" tsh/Web UI interactions - add, ls and rm.

@Joerger Joerger force-pushed the joerger/auth-connector-mfa-settings branch from 3cf210a to 798c5e3 Compare September 30, 2024 17:10
Base automatically changed from joerger/auth-connector-mfa-settings to master September 30, 2024 19:51
api/types/mfa.go Outdated
Comment on lines 150 to 162
func (d *MFADevice) MFAType() string {
switch d.Device.(type) {
switch d := d.Device.(type) {
case *MFADevice_Totp:
return "TOTP"
case *MFADevice_U2F:
return "U2F"
case *MFADevice_Webauthn:
return "WebAuthn"
case *MFADevice_Sso:
return d.Sso.ConnectorType
default:
return "unknown"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosstimothy Following from our discussion earlier, should be no big deal if this doesn't make it into v17.0.0. Old client's will just get an "unknown" device. WebUI also handles it. Still would be good if I can get this open with tests and merged to v17.0.0.

@Joerger Joerger changed the base branch from master to joerger/use-second-factors October 10, 2024 22:01
@Joerger Joerger force-pushed the joerger/use-second-factors branch 2 times, most recently from 9bbd5af to 2bc96f1 Compare October 11, 2024 20:29
@@ -1388,9 +1389,71 @@ func (s *IdentityService) GetMFADevices(ctx context.Context, user string, withSe
}
devices = append(devices, &d)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tigrato I just happened upon your PR adding weakest MFA device logic. Should we incorporate SSO MFA into this weakest device logic?

In general, SSO MFA should be considered stronger than TOTP but weaker than Webauthn, but really it depends on the implementation. If the admin sets it up properly, they'll have WebAuthn or stronger/equivalent (custom hardware keys like bloomber's b-units. Worst case it uses their SSO login session to re-auth them, or prompts them to re-login via SSO.

The main difficulty is that we don't actually create these MFA devices for the user. When MFA is enabled for the user's auth connector, it makes it so GetMFADevices includes an ephemeral SSO MFA device (shown here). So whenever an auth connector is updated with mfa.enabled = true/false, we would need to loop through all users created by the connector and call upsertUserStatusMFADevice(ctx, user) to update their weakest MFA device state. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tigrato bump on this concern, will things break as is - without adding SSO MFA to the weakest device logic?

@Joerger Joerger marked this pull request as ready for review October 12, 2024 01:42
@github-actions github-actions bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 12, 2024
lib/auth/grpcserver_test.go Outdated Show resolved Hide resolved
lib/services/local/users.go Outdated Show resolved Hide resolved
Base automatically changed from joerger/use-second-factors to master October 16, 2024 19:40
api/types/mfa.go Show resolved Hide resolved
lib/services/local/users.go Show resolved Hide resolved
lib/services/readonly/readonly.go Outdated Show resolved Hide resolved
@Joerger Joerger added this pull request to the merge queue Oct 16, 2024
Merged via the queue into master with commit 5d27bf8 Oct 16, 2024
42 checks passed
@Joerger Joerger deleted the joerger/sso-mfa-device branch October 16, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants