Save changes.

Author: kannanjgithub

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

@@ -38,14 +38,16 @@ private InternalProtocolNegotiators() {}
    * Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will
    * be negotiated, the {@code handler} is added and writes to the {@link io.netty.channel.Channel}
    * may happen immediately, even before the TLS Handshake is complete.
+   *
    * @param executorPool a dedicated {@link Executor} pool for time-consuming TLS tasks
+   * @param isXdsTarget
    */
   public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslContext,
-          ObjectPool<? extends Executor> executorPool,
-          Optional<Runnable> handshakeCompleteRunnable,
-          String sni) {
+                                                                  ObjectPool<? extends Executor> executorPool,
+                                                                  Optional<Runnable> handshakeCompleteRunnable,
+                                                                  String sni, boolean isXdsTarget) {
     final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.tls(sslContext,
-        executorPool, handshakeCompleteRunnable, null, sni);
+        executorPool, handshakeCompleteRunnable, null, sni, isXdsTarget);
     final class TlsNegotiator implements InternalProtocolNegotiator.ProtocolNegotiator {
 
       @Override
@@ -73,8 +75,8 @@ public void close() {
    * may happen immediately, even before the TLS Handshake is complete.
    */
   public static InternalProtocolNegotiator.ProtocolNegotiator tls(
-      SslContext sslContext, String sni) {
-    return tls(sslContext, null, Optional.absent(), sni);
+      SslContext sslContext, String sni, boolean isXdsTarget) {
+    return tls(sslContext, null, Optional.absent(), sni, isXdsTarget);
   }
 
   /**

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

@@ -652,7 +652,7 @@ static ProtocolNegotiator createProtocolNegotiatorByType(
       case PLAINTEXT_UPGRADE:
         return ProtocolNegotiators.plaintextUpgrade();
       case TLS:
-        return ProtocolNegotiators.tls(sslContext, executorPool, Optional.absent(), null, null);
+        return ProtocolNegotiators.tls(sslContext, executorPool, Optional.absent(), null, null, false);
       default:
         throw new IllegalArgumentException("Unsupported negotiationType: " + negotiationType);
     }

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.common.base.Strings;
 import com.google.errorprone.annotations.ForOverride;
 import io.grpc.Attributes;
 import io.grpc.CallCredentials;
@@ -583,7 +582,7 @@ public ClientTlsProtocolNegotiator(SslContext sslContext,
                                        ObjectPool<? extends Executor> executorPool,
                                        Optional<Runnable> handshakeCompleteRunnable,
                                        X509TrustManager x509ExtendedTrustManager,
-                                       String sni) {
+                                       String sni, boolean isXdsTarget) {
       this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext");
       this.executorPool = executorPool;
       if (this.executorPool != null) {
@@ -592,13 +591,15 @@ public ClientTlsProtocolNegotiator(SslContext sslContext,
       this.handshakeCompleteRunnable = handshakeCompleteRunnable;
       this.x509ExtendedTrustManager = x509ExtendedTrustManager;
       this.sni = sni;
+      this.isXdsTarget = isXdsTarget;
     }
 
     private final SslContext sslContext;
     private final ObjectPool<? extends Executor> executorPool;
     private final Optional<Runnable> handshakeCompleteRunnable;
     private final X509TrustManager x509ExtendedTrustManager;
     private final String sni;
+    private final boolean isXdsTarget;
     private Executor executor;
 
     @Override
@@ -611,7 +612,7 @@ public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
       ChannelHandler gnh = new GrpcNegotiationHandler(grpcHandler);
       ChannelLogger negotiationLogger = grpcHandler.getNegotiationLogger();
       ChannelHandler cth = new ClientTlsHandler(gnh, sslContext,
-          !Strings.isNullOrEmpty(sni) ? sni : grpcHandler.getAuthority(),
+          isXdsTarget ? sni : grpcHandler.getAuthority(),
           this.executor, negotiationLogger, handshakeCompleteRunnable, null,
           x509ExtendedTrustManager);
       return new WaitUntilActiveHandler(cth, negotiationLogger);
@@ -753,13 +754,14 @@ static HostPort parseAuthority(String authority) {
    * Returns a {@link ProtocolNegotiator} that ensures the pipeline is set up so that TLS will
    * be negotiated, the {@code handler} is added and writes to the {@link io.netty.channel.Channel}
    * may happen immediately, even before the TLS Handshake is complete.
+   *
    * @param executorPool a dedicated {@link Executor} pool for time-consuming TLS tasks
    */
   public static ProtocolNegotiator tls(SslContext sslContext,
-      ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable,
-      X509TrustManager x509ExtendedTrustManager, String sni) {
+                                       ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable,
+                                       X509TrustManager x509ExtendedTrustManager, String sni, boolean isXdsTarget) {
     return new ClientTlsProtocolNegotiator(sslContext, executorPool, handshakeCompleteRunnable,
-        x509ExtendedTrustManager, sni);
+        x509ExtendedTrustManager, sni, isXdsTarget);
   }
 
   /**
@@ -769,7 +771,7 @@ public static ProtocolNegotiator tls(SslContext sslContext,
    */
   public static ProtocolNegotiator tls(SslContext sslContext,
       X509TrustManager x509ExtendedTrustManager) {
-    return tls(sslContext, null, Optional.absent(), x509ExtendedTrustManager, null);
+    return tls(sslContext, null, Optional.absent(), x509ExtendedTrustManager, null, false);
   }
 
   public static ProtocolNegotiator.ClientFactory tlsClientFactory(SslContext sslContext,

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

@@ -836,7 +836,7 @@ public void tlsNegotiationServerExecutorShouldSucceed() throws Exception {
         .keyManager(clientCert, clientKey)
         .build();
     ProtocolNegotiator negotiator = ProtocolNegotiators.tls(clientContext, clientExecutorPool,
-        Optional.absent(), null, null);
+        Optional.absent(), null, null, false);
     // after starting the client, the Executor in the client pool should be used
     assertEquals(true, clientExecutorPool.isInUse());
     final NettyClientTransport transport = newTransport(negotiator);

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

@@ -1271,7 +1271,7 @@ public void clientTlsHandler_firesNegotiation() throws Exception {
     }
     FakeGrpcHttp2ConnectionHandler gh = FakeGrpcHttp2ConnectionHandler.newHandler();
     ClientTlsProtocolNegotiator pn = new ClientTlsProtocolNegotiator(clientSslContext,
-        null, Optional.absent(), null, null);
+        null, Optional.absent(), null, null, false);
     WriteBufferingAndExceptionHandler clientWbaeh =
         new WriteBufferingAndExceptionHandler(pn.newHandler(gh));
 

s2a/src/main/java/io/grpc/s2a/internal/handshaker/S2AProtocolNegotiatorFactory.java

@@ -38,7 +38,6 @@
 import io.grpc.netty.InternalProtocolNegotiator.ProtocolNegotiator;
 import io.grpc.netty.InternalProtocolNegotiators;
 import io.grpc.netty.InternalProtocolNegotiators.ProtocolNegotiationHandler;
-import io.grpc.s2a.internal.handshaker.S2AIdentity;
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandlerAdapter;
 import io.netty.channel.ChannelHandlerContext;
@@ -260,7 +259,7 @@ public void run() {
                               s2aStub.close();
                             }
                           }),
-                          null)
+                          null, false)
                       .newHandler(grpcHandler);
 
               // Delegate the rest of the handshake to the TLS handler. and remove the 

xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java

@@ -54,9 +54,6 @@
 @VisibleForTesting
 public final class SecurityProtocolNegotiators {
 
-  static boolean useChannelAuthorityIfNoSniApplicable =
-          GrpcUtil.getFlag("GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE", false);
-
   /** Name associated with individual address, if available (e.g., DNS name). */
   @EquivalentAddressGroup.Attr
   public static final Attributes.Key<String> ATTR_ADDRESS_NAME =
@@ -196,6 +193,8 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
   @VisibleForTesting
   static final class ClientSecurityHandler
       extends InternalProtocolNegotiators.ProtocolNegotiationHandler {
+    static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
+
     private final GrpcHttp2ConnectionHandler grpcHandler;
     private final SslContextProviderSupplier sslContextProviderSupplier;
     private final String sni;
@@ -219,12 +218,12 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
       this.sslContextProviderSupplier = sslContextProviderSupplier;
       EnvoyServerProtoData.BaseTlsContext tlsContext = sslContextProviderSupplier.getTlsContext();
       UpstreamTlsContext upstreamTlsContext = ((UpstreamTlsContext) tlsContext);
-      String sniVal = upstreamTlsContext.getAutoHostSni() && !Strings.isNullOrEmpty(endpointHostname)
-              ? endpointHostname : upstreamTlsContext.getSni();
-      if (Strings.isNullOrEmpty(sniVal) && useChannelAuthorityIfNoSniApplicable) {
-        sniVal = grpcHandler.getAuthority();
+      if (isXdsSniEnabled) {
+        sni = upstreamTlsContext.getAutoHostSni() && !Strings.isNullOrEmpty(endpointHostname)
+            ? endpointHostname : upstreamTlsContext.getSni();
+      } else {
+        sni = grpcHandler.getAuthority();
       }
-      sni = sniVal;
     }
 
     @VisibleForTesting
@@ -250,7 +249,7 @@ public void updateSslContext(SslContext sslContext) {
                   "ClientSecurityHandler.updateSslContext authority={0}, ctx.name={1}",
                   new Object[]{grpcHandler.getAuthority(), ctx.name()});
               ChannelHandler handler =
-                  InternalProtocolNegotiators.tls(sslContext, sni).newHandler(grpcHandler);
+                  InternalProtocolNegotiators.tls(sslContext, sni, true).newHandler(grpcHandler);
 
               // Delegate rest of handshake to TLS handler
               ctx.pipeline().addAfter(ctx.name(), null, handler);

xds/src/main/java/io/grpc/xds/internal/security/trust/XdsX509TrustManager.java

@@ -27,6 +27,7 @@
 import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext;
 import io.envoyproxy.envoy.type.matcher.v3.RegexMatcher;
 import io.envoyproxy.envoy.type.matcher.v3.StringMatcher;
+import io.grpc.internal.GrpcUtil;
 import io.grpc.internal.SpiffeUtil;
 import java.net.Socket;
 import java.security.cert.CertificateException;
@@ -52,6 +53,8 @@
  */
 final class XdsX509TrustManager extends X509ExtendedTrustManager implements X509TrustManager {
 
+  static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
+
   // ref: io.grpc.okhttp.internal.OkHostnameVerifier and
   // sun.security.x509.GeneralNameInterface
   private static final int ALT_DNS_NAME = 2;
@@ -217,7 +220,7 @@ void verifySubjectAltNameInChain(X509Certificate[] peerCertChain) throws Certifi
       return;
     }
     @SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
-    List<StringMatcher> verifyList = !Strings.isNullOrEmpty(sniForSanMatching)
+    List<StringMatcher> verifyList = isXdsSniEnabled && !Strings.isNullOrEmpty(sniForSanMatching)
             ? ImmutableList.of(StringMatcher.newBuilder().setExact(sniForSanMatching).build())
             : certContext.getMatchSubjectAltNamesList();
     if (verifyList.isEmpty()) {

xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java

@@ -282,10 +282,6 @@ public void tlsClientServer_useSystemRootCerts_mtls() throws Exception {
     }
   }
 
-  /**
-   * Use system root ca cert for TLS channel - no mTLS.
-   * Subj Alt Names to match are specified in the validation context.
-   */
   @Test
   public void tlsClientServer_noAutoSniValidation_failureToMatchSubjAltNames()
       throws Exception {
@@ -346,7 +342,7 @@ public void tlsClientServer_autoSniValidation_sniInUTC()
   }
 
   @Test
-  public void tlsClientServer_sni_san_validation_from_hostname()
+  public void tlsClientServer_autoSniValidation_sniFromHostname()
       throws Exception {
     Path trustStoreFilePath = getCacertFilePathForTestCa();
     try {