EnableWebSocketSecurity is not 1:1 replacement for AbstractSecurityWebSocketMessageBrokerConfigurer

[filiphr]

Describe the bug
When using AbstractSecurityWebSocketMessageBrokerConfigurer we can override sameOriginDisabled and enable or disable the CsrfChannelInterceptor. In configureClientInboundChannel it does

if (!sameOriginDisabled()) {
    registration.interceptors(this.context.getBean(CsrfChannelInterceptor.class));
}

However, with @EnableWebSocketSecurity it is not possible to disable it since in configureClientInboundChannel it does

ChannelInterceptor csrfChannelInterceptor = getBeanOrNull(CSRF_CHANNEL_INTERCEPTOR_BEAN_NAME, ChannelInterceptor.class);
    if (csrfChannelInterceptor != null) {
        this.csrfChannelInterceptor = csrfChannelInterceptor;
    }

and in the configuration itself it is already defined through

private ChannelInterceptor csrfChannelInterceptor = new XorCsrfChannelInterceptor();

The same sameOriginDisabled is also used in afterSingletonsInstantiated to configure the CSRF for the handler mappings.

Expected behavior
Provide a 1:1 replacement for the deprecated AbstractSecurityWebSocketMessageBrokerConfigurer

[sjohnr]

Hi @filiphr, thanks for reaching out!

The intention of the check when using @EnableWebSocketSecurity is to simply use any provided ChannelInterceptor (named csrfChannelInterceptor). So I believe you can do:

	@Bean
	public ChannelInterceptor csrfChannelInterceptor() {
		return new ChannelInterceptor() {};
	}

Does that provide a viable path forward for you?

[filiphr]

That workaround is a bit more complicated, especially considering the fact that we are creating returning the boolean based on a property configuration.

We could try using Boot's @ConditionalOnProperty. However, I think that the best approach would be if you provide a customizer bean that people can implement and have the exact same behaviour as it was previously. An additional alternative would be to not to deprecate the AbstractSecurityWebSocketMessageBrokerConfigurer

[sjohnr]

@filiphr sorry for the delay on replies.

You can use Boot's @ConditionalOn... or you can also embed the logic directly in the @Bean method. You can also inject other beans such as the ApplicationContext using method injection. For example:

	@Bean
	public ChannelInterceptor csrfChannelInterceptor(ApplicationContext context) {
		if (!sameOriginDisabled(context)) {
			return new XorCsrfChannelInterceptor();
		} else {
			return new ChannelInterceptor() {};
		}
	}

	// ...

Does that help?

In regard to making @EnableWebSocketSecurity a 1:1 replacement, switching from extending an abstract class to publishing a @Bean should not usually limit you since you have most of Spring's IoC capabilities available when bootstrapping the application. Do you agree? If so, can we close this issue? Let me know if you have other thoughts around this.

[filiphr]

@sjohnr that is not enough. That will only cover the case when invoking configureClientInboundChannel. However, in AbstractSecurityWebSocketMessageBrokerConfigurer#afterSingletonsInstantiated the same check is being used.

spring-security/config/src/main/java/org/springframework/security/config/annotation/web/socket/AbstractSecurityWebSocketMessageBrokerConfigurer.java

Lines 222 to 245 in 9de717a

	public void afterSingletonsInstantiated() {
	if (sameOriginDisabled()) {
	return;
	}
	String beanName = "stompWebSocketHandlerMapping";
	SimpleUrlHandlerMapping mapping = this.context.getBean(beanName, SimpleUrlHandlerMapping.class);
	Map<String, Object> mappings = mapping.getHandlerMap();
	for (Object object : mappings.values()) {
	if (object instanceof SockJsHttpRequestHandler) {
	setHandshakeInterceptors((SockJsHttpRequestHandler) object);
	}
	else if (object instanceof WebSocketHttpRequestHandler) {
	setHandshakeInterceptors((WebSocketHttpRequestHandler) object);
	}
	else {
	throw new IllegalStateException("Bean " + beanName + " is expected to contain mappings to either a "
	+ "SockJsHttpRequestHandler or a WebSocketHttpRequestHandler but got " + object);
	}
	}
	if (this.inboundRegistry.containsMapping() && !this.inboundRegistry.isSimpDestPathMatcherConfigured()) {
	PathMatcher pathMatcher = getDefaultPathMatcher();
	this.inboundRegistry.simpDestPathMatcher(pathMatcher);
	}
	}

When using @EnableWebSocketSecurity the csrf is always configured on the SimpleUrlHandlerMapping.

I think that the best solution would be for Spring Security to provide some other interface that would be used in WebSocketMessageBrokerSecurityConfiguration in the exact same way as sameOriginDisabled is used in AbstractSecurityWebSocketMessageBrokerConfigurer

In regard to making @EnableWebSocketSecurity a 1:1 replacement, switching from extending an abstract class to publishing a @Bean should not usually limit you since you have most of Spring's IoC capabilities available when bootstrapping the application. Do you agree? If so, can we close this issue? Let me know if you have other thoughts around this.

I do not agree with this. Since the capabilities of the abstract class are not the same of the capabilities of the @EnableWebSocketSecurity. See my comments above

[sjohnr]

Hi @filiphr, thanks for following up! I did not focus on the afterSingletonsInstantiated() and missed that you would want to disable the handshake interceptor as well. That makes sense.

Currently with CsrfTokenHandshakeInterceptor, it seems possible to cause it to be skipped functionally if CSRF protection is disabled for the handshake request. I haven't prototyped that, but I understand that it would be a workaround at best (assuming it is a viable workaround) and not ideal to need multiple configurations to migrate to the new support.

I don't have full context on sameOriginDisabled but it matches the XML configuration attribute <websocket-message-broker same-origin-disabled=""> and the attribute's description reads:

Disables the requrement for CSRF token to be present in the Stomp headers (default false).
Changing the default is useful if it is necessary to allow other origins to make SockJS
connections.

I'm not keen on re-introducing this type of configuration element if we can be more straightforward then that. Since this seems to be about disabling CSRF protection, I'd prefer to provide a direct mechanism for disabling it.

I wonder if it would be better to make it possible to simply disable the protection through publishing a bean. Instead of a customizer with property names, we could add a DisableCsrfChannelInterceptor or similar (we can name it however we want). If such a ChannelInterceptor is published as a bean, it would also disable the HandshakeInterceptor registration.

Alternatively, we could detect the HandshakeInterceptor for CSRF as a bean and if exists, register that instead of CsrfTokenHandshakeInterceptor. This would allow registering no-op implementations for both. It seems less desirable to need two configurations to fully disable CSRF, but may make it more logical (and hopefully intuitive) to be able to configure the channel and handshake separately.

Either way, the goal would be to allow a smooth migration (with some good javadoc to help guide the user), but not simply replicate old patterns needlessly. Would either of these be a promising path forward?

[filiphr]

I'm not keen on re-introducing this type of configuration element if we can be more straightforward then that. Since this seems to be about disabling CSRF protection, I'd prefer to provide a direct mechanism for disabling it.

I am completely OK to have some other mechanism that will disable the CSRF protection. It doesn't have to be named sameOriginsDisabled, or mention anything for same origins.

I wonder if it would be better to make it possible to simply disable the protection through publishing a bean. Instead of a customizer with property names, we could add a DisableCsrfChannelInterceptor or similar (we can name it however we want). If such a ChannelInterceptor is published as a bean, it would also disable the HandshakeInterceptor registration.

Using a special bean for the ChannelInterceptor to disable the HandshakeInterceptor is a bit strange in my opinion and not really intuitive.

Alternatively, we could detect the HandshakeInterceptor for CSRF as a bean and if exists, register that instead of CsrfTokenHandshakeInterceptor. This would allow registering no-op implementations for both. It seems less desirable to need two configurations to fully disable CSRF, but may make it more logical (and hopefully intuitive) to be able to configure the channel and handshake separately.

Being able to detect a custom HandshakeInterceptor for CSRF is fine. However, requiring to register 2 beans to disable CSRF for STOMP is a bit strange.

Either way, the goal would be to allow a smooth migration (with some good javadoc to help guide the user), but not simply replicate old patterns needlessly. Would either of these be a promising path forward?

The goal is indeed to allow a smooth migration. How this is done doesn't really matter. However, I do not like the proposals as they are not that intuitive.

I would suggest the following. Why not introduce something like the WebSocketMessageBrokerConfigurer? e.g.

public interface WebSocketSecurityConfigurer {

    /**
     * <p>
     * Determines if a CSRF token is required for connecting. This protects against remote
     * sites from connecting to the application and being able to read/write data over the
     * connection. The default is false (the token is required).
     * </p>
     *  <p>
     * Subclasses can override this method to disable CSRF protection
     * </p>
     * @return false if a CSRF token is required for connecting, else true
     */
    default boolean csrfDisabled() {
        return false;
    }

}

and then in WebSocketMessageBrokerSecurityConfiguration

you would use such a bean (if one exists);

If the csrfDisabled method returns true then you would use the DisableCsrfChannelInterceptor bean for the channel interceptor and you won't call configureCsrf.

That makes it relatively easy for people to configure what they need to configure. At least in my opinion

[sjohnr]

Thanks for the feedback, @filiphr. I appreciate the conversation!

Why not introduce something like the WebSocketMessageBrokerConfigurer?

Since the existing abstract configurer class was deprecated in favor of annotations + configuration through publishing beans, my preference would be not to introduce a new interface like this to replace it. This is also generally the opposite of the direction the Spring Security codebase is headed, which favors publishing components as beans that are directly configurable themselves (instead of configurers that apply or implement configuration).

Perhaps we could look at introducing an attribute on the annotation as well, such as @EnableWebSocketSecurity(csrfEnabled = false). I'll spend some time thinking about this a bit more.

[filiphr]

Perhaps we could look at introducing an attribute on the annotation as well, such as @EnableWebSocketSecurity(csrfEnabled = false). I'll spend some time thinking about this a bit more.

The annotation is not enough, since it doesn't allow programatic configuration through properties. It means you would be forcing people to use Spring Boot and @ConditionalOn or some other complex properties.

Since the existing abstract configurer class was deprecated in favor of annotations + configuration through publishing beans, my preference would be not to introduce a new interface like this to replace it. This is also generally the opposite of the direction the Spring Security codebase is headed, which favors publishing components as beans that are directly configurable themselves (instead of configurers that apply or implement configuration).

You can still achieve the same thing by allowing people to publish beans. Thinking more about it, to make it even more easier to configure CSRF you can introduce an enum

public enum WebSocketCsrfStrategy {

    XOR_CSRF,
    CSRF,
    NONE,
}

People can expose this bean and based on this you can even switch to using CsrfChannelInterceptor if WebSocketCsrfStrategy is CSRF (or whatever name you pick).

Anyways, I'll leave it up to you. I completely understand the fact that you would like to favor another direction for Spring Security configuration. I am trying to provide feedback of how we use some parts of Spring Security and what kind of stuff we need to configure for our projects.

[sjohnr]

Hi @filiphr.

The annotation is not enough

Agreed, I mentioned it as an additional option to be the easiest way for folks migrating to fully disable CSRF. But if it isn't useful immediately, we don't need to introduce it right now.

[duongxinh2003]

Any update?

[YaraslauBarysenka]

Any update?