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

relay_state should not be included in signing calculation when it is null #13913

Closed
fr2lancer opened this issue Oct 2, 2023 · 6 comments
Closed
Assignees
Labels
in: saml2 An issue in SAML2 modules type: bug A general bug
Milestone

Comments

@fr2lancer
Copy link

fr2lancer commented Oct 2, 2023

Describe the bug

Relay Status is optional value so if it is not provided or empty value, it doesn't need to be in signing calculation

To Reproduce

# in org.springframework.security.saml2.provider.service.web.authentication.OpenSamlAuthenticationRequestResolver#resolve(HttpServletRequest(HttpServletRequest, BiConsumber)
....

Map<String, String> parameters = OpenSamlSigningUtils.sign(registration)
    .param(Saml2ParameterNames.SAML_REQUEST, deflatedAndEncoded)
    .param(Saml2ParameterNames.RELAY_STATE, relayState).parameters();

builder.sigAlg(parameters.get(Saml2ParameterNames.SIG_ALG))
    .signature(parameters.get(Saml2ParameterNames.SIGNATURE));

Expected behavior

# L177 in org.springframework.security.saml2.provider.service.web.authentication.OpenSamlAuthenticationRequestResolver
...

.param(Saml2ParameterNames.RELAY_STATE, relayState).parameters(); 

should be included optionally when it is not empty or null.

@fr2lancer fr2lancer added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Oct 2, 2023
@fr2lancer fr2lancer changed the title relay_state should be included in sigining calculation when auth request is creted in 6.1.x relay_state needs to be included as an option in signing calculation when auth request is created in 6.1.x Oct 2, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @fr2lancer, thanks for the report. Is the empty relay state property causing you problems with the signature?

@marcusdacoregio marcusdacoregio self-assigned this Oct 4, 2023
@marcusdacoregio marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 4, 2023
@fr2lancer
Copy link
Author

Hi. Thanks for the reply.

Yes it has caused auth request signing mismatch error with Azure.

@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 labels Oct 9, 2023
@marcusdacoregio
Copy link
Contributor

Is there any reason why you do not want to add the relay state parameter? I'm just trying to understand your use case.

@marcusdacoregio marcusdacoregio added in: saml2 An issue in SAML2 modules status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 9, 2023
@fr2lancer
Copy link
Author

Hi.
just existing code didn't send relaystate when you log in, while it sent it when registration..

@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 labels Oct 13, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @fr2lancer. I don't think I follow exactly what you meant in your last comment, can you elaborate more on that?
Do you mean that if RelayState is null then we should not consider it as a signature component?

@fr2lancer
Copy link
Author

Hi

the example is

AuthRequest=AAA -> (no RelayState) this is data what my app send to IDP so this string is to be calculated in sign

however in the current logic,

AuthRequest=AAA&RelayState= is used to calculated in the signing. even RelayState value is not providided.

So it causes mismatch.

@marcusdacoregio marcusdacoregio changed the title relay_state needs to be included as an option in signing calculation when auth request is created in 6.1.x relay_state should not be included in signing calculation when it is null Oct 19, 2023
@marcusdacoregio marcusdacoregio removed the status: feedback-provided Feedback has been provided label Oct 19, 2023
@marcusdacoregio marcusdacoregio added this to the 5.8.9 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: bug A general bug
Projects
Status: No status
Development

No branches or pull requests

3 participants