grpc-spring icon indicating copy to clipboard operation
grpc-spring copied to clipboard

Configure AccessPredicate based on Custom method level annotation

Open chipim opened this issue 3 years ago • 11 comments

The context

I wish to use a custom Annotation to tag gRPC service methods to enable custom authentication logic on them, specifically to use a specific AccessPredicate for that given method if the annotation is present. Example of desired implementation, where @CustomPermitAll would be the custom annotation:

@GrpcService
class SomeService : SomeServiceCoroutineImplBase() {

        @CustomPermitAll
        override suspend fun someMethod(request: Empty) {...}
}

The question

Does the library provide some way to implement these kind of custom method level security on RPCs? Or is there any component the library provides to safely do the scan of the GrpcServices to identify the annotated methods when setting up the GrpcSecurityMetadataSource bean? For the moment I've tried using the Spring context and getting the GrpcService annotated beans and then going trough the methods using Java reflection by my own implementation, but wanted to know if there is a better way anyone knows to achieve what I want.

Sorry if it is not explained that well, please ask me for more info if needed. Any help will be greatly appreciated!

chipim avatar Aug 22 '22 09:08 chipim

Currently there is no embedded/known library way for exactly that.

Could you share your intention behind this feature request? Could you share your source code for the annotation scanner? Maybe someone else might want to do something similar or has a suggestion to do this more easily.

Is there a reason why you don't use spring securities @Secured (or similar) annotations instead?

ST-DDT avatar Aug 22 '22 14:08 ST-DDT

Hey, thanks for the answer!

  1. The intention is to specify a default AccessPredicate for all rpcs to be fully authenticated like so:
@Bean
fun grpcSecurityMetadataSource(): GrpcSecurityMetadataSource {
    val source = ManualGrpcSecurityMetadataSource()
    return source.setDefault(AccessPredicate.fullyAuthenticated())
}

But then override that for individual rpcs method implementations using that @CustomPermitAll annotation, to specify another AccessPredicate for those specifically.

  1. The code goes something like this:
val grpcServices = applicationContext.getBeansWithAnnotation(GrpcService::class.java)
    .mapNotNull { (_, bean) ->
        bean as? BindableService
    }

val customMethodDefinitions = grpcServices.map { service ->
    service.methodDefinitions.filter { methodDefinition ->
        service.javaClass.declaredMethods.find { method ->
                method -> method.name.equals(methodDefinition.methodDescriptor.bareMethodName, true)
        }?.let {
            AnnotatedElementUtils.hasAnnotation(it, CustomPermitAll::class.java)
        } ?: false
    }
}

This being executed in the GrpcSecurityMetadataSource bean definition method, using an injected Spring ApplicationContext.

  1. The reason I am not using the Spring Security method level annotations is because I couldn't make them work in conjunction with the SecurityMetadataSource configuration and the AuthenticationReader I've configured from this library (the Authentication field in the SecurityContext is always empty when the AbstractSecurityInterceptor evaluates the @PreAuthorize("permitAll()") method leven annotation), but also I lack understanding on how Spring security works so any help there is also welcomed.

Anyways any help you can give me, either with the approach I am currently using or shedding some light on how I should use the method level annotations for what I want to do, I will greatly appreciate! Sorry if the explanation is a little lacklustre, please ask me for more specific info if that helps.

chipim avatar Aug 24 '22 08:08 chipim

  1. Looks good to me
  2. Also looks good to me
  3. This might be a spring-security kotlin bug.

I searched a bit and found this issue: https://github.com/spring-projects/spring-security/issues/5774 According to that you have to un-final/open(?) your implementations explicitly. Have you tried that yet? Could you please give me some feedback on this, then I might be able to write a more explicit warning in the docs.

You might also want to double check this documentation: https://yidongnan.github.io/grpc-spring-boot-starter/en/server/security.html#spring-annotation-security-checks

Here is one of our tests for spring-security, grpc and annotations: https://github.com/yidongnan/grpc-spring-boot-starter/blob/master/tests/src/test/java/net/devh/boot/grpc/test/security/AnnotatedSecurityWithBasicAuthTest.java

Just for context, the authentication step (AuthenticationReader and AuthenticationProvider/Manager) is separate from the authorization step, so if either of the two work, then both should. Here is a graphic showing it in more detail: https://yidongnan.github.io/grpc-spring-boot-starter/en/server/security.html#authentication-and-authorization

If you have any suggestions on how we could improve the documentation, please let us know.

I think your approach to the error might be worth exploring/adding to this repository (read spring's annotations and use that to populate the ManualGrpcSecurityMetadataSource). We should/will use spring's security annotation parser (or whatever it is called) though.

ST-DDT avatar Aug 24 '22 13:08 ST-DDT

Thanks so much for the help and the different links provided!

About the first point, we did set our service implementations with the open qualifier explicitly; without it it couldn't even load the ApplicationContext, so maybe that was an outcome of the https://github.com/spring-projects/spring-security/issues/5774?

And regarding the other things, if I understand correctly, then the idea is to either use the GrpcSecurityMetadataSource OR the Spring security method level annotations; and in the tests you referenced me you are only using the latter, right?

What I see happening in my case is that by creating the Bean ManualGrpcSecurityMetadataSource then the AuthorizationCheckingServerInterceptor is the first one to invoke the AbstractSecurityInterceptor.beforeInvocation
method from the superclass and in that execution the SecurityContext has the Authentication that the DefaultAuthenticatingServerInterceptor had set.

Then after that, there is a second invocation of AbstractSecurityInterceptor.beforeInvocation by a MethodSecurityInterceptor instance (this would be part of the SpringSecurityProxy part I assume) but then the SecurityContext is empty. I've tried not creating the ManualGrpcSecurityMetadataSource Bean so that the AuthorizationCheckingServerInterceptor isn't configured and the only one checking the Authorization is the SpringSecurityProxy like in your tests but even then the SecurityContext's Authentication is empty 🤔.

But anyways, regarding the final point you mention: to manually populate the ManualGrpcSecurityMetadataSource with the spring's annotations, you mean not configuring the @EnableGlobalMethodSecurity and just leaving the AuthorizationCheckingServerInterceptor as the only "authorizer"? I will check if I can find the Spring's security annotation parser logic to see if I can manually populate GrpcSecurityMetadataSource for my case at least, but I don't know if with my poor understanding of this all I can get there 🙈

But again any clarifications to my misunderstandings or other suggestions on the best way to handle what I want to do with existing tools is ultra welcomed 🙇‍♂️

chipim avatar Aug 25 '22 10:08 chipim

either use the GrpcSecurityMetadataSource OR the Spring security method level annotations

There is no requirement for that, but IMO it doesn't make much sense to use two authorization checks.

Then after that, there is a second invocation of AbstractSecurityInterceptor.beforeInvocation by a MethodSecurityInterceptor instance (this would be part of the SpringSecurityProxy part I assume) but then the SecurityContext is empty. I've tried not creating the ManualGrpcSecurityMetadataSource Bean so that the AuthorizationCheckingServerInterceptor isn't configured and the only one checking the Authorization is the SpringSecurityProxy like in your tests but even then the SecurityContext's Authentication is empty 🤔.

Could you please check whether this is still the same thread as the one in the ManualGrpcSecurityMetadataSource. Please also check the GrpcContext.

https://github.com/yidongnan/grpc-spring-boot-starter/blob/75a97fb8dccdf67ac3d5309351099d3018f53208/grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java#L119-L121

you mean not configuring the @EnableGlobalMethodSecurity and just leaving the AuthorizationCheckingServerInterceptor as the only "authorizer"?

Yes, at least for the grpc route (not sure if you have others).

But again any clarifications to my misunderstandings or other suggestions on the best way to handle what I want to do with existing tools is ultra welcomed 🙇‍♂️

Not sure whether these are the correct ones:

Before you proceed with a custom re-implementation, try to understand why the security context is no longer available at that time. Is this potentially related to something like kotlin coroutines, that perform a thread switch somewhere it between? Is this related? https://github.com/yidongnan/grpc-spring-boot-starter/issues/628#issuecomment-1021911034

ST-DDT avatar Aug 25 '22 13:08 ST-DDT

Please note that the internal behavior/implementation of the ManualGrpcSecurityMetadataSource will probably change in the next or a future version. (e.g. currently it only takes the Authentication instance into consideration, but in the future it will also take the call context into considerations thus allowing ip allow/deny-listing). It shouldn't have much impact on your usecase, you might have to update your implementation/the used interfaces a bit though.

Ref: https://github.com/yidongnan/grpc-spring-boot-starter/issues/682#issuecomment-1146471401

ST-DDT avatar Aug 25 '22 13:08 ST-DDT

Like you suggested, it seems they are two different threads for each AbstractSecurityInterceptor(first one being the implementation from this library and the second one the Spring Security Proxy) but after implementing the https://github.com/yidongnan/grpc-spring-boot-starter/issues/628#issuecomment-1021911034 CoroutineContextServerInterceptor it seems that the second coroutine gets access to the "authenticated" SecurityContext successfully.

However I get a bit lost in regards to the GrpcContext part. Before even implementing the CoroutineContextServerInterceptor I tried debugging to peep the Context set in the DefaultAuthenticatingServerInterceptor like you mentioned and I saw that (despite seeing the https://github.com/yidongnan/grpc-spring-boot-starter/blob/75a97fb8dccdf67ac3d5309351099d3018f53208/grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java#L136 sentence being invoked ) I see that the same Context still lives in both threads as a ThreadLocal entry and it has both the Authentication and SecurityContext set in the DefaultAuthenticatingServerInterceptor. Now I don't fully understand why or who is making that happen, but it is something that shouldn't happen, right? (this behaviour was even before implementing the CoroutineContextServerInterceptor )

First execution of the beforeInvocation method by DefaultAuthenticatingServerInterceptor image

Second execution of the beforeInvocation method by MethodSecurityInterceptor(Spring Security Proxy)_ image

Anyhow the https://github.com/yidongnan/grpc-spring-boot-starter/issues/682#issuecomment-1146471401 works for making the SecurityContext accesible in both threads, but I don't understand why if the SecurityContext being different is caused by ThreadLocal the same doesn't happen for the GrpcContext that also uses ThreadLocal. Do you know if I might be understanding or seeing something wrong?

Finally I will give the Spring security component references a look a tomorrow to see if I can use them for the alternative implementation like we talked about.

As always thanks so much for your assistance and patience! 🙇‍♂️

chipim avatar Aug 25 '22 17:08 chipim

Do you know if I might be understanding or seeing something wrong?

I assume that the grpc-kotlin library itself knows only about the GrpcContext and thus forwards it to the correct thread. However the library does not know about Spring's ThreadLocal and thus you have to pass it along manually.

Could you please write a short documentation PR with some kind of guide for kotlin so others can find the solution more easily?

ST-DDT avatar Aug 25 '22 18:08 ST-DDT

Hey @ST-DDT! Do you mean writing about using the CoroutineContextServerInterceptor approach when implementing the Spring Method Security? Or something other than that?

If so I can surely try 🙈

chipim avatar Aug 29 '22 08:08 chipim

That would be very nice. Thank you.

ST-DDT avatar Aug 29 '22 10:08 ST-DDT

FYI: I created a PR that grants the developer access to the ServerCall during authorization checks (e.g. the client address): https://github.com/yidongnan/grpc-spring-boot-starter/pull/742 This PR can be considered BREAKING if you use our grpc authorization classes for more than just configuration. If you just use it for simple configuration, then you don't have to change anything.

ST-DDT avatar Sep 13 '22 23:09 ST-DDT