ok-http: Per-rpc call option authority verification #11754
Conversation
…orityverifyokhttp
| @@ -99,10 +99,10 @@ public int compare(ManagedChannelProvider o1, ManagedChannelProvider o2) { | |||
| public static synchronized ManagedChannelRegistry getDefaultRegistry() { | |||
| if (instance == null) { | |||
| List<ManagedChannelProvider> providerList = ServiceProviders.loadAll( | |||
| ManagedChannelProvider.class, | |||
There was a problem hiding this comment.
Revert all changes to this file? I only see reformatting.
examples/example-tls/build.gradle
Outdated
| implementation "io.grpc:grpc-api:${grpcVersion}" | ||
| implementation "io.grpc:grpc-okhttp:${grpcVersion}" |
| @Override | ||
| public SSLParameters getSSLParameters() { | ||
| SSLParameters sslParameters = sslSocket.getSSLParameters(); | ||
| sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); |
There was a problem hiding this comment.
The endpointIdentificationAlgorithm in sslParameters of the real sslSocket is null. In the case of Netty we are setting it here
for the handshake but not for Okhttp.There was a problem hiding this comment.
But changing here isn't actually changing it for the connection. I think we're generally depending on the HostnameVerifier instead. setEndpointIdentificationAlgorithm() was added in Android API level 24. We're claiming to support API level 21 (although we should change that to 23)
I'm really bothered by the lack of failure building this. AnimalSniffer missed some things at times, but it seems to be completely broken right now. Running with --console=plain I see animalsnifferMain is SKIPPED. Digging deeper, it seems we're missing @signature in the version so it hasn't been running.
There was a problem hiding this comment.
But the setting endpoint algorithm in the wrapper SslSocket was for causing the hostname check to occur when we do the per rpc check using the X509ExrendedTrustManager. Do we not want that to happen?
There was a problem hiding this comment.
As long as we don't have a minimum API level of 24 we can't check authority using X509ExtendedTrustManager. Should we then remove these changes and use only HostnameVerifier, and disallow per rpc authority override if one is not provided?
There was a problem hiding this comment.
Discussed offline and understood we let the same checks during handshake apply during RPC and removed calling setEndpointIdentificationAlgorithm.
| /** | ||
| * SSLSocket wrapper that provides a fake SSLSession for handshake session. | ||
| */ | ||
| static class SslSocketWrapper extends SSLSocket { |
There was a problem hiding this comment.
Make a NoopSslSocket, like we did for Netty?
| /** | ||
| * Fake SSLSession instance that provides the peer host name to verify for per-rpc check. | ||
| */ | ||
| static class FakeSslSession implements SSLSession { |
There was a problem hiding this comment.
Do we want the NoopSslSession from Netty to move to io.grpc.internal and then use it here? Or we just make a copy. I don't think it actually matters much which way we go in this case.
| @@ -425,7 +426,7 @@ static HandshakerSocketFactoryResult handshakerSocketFactoryFrom(ServerCredentia | |||
| tm = tlsCreds.getTrustManagers().toArray(new TrustManager[0]); | |||
| } else if (tlsCreds.getRootCertificates() != null) { | |||
| try { | |||
| tm = OkHttpChannelBuilder.createTrustManager(tlsCreds.getRootCertificates()); | |||
| tm = createTrustManager(tlsCreds.getRootCertificates()); | |||
There was a problem hiding this comment.
Revert? This is the only change, and it seems more clear before.
There was a problem hiding this comment.
No, I have moved this method to io.grpc.internal.CertificateUtil and static imported the method since there is a name clash for CertificateUtil.
| if (tlsCreds.getTrustManagers() != null) { | ||
| tm = tlsCreds.getTrustManagers().toArray(new TrustManager[0]); | ||
| } else if (tlsCreds.getRootCertificates() != null) { | ||
| tm = io.grpc.internal.CertificateUtils.createTrustManager(tlsCreds.getRootCertificates()); |
There was a problem hiding this comment.
Import CertificateUtils? Is there a name collision I'm not seeing?
| throws GeneralSecurityException { | ||
| TrustManager[] tm = null; | ||
| // Using the same way of creating TrustManager from | ||
| // {@link OkHttpChannelBuilder#sslSocketFactoryFrom}. |
There was a problem hiding this comment.
Don't use Javadoc-style in regular comments. OkHttpChannelBuilder.sslSocketFactoryFrom()
| tmf.init((KeyStore) null); | ||
| tm = tmf.getTrustManagers(); | ||
| } | ||
| return Arrays.stream(tm).filter( |
…oved setting sslEndpointAlgorithm.
|
Yet to make the changes to use X509ExtendedTrustManager only via reflection if it is available in the class loader. |
No description provided.