Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Spring Security @Secured annotation #61

Closed
cbornet opened this issue Oct 13, 2017 · 19 comments
Closed

Support Spring Security @Secured annotation #61

cbornet opened this issue Oct 13, 2017 · 19 comments

Comments

@cbornet
Copy link

cbornet commented Oct 13, 2017

Annotating a server implementation method with @Secured annotation currently doesn't have any effect.
It would be nice if the @Secured annotation is supported and a Status.PERMISSION_DENIED error is sent if the Authentication doesn't contain the proper authority.

@Component
public class AuthenticationInterceptor implements ServerInterceptor {
    @Override
    public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> serverCall, Metadata metadata, ServerCallHandler<ReqT, RespT> serverCallHandler) {
        Authentication authentication = new UsernamePasswordAuthenticationToken("user", "user",
            Collections.singletonList(new SimpleGrantedAuthority(AuthoritiesConstants.USER));
        SecurityContextHolder.getContext().setAuthentication(authentication);
        return serverCallHandler.startCall(serverCall, metadata);  
    }
}
@GRpcService(interceptors = { LogInterceptor.class, AuthenticationInterceptor.class })
public class GreeterService extends GreeterGrpc.GreeterImplBase {
    @Override
    @Secured({ "ROLE_ADMIN" })
    public void sayHello(GreeterOuterClass.HelloRequest request, StreamObserver<GreeterOuterClass.HelloReply> responseObserver) {
        String message = "Hello " + request.getName();
        final GreeterOuterClass.HelloReply.Builder replyBuilder = GreeterOuterClass.HelloReply.newBuilder().setMessage(message);
        responseObserver.onNext(replyBuilder.build());
        responseObserver.onCompleted();
        log.info("Returning " +message);
    }
}

then stub.sayHello should fail with Status.PERMISSION_DENIED

@jorgheymans
Copy link
Contributor

probably same as #41 ?

@cbornet
Copy link
Author

cbornet commented Oct 13, 2017

Maybe related but there is also the need to translate the exception. An exception translator like Spring's ControllerAdvice would be awesome

@pagrus7
Copy link

pagrus7 commented Oct 24, 2017

Out of interest I've put together an example of Secured annotation (which just works) and a basic exception-to-status translation interceptor.

I don't think that would be sufficient to cover a meaningful scenario though. Just curious, where the authentication will be coming from in the intended use case?

@alexleigh
Copy link

I've also put together a standalone example of method-based security with gRPC (using pre-post-annotations instead of secured-annotations) with an exception-to-status translator, using Authorization metadata for Basic Auth credentials and JWT tokens. It works pretty well for our production use-case, though the default ThreadLocal security context storage is less than ideal.

@cbornet
Copy link
Author

cbornet commented Nov 10, 2017

I guess I should try again because spring.aop.proxy-target-class=true didn't work for me when I tried it ...

@alexleigh
Copy link

I've also had weird issues with the method security AOP interceptors not applying to the gRPC implementation class at times. In fact, in my standalone example, if I remove spring-jdbc from runtime dependencies, the AOP stops applying.

@pagrus7
Copy link

pagrus7 commented Nov 11, 2017

@alexleigh I think you are hitting another flavour of the autoproxying issue. By default spring boot 1.x would use a a dynamic proxies, but with spring-jdbc and spring-tx pulled into the classpath the autoconfiguration kicks in and configures cglib proxies instead.

I opened a PR which explicitly specifies proxyTargetClass = true on @EnableGlobalMethodSecurity annotation. Security then still works without spring-boot-starter-jdbc.

@alexleigh
Copy link

That's really good to know. Thanks for the help!

@ST-DDT
Copy link

ST-DDT commented Oct 18, 2018

See also grpc/grpc-java#4970


EDIT

WARNING

The security examples linked above have security vulnerabilities and are dangerous to use. See below for more details.

@cbornet
Copy link
Author

cbornet commented Oct 23, 2018

@ST-DDT I've seen your discussion on the grpc-java issue. Do you know how to proceed ?

@ST-DDT
Copy link

ST-DDT commented Oct 23, 2018

In order for SpringSecurity to work you need two things:

  1. ServerInterceptors (See the example linked above (Security issues))
    • With Exception mapping
    • With SecurityContext builder/Authentication
  2. A security annotated GrpcService impl
    • Which has either an interface with ALL methods (created and maintained by yourself, or somehow generated at compile time)
    • Or you use @EnableGlobalMethodSecurity(proxyTargetClass = true) and be okay with one line of warning per service class during each startup.

If you get an INTERNAL status response, then you have an issue with the first one.
If you get an UNAVAILABLE status response, then you have an issue with the second one.
If the user can pass through unauthenticated then you probably have to configure the annotation to enable your desired annotation.

As an alternative you could avoid the security annotation and use another server interceptor to map the access checks yourself. Using something like similar to a Map<MethodDescriptor, AccessDecider>.

PS: I might be able to contribute the server interceptors (if I get the permission).

@jvmlet
Copy link
Collaborator

jvmlet commented Oct 23, 2018

@cbornet , @ST-DDT , have you seen this ? He is using this starter and implemented interceptors to integrate spring security together with authorization.
I've asked the author if he want to contribute to this repo... still no answer.

@ST-DDT
Copy link

ST-DDT commented Oct 23, 2018

That example is somewhat better than the above mentioned, as it used an AuthenticationManager. But it also has a few drawbacks:

  • It hasn't been contributed to this project and is no standalone library (-> copy & paste)
  • It uses a different license (-> so you have to actually add the license statements to those classes/wrapper library (not that big of a problem though))
  • Analyses the cause stack which you might not want, especially if you have other ExceptionMappers
  • Lacks in code documentation/javadocs
  • EDIT: Both have security issues

Don't get me wrong, it is good code, but puts some obstacle in your way to use and maintain it.

EDIT: And its dangerous to use.

@ST-DDT
Copy link

ST-DDT commented Nov 12, 2018

WARNING

The security demo by revinate and the security example by pagrus7 are vulnerable to concurrency issues.

You might see the following issues:

  • Unauthenticated users can execute calls
  • Authenticated requests get rejected due to UNAUTHENTICATED
  • Authenticated requests might show a different username

During my tests this issues only occurred in very few cases (aka hard to reproduce) .
It's easily reproducible by executing at least two calls (with different auth status) simultaneously .
It's easier to reproduce the bug with (many) concurrent calls by different users (including unauthenticated ones) and maybe a busy CPU (to force the thread context switches).

You can avoid that issue by rewriting the authenticating interceptors to work similar to grpc-contexts.
Or have a look at my PR (https://github.com/yidongnan/grpc-spring-boot-starter/pull/126) which tries to fix that issue for a different grpc-spring-boot library.

See also this SO question/answer


That issue is not related to this repository. It's only related to the mentioned security demo and all variants that work in a similar way.

ST-DDT referenced this issue in pagrus7/grpc-spring-boot-starter Nov 12, 2018
@alex-lzl
Copy link

WARNING: The security demo by revinate and the security example by pagrus7 are vulnerable to concurrency issues.

THANK YOU SO MUCH!!! I just ran into the concurrency issue mentioned above and super lucky found your post. Also just in time for the release of net.devh:grpc-spring-boot-starter:2.2.0.RELEASE.

@majusko
Copy link

majusko commented Nov 11, 2019

@cbornet , @ST-DDT , @jvmlet

Probably not interested anymore, but if so:

I created a simple JWT Spring Boot Starter extending this library from LogNet: https://github.com/majusko/grpc-jwt-spring-boot-starter

Already using it in few production projects so it will be definitely supported. Also, feel free to contribute ;)

Simple usage showcase here:
https://github.com/majusko/grpc-example/blob/master/src/main/kotlin/io/github/majusko/grpc/example/ServerExample.kt

@jvmlet
Copy link
Collaborator

jvmlet commented Nov 11, 2019

Thanks for sharing, @majusko. I'll definitely have a look

@balchua
Copy link

balchua commented Apr 28, 2020

Is there some plan on supporting spring security? Thanks.

@jvmlet
Copy link
Collaborator

jvmlet commented Sep 17, 2020

It's better later than never :-), implemented in 4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants