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

RFD 180: SSO as MFA Method #44699

Merged
merged 22 commits into from
Nov 20, 2024
Merged

RFD 180: SSO as MFA Method #44699

merged 22 commits into from
Nov 20, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Jul 26, 2024

RFD for implementing SSO as an MFA method for Per-session MFA and other MFA related features.

@Joerger Joerger force-pushed the rfd/180-sso-as-mfa-method branch from 9bd4112 to 03520c3 Compare July 26, 2024 17:53
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
@Joerger Joerger marked this pull request as ready for review August 24, 2024 01:36
@Joerger Joerger requested a review from rosstimothy August 24, 2024 01:36
@github-actions github-actions bot requested review from eriktate and hugoShaka August 24, 2024 01:36
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions github-actions bot added rfd Request for Discussion size/md labels Aug 24, 2024
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Aug 24, 2024
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
@Joerger
Copy link
Contributor Author

Joerger commented Aug 30, 2024

FYI I am in the middle of making small-large changes in every section, so hold off on reviewing for now.

@Joerger
Copy link
Contributor Author

Joerger commented Sep 6, 2024

This is ready for re-review now. I left a few TODOs for details I have not yet POC'd.

rfd/0105-headless-authentication.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
rfd/0180-sso-as-mfa-method.md Outdated Show resolved Hide resolved
@Joerger
Copy link
Contributor Author

Joerger commented Sep 30, 2024

@klizhentas @xinding33 friendly ping for review

Copy link

@francesco-doyensec francesco-doyensec left a comment

Choose a reason for hiding this comment

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

hello team! 👋 Here my two bits about the RFD. Overall a great work and feature.
The main missing aspects are:

  • Missing dedicated section to the security characteristics of the flow (anti-replay, anti-phishing, IdP configuration assumptions)
  • References to the IdP behaviour and documentation to further describe the tech supporting the flow dip-side

rfd/0180-sso-mfa.md Show resolved Hide resolved
rfd/0180-sso-mfa.md Show resolved Hide resolved
rfd/0180-sso-mfa.md Show resolved Hide resolved
rfd/0180-sso-mfa.md Show resolved Hide resolved
end
Client ->> Node: connect w/ MFA verified certs
```

Choose a reason for hiding this comment

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

Not clear how the successful IdP Auth response should go directly to Teleport. Same for the callback at step 9.

Additionally:

  • since the SSO MFA flow and the per-session MFA flow are separated, it is not clear how the final step is protected against phishing. Probably it is in the plan by using the request ID, but I would state it as requirement in the RFD
  • How will replay attempts be treated? As per-session, the token should not be usable two times in a row, same for the certs. What is the plan in that sense (I would add it to the RFD)

Copy link
Contributor Author

@Joerger Joerger Oct 23, 2024

Choose a reason for hiding this comment

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

since the SSO MFA flow and the per-session MFA flow are separated, it is not clear how the final step is protected against phishing. Probably it is in the plan by using the request ID, but I would state it as requirement in the RFD

For tsh and Teleport Connect, the client passes a secret key in the sso mfa challenge request, and the server encodes the MFA token with that key before sending it back. The request id is also provided by the server and should only be known by the requesting client, the mfa token alone is useless without knowledge of the request id.

For the Web, we don't encode the mfa token with a secret key, we just depend on the request id being unknown by anyone but the client.

Also note that the mfa token is only valid for the user who requested it. So the requesting user with active key+certs can cash in the mfa token w/ request ID to get their MFA verified certs.

Does this all sound good? This was my main concern that I wanted a second opinion on, so thanks for spotting it out. I'll update the document with agreed upon details.

How will replay attempts be treated? As per-session, the token should not be usable two times in a row, same for the certs. What is the plan in that sense (I would add it to the RFD)

For the most part, the MFA token will be discarded as soon as it's verified for mfa-verified certs, etc. However, we also track the MFA scope for SSO MFA requests and allow reuse in certain contexts. e.g. MFA for back-to-back admin actions: create user -> create reset password token for registration.

Let me know if you are not convinced that SSO MFA is phishing resistant enough to allow reuse like we do with WebAuthn, with the details above in mind.

Choose a reason for hiding this comment

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

Hi @Joerger! The described approach looks legitimate to me, I agree with the request ID usage and anti-replay strategy

@Joerger Joerger requested a review from rosstimothy October 29, 2024 02:05
@Joerger
Copy link
Contributor Author

Joerger commented Oct 30, 2024

@francesco-doyensec Please take another look when you get a chance. I addressed your concerns in the recent commits. In summary:

Teleport doesn't make any assumptions on the exact way MFA is implemented IdP-side. This is left as an exercise for the IdP administrators since it depends on the IdP and desired outcome. The security of this feature depends on proper configuration from the cluster administrators, and as such there will be warnings in the documentation.

We do add a small guardrail to improper configurations; forcing re-authentication by default so that even if MFA is not configured IdP-side, users will be forced to re-login via SSO for each MFA check. IIUC, even this is IdP dependent and some providers might not respect the re-authentication requirement.

Whether or not this is phishing-resistant depends on the IdP-side configuration.

Replay attacks are prevented by immediately discarding validated SSO MFA tokens (unless reuse is explicitly allowed).

Let me know if you have any more concerns as this has been implemented and is planned to be included in v17.0.0, which will be released in the coming weeks. The full implementation (tsh and tctl only) is now on master if you'd like to take a look at the code or test it.

@anaximand3r
Copy link

@Joerger, just finished reviewing the updated version. It covers my initial concerns, I agree about the extra guardrail requiring SSO MFA at each interaction. Well structured and described!

@Joerger Joerger added this pull request to the merge queue Nov 20, 2024
Merged via the queue into master with commit 7fe216b Nov 20, 2024
39 checks passed
@Joerger Joerger deleted the rfd/180-sso-as-mfa-method branch November 20, 2024 19:05
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 rfd Request for Discussion size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants