ok-http: Per-rpc call option authority verification #11754
Conversation
…orityverifyokhttp
…oved setting sslEndpointAlgorithm.
This reverts commit f5b3614.
|
Yet to make the changes to use X509ExtendedTrustManager only via reflection if it is available in the class loader. |
Done. |
|
Moved the authority verification from OkHttpClientTransport.newStream down to the point just before sending the headers over the network so any overwrites to the authority gets seen. |
examples/src/main/java/io/grpc/examples/deadline/DeadlineServer.java
Outdated
Show resolved
Hide resolved
# Conflicts: # core/src/main/java/io/grpc/internal/CertificateUtils.java # okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
| if (peerVerificationResults.containsKey(authority)) { | ||
| peerVerificationStatus = peerVerificationResults.get(authority); | ||
| } else { | ||
| if (x509ExtendedTrustManager == null) { |
There was a problem hiding this comment.
If x509ExtendedTrustManagerClass == null, we want to let this authority through as we wouldn't use it during the connection handshake either.
... And digging further to see what conditions we use, it looks like... we never use the trust manager for hostname verification. Looking at the SSLParameters, I don't see us ever calling setEndpointIdentificationAlgorithm(). And I commented out the hostnameVerifier.verify() call in OkHttpTlsUpgrader and TlsTest failed because hostname verification was not being performed. I added io.grpc.internal.testing.TestUtils.installConscryptIfAvailable(); to the @Before method, and the error stderr logs changed but still the same lack of verification. And that shouldn't be a surprise since those APIs were added in API level 24 which came out in August 2016, four days after grpc-java 1.0.0. It would be years before enough devices supported it to be interesting. I thought we had checked some of this before, but it was too long ago for me to figure out where we went wrong.
(Note that there still may be cases where it is used, but that'd have to be with a custom SSLSocketFactory.)
There was a problem hiding this comment.
<<< If x509ExtendedTrustManagerClass == null, we want to let this authority through as we wouldn't use it during the connection handshake either.
Now I'm only doing the authority check during RPC if (!authority.equals(defaultAuthority)). If x509ExtendedTrustManager is null, can we check the value of sslParameters endpoint identification algorithm, and only if it is set to HTTPS then fail the RPC because the checkServerTrusted won't check the hostname anyway if the endpoint identification algorithm is not HTTPS.
Also should we then do the checkServerTrusted check here only if sslParameters had endpoint identification algorithm set to 'https' in the protocol negotiator?
There was a problem hiding this comment.
#11754 (comment) has a discussion about still running the x509ExtendedTrustManager even though the endpoint algorithm isn't set. I'm thinking the idea was that while the default verifier won't be checking the hostname a user-provided TrustManager could be.
If x509ExtendedTrustManagerClass == null (that's the class), then the trust manager can't verify the host name, because the API doesn't exist. x509ExtendedTrustManagerClass == null && x509ExtendedTrustManager == null is common enough, as it just means you're on an old Android version. The endpoint identification algorithm has nothing to do with it as that API doesn't exist either on those old versions. x509ExtendedTrustManagerClass != null && x509ExtendedTrustManager == null is a case I thought I didn't care too much about, because the GeneralSecurityException when creating the trust manager is highly unlikely. So I was fine with it failing. But I see now it is also the case where there is a trust manager, it just isn't an X509ExtendedTrustManager.
I don't think we should search for a an extended manager in getX509ExtendedTrustManager(). That's not how it is done during handshaking. Instead, look for an X509TrustManager. Then have the isInstance() check when we are doing the actual verification for an individual RPC. That means we can also run the code from getX509ExtendedTrustManager() even if x509ExtendedTrustManagerClass == null. Then x509ExtendedTrustManager == null will only be true when we encountered the GeneralSecurityException
| ManagedChannel channel = grpcCleanupRule.register( | ||
| OkHttpChannelBuilder.forAddress("localhost", server.getPort(), channelCreds) | ||
| .overrideAuthority(TestUtils.TEST_SERVER_HOST) | ||
| .hostnameVerifier((hostname, session) -> true) |
There was a problem hiding this comment.
I'm unable to set hostname verifier after setting channel creds. I need to override hostname verifier behavior to simply return true, and TLS credentials so that authority verification happens. I'm facing this exception
java.lang.IllegalStateException: Cannot change security when using ChannelCredentials
at com.google.common.base.Preconditions.checkState(Preconditions.java:513)
at io.grpc.okhttp.OkHttpChannelBuilder.hostnameVerifier(OkHttpChannelBuilder.java:395)
at io.grpc.okhttp.TlsTest.perRpcAuthorityOverride_peerVerificationFails_rpcFails(TlsTest.java:302)
How can I do what I want to do?
There was a problem hiding this comment.
To change the hostname verifier, you can't pass channel creds to the channel builder.
| if (goAwayStatus != null) { | ||
| clientStream.transportState().transportReportStatus( | ||
| goAwayStatus, RpcProgress.MISCARRIED, true, new Metadata()); | ||
| } else if (streams.size() >= maxConcurrentStreams) { | ||
| pendingStreams.add(clientStream); | ||
| setInUse(clientStream); | ||
| } else { | ||
| if (!authority.equals(defaultAuthority)) { |
There was a problem hiding this comment.
This is a lot of code is being added to a block that used to be one line in an eight line method. Move most of it to a helper method. You can have the caching logic here. We should only have a single cache that has the result from the combined hostnameVerifier and trustManager checks.
| tmf.init((KeyStore) null); | ||
| tm = tmf.getTrustManagers(); | ||
| } | ||
| return tm[0]; |
There was a problem hiding this comment.
You still need the loop to find the first _X509_TrustManager.
| if (goAwayStatus != null) { | ||
| clientStream.transportState().transportReportStatus( | ||
| goAwayStatus, RpcProgress.MISCARRIED, true, new Metadata()); | ||
| } else if (streams.size() >= maxConcurrentStreams) { | ||
| pendingStreams.add(clientStream); | ||
| setInUse(clientStream); | ||
| } else { | ||
| if (!authority.equals(defaultAuthority)) { |
There was a problem hiding this comment.
You deleted the socket instanceof SSLSocket check. Add that to this authority check? if (socket instanceof SSLSocket && !authority.equals(defaultAuthority))
No description provided.