Skip to content

Document request-claim fallback provenance - #6656

Merged
rdimitrov merged 3 commits into
mainfrom
rdimitrov/document-request-claim-fallback-provenance
Sep 14, 2026
Merged

rdimitrov merged 3 commits into
mainfrom
rdimitrov/document-request-claim-fallback-provenance

Conversation

@rdimitrov

@rdimitrov rdimitrov commented Sep 14, 2026

Copy link
Copy Markdown
Member

Summary

  • The operator guide told Google and GitHub users the wrong thing. It stated there is "one exception" where Cedar evaluates the ToolHive-issued token's own claims — delegated and JWT-bearer tokens — and omitted the opaque-access-token path entirely. Google (ya29.*) and GitHub (gho_*) access tokens are opaque, so that path is the normal state for those upstreams, not an edge case. An operator following this guide would believe their Cedar policies evaluate upstream-asserted claims when they do not.
  • The same omission sat on the field operators actually configure. ConfigOptions.PrimaryUpstreamProvider described the session-less nil map as the sole case where request-token claims are evaluated; the opaque path does the same thing and went unmentioned.
  • The multi-upstream caveat was filed under the wrong path. docs/authz.md explained that fallback claim_email/claim_name need not belong to the pinned provider — but only in the delegation section. Moved it to a shared subsection both fallback paths link to, split by grant type, since what the fallback token carries genuinely differs: the opaque path mirrors the first upstream, RFC 8693 delegation copies from the subject token (an external IdP's own assertion when that token came from a trusted external issuer), and RFC 7523 JWT-bearer carries neither claim.
  • Clarified "primary upstream" in the chain-state struct, which used it to mean first in the chain — distinct from primaryUpstreamProvider, the upstream Cedar trusts for claims.

Comments and documentation only — no behavior change.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

No code paths change, so there is nothing to exercise. Verification performed:

  • task build passes on this branch (and on main), confirming the comment-only Go edits compile.
  • Every factual claim in the new prose read back against the implementation: resolveClaims and the claim-source constants (pkg/authz/authorizers/cedar/core.go:659, :163-190); the opaque path's mirror origin (pkg/authserver/server/handlers/callback.go:120-153 -> pkg/authserver/server/session/session.go:151-157); the delegation and JWT-bearer claim origins (pkg/authserver/server/tokenexchange/handler.go:198-205, jwt_bearer_handler.go:204, validator.go:401-405); the pinned-provider default (cmd/thv-operator/pkg/vmcpconfig/converter.go:420-424); and the CLI-path override (pkg/runner/middleware.go:582-584).
  • All cross-references confirmed to resolve to real headings.
  • gofmt -l clean on both touched Go files.

task lint-fix fails on this machine for a pre-existing environment reason unrelated to this branch: golangci-lint's typecheck cannot read Go 1.27.1 export data, reported against pkg/security/security.go, which this PR does not touch. I confirmed the identical failure on the base commit, so CI should be the judge of lint.

The one red CI check, pkg/secrets/keyring/keyctl_linux_test.go:59, is a kernel-keyring flake unrelated to this diff.

Changes

File Change
docs/operator/virtualmcpserver-kubernetes-guide.md Document the opaque-token fallback alongside the session-less one; correct what profile claims each path carries
docs/authz.md Add "What the fallback claims actually are", split by grant type; note the divergence is operator-only; point the table row and the delegation bullet at it
pkg/authz/authorizers/cedar/core.go Comment-only: note the opaque-token fallback on ConfigOptions.PrimaryUpstreamProvider
pkg/authserver/storage/types.go Comment-only: say "first upstream in the chain", and call out why that is not PrimaryUpstreamProvider

Does this introduce a user-facing change?

No — documentation only. It does correct guidance that could have led operators to write Cedar policies on the assumption that claims were upstream-asserted when they were not.

Special notes for reviewers

This came out of triaging GHSA-ch9x-m6fv-82vp, which reported the opaque-token fallback as a vulnerability. It is not: the fallback is deliberate (#5147, fixing the #5146 deny-all for Google/GitHub upstreams), is surfaced to policies through the unspoofable thv_claim_source attribute, and is covered by tests that stop the degraded path from impersonating a healthy one. What the report did expose is that the documentation described the degradation for only one of the two paths that reach it.

There is a genuine but narrow claim-resolution gap underneath, tracked in #6655. It is deliberately not addressed here: the fix is a no-op for GitHub-style upstreams (no id_token is stored at all) and for single-upstream OIDC (the mirror and the id_token carry the same value), so it only changes behavior for a multi-upstream chain pinned to a later leg, and it raises a design question about which identity that branch should assert.

One pre-existing inaccuracy is left alone as out of scope: the guide's "No upstream ever logged the subject in" is false for RFC 8693 delegation — the subject did log in, the delegated token just carries no tsid link (tokenexchange/handler.go:200). Happy to fold that in if a reviewer prefers.

Generated with Claude Code

Cedar's request-token fallback paths were documented as if only delegated
and JWT-bearer tokens could reach them. The opaque-access-token path was
missing from the operator guide entirely, even though Google and GitHub
upstreams always take it, and the multi-upstream caveat that fallback
profile claims belong to the first upstream in the chain was recorded only
under delegation although it applies to both fallback paths equally.

"Primary upstream" also meant two different things across the codebase:
first in the chain, and the provider Cedar trusts for claims. They
coincide by default, so the collision was easy to miss and load-bearing
for authorization when pinning explicitly.

Comments and documentation only; no behavior change. The claim-resolution
gap these docs describe is tracked separately in #6655.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Sep 14, 2026
@rdimitrov

Copy link
Copy Markdown
Member Author

/retest

The first draft claimed both request-token fallback labels mirror name and
email from the first upstream in the chain. That is true only on the
opaque-token path. RFC 7523 JWT-bearer tokens carry neither claim, and RFC
8693 delegated tokens copy them from the subject token, which holds an
external IdP's own assertion when that token came from a trusted external
issuer. The draft also contradicted correct existing text in the same file.

Split the explanation by grant instead, and drop the paragraph naming the
two senses of "primary upstream": after this branch renames the chain-state
comments, no first-in-chain sense remains for it to disambiguate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Sep 14, 2026
ConfigOptions.PrimaryUpstreamProvider described the session-less nil map as
the sole case where request-token claims are evaluated. The opaque-token
path does the same thing and went unmentioned on the field an operator
configures, which is the omission this branch already fixes in the operator
guide.

Also scope the divergence warning: the CLI always resolves the provider to
the first upstream, so pinning to a later leg is reachable only through the
operator.

Co-Authored-By: Claude Opus 5 (1M context) <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 size/S Small PR: 100-299 lines changed labels Sep 14, 2026
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.98%. Comparing base (e532cf0) to head (ef4619a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6656   +/-   ##
=======================================
  Coverage   78.98%   78.98%           
=======================================
  Files         782      782           
  Lines       78065    78065           
=======================================
+ Hits        61658    61662    +4     
+ Misses      16402    16398    -4     
  Partials        5        5           

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

@rdimitrov

Copy link
Copy Markdown
Member Author

/retest

@rdimitrov
rdimitrov merged commit c262106 into main Sep 14, 2026
108 of 110 checks passed
@rdimitrov
rdimitrov deleted the rdimitrov/document-request-claim-fallback-provenance branch September 14, 2026 20:25
@github-actions github-actions Bot mentioned this pull request Sep 18, 2026
2 tasks
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