In-progress review comments.

Author: kannanjgithub

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

@@ -0,0 +1,52 @@
+package io.grpc.internal;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Optional;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509ExtendedTrustManager;
+import javax.security.auth.x500.X500Principal;
+
+public class CertificateUtils {
+  /**
+   * Creates a X509ExtendedTrustManager using the provided CA certs if applicable for the
+   * certificate type.
+   */
+  public static Optional<TrustManager> getX509ExtendedTrustManager(InputStream rootCerts)
+      throws GeneralSecurityException {
+    KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());
+    try {
+      ks.load(null, null);
+    } catch (IOException ex) {
+      // Shouldn't really happen, as we're not loading any data.
+      throw new GeneralSecurityException(ex);
+    }
+    X509Certificate[] certs = CertificateUtils.getX509Certificates(rootCerts);
+    for (X509Certificate cert : certs) {
+      X500Principal principal = cert.getSubjectX500Principal();
+      ks.setCertificateEntry(principal.getName("RFC2253"), cert);
+    }
+
+    TrustManagerFactory trustManagerFactory =
+        TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+    trustManagerFactory.init(ks);
+    return Arrays.stream(trustManagerFactory.getTrustManagers())
+        .filter(trustManager -> trustManager instanceof X509ExtendedTrustManager).findFirst();
+  }
+
+  private static X509Certificate[] getX509Certificates(InputStream inputStream)
+      throws CertificateException {
+    CertificateFactory factory = CertificateFactory.getInstance("X.509");
+    Collection<? extends Certificate> certs = factory.generateCertificates(inputStream);
+    return certs.toArray(new X509Certificate[0]);
+  }
+}

examples/example-tls/build.gradle

@@ -30,7 +30,6 @@ def protocVersion = '3.25.5'
 dependencies {
     implementation "io.grpc:grpc-protobuf:${grpcVersion}"
     implementation "io.grpc:grpc-stub:${grpcVersion}"
-    implementation "io.grpc:grpc-api:${grpcVersion}"
     compileOnly "org.apache.tomcat:annotations-api:6.0.53"
     runtimeOnly "io.grpc:grpc-netty-shaded:${grpcVersion}"
 }
@@ -75,8 +74,6 @@ application {
     applicationDistribution.into('bin') {
         from(helloWorldTlsServer)
         from(helloWorldTlsClient)
-        filePermissions {
-            unix(0755)
-        }
+        fileMode = 0755
     }
 }

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

@@ -203,32 +203,25 @@ public ClientStream newStream(
     if (channel == null) {
       return new FailingClientStream(statusExplainingWhyTheChannelIsNull, tracers);
     }
-    if (negotiator instanceof ClientTlsProtocolNegotiator && callOptions.getAuthority() != null) {
-      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) {
+    try {
+      if (callOptions.getAuthority() != null && !negotiator.mayBeVerifyAuthority(
+          callOptions.getAuthority())) {
+        logger.log(Level.FINE, "Peer hostname verification failed for authority '{}'.",
+            callOptions.getAuthority());
         return new FailingClientStream(Status.INTERNAL.withDescription(
             "Peer hostname verification failed for authority"), tracers);
       }
+    } catch (UnsupportedOperationException ex) {
+      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 '{}'.",
+          callOptions.getAuthority());
+      return new FailingClientStream(Status.INTERNAL.withDescription(
+          "Peer hostname verification failed for authority"), tracers);
     }
     StatsTraceContext statsTraceCtx =
         StatsTraceContext.newClientContext(tracers, getAttributes(), headers);

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

@@ -19,7 +19,9 @@
 import io.grpc.internal.ObjectPool;
 import io.netty.channel.ChannelHandler;
 import io.netty.util.AsciiString;
+import java.security.cert.CertificateException;
 import java.util.concurrent.Executor;
+import javax.net.ssl.SSLPeerUnverifiedException;
 
 /**
  * An class that provides a Netty handler to control protocol negotiation.
@@ -63,4 +65,15 @@ interface ServerFactory {
      */
     ProtocolNegotiator newNegotiator(ObjectPool<? extends Executor> offloadExecutorPool);
   }
+
+  /**
+   * 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.
+   * @throws SSLPeerUnverifiedException if peer verification failed
+   * @throws CertificateException if certificates have a problem
+   */
+  default boolean mayBeVerifyAuthority(String authority) throws SSLPeerUnverifiedException, CertificateException {
+    return true;
+  }
 }

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

@@ -18,7 +18,7 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static io.grpc.util.CertificateUtils.getX509ExtendedTrustManager;
+import static io.grpc.internal.CertificateUtils.getX509ExtendedTrustManager;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -42,6 +42,7 @@
 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;
@@ -85,6 +86,7 @@
 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.net.ssl.SSLEngine;
 import javax.net.ssl.SSLEngineResult;
@@ -640,6 +642,35 @@ public void setSslEngine(SSLEngine sslEngine) {
     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 {

util/src/main/java/io/grpc/util/CertificateUtils.java

@@ -23,9 +23,7 @@
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.UnsupportedEncodingException;
-import java.security.GeneralSecurityException;
 import java.security.KeyFactory;
-import java.security.KeyStore;
 import java.security.NoSuchAlgorithmException;
 import java.security.PrivateKey;
 import java.security.cert.Certificate;
@@ -34,13 +32,7 @@
 import java.security.cert.X509Certificate;
 import java.security.spec.InvalidKeySpecException;
 import java.security.spec.PKCS8EncodedKeySpec;
-import java.util.Arrays;
 import java.util.Collection;
-import java.util.Optional;
-import javax.net.ssl.TrustManager;
-import javax.net.ssl.TrustManagerFactory;
-import javax.net.ssl.X509ExtendedTrustManager;
-import javax.security.auth.x500.X500Principal;
 
 /**
  * Contains certificate/key PEM file utility method(s).
@@ -99,31 +91,5 @@ public static PrivateKey getPrivateKey(InputStream inputStream)
       }
     }
   }
-
-  /**
-   * Creates a X509ExtendedTrustManager using the provided CA certs if applicable for the
-   * certificate type.
-   */
-  public static Optional<TrustManager> getX509ExtendedTrustManager(InputStream rootCerts)
-      throws GeneralSecurityException {
-    KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());
-    try {
-      ks.load(null, null);
-    } catch (IOException ex) {
-      // Shouldn't really happen, as we're not loading any data.
-      throw new GeneralSecurityException(ex);
-    }
-    X509Certificate[] certs = CertificateUtils.getX509Certificates(rootCerts);
-    for (X509Certificate cert : certs) {
-      X500Principal principal = cert.getSubjectX500Principal();
-      ks.setCertificateEntry(principal.getName("RFC2253"), cert);
-    }
-
-    TrustManagerFactory trustManagerFactory =
-        TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
-    trustManagerFactory.init(ks);
-    return Arrays.stream(trustManagerFactory.getTrustManagers())
-        .filter(trustManager -> trustManager instanceof X509ExtendedTrustManager).findFirst();
-  }
 }