Security implementation broken

[ST-DDT]

The security implementation (#155) as released in v4.0.0 is vulnerable to concurrency issues.

• Impact: high (only affects authenticated users, loss/gain of random authentication)
• Reproducibility: easy (two concurrent calls, especially streaming calls)

Do NOT use that feature.

(Originally posted in #155 (comment) . I moved it here for better visibility)

Proof

Here a JUnit test that can be used to verify the issue (Click to expand)

package org.lognet.springboot.grpc.auth;

import static org.junit.Assert.assertThrows;
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.lognet.springboot.grpc.GrpcServerTestBase;
import org.lognet.springboot.grpc.demo.DemoApp;
import org.lognet.springboot.grpc.security.AuthCallCredentials;
import org.lognet.springboot.grpc.security.AuthHeader;
import org.lognet.springboot.grpc.security.EnableGrpcSecurity;
import org.lognet.springboot.grpc.security.GrpcSecurity;
import org.lognet.springboot.grpc.security.GrpcSecurityConfigurerAdapter;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.context.annotation.Import;
import org.springframework.security.authentication.dao.DaoAuthenticationProvider;
import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.crypto.password.NoOpPasswordEncoder;
import org.springframework.security.provisioning.InMemoryUserDetailsManager;
import org.springframework.test.context.junit4.SpringRunner;

import com.google.protobuf.Empty;

import io.grpc.Status;
import io.grpc.StatusRuntimeException;
import io.grpc.examples.SecuredGreeterGrpc;
import io.grpc.examples.GreeterGrpc.GreeterFutureStub;
import lombok.extern.slf4j.Slf4j;

@SpringBootTest(classes = DemoApp.class, properties = "spring.cloud.service-registry.auto-registration.enabled=false")
@RunWith(SpringRunner.class)
@Import({ ConcurrentAuthConfigTest.TestCfg.class })
@Slf4j
public class ConcurrentAuthConfigTest extends GrpcServerTestBase {

	private AuthCallCredentials callCredentials = new AuthCallCredentials(
			AuthHeader.builder().basic("test", "test".getBytes()));

	@TestConfiguration
	static class TestCfg {

		@EnableGrpcSecurity
		public class DemoGrpcSecurityConfig extends GrpcSecurityConfigurerAdapter {

			@Override
			public void configure(GrpcSecurity builder) throws Exception {
				DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
				List<GrantedAuthority> roles = Arrays.asList(new SimpleGrantedAuthority("ROLE_TEST"));
				UserDetailsService users = new InMemoryUserDetailsManager(new User("test", "test", roles));
				provider.setUserDetailsService(users);
				provider.setPasswordEncoder(NoOpPasswordEncoder.getInstance());

				builder.authenticationProvider(provider);
				builder.authorizeRequests().anyMethod().authenticated();
			}

		}
	}

	@Test
	public void concurrentTest() throws InterruptedException {
		System.out.println();

		final SecuredGreeterGrpc.SecuredGreeterBlockingStub unsecuredFutureStub = SecuredGreeterGrpc
				.newBlockingStub(selectedChanel);
		final SecuredGreeterGrpc.SecuredGreeterBlockingStub securedFutureStub = unsecuredFutureStub
				.withCallCredentials(callCredentials);

		int parallelTests = 10;

		List<Thread> threads = new ArrayList<>();
		// Number of threads that passed the test
		AtomicInteger successCounter = new AtomicInteger(0);
		AtomicInteger failureCounter = new AtomicInteger(0);

		Runnable authenticated = () -> {
			securedFutureStub.sayAuthHello(Empty.getDefaultInstance()).getMessage();
//			log.warn("Call passed");
		};
		Runnable unauthenticated = () -> {
			StatusRuntimeException err = assertThrows(StatusRuntimeException.class,
					() -> unsecuredFutureStub.sayAuthHello(Empty.getDefaultInstance()).getMessage());
			assertEquals(Status.Code.UNAUTHENTICATED, err.getStatus().getCode());
//			log.warn("Call blocked");
		};

		// Check that the assertions work as is (single threaded)
		authenticated.run();
		unauthenticated.run();

		for (int i = 0; i < parallelTests; i++) {
			Thread success = new Thread(() -> {

				for (int j = 0; j < 1000; j++) {
					authenticated.run();
				}
				successCounter.incrementAndGet();
				log.info("All passed");
			});
			success.setUncaughtExceptionHandler((thread, ex) -> {
				log.error("SECURITY ???", ex);
			});
			threads.add(success);

			Thread failure = new Thread(() -> {

				for (int j = 0; j < 1000; j++) {
					unauthenticated.run();
				}
				failureCounter.incrementAndGet();
				log.info("All passed");
			});
			failure.setUncaughtExceptionHandler((thread, ex) -> {
				log.error("SECURITY BYPASSED", ex);
			});

			threads.add(failure);
		}

		Collections.shuffle(threads);
		for (Thread thread : threads) {
			thread.start();
		}
		for (Thread thread : threads) {
			thread.join();
		}

		assertAll(() -> assertEquals(parallelTests, successCounter.get()),
				() -> assertEquals(parallelTests, failureCounter.get()));
	}

	@Override
	protected GreeterFutureStub beforeGreeting(GreeterFutureStub stub) {
		return stub.withCallCredentials(callCredentials);
	}

}

You will encounter the following errors in the test:

java.lang.NullPointerException: null // (Current authentication instance in authenticated call)
	at org.lognet.springboot.grpc.demo.SecuredGreeterService.sayAuthHello(SecuredGreeterService.java:22) ~[main/:na]
	at io.grpc.examples.SecuredGreeterGrpc$MethodHandlers.invoke(SecuredGreeterGrpc.java:209) ~[main/:na]
	at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:172) ~[grpc-stub-1.29.0.jar:1.29.0]
	at java.base/java.util.Optional.ifPresent(Optional.java:183) ~[na:na]
	at org.lognet.springboot.grpc.security.SecurityInterceptor$SecurityServerCallListener.onHalfClose(SecurityInterceptor.java:43) ~[main/:na]
	at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:331) ~[grpc-core-1.29.0.jar:1.29.0]
	at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:818) ~[grpc-core-1.29.0.jar:1.29.0]
	at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) ~[grpc-core-1.29.0.jar:1.29.0]
	at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123) ~[grpc-core-1.29.0.jar:1.29.0]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[na:na]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[na:na]
	at java.base/java.lang.Thread.run(Thread.java:834) ~[na:na]

This error is caused by another call that completed and thus the security being cleared before this call was processed.
However, it is also possible that if two authenticated calls were to happen at the same time to gain the authentication of the other call/user.

Note

Due to the structure of this library's interceptor you cannot (not sure) bypass authentication, but can still obtain a different users authentication (or loose your own as shown in the above stacktrace). Especially for streaming calls.

EDIT: You can also bypass security annotations (in non-streaming calls only) if the server uses them instead of the GrpcSecurityConfigurerAdapter (Not tested, but it's the same as the above error).

See also

• Support Spring Security @Secured annotation #61 (comment)
• Spring boot security #155 (comment)
• Add implementation note regarding server interceptors and thread locals grpc/grpc-java#7482

[jvmlet]

Thanks for your investigation. The implementation is not broken, but obtaining the current user in authenticated call might be, via SecurityContextHolder. It will be replaced with GRPC Context native API.

[ST-DDT]

Thanks for your investigation. The implementation is not broken, but obtaining the current user in authenticated call might be, via SecurityContextHolder. It will be replaced with GRPC Context native API.

It is broken. Once you try to implement it via GrpcContext (which is also based on a ThreadLocal) you will probably encounter errors because the context isn't closed at the appropriate places.

Please refer to the official repo for an example of contextual data inside an interceptor:

https://github.com/grpc/grpc-java/blob/0f7fd289a3555b95b32e9427413cecb0a5f009cf/api/src/main/java/io/grpc/Contexts.java#L44-L57

  public static <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
        Context context,
        ServerCall<ReqT, RespT> call,
        Metadata headers,
        ServerCallHandler<ReqT, RespT> next) {
    Context previous = context.attach();
    try {
      return new ContextualizedServerCallListener<>(
          next.startCall(call, headers),
          context);
    } finally {
      context.detach(previous);
    }
  }

https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/Contexts.java#L72-L80

    @Override
    public void onMessage(ReqT message) {
      Context previous = context.attach();
      try {
        super.onMessage(message);
      } finally {
        context.detach(previous);
      }
    }

As you can see the context is cleared after the method call and reattached and cleared for the nested calls.

[jvmlet]

This is what I meant - Authentication will be passed via managed context

[ST-DDT]

I'm not sure whether i got you right or whether you got me right.
What is important is not that it uses Spring's SecurityContextHolder or grpc's Context, but that it gets set at the start of startCall, onMessage, `onHalfClose, ... and cleared before existing each of them.

[markbanierink]

I'm not sure if I have configured it correctly, but now with the implementation resolving this issue, the security context is cleared after returning the listener.
When using spring security in our domain services (@PreAuthorize for instance), there is no authentication set anymore in the SecurityContextHolder, since these are called after the security context is cleared in the finally block of the SecurityInterceptor.

1. Is this deliberately implemented this way?
2. Should we implement our own GrpcInterceptor return a listener that sets the authentication in security context (again)?

[jvmlet]

SecurityContextHolder was never exposed to the end user to obtain currently logged in user. It's grpc security internal implementation. Please use GrpcSecurity.AUTHENTICATION_CONTEXT_KEY.get() to get the current user.
@PreAuthorize is not supported yet.

[markbanierink]

Thank you for the quick response.
So, in order to use spring security features further down in our domain services, we have to add an interceptor like this and set the security context ourselves:

public class AuthenticationInterceptor implements ServerInterceptor {
    
    @Override
    public <ReqT, RespT> Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> call, Metadata headers, ServerCallHandler<ReqT, RespT> next) {
        
        return new SimpleForwardingServerCallListener<ReqT>(next.startCall(call, headers)) {
            @Override
            public void onHalfClose() {
                try {
                    SecurityContextHolder.getContext().setAuthentication((Authentication)GrpcSecurity.AUTHENTICATION_CONTEXT_KEY.get());
                    super.onHalfClose();
                } finally {
                    SecurityContextHolder.clearContext();
                }
            }
        };
    }   
}

Just for me to understand, but why wasn't something like this implemented in the SecurityInterceptor?

[jvmlet]

SecurityContextHolder usesThreadLocal as repository for storing the Authentication object. While this approach might fit the servlet base MVC implementation where filters are executed on the same thread, grpc java doesn't guarantee this when executing interceptors and listeners. Instead it provides Contextualized listener if I recall properly that gets attached and detached before/after invoking the listener. It might be using the same thread local to store the context, but this is transparent for the user as long as he uses Context API and not ThreadLocal directly. BTW, here we can see how bad the static functions could be affecting the flexibility. I would prefer to see the accessor pattern for accessing the security context instead of static function you can't override/substitute.
Having said this, I felt that cleaning the security context right after authenticating the user would be the right thing to do to not compromise the executing thread.

Which security features you are talking about further down in your services?

[markbanierink]

We currently use @PreAuthorize and @PostFilter "further down" in our beans. These annotations use the SecurityContextHolder, hence our need to set the Authentication there. We preferably don't touch the SecurityContextHolder ourselves, spring security handles all this for us, setting the Authentication by the AuthenticationProvider. Methods annotated with these annotations perform our business logic and are called from the api-layer (gRPC, but possibly also REST, GraphQL, etc.), so it is inconvenient to secure our gRPC services, since we obviously prefer to do this in a single place at the domain level.
With regard to the interceptors possibly running in another thread, I agree cleaning the context after authentication is the right thing to do.
Maybe something like our implementation (probably extended for onMessage(), etc.) could be added to the default grpc security configuration, so spring security can be used by default?

[jvmlet]

Sure, I'll try to accomplish this once I'm back from vacation( ~ end of July)
as well as @PreAuthorize and @PostFilter integration.

[markbanierink]

If the Authentication is added to the SecurityContextHolder, then @PreAuthorize etc. automatically works through spring security.

[markbanierink]

I have created #233 as a possible implementation.