Skip to content

Document that Http-Redirect binding not supported for SAML 2.0 responses #11161

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.

Sign up for GitHub

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

Jump to bottom
Open
shawnweeks opened this issue Apr 25, 2022 · 5 comments · May be fixed by #17026
Open

Document that Http-Redirect binding not supported for SAML 2.0 responses #11161

shawnweeks opened this issue Apr 25, 2022 · 5 comments · May be fixed by #17026
Assignees
@jzheaux
Labels
in: docs An issue in Documentation or samples status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement

Comments

@shawnweeks
Copy link

Describe the bug
When setting RelyingPartyRegistrations assertionConsumerServiceBinding to Saml2MessageBinding.REDIRECT we do not consume the "SigAlg" and "Signature" query parameters in the response to validate the SAMLResponse leading to the following error message.

Either the response or one of the assertions is unsigned. Please either sign the response or all of the assertions.

We do set the parameters on the outgoing redirect requests to the IDP as resolved in #7711 so this is basically the flip side of that issue.

To Reproduce
Configure a RelyingPartyRegistrationRepository with assertionConsumerServiceBinding set Saml2MessageBinding.REDIRECT and your IDP set to sign responses but not assertions. In Keycloak I just have the "Sign Documents" option checked but not the "Sign Assertions". If you change the binding to POST everything should work normally with just the document but not assertions signed but on REDIRECT it will fail because the Query Parameter isn't considered.

Expected behavior
Both REDIRECT and POST SAML Response Bindings should work with just the response signed.

Sample

A link to a GitHub repository with a minimal, reproducible sample.

Reports that include a sample will take priority over reports that do not.
At times, we may require a sample, so it is good to try and include a sample up front.
@shawnweeks shawnweeks added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 25, 2022
@sjohnr sjohnr added the in: saml2 An issue in SAML2 modules label Apr 25, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 6, 2022

Hi, @shawnweeks, thanks for the report.

Setting the ACS binding to REDIRECT is not supported, given that the SAML Specification (line 420) states that it must not be used (emphasis mine):

  1. Identity Provider issues to Service Provider
    In step 5, the identity provider issues a message to be delivered by the user agent
    to the service provider. Either the HTTP POST, or HTTP Artifact binding can be used to transfer
    the message to the service provider through the user agent. The message may indicate an error,
    or will include (at least) an authentication assertion. The HTTP Redirect binding MUST NOT be
    used    , as the response will typically exceed the URL length permitted by most user agents.

As this question has come up before, I think it would be good to add this detail to the JavaDoc. @shawnweeks, are you able to add a link to this detail in the spec to the RelyingPartyRegistration.Builder#assertionConsumerServiceBInding method?

@jzheaux jzheaux assigned jzheaux and unassigned marcusdacoregio May 6, 2022
@jzheaux jzheaux added in: docs An issue in Documentation or samples type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug in: saml2 An issue in SAML2 modules labels May 6, 2022
@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label May 25, 2022
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jun 1, 2022
@shawnweeks
Copy link
Author

It would be a while before I have time to submit a PR for this. It should be noted though Keycloak and presumably other SAML IDPs do actually support this binding and despite the spec saying not to do it.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Jun 1, 2022
@jzheaux jzheaux changed the title Saml2 Redirect Response Signature Is Not Used from Query Parameter Document that Http-Redirect binding not supported for SAML 2.0 responses Jun 21, 2022
@jzheaux jzheaux added status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: feedback-provided Feedback has been provided labels Jun 21, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Jun 21, 2022

Thanks for the feedback, @shawnweeks. For now, I think we should focus on the spec. If applications need to depart from the spec, they can construct a filter.

@AkashB23
Copy link

@jzheaux I got that as per spec HTTP-REDIRECT is not recommended for SAMLResponse, then why do we need the
https://github.com/spring-projects/spring-security/blob/8f104b60fc0e7c3f7434da83f14016f9b1fbffd7/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java#L108

where if we get the request with GET, we try to inflate it, why do we need , any specific reason it is present. (as per my understanding deflated responses will only be part of REDIRECT call

This was referenced May 1, 2025
Closed
Open
snowykte0426 added a commit to snowykte0426/spring-security that referenced this issue May 2, 2025
@snowykte0426
docs(saml2): migrate SAML 2.0 Migration Guide from Wiki
7465866 
Fixes: spring-projectsgh-11161
Signed-off-by: snowykte0426 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jzheaux jzheaux

Labels
in: docs An issue in Documentation or samples status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
6 participants
@jzheaux @sjohnr @shawnweeks @marcusdacoregio @spring-projects-issues @AkashB23