Skip to content

Commit 3d744d9

Browse files
kannanjgithubkannanjgithub
committed
Review comments.
1 parent 8dd8749 commit 3d744d9

File tree

4 files changed

+62
-27
lines changed

4 files changed

+62
-27
lines changed

‎netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
import java.util.logging.Logger;
8989
import javax.annotation.Nonnull;
9090
import javax.annotation.Nullable;
91+
import javax.annotation.concurrent.GuardedBy;
9192
import javax.net.ssl.SSLEngine;
9293
import javax.net.ssl.SSLException;
9394
import javax.net.ssl.SSLParameters;
@@ -145,10 +146,15 @@ public static FromChannelCredentialsResult from(ChannelCredentials creds) {
145146
trustManagers = Arrays.asList(tmf.getTrustManagers());
146147
}
147148
builder.trustManager(new FixedTrustManagerFactory(trustManagers));
148-
Optional<TrustManager> x509ExtendedTrustManager = trustManagers.stream().filter(
149-
trustManager -> trustManager instanceof X509ExtendedTrustManager).findFirst();
149+
TrustManager x509ExtendedTrustManager = null;
150+
for (TrustManager trustManager: trustManagers) {
151+
if (trustManager instanceof X509ExtendedTrustManager) {
152+
x509ExtendedTrustManager = trustManager;
153+
break;
154+
}
155+
}
150156
return FromChannelCredentialsResult.negotiator(tlsClientFactory(builder.build(),
151-
(X509ExtendedTrustManager) x509ExtendedTrustManager.orElse(null)));
157+
(X509ExtendedTrustManager) x509ExtendedTrustManager));
152158
} catch (SSLException | GeneralSecurityException ex) {
153159
log.log(Level.FINE, "Exception building SslContext", ex);
154160
return FromChannelCredentialsResult.error(
@@ -567,6 +573,7 @@ protected void userEventTriggered0(ChannelHandlerContext ctx, Object evt) throws
567573
}
568574

569575
static final class ClientTlsProtocolNegotiator implements ProtocolNegotiator {
576+
@GuardedBy("this")
570577
private final LinkedHashMap<String, Status> peerVerificationResults =
571578
new LinkedHashMap<String, Status>() {
572579
@Override
@@ -617,10 +624,12 @@ public void close() {
617624

618625
@Override
619626
public synchronized Status verifyAuthority(@Nonnull String authority) {
620-
if (!canVerifyAuthorityOverride()) {
627+
// sslEngine won't be set when creating ClientTlsHandler from InternalProtocolNegotiators
628+
// for example.
629+
if (sslEngine == null || x509ExtendedTrustManager == null) {
621630
return Status.FAILED_PRECONDITION.withDescription(
622-
"Can't allow authority override in rpc when X509ExtendedTrustManager is not "
623-
+ "available");
631+
"Can't allow authority override in rpc when SslEngine or X509ExtendedTrustManager"
632+
+ " is not available");
624633
}
625634
if (peerVerificationResults.containsKey(authority)) {
626635
return peerVerificationResults.get(authority);
@@ -631,7 +640,7 @@ public synchronized Status verifyAuthority(@Nonnull String authority) {
631640
peerVerificationStatus = Status.OK;
632641
} catch (SSLPeerUnverifiedException | CertificateException e) {
633642
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
634-
String.format("Peer hostname verification failed for authority '%s'",
643+
String.format("Peer hostname verification during rpc failed for authority '%s'",
635644
authority)).withCause(e);
636645
}
637646
peerVerificationResults.put(authority, peerVerificationStatus);
@@ -643,17 +652,11 @@ public void setSslEngine(SSLEngine sslEngine) {
643652
this.sslEngine = sslEngine;
644653
}
645654

646-
boolean canVerifyAuthorityOverride() {
647-
// sslEngine won't be set when creating ClientTlsHandlder from InternalProtocolNegotiators
648-
// for example.
649-
return sslEngine != null && x509ExtendedTrustManager != null;
650-
}
651-
652655
private void verifyAuthorityAllowedForPeerCert(String authority)
653656
throws SSLPeerUnverifiedException, CertificateException {
654657
SSLEngine sslEngineWrapper = new SslEngineWrapper(sslEngine, authority);
655658
// The typecasting of Certificate to X509Certificate should work because this method will only
656-
// be called when there is a X509ExtendedTrustManager available.
659+
// be called when using TLS and thus X509.
657660
Certificate[] peerCertificates = sslEngine.getSession().getPeerCertificates();
658661
X509Certificate[] x509PeerCertificates = new X509Certificate[peerCertificates.length];
659662
for (int i = 0; i < peerCertificates.length; i++) {

‎netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ Rpc.METHOD, new Metadata(), CallOptions.DEFAULT.withAuthority("foo.test.google.i
867867
stream.appendTimeoutInsight(insightBuilder);
868868
assertThat(insightBuilder.toString()).contains(
869869
"Status{code=FAILED_PRECONDITION, description=Can't allow authority override in rpc when "
870-
+ "X509ExtendedTrustManager is not available, cause=null}");
870+
+ "SslEngine or X509ExtendedTrustManager is not available, cause=null}");
871871
}
872872

873873
@Test
@@ -891,8 +891,10 @@ Rpc.METHOD, new Metadata(), CallOptions.DEFAULT.withAuthority("foo.test.google.i
891891
InsightBuilder insightBuilder = new InsightBuilder();
892892
stream.appendTimeoutInsight(insightBuilder);
893893
assertThat(insightBuilder.toString()).contains(
894-
"Status{code=UNAVAILABLE, description=Peer hostname verification failed for authority"
895-
+ " 'foo.test.google.in', cause=null}");
894+
"Status{code=UNAVAILABLE, description=Peer hostname verification during rpc failed for"
895+
+ " authority 'foo.test.google.in'");
896+
assertThat(insightBuilder.toString()).contains("cause=java.security.cert.CertificateException:"
897+
+ " No subject alternative DNS name matching foo.test.google.in found.");
896898
}
897899

898900
@Test

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

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,37 @@
1616

1717
package io.grpc.okhttp;
1818

19+
import static com.google.common.base.Preconditions.checkNotNull;
20+
import static io.grpc.internal.GrpcUtil.DEFAULT_KEEPALIVE_TIMEOUT_NANOS;
21+
import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED;
22+
1923
import com.google.common.annotations.VisibleForTesting;
2024
import com.google.common.base.Preconditions;
21-
import io.grpc.*;
22-
import io.grpc.internal.*;
25+
import io.grpc.CallCredentials;
26+
import io.grpc.ChannelCredentials;
27+
import io.grpc.ChannelLogger;
28+
import io.grpc.ChoiceChannelCredentials;
29+
import io.grpc.CompositeCallCredentials;
30+
import io.grpc.CompositeChannelCredentials;
31+
import io.grpc.ExperimentalApi;
32+
import io.grpc.ForwardingChannelBuilder2;
33+
import io.grpc.InsecureChannelCredentials;
34+
import io.grpc.Internal;
35+
import io.grpc.ManagedChannelBuilder;
36+
import io.grpc.TlsChannelCredentials;
37+
import io.grpc.internal.AtomicBackoff;
38+
import io.grpc.internal.ClientTransportFactory;
39+
import io.grpc.internal.ConnectionClientTransport;
40+
import io.grpc.internal.FixedObjectPool;
41+
import io.grpc.internal.GrpcUtil;
42+
import io.grpc.internal.KeepAliveManager;
43+
import io.grpc.internal.ManagedChannelImplBuilder;
2344
import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider;
2445
import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder;
46+
import io.grpc.internal.ObjectPool;
2547
import io.grpc.internal.SharedResourceHolder.Resource;
48+
import io.grpc.internal.SharedResourcePool;
49+
import io.grpc.internal.TransportTracer;
2650
import io.grpc.okhttp.internal.CipherSuite;
2751
import io.grpc.okhttp.internal.ConnectionSpec;
2852
import io.grpc.okhttp.internal.Platform;
@@ -41,17 +65,22 @@
4165
import java.util.Collections;
4266
import java.util.EnumSet;
4367
import java.util.Set;
44-
import java.util.concurrent.*;
68+
import java.util.concurrent.Executor;
69+
import java.util.concurrent.ExecutorService;
70+
import java.util.concurrent.Executors;
71+
import java.util.concurrent.ScheduledExecutorService;
72+
import java.util.concurrent.TimeUnit;
4573
import java.util.logging.Level;
4674
import java.util.logging.Logger;
4775
import javax.annotation.CheckReturnValue;
4876
import javax.annotation.Nullable;
4977
import javax.net.SocketFactory;
50-
import javax.net.ssl.*;
51-
52-
import static com.google.common.base.Preconditions.checkNotNull;
53-
import static io.grpc.internal.GrpcUtil.DEFAULT_KEEPALIVE_TIMEOUT_NANOS;
54-
import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED;
78+
import javax.net.ssl.HostnameVerifier;
79+
import javax.net.ssl.KeyManager;
80+
import javax.net.ssl.KeyManagerFactory;
81+
import javax.net.ssl.SSLContext;
82+
import javax.net.ssl.SSLSocketFactory;
83+
import javax.net.ssl.TrustManager;
5584

5685
/** Convenience class for building channels with the OkHttp transport. */
5786
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785")

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import io.grpc.InsecureChannelCredentials;
3535
import io.grpc.ManagedChannel;
3636
import io.grpc.TlsChannelCredentials;
37+
import io.grpc.internal.CertificateUtils;
3738
import io.grpc.internal.ClientTransportFactory;
3839
import io.grpc.internal.ClientTransportFactory.SwapChannelCredentialsResult;
3940
import io.grpc.internal.FakeClock;
@@ -212,7 +213,7 @@ public void sslSocketFactoryFrom_tls_mtls() throws Exception {
212213

213214
TrustManager[] trustManagers;
214215
try (InputStream ca = TlsTesting.loadCert("ca.pem")) {
215-
trustManagers = OkHttpChannelBuilder.createTrustManager(ca);
216+
trustManagers = CertificateUtils.createTrustManager(ca);
216217
}
217218

218219
SSLContext serverContext = SSLContext.getInstance("TLS");
@@ -257,7 +258,7 @@ public void sslSocketFactoryFrom_tls_mtls_keyFile() throws Exception {
257258
InputStream ca = TlsTesting.loadCert("ca.pem")) {
258259
serverContext.init(
259260
OkHttpChannelBuilder.createKeyManager(server1Chain, server1Key),
260-
OkHttpChannelBuilder.createTrustManager(ca),
261+
CertificateUtils.createTrustManager(ca),
261262
null);
262263
}
263264
final SSLServerSocket serverListenSocket =

0 commit comments

Comments
 (0)