Skip to content

Bound delegated token audience by subject token - #5882

Merged
jhrozek merged 2 commits into
stacklok:mainfrom
jhrozek:te-audience-containment
Jul 20, 2026
Merged

Bound delegated token audience by subject token#5882
jhrozek merged 2 commits into
stacklok:mainfrom
jhrozek:te-audience-containment

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Token exchange (RFC 8693) bounded the delegated token's scopes by the subject token but not its audience. A confidential client registered for audiences A and B could exchange a user token minted only for A into a delegated token for B — an escalation to a resource the user never consented to. grantScopes already narrows scopes to the subject token; audience had no equivalent bound.

This adds ensureAudienceSubsetOfSubject, enforced after all audiences are granted (from the audience param, the resource param, or the default-audience fallback), so every granted audience must be covered by the subject token's own aud. Delegation narrows, never broadens, the resource boundary.

Note: this handler is not yet wired up to live confidential clients, so no active deployment is affected — this lands the fix ahead of that.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Test plan

  • Unit tests added: audience escalation blocked by subject token; audience within subject token granted.
  • Existing token-exchange handler tests pass unchanged (subject tokens carry aud=[testIssuer], which the default/resource grants stay within).

Generated with Claude Code

Token exchange bounded the delegated token's scopes by the subject
token but not its audience. A confidential client registered for
audiences A and B could exchange a user token minted only for A into a
delegated token for B — an escalation to a resource the user never
consented to.

Add ensureAudienceSubsetOfSubject, enforced after all audiences are
granted, so every granted audience must be covered by the subject
token's own aud. Delegation narrows, never broadens, the resource
boundary, mirroring how grantScopes already bounds scopes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 20, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for closing this — it's exactly the audience-escalation gap flagged on #5822/#5881, and the fix is correct. I'd approve once CI is green.

Verified against the code:

  • Correct placement: ensureAudienceSubsetOfSubject runs at handler.go:149, after all three audience-grant paths (grantAudiences:130, grantResourceAudience:134, grantDefaultAudience:138), so GetGrantedAudience() reflects every granted audience regardless of whether it came from the audience param, the resource param, or the default fallback — no bypass.
  • Fail-closed & exact: every granted audience must be an exact member of the subject token's aud; an empty subject aud rejects everything (and the validator already guarantees ≥1). Correctly mirrors the grantScopes narrowing precedent, and ErrInvalidTarget is the right RFC 8693 error code.
  • Tests: both directions covered (escalation to a non-subject audience blocked; audience within the subject token granted).

🔴 One mechanical blocker

Lint fails: handler.go:79 — the added branch tips HandleTokenEndpointRequest to gocyclo 16 (>15). Extract the audience sequence into a helper — e.g. a grantAndBoundAudiences(ctx, requester, client, validatedClaims.Audience) that wraps grantAudiencesgrantResourceAudiencegrantDefaultAudienceensureAudienceSubsetOfSubject — which both drops the complexity back under the limit and reads as one "resolve + bound the delegated audience" step.

🟡 Minor

Behavior tightening worth a word in the PR/commit: if the default-audience fallback ever grants an audience not in the subject token's aud, this now rejects an exchange that previously succeeded. That's the correct, intended direction (delegation must not broaden), and in practice AS-issued subject tokens carry the AS audience so the happy path is unaffected — just calling it out so it's a deliberate change.

Approve on green after the gocyclo extraction.

🤖 AI-assisted review via Claude Code (security/OAuth). Line numbers against f17b70e.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5882      +/-   ##
==========================================
+ Coverage   71.34%   71.37%   +0.03%     
==========================================
  Files         693      693              
  Lines       70587    70600      +13     
==========================================
+ Hits        50358    50393      +35     
+ Misses      16596    16556      -40     
- Partials     3633     3651      +18     

☔ 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.

Extract the audience grant sequence and subset check into
grantAndBoundAudiences, keeping HandleTokenEndpointRequest under the
gocyclo threshold. No behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 20, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. ✅ This closes the audience-escalation gap from the #5822/#5881 thread, and the fix is verified correct:

  • Correct & fail-closed: every granted audience must be an exact member of the subject token's aud; an empty subject aud rejects everything (validator guarantees ≥1). ErrInvalidTarget is the right RFC 8693 code, and it mirrors the grantScopes narrowing precedent.
  • No bypass after the refactor: the new grantAndBoundAudiences helper wraps grantAudiencesgrantResourceAudiencegrantDefaultAudienceensureAudienceSubsetOfSubject in the same order, so the subset check still runs over the full GetGrantedAudience() regardless of which grant path set it — a clean, behavior-preserving extraction that also resolves the gocyclo finding.
  • Tests cover both directions (escalation blocked; audience within the subject granted).

Lint and swagger checks are green. The only red is MCP Conformance / tools-call-sampling — the pre-existing sampling flake, which is unrelated to this PR (it touches only tokenexchange/handler.go, nothing in the streamable proxy) and is being tracked separately.

Nice quick turnaround on the extraction.

🤖 AI-assisted review via Claude Code (security/OAuth). Line numbers/verification against edc0996.

@jhrozek
jhrozek merged commit 62496e9 into stacklok:main Jul 20, 2026
72 of 74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants