fix: bug in overlap detection of cert SANs (envoyproxy#7234)

Signed-off-by: shawnh2 <shawnhxh@outlook.com>

Author: rudrakhp

internal/gatewayapi/listener.go

@@ -241,7 +241,7 @@ func checkOverlappingHostnames(httpsListeners []*ListenerContext) {
 			if httpsListeners[i].Port != httpsListeners[j].Port {
 				continue
 			}
-			if isOverlappingHostname(httpsListeners[i].Hostname, httpsListeners[j].Hostname) {
+			if areOverlappingHostnames(httpsListeners[i].Hostname, httpsListeners[j].Hostname) {
 				// Overlapping listeners can be more than two, we only report the first two for simplicity.
 				overlappingListeners[i] = &overlappingListener{
 					gateway1:  httpsListeners[i].gateway,
@@ -393,7 +393,7 @@ type overlappingCertificate struct {
 func isOverlappingCertificate(cert1DNSNames, cert2DNSNames []string) *overlappingCertificate {
 	for _, dns1 := range cert1DNSNames {
 		for _, dns2 := range cert2DNSNames {
-			if isOverlappingHostname(ptr.To(gwapiv1.Hostname(dns1)), ptr.To(gwapiv1.Hostname(dns2))) {
+			if areOverlappingHostnames(ptr.To(gwapiv1.Hostname(dns1)), ptr.To(gwapiv1.Hostname(dns2))) {
 				return &overlappingCertificate{
 					san1: dns1,
 					san2: dns2,
@@ -404,22 +404,31 @@ func isOverlappingCertificate(cert1DNSNames, cert2DNSNames []string) *overlappin
 	return nil
 }
 
-// isOverlappingHostname checks if two hostnames overlap.
-func isOverlappingHostname(hostname1, hostname2 *gwapiv1.Hostname) bool {
-	if hostname1 == nil || hostname2 == nil {
+func areOverlappingHostnames(this, other *gwapiv1.Hostname) bool {
+	if this == nil || other == nil {
 		return true
 	}
-	domain1 := strings.Replace(string(*hostname1), "*.", "", 1)
-	domain2 := strings.Replace(string(*hostname2), "*.", "", 1)
-	return isSubdomain(domain1, domain2) || isSubdomain(domain2, domain1)
+	return hostnameMatchesWithOther(this, other) || hostnameMatchesWithOther(other, this)
 }
 
-// isSubdomain checks if subDomain is a sub-domain of domain
-func isSubdomain(subDomain, domain string) bool {
-	if subDomain == domain {
-		return true
+// hostnameMatchesWithOther returns true if this hostname matches other hostname.
+// Assumes that hostnames will either be fully qualified or a wildcard hostname prefixed with a single wildcard.
+// E.g. "*.*.example.com" is not valid.
+func hostnameMatchesWithOther(this, other *gwapiv1.Hostname) bool {
+	thisString := string(*this)
+	otherString := string(*other)
+	if hasWildcardPrefix(other) && !hasWildcardPrefix(this) {
+		return strings.HasSuffix(thisString, otherString[1:]) &&
+			!strings.Contains(strings.TrimSuffix(thisString, otherString[1:]), ".") // not a subdomain
+	}
+	return thisString == otherString
+}
+
+func hasWildcardPrefix(h *gwapiv1.Hostname) bool {
+	if h == nil {
+		return false
 	}
-	return strings.HasSuffix(subDomain, fmt.Sprintf(".%s", domain))
+	return len(string(*h)) > 1 && string(*h)[0] == '*'
 }
 
 func buildListenerMetadata(listener *ListenerContext, gateway *GatewayContext) *ir.ResourceMetadata {

internal/gatewayapi/listener_test.go

@@ -95,7 +95,7 @@ func TestProxySamplingRate(t *testing.T) {
 	}
 }
 
-func TestIsOverlappingHostname(t *testing.T) {
+func TestAreOverlappingHostnames(t *testing.T) {
 	tests := []struct {
 		name      string
 		hostname1 *gwapiv1.Hostname
@@ -115,10 +115,10 @@ func TestIsOverlappingHostname(t *testing.T) {
 			want:      true,
 		},
 		{
-			name:      "two wildcards matches subdomain",
+			name:      "two wildcards with subdomain does not match",
 			hostname1: ptr.To(gwapiv1.Hostname("*.example.com")),
 			hostname2: ptr.To(gwapiv1.Hostname("*.test.example.com")),
-			want:      true,
+			want:      false,
 		},
 		{
 			name:      "nil hostname matches all",
@@ -139,22 +139,22 @@ func TestIsOverlappingHostname(t *testing.T) {
 			want:      true,
 		},
 		{
-			name:      "wildcard matches exact",
+			name:      "wildcard matches exactly one level of subdomain",
 			hostname1: ptr.To(gwapiv1.Hostname("*.example.com")),
 			hostname2: ptr.To(gwapiv1.Hostname("test.example.com")),
 			want:      true,
 		},
 		{
-			name:      "wildcard matches subdomain",
+			name:      "wildcard matches only one level of subdomain",
 			hostname1: ptr.To(gwapiv1.Hostname("*.example.com")),
 			hostname2: ptr.To(gwapiv1.Hostname("sub.test.example.com")),
-			want:      true,
+			want:      false,
 		},
 		{
-			name:      "wildcard matches empty subdomain",
+			name:      "wildcard does not match empty subdomain",
 			hostname1: ptr.To(gwapiv1.Hostname("*.example.com")),
 			hostname2: ptr.To(gwapiv1.Hostname("example.com")),
-			want:      true,
+			want:      false,
 		},
 		{
 			name:      "different domains",
@@ -180,15 +180,21 @@ func TestIsOverlappingHostname(t *testing.T) {
 			hostname2: ptr.To(gwapiv1.Hostname("testing-api.foo.dev")),
 			want:      false,
 		},
+		{
+			name:      "sub domain does not match with parent domain",
+			hostname1: ptr.To(gwapiv1.Hostname("api.foo.dev")),
+			hostname2: ptr.To(gwapiv1.Hostname("foo.dev")),
+			want:      false,
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := isOverlappingHostname(tt.hostname1, tt.hostname2); got != tt.want {
+			if got := areOverlappingHostnames(tt.hostname1, tt.hostname2); got != tt.want {
 				t.Errorf("isOverlappingHostname(%q, %q) = %v, want %v", ptr.Deref(tt.hostname1, ""), ptr.Deref(tt.hostname2, ""), got, tt.want)
 			}
 			// Test should be symmetric
-			if got := isOverlappingHostname(tt.hostname2, tt.hostname1); got != tt.want {
+			if got := areOverlappingHostnames(tt.hostname2, tt.hostname1); got != tt.want {
 				t.Errorf("isOverlappingHostname(%q, %q) = %v, want %v", ptr.Deref(tt.hostname2, ""), ptr.Deref(tt.hostname1, ""), got, tt.want)
 			}
 		})
@@ -301,15 +307,14 @@ func TestCheckOverlappingHostnames(t *testing.T) {
 							Name:     "listener-3",
 							Protocol: gwapiv1.HTTPSProtocolType,
 							Port:     443,
-							Hostname: ptr.To(gwapiv1.Hostname("sub.test.example.com")),
+							Hostname: ptr.To(gwapiv1.Hostname("sub.test.example.com")), // sub domain does not match with parent domain
 						},
 					},
 				},
 			},
 			expected: map[int]string{
 				0: "test.example.com",
 				1: "*.example.com",
-				2: "*.example.com",
 			},
 		},
 		{