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 Joerger force-pushed the joerger/sso-mfa-device branch from e112a1f to bc1961a Compare September 18, 2024 00:47
@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?

@Joerger Joerger mentioned this pull request Sep 18, 2024
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from bc1961a to f3e2ed9 Compare September 18, 2024 20:13
@Joerger Joerger force-pushed the joerger/auth-connector-mfa-settings branch from f4f51dc to 3cf210a Compare September 21, 2024 00:24
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from f3e2ed9 to 2e155d3 Compare September 21, 2024 00:27
@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
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from 2e155d3 to 019b2e6 Compare September 30, 2024 17:32
Base automatically changed from joerger/auth-connector-mfa-settings to master September 30, 2024 19:51
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from 019b2e6 to 628353b Compare October 5, 2024 01:50
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 force-pushed the joerger/sso-mfa-device branch from 628353b to a3442aa Compare October 9, 2024 19:21
@Joerger Joerger changed the base branch from master to joerger/use-second-factors October 10, 2024 22:01
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from a3442aa to c623e18 Compare October 10, 2024 22:02
@Joerger Joerger force-pushed the joerger/use-second-factors branch from ed14fdc to d00fd0e Compare October 10, 2024 22:09
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from c623e18 to cb0491c Compare October 10, 2024 22:09
@Joerger Joerger force-pushed the joerger/use-second-factors branch 2 times, most recently from 9bbd5af to 2bc96f1 Compare October 11, 2024 20:29
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from cb0491c to 8963f30 Compare October 11, 2024 22:54
@Joerger Joerger force-pushed the joerger/use-second-factors branch from 2bc96f1 to d4536b2 Compare October 11, 2024 23:36
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from 8963f30 to 14a5fab Compare October 11, 2024 23:49
@@ -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 force-pushed the joerger/use-second-factors branch from d4536b2 to 8ce6e31 Compare October 12, 2024 01:41
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from 14a5fab to 2e6c6a3 Compare October 12, 2024 01:42
@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
@Joerger Joerger requested a review from codingllama October 16, 2024 17:55
@Joerger Joerger force-pushed the joerger/sso-mfa-device branch from eb26c43 to f066a2d Compare October 16, 2024 18:50
lib/auth/grpcserver_test.go Outdated Show resolved Hide resolved
lib/services/local/users.go Outdated Show resolved Hide resolved
@Joerger Joerger requested a review from rosstimothy October 16, 2024 19:38
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 force-pushed the joerger/sso-mfa-device branch from f075f9d to 3b60521 Compare October 16, 2024 22:06
@Joerger Joerger enabled auto-merge October 16, 2024 22:18
@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