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

Saml2AuthenticationRequestRepository does not work in combination with Spring Session #10828

Closed
tompson opened this issue Feb 11, 2022 · 5 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug

Comments

@tompson
Copy link

tompson commented Feb 11, 2022

Summary

The HttpSessionSaml2AuthenticationRequestRepository saves the Saml2AuthenticationRequest in the session and tries to load it after the IdP authenticated the user.

This does not work when using Spring Session because the session cookie is not sent to the server after the IdP authenticated the user.

Spring Session creates a session cookie with SameSite=Lax which causes the browser not to send the cookie when sending the POST request after the IdP authentication.

To Reproduce

Create a minimal Spring Boot application with Spring Security SAML and Spring Session active.

Expected behaviour

After login at the IdP and being redirected to the application the user should be signed in and seeing the secured URL.

Actual behaviour

A new session is created and the user is at the login page again. When he tries to load the secured URL he is able to request it.

@tompson tompson added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Feb 11, 2022
@eleftherias eleftherias added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 14, 2022
@eleftherias
Copy link
Contributor

Thanks for reaching out @tompson.

Note that we have a workaround shared in spring-projects/spring-session#1577.

We will need to think about what changes in Spring Security or Spring Session could make this scenario work, since there is no clear solution at the moment.
Feel free to share any suggestions.

@jzheaux
Copy link
Contributor

jzheaux commented Feb 15, 2022

@tompson, can you clarify how this is an issue with Saml2AuthenticationRequestRepository? It sounds like it's an issue with the RequestCache.

Saml2AuthenticationRequestRepository stores the AuthnRequest for the purpose of validating the InResponseTo value in the SAML 2.0 Response, but such validation has not yet been added.

@tompson
Copy link
Author

tompson commented Feb 16, 2022

@eleftherias thanks for that hint, I experimented with different settings for SameSite myself but it is not an option for us to reduce protection against CSRF

@jzheaux I stumbled across this when trying to solve #10550 and I realised later that it is just an optional validation of InResponseTo - so it is no blocker for us

@jzheaux
Copy link
Contributor

jzheaux commented Feb 16, 2022

Thanks, @tompson. I wonder if you could create an implementation of RequestCache that keys the saved requests by the AuthnRequest#ID instead of by the session id.

Since there is no problem with Saml2AuthenticationRequestRepository, I'm going to decline this ticket. Feel free to continue the conversation, though, as needed.

@onmishkin
Copy link

Thanks, @tompson. I wonder if you could create an implementation of RequestCache that keys the saved requests by the AuthnRequest#ID instead of by the session id.

@jzheaux: Can you connect the dots for me a bit on this? Is the premise that one would write one's own implementation of Saml2AuthenticationRequestRepository that in turn used such an implementation of RequestCache? If so, I'm not seeing how that Saml2AuthenticationRequestRepository implementation's loadAuthenticationRequest(HttpServletRequest) implementation would work without duplicating a bunch of code that's in Saml2AuthenticationTokenConverter to parse the SAML response and then pull out its AuthnRequest#ID.

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 status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants