12326 :: DNS SAN matching does not handle wildcards in split patterns that Envoy does

testing/src/main/resources/certs/sni-test-certs/bad_wildcard_dns_certificate.pem

@@ -0,0 +1,22 @@
+-----BEGIN CERTIFICATE-----
+MIIDsjCCApqgAwIBAgIUCs5j4C2KXgCRVFa48kc5TYRS1JwwDQYJKoZIhvcNAQEL
+BQAwGTEXMBUGA1UEAwwOTXkgSW50ZXJuYWwgQ0EwIBcNMjUwOTIzMDc1NDUzWhgP
+MjEyNTA4MzAwNzU0NTNaMGUxCzAJBgNVBAYTAlVTMREwDwYDVQQIDAhJbGxpbm9p
+czEQMA4GA1UEBwwHQ2hpY2FnbzEVMBMGA1UECgwMRXhhbXBsZSwgQ28uMRowGAYD
+VQQDDBEqLnRlc3QuZ29vZ2xlLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC
+AQoCggEBAKoqcnNh9MV39GH6JjC5KVMN6MO1IoTw6wHJN0JJ/nGNx6ycIsBK8SgJ
+eYRR2BEpT6WZba+f04KChcB4Z9tiPISNvUBpmEv76rAsdtcAZwSpF06q4wxHVE5F
+rX6mNT8hk448mDBDGHUXNAT6g/e/Vlt6U0XRyuu713gbZq1X6JH29FG7EJ3LUx35
+h6sEkvTlZZ3m6NJr7zYoqrYh/gRkPigtPxaNcoXo0gVm4IEde0sYz27SWyNH4v/o
+23NynSulOwx4DwEhBOXekLb5QJHBqwMTPynaMncBQIXF+PXeuxN9a3zR6DSn+jGw
+g008tS0tn2FuAvJDBl0paEykdOr2rNMCAwEAAaOBozCBoDALBgNVHQ8EBAMCBeAw
+EwYDVR0lBAwwCgYIKwYBBQUHAwEwPAYDVR0RBDUwM4IKbHkqKmZ0LmNvbYIIKnlm
+dC5jKm2CCS5seWZ0LmNvbYIOeG4tLSoubHlmdC5jb22CADAdBgNVHQ4EFgQUZoL2
+OzBtK/BUzSYfgXDx3iDjcIQwHwYDVR0jBBgwFoAUHlstFN5WSLSqyJgUDy6BB0z0
+BrgwDQYJKoZIhvcNAQELBQADggEBAMYwVOT7XZJMQ6n32pQqtZhJ/Z0wlVfCAbm0
+7xospeBt6KtOz2zIsvPpq0aqPjowMAeL1EZaBvmfm/XgWUU5e/3hLUIHOHyKfswB
+czDbY0RE8nfVDoF4Ck1ljPjvrFr4tSAxTzVA4JU5o3UXkblBg0LG6tTuLlZ3x5aF
+KtkZnszxjE+vOg6J9MDbFP/xtA1oVHyCvk+cUgnBxAoPShI+87DINGVTmztBSetK
+nJN9dOh7Q88NhTLHOe67Ora9Y0ZP+uFKHaqFv8qj8B/Q6ptb0CAksdL5EunkIHrq
+glKdVdYgIP2JpRwtvVHK5FzWBlGXCi3DxTyYi6FWqsSJ+heCS2w=
+-----END CERTIFICATE-----

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

@@ -147,6 +147,9 @@ private static boolean verifyDnsNameExact(
     if (Strings.isNullOrEmpty(sanToVerifyExact)) {
       return false;
     }
+    if (sanToVerifyExact.contains("*")) {
+      return verifyDnsNameWildcard(altNameFromCert, sanToVerifyExact, ignoreCase);
+    }
     return ignoreCase
         ? sanToVerifyExact.equalsIgnoreCase(altNameFromCert)
         : sanToVerifyExact.equals(altNameFromCert);
@@ -303,4 +306,49 @@ public X509Certificate[] getAcceptedIssuers() {
     }
     return delegate.getAcceptedIssuers();
   }
+
+  private static boolean verifyDnsNameWildcard(
+      String altNameFromCert, String sanToVerify, boolean ignoreCase) {
+    String[] splitPattern = splitAtFirstDelimiter(ignoreCase
+        ? sanToVerify.toLowerCase(Locale.ROOT) : sanToVerify);
+    String[] splitDnsName = splitAtFirstDelimiter(ignoreCase
+        ? altNameFromCert.toLowerCase(Locale.ROOT) : altNameFromCert);
+    if (splitPattern == null || splitDnsName == null) {
+      return false;
+    }
+    if (splitDnsName[0].startsWith("xn--")) {
+      return false;
+    }
+    if (splitPattern[0].contains("*")
+        && !splitPattern[1].contains("*")
+        && !splitPattern[0].startsWith("xn--")) {
+      return splitDnsName[1].equals(splitPattern[1])
+          && labelWildcardMatch(splitDnsName[0], splitPattern[0]);
+    }
+    return false;
+  }
+
+  private static boolean labelWildcardMatch(String dnsLabel, String pattern) {
+    final char glob = '*';
+    // Check the special case of a single * pattern, as it's common.
+    if (pattern.equals("*")) {
+      return !dnsLabel.isEmpty();
+    }
+    int globIndex = pattern.indexOf(glob);
+    if (pattern.indexOf(glob, globIndex + 1) == -1) {
+      return dnsLabel.length() >= pattern.length() - 1
+          && dnsLabel.startsWith(pattern.substring(0, globIndex))
+          && dnsLabel.endsWith(pattern.substring(globIndex + 1));
+    }
+    return false;
+  }
+
+  @Nullable
+  private static String[] splitAtFirstDelimiter(String s) {
+    int index = s.indexOf('.');
+    if (index == -1) {
+      return null;
+    }
+    return new String[]{s.substring(0, index), s.substring(index + 1)};
+  }
 }

xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java

@@ -60,6 +60,8 @@ public class CommonTlsContextTestsUtil {
   public static final String BAD_SERVER_KEY_FILE = "badserver.key";
   public static final String BAD_CLIENT_PEM_FILE = "badclient.pem";
   public static final String BAD_CLIENT_KEY_FILE = "badclient.key";
+  public static final String BAD_WILDCARD_DNS_PEM_FILE =
+      "sni-test-certs/bad_wildcard_dns_certificate.pem";
 
   /** takes additional values and creates CombinedCertificateValidationContext as needed. */
   private static CommonTlsContext buildCommonTlsContextWithAdditionalValues(

xds/src/test/java/io/grpc/xds/internal/security/trust/XdsX509TrustManagerTest.java

@@ -18,9 +18,11 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.BAD_SERVER_PEM_FILE;
+import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.BAD_WILDCARD_DNS_PEM_FILE;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.CA_PEM_FILE;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.CLIENT_PEM_FILE;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.CLIENT_SPIFFE_PEM_FILE;
+import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.SERVER_0_PEM_FILE;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.SERVER_1_PEM_FILE;
 import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.SERVER_1_SPIFFE_PEM_FILE;
 import static org.junit.Assert.fail;
@@ -42,6 +44,7 @@
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import javax.net.ssl.SSLEngine;
@@ -52,15 +55,16 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
 /**
  * Unit tests for {@link XdsX509TrustManager}.
  */
-@RunWith(JUnit4.class)
+@RunWith(Parameterized.class)
 public class XdsX509TrustManagerTest {
 
   @Rule
@@ -74,6 +78,12 @@ public class XdsX509TrustManagerTest {
 
   private XdsX509TrustManager trustManager;
 
+  private final TestParam testParam;
+
+  public XdsX509TrustManagerTest(TestParam testParam) {
+    this.testParam = testParam;
+  }
+
   @Test
   public void nullCertContextTest() throws CertificateException, IOException {
     trustManager = new XdsX509TrustManager(null, mockDelegate);
@@ -691,6 +701,52 @@ public void unsupportedAltNameType() throws CertificateException, IOException {
     }
   }
 
+  @Test
+  public void testDnsWildcardPatterns()
+      throws CertificateException, IOException {
+    StringMatcher stringMatcher =
+        StringMatcher.newBuilder()
+            .setExact(testParam.sanPattern)
+            .setIgnoreCase(testParam.ignoreCase)
+            .build();
+    @SuppressWarnings("deprecation")
+    CertificateValidationContext certContext =
+        CertificateValidationContext.newBuilder()
+            .addMatchSubjectAltNames(stringMatcher)
+            .build();
+    trustManager = new XdsX509TrustManager(certContext, mockDelegate);
+    X509Certificate[] certs =
+        CertificateUtils.toX509Certificates(TlsTesting.loadCert(testParam.certFile));
+    try {
+      trustManager.verifySubjectAltNameInChain(certs);
+      assertThat(testParam.expected).isTrue();
+    } catch (CertificateException certException) {
+      assertThat(testParam.expected).isFalse();
+      assertThat(certException).hasMessageThat().isEqualTo("Peer certificate SAN check failed");
+    }
+  }
+
+  @Parameters(name = "{index}: {0}")
+  public static Collection<Object[]> getParameters() {
+    return Arrays.asList(new Object[][] {
+        {new TestParam("*.test.google.fr", SERVER_1_PEM_FILE, false, true)},
+        {new TestParam("*.test.youtube.com", SERVER_1_PEM_FILE, false, true)},
+        {new TestParam("waterzooi.test.google.be", SERVER_1_PEM_FILE, false, true)},
+        {new TestParam("192.168.1.3", SERVER_1_PEM_FILE, false, true)},
+        {new TestParam("*.TEST.YOUTUBE.com", SERVER_1_PEM_FILE, true, true)},
+        {new TestParam("w*i.test.google.be", SERVER_1_PEM_FILE, false, true)},
+        {new TestParam("w*a.test.google.be", SERVER_1_PEM_FILE, false, false)},
+        {new TestParam("*.test.google.com.au", SERVER_0_PEM_FILE, false, false)},
+        {new TestParam("*.TEST.YOUTUBE.com", SERVER_1_PEM_FILE, false, false)},
+        {new TestParam("*waterzooi", SERVER_1_PEM_FILE, false, false)},
+        {new TestParam("*.lyft.com", BAD_WILDCARD_DNS_PEM_FILE, false, false)},
+        {new TestParam("ly**ft.com", BAD_WILDCARD_DNS_PEM_FILE, false, false)},
+        {new TestParam("*yft.c*m", BAD_WILDCARD_DNS_PEM_FILE, false, false)},
+        {new TestParam("xn--*.lyft.com", BAD_WILDCARD_DNS_PEM_FILE, false, false)},
+        {new TestParam("", BAD_WILDCARD_DNS_PEM_FILE, false, false)},
+    });
+  }
+
   private TestSslEngine buildTrustManagerAndGetSslEngine()
       throws CertificateException, IOException, CertStoreException {
     SSLParameters sslParams = buildTrustManagerAndGetSslParameters();
@@ -754,4 +810,18 @@ public void setSSLParameters(SSLParameters sslParameters) {
 
     private SSLParameters sslParameters;
   }
+
+  private static class TestParam {
+    final String sanPattern;
+    final String certFile;
+    final boolean ignoreCase;
+    final boolean expected;
+
+    TestParam(String sanPattern, String certFile, boolean ignoreCase, boolean expected) {
+      this.sanPattern = sanPattern;
+      this.certFile = certFile;
+      this.ignoreCase = ignoreCase;
+      this.expected = expected;
+    }
+  }
 }