xds/rbac: enforce strict presence-based short-circuit in authenticatedMatcher (#9111)

This PR fixes a bug in the xDS RBAC authenticatedMatcher where it
incorrectly falls through URI/DNS SANs to the Subject DN. According to
gRFC A41 and Envoy's specification, only the first non-empty identity
source must be consulted. This bug allows an authorization bypass in
certain scenarios. A regression test is included.

RELEASE NOTES:
- xds/rbac: Fix `Authenticated` matcher to use the first non-empty
identity source (URI SAN, DNS SAN, and Subject DN) in the order
specified in gRFC A41.

Author: al4an444

internal/xds/rbac/matchers.go

@@ -435,17 +435,23 @@ func (am *authenticatedMatcher) match(data *rpcData) bool {
 		return am.stringMatcher.Match("")
 	}
 	cert := data.certs[0]
-	// The order of matching as per the RBAC documentation (see package-level comments)
-	// is as follows: URI SANs, DNS SANs, and then subject name.
-	for _, uriSAN := range cert.URIs {
-		if am.stringMatcher.Match(uriSAN.String()) {
-			return true
+	// Use the first non-empty identity source in priority order:
+	// URI SANs, then DNS SANs, then Subject.
+	if len(cert.URIs) > 0 {
+		for _, uriSAN := range cert.URIs {
+			if am.stringMatcher.Match(uriSAN.String()) {
+				return true
+			}
 		}
+		return false
 	}
-	for _, dnsSAN := range cert.DNSNames {
-		if am.stringMatcher.Match(dnsSAN) {
-			return true
+	if len(cert.DNSNames) > 0 {
+		for _, dnsSAN := range cert.DNSNames {
+			if am.stringMatcher.Match(dnsSAN) {
+				return true
+			}
 		}
+		return false
 	}
 	return am.stringMatcher.Match(cert.Subject.String())
 }

internal/xds/rbac/rbac_engine_test.go

@@ -1111,8 +1111,8 @@ func (s) TestChainEngine(t *testing.T) {
 				},
 			},
 		},
-		// This test tests that when there are no SANs or Subject's
-		// distinguished name in incoming RPC's, that authenticated matchers
+		// This test verifies that when there are no SANs or Subject
+		// distinguished name in incoming RPCs, authenticated matchers
 		// match against the empty string.
 		{
 			name: "default-matching-no-credentials",
@@ -1131,10 +1131,9 @@ func (s) TestChainEngine(t *testing.T) {
 				},
 			},
 			rbacQueries: []rbacQuery{
-				// This incoming RPC Call should match with the service admin
-				// policy. No authentication info is provided, so the
-				// authenticated matcher should match to the string matcher on
-				// the empty string, matching to the service-admin policy.
+				// This incoming RPC should match the service admin policy. No
+				// authentication info is provided, so the authenticated matcher
+				// evaluates the empty string.
 				{
 					rpcData: &rpcData{
 						fullMethod: "some method",
@@ -1156,6 +1155,18 @@ func (s) TestChainEngine(t *testing.T) {
 							},
 						},
 					},
+					wantStatusCode: codes.PermissionDenied,
+				},
+				{
+					rpcData: &rpcData{
+						fullMethod: "some method",
+						peerInfo: &peer.Peer{
+							Addr: &addr{ipAddress: "0.0.0.0:8080"},
+							AuthInfo: credentials.TLSInfo{
+								State: tls.ConnectionState{},
+							},
+						},
+					},
 					wantStatusCode: codes.OK,
 				},
 			},
@@ -1932,3 +1943,95 @@ func createXDSTypedStruct(t *testing.T, in map[string]any, name string) *anypb.A
 	}
 	return customConfig
 }
+
+func TestAuthenticatedMatcherIdentitySourcePrecedence(t *testing.T) {
+	tests := []struct {
+		name            string
+		uris            []string
+		dnsNames        []string
+		subjectCN       string
+		matcherContains string
+		wantMatch       bool
+	}{
+		{
+			name:            "uriSANPresentAndMatches",
+			uris:            []string{"spiffe://corp.example/svc/legitimate"},
+			matcherContains: "spiffe://corp.example/svc/legitimate",
+			wantMatch:       true,
+		},
+		{
+			name:            "uriSANPresentDoesNotFallbackToDNS",
+			uris:            []string{"spiffe://corp.example/svc/other"},
+			dnsNames:        []string{"spiffe://corp.example/svc/legitimate"},
+			matcherContains: "spiffe://corp.example/svc/legitimate",
+			wantMatch:       false,
+		},
+		{
+			name:            "uriSANPresentDoesNotFallbackToSubject",
+			uris:            []string{"spiffe://corp.example/svc/other"},
+			subjectCN:       "spiffe://corp.example/svc/legitimate",
+			matcherContains: "spiffe://corp.example/svc/legitimate",
+			wantMatch:       false,
+		},
+		{
+			name:            "dnsSANPresentAndMatchesWhenNoURI",
+			dnsNames:        []string{"svc.legitimate.internal"},
+			matcherContains: "svc.legitimate.internal",
+			wantMatch:       true,
+		},
+		{
+			name:            "dnsSANPresentDoesNotFallbackToSubject",
+			dnsNames:        []string{"svc.other.internal"},
+			subjectCN:       "svc.legitimate.internal",
+			matcherContains: "svc.legitimate.internal",
+			wantMatch:       false,
+		},
+		{
+			name:            "subjectUsedWhenNoURINorDNS",
+			subjectCN:       "svc.legitimate.internal",
+			matcherContains: "svc.legitimate.internal",
+			wantMatch:       true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var uriSANs []*url.URL
+			for _, u := range tt.uris {
+				parsed, err := url.Parse(u)
+				if err != nil {
+					t.Fatalf("url.Parse(%q) failed: %v", u, err)
+				}
+				uriSANs = append(uriSANs, parsed)
+			}
+			cert := &x509.Certificate{
+				URIs:     uriSANs,
+				DNSNames: tt.dnsNames,
+				Subject: pkix.Name{
+					CommonName: tt.subjectCN,
+				},
+			}
+
+			amConfig := &v3rbacpb.Principal_Authenticated{
+				PrincipalName: &v3matcherpb.StringMatcher{
+					MatchPattern: &v3matcherpb.StringMatcher_Contains{
+						Contains: tt.matcherContains,
+					},
+				},
+			}
+			matcher, err := newAuthenticatedMatcher(amConfig)
+			if err != nil {
+				t.Fatalf("failed to create matcher: %v", err)
+			}
+
+			rpcData := &rpcData{
+				authType: "tls",
+				certs:    []*x509.Certificate{cert},
+			}
+
+			if got := matcher.match(rpcData); got != tt.wantMatch {
+				t.Errorf("match() = %v, want %v", got, tt.wantMatch)
+			}
+		})
+	}
+}