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

Add RelayState-based Authentication Request Respository #14793

Open
shardasud16 opened this issue Mar 22, 2024 · 5 comments
Open

Add RelayState-based Authentication Request Respository #14793

shardasud16 opened this issue Mar 22, 2024 · 5 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@shardasud16
Copy link

shardasud16 commented Mar 22, 2024

Describe the bug
We have a business use case where an application opens multiple tabs of our application, and they share the same session ID. When an HTTP session times out and a new request is sent, all tabs send parallel requests. Based on my understanding so far, HttpSessionSaml2AuthenticationRequestRepository saves the Saml2AuthenticationRequest in the HttpSession using a static key per session. In our case, if six requests are sent in parallel, it overwrites the request. When the SSO response is received, say for the first request, the InResponseTo validation fails as the session has a different Saml2AuthenticationRequest with a different ID. I have very limited experience with Spring Security before this, thus my understanding might be limited.

To Reproduce

Open multiple tabs of an app which has SSO SAML2 authentication (open-saml-4.2.0).
Set HTTP Session to 1 minute.
Once all tabs' sessions expire, click on "your session has expired," click all one after the other.
The same session gets shared between multiple tabs, and the request is stored in the session.
When a response comes from the first request, the session might have request 2 stored, and in this case, InResponseTo validation fails.
Expected behavior
HttpSessionSaml2AuthenticationRequestRepository should store the Saml2AuthenticationRequest within the session with a unique ID added to the key like "RelayState":

private static final String DEFAULT_SAML2_AUTHN_REQUEST_ATTR_NAME = HttpSessionSaml2AuthenticationRequestRepository.class.getName().concat(".SAML2_AUTHN_REQUEST");

My knowledge is limited here and might be missing some key understanding. If you can redirect me to the correct usage, it would be helpful, as I can't find any thread for this similar issue anywhere. We can use some existing solutions like storing requests in our Hazelcast cluster and loading it for authentication, but before this, I want to understand if there is any issue with the current implementation.implementation

Spring Version: 6.2.1
open-saml- 4.2
SAML2.0
JAVA 17
Tomcat 10.1.8

@shardasud16 shardasud16 added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Mar 22, 2024
@shardasud16 shardasud16 changed the title In InResponseTo Validation failing for multiple Authentication Request for same session. Mar 22, 2024
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 2, 2024
@jzheaux jzheaux self-assigned this Apr 2, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Apr 5, 2024

Thanks for the detailed description, @shardasud16. Your idea to look up by relay state does have some merit, though there are important security implications to consider. In addition to that, the AuthnRequest isn't the only thing stored in the session (e.g. the RequestCache), and so it's not necessarily a panacea to store the AuthnRequest outside of the session.

That said, this has been requested more than once now, and I think that it's worth reconsidering so long as these tradeoffs are outlined in the documentation. While this won't become the Spring Security default, I think it's reasonable to have it as an option.

Will you please try this out in your application and tell me if it addresses your issue? If so, we can reconsider adding it to Spring Security:

@Component
public class SpringCacheSaml2AuthenticationRequestRepository implements Saml2AuthenticationRequestRepository<AbstractSaml2AuthenticationRequest> {
	private final Cache cache = new ConcurrentMapCache("authentication-requests");

	@Override
	public AbstractSaml2AuthenticationRequest loadAuthenticationRequest(HttpServletRequest request) {
		String relayState = request.getParameter(Saml2ParameterNames.RELAY_STATE);
		return this.cache.get(relayState, AbstractSaml2AuthenticationRequest.class);
	}

	@Override
	public void saveAuthenticationRequest(AbstractSaml2AuthenticationRequest authenticationRequest, HttpServletRequest request, HttpServletResponse response) {
		String relayState = request.getParameter(Saml2ParameterNames.RELAY_STATE);
		this.cache.put(relayState, authenticationRequest);
	}

	@Override
	public AbstractSaml2AuthenticationRequest removeAuthenticationRequest(HttpServletRequest request, HttpServletResponse response) {
		String relayState = request.getParameter(Saml2ParameterNames.RELAY_STATE);
		AbstractSaml2AuthenticationRequest authenticationRequest = this.cache.get(relayState, AbstractSaml2AuthenticationRequest.class);
		if (authenticationRequest == null) {
			return null;
		}
		this.cache.evict(relayState);
		return authenticationRequest;
	}
}

@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Apr 5, 2024
@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 Apr 12, 2024
@shardasud16
Copy link
Author

Apologies for the delay. We tested the suggestions provided earlier, and they resolved the issue. Thank you.

However, we encountered another problem where we still receive random errors such as:

"Did not find SecurityContext in HttpSession ###### using the SPRING_SECURITY_CONTEXT session attribute."

From what I gathered, if one request is successfully authenticated, the context is cleared. If another request's response arrives at the same time, we encounter this error. We also observed the same behavior in version 5.6.x. Since this occurrence is rare and similar to what was experienced in 5.6.x, we are proceeding with this solution.

I have a basic question: Does Spring support multiple SAML Auth requests for the same session ID? I believe this is a fundamental use case, and thus, I suspect we might be doing something wrong. Please provide any insights or suggestions if you have them.

Scenario:
Multiple SAML2 authentication requests for the same session ID (multiple tabs on a browser).

@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 Apr 17, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Apr 17, 2024

"Did not find SecurityContext in HttpSession ###### using the SPRING_SECURITY_CONTEXT session attribute."

Such a message is not necessarily a problem. This message can come up quite often under normal pre-authenticated circumstances. Usually, it only means that this request is not yet authenticated, which is certainly the case when the authnrequest first arrives.

That said, it could indicate that one of the two scenarios I describe below is happening.

Does Spring support multiple SAML Auth requests for the same session ID?

Good question. To be clear, I think the question is more general than that: What happens if I try and log the user in on multiple tabs at the same time?

It's typically one of two things:

  1. If the first request hasn't yet written the session cookie to the browser, the second request may not be using the same session identifier. In the end, this may recreate the session multiple times with the browser only using the one that finished last.
  2. If both requests have the same session id, it may be that the first request hasn't finished writing to the session before the second request reads it. In the end, this may authenticate the same session multiple times.

These two scenarios are what happens any time two side-effect requests operate on a shared resource. The processing of each request isn't atomic and so there is bound to be interleaving. That said, it often is not an issue since the request material is the same and the underlying effect of logging in is idempotent. But, if it is an issue, you'd likely need to reconsider this part of your design to see if you can manage to perform only one login.

@jzheaux jzheaux added this to the 6.4.x milestone Apr 17, 2024
@jzheaux jzheaux added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Apr 17, 2024
@jzheaux jzheaux changed the title InResponseTo Validation failing for multiple Authentication Request for same session. Add RelayState-based Authentication Request Respository Apr 17, 2024
@shardasud16
Copy link
Author

Thank you very much and appreciate your help.

@jzheaux jzheaux modified the milestones: 6.4.x, 6.4.0-M1 May 23, 2024
@marcusdacoregio marcusdacoregio modified the milestones: 6.4.0-M1, 6.4.0-M2 Jul 15, 2024
@sjohnr sjohnr modified the milestones: 6.4.0-M2, 6.4.0-M3 Aug 19, 2024
@marcusdacoregio marcusdacoregio modified the milestones: 6.4.0-M3, 6.4.0-M4 Aug 22, 2024
@jzheaux jzheaux removed this from the 6.4.0-M4 milestone Sep 16, 2024
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: enhancement A general enhancement
Projects
Status: No status
Development

No branches or pull requests

5 participants