-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
e112a1f
to
bc1961a
Compare
@codingllama You mentioned in the RFD that we should add SSO devices to various tests, do you have any in particular in mind? |
bc1961a
to
f3e2ed9
Compare
f4f51dc
to
3cf210a
Compare
f3e2ed9
to
2e155d3
Compare
No specific tests in mind, but I think it's best to cover all the "big" tsh/Web UI interactions - add, ls and rm. |
3cf210a
to
798c5e3
Compare
2e155d3
to
019b2e6
Compare
019b2e6
to
628353b
Compare
api/types/mfa.go
Outdated
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" | ||
} |
There was a problem hiding this comment.
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.
628353b
to
a3442aa
Compare
a3442aa
to
c623e18
Compare
ed14fdc
to
d00fd0e
Compare
c623e18
to
cb0491c
Compare
9bbd5af
to
2bc96f1
Compare
cb0491c
to
8963f30
Compare
2bc96f1
to
d4536b2
Compare
8963f30
to
14a5fab
Compare
lib/services/local/users.go
Outdated
@@ -1388,9 +1389,71 @@ func (s *IdentityService) GetMFADevices(ctx context.Context, user string, withSe | |||
} | |||
devices = append(devices, &d) | |||
} | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
d4536b2
to
8ce6e31
Compare
14a5fab
to
2e6c6a3
Compare
eb26c43
to
f066a2d
Compare
c727a6e
to
f075f9d
Compare
…ces from being stored in the backend.
f075f9d
to
3b60521
Compare
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