Skip to content

Commit f820297

Browse files
kannanjgithubkannanjgithub
committed
Review comments
1 parent 671b6a7 commit f820297

File tree

4 files changed

+97
-85
lines changed

4 files changed

+97
-85
lines changed

‎core/src/main/java/io/grpc/internal/CertificateUtils.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
/**
3434
* Contains certificate/key PEM file utility method(s) for internal usage.
3535
*/
36-
public class CertificateUtils {
36+
public final class CertificateUtils {
3737
/**
3838
* Creates X509TrustManagers using the provided CA certs.
3939
*/

‎okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java‎

Lines changed: 85 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import io.grpc.okhttp.OkHttpChannelBuilder.OkHttpTransportFactory;
6363
import io.grpc.okhttp.internal.ConnectionSpec;
6464
import io.grpc.okhttp.internal.Credentials;
65+
import io.grpc.okhttp.internal.OkHostnameVerifier;
6566
import io.grpc.okhttp.internal.StatusLine;
6667
import io.grpc.okhttp.internal.framed.ErrorCode;
6768
import io.grpc.okhttp.internal.framed.FrameReader;
@@ -167,20 +168,24 @@ private static Map<ErrorCode, Status> buildErrorCodeToStatusMap() {
167168
return Collections.unmodifiableMap(errorToStatus);
168169
}
169170

170-
private static Class<?> x509ExtendedTrustManagerClass;
171-
private static Method checkServerTrustedMethod;
171+
private static final Class<?> x509ExtendedTrustManagerClass;
172+
private static final Method checkServerTrustedMethod;
172173

173174
static {
175+
Class<?> x509ExtendedTrustManagerClass1 = null;
176+
Method checkServerTrustedMethod1 = null;
174177
try {
175-
x509ExtendedTrustManagerClass = Class.forName("javax.net.ssl.X509ExtendedTrustManager");
176-
checkServerTrustedMethod = x509ExtendedTrustManagerClass.getMethod("checkServerTrusted",
178+
x509ExtendedTrustManagerClass1 = Class.forName("javax.net.ssl.X509ExtendedTrustManager");
179+
checkServerTrustedMethod1 = x509ExtendedTrustManagerClass1.getMethod("checkServerTrusted",
177180
X509Certificate[].class, String.class, Socket.class);
178181
} catch (ClassNotFoundException e) {
179182
// Per-rpc authority override via call options will be disallowed.
180183
} catch (NoSuchMethodException e) {
181184
// Should never happen since X509ExtendedTrustManager was introduced in Android API level 24
182185
// along with checkServerTrusted.
183186
}
187+
x509ExtendedTrustManagerClass = x509ExtendedTrustManagerClass1;
188+
checkServerTrustedMethod = checkServerTrustedMethod1;
184189
}
185190

186191
private final InetSocketAddress address;
@@ -254,10 +259,10 @@ protected boolean removeEldestEntry(Map.Entry<String, T> eldest) {
254259
}
255260
}
256261

257-
private final Map<String, Boolean> hostnameVerificationResults =
258-
Collections.synchronizedMap(new LruCache<>());
259-
private final Map<String, Status> peerVerificationResults =
260-
Collections.synchronizedMap(new LruCache<>());
262+
@GuardedBy("lock")
263+
private final Map<String, Boolean> hostnameVerificationResults = new LruCache<>();
264+
@GuardedBy("lock")
265+
private final Map<String, Status> peerVerificationResults = new LruCache<>();
261266

262267
@GuardedBy("lock")
263268
private final InUseStateAggregator<OkHttpClientStream> inUseState =
@@ -333,7 +338,8 @@ private OkHttpClientTransport(
333338
this.socketFactory = transportFactory.socketFactory == null
334339
? SocketFactory.getDefault() : transportFactory.socketFactory;
335340
this.sslSocketFactory = transportFactory.sslSocketFactory;
336-
this.hostnameVerifier = transportFactory.hostnameVerifier;
341+
this.hostnameVerifier = transportFactory.hostnameVerifier != null
342+
? transportFactory.hostnameVerifier : OkHostnameVerifier.INSTANCE;
337343
this.connectionSpec = Preconditions.checkNotNull(
338344
transportFactory.connectionSpec, "connectionSpec");
339345
this.stopwatchFactory = Preconditions.checkNotNull(stopwatchFactory, "stopwatchFactory");
@@ -373,7 +379,7 @@ private OkHttpClientTransport(
373379
@SuppressWarnings("AddressSelection") // An IP address always returns one address
374380
@VisibleForTesting
375381
OkHttpClientTransport(
376-
OkHttpChannelBuilder.OkHttpTransportFactory transportFactory,
382+
OkHttpTransportFactory transportFactory,
377383
String userAgent,
378384
Supplier<Stopwatch> stopwatchFactory,
379385
Variant variant,
@@ -493,7 +499,7 @@ public OkHttpClientStream newStream(
493499

494500
private TrustManager getX509ExtendedTrustManager(TlsChannelCredentials tlsCreds)
495501
throws GeneralSecurityException {
496-
TrustManager[] tm = null;
502+
TrustManager[] tm;
497503
// Using the same way of creating TrustManager from OkHttpChannelBuilder.sslSocketFactoryFrom()
498504
if (tlsCreds.getTrustManagers() != null) {
499505
tm = tlsCreds.getTrustManagers().toArray(new TrustManager[0]);
@@ -522,76 +528,80 @@ void streamReadyToStart(OkHttpClientStream clientStream, String authority) {
522528
pendingStreams.add(clientStream);
523529
setInUse(clientStream);
524530
} else {
525-
if (hostnameVerifier != null && socket instanceof SSLSocket) {
526-
Boolean hostnameVerificationResult;
527-
if (hostnameVerificationResults.containsKey(authority)) {
528-
hostnameVerificationResult = hostnameVerificationResults.get(authority);
529-
} else {
530-
hostnameVerificationResult = hostnameVerifier.verify(
531-
authority, ((SSLSocket) socket).getSession());
532-
hostnameVerificationResults.put(authority, hostnameVerificationResult);
533-
}
534-
if (!hostnameVerificationResult) {
535-
if (enablePerRpcAuthorityCheck) {
536-
clientStream.transportState().transportReportStatus(
537-
Status.UNAVAILABLE.withDescription(
538-
String.format("HostNameVerifier verification failed for authority '%s'",
539-
authority)),
540-
RpcProgress.DROPPED, true, new Metadata());
541-
return;
531+
if (!authority.equals(defaultAuthority)) {
532+
if (hostnameVerifier != null && socket instanceof SSLSocket) {
533+
Boolean hostnameVerificationResult;
534+
if (hostnameVerificationResults.containsKey(authority)) {
535+
hostnameVerificationResult = hostnameVerificationResults.get(authority);
542536
} else {
543-
log.log(Level.WARNING, String.format("HostNameVerifier verification failed for "
544-
+ "authority '%s'. This will be an error in the future.",
545-
authority));
546-
}
547-
}
548-
}
549-
if (socket instanceof SSLSocket && authority != null
550-
&& channelCredentials != null
551-
&& channelCredentials instanceof TlsChannelCredentials) {
552-
Status peerVerificationStatus = null;
553-
if (peerVerificationResults.containsKey(authority)) {
554-
peerVerificationStatus = peerVerificationResults.get(authority);
555-
} else {
556-
if (x509ExtendedTrustManager == null) {
557-
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
558-
String.format("Could not verify authority '%s' for the rpc with no "
559-
+ "X509ExtendedTrustManager available",
560-
authority));
561-
}
562-
if (x509ExtendedTrustManager != null) {
563-
try {
564-
Certificate[] peerCertificates = sslSession.getPeerCertificates();
565-
X509Certificate[] x509PeerCertificates = new X509Certificate[peerCertificates.length];
566-
for (int i = 0; i < peerCertificates.length; i++) {
567-
x509PeerCertificates[i] = (X509Certificate) peerCertificates[i];
568-
}
569-
checkServerTrustedMethod.invoke(x509ExtendedTrustManager, x509PeerCertificates,
570-
"RSA", new SslSocketWrapper((SSLSocket) socket, authority));
571-
peerVerificationStatus = Status.OK;
572-
} catch (SSLPeerUnverifiedException | InvocationTargetException
573-
| IllegalAccessException e) {
574-
peerVerificationStatus = Status.UNAVAILABLE.withCause(e).withDescription(
575-
"Peer verification failed");
537+
hostnameVerificationResult = hostnameVerifier.verify(
538+
authority, ((SSLSocket) socket).getSession());
539+
hostnameVerificationResults.put(authority, hostnameVerificationResult);
540+
if (!hostnameVerificationResult && !enablePerRpcAuthorityCheck) {
541+
log.log(Level.WARNING, String.format("HostNameVerifier verification failed for "
542+
+ "authority '%s'. This will be an error in the future.",
543+
authority));
576544
}
577-
if (peerVerificationStatus != null) {
578-
peerVerificationResults.put(authority, peerVerificationStatus);
545+
}
546+
if (!hostnameVerificationResult) {
547+
if (enablePerRpcAuthorityCheck) {
548+
clientStream.transportState().transportReportStatus(
549+
Status.UNAVAILABLE.withDescription(String.format(
550+
"HostNameVerifier verification failed for authority '%s'",
551+
authority)),
552+
RpcProgress.PROCESSED, true, new Metadata());
553+
return;
579554
}
580555
}
581556
}
582-
if (peerVerificationStatus != null && !peerVerificationStatus.isOk()) {
583-
if (enablePerRpcAuthorityCheck) {
584-
clientStream.transportState().transportReportStatus(
585-
peerVerificationStatus, RpcProgress.DROPPED, true, new Metadata());
586-
return;
557+
if (socket instanceof SSLSocket
558+
&& (channelCredentials != null
559+
&& channelCredentials instanceof TlsChannelCredentials)
560+
|| sslSocketFactory != null) {
561+
Status peerVerificationStatus;
562+
if (peerVerificationResults.containsKey(authority)) {
563+
peerVerificationStatus = peerVerificationResults.get(authority);
587564
} else {
588-
if (peerVerificationStatus.getCause() != null) {
589-
log.log(Level.WARNING, peerVerificationStatus.getDescription()
590-
+ ". This will be an error in the future.",
591-
peerVerificationStatus.getCause());
565+
if (x509ExtendedTrustManager == null) {
566+
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
567+
String.format("Could not verify authority '%s' for the rpc with no "
568+
+ "X509ExtendedTrustManager available",
569+
authority));
592570
} else {
593-
log.log(Level.WARNING, peerVerificationStatus.getDescription()
594-
+ ". This will be an error in the future.");
571+
try {
572+
Certificate[] peerCertificates = sslSession.getPeerCertificates();
573+
X509Certificate[] x509PeerCertificates =
574+
new X509Certificate[peerCertificates.length];
575+
for (int i = 0; i < peerCertificates.length; i++) {
576+
x509PeerCertificates[i] = (X509Certificate) peerCertificates[i];
577+
}
578+
checkServerTrustedMethod.invoke(x509ExtendedTrustManager, x509PeerCertificates,
579+
"RSA", new SslSocketWrapper((SSLSocket) socket, authority));
580+
peerVerificationStatus = Status.OK;
581+
} catch (SSLPeerUnverifiedException | InvocationTargetException
582+
| IllegalAccessException e) {
583+
peerVerificationStatus = Status.UNAVAILABLE.withCause(e).withDescription(
584+
"Peer verification failed");
585+
}
586+
if (peerVerificationStatus != null) {
587+
peerVerificationResults.put(authority, peerVerificationStatus);
588+
}
589+
}
590+
}
591+
if (peerVerificationStatus != null && !peerVerificationStatus.isOk()) {
592+
if (enablePerRpcAuthorityCheck) {
593+
clientStream.transportState().transportReportStatus(
594+
peerVerificationStatus, RpcProgress.PROCESSED, true, new Metadata());
595+
return;
596+
} else {
597+
if (peerVerificationStatus.getCause() != null) {
598+
log.log(Level.WARNING, peerVerificationStatus.getDescription()
599+
+ ". This will be an error in the future.",
600+
peerVerificationStatus.getCause());
601+
} else {
602+
log.log(Level.WARNING, peerVerificationStatus.getDescription()
603+
+ ". This will be an error in the future.");
604+
}
595605
}
596606
}
597607
}
@@ -603,7 +613,7 @@ void streamReadyToStart(OkHttpClientStream clientStream, String authority) {
603613
@SuppressWarnings("GuardedBy")
604614
@GuardedBy("lock")
605615
private void startStream(OkHttpClientStream stream) {
606-
Preconditions.checkState(
616+
checkState(
607617
stream.transportState().id() == OkHttpClientStream.ABSENT_ID, "StreamId already assigned");
608618
streams.put(nextStreamId, stream);
609619
setInUse(stream);

‎okhttp/src/main/java/io/grpc/okhttp/OkHttpTlsUpgrader.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@
1919
import com.google.common.annotations.VisibleForTesting;
2020
import com.google.common.base.Preconditions;
2121
import io.grpc.okhttp.internal.ConnectionSpec;
22-
import io.grpc.okhttp.internal.OkHostnameVerifier;
2322
import io.grpc.okhttp.internal.Protocol;
2423
import java.io.IOException;
2524
import java.net.Socket;
2625
import java.util.Arrays;
2726
import java.util.Collections;
2827
import java.util.List;
28+
import javax.annotation.Nonnull;
2929
import javax.net.ssl.HostnameVerifier;
3030
import javax.net.ssl.SSLPeerUnverifiedException;
3131
import javax.net.ssl.SSLSocket;
@@ -52,7 +52,7 @@ final class OkHttpTlsUpgrader {
5252
* @throws RuntimeException if the upgrade negotiation failed.
5353
*/
5454
public static SSLSocket upgrade(SSLSocketFactory sslSocketFactory,
55-
HostnameVerifier hostnameVerifier, Socket socket, String host, int port,
55+
@Nonnull HostnameVerifier hostnameVerifier, Socket socket, String host, int port,
5656
ConnectionSpec spec) throws IOException {
5757
Preconditions.checkNotNull(sslSocketFactory, "sslSocketFactory");
5858
Preconditions.checkNotNull(socket, "socket");
@@ -67,9 +67,6 @@ public static SSLSocket upgrade(SSLSocketFactory sslSocketFactory,
6767
"Only " + TLS_PROTOCOLS + " are supported, but negotiated protocol is %s",
6868
negotiatedProtocol);
6969

70-
if (hostnameVerifier == null) {
71-
hostnameVerifier = OkHostnameVerifier.INSTANCE;
72-
}
7370
if (!hostnameVerifier.verify(canonicalizeHost(host), sslSocket.getSession())) {
7471
throw new SSLPeerUnverifiedException("Cannot verify hostname: " + host);
7572
}

‎okhttp/src/test/java/io/grpc/okhttp/TlsTest.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,12 +296,17 @@ public void perRpcAuthorityOverride_peerVerificationFails_rpcFails()
296296
.build();
297297
}
298298
Server server = grpcCleanupRule.register(server(serverCreds));
299-
ManagedChannel channel = grpcCleanupRule.register(clientChannel(server, channelCreds));
299+
ManagedChannel channel = grpcCleanupRule.register(
300+
OkHttpChannelBuilder.forAddress("localhost", server.getPort(), channelCreds)
301+
.overrideAuthority(TestUtils.TEST_SERVER_HOST)
302+
.hostnameVerifier((hostname, session) -> true)
303+
.directExecutor()
304+
.build());
300305

301306
try {
302307
fakeTrustManager.setFailCheckServerTrusted();
303308
ClientCalls.blockingUnaryCall(channel, SimpleServiceGrpc.getUnaryRpcMethod(),
304-
CallOptions.DEFAULT.withAuthority("foo.test.google.fr"),
309+
CallOptions.DEFAULT.withAuthority("foo.test.google.in"),
305310
SimpleRequest.getDefaultInstance());
306311
fail("Expected exception for authority verification failure.");
307312
} catch (StatusRuntimeException ex) {
@@ -371,13 +376,13 @@ public void perRpcAuthorityOverride_tlsCreds_noX509ExtendedTrustManager_fails()
371376

372377
try {
373378
ClientCalls.blockingUnaryCall(channel, SimpleServiceGrpc.getUnaryRpcMethod(),
374-
CallOptions.DEFAULT.withAuthority("foo.test.google.fr"),
379+
CallOptions.DEFAULT.withAuthority("moo.test.google.fr"),
375380
SimpleRequest.getDefaultInstance());
376381
fail("Expected exception for authority verification failure.");
377382
} catch (StatusRuntimeException ex) {
378383
assertThat(ex.getStatus().getCode()).isEqualTo(Status.Code.UNAVAILABLE);
379384
assertThat(ex.getStatus().getDescription()).isEqualTo(
380-
"Could not verify authority 'foo.test.google.fr' for the rpc with no "
385+
"Could not verify authority 'moo.test.google.fr' for the rpc with no "
381386
+ "X509ExtendedTrustManager available");
382387
}
383388
} finally {

0 commit comments

Comments
 (0)