netty: Per-rpc call option authority verification against peer cert subject names #11724
Conversation
|
I don't know what this error is about: java/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java:621:18: 'public' modifier out of order with the JLS suggestions. [ModifierOrder] |
|
JLS == Java Language Specification The relevant part of the style guide: |
I have done this change in the source files. Can we bypass the animal sniffer check for the tests, because in ProtocolNegotiatorsTest I'm mocking X509ExtendedTrustManager and I can't do this with reflecttion because java.lang.Class cannot be mocked. |
Yeah, that's not a problem. We don't run the netty/okhttp unit tests on Android. Only in interop-testing/src/main (it's tests don't run on Android either) do you need to be more careful, like with AbstractInteropTest. |
How to disable animal sniffer for tests? I see there is an excludeDependencies but the supported artifact patterns don't have groupId:artifactId::scope that we can use to exclude the test scope. |
Can you help with with this? |
|
Use |
The usage of |
| FakeClientTransportListener fakeClientTransportListener = new FakeClientTransportListener(); | ||
| callMeMaybe(transport.start(fakeClientTransportListener)); | ||
| synchronized (fakeClientTransportListener) { | ||
| fakeClientTransportListener.wait(10000); |
There was a problem hiding this comment.
Wait is allowed to spuriously return early, which means this could be flaky. It also looks like isConnected should be @GuardedBy("this") and all reads/writes to it happen within the lock, since you can't guarantee the wakeup was because of the notify().
Dealing with the spurious wakeup is slightly annoying when there's a timeout. I suggest using a Future<Void> connected = SettableFuture.create() and completing the future instead of setting isConnected to true. Or use a CountDownLatch.
| new ClientStreamTracer[]{new ClientStreamTracer() { | ||
| }}); | ||
|
|
||
| assertThat(stream).isInstanceOf(FailingClientStream.class); |
There was a problem hiding this comment.
The instanceOf is a bit of a code smell, but within acceptability, but abusing the timeout insight is not great. The nice opaque/black-box testing here is to pass a Listener (a mock is fine) to stream.start() and see that the stream fails (closed() is called, and you can see the passed status).
You can easily use a forwarding protocol negotiator (or mock+delegatesTo()) by injecting it on one of the (mock+delegatesTo() is actually really nice in general.) |
Done. |
| } | ||
|
|
||
| public void setSslEngine(SSLEngine sslEngine) { | ||
| this.sslEngine = sslEngine; |
There was a problem hiding this comment.
Bad news.
Looking at "unnecessary" changes to the tests made me realize what's happening here. We can't do this. The ProtocolNegotiator instance is shared across transports/connections. ChannelHandler returned by ProtocolNegotiator.newHandler() is per-connection, not the negotiator itself. The authority check has to be done for a specific connection; the server's certificate can be different on a different connection. That means we need to plumb the negotiator's handler (result of ProtocolNegotiator.newHandler()) or the negotiator handler results instead of the negotiator itself.
To plumb using the negotiator's handler, we could extend ChannelHandler with our own interface that has a verifyAuthority() method, return that from ProtocolNegotiator.newHandler(), save that in the NettyClientTransport, and call it per-RPC. But that will require changing more implementations (including Google-internal ones) and will get ugly as the ProtocolNegotiator API is shared between client-side and server-side.
Plumbing using the negotiator's handler results seems easier. The results are communicated from the handler with ProtocolNegotiationEvent. Those results then are plumbed into NettyClientHandler.handleProtocolNegotiationComplete() and saved. (FYI: The Attributes are the same ones that are exposed in ClientCall.getAttributes().) We can make an interface for Status verifyAuthority(String authority), create an internal Attribute.Key, and store an object able to do the verifyAuthority in those attributes from the tls handler. NettyClientTransport has access to those attributes via handler.getAttributes(). That should be pretty easy, but could be annoying in the new tests (no existing tests need updating).
| @@ -194,6 +210,25 @@ public ClientStream newStream( | |||
| if (channel == null) { | |||
| return new FailingClientStream(statusExplainingWhyTheChannelIsNull, tracers); | |||
| } | |||
| if (callOptions.getAuthority() != null) { | |||
There was a problem hiding this comment.
I see now that we're getting this per-RPC authority separately from the data flow that uses the authority. That makes me nervous, especially for security. As the code changes, it's likely this will break. In fact, it is already broken because the LB-based override only influences setAuthority().
For using the authority, the per-RPC authority is copied to the stream in ClientCallImpl. NettyClientStream then copies it into the Netty Http2Headers (while running an application's thread).
Having the logic in NettyClientTransport has been a bit odd, but fine, and I saw why it was done to avoid plumbing of the protocol negotiator to other places. Normally we'd add RPC logic in NettyClientHandler/NettyClientStream. NettyClientTransport doesn't do much other than create/manage the Netty channel (connection). I suggest we move this logic to NettyClientHandler and forgo any cache synchronization because it will always run on the transport thread.
No description provided.