Wrong SecurityContext with async-mode-enabled: true results

[ncioj10]

Describe the bug
The Spring Security Context obtained by the OncePerRequestFilter is wrong when upgrading to 11.1.0 with async-mode-enabled: true by default.
This can lead to very serious security concerns as the context is also not cleared correctly so requests get sometimes authorized with credentials from other users.

To Reproduce
Create a Filter and try to access the context with SecurityContextHolder within the dataFetchers.

Expected behavior
The Security Context should contain the correct context.

[oliemansm]

Thanks for reporting @ncioj10! Just noticed the same thing. The problem is caused by this snippet in HttpRequestInvokerImpl in graphql-java-servlet:

    asyncContext.start(
        () -> {
          FutureExecutionResult futureResult = invoke(invocationInput, request, response);
          futureHolder.set(futureResult);
          handle(futureResult, request, response, listenerHandler)
              .thenAccept(it -> asyncContext.complete());
        });

We probably need to introduce something like an AsyncContextTaskDecorator in that library, and then provide an autoconfigured AsyncSecurityContextTaskDecorator implementation of it here if Spring Security is on the class path that wraps that runnable to propagate the SecurityContext.

[ncioj10]

As this has some security implications I would propose to not set it as default value if possible as a first step?

As for the fix: I'm not too deep into the code of graphql-java-servlet, but I think one of the problems will be that Spring can take care of this if you have it running on a separate thread, but in this case we're running on the same Thread only in completable futures, so it might get a little interesting to set this without producing interference as it sets it globally?

[oliemansm]

You'll have to stick with 11.0.0 for now to not have this behavior or just disable async-mode through props as you've already figured out.

It's not running on the same Thread actually. In the snippet above, the asyncContext.start(Runnable) method starts that lambda as a Thread. So having that Runnable wrapped with the decorator I descirbed that copies the SecurityContext before starting and clears it again after should do the trick.

[ncioj10]

Yes, I'd just try to mitigate problems for those who update the library and did not notice the change. Otherwise it would make sense to update the Readme so it warns about it.

I see, you mean something like this as fix right: spring-projects/spring-security#6856 (comment) ?

[ncioj10]

Sorry, wrong button

[oliemansm]

Yes exactly. Good point about the readme. I'll add a warning at the top and update the release notes too.

[ssprang]

Shouldn't the executing thread be wrapped in a org.springframework.security.task.DelegatingSecurityContextAsyncTaskExecutor instead of introducing a AsyncContextTaskDecorator?

[oliemansm]

I understand the thought, but graphql-java-servlet is independent of Spring, so that's not an option unfortunately.

[ssprang]

Is there a way to globally specify the Executor for the AsyncDataFetcher? That way I could specify one that decorates threads with a DelegatingSecurityContext..

val executor = ThreadPoolTaskExecutor()
// Set parallelism explicitly, default is 1
executor.corePoolSize = 5
// Max pool size (threads to scale up to if queue is full)
executor.maxPoolSize = 10
// Possibility to set queue capacity, default is Integer.MAX_VALUE
// executor.setQueueCapacity(500)
executor.setThreadNamePrefix("async-data-fetching-")
executor.setTaskDecorator { runnable -> DelegatingSecurityContextRunnable(runnable) }
executor.initialize()
....
// Pass executor for the AsyncDataFetcher somehow

[oliemansm]

That ThreadPoolTaskExecutor is again a Spring construct. Reading this article sheds some insight and raises the question whether we should be using this in the first place, or maybe like @ncioj10 suggested we should at least disable it by default. The article is from 2012, but I just ran a test and it's currently still using the HTTP worker thread pool to run these async Threads on.

See: https://dzone.com/articles/limited-usefulness

[ssprang]

Sorry, my example was too specific. It doesn't really matter which executor I pass in, I just wonder if there's a way to specify it myself because then I have the possibility to decide stuff like amount of threads and wrap it in a DelegatingSecurityContext in my Spring project.

[oliemansm]

So far I haven't found a way to define the thread pool executor to be used when running AsyncContext.start. And the latter is necessary to be able to benefit from the timeout option. If we can find a way to do that then we can make it configurable of course. Any help trying to find that way is appreciated... 😬

[oliemansm]

Found an example: https://developerlife.com/2011/04/13/creating-asynchronous-servlets-with-tomcat-7-servlet-3-0-api/

[oliemansm]

@ssprang Got a proof of concept of this working locally:

Is there a way to globally specify the Executor for the AsyncDataFetcher? That way I could specify one that decorates threads with a DelegatingSecurityContext..

The proof of concept I have now creates such a ThreadPoolTaskExecutor and wraps it using the DelegatingSecurityContextAsyncTaskExecutor provided by Spring Security like this:

  public Executor threadPoolTaskExecutor() {
    ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
    executor.setCorePoolSize(asyncServletProperties.getThreads().getMin());
    executor.setMaxPoolSize(asyncServletProperties.getThreads().getMax());
    executor.setThreadNamePrefix("graphql-");
    executor.initialize();
    return new DelegatingSecurityContextAsyncTaskExecutor(executor);
  }

Now I can simply add simple props to be able to configure this async thread pool executor using props and enabling/disabling the security context delegation through props too. Default setting would be that if Spring Security is found on the class path and async mode is enabled it'll configure this for you automatically, assuming you'd want something like this anyway. But with the ability to opt-out.

You also raised the idea of being able to provide an Executor of your own. Something that could be an additional option. Main problem with that one is that usually there already is an Executor bean or maybe even more than one. Not sure what best practices are in this particular situation. Maybe a specific qualifier should be used?

[oliemansm]

Release 12.0.0 will contain support for this. Both through props or even by providing your own Executor that needs to be named graphqlAsyncTaskExecutor in that case to be picked up for this purpose. By default if Spring Security is found on the class path and async mode is enabled it'll enable the security context delegation. Configurable through props (default values listed here):

graphql:
  servlet:
    async:
      enabled: true
      timeout: 30000
      delegate-security-context: true
      threads:
        min: 10 # same default as Tomcat HTTP workers
        max: 200  # same default as Tomcat HTTP workers
        name-prefix: graphql-exec-

[ssprang]

Very nice! Thank you @oliemansm 🙏🏻 ! This is what I was looking for 😍

[ncioj10]

Really cool thanks a lot!

[PhilippS93]

Hi!
We are using version 14.0.0 of the library and having some issues with the SecurityContext.
We are using resolvers and queries. For each, we use @PreAuthorize("isAuthenticated()") as a security check.
Only for the query, this is working, for all resolvers (which are executed after the query), we get

"Exception while fetching data (/userStatistics/workload/load) : An Authentication object was not found in the SecurityContext"

because @PreAuthorize("isAuthenticated()") fails the security check on the resolvers. A resolver looks like this:

  @PreAuthorize("isAuthenticated()")
    public @NotNull CompletionStage<List<DoubleTimePoint>> load(WorkloadStatisticsDTO workloadStatisticsDTO, DataFetchingEnvironment env) throws Exception {
      // doing stuff

        return supplyAsync(() ->  result);
    }

The query looks like

    @PreAuthorize("isAuthenticated()")
    public CompletionStage<UserStatisticsResponseDTO> userStatistics(UserStatisticsInputDTO input, DataFetchingEnvironment env) throws Exception {
        UserStatisticsResponseDTO response = new UserStatisticsResponseDTO();
  // doing stuff

        return CompletableFuture.completedFuture(response);
    }

Why is the Authentication only set in the query, but not in the resolvers?

[rohanliston]

@PhilippS93 From what I can gather, CompletableFuture.completedFuture spawns the new thread using the ThreadPoolTaskExecutor that is configured to pass on the security context, whereas CompletableFuture.supplyAsync spawns threads using the common ForkJoinPool.

I don't know if the above is 100% accurate (it's based on a semi-related Stack Overflow answer), but from my own observation, using completedFuture exclusively in resolvers seems to work for me.

@oliemansm Do you have any wisdom/comments to offer on the above?

EDIT: Based on this tutorial on parallel resolvers, it looks like you can supply the executor as an argument to supplyAsync. I haven't tried this yet, but it's possible that might fix the issue as well.