xds: suppress hostname check but keep trust check in the delegated X509TrustManagerImpl (#6589)

xds/src/main/java/io/grpc/xds/sds/trust/SdsTrustManagerFactory.java

@@ -19,6 +19,7 @@ package io.grpc.xds.sds.trust;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import io.envoyproxy.envoy.api.v2.auth.CertificateValidationContext;
 import io.envoyproxy.envoy.api.v2.core.DataSource.SpecifierCase;
@@ -53,7 +54,7 @@ public final class SdsTrustManagerFactory extends SimpleTrustManagerFactory {
   public SdsTrustManagerFactory(CertificateValidationContext certificateValidationContext)
       throws CertificateException, IOException, CertStoreException {
     checkNotNull(certificateValidationContext, "certificateValidationContext");
-    createSdsX509TrustManager(
+    sdsX509TrustManager = createSdsX509TrustManager(
         getTrustedCaFromCertContext(certificateValidationContext), certificateValidationContext);
   }
 
@@ -76,7 +77,8 @@ public final class SdsTrustManagerFactory extends SimpleTrustManagerFactory {
     }
   }
 
-  private void createSdsX509TrustManager(
+  @VisibleForTesting
+  static SdsX509TrustManager createSdsX509TrustManager(
       X509Certificate[] certs, CertificateValidationContext certContext) throws CertStoreException {
     TrustManagerFactory tmf = null;
     try {
@@ -109,7 +111,7 @@ public final class SdsTrustManagerFactory extends SimpleTrustManagerFactory {
     if (myDelegate == null) {
       throw new CertStoreException("Native X509 TrustManager not found.");
     }
-    sdsX509TrustManager = new SdsX509TrustManager(certContext, myDelegate);
+    return new SdsX509TrustManager(certContext, myDelegate);
   }
 
   @Override

xds/src/main/java/io/grpc/xds/sds/trust/SdsX509TrustManager.java

@@ -30,6 +30,8 @@ import java.util.List;
 import java.util.Locale;
 import javax.annotation.Nullable;
 import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.SSLSocket;
 import javax.net.ssl.X509ExtendedTrustManager;
 import javax.net.ssl.X509TrustManager;
 
@@ -256,6 +258,14 @@ final class SdsX509TrustManager extends X509ExtendedTrustManager implements X509
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
       throws CertificateException {
+    if (socket instanceof SSLSocket) {
+      SSLSocket sslSocket = (SSLSocket) socket;
+      SSLParameters sslParams = sslSocket.getSSLParameters();
+      if (sslParams != null) {
+        sslParams.setEndpointIdentificationAlgorithm(null);
+        sslSocket.setSSLParameters(sslParams);
+      }
+    }
     delegate.checkServerTrusted(chain, authType, socket);
     verifySubjectAltNameInChain(chain);
   }
@@ -263,6 +273,11 @@ final class SdsX509TrustManager extends X509ExtendedTrustManager implements X509
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine sslEngine)
       throws CertificateException {
+    SSLParameters sslParams = sslEngine.getSSLParameters();
+    if (sslParams != null) {
+      sslParams.setEndpointIdentificationAlgorithm(null);
+      sslEngine.setSSLParameters(sslParams);
+    }
     delegate.checkServerTrusted(chain, authType, sslEngine);
     verifySubjectAltNameInChain(chain);
   }

xds/src/test/java/io/grpc/xds/sds/trust/SdsX509TrustManagerTest.java

@@ -17,15 +17,26 @@
 package io.grpc.xds.sds.trust;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import io.envoyproxy.envoy.api.v2.auth.CertificateValidationContext;
 import io.grpc.internal.testing.TestUtils;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.security.cert.CertStoreException;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLSocket;
 import javax.net.ssl.X509ExtendedTrustManager;
-import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -33,6 +44,7 @@ import org.junit.runners.JUnit4;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
+import sun.security.validator.ValidatorException;
 
 /**
  * Unit tests for {@link SdsX509TrustManager}.
@@ -40,21 +52,32 @@ import org.mockito.junit.MockitoRule;
 @RunWith(JUnit4.class)
 public class SdsX509TrustManagerTest {
 
+  /** Trust store cert. */
+  private static final String CA_PEM_FILE = "ca.pem";
+
   /** server1 has 4 SANs. */
   private static final String SERVER_1_PEM_FILE = "server1.pem";
 
   /** client has no SANs. */
   private static final String CLIENT_PEM_FILE = "client.pem";
 
+  /** Untrusted server. */
+  private static final String BAD_SERVER_PEM_FILE = "badserver.pem";
+
   @Rule
   public final MockitoRule mockitoRule = MockitoJUnit.rule();
 
   @Mock
   private X509ExtendedTrustManager mockDelegate;
 
+  @Mock
+  private SSLSession mockSession;
+
+  private SdsX509TrustManager trustManager;
+
   @Test
   public void nullCertContextTest() throws CertificateException, IOException {
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(null, mockDelegate);
+    trustManager = new SdsX509TrustManager(null, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -63,7 +86,7 @@ public class SdsX509TrustManagerTest {
   @Test
   public void emptySanListContextTest() throws CertificateException, IOException {
     CertificateValidationContext certContext = CertificateValidationContext.getDefaultInstance();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -73,10 +96,10 @@ public class SdsX509TrustManagerTest {
   public void missingPeerCerts() throws CertificateException, FileNotFoundException {
     CertificateValidationContext certContext =
         CertificateValidationContext.newBuilder().addVerifySubjectAltName("foo.com").build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     try {
       trustManager.verifySubjectAltNameInChain(null);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate(s) missing");
     }
@@ -86,10 +109,10 @@ public class SdsX509TrustManagerTest {
   public void emptyArrayPeerCerts() throws CertificateException, FileNotFoundException {
     CertificateValidationContext certContext =
         CertificateValidationContext.newBuilder().addVerifySubjectAltName("foo.com").build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     try {
       trustManager.verifySubjectAltNameInChain(new X509Certificate[0]);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate(s) missing");
     }
@@ -99,12 +122,12 @@ public class SdsX509TrustManagerTest {
   public void noSansInPeerCerts() throws CertificateException, IOException {
     CertificateValidationContext certContext =
         CertificateValidationContext.newBuilder().addVerifySubjectAltName("foo.com").build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(CLIENT_PEM_FILE));
     try {
       trustManager.verifySubjectAltNameInChain(certs);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate SAN check failed");
     }
@@ -116,7 +139,7 @@ public class SdsX509TrustManagerTest {
         CertificateValidationContext.newBuilder()
             .addVerifySubjectAltName("waterzooi.test.google.be")
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -130,7 +153,7 @@ public class SdsX509TrustManagerTest {
             .addVerifySubjectAltName("x.foo.com")
             .addVerifySubjectAltName("waterzooi.test.google.be")
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -141,12 +164,12 @@ public class SdsX509TrustManagerTest {
       throws CertificateException, IOException {
     CertificateValidationContext certContext =
         CertificateValidationContext.newBuilder().addVerifySubjectAltName("x.foo.com").build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     try {
       trustManager.verifySubjectAltNameInChain(certs);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate SAN check failed");
     }
@@ -160,7 +183,7 @@ public class SdsX509TrustManagerTest {
             .addVerifySubjectAltName("x.foo.com")
             .addVerifySubjectAltName("abc.test.youtube.com") // should match *.test.youtube.com
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -174,7 +197,7 @@ public class SdsX509TrustManagerTest {
             .addVerifySubjectAltName("x.foo.com")
             .addVerifySubjectAltName("abc.test.google.fr") // should match *.test.google.fr
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -190,12 +213,12 @@ public class SdsX509TrustManagerTest {
         CertificateValidationContext.newBuilder()
             .addVerifySubjectAltName("sub.abc.test.youtube.com")
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     try {
       trustManager.verifySubjectAltNameInChain(certs);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate SAN check failed");
     }
@@ -208,7 +231,7 @@ public class SdsX509TrustManagerTest {
             .addVerifySubjectAltName("x.foo.com")
             .addVerifySubjectAltName("192.168.1.3")
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     trustManager.verifySubjectAltNameInChain(certs);
@@ -221,14 +244,132 @@ public class SdsX509TrustManagerTest {
             .addVerifySubjectAltName("x.foo.com")
             .addVerifySubjectAltName("192.168.2.3")
             .build();
-    SdsX509TrustManager trustManager = new SdsX509TrustManager(certContext, mockDelegate);
+    trustManager = new SdsX509TrustManager(certContext, mockDelegate);
     X509Certificate[] certs =
         CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
     try {
       trustManager.verifySubjectAltNameInChain(certs);
-      Assert.fail("no exception thrown");
+      fail("no exception thrown");
     } catch (CertificateException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("Peer certificate SAN check failed");
     }
   }
+
+  @Test
+  public void checkServerTrustedSslEngine()
+      throws CertificateException, IOException, CertStoreException {
+    TestSslEngine sslEngine = buildTrustManagerAndGetSslEngine();
+    X509Certificate[] serverCerts =
+        CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
+    trustManager.checkServerTrusted(serverCerts, "ECDHE_ECDSA", sslEngine);
+    verify(sslEngine, times(1)).getHandshakeSession();
+  }
+
+  @Test
+  public void checkServerTrustedSslEngine_untrustedServer_expectException()
+      throws CertificateException, IOException, CertStoreException {
+    TestSslEngine sslEngine = buildTrustManagerAndGetSslEngine();
+    X509Certificate[] badServerCert =
+        CertificateUtils.toX509Certificates(TestUtils.loadCert(BAD_SERVER_PEM_FILE));
+    try {
+      trustManager.checkServerTrusted(badServerCert, "ECDHE_ECDSA", sslEngine);
+      fail("exception expected");
+    } catch (ValidatorException expected) {
+      assertThat(expected).hasMessageThat()
+          .endsWith("unable to find valid certification path to requested target");
+    }
+    verify(sslEngine, times(1)).getHandshakeSession();
+  }
+
+  @Test
+  public void checkServerTrustedSslSocket()
+      throws CertificateException, IOException, CertStoreException {
+    TestSslSocket sslSocket = buildTrustManagerAndGetSslSocket();
+    X509Certificate[] serverCerts =
+        CertificateUtils.toX509Certificates(TestUtils.loadCert(SERVER_1_PEM_FILE));
+    trustManager.checkServerTrusted(serverCerts, "ECDHE_ECDSA", sslSocket);
+    verify(sslSocket, times(1)).isConnected();
+    verify(sslSocket, times(1)).getHandshakeSession();
+  }
+
+  @Test
+  public void checkServerTrustedSslSocket_untrustedServer_expectException()
+      throws CertificateException, IOException, CertStoreException {
+    TestSslSocket sslSocket = buildTrustManagerAndGetSslSocket();
+    X509Certificate[] badServerCert =
+        CertificateUtils.toX509Certificates(TestUtils.loadCert(BAD_SERVER_PEM_FILE));
+    try {
+      trustManager.checkServerTrusted(badServerCert, "ECDHE_ECDSA", sslSocket);
+      fail("exception expected");
+    } catch (ValidatorException expected) {
+      assertThat(expected).hasMessageThat()
+          .endsWith("unable to find valid certification path to requested target");
+    }
+    verify(sslSocket, times(1)).isConnected();
+    verify(sslSocket, times(1)).getHandshakeSession();
+  }
+
+  private TestSslEngine buildTrustManagerAndGetSslEngine()
+      throws CertificateException, IOException, CertStoreException {
+    SSLParameters sslParams = buildTrustManagerAndGetSslParameters();
+
+    TestSslEngine sslEngine = mock(TestSslEngine.class, CALLS_REAL_METHODS);
+    sslEngine.setSSLParameters(sslParams);
+    doReturn(mockSession).when(sslEngine).getHandshakeSession();
+    return sslEngine;
+  }
+
+  private TestSslSocket buildTrustManagerAndGetSslSocket()
+      throws CertificateException, IOException, CertStoreException {
+    SSLParameters sslParams = buildTrustManagerAndGetSslParameters();
+
+    TestSslSocket sslSocket = mock(TestSslSocket.class, CALLS_REAL_METHODS);
+    sslSocket.setSSLParameters(sslParams);
+    doReturn(true).when(sslSocket).isConnected();
+    doReturn(mockSession).when(sslSocket).getHandshakeSession();
+    return sslSocket;
+  }
+
+  private SSLParameters buildTrustManagerAndGetSslParameters()
+      throws CertificateException, IOException, CertStoreException {
+    X509Certificate[] caCerts =
+        CertificateUtils.toX509Certificates(TestUtils.loadCert(CA_PEM_FILE));
+    trustManager = SdsTrustManagerFactory.createSdsX509TrustManager(caCerts,
+        null);
+    when(mockSession.getProtocol()).thenReturn("TLSv1.2");
+    when(mockSession.getPeerHost()).thenReturn("peer-host-from-mock");
+    SSLParameters sslParams = new SSLParameters();
+    sslParams.setEndpointIdentificationAlgorithm("HTTPS");
+    return sslParams;
+  }
+
+  private abstract static class TestSslSocket extends SSLSocket {
+
+    @Override
+    public SSLParameters getSSLParameters() {
+      return sslParameters;
+    }
+
+    @Override
+    public void setSSLParameters(SSLParameters sslParameters) {
+      this.sslParameters = sslParameters;
+    }
+
+    private SSLParameters sslParameters;
+  }
+
+  private abstract static class TestSslEngine extends SSLEngine {
+
+    @Override
+    public SSLParameters getSSLParameters() {
+      return sslParameters;
+    }
+
+    @Override
+    public void setSSLParameters(SSLParameters sslParameters) {
+      this.sslParameters = sslParameters;
+    }
+
+    private SSLParameters sslParameters;
+  }
 }