spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

SecurityReactorContextSubscriber#LoadingMap fails to retrieve Authentication

Open ttddyy opened this issue 1 year ago • 2 comments

While upgrading Spring Boot from 2.6 to 2.7, one of our tests started failing. The test verifies thread switching with WebClient for OAuth2 client in the servlet environment.

This happens when WebClient uses subscribeOn.(uses a different thread than the caller thread) The SecurityReactorContextSubscriber/LoadingMap resolves null for Authentication.

I have created a minimum repro here.

This is the test case:

@SpringJUnitConfig
public class MyTest {

	// To run this test, it needs to have "spring-boot-starter-oauth2-resource-server" and "spring-boot-starter-oauth2-client"
	// dependencies which triggers "OAuth2ImportSelector" to import "SecurityReactorContextConfiguration".
	//

	// from SecurityReactorContextConfiguration.SecurityReactorContextSubscriberRegistrar#SECURITY_REACTOR_CONTEXT_OPERATOR_KEY
	// also used by ServletOAuth2AuthorizedClientExchangeFilterFunction
	static final String SECURITY_REACTOR_CONTEXT_ATTRIBUTES_KEY = "org.springframework.security.SECURITY_CONTEXT_ATTRIBUTES";

	@Test
	@WithMockUser("foo")
	void demoTest() {
		Authentication authInMainThread = TestSecurityContextHolder.getContext().getAuthentication();

		AtomicReference<Authentication> authInFilter = new AtomicReference<>();
		WebClient webClient = WebClient.builder()
//				.filter(new ServletOAuth2AuthorizedClientExchangeFilterFunction())
				.filter((request, next) -> {
					return Mono.deferContextual(context -> {
						Map<Object, Object> contextAttributes = context.get(SECURITY_REACTOR_CONTEXT_ATTRIBUTES_KEY);
						Authentication auth = (Authentication) contextAttributes.get(Authentication.class);
						authInFilter.set(auth);
						return next.exchange(request);
					});
				}).build();

		webClient.get().uri("https://vmware.com")
				.retrieve()
				.bodyToMono(String.class)
				.subscribeOn(Schedulers.boundedElastic())  // <-- use different thread to make a call
				.block();

		assertThat(authInFilter.get()).isSameAs(authInMainThread);
	}

	@Configuration(proxyBeanMethods = false)
	@EnableWebSecurity
	static class MyConfig {

	}
}

Root cause

The issue is introduced by this commit which added the LoadingMap to lazily retrieve the servlet request/response/auth. Prior to this change, the request/response/auth were resolved on the caller's thread when the SecurityReactorContextSubscriber is created by the lifter. However, with LoadingMap the callback is deferred until the webclient operations are executed. When the thread is not on the caller's thread(by subscribeOn), it cannot retrieve any threadlocal values and they become null.

I haven't tested but the ServletOAuth2AuthorizedClientExchangeFilterFunction, which uses SecurityReactorContextSubscriber mechanism, should fail to resolve Authentication in this scenario.

ttddyy avatar Oct 08 '22 03:10 ttddyy

Note that https://github.com/spring-projects/spring-security/issues/11885 may supersede this issue.

That said, this seems like a problem that is going to come up more often given that Spring Security components are moving towards deferring the lookup of Authentication until it is needed. I feel like the solution in that and other cases is either for applications to use Spring Security's context propagation support or InheritableThreadLocalSecurityContextHolderStrategy.

@ttddyy is there something that I'm missing that is unique to this scenario where we'd want to make a special context propagation accommodation? If not, I'd prefer to close this issue in favor of the above explanation.

jzheaux avatar Oct 10 '22 18:10 jzheaux

That said, this seems like a problem that is going to come up more often given that Spring Security components are moving towards deferring the lookup of Authentication until it is needed.

Yes, I think so, especially when a thread is switched. For normal runnable/callable/scheduler/executor would be ok with the Spring Security's context propagation classes. But, I don't think it supports the propagation in this case from imperative(servlet thread) to reactive(reactor thread).

Also, I'm not sure InheritableThreadLocal can be used between servlet threads and reactor threads because they are managed separately.

is there something that I'm missing that is unique to this scenario where we'd want to make a special context propagation accommodation?

With #11885, I don't think it would resolve this case since Suppliler doesn't get a hold of the thread unless the returning value has already been resolved, then supplied.

The micrometer's context-propagation may be a suit here with contextCapture support in reactor(https://github.com/reactor/reactor-core/pull/3145)?

I'm wondering why the auth context resolution is deferred in this case(SecurityReactorContextSubscriber) to begin with? I think resolving the authentication at the reactor operation assembling time is the correct time to resolve it(previous implementation).

ttddyy avatar Oct 10 '22 21:10 ttddyy

It's due to #9841. On shutdown, the subscriber is exercised. Before #9841, this meant a new SecurityContext was created and placed in the underlying ThreadLocal on shutdown, creating a memory leak. In the worst case, we can regress #9841 and explain that a memory leak on shutdown is unlikely to be a real problem, but I'd rather see if there is a solution that works that also doesn't create the memory leak.

jzheaux avatar Oct 24 '22 20:10 jzheaux