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

vimanikag · 2025-09-09T11:56:46Z

Fixes : #12326

… that Envoy does

kannanjgithub

Left comments.

kannanjgithub · 2025-09-15T05:05:27Z

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

+    return labelWildcardMatch(certLabels[0], sanLabels[0]);
+  }
+
+  private static boolean labelWildcardMatch(String certLabel, String sanLabel) {


Rename certLabel to pattern and sanLabel to dnsLabel like in Envoy code, it is more clear that way.

Addressed the review comments

kannanjgithub · 2025-09-15T05:08:41Z

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

+    int starIndex = sanLabel.indexOf('*');
+    String prefix = sanLabel.substring(0, starIndex);
+    String suffix = sanLabel.substring(starIndex + 1);
+    if (certLabel.length() < prefix.length() + suffix.length()) {


This is better said as certLabel.length() <= sanLabel.length() + 1 like in Envoy code. The +1 is for * that stands for 0 or more characters. Combine with below condition in the return statement

Thanks @kannanjgithub for the review , I have addressed the review comments.

kannanjgithub · 2025-09-15T05:11:27Z

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

+
+  public static boolean verifyDnsNameWildcard(
+          String altNameFromCert, String sanToVerify, boolean ignoreCase) {
+    if (Strings.isNullOrEmpty(altNameFromCert) || Strings.isNullOrEmpty(sanToVerify)) {


We don't need this check since the logic can already handle empty strings, and null won't be passed for sanToVerify here. altNameFromCert wont' be null.
Instead have the trivial check for "*" like Envoy does

Addressed the review comments

kannanjgithub · 2025-09-15T05:32:50Z

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

+        ? sanToVerifySuffix.toLowerCase(Locale.ROOT)
+        : sanToVerifySuffix).contains("*")) {
+      return verifyDnsNameWildcard(altNameFromCert, sanToVerifySuffix , ignoreCase);
+    }


Looking at the envoy code here it is not doing wildcard matching for prefix, suffix, and contains and for safe regex it is compiling the pattern as regex. gRPC code is already doing the same for these 3 cases, so only for exact DNS match we need to have the changes for handling split patterns and *.

Addressed the review comments

kannanjgithub · 2025-09-15T05:38:23Z

testing/src/main/resources/certs/bad_wildcard_dns_certificate.pem

@@ -0,0 +1,24 @@
+-----BEGIN CERTIFICATE-----
+MIID7zCCAtegAwIBAgIUCs5j4C2KXgCRVFa48kc5TYRS1JswDQYJKoZIhvcNAQEL
+BQAwGTEXMBUGA1UEAwwOTXkgSW50ZXJuYWwgQ0EwHhcNMjUwOTA4MTI0NTQyWhcN


No need to create any new certificate, we can work with
testing/src/main/resources/certs/server0.pem and server1.pem themselves. server0.pem is issued to *.test.google.com.au server1.pem is issued to the names
*.test.google.fr, waterzooi.test.google.be, *.test.youtube.com, IP Address:192.168.1.3

Addressed the review comments and used the existing server1.pem for the junit's

panchenko · 2025-09-16T16:56:01Z

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

@@ -147,6 +147,11 @@ private static boolean verifyDnsNameExact(
    if (Strings.isNullOrEmpty(sanToVerifyExact)) {
      return false;
    }
+    if ((ignoreCase
+        ? sanToVerifyExact.toLowerCase(Locale.ROOT)


ignoreCase ? sanToVerifyExact.toLowerCase(Locale.ROOT) is redundant
as "*" is case-insensitive

Addressed the review comments

kannanjgithub

Doesn't match envoy pattern magic logic.

kannanjgithub · 2025-09-17T05:02:39Z

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

+    if ((ignoreCase
+        ? sanToVerifyExact.toLowerCase(Locale.ROOT)
+        : sanToVerifyExact).contains("*")) {
+      return verifyDnsNameWildcard(altNameFromCert, sanToVerifyExact , ignoreCase);


nit: Remove space before the comma.

kannanjgithub · 2025-09-17T05:23:25Z

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

+  public static boolean verifyDnsNameWildcard(
+          String altNameFromCert, String sanToVerify, boolean ignoreCase) {
+    String[] pattern = (ignoreCase ? altNameFromCert.toLowerCase(Locale.ROOT) : altNameFromCert)
+            .split("\\.", -1);


We need only 1 split, check envoy code: https://github.com/envoyproxy/envoy/blob/d8c394ca5aecd237086f6f89933fcfced582386e/source/common/tls/utility.cc#L81

-1 for split allows any number of splits.

Thanks @kannanjgithub for the review , I have updated this logic as per the envoy code and now the we have the 1 split by delimiter and comparing the left and right labels.

kannanjgithub · 2025-09-17T05:33:31Z

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

+    String prefix = dnsLabel.substring(0, starIndex);
+    String suffix = dnsLabel.substring(starIndex + 1);
+    return pattern.length() >= (dnsLabel.length() - 1)
+        && pattern.startsWith(prefix)


We should only look for * in pattern, not in dnsLabel. Check https://github.com/envoyproxy/envoy/blob/d8c394ca5aecd237086f6f89933fcfced582386e/source/common/tls/utility.cc#L64

Thanks @kannanjgithub for the suggestions, I have strictly followed the Envoy logic in latest changes, excluding the exact match check, as it is handled in verifyDnsNameExact, and have updated the changes accordingly.

if (lower_case_dns_name == lower_case_pattern) {
return true;
}

…he envoy code.

panchenko · 2025-09-17T13:56:45Z

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

+  }
+
+  private static String[] splitAtFirstDelimiter(String s) {
+    int index = s.indexOf(".");


'.' - as character

panchenko · 2025-09-17T13:57:54Z

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

+  private static String[] splitAtFirstDelimiter(String s) {
+    int index = s.indexOf(".");
+    if (index == -1) {
+      return new String[]{s};


the contents of the single element array is not used, in terms of performance I would return null

@panchenko , I agree that the single-element array isn't used in most cases. However, returning it prevents potential NullPointerExceptions, allowing the caller to use splitAtFirstDelimiter safely without needing a null check. Given this, I believe the current approach is more robust and maintainable. I would appreciate your thoughts before proceeding with any changes.

@vimanikag So the method should be annotated with the @Nullable annotation.
If returning null then slightly shorter code here and also the check if no delimiter found.

In general, what should be returned if delimiter is not found varies by use case, in some situations it could be["", s] or the other way around, so returning null to indicate that there are no 2 elements it not a bad idea.

Thanks, @panchenko , for sharing your thoughts. I've addressed the review comments.

panchenko · 2025-09-17T14:15:27Z

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

+    if (pattern.length() == 1 && pattern.charAt(0) == glob) {
+      return true;
+    }
+    if (pattern.chars().filter(ch -> ch == glob).count() == 1) {


in terms of performance it can be

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));

Thanks @panchenko for the suggestions, I have addressed the review comments

kannanjgithub

Changes needed.

kannanjgithub · 2025-09-19T07:28:27Z

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

+
+  private static boolean labelWildcardMatch(String dnsLabel, String pattern) {
+    final char glob = '*';
+    if (pattern.length() == 1 && pattern.charAt(0) == glob) {


Just do if (pattern.equals("*")). Envoy does in a C++ typical way using char comparison, we don't need to do that in Java. String.equals() is highly optimized in Java and we don't need to do it ourselves.

kannanjgithub · 2025-09-19T07:28:54Z

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

+  }
+
+  private static boolean labelWildcardMatch(String dnsLabel, String pattern) {
+    final char glob = '*';


Also add the comment
// Check the special case of a single * pattern, as it's common.

kannanjgithub · 2025-09-19T08:15:53Z

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

+    assertFalse(verifyDnsNameWildcard("", "*lyft.com", true));
+    assertFalse(verifyDnsNameWildcard("lyft.com", "", true));
+    assertFalse(verifyDnsNameWildcard("xn--lyft.com", "*.xn--lyft.com", true));
+  }


This adds no value, we deny if xn-- is present in pattern[0], the failure here is due to other reasons, and is misleading. Change it to a negative test case with xn-- in pattern[0].

@kannanjgithub , I have not addressed this review comment yet because our current certificates do not include this pattern. I will address this in a future commit once we finalize the new patterns.

kannanjgithub · 2025-09-19T08:21:15Z

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

+  }
+
+  @Test
+  public void testVerifyDnsNameWildcard() {


verifyDnsNameWildcard should never have been a public method, it should only be a private method and it is easily testable via the public method. Have a helper method take the arguments

Pattern

Certificate file to use - server0 or server1

Ignore case or not

Assert true or false

And have a single @test that calls this helper method with all the test data you have.
This can be a junit parameterized test but I'm not suggesting that because I have parameterized tests upcoming for a different feature with a different set of parameters.

You will have certificate patterns like "l-lots-of-stuff-ft.com", ".lyft.com", ""
that are not testable because our test certificates don't have them. Some of these such as the above 3 SAN names seem redundant to test. If there are others that are unable to be tested via existing certs and are non-redundant please list them and we can think whether we need to create test certs with those names.

@kannanjgithub , I have partially addressed your review comment and will complete the remaining work in a future commit once we finalize the new patterns for our certificates.

To assist with this, I have compiled a list of valid and invalid patterns below. Most of these have already been included in my initial commit (919285c). Please review these patterns and confirm if the attached certificates are acceptable. If you approve, I will ensure they are included in the next commit with the new certificates.

I have attached a document link/attachment as few patterns are altered in GitHub comments :
https://docs.google.com/document/d/1O0hzmTdwTEO8znjJGje9plXm13EWwGpE2xqdD5-vnpM/edit?usp=sharing

VIDNSName.txt

kannanjgithub · 2025-09-19T08:21:40Z

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

+  }
+
+  @Test
+  public void testVerifyDnsNameWildcard() {


Add a test for exact match without wildcard by setting
StringMatcher.newBuilder() .setExact("waterzooi.test.google.be")
This SAN name is present in the server1.pem test certificate.

Add a failure case with
.setExact("*.TEST.YOUTUBE.com") .setIgnoreCase(false)
and assert that the call fails.

Add a test for String matcher exact pattern with * and without any '.' in it is it fails validation because split patterns will return null.

vimanikag added 4 commits September 9, 2025 11:52

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

919285c

… that Envoy does

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

6b0de5c

… that Envoy does

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

3f45fa6

… that Envoy does

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

f1265e7

… that Envoy does

vimanikag marked this pull request as draft September 9, 2025 15:44

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

35e38ce

… that Envoy does

vimanikag marked this pull request as ready for review September 10, 2025 02:42

Sangamesh1997 added 2 commits September 10, 2025 05:23

cosmatic changes

1100be4

xds : XdsX509TrustManager: cosmetic changes

9e9875d

kannanjgithub requested changes Sep 15, 2025

View reviewed changes

11246 :: addressed the review comments

403faa8

panchenko reviewed Sep 16, 2025

View reviewed changes

kannanjgithub requested changes Sep 17, 2025

View reviewed changes

11246 :: addressed the review comments and updated the logic as per t…

cee6325

…he envoy code.

panchenko reviewed Sep 17, 2025

View reviewed changes

vimanikag added 2 commits September 18, 2025 08:17

11246 :: addressed the review comments.

2e75fe0

11246 :: addressed the review comments #discussion_r2355624175

e77a957

kannanjgithub requested changes Sep 19, 2025

View reviewed changes

11246 :: addressed the junit's review comments

f1b3262

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

Are you sure you want to change the base?

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

Conversation

vimanikag commented Sep 9, 2025

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panchenko Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

panchenko Sep 16, 2025 •

edited

Loading

kannanjgithub Sep 19, 2025 •

edited

Loading

vimanikag Sep 19, 2025 •

edited

Loading

kannanjgithub Sep 19, 2025 •

edited

Loading