Using strategy MODE_INHERITABLETHREADLOCAL is dangerous with thread pools

[jukkasi]

Summary

When Spring Async annotation is used, it is often instructed to set SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL) so that security context is accessible in spawned threads.

However with thread pools (for Spring Boot pool is auto-configured) this is highly dangerous, because when thread is reused from pool, also security context which was set for thread when it was created is reused leading to issue where task relies on completely wrong, some other user's security context.

So I begin to wonder why SecurityContextHolder.MODE_INHERITABLETHREADLOCAL even exists because proper way to avoid this problem is to wrap executor using DelegatingSecurityContextExecutor

[rwinch]

it is often instructed to set SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL) so that security context is accessible in spawned threads.

Can you point me to where you are seeing these instructions? The inherited mode is generally only useful when a new Thread is created each time. Spring Security provides Concurrency Support which ensures the context is cleared out appropriately.

[jukkasi]

By instructed I mean first 10 hits from Google, granted that none is "official" documentation.

I am aware of DelegatingSecurityContextRunnable but it leads to other issue I linked because same SecurityContext is still used, granted that sometimes it might be useful.

But like I said it works and should be used for any type of executor so it would be safer that INHERITABLETHREADLOCAL mode would not exist at all.

[rwinch]

because same SecurityContext is still used,

If you do not inject a SecurityContext, then the current context is used automatically. This means it is not the same SecurityContext. For that reason, I'm not sure I understand your concern about using the concurrency support. Can you please expand on why it will not work for you?

[jukkasi]

I'm not sure what you mean by injecting SecurityContext.

What I'm trying to say in this issue is that when MODE_INHERITABLETHREADLOCAL is used along with thread pool executor, which is created by default for example by Spring Boot autoconfiguration, and which is not wrapped by DelegatingSecurityContextExecutor, you get a very serious problem where tasks are executed possibly by using a wrong security context.

If SecurityContextHolder.strategy is left to default and DelegatingSecurityContextExecutor is used as executor, wrapping the default one what ever it might be, SecurityContext is still shared (same instance) between threads, like described in #3378 and you get a problem when for example user logs out in the middle of task processing.

[rwinch]

Yes you should not use MODE_INHERITABLETHREADLOCAL in a pooled environment. If you do that, you have a misconfiguration and there isn't much we can do.

Logging out is not going to cancel any tasks that are running in the background. You will have issues with this regardless, because most likely the task is going to get the SecurityContext and store it in a variable which we won't have access to it.

SecurityContext context = SecurityContextHolder.getContext();
// security checks performed
// if the user logs out nothing we can do
// ...processing the task

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[spring-projects-issues]

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

[captrespect]

Ouch, we just ran into this issue ourselves. We've been happily using MODE_INHERITABLETHREADLOCAL to propagate the security context. Then in our last release we implemented AsyncConfigurer to log uncaught exceptions in out async tasks. To do so we also had to impliment AsyncConfigurer.getAsyncExecutor() as well. We had it return a thread pool executor and soon found that our tasks were running with incorrect security contexts.

We ended up just rolling back the change. Now I'm trying to figure out how to do this properly.

[jukkasi]

We ended up using TaskDecorator, which can be set for executor, that creates a new SecurityContext for running thread and sets authentication for that context. However I'm not sure if this has any side effects, any feedback is appreciated.

private fun authenticatedTask(): TaskDecorator {
    return TaskDecorator { runnable ->
        val authentication = SecurityContextHolder.getContext().authentication
        Runnable {
            try {
                SecurityContextHolder.getContext().authentication = authentication
                runnable.run()
            } finally {
                SecurityContextHolder.clearContext()
            }
        }
    }
}

(code is Kotlin)

[captrespect]

In my case I should have extended AsyncConfigurerSupport instead. I didn't really need the thread pool.

[peternelissen]

@rwinch

You are absolutely right that "we should not use MODE_INHERITABLETHREADLOCAL in a pooled environment. If you do that, you have a misconfiguration and there isn't much we can do."!

And also, that using Concurrency Support of spring security solves the issue.

But I think it should be mentioned here too: https://docs.spring.io/spring-security/site/docs/5.2.0.M4/reference/htmlsingle/#securitycontextholder-securitycontext-and-authentication-objects

This documentation suggests that MODE_INHERITABLETHREADLOCAL just works. It's dangerous to not mention the absence of pooled environments as precondition. It looks like many people make this mistake, and maybe many people have the mistake in production.

I follow jukkasi for suggesting that MODE_INHERITABLETHREADLOCAL is just dangerous. If many people use a feature the wrong way, it's too easy to say it's their misconfiguration. Is there a way to log a warning if MODE_INHERITABLETHREADLOCAL is used with a pooled environment? (or throw an exception of course)

@jukkasi
I don't get how your solution is different than using executor.setTaskDecorator(runnable -> new DelegatingSecurityContextRunnable(runnable)) of Concurrency Support of spring security. Can you explain please?

[jukkasi]

@peternelissen see my previous comment "If SecurityContextHolder.strategy is left to default...". My solution creates NEW context for running task. Delegating will end up using same context for original and task thread, so if user logs out for example, the context that task is using is cleared and might fail if it still needs to use context.

But this depends on your use case and sometimes might be the wanted result.

[peternelissen]

As info for others who stumble on this issue, there are several options to solve this:

1. Using spring security Concurrency Support: DelegatingSecurityContextRunnable

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	executor.setTaskDecorator(runnable -> new DelegatingSecurityContextRunnable(runnable));
	return executor;
  }

Or if you want the async task to survive a change in the original session (like a logout), a slight different implementation (see comment of jukkasi) is needed of DelegatingSecurityContextRunnable.

2. Using spring security Concurrency Support: DelegatingSecurityContextAsyncTaskExecutor

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	return executor;
  }

  @Bean
  public DelegatingSecurityContextAsyncTaskExecutor taskExecutor(ThreadPoolTaskExecutor delegate) {
     return new DelegatingSecurityContextAsyncTaskExecutor(delegate);
  }

3. using MODE_INHERITABLETHREADLOCAL in combination with non-pooled task executor. For example SimpleAsyncTaskExecutor.

[manishbansal8843]

We ended up using TaskDecorator, which can be set for executor, that creates a new SecurityContext for running thread and sets authentication for that context. However I'm not sure if this has any side effects, any feedback is appreciated.

private fun authenticatedTask(): TaskDecorator {
    return TaskDecorator { runnable ->
        val authentication = SecurityContextHolder.getContext().authentication
        Runnable {
            try {
                SecurityContextHolder.getContext().authentication = authentication
                runnable.run()
            } finally {
                SecurityContextHolder.clearContext()
            }
        }
    }
}

(code is Kotlin)

I also had the same requirement and implemented this fix. Further, i have analyzed the spring code and tested it thoroghly. Seems like a good hack. Below is the java code for reference i used.

executor.setTaskDecorator(new TaskDecorator() {
			@Override
			public Runnable decorate(Runnable runnable) {
				final Authentication auth = SecurityContextHolder.getContext().getAuthentication();
				return () -> {
					try {
						SecurityContextHolder.getContext().setAuthentication(auth);
						runnable.run();
					} finally {
						SecurityContextHolder.clearContext();
					}
				};
			}
		});

[ghousek1]

As info for others who stumble on this issue, there are several options to solve this:

1. Using spring security Concurrency Support: DelegatingSecurityContextRunnable

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	executor.setTaskDecorator(runnable -> new DelegatingSecurityContextRunnable(runnable));
	return executor;
  }

Or if you want the async task to survive a change in the original session (like a logout), a slight different implementation (see comment of jukkasi) is needed of DelegatingSecurityContextRunnable.

2. Using spring security Concurrency Support: DelegatingSecurityContextAsyncTaskExecutor

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	return executor;
  }

  @Bean
  public DelegatingSecurityContextAsyncTaskExecutor taskExecutor(ThreadPoolTaskExecutor delegate) {
     return new DelegatingSecurityContextAsyncTaskExecutor(delegate);
  }

3. using MODE_INHERITABLETHREADLOCAL in combination with non-pooled task executor. For example SimpleAsyncTaskExecutor.

Are these methods safe to use in a production application. security context will get cleared if left that thread? pls reply

[velykov]

If I use fork join pool standard methods like parallelStream with SecurityContext MODE_INHERITABLETHREADLOCAL mode, then i've got a token expiration exception

[busaeed]

As info for others who stumble on this issue, there are several options to solve this:

1. Using spring security Concurrency Support: DelegatingSecurityContextRunnable

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	executor.setTaskDecorator(runnable -> new DelegatingSecurityContextRunnable(runnable));
	return executor;
  }

Or if you want the async task to survive a change in the original session (like a logout), a slight different implementation (see comment of jukkasi) is needed of DelegatingSecurityContextRunnable.

2. Using spring security Concurrency Support: DelegatingSecurityContextAsyncTaskExecutor

  @Bean
  public ThreadPoolTaskExecutor threadPoolTaskExecutor() {
	ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
	executor.setCorePoolSize(10);
	executor.setMaxPoolSize(100);
	executor.setQueueCapacity(50);
	executor.setThreadNamePrefix("async-");
	return executor;
  }

  @Bean
  public DelegatingSecurityContextAsyncTaskExecutor taskExecutor(ThreadPoolTaskExecutor delegate) {
     return new DelegatingSecurityContextAsyncTaskExecutor(delegate);
  }

3. using MODE_INHERITABLETHREADLOCAL in combination with non-pooled task executor. For example SimpleAsyncTaskExecutor.

I tried your 2nd solution by adding the code to my security configuration class but it didn't work. It returns anonymousUser when trying to get the userPrincipal() from SecurityContext.

[maciejmiklas]

How can we propagate Security Context to Fork Join Pool / Executor Service from JDK?