Address review comments.

Author: kannanjgithub

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

@@ -60,9 +60,11 @@
 import io.netty.util.concurrent.GenericFutureListener;
 import java.net.SocketAddress;
 import java.nio.channels.ClosedChannelException;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
@@ -71,8 +73,6 @@
  */
 class NettyClientTransport implements ConnectionClientTransport {
 
-  private static final boolean enablePerRpcAuthorityCheck =
-          GrpcUtil.getFlag("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK", false);
   private final InternalLogId logId;
   private final Map<ChannelOption<?>, ?> channelOptions;
   private final SocketAddress remoteAddress;
@@ -108,6 +108,14 @@ class NettyClientTransport implements ConnectionClientTransport {
   private final ChannelLogger channelLogger;
   private final boolean useGetForSafeMethods;
   private final Ticker ticker;
+  private final Logger logger = Logger.getLogger(NettyClientTransport.class.getName());
+  private final LinkedHashMap<String, Status> peerVerificationResults =
+      new LinkedHashMap<String, Status>() {
+        @Override
+        protected boolean removeEldestEntry(Map.Entry<String, Status> eldest) {
+          return size() > 100;
+        }
+      };
 
   NettyClientTransport(
       SocketAddress address,
@@ -198,9 +206,19 @@ public ClientStream newStream(
       return new FailingClientStream(statusExplainingWhyTheChannelIsNull, tracers);
     }
     if (callOptions.getAuthority() != null) {
-      Status verificationStatus = negotiator.verifyAuthority(callOptions.getAuthority());
+      Status verificationStatus = peerVerificationResults.get(callOptions.getAuthority());
+      if (verificationStatus == null) {
+        verificationStatus = negotiator.verifyAuthority(callOptions.getAuthority());
+        if (!verificationStatus.isOk()) {
+          logger.log(Level.WARNING, String.format("Peer hostname verification during rpc failed "
+                          + "for authority '%s' for method '%s' with the error \"%s\". This will "
+                          + "be an error in the future.", callOptions.getAuthority(),
+                  method.getFullMethodName(), verificationStatus.getDescription()),
+                  verificationStatus.getCause());
+        }
+      }
       if (!verificationStatus.isOk()) {
-        if (enablePerRpcAuthorityCheck) {
+        if (GrpcUtil.getFlag("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK", false)) {
           return new FailingClientStream(verificationStatus, tracers);
         }
       }

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

@@ -21,7 +21,6 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
-import com.google.common.base.Preconditions;
 import com.google.errorprone.annotations.ForOverride;
 import io.grpc.Attributes;
 import io.grpc.CallCredentials;
@@ -84,16 +83,13 @@
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.EnumSet;
-import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Executor;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLParameters;
@@ -589,31 +585,27 @@ protected void userEventTriggered0(ChannelHandlerContext ctx, Object evt) throws
   }
 
   static final class ClientTlsProtocolNegotiator implements ProtocolNegotiator {
-    private static final Logger logger = Logger.getLogger(ClientTlsProtocolNegotiator.class.getName());
+    private static final Logger logger =
+            Logger.getLogger(ClientTlsProtocolNegotiator.class.getName());
     private static final Method checkServerTrustedMethod;
+
     static {
       Method method = null;
       try {
-        Class<?> x509ExtendedTrustManagerClass = Class.forName("javax.net.ssl.X509ExtendedTrustManager");
+        Class<?> x509ExtendedTrustManagerClass =
+                Class.forName("javax.net.ssl.X509ExtendedTrustManager");
         method = x509ExtendedTrustManagerClass.getMethod("checkServerTrusted",
                 X509Certificate[].class, String.class, SSLEngine.class);
       } catch (ClassNotFoundException e) {
+        // Per-rpc authority overriding via call options will be disallowed.
       } catch (NoSuchMethodException e) {
         // Should never happen.
-        logger.log(Level.WARNING, "Method checkServerTrusted not found in " +
-                "javax.net.ssl.X509ExtendedTrustManager", e);
+        logger.log(Level.WARNING, "Method checkServerTrusted not found in "
+                + "javax.net.ssl.X509ExtendedTrustManager", e);
       }
       checkServerTrustedMethod = method;
     }
 
-    @GuardedBy("this")
-    private final LinkedHashMap<String, Status> peerVerificationResults =
-            new LinkedHashMap<String, Status>() {
-          @Override
-          protected boolean removeEldestEntry(Map.Entry<String, Status> eldest) {
-            return size() > 100;
-          }
-        };
     private SSLEngine sslEngine;
 
     public ClientTlsProtocolNegotiator(SslContext sslContext,
@@ -656,32 +648,25 @@ public void close() {
     }
 
     @Override
-    public synchronized Status verifyAuthority(@Nonnull String authority) {
+    public Status verifyAuthority(@Nonnull String authority) {
       // sslEngine won't be set when creating ClientTlsHandler from InternalProtocolNegotiators
       // for example.
       if (sslEngine == null || x509ExtendedTrustManager == null) {
         return Status.FAILED_PRECONDITION.withDescription(
                 "Can't allow authority override in rpc when SslEngine or X509ExtendedTrustManager"
                         + " is not available");
       }
-      if (peerVerificationResults.containsKey(authority)) {
-        return peerVerificationResults.get(authority);
-      } else {
-        Status peerVerificationStatus;
-        try {
-          verifyAuthorityAllowedForPeerCert(authority);
-          peerVerificationStatus = Status.OK;
-        } catch (SSLPeerUnverifiedException | CertificateException | InvocationTargetException |
-                 IllegalAccessException | IllegalStateException e) {
-          peerVerificationStatus = Status.UNAVAILABLE.withDescription(
-                  String.format("Peer hostname verification during rpc failed for authority '%s'",
-                  authority)).withCause(e);
-          logger.log(Level.WARNING, "Authority verification failed (this will be an error in the "
-                  + "future).", e);
-        }
-        peerVerificationResults.put(authority, peerVerificationStatus);
-        return peerVerificationStatus;
+      Status peerVerificationStatus;
+      try {
+        verifyAuthorityAllowedForPeerCert(authority);
+        peerVerificationStatus = Status.OK;
+      } catch (SSLPeerUnverifiedException | CertificateException | InvocationTargetException
+               | IllegalAccessException | IllegalStateException e) {
+        peerVerificationStatus = Status.UNAVAILABLE.withDescription(
+                String.format("Peer hostname verification during rpc failed for authority '%s'",
+                authority)).withCause(e);
       }
+      return peerVerificationStatus;
     }
 
     public void setSslEngine(SSLEngine sslEngine) {
@@ -692,7 +677,8 @@ private void verifyAuthorityAllowedForPeerCert(String authority)
             throws SSLPeerUnverifiedException, CertificateException, InvocationTargetException,
             IllegalAccessException {
       if (checkServerTrustedMethod == null) {
-        throw new IllegalStateException("Method checkServerTrusted not found in javax.net.ssl.X509ExtendedTrustManager");
+        throw new IllegalStateException("Method checkServerTrusted not found in "
+                + "javax.net.ssl.X509ExtendedTrustManager");
       }
       SSLEngine sslEngineWrapper = new SslEngineWrapper(sslEngine, authority);
       // The typecasting of Certificate to X509Certificate should work because this method will only
@@ -702,7 +688,8 @@ private void verifyAuthorityAllowedForPeerCert(String authority)
       for (int i = 0; i < peerCertificates.length; i++) {
         x509PeerCertificates[i] = (X509Certificate) peerCertificates[i];
       }
-      checkServerTrustedMethod.invoke(x509ExtendedTrustManager, x509PeerCertificates, "RSA", sslEngineWrapper);
+      checkServerTrustedMethod.invoke(
+              x509ExtendedTrustManager, x509PeerCertificates, "RSA", sslEngineWrapper);
     }
 
     @VisibleForTesting

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

@@ -853,7 +853,8 @@ public void authorityOverrideInCallOptions_noX509ExtendedTrustManager_newStreamC
     try {
       startServer();
       InputStream caCert = TlsTesting.loadCert("ca.pem");
-      X509TrustManager x509ExtendedTrustManager = (X509TrustManager) getX509ExtendedTrustManager(caCert);
+      X509TrustManager x509ExtendedTrustManager =
+              (X509TrustManager) getX509ExtendedTrustManager(caCert);
       ProtocolNegotiators.FromChannelCredentialsResult result =
               ProtocolNegotiators.from(TlsChannelCredentials.newBuilder()
                       .trustManager(new FakeTrustManager(x509ExtendedTrustManager)).build());
@@ -874,8 +875,9 @@ Rpc.METHOD, new Metadata(), CallOptions.DEFAULT.withAuthority("foo.test.google.i
       InsightBuilder insightBuilder = new InsightBuilder();
       stream.appendTimeoutInsight(insightBuilder);
       assertThat(insightBuilder.toString()).contains(
-              "Status{code=FAILED_PRECONDITION, description=Can't allow authority override in rpc when "
-                      + "SslEngine or X509ExtendedTrustManager is not available, cause=null}");
+              "Status{code=FAILED_PRECONDITION, description=Can't allow authority override in rpc "
+                      + "when SslEngine or X509ExtendedTrustManager is not available, "
+                      + "cause=null}");
     } finally {
       System.clearProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK");
     }
@@ -904,9 +906,10 @@ Rpc.METHOD, new Metadata(), CallOptions.DEFAULT.withAuthority("foo.test.google.i
       InsightBuilder insightBuilder = new InsightBuilder();
       stream.appendTimeoutInsight(insightBuilder);
       assertThat(insightBuilder.toString()).contains(
-              "Status{code=UNAVAILABLE, description=Peer hostname verification during rpc failed for"
-                      + " authority 'foo.test.google.in'");
-      assertThat(insightBuilder.toString()).contains("cause=java.security.cert.CertificateException:"
+              "Status{code=UNAVAILABLE, description=Peer hostname verification during rpc failed "
+                      + "for authority 'foo.test.google.in'");
+      assertThat(insightBuilder.toString()).contains(
+              "Caused by: java.security.cert.CertificateException:"
               + " No subject alternative DNS name matching foo.test.google.in found.");
     } finally {
       System.clearProperty("GRPC_ENABLE_PER_RPC_AUTHORITY_CHECK");

netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java

@@ -26,12 +26,10 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 
 import com.google.common.base.Optional;
 import io.grpc.Attributes;
@@ -69,7 +67,6 @@
 import io.grpc.netty.ProtocolNegotiators.ClientTlsProtocolNegotiator;
 import io.grpc.netty.ProtocolNegotiators.HostPort;
 import io.grpc.netty.ProtocolNegotiators.ServerTlsHandler;
-import io.grpc.netty.ProtocolNegotiators.SslEngineWrapper;
 import io.grpc.netty.ProtocolNegotiators.WaitUntilActiveHandler;
 import io.grpc.testing.TlsTesting;
 import io.grpc.util.CertificateUtils;
@@ -122,7 +119,6 @@
 import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.NoSuchAlgorithmException;
-import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.ArrayDeque;
@@ -144,7 +140,6 @@
 import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.SSLSession;
 import javax.net.ssl.TrustManagerFactory;
-import javax.net.ssl.X509ExtendedTrustManager;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -1037,44 +1032,6 @@ private ClientTlsProtocolNegotiator getClientTlsProtocolNegotiator() throws SSLE
         null, Optional.absent(), null);
   }
 
-  @Test
-  public void allowedAuthoritiesForTransport_LruCache() throws SSLException, CertificateException {
-    X509ExtendedTrustManager mockX509ExtendedTrustManager = mock(X509ExtendedTrustManager.class);
-    ClientTlsProtocolNegotiator negotiator = new ClientTlsProtocolNegotiator(
-        GrpcSslContexts.forClient().trustManager(
-            TlsTesting.loadCert("ca.pem")).build(),
-        null, Optional.absent(), mockX509ExtendedTrustManager);
-    SSLEngine mockSslEngine = mock(SSLEngine.class);
-    negotiator.setSslEngine(mockSslEngine);
-    SSLSession mockSslSession = mock(SSLSession.class);
-    when(mockSslEngine.getSession()).thenReturn(mockSslSession);
-    when(mockSslSession.getPeerCertificates()).thenReturn(new Certificate[0]);
-
-    // Fill the cache
-    for (int i = 0; i < 100; i++) {
-      Status unused = negotiator.verifyAuthority("authority" + i);
-    }
-    // Should use value from the cache.
-    Status unused = negotiator.verifyAuthority("authority0");
-    // Should evict authority0.
-    unused = negotiator.verifyAuthority("authority100");
-    // Should call TrustManager as the cached value has been evicted for this authority value.
-    unused = negotiator.verifyAuthority("authority0");
-
-    ArgumentCaptor<SslEngineWrapper> sslEngineWrapperArgumentCaptor =
-        ArgumentCaptor.forClass(SslEngineWrapper.class);
-    verify(mockX509ExtendedTrustManager, times(102)).checkServerTrusted(any(), eq("RSA"),
-        sslEngineWrapperArgumentCaptor.capture());
-    List<SslEngineWrapper> sslEngineWrappersCaptured =
-        sslEngineWrapperArgumentCaptor.getAllValues();
-    assertThat(sslEngineWrappersCaptured).hasSize(102);
-    for (int i = 0; i < 100; i++) {
-      assertThat(sslEngineWrappersCaptured.get(i).getPeerHost()).isEqualTo("authority" + i);
-    }
-    assertThat(sslEngineWrappersCaptured.get(100).getPeerHost()).isEqualTo("authority100");
-    assertThat(sslEngineWrappersCaptured.get(101).getPeerHost()).isEqualTo("authority0");
-  }
-
   @Test
   public void engineLog() {
     ChannelHandler handler = new ServerTlsHandler(grpcHandler, sslContext, null);