Filter upstream auth chain via callback hook#5725
Conversation
The embedded OAuth authorization server walks every configured upstream on every authorization, prompting for and storing tokens that a given authorization may not need. This widens the stored-token surface and forces needless upstream prompts. Add an optional UpstreamFilter hook (WithUpstreamFilter), consulted once in the callback handler after the first upstream resolves, that narrows the remaining chain to a subset of the configured upstreams: - The first upstream is always required and is never passed to nor removable by the filter. - The kept set is computed once and carried in PendingAuthorization (ChainUpstreams) so the filter is not re-run per leg. - nextMissingUpstream and the cross-leg identity check now operate on the effective chain rather than the raw config. - A filter error fails the authorization with a server error; it never silently falls back to walking every upstream. With no filter configured, behaviour is unchanged: the handler walks all configured upstreams as before. SingleLeg flows are unaffected and the upstream-token storage key contracts are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5725 +/- ##
=======================================
Coverage 70.65% 70.66%
=======================================
Files 682 682
Lines 68854 68925 +71
=======================================
+ Hits 48651 48707 +56
- Misses 16657 16668 +11
- Partials 3546 3550 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security, architecture-go, storage-persistence, test-coverage, general-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Legacy-pending recompute can use the wrong leg's request context for the filter | 9/10 | MEDIUM | Fix |
| 2 | Stored ChainUpstreams is trusted without validation against configured upstreams |
7/10 | MEDIUM | Fix |
| 3 | Identity-mismatch log loses structured fields after the verifyChainIdentity extraction |
7/10 | MEDIUM | Fix |
| 4 | Missing backward-compatibility test for legacy Redis pending records lacking chain_upstreams |
7/10 | MEDIUM | Fix |
Overall
This adds an optional UpstreamFilter hook, injected via WithUpstreamFilter, that narrows a multi-upstream OAuth authorization chain to a subset after the first upstream resolves. The implementation matches the linked issue's proposed shape closely, computes the effective chain once and carries it forward in PendingAuthorization.ChainUpstreams rather than re-invoking the filter per leg, and fails closed (server error, no fallback to a full walk) on filter error. computeChain's narrowing logic is small, pure, and has thorough table-driven coverage of every scenario called out in the linked issue. For existing no-filter deployments, behavior is unchanged end to end.
The findings below are all MEDIUM and none block merging. The most notable one (#1) is a narrow edge case in the "empty ChainUpstreams" recompute branch: it can't distinguish a genuine first leg from a non-first leg whose pending predates this field (a realistic rolling-deploy scenario for the Redis backend), so a context-sensitive filter could see the wrong leg's request context during that window. The other three are smaller and self-contained: a defense-in-depth gap where a chain loaded from storage isn't cross-checked against the configured upstream list, a logging regression where the identity-mismatch check lost its structured slog fields during extraction into verifyChainIdentity, and a test-coverage gap for the legacy-Redis-record backward-compatibility case (the repo already has a precedent for exactly this kind of test at redis_test.go:1109).
Documentation
UpstreamFilter's doc comment (pkg/authserver/server/handlers/handler.go) states it receives "the request context of the first leg's callback" — worth adding a note there about the rolling-upgrade edge case in finding #1, and mentioning that the context it receives already carries auth.WithPlatformUser, since that's the most concrete signal a real filter implementation would key off of.
Generated with Claude Code
Addresses #5725 review comments: - MEDIUM callback.go (3525733910): distinguish a true first leg (ResolvedUserID == "") from a subsequent leg; reject a subsequent-leg pending that lacks a computed chain instead of recomputing the filter against the wrong leg's request context. - MEDIUM callback.go (3525733913): validate a chain loaded from storage (non-empty, leads with the first configured upstream, names only configured upstreams) so a tampered PendingAuthorization row cannot shrink the chain and disable the cross-leg identity check. - MEDIUM callback.go (3525733916): restore discrete expected/got/provider slog fields on the identity-mismatch check after the verifyChainIdentity extraction, so log pipelines can still alert on it. - MEDIUM redis_test.go (3525733919): add a backward-compatibility test that loads a legacy pending JSON lacking chain_upstreams and asserts an empty ChainUpstreams. Also expands the UpstreamFilter doc comment (first-leg-context guarantee across rolling upgrades, WithPlatformUser in ctx) and updates existing chain tests to carry ChainUpstreams on subsequent legs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/L
tgrunnagle
requested review from
ChrisJBurns,
JAORMX,
jhrozek and
rdimitrov
as code owners
A useful upstream filter needs to key its narrowing decision on who authenticated — the subject and the authorization-relevant claims from the first upstream — not just on request context. Widen the hook to receive that identity explicitly. - FilterUpstreams now takes the canonical platform user ID and the resolved auth.PrincipalInfo (claim-mapped Subject, Name, Email, and the Claims map used by authz policies) alongside the configured upstreams. The principal is passed as an explicit parameter, not via context: the callback deliberately avoids placing a bearer-less Identity in ctx. - Capture the upstream claims that previously had nowhere to live: add Claims to upstream.Identity, populated from the validated ID token (OIDC) and the userinfo response (OAuth2). identityFromToken and synthetic OAuth2 resolve no structured claim set and leave it nil. - The callback builds the principal from the first leg's resolved identity and threads it through continueChainOrComplete -> resolveChain -> computeChain to the filter. Tests: an OIDC provider integration test (real provider + mock IdP) asserting ID-token claims are captured into the resolved Identity, and a callback integration test asserting the first upstream's subject and claims reach FilterUpstreams. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/L
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Went through this carefully, including tracing the storage / replay / cross-leg interleave angles. Overall it's solid: session isolation holds via the fresh per-/authorize sessionID, the filter can only narrow (never reorder or extend the chain), every failure path fails closed with token cleanup, and ChainUpstreams is cloned on all four storage crossings. Build and the authserver package tests pass locally.
Approving. A few non-blocking things:
- validateChain doesn't fully enforce what its doc comment claims — the one substantive item, left inline.
- NewHandler (pkg/authserver/server/handlers/handler.go) doesn't reject duplicate upstream names, even though NamedUpstream's doc says they must be unique. upstreamByName returns first-match and tokens key by name, and this PR now keys the chain itself by name, so the invariant is newly load-bearing. Worth a dedup check in the existing validation loop so it fails loudly at construction. (Pre-existing, so noting it here rather than inline since that block isn't in the diff.)
- Two doc clarifications inline (the filter Claims contract, and the verifyChainIdentity scope wording).
| // validateChain rejects an upstream chain loaded from storage that does not agree | ||
| // with the configured upstreams. A valid chain is non-empty, leads with the | ||
| // required first upstream, and names only configured upstreams. This guards the | ||
| // reuse path against a corrupt or tampered PendingAuthorization row shrinking an | ||
| // in-flight chain to skip legs — which would also silently disable the cross-leg | ||
| // identity check, since verifyChainIdentity is a no-op for a single-element chain. | ||
| func (h *Handler) validateChain(chain []string) error { | ||
| if len(chain) == 0 { | ||
| return fmt.Errorf("chain is empty") | ||
| } | ||
| if chain[0] != h.upstreams[0].Name { | ||
| return fmt.Errorf("chain must lead with the first configured upstream %q, got %q", | ||
| h.upstreams[0].Name, chain[0]) | ||
| } | ||
| for _, name := range chain { | ||
| if _, ok := h.upstreamByName(name); !ok { | ||
| return fmt.Errorf("chain contains unconfigured upstream %q", name) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
validateChain only checks that the chain is non-empty, leads with the first upstream, and that every name is configured — it doesn't check the chain is a faithful in-order subset of the config. A row shrunk to just ["provider-1"] passes here, skips the backend legs, and then slips past verifyChainIdentity (which no-ops on a single-element chain); ["provider-1","provider-1"] even satisfies that identity check trivially. It needs write access to the pending store to hit, so it's not really exploitable in practice — but the doc comment promises tamper-resistance we don't actually deliver. Enforcing an in-order, dedup'd subsequence closes it with no new state:
| // validateChain rejects an upstream chain loaded from storage that does not agree | |
| // with the configured upstreams. A valid chain is non-empty, leads with the | |
| // required first upstream, and names only configured upstreams. This guards the | |
| // reuse path against a corrupt or tampered PendingAuthorization row shrinking an | |
| // in-flight chain to skip legs — which would also silently disable the cross-leg | |
| // identity check, since verifyChainIdentity is a no-op for a single-element chain. | |
| func (h *Handler) validateChain(chain []string) error { | |
| if len(chain) == 0 { | |
| return fmt.Errorf("chain is empty") | |
| } | |
| if chain[0] != h.upstreams[0].Name { | |
| return fmt.Errorf("chain must lead with the first configured upstream %q, got %q", | |
| h.upstreams[0].Name, chain[0]) | |
| } | |
| for _, name := range chain { | |
| if _, ok := h.upstreamByName(name); !ok { | |
| return fmt.Errorf("chain contains unconfigured upstream %q", name) | |
| } | |
| } | |
| return nil | |
| } | |
| // validateChain rejects an upstream chain loaded from storage that does not agree | |
| // with the configured upstreams. A valid chain is a non-empty, in-order, | |
| // duplicate-free subsequence of the configured upstreams led by the required | |
| // first upstream. This guards the reuse path against a corrupt or tampered | |
| // PendingAuthorization row shrinking or reordering an in-flight chain to skip legs | |
| // — which would also silently disable the cross-leg identity check, since | |
| // verifyChainIdentity is a no-op for a single-element chain. | |
| // | |
| // Note: this validates that each name is currently configured, not that the name | |
| // still points at the same provider it did when the chain was computed. A name | |
| // rebound to a different IdP mid-flight (a rolling config change) still validates; | |
| // the frozen subject and the pending TTL bound that exposure. | |
| func (h *Handler) validateChain(chain []string) error { | |
| if len(chain) == 0 { | |
| return fmt.Errorf("chain is empty") | |
| } | |
| if chain[0] != h.upstreams[0].Name { | |
| return fmt.Errorf("chain must lead with the first configured upstream %q, got %q", | |
| h.upstreams[0].Name, chain[0]) | |
| } | |
| // Walk both, advancing the config cursor past each match. A name that never | |
| // matches — unconfigured, out of order, or a duplicate of an already-consumed | |
| // entry — is rejected, so the stored chain is a faithful narrowing of the config. | |
| ci := 0 | |
| for _, name := range chain { | |
| for ci < len(h.upstreams) && h.upstreams[ci].Name != name { | |
| ci++ | |
| } | |
| if ci >= len(h.upstreams) { | |
| return fmt.Errorf("chain entry %q is unconfigured, out of order, or duplicated", name) | |
| } | |
| ci++ // consume the matched upstream so it cannot be reused | |
| } | |
| return nil | |
| } |
Separately (nit): the rejection branches have no direct coverage right now — the existing missing-chain test trips the len==0 guard in resolveChain before validateChain runs. A small table test on validateChain (wrong-first / unconfigured / out-of-order / duplicate) would lock this down.
| // or synthetic upstreams, which resolve no structured claim set); Name and | ||
| // Email are populated when the upstream provides them. It carries NO tokens — | ||
| // it is the credential-free auth.PrincipalInfo projection. The filter MUST | ||
| // treat principal (including its Claims map) as read-only. |
There was a problem hiding this comment.
Since Claims now gets handed to operator-written filters, worth spelling out two things in this contract:
- The values are untyped and come straight from the upstream IdP, so implementations should assert types defensively rather than trusting shape — a malformed IdP response shouldn't be able to panic the filter.
- Claims can be nil on a transient OIDC claims-extraction failure too, not only for OAuth2/synthetic upstreams. Anything security-relevant should treat nil/absent Claims as fail-closed.
Minor, but also worth noting aud in here is the upstream IdP's client_id, not this authorization server.
| // upstream has no cross-legs to reconcile and the check is a no-op. Returns a | ||
| // non-nil error when the storage lookup fails or an identity mismatch is detected; | ||
| // the caller maps either to a server error. | ||
| func (h *Handler) verifyChainIdentity(ctx context.Context, sessionID string, chain []string, subject string) error { |
There was a problem hiding this comment.
Small doc thing: "identity consistency across chain legs" reads stronger than what this actually does — it only cross-checks chain[0] against the resolved subject. Intermediate legs aren't identity-checked, which is intentional (later upstreams are connect-this-backend flows whose identity can legitimately differ). Not a behavior change in this PR, just worth making the wording match the scope.
2 participants
Summary
The embedded OAuth authorization server walks every configured upstream provider
on every authorization. There is no way for a consumer to say that only a subset
of the configured upstreams is relevant for a given authorization, so the flow
prompts for upstreams the authorization does not need and stores upstream tokens
that are never used — widening the stored-token surface for no benefit.
This PR adds an optional filter hook, consulted once in the callback handler after
the first upstream resolves, that narrows the remaining chain to a subset of the
configured upstreams. The filter is given the identity established by the first
upstream — the canonical user plus the authorization-relevant claims — so it can
decide the subset based on who authenticated. The change is fully backward
compatible: with no filter configured, the handler walks all configured upstreams
exactly as before.
keyed on the authenticated principal — avoiding needless prompts and reducing
the stored-token surface.
UpstreamFilterinterface andWithUpstreamFilterconstructionoption. The filter receives the canonical platform user ID and the resolved
auth.PrincipalInfo(claim-mapped subject, name, email, and theClaimsmapused by authorization policies) alongside the non-first configured upstream
names. The effective chain is computed once on the first leg and carried in
PendingAuthorizationso the filter is not re-run per leg; subsequent legs reusethe carried chain after validating it against the configured upstreams. The
missing-leg walk and the cross-leg identity check operate on the effective chain
rather than the raw config. Upstream ID-token / userinfo claims — previously
discarded — are now captured into
upstream.Identityso the filter can use them.Closes #5724
Type of change
Test plan
task test)task test-e2e)task lint-fix)Unit and integration tests cover claim capture, the callback flow, chain
computation, identity propagation, and storage round-trips:
OIDCProviderImpl+ mock IdP): an ID tokencarrying
emailand a multi-valuedgroupsclaim is captured intoIdentity.Claims.the canonical platform user ID, reach
FilterUpstreamsduring callback handling.issues the code without prompting for the dropped leg.
pending authorization.
to a full walk).
is rejected and fails closed rather than recomputing against a later leg.
computeChaintable cases: no-filter full walk in order, single upstream skipsthe filter, subset kept with the first upstream always leading, empty keep set
leaves only the first upstream, filter return order ignored in favor of
configured order, unknown / first-upstream names in the keep set ignored, filter
error propagated, and the principal passed through to the filter verbatim.
nextMissingUpstreamrespects the chain subset rather than the raw config.ChainUpstreams(in-memory and Redis), plus abackward-compatibility test loading a legacy Redis pending without
chain_upstreams.Changes
pkg/authserver/server/handlers/handler.goUpstreamFilterinterface (takes the platform user ID +auth.PrincipalInfo+ configured upstreams),WithUpstreamFilteroption, andcomputeChain/resolveChain/validateChain;nextMissingUpstreamnow walks the effective chainpkg/authserver/server/handlers/callback.goverifyChainIdentity(with structured mismatch logging) gating on the effective chainpkg/authserver/storage/types.goChainUpstreamsfield toPendingAuthorizationpkg/authserver/storage/memory.go,redis.goChainUpstreamspkg/authserver/upstream/types.goClaimstoupstream.Identitypkg/authserver/upstream/oidc.go,oauth2.gopkg/authserver/**/*_test.goDoes this introduce a user-facing change?
No end-user-facing change to the
thvCLI. For consumers embedding the authserver, this adds a new optional construction option (
WithUpstreamFilter). It isopt-in and behavior is unchanged when unset.
Special notes for reviewers
upstreams[0]; it is not passed to the filter and cannot be removed by it.computeChainiterates theconfigured order and ignores any returned name that is not a non-first configured
upstream, so a filter cannot reorder the chain or inject unknown providers.
avoids placing a bearer-less
Identityinctx(no ToolHive bearer has beenminted yet). The filter receives the credential-free
auth.PrincipalInfo(Subject = claim-mapped upstream subject;
Claims= ID-token/userinfo claims,nil for
identityFromToken/ synthetic OAuth2) and must treat it as read-only.server error rather than reverting to walking every upstream.
ChainUpstreamsmust benon-empty, lead with the first configured upstream, and name only configured
upstreams — so a tampered pending row cannot shrink the chain to skip legs (which
would also disable the cross-leg identity check).
ChainUpstreamsfield existed is rejected (fail closed, forcing a freshauthorization) rather than recomputed against the wrong leg's context, preserving
the "filter runs once, on the first leg" guarantee.
SingleLegflows are unaffected and theupstream-token storage key contracts are unchanged.
Generated with Claude Code