Issues with SpringSecurity - AOP final issue vs missing interface proxying

[ST-DDT]

What version of gRPC are you using?

1.15.0

---

I would like to use SpringSecurity to secure my grpc service.

public class TestService extends TestServiceGrpc.TestServiceImplBase {

    @Override
    @Secured("ROLE_USER")
    public void getVersion(Empty request, StreamObserver<Version> responseObserver) {
        responseObserver.onNext(Version.newBuilder().setVersion("1337.42").build());
        responseObserver.onCompleted();
    }

}

(The authentication and exception handling is done via interceptors.)

However if I do that I get one of two issues.

1. First, the default way of Spring to apply the value is using an interface based proxy. This will cause

   io.grpc.StatusRuntimeException: UNIMPLEMENTED: Method not found: TestService/getVersion

   errors because the actual methods aren't present in any interface.

2. I can avoid that error by using an AOP based proxy, but if I do that, then I get a warning in the logs:

   WARN  CglibAopProxy - Unable to proxy interface-implementing method [public final io.grpc.ServerServiceDefinition TestServiceGrpc$TestServiceImplBase.bindService()] because it is marked as final: Consider using interface-based JDK proxies instead!

The error was introduced by: #2552 / #2553

Possible solutions:

1. Manually create an interface for that 😒 .
2. Remove final modifier from that method 😐 .
3. Let grpc-java create an interface for that 👍 . (Used as a proxy or to be implemented via ImplBase)
4. Switch to java 8 (or later) and convert the ImplBase to an interface (combines 2+3) 👍 😄 .

---

PS: I used Spring-Security as an example, but the same issues apply for any kind of annotation based runtime code injection. Such as method call parameter logging, timing/metrics...

[ejona86]

The final modifier is there to allow grpc to make modifications to the method. Also, it prevents mocking frameworks from overriding the method. Removing the final would probably break users tests. It also seems like it may break your use-case as AOP may not override the method correctly. So no to (2).

Since we still support Java 7, default methods on interfaces are not an option. gRPC requires that adding a new method to a service's .proto file be backward-compatible, so we can't make any interface containing grpc methods. That kills (3).

ImplBase must continue being an abstract class, even on Java 8, as otherwise it would break existing users (they have extends in their code and it would need to change to implements). So no to (4).

It actually sounds like everything works correctly, except for a useless warning. So maybe there could be a way to tell CglibAopProxy that you don't care about the final method.

[ST-DDT]

The final modifier is there to allow grpc to make modifications to the method. Also, it prevents mocking frameworks from overriding the method. Removing the final would probably break users tests. It also seems like it may break your use-case as AOP may not override the method correctly. So no to (2).

AFAIK it would change it forward to the original method, so I assume it would still work. However, I still wouldn't go this way, if I have an alternative.

Since we still support Java 7, default methods on interfaces are not an option.

I don't want default methods so there won't be a conflict with Java 7.

gRPC requires that adding a new method to a service's .proto file be backward-compatible, so we can't make any interface containing grpc methods.

The interface will be generated alongside the other the other classes, maybe even as a nested class, similar to the ImplBase class. That way they will always be compatible to each other. I explicitly don't want a standalone interface that I can use independently from the other classes.

Example:

public final class TestServiceGrpc {
    [...]
    public static final class TestServiceBlockingStub { [...] }
    [...]
    public static final class TestServiceFutureStub { [...] }
    [...]
    /**
     * This interface can be used to reference the service or create wrappers.
     * Actual service implementations should be implemented using the TestServiceImplBase.
     * Wrappers should always delegate the bindService() method to the underlying service unmodified.
     */
    public interface TestServiceImplFace extends BindableService {
         void getVersion(Empty request, StreamObserver<Version> responseObserver);
    }
    [...]
    // public static abstract class TestServiceImplBase implements BindableService { // current
    public static abstract class TestServiceImplBase implements TestServiceImplFace { [...] }
    [...]
    public static final class TestServiceStub { [...] }

}

ImplBase must continue being an abstract class, even on Java 8, as otherwise it would break existing users (they have extends in their code and it would need to change to implements). So no to (4).

I agree with that. The interface could be used later to start the migration to an abstract class less variant (if we drop Java 7 support and want to actually drop the class). Not saying we should, but it would be an option.

It actually sounds like everything works correctly, except for a useless warning. So maybe there could be a way to tell CglibAopProxy that you don't care about the final method.

I will check for that but I don't make high hopes.

[ejona86]

The final modifier is there to allow grpc to make modifications to the method. Also, it prevents mocking frameworks from overriding the method. Removing the final would probably break users tests. It also seems like it may break your use-case as AOP may not override the method correctly. So no to (2).

AFAIK it would change it forward to the original method, so I assume it would still work. However, I still wouldn't go this way, if I have an alternative.

It depends on how it is implemented. If it extends your class, then it would be fine. If it delegates to your class (as a separate instance), then you would lose the AOP changes since gRPC would call your class directly. (The wrong this would be used.) With it being final, it doesn't matter whether it extends or delegates; both work correctly.

Since we still support Java 7, default methods on interfaces are not an option.

I don't want default methods so there won't be a conflict with Java 7.

gRPC can't have any interface that changes in a backward incompatible way. Yes, you would be fine if it changes, but other users could implement it directly and break when a method is added. Default methods is the only way to get "interface" + "adding methods doesn't break."

[carl-mastrangelo]

@ejona86 I have a way that there could be an interface (though it's ugly), and more of a gimmick:

package mypackage.proto;

public final class MyService {

  public interface BlockingMyService {
    MyResponse myMethod(MyRequest req);

    void dontImplementMe(Private1 p);

    void dontImplementMe(Private2 p);
  }

  private interface Private1 {}
  private interface Private2 {}

  public static abstract class MyServiceBlockingImpl 
      extends AbstractStub implements BlockingMyService {

    @Override
    public final void dontImplementMe(Private1 p) {
      throw new AssertionError();
    }

    @Override
    public final void dontImplementMe(Private2 p) {
      throw new AssertionError();
    }
    // ...
  }
}

[ejona86]

Oh, yes, that Go-"I want an abstract class" gimmick taken to the next level that you figured out. That also protects against the "this looks like a functional interface" in Java 8.

That gives us an interface that is useful only for proxy-based solutions, and in the eventual Java 8 world we could remove the Private1/2 and swap to default methods.

But... do we want to do it? We would need to add a static bindService method for it, but that's no big deal.

[carl-mastrangelo]

Do we want it? Personally, I am slightly leaning towards no.

Pros:

• Enables reflect (and other) proxies

Cons:

• hard to understand
• increases number of classes

If you care about that use, we could poke java libraries team and see what they think. (I suspect we will be reprimanded.)

[ST-DDT]

Before we start with private parameter types:
Wouldn't this strictly limit the usage to jdk proxies? Any kind of wrapper that would rely on generated classes would still fail because the of the private types. Maybe we could make the interface itself private so that it isnt visible in the first place. Tools will still see it probably.

But i still dont understand why it is neccesary in the first place.
Having an interface does not affect client or server compatibility. If it does please tell me how.
If it does not affect the interaction between the two it could affect the update process. Lets check the update process/DEV. The client side shouldnt have an implementation of the server interface in the first place so no issues here. As for the server there would only an issue if you manually implemented the interface (which you shouldnt do in the first place). There you would get a compiler error and the issue would be visible.

As for ABI/API incompatibilities there can only be some if the interface is implemented directly. And then you get a compuler error. The only way that you dont get a compiler error is when you have an separate lib that has a different API version than the main server ones. (Server depends on APIv2 and ServiceImpl. ServiceImpl depends on APIv1.) Server will be compiled last so it wont show any CTEs but will explode at RT due to ABI incompatibility. However I'm sure that i can cause ABI incompatibilities in that case even without having a separate interface. So that reason does not count.

Whether you expose an abstract base class or an interface does not make any difference IMO and if its a nested class then they cannot expose it as a standalone thing. So there shouldnt be any trouble here either.

TLDR: in which case would there be an issue with having a public interface? Could you give me an example?

[carl-mastrangelo]

Wouldn't this strictly limit the usage to jdk proxies?

yes

But i still dont understand why it is neccesary in the first place. Having an interface does not affect client or server compatibility.

Unfortunately it does. When a proto file gets a new method, the generated code is updated to have the new method present. Any application code that implemented the service interface would no longer compile. This has a very negative effect of service owners never evolving their service definitions because it is so painfully hard to update every user of the API.

This is why Java8 default methods are relevant. If new methods can be added to an interface without breaking backwards compatibility, then they would be usable for Protobuf generated methods.

Server will be compiled last so it wont show any CTEs but will explode at RT due to ABI incompatibility.

It is frequently the case that the owner of the Service definition (e.g. the .proto) cannot upgrade all users of it. They may not have permission to do so, or the code may have already been compiled and distributed. Even if they can go upgrade all the users, it has to be done atomically. The proto would have to be updated at the same time every instantiation site is updated. That would result in either a very large change (which may not be feasible to push through), or a change to multiple unsyncrhonized repos. Neither is appealing.

For posterity, we (Google) did have interfaces on the previous internal iteration of gRPC. It was a pain point for years (decades?) and we ripped it out with gRPC. Upgradability of protos is a core feature, while AOP is niche. That's why we are talking about niche work-arounds.

[ST-DDT]

Thanks for your detailed answer.

But i still dont understand why it is neccesary in the first place. Having an interface does not affect client or server compatibility.

Unfortunately it does. When a proto file gets a new method, the generated code is updated to have the new method present. Any application code that implemented the service interface would no longer compile. This has a very negative effect of service owners never evolving their service definitions because it is so painfully hard to update every user of the API.

The server interface shadows the exact implementation of the server. The client would still use the client stubs which are independent from said interface. So why would you need to update the client then? Please note that @carl-mastrangelo example interface is for a client, which I dont need at all.

[carl-mastrangelo]

The server interface shadows the exact implementation of the server.

Alas, no again. Here's an example: You publish you proto for use, but don't include a mock server to use for testing. Another user implements your server interface and mocks out some responses for their unit tests. When you modify your service to support another method, the user who implemented your interface now has broken unit tests. There are more service implementations than service definitions, and not all of them are owned by the service owner.

Again, I point to examples where this is an issue. gRPC Go does expose an interface, and that team is actively trying to back out of the generated interface API.

[ST-DDT]

Well I would like to say its their own fault for not reading the javadocs, but i can understand your point of making it impossible for them to do it wrong.

Last Question: Is there a way to get the implementing method (or the annotations) from a MethodDescriptor?
Probably only with some manual reflection.

Otherwise i have to rely on a manual mapping of grpc method and security roles.

• Or generate an interface myself (maybe some kind of annotation processor or protoc plugin).
• Or ask the server team to ignore these warnings (not gonna happen... too much work: clean the logs+monitoring, document that somewhere for every service).

The private interface stuff would probably only lead to more confusion than benefit so we better skip that.

[carl-mastrangelo]

@ST-DDT We provide Annotations on the method descriptors, which are suitable for use in annotation processors. It is possible to use these to generate your own stubs programmatically, and without using reflection, and without generating files. This method is used internally for Dagger Modules, if that gives any confidence to the use.

[ST-DDT]

Thanks for your help.