Skip to content

fix(deps): bump mcp-oauth to the rotation-race branch build (giantswarm#37164)#977

Merged
paurosello merged 11 commits into
mainfrom
fix/mcp-oauth-rotation-race
Jul 21, 2026
Merged

fix(deps): bump mcp-oauth to the rotation-race branch build (giantswarm#37164)#977
paurosello merged 11 commits into
mainfrom
fix/mcp-oauth-rotation-race

Conversation

@paurosello

@paurosello paurosello commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What

Fixes the garm re-exchange rotation-storm deauth (giantswarm#37164) with two related muster source changes plus a dependency bump to a released mcp-oauth tag:

  1. Persist the reconnect ID token (a46fd8e) — a long-lived SSO session that reconnects after its login-time ID token expired now persists that token to the OAuth-proxy store, so muster's background SSO re-exchange stops falling back to the client-refresh-token-rotating path.
  2. Route the background SSO refresh to a provider-only path (4538c0d) — the background refresher now calls mcp-oauth's RefreshSessionProvider, which refreshes the upstream (dex) provider token without rotating the client's mcp refresh token.
  3. Dep bump github.com/giantswarm/mcp-oauth v1.0.10 → tagged v1.2.0 (fab38e5) — the released tag that carries the provider-only RefreshSessionProvider API on mainline (it was only on an untagged branch commit before; re-pinning to a real tag removes the provenance risk and prevents a later go get -u from silently dropping the fix).

Supersedes #981 (closed).

Why: an SSO session on graveler deauthed with the pre-fix build

A long-lived SSO session that reconnects after its login-time ID token expired (e.g. after a pod restart) re-inits SSO in initSSOForSession from the live request context, but never persisted that token to the OAuth-proxy store. The background re-exchange closure getIDTokenForForwarding runs on a detached context.Background() and can only read that store → finds nothing → no subject ID token available for re-exchange → falls back to the in-process refresher RefreshSessionRefreshAccessToken, which rotates the client's mcp refresh token.

On a token-exchange backend (garm-mcp-kubernetes) whose continuous-listen retries every ~1s, this rotated the refresh-token family ~56×/min for ~15 min until two rotations collided → OAuth 2.1 reuse detection revoked the whole family → deauth. Connect-time exchange kept succeeding the whole time (it reads the live request context), which is why it looked intermittent.

The fix

  1. Persist the request-context ID token in initSSOForSession (via the existing storeIDTokenForSSO, which no-ops on empty/unparseable tokens). Since onAuthenticated re-inits on active requests, the proxy store stays fresh while the session is active, so the background re-exchange resolves a subject from the store instead of falling through. Also drops the now-redundant explicit storeIDTokenForSSO call in SessionCreationHandler.
  2. Provider-only background refresh. Even with a subject resolved, the background refresher previously rotated the client-facing refresh token. It now routes to mcp-oauth's RefreshSessionProvider, which repopulates the upstream provider token (firing TokenRefreshHandler) without touching the client's mcp refresh token — removing the rotation hazard itself, not just its trigger.

⚠️ Deploy note (mcp-oauth Valkey layout change)

The new mcp-oauth build changes the Valkey provider-token storage layout (single shared entry per user; no legacy read-fallback). Rolling this out:

  1. Every user re-authenticates exactly once — existing sessions do not carry over.
  2. Optional: flush the affected Valkey token keys (token:*, refresh:*, and related meta:* / family:* / user-client set keys under the configured prefix) for a clean cutover. Skipping the flush is safe — leftover keys are never read and just linger until TTL; no false reuse/theft detection.

Make sure this reaches whoever runs the rollout, not just PR readers.

Testing

  • go build ./..., go vet ./internal/aggregator/ ./internal/server/
  • go test -race ./internal/aggregator/ ./internal/server/
  • TestInitSSOForSession_PersistsIDToken (+ empty-token no-op) — persist fix.
  • session_refresher_test.go asserts the background refresher delegates to RefreshSessionProvider; no bare RefreshSession callers remain.
  • Dep bump to v1.2.0 pulls newer transitive prometheus deps (client_golang 1.23.2 → 1.24.0, common, procfs).

@paurosello
paurosello requested a review from a team as a code owner July 17, 2026 08:35
@paurosello paurosello closed this Jul 17, 2026
@paurosello
paurosello deleted the fix/mcp-oauth-rotation-race branch July 17, 2026 08:53
@paurosello
paurosello restored the fix/mcp-oauth-rotation-race branch July 17, 2026 08:55
@paurosello paurosello reopened this Jul 17, 2026
@paurosello
paurosello force-pushed the fix/mcp-oauth-rotation-race branch from 3817131 to 09ff543 Compare July 20, 2026 07:44
…rm#37164)

Pull in the mcp-oauth refresh-token rotation-race fix (root cause 2 of giantswarm#37164) via a pseudo-version pinned to the fix branch (giantswarm/mcp-oauth#511), ahead of the tagged v1.1.0 release.

go.mod: mcp-oauth v1.0.12 -> v1.0.13-0.20260720074145-d91d0369e965 (branch build).

Includes the #511 GetToken provider-token-ref resolution fix, without which muster SSO / token-forwarding / RFC-8693 token-exchange regressed (14 integration scenarios). With it, 'muster test' is 178/178.

Signed-off-by: Pau Rosello <pau@giantswarm.io>
@paurosello
paurosello force-pushed the fix/mcp-oauth-rotation-race branch from 09ff543 to b007590 Compare July 20, 2026 07:48
…ntswarm#37164)

Replace the rotation-race branch pseudo-version with the tagged release now
that the fix has shipped upstream.

go.mod: mcp-oauth v1.0.13-0.20260720074145-d91d0369e965 (branch build) -> v1.1.1.

v1.1.0 ships the full rotation-race fix for giantswarm#37164 root cause 2 (all
four slices: shared per-user provider-token store, per-user single-flight
refresh lock, all provider-refresh entry points routed through the coordinator,
no-orphan reordering) plus the adversarial-review hardening fixes -- the exact
change the branch build was pinned ahead of the tag to get. v1.1.1 is the
latest patch on top.

Verified green: go build/vet, goimports/gofmt, golangci-lint (gosec+goconst, 0
issues), go test -race (all pass), and 'muster test' 178/178. CRDs regenerate
from the Go types with zero diff.

Signed-off-by: Pau Rosello <pau@giantswarm.io>
…on storm

A long-lived SSO session that reconnects after its login-time ID token has
expired (e.g. after a pod restart) re-inits its SSO backend connections in
initSSOForSession using the ID token from the live request context, but never
persisted that token to the OAuth-proxy store. The background re-exchange /
forwarding closure (getIDTokenForForwarding) runs on a detached
context.Background() and can only read that store, so it found nothing and
logged "no subject ID token available for re-exchange", then fell back to the
in-process refresher (RefreshSession -> RefreshAccessToken), which rotates the
client's mcp refresh token. On a token-exchange backend whose continuous-listen
retries every ~1s, this rotated the refresh-token family ~56x/min until two
rotations collided and OAuth 2.1 reuse detection revoked the whole family,
deauthing the session.

Persist the request-context ID token in initSSOForSession so the store stays
populated for as long as the session keeps making authenticated requests, which
lets the background re-exchange resolve a subject without triggering the
refresher. storeIDTokenForSSO already no-ops on empty/unparseable tokens.

This is independent of the mcp-oauth provider-token rotation race
(giantswarm/giantswarm#37164); a follow-up should stop the background
re-exchange from rotating the client-facing refresh token at all (an mcp-oauth
provider-only refresh that fires TokenRefreshHandler without rotating).
Latest tagged release; v1.1.1 was one behind.
@paurosello

Copy link
Copy Markdown
Contributor Author

Folded in a second, independent fix (commit a46fd8e) plus a bump to mcp-oauth v1.1.2 (f6bac04, latest tag).

Why: an SSO session on graveler still deauthed with this branch deployed

Investigation of the graveler deauth (session revoked at 09:52:43Z, family + all user/client tokens nuked) showed it was not the mcp-oauth provider-token rotation race this branch targets — it's a muster lifecycle gap:

  1. A long-lived SSO session reconnects after its login-time ID token expired (e.g. after a pod restart). onAuthenticated takes the authAlive branch and re-inits SSO via initSSOForSession using the ID token from the live request context, but never persisted it to the OAuth-proxy store.
  2. The background re-exchange closure getIDTokenForForwarding runs on a detached context.Background() and can only read that store → finds nothing → no subject ID token available for re-exchange → falls back to the in-process refresher RefreshSessionRefreshAccessToken, which rotates the client's mcp refresh token.
  3. On a token-exchange backend (garm-mcp-kubernetes) whose continuous-listen retries every ~1s, this rotated the refresh-token family ~56×/min for ~15 min until two rotations collided → OAuth 2.1 reuse detection revoked the family → deauth. Connect-time exchange kept succeeding the whole time (it reads the live request context), which is why it looked intermittent.

The fix (a46fd8e)

Persist the request-context ID token in initSSOForSession (via the existing storeIDTokenForSSO, which no-ops on empty/unparseable tokens). Since onAuthenticated re-inits on active requests, the proxy store now stays fresh while the session is active, so the background re-exchange resolves a subject from the store and never hits the rotating refresher.

Follow-up (not in this branch)

This kills the trigger but does not remove the underlying hazard that the background SSO re-exchange refresher rotates the client-facing refresh token at all. Proper follow-up: an mcp-oauth provider-only refresh that fires TokenRefreshHandler (repopulating the ID token) without rotating the client's mcp refresh token.

Tests: new TestInitSSOForSession_PersistsIDToken (+ empty-token no-op); internal/aggregator passes with -race, go vet clean, full module builds on v1.1.2. Supersedes #981.

@QuentinBisson QuentinBisson 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.

Reviewed the branch as checked out (not just the rendered diff). Sound change, no blocking defects — the one real problem is the description, which is stale and understates what's in the branch.

1. PR description is inaccurate

The body no longer matches the diff:

  • It says the pin is the pseudo-version v1.0.11-0.20260717070146-dbfd1a44d765; the branch is actually on tagged v1.1.2. That's good — it resolves the "replace pseudo-version with a tagged release" follow-up — but the body still lists that follow-up as open.
  • It states "No muster source changes — mcp-oauth's exported API is unchanged." That's not true: commit a46fd8e carries the initSSOForSession ID-token-persist fix + a 58-line test. Someone reading only the description wouldn't expect a behavioral change in the SSO re-init path.

Please rewrite the body to cover both changes (dep bump and the persist fix), or drop a46fd8e and let the source fix land via its own PR (see 3).

2. Dep bump is clean

mcp-oauth's go.mod hash is unchanged across v1.0.12 → v1.1.2, so no new transitive deps entered. go build ./internal/aggregator/ and the new test both pass against v1.1.2. The Valkey storage-layout deploy note (every user re-auths once, no legacy read-fallback) is a genuine operational risk and is documented well — just make sure it reaches whoever runs the rollout, not only PR readers.

3. Overlap with #981 — coordinate the merge

Commit a46fd8e is the same source change as #981. Landing both independently risks a duplicate/conflicting merge. Decide explicitly: either #981 owns the source fix and this PR is dep-bump-only, or this PR carries both and #981 is closed.

On the persist fix itself (shared with #981)

Correct and safe. On the authAlive reconnect branch, initSSOForSession runs on a detached context.Background(), so persisting the live request-context ID token is exactly what lets getIDTokenForForwarding resolve a subject instead of falling through to the rotating refresher. The external-bearer/OBO path isn't poisoned either — injectExternalIDToken already mirrors the same token under (sessionID, musterIssuer), so re-storing it is idempotent.

Two minor, non-blocking notes left inline.

Comment thread internal/aggregator/auth_resource.go Outdated
// OAuth 2.1 reuse detection revokes the family. Storing it here keeps the
// store fresh for as long as the session keeps making authenticated
// requests. storeIDTokenForSSO no-ops on empty/unparseable tokens.
if sso.tokens.IDToken != "" {

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.

Two minor, non-blocking notes:

  1. This guard duplicates the empty-check already inside storeIDTokenForSSO, and the test's second subtest exercises the empty case regardless. Harmless, just redundant — fine to keep for explicitness.
  2. storeIDTokenForSSO overwrites the store entry unconditionally (no "only if newer exp" guard), so in principle a request carrying an older ID token could regress a fresher stored entry. In practice the request-context token just passed validation, so this is a low-risk edge rather than a bug — worth a mental note if store-freshness anomalies ever show up.

Separately (can't inline, it's in server.go): SessionCreationHandler now calls storeIDTokenForSSO twice — once inside initSSOForSession, once explicitly right after. Idempotent, but the explicit second call is now dead weight and could be dropped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Empty-token guard — not changed (kept for explicitness, as you suggested).
  2. Unconditional overwrite / no newer-exp guard — not changed; agreed low-risk edge since the token just validated, noted to check here first if any store-freshness anomaly shows up.
  3. SessionCreationHandler double call — done, dropped the redundant storeIDTokenForSSO in 135b254.

…Handler

initSSOForSession now persists the request-context ID token to the
OAuth-proxy store itself (commit a46fd8e), so the explicit
storeIDTokenForSSO call right after it in SessionCreationHandler is dead
weight. Idempotent either way; removing it keeps a single source of
truth for the persist. Addresses review note on #977.
@paurosello

Copy link
Copy Markdown
Contributor Author

Per-point status on the review summary:

  1. Description inaccurate — done, rewrote the body to cover both the bump and the a46fd8e persist fix.
  2. Dep bump clean / deploy note — acknowledged (no code change); will get the Valkey re-auth/flush note to whoever runs the rollout, not just PR readers.
  3. Overlap with fix(oauth): persist reconnect ID token to stop SSO re-exchange rotation storm #981done, fix(oauth): persist reconnect ID token to stop SSO re-exchange rotation storm #981 is closed; this PR owns both changes.

…ntswarm#37164)

Defect #2 of the graveler re-exchange deauth: muster's background SSO
re-exchange, when its cached ID token expired, called the OAuth server's
RefreshSession — which delegates to RefreshAccessToken and rotates the
client-facing mcp refresh token. On a token-exchange backend whose listen
stream retries ~1/s this rotated the refresh-token family server-side until the
client's own refresh presented a superseded token and OAuth 2.1 reuse detection
revoked the whole family (deauth).

Reroute the background refresher to mcp-oauth's new provider-only
RefreshSessionProvider: it refreshes the upstream (dex) provider token and
repopulates the SSO ID token (via TokenRefreshHandler) WITHOUT rotating the
client's mcp refresh token. Renamed across the oauthServer interface, the
OAuthHTTPServer / LazyOAuthHTTPServer wrappers, and sessionRefresher; added a
wiring guard test (TestSessionRefresher_UsesProviderOnlyRefresh).

Bumps mcp-oauth to the branch build carrying RefreshSessionProvider
(v1.1.2-0.20260720165823-c4f618c5a6b1, giantswarm/mcp-oauth#524) for graveler
branch-build validation; will re-pin to the tagged release before merge.

Claude-Session: https://claude.ai/code/session_01PC362NB9bbCmGFCv6Goems
@paurosello
paurosello requested a review from QuentinBisson July 21, 2026 07:38

@QuentinBisson QuentinBisson 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.

Approving. The muster changes are correct and well-tested: the initSSOForSession persist keeps the proxy store populated on reconnect, and switching the background refresher to the provider-only RefreshSessionProvider removes the client refresh-token rotation that tripped OAuth 2.1 reuse detection (giantswarm#37164). Build/vet/tests pass with -race on the touched packages, and the rename is complete (no bare RefreshSession callers left).

One thing to resolve before this fix is durable, tracking separately rather than blocking:

Re-pin to a released mcp-oauth tag. go.mod currently pins the pseudo-version v1.1.2-0.20260720165823-c4f618c5a6b1, not the v1.1.2 tag. I checked: server/refresh_session_provider.go does not exist in v1.1.2, v1.1.3, or v1.1.4. The provider-only API lives only on the untagged branch commit c4f618c5, which diverged from mainline. Two risks:

  1. A later routine go get -u to any current mcp-oauth tag silently drops RefreshSessionProvider and regresses the deauth fix, since no released tag carries it.
  2. The fix's provenance is a branch commit that can be force-pushed or GC'd.

Please land the provider-only refresh in mcp-oauth main, cut a real tag (e.g. v1.1.5), and bump muster's pin to it. The PR body still says "bump to tagged v1.1.2 (latest tag)", which is not what go.mod pins, so update that too.

Non-blocking: add a CHANGELOG ### Fixed entry for the deauth fix plus a note that the mcp-oauth bump changes the Valkey token layout with no legacy read-fallback (every user re-authenticates once). The "Follow-up (not in this PR)" section of the body describes the provider-only refresh as future work, but this diff already wires it, so that section is stale.

Replaces the branch pseudo-version v1.1.2-0.20260720165823-c4f618c5a6b1
with the released v1.2.0 tag, which carries the provider-only
RefreshSessionProvider API on mainline. Removes the provenance risk of
pinning an untagged, force-pushable branch commit and prevents a future
go get -u from silently dropping the deauth fix.

v1.2.0 also pulls newer transitive prometheus deps (client_golang
1.23.2->1.24.0, common, procfs). Build/vet/race-tests pass on
internal/aggregator and internal/server.
…th v1.2.0 Valkey layout note (giantswarm#37164)
@paurosello
paurosello merged commit 9445486 into main Jul 21, 2026
15 checks passed
@paurosello
paurosello deleted the fix/mcp-oauth-rotation-race branch July 21, 2026 14:04
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.

2 participants