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

SecurityReactorContextSubscriber#LoadingMap fails to retrieve Authentication #11973

Open
ttddyy opened this issue Oct 8, 2022 · 3 comments
Open
Assignees
Labels
status: feedback-provided Feedback has been provided type: bug A general bug

Comments

@ttddyy
Copy link
Contributor

ttddyy commented Oct 8, 2022

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 ttddyy added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Oct 8, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Oct 10, 2022

Note that #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 jzheaux self-assigned this Oct 10, 2022
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 10, 2022
@ttddyy
Copy link
Contributor Author

ttddyy commented Oct 10, 2022

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(reactor/reactor-core#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).

@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 labels Oct 10, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Oct 24, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: bug A general bug
Projects
Status: No status
Development

No branches or pull requests

3 participants