Skip to content

Commit 649f53c

Browse files
kannanjgithubkannanjgithub
committed
Some changes based on similar comments in the authority check for Netty PR.
1 parent ce800f7 commit 649f53c

File tree

4 files changed

+43
-57
lines changed

4 files changed

+43
-57
lines changed

‎api/src/main/java/io/grpc/ManagedChannelRegistry.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,4 +225,4 @@ public ProviderNotFoundException(String msg) {
225225
super(msg);
226226
}
227227
}
228-
}
228+
}

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

Lines changed: 33 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,11 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
134134
private static final Logger log = Logger.getLogger(OkHttpClientTransport.class.getName());
135135
private static final String GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK =
136136
"GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK";
137+
static boolean enablePerRpcAuthorityCheck =
138+
GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false);
137139
private final ChannelCredentials channelCredentials;
138140
private Socket sock;
139141
private SSLSession sslSession;
140-
private final Logger logger = Logger.getLogger(OkHttpClientTransport.class.getName());
141142

142143
private static Map<ErrorCode, Status> buildErrorCodeToStatusMap() {
143144
Map<ErrorCode, Status> errorToStatus = new EnumMap<>(ErrorCode.class);
@@ -179,9 +180,8 @@ private static Map<ErrorCode, Status> buildErrorCodeToStatusMap() {
179180
} catch (ClassNotFoundException e) {
180181
// Per-rpc authority override via call options will be disallowed.
181182
} catch (NoSuchMethodException e) {
182-
// Should never happen.
183-
Logger.getLogger(OkHttpClientTransport.class.getName()).warning("Method checkServerTrusted "
184-
+ "not found in javax.net.ssl.X509ExtendedTrustManager");
183+
// Should never happen since X509ExtendedTrustManager was introduced in Android API level 24
184+
// along with checkServerTrusted.
185185
}
186186
}
187187

@@ -246,13 +246,13 @@ private static Map<ErrorCode, Status> buildErrorCodeToStatusMap() {
246246
private final boolean useGetForSafeMethods;
247247
@GuardedBy("lock")
248248
private final TransportTracer transportTracer;
249-
private final LinkedHashMap<String, Status> peerVerificationResults =
249+
private final Map<String, Status> peerVerificationResults = Collections.synchronizedMap(
250250
new LinkedHashMap<String, Status>() {
251251
@Override
252252
protected boolean removeEldestEntry(Map.Entry<String, Status> eldest) {
253253
return size() > 100;
254254
}
255-
};
255+
});
256256

257257
@GuardedBy("lock")
258258
private final InUseStateAggregator<OkHttpClientStream> inUseState =
@@ -453,69 +453,52 @@ public ClientStream newStream(
453453
if (hostnameVerifier != null && socket instanceof SSLSocket
454454
&& !hostnameVerifier.verify(callOptions.getAuthority(),
455455
((SSLSocket) socket).getSession())) {
456-
if (GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false)) {
456+
if (enablePerRpcAuthorityCheck) {
457457
return new FailingClientStream(Status.UNAVAILABLE.withDescription(
458458
String.format("HostNameVerifier verification failed for authority '%s'",
459459
callOptions.getAuthority())), tracers);
460460
}
461-
logger.warning(String.format("HostNameVerifier verification failed for authority '%s'.",
462-
callOptions.getAuthority()));
463461
}
464462
if (socket instanceof SSLSocket && callOptions.getAuthority() != null
465463
&& channelCredentials != null && channelCredentials instanceof TlsChannelCredentials) {
466464
Status peerVerificationStatus = null;
467465
if (peerVerificationResults.containsKey(callOptions.getAuthority())) {
468466
peerVerificationStatus = peerVerificationResults.get(callOptions.getAuthority());
469467
} else {
470-
TrustManager x509ExtendedTrustManager = null;
471-
boolean warningLogged = false;
468+
TrustManager x509ExtendedTrustManager;
472469
try {
473470
x509ExtendedTrustManager = x509ExtendedTrustManagerClass != null
474471
? getX509ExtendedTrustManager((TlsChannelCredentials) channelCredentials) : null;
475-
} catch (GeneralSecurityException e) {
476-
if (GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false)) {
477-
return new FailingClientStream(Status.UNAVAILABLE.withDescription(
478-
"Failure getting X509ExtendedTrustManager from TlsCredentials").withCause(e),
479-
tracers);
480-
}
481-
logger.warning(String.format("Failure getting X509ExtendedTrustManager from "
482-
+ "TlsCredentials due to: %s", e.getMessage()));
483-
warningLogged = true;
484-
}
485-
if (x509ExtendedTrustManager == null) {
486-
if (GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false)) {
487-
return new FailingClientStream(Status.UNAVAILABLE.withDescription(
488-
"Can't allow authority override in rpc when X509ExtendedTrustManager is not "
489-
+ "available"), tracers);
490-
}
491-
if (!warningLogged) {
492-
logger.warning("Authority override set for rpc when X509ExtendedTrustManager is not "
493-
+ "available.");
494-
}
495-
} else {
496-
try {
497-
Certificate[] peerCertificates = sslSession.getPeerCertificates();
498-
X509Certificate[] x509PeerCertificates = new X509Certificate[peerCertificates.length];
499-
for (int i = 0; i < peerCertificates.length; i++) {
500-
x509PeerCertificates[i] = (X509Certificate) peerCertificates[i];
472+
if (x509ExtendedTrustManager == null) {
473+
if (GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false)) {
474+
return new FailingClientStream(Status.UNAVAILABLE.withDescription(
475+
"Can't allow authority override in rpc when X509ExtendedTrustManager is not "
476+
+ "available"), tracers);
501477
}
502-
// Should never happen
503-
if (checkServerTrustedMethod == null) {
504-
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
505-
"Method checkServerTrusted not found in "
506-
+ "javax.net.ssl.X509ExtendedTrustManager");
507-
} else {
478+
} else {
479+
try {
480+
Certificate[] peerCertificates = sslSession.getPeerCertificates();
481+
X509Certificate[] x509PeerCertificates = new X509Certificate[peerCertificates.length];
482+
for (int i = 0; i < peerCertificates.length; i++) {
483+
x509PeerCertificates[i] = (X509Certificate) peerCertificates[i];
484+
}
508485
checkServerTrustedMethod.invoke(x509ExtendedTrustManager, x509PeerCertificates,
509486
"RSA", new SslSocketWrapper((SSLSocket) socket, callOptions.getAuthority()));
510487
peerVerificationStatus = Status.OK;
488+
} catch (SSLPeerUnverifiedException | InvocationTargetException
489+
| IllegalAccessException e) {
490+
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
491+
String.format("Failure in verifying authority '%s' against peer during rpc",
492+
callOptions.getAuthority())).withCause(e);
511493
}
512-
} catch (SSLPeerUnverifiedException | InvocationTargetException
513-
| IllegalAccessException e) {
514-
peerVerificationStatus = Status.UNAVAILABLE.withDescription(
515-
String.format("Failure in verifying authority '%s' against peer during rpc",
516-
callOptions.getAuthority())).withCause(e);
494+
peerVerificationResults.put(callOptions.getAuthority(), peerVerificationStatus);
495+
}
496+
} catch (GeneralSecurityException e) {
497+
if (GrpcUtil.getFlag(GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK, false)) {
498+
return new FailingClientStream(Status.UNAVAILABLE.withDescription(
499+
"Failure getting X509ExtendedTrustManager from TlsCredentials").withCause(e),
500+
tracers);
517501
}
518-
peerVerificationResults.put(callOptions.getAuthority(), peerVerificationStatus);
519502
}
520503
}
521504
if (peerVerificationStatus != null && !peerVerificationStatus.isOk()) {
@@ -1610,7 +1593,7 @@ public void alternateService(int streamId, String origin, ByteString protocol, S
16101593
/**
16111594
* SSLSocket wrapper that provides a fake SSLSession for handshake session.
16121595
*/
1613-
static class SslSocketWrapper extends NoopSslSocket {
1596+
static final class SslSocketWrapper extends NoopSslSocket {
16141597

16151598
private final SSLSession sslSession;
16161599
private final SSLSocket sslSocket;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -817,7 +817,7 @@ public void perRpcAuthoritySpecified_hostnameVerification_SslSocket_successCase(
817817
@Test
818818
public void perRpcAuthoritySpecified_hostnameVerification_SslSocket_failureCase()
819819
throws Exception {
820-
System.setProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK", "true");
820+
OkHttpClientTransport.enablePerRpcAuthorityCheck = true;
821821
try {
822822
startTransport(
823823
DEFAULT_START_STREAM_ID, null, true, null,
@@ -833,7 +833,7 @@ public void perRpcAuthoritySpecified_hostnameVerification_SslSocket_failureCase(
833833
+ "cause=null}");
834834
shutdownAndVerify();
835835
} finally {
836-
System.clearProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK");
836+
OkHttpClientTransport.enablePerRpcAuthorityCheck = false;
837837
}
838838
}
839839

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@
6060
import javax.net.ssl.X509ExtendedTrustManager;
6161
import javax.net.ssl.X509TrustManager;
6262
import javax.security.auth.x500.X500Principal;
63+
64+
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
6365
import org.junit.Assume;
6466
import org.junit.Before;
6567
import org.junit.Rule;
@@ -69,6 +71,7 @@
6971

7072
/** Verify OkHttp's TLS integration. */
7173
@RunWith(JUnit4.class)
74+
@IgnoreJRERequirement
7275
public class TlsTest {
7376
@Rule
7477
public final GrpcCleanupRule grpcCleanupRule = new GrpcCleanupRule();
@@ -110,7 +113,7 @@ public void basicTls_succeeds() throws Exception {
110113

111114
@Test
112115
public void perRpcAuthorityOverride_checkServerTrustedIsCalled() throws Exception {
113-
System.setProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK", "true");
116+
OkHttpClientTransport.enablePerRpcAuthorityCheck = true;
114117
try {
115118
ServerCredentials serverCreds;
116119
try (InputStream serverCert = TlsTesting.loadCert("server1.pem");
@@ -137,7 +140,7 @@ public void perRpcAuthorityOverride_checkServerTrustedIsCalled() throws Exceptio
137140
SimpleRequest.getDefaultInstance());
138141
assertThat(fakeTrustManager.checkServerTrustedCalled).isTrue();
139142
} finally {
140-
System.clearProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK");
143+
OkHttpClientTransport.enablePerRpcAuthorityCheck = false;
141144
}
142145
}
143146

@@ -148,7 +151,7 @@ public void perRpcAuthorityOverride_checkServerTrustedIsCalled() throws Exceptio
148151
@Test
149152
public void perRpcAuthorityOverride_tlsCreds_noX509ExtendedTrustManager_fails()
150153
throws Exception {
151-
System.setProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK", "true");
154+
OkHttpClientTransport.enablePerRpcAuthorityCheck = true;
152155
try {
153156
ServerCredentials serverCreds;
154157
try (InputStream serverCert = TlsTesting.loadCert("server1.pem");
@@ -180,7 +183,7 @@ public void perRpcAuthorityOverride_tlsCreds_noX509ExtendedTrustManager_fails()
180183
+ "available");
181184
}
182185
} finally {
183-
System.clearProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK");
186+
OkHttpClientTransport.enablePerRpcAuthorityCheck = false;
184187
}
185188
}
186189

0 commit comments

Comments
 (0)