Added support for the CAS gateway feature #14193

leleuj · 2023-11-24T15:44:28Z

This PR aims to support the CAS gateway feature in the spring-security-cas library.

It follows: #40 and especially: #9881 as discussed with @marcusdacoregio.

Unit tests pass and I tested it manually.

marcusdacoregio · 2023-11-27T11:07:09Z

Thank you very much @leleuj. I'll try to review this PR for 6.3.

leleuj · 2023-12-05T10:04:42Z

Thanks. Can I do something to help you in the review process?

marcusdacoregio

As far as I understood from #9881 and #40 the idea behind this feature is to add a filter that would trigger the redirect to the CAS service with the gateway=true parameter.

Based on previous reviews from @rwinch, the design was pretty much aligned and there were some details left like having a mechanism to avoid hitting the CAS server for every request.

I might be missing something but I do not see how the changes and the new components proposed in this PR work together to achieve the desired behavior. If you could outline the idea or maybe put together a sample (you can use this) it would be great to help me understand the proposed design.

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationEntryPoint.java

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java

leleuj · 2023-12-07T07:33:43Z

"the idea behind this feature is to add a filter that would trigger the redirect to the CAS service with the gateway=true parameter":

Not exactly, the idea behind this feature is not to add a new filter, this is just a possible implementation.

The idea behind this feature is to be able to call the CAS server with gateway=true, meaning that if the user is not auhtenticated, it comes back to the client app being anonymous and if the user is authenticated, it comes back to the client app being authenticated.

leleuj · 2023-12-07T11:36:48Z

I had an issue with my test demo. I will test again and keep you posted.

leleuj · 2023-12-07T11:37:28Z

Forget my previous comments for now.

leleuj · 2023-12-07T13:08:51Z

You were right: I don't understand how the provider was called in the gateway mode.

Anyway, I restarted my work from the provided demo: https://github.com/spring-projects/spring-security-samples/tree/main/servlet/spring-boot/java/cas/login

leleuj · 2023-12-07T13:41:10Z

OK. I just updated the PR.

I used the provided demo: https://github.com/spring-projects/spring-security-samples/tree/main/servlet/spring-boot/java/cas/login

I added: filter.setServiceProperties(serviceProperties()); in the casAuthenticationFilter method. I think this is missing by default.

And to test the gateway support, I added: serviceProperties.setGateway(true); in the serviceProperties() method.

The changes are now even easier. They mainly impact the CasAuthenticationFilter.

leleuj · 2023-12-07T13:42:22Z

I still keep the idea that no additional filter is required, the CasAuthenticationFilter can be used in gateway mode thanks to the serviceProperties.

Multiple filters can be defined on different URLs if need be.

And I still prefer to have the gateway and the renew settings in the same place (in the ServiceProperties).

leleuj · 2023-12-07T13:44:28Z

The CasAuthenticationFilter is aware of the gateway configuration.

This is used to know if a specific token must be created for the "gateway mode" or if the regular CAS login must be performed:

if (serviceTicket == null) {
	if (this.gateway) {
		this.logger.debug("Gateway request with no CAS ticket");
		return new CasGatewayNoServiceTicketAuthenticationToken();
	}
	else {
		this.logger.debug("Failed to obtain an artifact (cas ticket)");
		serviceTicket = "";
	}
}

This is the final authentication available: CasGatewayNoServiceTicketAuthenticationToken(authenticated=true, roles=ROLE_ANONYMOUS) in gateway (no authentication at the CAS server level) mode.

leleuj · 2023-12-07T13:46:52Z

For each request, the CasAuthenticationFilter and especially its requiresAuthentication method is called. Now, it is checked if the gateway mode is enabled and if the token is an "expired" gateway one:

final Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if (this.gateway && auth instanceof CasGatewayNoServiceTicketAuthenticationToken token) {
	final Instant creationDate = token.getCreationDate();
	final Instant startRetainDate = Instant.now().minusSeconds(this.gatewayAuthenticationRetainDelayInSeconds);
	// removes authentication after a certain retain delay
	if (creationDate.isBefore(startRetainDate)) {
		this.logger.debug("Removes authentication because of gateway setting");
		SecurityContextHolder.getContext().setAuthentication(null);
	}
}

If so, it is removed from the context and a new authentication is triggered, meaning a new round-trip to the CAS server is done (once again using the gateway support).

leleuj · 2023-12-07T13:49:40Z

Sorry for the false start. Are things clearer now?

marcusdacoregio

Thanks, @leleuj, I left some feedback inline.

I still think that we should have a new filter with the responsibility of redirecting to the CAS server with gateway=true if needed.

I was taking a look at #9881 and it looks like there is support to prevent making too many requests via CasCookieGatewayRequestMatcher

It is fine to have the gateway property inside ServiceProperties, that way the CasAuthenticationFilter can proper handle an unsuccessfulAuthentication if there is no service ticket.

marcusdacoregio · 2023-12-08T13:55:11Z

...pringframework/security/cas/authentication/CasGatewayNoServiceTicketAuthenticationToken.java

+ * @author Jerome LELEU
+ * @since 6.3.0
+ */
+public final class CasGatewayNoServiceTicketAuthenticationToken extends AbstractAuthenticationToken {


I think this would be better by extending AnonymousAuthenticationToken. My concern is that there might be some places where the code checks if token instanceof AnonymousAuthenticationToken. This might not even be needed depending on the strategy that we adopt.

OK. I get the point, but indeed, this may not be needed depending on the chosen strategy.

marcusdacoregio · 2023-12-08T14:24:32Z

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java

-			serviceTicket = "";
+			if (this.gateway) {
+				this.logger.debug("Gateway request with no CAS ticket");
+				return new CasGatewayNoServiceTicketAuthenticationToken();


What happens on a public URL when there is no AuthenticationException? Will the CasAuthenticationEntryPoint be invoked?
To be able to invoke this filter to different URLs, the filterProcessingUrl should allow customization, however, I'm still not convinced that it is the responsibility of this filter to be aware of the gateway request.

If we take a look at how OAuth2 components work, there are at least two filters, one that performs the redirect to the Authorization Server and another that processes the authorization response.

The CasAuthenticationEntryPoint will only be invoked if the URL is configured with authenticated.

And the CasGatewayNoServiceTicketAuthenticationToken cannot be an AnonymousAuthenticationToken as it leads to an infinite loop.

On the other hand, this prevents triggering a new CAS round-trip and only leads to a 403 error page for another URL protected with non-gateway CAS login. It would certainly need a custom AccessDeniedHandler.

So although re-using the CasAuthenticationFilter for the gateway setting seems an alluring idea, I think I was wrong on this: I will create a new filter. I will do that next Monday.

I think it's better to close this PR and submit a new PR, isn't it?

You can use the same PR if you want, just force-push the new commits. You can also cherry-pick the commit from #9881 and polish it to achieve the desired result.

OK. I will do that today. Thanks for your time and advices

@marcusdacoregio I just submitted a polished-to-main-branch PR from #9881.

Just let me know if you have additional comments.

marcusdacoregio

Thanks, @leleuj. It is looking nice.

I've left some feedback inline.

marcusdacoregio · 2023-12-13T16:14:58Z

cas/src/main/java/org/springframework/security/cas/web/TriggerCasGatewayFilter.java

+		HttpServletResponse response = (HttpServletResponse) res;
+
+		if (this.requestMatcher.matches(request)) {
+			throw new TriggerCasGatewayException("Try a CAS gateway authentication");


Based on @rwinch comment here, I also think the exception should not be thrown here, instead, if the request matches, we should redirect to the CAS server with gateway=true.

I understand. I think the solution here is to use the CasAuthenticationEntryPoint to perform the gateway redirection.

marcusdacoregio · 2023-12-13T16:15:56Z

cas/src/main/java/org/springframework/security/cas/web/TriggerCasGatewayFilter.java

+ */
+public class TriggerCasGatewayFilter extends GenericFilterBean {
+
+	private RequestMatcher requestMatcher;


The default could be the CasCookieGatewayRequestMatcher and it can be customized via setRequestMatcher.

The constructor can be removed.

I see your point. I can set the CasCookieGatewayRequestMatcher by default.

marcusdacoregio · 2023-12-13T16:17:40Z

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationEntryPoint.java

There is probably no need to change this class since we will not rely on it to redirect to the CAS server, instead the TriggerCasGatewayFilter will do that for us.

There is some logic in the CasAuthenticationEntryPoint we may not want to duplicate in the TriggerCasGatewayFilter (like the encodeServiceUrlWithSessionId property).

Are you sure you don't want to use the CasAuthenticationEntryPoint in the `TriggerCasGatewayFilter?

My concern with throwing the exception in TriggerCasGatewayFilter is that it is not a failed authentication attempt, therefore I would refrain from using the CasAuthenticationEntryPoint. One thing that we can do is to create a package-private CasUrlUtils, something like:

final class CasUrlUtils { static String createServiceUrl(HttpServletResponse response, ServiceProperties serviceProperties, boolean encodeServiceUrlWithSessionId) { return WebUtils.constructServiceUrl(null, response, serviceProperties.getService(), null, serviceProperties.getArtifactParameter(), encodeServiceUrlWithSessionId); } static String createRedirectUrl(String loginUrl, String serviceUrl, ServiceProperties serviceProperties, boolean isGateway) { return CommonUtils.constructRedirectUrl(loginUrl, serviceProperties.getServiceParameter(), serviceUrl, serviceProperties.isSendRenew(), isGateway); } }

Then we can use this util class in both CasAuthenticationFilter and TriggerCasGatewayFilter. Is the encodeServiceUrlWithSessionId still needed in recent versions of CAS? If not, TriggerCasGatewayFilter might not even care about it.

Yes, we can forget about the encodeServiceUrlWithSessionId which is related to CAS 2 (15 years old). I won't use the CasAuthenticationEntryPoint and ignore this setting.
It should even be removed (at least deprecated) in the CasAuthenticationEntryPoint.

marcusdacoregio · 2023-12-13T16:33:14Z

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java

+		if (StringUtils.hasText(obtainArtifact(request))) {
+			super.unsuccessfulAuthentication(request, response, failed);
+		}
+		else {
+			SavedRequest savedRequest = this.requestCache.getRequest(request, response);
+			if (savedRequest != null) {
+				this.redirectStrategy.sendRedirect(request, response, savedRequest.getRedirectUrl());
+			}
+		}


I'm finding this strategy to decide if that was a gateway request too sensitive.

Is it possible to make CAS redirect to the service URL with a query param? Something like my.company.com/cas/login?gateway=true&service=https://server3.company.com/webapp/login/cas%3Fgateway=true. With that, upon successful/failed authentication, the CAS server would redirect to https://server3.company.com/webapp/login/cas?gateway=true and we can be sure whether that was a gateway request or not.

You're right, this strategy is a bit too sensitive (checking if we already have a saved request).

Though, using a parameter in the serviceUrl will trigger a lot of complexity. Indeed, the service tickets created by the CAS server are linked to the original service URL.
It means that we can't reuse the existing ticketValidator to validate service tickets created on a gateway request as the service URL won't match, it has gateway=true for example while we try to validate the service ticket for the service without gateway=true.

I can give it a try if you want. I just want you to confirm.

Let's discard the parameter for now. What about setting some attribute in the session (if the session exists) and checking that attribute later on

By the way, sorry for the back and forth on this, I haven't had time to test how this works on my own yet.

OK. I will try to use the session for that.

No problem in that case, the OAuth2AuthorizationRequestRedirectFilter also saves the request before the redirect.

@marcusdacoregio I have updated the PR to take into account all your feedbacks:

the exception has been removed

the TriggerCasGatewayFilter performs the redirection to the CAS server with gateway=true

the CasAuthenticationEntryPoint is no longer changed

the gateway nature of the CAS round-trip is saved into the web session.

BTW, I have added a TriggerCasGatewayFilterTests to test the TriggerCasGatewayFilter and cropped all commits into one.

We should be good for merging. Just let me know.

@marcusdacoregio Did you get some time to check this PR?

I'll take a look at it today

Issue spring-projectsgh-14193

marcusdacoregio · 2023-12-20T13:57:58Z

Hello, @leleuj. I rebased your branch and applied a polish commit on top of yours. I'll wait for the CI to pass to merge it. As soon as the SNAPSHOTs are available I'll update the sample to include the gateway feature.

marcusdacoregio · 2023-12-20T14:07:09Z

Thanks @leleuj, this is now merged into main

leleuj · 2023-12-20T14:30:57Z

Excellent! Thank your very much! As it's a not a breaking change, can I open a new PR for the 5.8.x stream?

marcusdacoregio · 2023-12-20T14:32:29Z

Sorry, but that is not possible. We only add enhancements to minor versions.

leleuj · 2023-12-20T14:37:05Z

I'm not sure I understand your answer: "We only add enhancements to minor versions."

Would it be possible in a new version 5.9.x or only in version 6.3?

marcusdacoregio · 2023-12-20T14:39:28Z

Sorry if I was not clear. We follow the semantic versioning (major.minor.patch), and our policy is to add new features only when we increment the minor version. Since the next minor release is 6.3 (we are currently in 6.2), that release is the target for this PR.

The 5.x line ended with 5.8.

leleuj · 2023-12-20T14:44:42Z

OK. That makes sense. Thanks for your time and help.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 24, 2023

marcusdacoregio self-assigned this Nov 27, 2023

marcusdacoregio added in: cas An issue in spring-security-cas type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 27, 2023

marcusdacoregio suggested changes Dec 6, 2023

View reviewed changes

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationEntryPoint.java Outdated Show resolved Hide resolved

cas/src/main/java/org/springframework/security/cas/web/CasAuthenticationFilter.java Outdated Show resolved Hide resolved

marcusdacoregio suggested changes Dec 8, 2023

View reviewed changes

leleuj closed this Dec 11, 2023

leleuj force-pushed the casgateway branch from 2b53e73 to 4a50d5a Compare December 11, 2023 07:24

leleuj reopened this Dec 11, 2023

marcusdacoregio suggested changes Dec 13, 2023

View reviewed changes

leleuj force-pushed the casgateway branch from a97c311 to 6d2fc6b Compare December 15, 2023 06:51

leleuj and others added 2 commits December 20, 2023 10:56

Added support for the CAS gateway feature

40fbfe9

Polish spring-projectsgh-14193

60b3466

Issue spring-projectsgh-14193

marcusdacoregio force-pushed the casgateway branch from a4ccfd4 to 60b3466 Compare December 20, 2023 13:56

marcusdacoregio added this to the 6.3.0-M1 milestone Dec 20, 2023

marcusdacoregio approved these changes Dec 20, 2023

View reviewed changes

marcusdacoregio merged commit 69808bf into spring-projects:main Dec 20, 2023
2 checks passed

leleuj deleted the casgateway branch December 20, 2023 14:28

marcusdacoregio mentioned this pull request Dec 20, 2023

Add CAS Gateway sample spring-projects/spring-security-samples#174

Closed

Added support for the CAS gateway feature #14193

Added support for the CAS gateway feature #14193

Conversation

leleuj commented Nov 24, 2023 • edited Loading

marcusdacoregio commented Nov 27, 2023 • edited Loading

leleuj commented Dec 5, 2023

marcusdacoregio left a comment

Choose a reason for hiding this comment

leleuj commented Dec 7, 2023

leleuj commented Dec 7, 2023

leleuj commented Dec 7, 2023

leleuj commented Dec 7, 2023

leleuj commented Dec 7, 2023 • edited Loading

leleuj commented Dec 7, 2023 • edited Loading

leleuj commented Dec 7, 2023 • edited Loading

leleuj commented Dec 7, 2023

leleuj commented Dec 7, 2023

marcusdacoregio left a comment

Choose a reason for hiding this comment

marcusdacoregio Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusdacoregio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusdacoregio commented Dec 20, 2023 • edited Loading

marcusdacoregio commented Dec 20, 2023

leleuj commented Dec 20, 2023

marcusdacoregio commented Dec 20, 2023

leleuj commented Dec 20, 2023

marcusdacoregio commented Dec 20, 2023

leleuj commented Dec 20, 2023

leleuj commented Nov 24, 2023 •

edited

Loading

marcusdacoregio commented Nov 27, 2023 •

edited

Loading

leleuj commented Dec 7, 2023 •

edited

Loading

leleuj commented Dec 7, 2023 •

edited

Loading

leleuj commented Dec 7, 2023 •

edited

Loading

marcusdacoregio Dec 8, 2023 •

edited

Loading

marcusdacoregio commented Dec 20, 2023 •

edited

Loading