deephaven / deephaven-core

Deephaven Community Core
Other
255 stars 81 forks source link

Centralize logic for ensuring RPCs have an authenticated Session #5535

Open devinrsmith opened 4 months ago

devinrsmith commented 4 months ago

Right now it's the responsibility for every RPC implementation to verify they have an authenticated session by calling SessionService#getCurrentSession, even if they don't need the resulting SessionState. As part of #5433, it was noted that we may want to centralize this logic into io.deephaven.server.session.SessionServiceGrpcImpl.SessionServiceInterceptor.

Currently, the following RPCs do not require a Session:

Of note, GetHeapInfo seems to have been an oversight in implementation / security, and should probably require an authenticated Session.

It's worthwhile to note that we may not want to indiscriminately apply this requirement to all RPCs, as non-DH / non-Flight RPCs might be mixed into the server. As such, care should be taken to make sure that DH is not too heavy handed in applying this. We might want to have an dagger opt-in that we can apply to all of our services by default. It might also be nice to think about any self-documenting annotations (either Java or gRPC/protobuf annotation) we might be able to leverage for these purposes.

devinrsmith commented 4 months ago

io.deephaven.auth.ServiceAuthWiring.AuthorizingServerCallHandler may also be a good place to centralize this sort of check.

devinrsmith commented 4 months ago

https://github.com/devinrsmith/deephaven-core/tree/rpc-message-options-extension is a sketch of using a message option extension to annotate each RPC method like:

service SomeService {
    rpc SomeRpc(SomeRequest) returns (SomeResponse) {
        option (io.deephaven.proto.backplane.grpc.rpc_options).requires_authentication = true;
    }

it's convenient to have this as a self-documenting field. On the flip side, we still need to hook in for non-DH-owned RPCs like flight. It's also not obvious if we should explicitly document RPCs that aren't technically correlated with a Session; for example, we could imagine a standalone StorageService that gets routed differently for an org (for example, maybe it's read-only and no authentication needed).