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

In 3.0.x and later, Spring Security cannot be used to secure a WebSocket upgrade request when using Jetty #37115

Closed
rcosne opened this issue Aug 28, 2023 · 10 comments
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@rcosne
Copy link

rcosne commented Aug 28, 2023

Hi,

I'm currently migrating an application to Spring Boot 3.x. This application declare a rest controller and a websocket endpoint authenticated via basic auth. But the basic auth does not work anymore on the websocket endpoint.

Sample application:
https://github.com/rcosne/ws-test

RestController: http://localhost:8080/test
Websocket endpoint: ws://localhost:8080/wstest

It seems that the whole security filter chain is skipped in this case. I've tried to declare a customer filter via @Component, and via a JettyServerCustomizer, in both case, the filter is applied in the rest controller, but not in the websocket endpoint.

I've also tested with Tomcat, then the basic auth works with the websocket.

Best Regards,
Rémy

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 28, 2023
@wilkinsona wilkinsona self-assigned this Aug 29, 2023
@wilkinsona
Copy link
Member

wilkinsona commented Aug 29, 2023

Thanks for the sample. The difference in behaviour is due to different ordering of the filters that handle the initial upgrade request. With Tomcat, this filter is at the end of the chain. Crucially, this means that Spring Security's filter runs before the upgrade request is handled and can require basic authentication. With Jetty, this filter is at the start of the chain and its handling of the upgrade request is such that the rest of the chain isn't called. Crucially, this means that Spring Security's filter doesn't get a chance to run.

With both Tomcat and Jetty, the filter that handles upgrades is registered last so it should be at the end of the chain. However, that's not the case with Jetty because Jetty forces it to the start of the chain. Unfortunately, this clashes with a filter-based security framework, such as Spring Security, preventing it from securing upgrade requests. This appears to have been a change in Jetty 10.

We could possibly override this behavior by registering Jetty's WebSocketUpgradeFilter ourselves rather than relying on org.eclipse.jetty.websocket.servlet.WebSocketUpgradeFilter.ensureFilter(ServletContext) but that feels potentially brittle. It also wouldn't help other Jetty users as org.eclipse.jetty.websocket.jakarta.server.config.JakartaWebSocketServletContainerInitializer calls ensureFilter.

For these reasons I think it would be better to address this in Jetty itself. To that end, please open a Jetty issue for this so that they can investigate. Looking at jetty/jetty.project#5648 (which drove the change to the filter's order) they did consider filters like Spring Security but they talk about web.xml which isn't applicable in a Spring Boot app using embedded Jetty.

If a general fix in Jetty isn't possible, we can re-open the issue and consider a solution that's specific to Spring Boot.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
@wilkinsona wilkinsona added for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 29, 2023
@joakime
Copy link

joakime commented Aug 29, 2023

The reason for the websocket upgrade filter being first is simply because there are thousands of existing Filters out in the wild that modify the request / response, wrap the HttpServletRequest and/or HttpServletResponse, or provide overrides of the HttpServletRequest.getInputStream() or HttpServletResponse.getOutputStream() that all break when websocket is in the mix.

The decision to put upgrade at the start is intentional to not break the thousands of webapps that are not websocket aware.

Also, (putting on my jakarta websocket hat), there is zero requirement for a server that supports jakarta websocket to support that upgrade via a filter.

Some actual implementations of jakarta.websocket server upgrade that exist in the wild.

  • No servlets involved at all, the server doesn't even support servlets or has a servlet jar.
  • Upgrade done outside of a ServletContext (and still supports jakarta.websocket.server.ServerApplicationConfig)
  • Upgrade at the same layer as Servlet Security, and Servlet Session handling (which is before the Filter Chain is evaluated)

Assuming that the jakarta.websocket upgrade will be done in a filter is a poor assumption.

(putting my jetty hat back on)

Jetty at one point had the javax.websocket upgrade at the steps before the Filter Chain is evaluated (after Servlet Security, and Servlet Session handling).
That upgrade style doesn't exist in Jetty 10 / 11 / 12 at this point in time.

A workaround for Spring Boot is to add the existing WebSocketUpgradeFilter itself (no need to override / extend the default one), before adding endpoints, or accessing the websocket ServerContainer.
Just make sure to define it with RequestDispatcher.REQUEST only, on a url-mapping of /*, with async-supported set to true.

@joakime
Copy link

joakime commented Aug 29, 2023

@wilkinsona would you like PR that introduces a special ServletContainerInitializer for spring-boot that puts the WebSocketUpgradeFilter where you want it?

I suggest a ServletContainerInitializer as that can be used with discovery tooling to support automatic additions of jakarta websocket endpoints.
aka.

import jakarta.websocket.Endpoint;
import jakarta.websocket.server.ServerApplicationConfig;
import jakarta.websocket.server.ServerContainer;
import jakarta.websocket.server.ServerEndpoint;
import jakarta.websocket.server.ServerEndpointConfig;

@HandlesTypes({ServerApplicationConfig.class, ServerEndpoint.class, Endpoint.class})
public class SpringBootJettyWebSocketUpgradeInitializer implements ServletContainerInitializer

@wilkinsona
Copy link
Member

Thanks, @joakime. Sounds like we need to try to do something in Boot to address this.

would you like PR that introduces a special ServletContainerInitializer

Thanks for the offer. Boot's doesn't honour the ServletContainertInitializer contract but I think we can achieve a similar end result using a FilterRegistrationBean.

@rcosne, you can work around your problem with the following addition to your app:

@Bean
FilterRegistrationBean<WebSocketUpgradeFilter> webSocketUpgradeFilter() {
	FilterRegistrationBean<WebSocketUpgradeFilter> registration = new FilterRegistrationBean<>(new WebSocketUpgradeFilter());
	registration.setAsyncSupported(true);
	registration.setDispatcherTypes(DispatcherType.REQUEST);
	registration.setName(WebSocketUpgradeFilter.class.getName());
	registration.setOrder(Ordered.LOWEST_PRECEDENCE);
	registration.setUrlPatterns(List.of("/*"));
	return registration;
}

We can probably add this bean directly to Boot but, given what @joakime has said above, I am a little wary of the potential for a regression. If someone's using Jetty and WebSockets without Spring Security they may be relying on the current filter ordering.

@wilkinsona wilkinsona added type: bug A general bug for: team-attention An issue we'd like other members of the team to review and removed for: external-project For an external project and not something we can fix labels Aug 30, 2023
@wilkinsona wilkinsona reopened this Aug 30, 2023
@wilkinsona wilkinsona added for: team-meeting An issue we'd like to discuss as a team to make progress and removed for: team-attention An issue we'd like other members of the team to review labels Aug 30, 2023
@wilkinsona
Copy link
Member

As suspected, Spring Boot 3.0.x (I tested 3.0.10) is also affected and 2.7.x (I tested 2.7.15) is not affected.

@wilkinsona wilkinsona changed the title Spring Boot 3.1.3 - Basic authentication not working for websocket with Jetty In 3.0.x and later, Spring Security cannot be used to secure a WebSocket upgrade request when using Jetty Aug 30, 2023
@wilkinsona wilkinsona added type: regression A regression from a previous release and removed type: bug A general bug labels Aug 30, 2023
@rcosne
Copy link
Author

rcosne commented Aug 30, 2023

Hi @wilkinsona, the workaround works ! Thank you.

@joakime
Copy link

joakime commented Aug 30, 2023

@wilkinsona nice.

BTW, If you have a user of Jetty 9, there's an extra requirement for that FilterRegistrationBean setup.

In Jetty 9, there is also a ServletContext attribute requirement for it to work. (thankfully we got rid of this requirement in Jetty 10)
It would look something like this ...

@Bean
FilterRegistrationBean<WebSocketUpgradeFilter> webSocketUpgradeFilter() {
	WebSocketUpgradeFilter websocketFilter = new WebSocketUpgradeFilter();
        getServletContext().setAttribute(WebSocketUpgradeFilter.ATTR_KEY, websocketFilter);
        FilterRegistrationBean<WebSocketUpgradeFilter> registration = new FilterRegistrationBean<>(websocketFilter);
	registration.setAsyncSupported(true);
	registration.setDispatcherTypes(DispatcherType.REQUEST);
	registration.setName(WebSocketUpgradeFilter.class.getName());
	registration.setOrder(Ordered.LOWEST_PRECEDENCE);
	registration.setUrlPatterns(List.of("/*"));
	return registration;
}

Without this ServletContext attribute in Jetty 9, there would be 2 WebSocketUpgradeFilters, causing your initialization to either fail, or in your FilterRegistrationBean to add a duplicate WebSocketUpgradeFilter to a place/position that doesn't matter (as the default one is still in its early location)

@wilkinsona
Copy link
Member

There's no need for the registration bean in Spring Boot 2.7 with Jetty 9 as the filter ordering's fine there. Regardless, thanks for sharing the tip about the attribute as it may prove useful for someone in the future who does need to tweak the filter ordering.

@joakime
Copy link

joakime commented Aug 30, 2023

With Jetty 9, the default behavior on standalone Jetty, or an embedded Jetty user that relies on the default WebApp / WebAppContext Configurations, is ...

  • The org.eclipse.jetty.websocket.jsr356.server.deploy.WebSocketServerContainerInitializer is responsible for adding the WebSocketUpgradeFilter
  • The nature of Servlet Initialization ordering (between webapp / container / server / and various descriptors) means this WebSocketUpgradeFilter will be first in line on the FilterChain (just like Jetty 10/11/12)

@wilkinsona
Copy link
Member

While we use WebSocketServerContainerInitializer, we do so in an unusual way. As mentioned above, Boot doesn't support the ServletContainerInitializer contract so WebSocketServerContainerInitializer is triggered through an explicit call to initialize. This happens after other filters have already been registered, allowing Spring Security to secure upgrade requests and also aligning the behavior with Tomcat.

@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 30, 2023
@wilkinsona wilkinsona added this to the 3.0.x milestone Aug 30, 2023
@wilkinsona wilkinsona modified the milestones: 3.0.x, 3.0.11 Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

4 participants