grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.52k stars 826 forks source link

AuthenticationContext not accessible outside of authenticated scope/early interceptors #462

Open SledgeHammer01 opened 3 years ago

SledgeHammer01 commented 3 years ago

My app is working with dynamic OAuth and all is good. I am now implementing request / response logging. One thing I need to log comes from the SecurityContext. This also works fine... in most cases.

I am using the standard Spring "throw new ResponseStatusException(...);" to return stuff like 404, etc. from rest methods. That also works fine and I wrote a server interceptor to catch those and translate to grpc errors. All good.

The bug / oversight is that in DefaultAuthenticatingServerInterceptor, if you encounter an exception, in AuthenticatingServerCallListener::detachAuthenticationContext you clear the SecurityContext before anything in my server interceptor can get it. It gets cleared within the context of the call it seems. And its not set until the call is ready to happen. With no ResponseStatusException, the SecurityContext is live when it gets to my server interceptor.

Doesn't seem to be a way to do anything about that without c&p'ing the entire DefaultAuthenticatingServerInterceptor and changing that.

ST-DDT commented 3 years ago

I'm not sure if I get you correctly, but I have to clear the security context before "exiting" the methods in the interceptor, because otherwise I'm leaking the security context and another thread call is able to pickup the still valid context and bypass security. If you tinker with it, make sure that the ConcurrentSecurityTest still works.

I assume the interceptors are ordered somewhat like this: Your LoggingInterceptor -> ExceptionTranslatingServerInterceptor -> DefaultAuthenticatingServerInterceptor. For that reason you unfortunately don't have access to the security context in your LoggingInterceptor.

Could you please explain/show what you wish to change (code), so that I understand your intentions better?

SledgeHammer01 commented 3 years ago

Sorry :), I didn't mean to change code here without getting your feedback, I meant c&p'ing in my project, but it might be an issue for others :).

In my config, I am set up as follows:

    @Bean
    public DefaultAuthenticatingServerInterceptor authenticatingServerInterceptor(GrpcAuthenticationReader authenticationReader,
                                                                                  DefaultTokenServices tokenServices) {
        GrpcOAuth2AuthenticationManager authenticationManager = new GrpcOAuth2AuthenticationManager();
        authenticationManager.setTokenServices(tokenServices);
        return new DefaultAuthenticatingServerInterceptor(authenticationManager, authenticationReader);
    }

    @Bean
    public GrpcAuthenticationReader grpcAuthenticationReader() {
        return new BearerAuthenticationReader(token -> new PreAuthenticatedAuthenticationToken(token, ""));
    }

    @GrpcGlobalServerInterceptor
    public ServerInterceptor xxxServerInterceptor() {
        return new xxxServerInterceptor();
    }

This actually handles getting the oauth token as needed using the Spring Boot infrastructure rather then hard coding one. I ended up having to modify the Spring OAuth2AuthenticationManager -> GrpcOAuth2AuthenticationManager slightly because the principal details from the token were getting overwritten by yours, so I modified that class slightly to merge them instead. That part is fine too.

A grpc controller might look like this (standard Spring):

MyDto dto = this.service.getDto();

if (dto == null)
  throw new ResponseStatusException(HttpStatus.NOT_FOUND);

xxxServerInterceptor inherits from ExceptionTranslatingServerInterceptor. It serves 2 purposes:

  1. translate ResponseStatusException to grpc status, so something like:
        private void handleResponseStatusException(final ServerCall<ReqT, RespT> call,
                                                   final ResponseStatusException e) {
            Status status = Status.UNKNOWN;
        System.out.println("1 -> cont: " + SecurityContextHelper.getCurrentUserId());
            switch (e.getStatus()) {
                case NOT_FOUND: {
                    status = Status.NOT_FOUND;
                    break;
                }

                default: {
                    this.logger.error("Unhandled ResponseStatusException: {}", e.getStatus());
                    break;
                }
            }

            call.close(status.withCause(e).withDescription(e.getLocalizedMessage()), new Metadata());
        }

The issue I opened for is that by the time the code gets to the println, the SecurityContext has been cleared already. The 2nd intention of the interceptor is to log request / response. So I'm trying to get the user info from the SecurityContext. If I don't throw the ResponseStatusException, the SecurityContext is still valid in my interceptor.

In the case where I am throwing a 404, the DefaultAuthenticatingServerInterceptor has already cleared the context. I've tried putting my println in:

handleResponseStatusException
onMessage
onHalfClose
onCancel
onComplete
close

Basically everywhere :).

In the case of a ResponseStatusException, the SecurityContext is cleared in every single place I tried. In the case of NO ResponseStatusException, its still valid in the close() method where I'm doing the logging.

The work-around I'm trying now is to create a GrpcResponseStatusException that captures the SecurityContext info during the throw while its still valid... but kind of ugly.

What I was talking about a change in DefaultAuthenticatingServerInterceptor which you can advise on is to maybe detach the security context later, etc. Not sure on that one...

ST-DDT commented 3 years ago

I will look into it soon.

As an alternative to the GrpcResponseStatusException, you could also add another interceptor after the authenticating one and write the username to a ThreadLocal/the Call instance (WeakReferenceMap?).

SledgeHammer01 commented 3 years ago

I will look into it soon.

As an alternative to the GrpcResponseStatusException, you could also add another interceptor after the authenticating one and write the username to a ThreadLocal/the Call instance (WeakReferenceMap?).

I tried some further things. Like extending the DefaultAuthenticatingServerInterceptor and actually the "issue" is in the interceptCall, at the end you have a:

        try {
            return new AuthenticatingServerCallListener<>(next.startCall(call, headers), context, authentication);
        } catch (final AccessDeniedException e) {
            if (authentication instanceof AnonymousAuthenticationToken) {
                throw new BadCredentialsException("No credentials found in the request", e);
            } else {
                throw e;
            }
        } finally {
            SecurityContextHolder.clearContext();
            context.detach(previousContext);
            log.debug("startCall - Authentication cleared");
        }

It's getting cleared in that finally block and there wouldn't be any way to catch that.

mr-w1lde commented 3 years ago

Same Problem with null authentication in SecurityContextHolder v2.10.1.RELEASE

ST-DDT commented 3 years ago

Same Problem with null authentication in SecurityContextHolder v2.10.1.RELEASE

Can please provide a little bit more context? Is there a stacktrace or something?

Does

https://github.com/yidongnan/grpc-spring-boot-starter/blob/2ee90f16ee4295370ee47c40829805ff1a9f4f51/grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java#L103

return null?

mr-w1lde commented 3 years ago

grpc-spring-boot-starter/grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java

I want to get Authentication from SecurityContextHolder.getContext(), but it is return null. Screenshot 2021-01-17 at 20 44 33

ST-DDT commented 3 years ago

Inside your "controller"/grpc service method or inside an interceptor? Were there authentication details attached to the request? What is your usecase? Please do not leave me starved for information.

As for accessing the authentication outside the authentication's request I haven't found a good way to do this yet. As a workaround you can do the following:

@Order(Early) // See also InterceptorOrder
public class PreAuthenticationContextAppender implements ServerInterceptor {

    interceptCall(ServerCall call, X x) {
        Context context = Context.get().with(Key.AUTHENTICATION_GHOST, new AtomicReference<Authentication>());
        Context old = context.attach() {
        try {
            return x.startCall();
        } finally {
            context.detach(old);
        }
    }

}

// The authentication takes place here....

@Order(Late)
public class PostAuthenticationContextAppender implements ServerInterceptor {

    interceptCall(ServerCall call, X x) {
        Authenticationcontext = Key.AUTHENTICATION_GHOST.get().set(SecurityContextHolder.get());
        return x.startCall();
    }

}

Then you can access the variable from a broader variety of interceptors. Just make sure you don't leak the authentication context.

If you have a feasible implementation, then we could try to integrate it into this library.

ST-DDT commented 3 years ago

Actually, I might have found a far easier way (Not tested):

@Order(Early) // See also InterceptorOrder
public class PreAuthenticationContextSniffer implements ServerInterceptor {

    interceptCall(ServerCall call, X x) {
        SecurityContext context = SecurityContextHolder.getContext();
        try {
            return x.startCall();
        } finally {
            sysout(context.getAuthentication());
        }
    }

}
ST-DDT commented 3 years ago

Slightly off-topic, but related (and a reminder for myself): With that in mind, it might be better (for the user) to share the SecurityContext within the request instead of the Authentication. That way a user could theoretically assign custom data to the SecurityContext and access that from everywhere inside the request:

((CustomSecurityContext) SecurityContextHolder.getContext()).getUserDataLookupCache()

That way, if you change the Authentication of the request, then it applies to all parts of the request processing instead of just the part where you assigned it.

If you don't use it, it doesn't change anything for the user. Related question on SO: https://stackoverflow.com/questions/65765196/restore-auth-using-securitycontext-or-authentication-instance-in-multi-threaded

goto1134 commented 3 years ago

If you use kotlin grpc with spring security you should create ThreadContextElement (source) like the one below:

    class SecurityCoroutineContext(
        private val securityContext: SecurityContext = SecurityContextHolder.getContext()
    ) : ThreadContextElement<SecurityContext?> {

        companion object Key : CoroutineContext.Key<SecurityCoroutineContext>

        override val key: CoroutineContext.Key<SecurityCoroutineContext> get() = Key

        override fun updateThreadContext(context: CoroutineContext): SecurityContext? {
            val previousSecurityContext = SecurityContextHolder.getContext()
            SecurityContextHolder.setContext(securityContext)
            return previousSecurityContext.takeIf { it.authentication != null }
        }

        override fun restoreThreadContext(context: CoroutineContext, oldState: SecurityContext?) {
            if (oldState == null) {
                SecurityContextHolder.clearContext()
            } else {
                SecurityContextHolder.setContext(oldState)
            }
        }
    }

and implement an interceptor like the one below to propagate SecurityContext:

  @GrpcGlobalServerInterceptor
    fun securityContextCoroutineContextServerInterceptor(): ServerInterceptor {
        return object : CoroutineContextServerInterceptor() {
            override fun coroutineContext(call: ServerCall<*, *>, headers: Metadata): CoroutineContext {
                return Dispatchers.Default + SecurityCoroutineContext()
            }
        }
    }