Address review comments.

Author: kannanjgithub

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

@@ -1,3 +1,19 @@
+/*
+ * Copyright 2024 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 package io.grpc.internal;
 
 import java.io.IOException;
@@ -16,6 +32,9 @@
 import javax.net.ssl.X509ExtendedTrustManager;
 import javax.security.auth.x500.X500Principal;
 
+/**
+ * Contains certificate/key PEM file utility method(s) for internal usage.
+ */
 public class CertificateUtils {
   /**
    * Creates a X509ExtendedTrustManager using the provided CA certs if applicable for the

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

@@ -45,7 +45,6 @@
 import io.grpc.internal.StatsTraceContext;
 import io.grpc.internal.TransportTracer;
 import io.grpc.netty.NettyChannelBuilder.LocalSocketPicker;
-import io.grpc.netty.ProtocolNegotiators.ClientTlsProtocolNegotiator;
 import io.netty.bootstrap.Bootstrap;
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelFactory;
@@ -63,7 +62,6 @@
 import java.nio.channels.ClosedChannelException;
 import java.security.cert.CertificateException;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
@@ -112,8 +110,6 @@ class NettyClientTransport implements ConnectionClientTransport {
   private final ChannelLogger channelLogger;
   private final boolean useGetForSafeMethods;
   private final Ticker ticker;
-  private final ConcurrentHashMap<String, Boolean> authoritiesAllowedForPeer =
-      new ConcurrentHashMap<>();
 
   NettyClientTransport(
       SocketAddress address,
@@ -206,22 +202,23 @@ public ClientStream newStream(
     try {
       if (callOptions.getAuthority() != null && !negotiator.mayBeVerifyAuthority(
           callOptions.getAuthority())) {
-        logger.log(Level.FINE, "Peer hostname verification failed for authority '{}'.",
+        String errMsg = String.format("Peer hostname verification failed for authority '%s'.",
             callOptions.getAuthority());
-        return new FailingClientStream(Status.INTERNAL.withDescription(
-            "Peer hostname verification failed for authority"), tracers);
+        logger.log(Level.FINE, errMsg);
+        return new FailingClientStream(Status.INTERNAL.withDescription(errMsg), tracers);
       }
     } catch (UnsupportedOperationException ex) {
-      logger.log(Level.FINE, "Can't allow authority override in rpc when X509ExtendedTrustManager is not available.",
+      logger.log(Level.FINE,
+          "Can't allow authority override in rpc when X509ExtendedTrustManager is not available.",
           callOptions.getAuthority());
       return new FailingClientStream(Status.INTERNAL.withDescription(
           "Can't allow authority override in rpc when X509ExtendedTrustManager is not available"),
           tracers);
     } catch (SSLPeerUnverifiedException | CertificateException ex) {
-      logger.log(Level.FINE, "Peer hostname verification failed for authority '{}'.",
+      String errMsg = String.format("Peer hostname verification failed for authority '%s'.",
           callOptions.getAuthority());
-      return new FailingClientStream(Status.INTERNAL.withDescription(
-          "Peer hostname verification failed for authority"), tracers);
+      logger.log(Level.FINE, errMsg, ex);
+      return new FailingClientStream(Status.INTERNAL.withDescription(errMsg), tracers);
     }
     StatsTraceContext statsTraceCtx =
         StatsTraceContext.newClientContext(tracers, getAttributes(), headers);

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

@@ -69,11 +69,12 @@ interface ServerFactory {
   /**
    * Verify the authority against peer if applicable depending on the transport credential type.
    * @throws UnsupportedOperationException if the verification should happen but the required
-   * type of TrustManager could not be found.
+   *     type of TrustManager could not be found.
    * @throws SSLPeerUnverifiedException if peer verification failed
    * @throws CertificateException if certificates have a problem
    */
-  default boolean mayBeVerifyAuthority(String authority) throws SSLPeerUnverifiedException, CertificateException {
+  default boolean mayBeVerifyAuthority(String authority)
+      throws SSLPeerUnverifiedException, CertificateException {
     return true;
   }
 }

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

@@ -42,7 +42,6 @@
 import io.grpc.Status;
 import io.grpc.TlsChannelCredentials;
 import io.grpc.TlsServerCredentials;
-import io.grpc.internal.FailingClientStream;
 import io.grpc.internal.GrpcAttributes;
 import io.grpc.internal.GrpcUtil;
 import io.grpc.internal.ObjectPool;
@@ -81,6 +80,7 @@
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.EnumSet;
+import java.util.LinkedHashMap;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Executor;
@@ -574,6 +574,8 @@ protected void userEventTriggered0(ChannelHandlerContext ctx, Object evt) throws
 
   static final class ClientTlsProtocolNegotiator implements ProtocolNegotiator {
 
+    private static final int MAX_AUTHORITIES_CACHE_SIZE = 100;
+    private final LinkedHashMap<String, Boolean> authoritiesAllowedForPeer = new LinkedHashMap<>();
     private SSLEngine sslEngine;
 
     public ClientTlsProtocolNegotiator(SslContext sslContext,
@@ -615,13 +617,43 @@ public void close() {
       }
     }
 
+    @Override
+    synchronized public boolean mayBeVerifyAuthority(@Nonnull String authority) {
+      if (!canVerifyAuthorityOverride()) {
+        throw new UnsupportedOperationException(
+            "Can't allow authority override in rpc when X509ExtendedTrustManager is not"
+                + "available.");
+      }
+      boolean peerVerified;
+      if (authoritiesAllowedForPeer.containsKey(authority)) {
+        peerVerified = authoritiesAllowedForPeer.get(authority);
+      } else {
+        try {
+          verifyAuthorityAllowedForPeerCert(authority);
+          peerVerified = true;
+        } catch (SSLPeerUnverifiedException | CertificateException e) {
+          peerVerified = false;
+        }
+        authoritiesAllowedForPeer.put(authority, peerVerified);
+        if (authoritiesAllowedForPeer.size() > MAX_AUTHORITIES_CACHE_SIZE) {
+          authoritiesAllowedForPeer.remove(
+              authoritiesAllowedForPeer.entrySet().iterator().next().getKey());
+        }
+      }
+      return peerVerified;
+    }
+
+    public void setSslEngine(SSLEngine sslEngine) {
+      this.sslEngine = sslEngine;
+    }
+
     boolean canVerifyAuthorityOverride() {
       // sslEngine won't be set when creating ClientTlsHandlder from InternalProtocolNegotiators
       // for example.
       return sslEngine != null && x509ExtendedTrustManager != null;
     }
 
-    public void verifyAuthorityAllowedForPeerCert(String authority)
+    private void verifyAuthorityAllowedForPeerCert(String authority)
         throws SSLPeerUnverifiedException, CertificateException {
       SSLEngine sslEngineWrapper = new SslEngineWrapper(sslEngine, authority);
       // The typecasting of Certificate to X509Certificate should work because this method will only
@@ -634,43 +666,10 @@ public void verifyAuthorityAllowedForPeerCert(String authority)
       x509ExtendedTrustManager.checkServerTrusted(x509PeerCertificates, "RSA", sslEngineWrapper);
     }
 
-    public void setSslEngine(SSLEngine sslEngine) {
-      this.sslEngine = sslEngine;
-    }
-
     @VisibleForTesting
     boolean hasX509ExtendedTrustManager() {
       return x509ExtendedTrustManager != null;
     }
-
-    public boolean mayBeVerifyAuthority(@Nonnull String authority) {
-        ClientTlsProtocolNegotiator clientTlsProtocolNegotiator =
-            (ClientTlsProtocolNegotiator) negotiator;
-        if (!clientTlsProtocolNegotiator.canVerifyAuthorityOverride()) {
-          return new FailingClientStream(Status.INTERNAL.withDescription(
-              "Can't allow authority override in rpc when X509ExtendedTrustManager is not available"),
-              tracers);
-        }
-        boolean peerVerified;
-        if (authoritiesAllowedForPeer.containsKey(callOptions.getAuthority())) {
-          peerVerified = authoritiesAllowedForPeer.get(callOptions.getAuthority());
-        } else {
-          try {
-            clientTlsProtocolNegotiator.verifyAuthorityAllowedForPeerCert(
-                callOptions.getAuthority());
-            peerVerified = true;
-          } catch (SSLPeerUnverifiedException | CertificateException e) {
-            peerVerified = false;
-            logger.log(Level.FINE, "Peer hostname verification failed for authority '{}'.",
-                callOptions.getAuthority());
-          }
-          authoritiesAllowedForPeer.put(callOptions.getAuthority(), peerVerified);
-        }
-        if (!peerVerified) {
-          return new FailingClientStream(Status.INTERNAL.withDescription(
-              "Peer hostname verification failed for authority"), tracers);
-        }
-      }
   }
 
   static final class ClientTlsHandler extends ProtocolNegotiationHandler {

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

@@ -891,8 +891,8 @@ Rpc.METHOD, new Metadata(), CallOptions.DEFAULT.withAuthority("foo.test.google.i
     InsightBuilder insightBuilder = new InsightBuilder();
     stream.appendTimeoutInsight(insightBuilder);
     assertThat(insightBuilder.toString()).contains(
-        "Status{code=INTERNAL, description=Peer hostname verification failed for authority, "
-            + "cause=null}");
+        "Status{code=INTERNAL, description=Peer hostname verification failed for authority"
+            + " 'foo.test.google.in'., cause=null}");
   }
 
   @Test

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

@@ -26,10 +26,12 @@
 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 io.grpc.Attributes;
 import io.grpc.CallCredentials;
@@ -66,6 +68,7 @@
 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;
@@ -118,6 +121,7 @@
 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;
@@ -140,6 +144,7 @@
 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;
@@ -1032,6 +1037,44 @@ private ClientTlsProtocolNegotiator getClientTlsProtocolNegotiator() throws SSLE
         null, Optional.empty(), 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.empty(), 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++) {
+      boolean unused = negotiator.mayBeVerifyAuthority("authority" + i);
+    }
+    // Should use value from the cache.
+    boolean unused = negotiator.mayBeVerifyAuthority("authority0");
+    // Should evict authority0.
+    unused = negotiator.mayBeVerifyAuthority("authority100");
+    // Should call TrustManager as the cached value has been evicted for this authority value.
+    unused = negotiator.mayBeVerifyAuthority("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);