Skip to content

fix: preserve saml idp redirect on device trust web auth#54530

Merged
flyinghermit merged 9 commits intomasterfrom
sshah/fix-saml-redirect-on-deviceauth
May 20, 2025
Merged

fix: preserve saml idp redirect on device trust web auth#54530
flyinghermit merged 9 commits intomasterfrom
sshah/fix-saml-redirect-on-deviceauth

Conversation

@flyinghermit
Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit commented May 6, 2025

Fixes #54121
Issue:

  • Internal /web path was expected as default, which didn't handle SAML IdP paths and query strings /enterprise/saml-idp/sso and /enterprise/saml-idp/login.
  • Missing URL encoding before launching parsed URL to Connect. That broke the encoded message sent by IdP.

Fix:

  • Preserve original authentication query by wrapping it under b64 encoded query, which otherwise is not presrved because the URL is parsed and decoded at multiple stages during device trust ceremony that breaks URL only encoding https://github.com/gravitational/teleport.e/pull/6464
  • Handle SAML SP initiated SSO path /enterprise/saml-idp/sso, IdP initiated SSO path /enterprise/saml-idp/login and respond with full redirect URL instead of a prefixed /web path after device authentication confirmation.
  • Responds with http.Error in case of an error.

Background on SAML IdP login redirection:
During SAML IdP authentication the user is redirected to authentication page under two scenarios:

  • If user does not have a valid session, they are redirected to root login page. With Device Trust web, the redirect link is passed to Connect too.
  • If per-session MFA is enabled, they get another round of redirection.

For each redirection, IdP preserved original request format by URL encoding them and appending them to redirect_uri query. This URL encoded query broke during device trust web authentication ceremony as the value is parsed in UI, Connect and device verification handler.

@flyinghermit
Copy link
Copy Markdown
Contributor Author

I went ahead to keep the existing implementation as it is with this patch, but the current way of passing SAML redirect url is prone to breaking in future. Upon looking at all the hops that the redirect_uri traverses through, I am strongly leaning towards keeping a server side state to hold the redirect_uri value and pass simple web safe token instead.

@flyinghermit
Copy link
Copy Markdown
Contributor Author

flyinghermit commented May 16, 2025

Manually tested following conditions:
With device trust enabled and both device verified or "open without device trust" condition

    • Regular sign in to Web UI, with and without redirection.
    • App access redirection that redirects user to login screen
    • SAML IdP:
      • Idp initiated login
      • Service provider initiated login with http redirect binding request
      • Service provider initiated login with http post binding request

@flyinghermit flyinghermit marked this pull request as ready for review May 16, 2025 18:44
@github-actions github-actions Bot requested review from kiosion and ravicious May 16, 2025 18:45
@flyinghermit flyinghermit added the no-changelog Indicates that a PR does not require a changelog entry label May 16, 2025
Comment on lines +20 to +21
const SAML_SP_INITIATED_SSO_PATH = '/enterprise/saml-idp/sso';
const SAML_IDP_INITIATED_SSO_PATH = '/enterprise/saml-idp/login';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SAML_SP_INITIATED_SSO_PATH is already defined in cfg routes, should we also define the /login in cfg api path too so we can have a single place to get this const?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Looking at these, I might just keep a single definition with /enterprise/saml-idp path. Let me keep this as is for now and I will check how the value can be shared and understood for all the use cases.

Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

my comment isnt a blocker btw, i dont care about tests spelling

Comment thread web/packages/shared/redirects/processRedirectUri.test.ts Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from smallinsky May 17, 2025 00:02
@flyinghermit flyinghermit added this pull request to the merge queue May 20, 2025
Merged via the queue into master with commit eec86b5 May 20, 2025
45 checks passed
@flyinghermit flyinghermit deleted the sshah/fix-saml-redirect-on-deviceauth branch May 20, 2025 15:10
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@flyinghermit See the table below for backport results.

Branch Result
branch/v17 Failed

flyinghermit added a commit that referenced this pull request May 22, 2025
* handle saml idp sso redirection in deviceWebConfirm

* preserve url search property for saml idp sso path

* connect: use decodeURI to launch unauthorized session url

* handle idp initiated sso url

* remove decodeURI from buildUnauthorizedSessionUrl

* fix test case name typo
github-merge-queue Bot pushed a commit that referenced this pull request May 22, 2025
…5048)

* handle saml idp sso redirection in deviceWebConfirm

* preserve url search property for saml idp sso path

* connect: use decodeURI to launch unauthorized session url

* handle idp initiated sso url

* remove decodeURI from buildUnauthorizedSessionUrl

* fix test case name typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Device trust web authentication breaks SAML IdP login redirection

3 participants