Skip to content

agent/xds: allow trailing semicolon-separated fields in XFCC rbac principal - #23920

Merged
nitin-sachdev-29 merged 3 commits into
mainfrom
nitin/fix-peering-gateway-xfcc-rbac
Sep 11, 2026
Merged

nitin-sachdev-29 merged 3 commits into
mainfrom
nitin/fix-peering-gateway-xfcc-rbac

Conversation

@nitin-sachdev-29

Copy link
Copy Markdown
Contributor

Description

Fixes an authorization failure (HTTP 403 Forbidden / RBAC: access denied) when an API Gateway proxies cross-cluster or peered traffic to services with service intentions.

Root Cause

PR #23647 introduced automatic DNS SAN registration (DNS:*.api-gateway.consul, etc.) into API Gateway leaf certificates. When an API Gateway establishes an mTLS connection across a cluster peering (via mesh gateway or direct peer resolver), Envoy populates the x-forwarded-client-cert (XFCC) header with the client certificate details:

By=spiffe://...;Hash=...;Subject="";URI=spiffe://<trust-domain>/ns/<ns>/dc/<dc>/svc/<svc>;DNS=gateway.default.svc;DNS=gateway.default.svc.cluster.local,By=...

In agent/xds/rbac.go:xfccPrincipal, the Envoy RBAC regex pattern used to validate the caller's SPIFFE ID in the first XFCC hop was:

^[^,]+;URI=<idPattern>(?:,.*)?$

This regex assumed that ;URI=<idPattern> is either immediately followed by a comma (for subsequent hops) or ends the string. Because PR #23647 added ;DNS=... fields immediately following ;URI=..., the regex failed to match. Consequently, the destination service proxy's Envoy RBAC filter denied incoming requests with rbac_access_denied_matched_policy[none].

Solution

Relax the pattern in agent/xds/rbac.go to permit subsequent semicolon-separated key-value fields (such as ;DNS=... or ;Subject=...) in the first XFCC component before comma-separated subsequent hops or end-of-string:

- pattern := `^[^,]+;URI=` + idPattern + `(?:,.*)?$`
+ pattern := `^[^,]+;URI=` + idPattern + `(?:;[^,]*)?(?:,.*)?$`

Testing & Reproduction steps

1. Unit Tests

  • Added TestXFCCPrincipal in agent/xds/rbac_test.go asserting regex matching behavior across:
    • Standard sidecar proxy: single-hop and multi-hop without DNS SANs
    • API Gateway: single-hop and multi-hop with auto-registered DNS SANs
    • Negative security assertions: ensuring service name prefix spoofing (e.g. gateway2 matching gateway), mismatched service identities, and identities appearing only in subsequent hops are rejected.
  • Updated golden fixture agent/xds/testdata/rbac/default-deny-peered-kitchen-sink--httpfilter.golden.

2. Acceptance Test Reproduction & Verification

  • Reproduction: Set up dual-cluster KinD environment (kind-dc1, kind-dc2) and ran TestPeering_Gateway with an unfixed build. Reproduced the exact failure:
    < HTTP/1.1 403 Forbidden
    curl: (22) The requested URL returned error: 403
    [debug] envoy.rbac enforced denied, matched policy none
    [debug] envoy.http Preparing local reply with details rbac_access_denied_matched_policy[none]
    
  • Verification: Applied this fix and verified live curl immediately returned HTTP/1.1 200 OK ("hello world"). Ran the full end-to-end TestPeering_Gateway test suite from scratch on fresh clusters:
    --- PASS: TestPeering_Gateway (337.67s)
    PASS
    ok   github.com/hashicorp/consul-k8s/acceptance/tests/peering 338.139s
    

Links


PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Go Test Coverage: 63.1%

See the workflow run for the full per-package breakdown and downloadable HTML report.

@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.88%. Comparing base (c115f1d) to head (cf700e2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23920      +/-   ##
==========================================
+ Coverage   58.87%   58.88%   +0.01%     
==========================================
  Files         973      973              
  Lines      120951   120951              
==========================================
+ Hits        71205    71223      +18     
+ Misses      43138    43117      -21     
- Partials     6608     6611       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread agent/xds/rbac.go Outdated
@nitin-sachdev-29
nitin-sachdev-29 enabled auto-merge (squash) September 11, 2026 09:58
…ncipal

When PR #23647 introduced automatic DNS SAN registration for API Gateway
leaf certificates, Envoy began appending ';DNS=<san>' after ';URI=<uri>'
in the 'x-forwarded-client-cert' (XFCC) header. The previous regex anchored
';URI=' directly to either a comma or end-of-string:
    ^[^,]+;URI=<pattern>(?:,.*)?$

This caused RBAC intention evaluation to reject valid cross-cluster/peered
API Gateway traffic with 403 Forbidden.

This commit relaxes the pattern to allow any trailing semicolon-separated
fields (such as ;DNS=... or ;Subject=...) in the first XFCC hop before any
subsequent hops or end-of-string:
    ^[^,]+;URI=<pattern>(?:;[^,]*)?(?:,.*)?$
Address review feedback by simplifying the trailing separator matching to
(?:[;,].*)?$. This handles any combination of semicolon-delimited fields
(e.g. ;DNS=..., ;Subject=...) in the first XFCC component as well as
comma-separated multi-hop chains.
@nitin-sachdev-29
nitin-sachdev-29 force-pushed the nitin/fix-peering-gateway-xfcc-rbac branch from b52df3b to cf700e2 Compare September 11, 2026 10:29
Comment thread agent/xds/rbac.go

@anandmukul93 anandmukul93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mrgupta7 mrgupta7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nitin-sachdev-29
nitin-sachdev-29 merged commit a34796b into main Sep 11, 2026
196 of 210 checks passed
@nitin-sachdev-29
nitin-sachdev-29 deleted the nitin/fix-peering-gateway-xfcc-rbac branch September 11, 2026 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants