Skip to content

support saml http-post binding request#54605

Merged
flyinghermit merged 19 commits intomasterfrom
sshah/connector-sp-postform
May 22, 2025
Merged

support saml http-post binding request#54605
flyinghermit merged 19 commits intomasterfrom
sshah/connector-sp-postform

Conversation

@flyinghermit
Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit commented May 7, 2025

SAML login from Web UI and console login methods have been updated to support SAML http-post binding request.
Supporting http-post binding request means requesting SAML authentication with HTML form that contains the SAML authentication request. The default we supported was http-redirect binding that sends SAML authentication request based on redirect URL. The implementation to support post binding remains largely same as the redirect binding, except for changes needed to support responding with HTML form.

Changes:

  • An HTML template is aded that generates full response HTML that contains the SAML auth request form.
  • SAMLConnectorSpecV2 proto.
    • PreferredRequestBinding: The field indicates preferred SAML request binding method.
  • SAMLAuthRequest proto:
    • PostForm: SAML authentication request form if the PreferredRequestBinding is http-post.
    • ClientVersion: facilitates auth to detect if client supports http-post binding.
  • Implements new SAMLCeremony that handles SAML auth request from clients such as tsh.

The SAML authentication request handling changes are in the e PR https://github.com/gravitational/teleport.e/pull/6493

Compatibility:
This PR only supports http-post binding for Web UI SSO and tsh and Connect SSO.
SSO MFA, Single Log Out, and tctl sso test commands are not covered and can be addressed as a followup.

Related to #54826

Testing Guide
  • In an existing SAML auth connector, update the connector spec with preferred_request_binding: http-post value.
  • Once the connector is updated, new sso request to this connector will be sent via HTML form, instead of HTTP meta redirection.
  • Test this against Okta or even Teleport SAML IdP, which supports post binding request.
  • If you are testing with Connect, it needs to be updated with latest tsh built from this branch.

changelog: Web UI, tsh and Connect SSO login now supports http-post binding request in Teleport SAML service provider.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 9, 2025

Amplify deployment status

Branch Commit Job ID Status Preview Updated (UTC)
sshah/connector-sp-postform 02a7c8c 13 ✅SUCCEED sshah-connector-sp-postform 2025-05-22 15:58:56

Comment thread lib/client/sso/redirector.go Outdated
@flyinghermit
Copy link
Copy Markdown
Contributor Author

Most of the change diff is due to boilerplate changes due to proto field update. Let me know if other changes should be extracted to a different PR.

Comment thread api/proto/teleport/legacy/types/types.proto
@ravicious ravicious self-requested a review May 16, 2025 16:30
- add comments to test, improve test case
- add comment to SSOLoginConsoleResponse struct
- return http error instead of redirection for error
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I tested this through Teleport as IdP, the connector seems to work both with preferred_request_binding: http-post and without it and I see difference in the requests sent in the network tab.

@anaximand3r
Copy link
Copy Markdown

Hello team 👋
Just finished reviewing the changes and performing some base testing. It looks legitimate to me, the templating applied with html/template is escaping malicious action reflections in the resulting POST Binding form, substituting the test payloads with #ZgotmplZ (see docs).

@flyinghermit
Copy link
Copy Markdown
Contributor Author

Thanks for the review @anaximand3r.
I am curious about the malicious action test payload entrypoint, is it from the auth connector configuration or there is a way to inject that between the SSO flow?

@anaximand3r
Copy link
Copy Markdown

flyinghermit

Hi! I was using the auth connector entity_descriptor, specifically the binding location in Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST". It is the only user-controlled part of the new http-post binding form, since it serves as form destination. An example vulnerable case is CVE-2023-45683

@anaximand3r
Copy link
Copy Markdown

Hello team 👋 Just finished reviewing the changes and performing some base testing. It looks legitimate to me, the templating applied with html/template is escaping malicious action reflections in the resulting POST Binding form, substituting the test payloads with #ZgotmplZ (see docs).

+1: As extra improvement, I would add the same logic you are already applying in the post binding when teleport acts as IdP. Function validateAssertionConsumerServicesEndpoint (see definition here)

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from kimlisa May 22, 2025 14:20
@flyinghermit
Copy link
Copy Markdown
Contributor Author

I was just adding that after I read your comment :) Done in 7459806

Generally the html/template should put us in a safer side and the ability to modify auth connector would be scary enough by itself. Though this additional url validation should cancel out that probability too.

@flyinghermit flyinghermit added this pull request to the merge queue May 22, 2025
Merged via the queue into master with commit 15e5934 May 22, 2025
44 checks passed
@flyinghermit flyinghermit deleted the sshah/connector-sp-postform branch May 22, 2025 16:47
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@flyinghermit See the table below for backport results.

Branch Result
branch/v17 Failed

github-merge-queue Bot pushed a commit that referenced this pull request May 23, 2025
* support saml http-post binding request (#54605)

* ref log in place of slog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants