Skip to content

xds/rbac: skip degenerate empty URI SANs in authenticatedMatcher - #9123

Closed
tobypang-agent wants to merge 2 commits into
grpc:masterfrom
tobypang-agent:fix/empty-uri-san-rbac-bypass
Closed

xds/rbac: skip degenerate empty URI SANs in authenticatedMatcher#9123
tobypang-agent wants to merge 2 commits into
grpc:masterfrom
tobypang-agent:fix/empty-uri-san-rbac-bypass

Conversation

@tobypang-agent

@tobypang-agent tobypang-agent commented May 13, 2026

Copy link
Copy Markdown

Summary

The authenticatedMatcher in the xDS RBAC engine treats len(cert.URIs) > 0 as "URI SANs are present". However, a certificate can contain a degenerate empty URI SAN (url.URL{}) where String() returns "". This satisfies the length check but produces an empty-string identity, which:

This PR adds a pre-scan to skip degenerate URI entries before deciding whether URI SANs constitute a present identity source.

Root cause

Commit 94b9449 (#9111) correctly enforces strict identity source precedence per gRFC A41. But the len(cert.URIs) > 0 check does not account for Go's x509.Certificate.URIs containing entries like &url.URL{}, which x509.CreateCertificate() accepts without error.

Fix

Before treating URI SANs as "present", scan for at least one entry where String() != "". If all URI entries are degenerate, fall through to DNS SANs as if no URI SANs existed.

Test plan

  • New TestAuthenticatedMatcherEmptyURISAN with 5 cases covering empty URI SAN fallthrough, mixed empty/valid URIs, and no-fallthrough when valid URIs exist
  • All existing TestAuthenticatedMatcherIdentitySourcePrecedence tests pass (no regression)

RELEASE NOTES:

  • xds/rbac: Fix authenticatedMatcher to skip degenerate empty URI SANs when determining the identity source, preventing silent DENY rule evasion.

A certificate containing an empty URI SAN (url.URL{}) causes
authenticatedMatcher to treat the URI SAN list as "present" since
len(cert.URIs) > 0, but url.URL{}.String() returns "", producing
a degenerate empty-string identity. This prevents fallthrough to
DNS SANs or Subject, and causes DENY rules using SPIFFE-based
prefix matching to be silently evaded.

This change adds a pre-scan to skip degenerate URI entries before
deciding whether URI SANs constitute a present identity source.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@linux-foundation-easycla

linux-foundation-easycla Bot commented May 13, 2026

Copy link
Copy Markdown

CLA Missing ID

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.03%. Comparing base (7c47d2a) to head (562cd89).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9123      +/-   ##
==========================================
- Coverage   83.21%   83.03%   -0.18%     
==========================================
  Files         413      413              
  Lines       33482    33487       +5     
==========================================
- Hits        27863    27807      -56     
- Misses       4212     4249      +37     
- Partials     1407     1431      +24     
Files with missing lines Coverage Δ
internal/xds/rbac/matchers.go 78.08% <100.00%> (+0.51%) ⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tobypang-agent

Copy link
Copy Markdown
Author

I have signed the CLA. Please recheck.

@easwars

easwars commented May 27, 2026

Copy link
Copy Markdown
Contributor

We discussed this PR amongst the gRPC maintainers and we don't consider that this is actually fixing a real issue. If a CA really signed a certificate with an empty URI, then we don't want to second guess the CA.

If you are willing to point us to some internet standards specifications that suggest that this is a real possibility and that this is a vulnerability that systems have to deal with, we will be happy to consider this.

Thanks for your contribution.

@easwars easwars closed this May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants