diff --git a/tests/test_mcp_admin_api.py b/tests/test_mcp_admin_api.py index fe226e6a..2c064338 100644 --- a/tests/test_mcp_admin_api.py +++ b/tests/test_mcp_admin_api.py @@ -201,7 +201,10 @@ def _enabled_oidc(profile: str = "entra") -> SimpleNamespace: bad-profile config instead to assert the rejection. """ return SimpleNamespace( - enabled=True, issuer="https://idp.example.com", obo_grant_profile=profile + enabled=True, + issuer="https://idp.example.com", + obo_grant_profile=profile, + capture_user_credential=True, ) @@ -764,6 +767,84 @@ class TestUpdateMcpServer: assert data["auth_type"] == "oauth_obo" assert data["oauth_audience"] == "api://mcp-a" + def test_same_type_static_edit_cannot_inject_oauth_columns(self, client, storage): + """Review finding (SECURITY): the OAuth columns must be a pure function + of the target auth_type on EVERY write, not just a flip. A same-type + static edit that injects oauth_authorization_server_url must be scrubbed + to NULL — otherwise a later flip to oauth_user (which legitimately uses + that column) would inherit the attacker AS URL and redirect every + consenting user's OAuth traffic.""" + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "static-inject", + "transport": "streamable-http", + "url": "https://mcp.example.com/sse", + "auth_type": "static", + }, + ) + sid = r.json()["server_id"] + # Same-type static edit trying to smuggle an oauth_user-only column. + r2 = client.put( + f"/v1/api/admin/mcp-servers/{sid}", + json={ + "auth_type": "static", + "oauth_authorization_server_url": "https://attacker.example", + }, + ) + assert r2.status_code == 200, r2.text + row = storage.get_mcp_server(sid) + assert (row.get("oauth_authorization_server_url") or None) is None + + def test_flip_to_oauth_user_does_not_inherit_stale_as_url(self, client, storage): + """Review finding (SECURITY): flipping a non-oauth_user row to oauth_user + must recompute the oauth_user-only columns from the request, never + inherit a stale/injected authorization_server_url left on the pre-flip + row (defence-in-depth for a value that predates the unconditional + scrub).""" + # Plant a static row that already carries a stale AS URL directly in DB. + storage.create_mcp_server( + server_id="stale-asurl-id", + name="stale-asurl", + transport="streamable-http", + url="https://mcp.example.com/sse", + auth_type="static", + oauth_authorization_server_url="https://attacker.example", + ) + # Flip to oauth_user WITHOUT supplying an AS URL in the body. + r = client.put( + "/v1/api/admin/mcp-servers/stale-asurl-id", + json={"auth_type": "oauth_user", "oauth_client_id": "cli_x"}, + ) + assert r.status_code == 200, r.text + row = storage.get_mcp_server("stale-asurl-id") + assert (row.get("oauth_authorization_server_url") or None) is None + assert row.get("oauth_client_id") == "cli_x" + + def test_create_obo_rejected_when_capture_disabled(self, client): + """Review finding: oauth_obo mints from the user's CAPTURED sign-in + credential, so with capture_user_credential off, login persists nothing + and every dispatch returns kind='missing' with an unsatisfiable remedy. + Reject at write time.""" + client.app.state.oidc_config = SimpleNamespace( + enabled=True, + issuer="https://idp.example.com", + obo_grant_profile="entra", + capture_user_credential=False, + ) + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "obo-no-capture", + "transport": "streamable-http", + "url": "https://mcp.example.com/sse", + "auth_type": "oauth_obo", + "oauth_audience": "api://mcp-a", + }, + ) + assert r.status_code == 400, r.text + assert "capture_user_credential" in r.json()["error"] + def test_create_obo_rejected_when_oidc_disabled(self, client): """Review finding: oauth_obo mints from the user's OIDC sign-in, so an install with OIDC disabled can NEVER mint. Reject at write time (a diff --git a/tests/test_mcp_obo_mint.py b/tests/test_mcp_obo_mint.py index 365afc38..2b40ec5c 100644 --- a/tests/test_mcp_obo_mint.py +++ b/tests/test_mcp_obo_mint.py @@ -762,7 +762,9 @@ class TestFailureHandling: common missing-tenant-grant case — the user never had a token for this server) must NOT emit a token_revoked audit for a row that never existed. The cooldown is still armed as the terminal backstop (the - credential survives), so the second dispatch short-circuits.""" + credential survives), and — because the failure was PERMANENT — the + second dispatch surfaces the honest permanent classification during the + cooldown window (not a misleading retryable transient).""" _seed_obo_server(storage) client = MagicMock(spec=httpx.AsyncClient) client.post = AsyncMock( @@ -783,8 +785,9 @@ class TestFailureHandling: first, second = asyncio.run(_run()) assert first.kind == "refresh_failed" - # Terminal: the cooldown short-circuits the second dispatch entirely. - assert second.kind == "refresh_failed_transient" + # Terminal: the cooldown short-circuits the second dispatch — and reports + # the PERMANENT classification, not a retryable transient. + assert second.kind == "refresh_failed" assert client.post.call_count == 1 # NOT re-minted # No row was ever deleted → no bogus revoke audit. events = storage.list_audit_events(action="mcp_server.oauth.token_revoked") @@ -867,35 +870,100 @@ class TestFailureHandling: assert result.token == "at-reminted" assert client.post.call_count == 1 - def test_force_refresh_reuses_row_minted_while_waiting_for_lock( + def test_force_refresh_reuses_concurrently_minted_token_without_reminting( self, storage: SQLiteBackend ) -> None: - """Review finding: the under-lock "another caller already minted" reuse - gate keyed on ``last_refreshed``, which obo cache rows NEVER set - (delete+create hardcodes NULL) — so every serialized force_refresh - waiter re-ran a full IdP redemption. The gate now keys on ``created`` - (the mint time under delete+create): a fresh row created at/after the - caller's lock-request time is served without a new mint.""" + """Serialized force_refresh waiters must single-flight the re-mint: a + waiter that acquires the lock AFTER a peer already re-minted reuses the + peer's fresh token instead of running its own redundant IdP redemption. + The reuse is decided by token IDENTITY (the under-lock row holds a + DIFFERENT token than the rejected one this caller came in with), not by + mint time — so a same-second concurrent mint is still reused.""" + from unittest.mock import patch + _seed_obo_server(storage) client = MagicMock(spec=httpx.AsyncClient) client.post = AsyncMock() # any IdP call would be a gate failure state = _make_app_state(storage, http_client=client, oidc_config=_make_oidc_config()) _seed_credential(state) - # Seeded moments before the call: at second granularity its ``created`` - # is >= the caller's lock-request time, exactly what a concurrent - # waiter observes after the winner persisted its mint. - _seed_cache_row(state, expires_in_seconds=3600, access_token="winner-minted-at") - async def _run() -> Any: - return await get_obo_access_token_classified( - app_state=state, user_id=USER, server_name=SERVER, force_refresh=True - ) + def _fresh_row(access_token: str) -> Any: + return { + "user_id": USER, + "server_name": SERVER, + "access_token": access_token, + "refresh_token": None, + "expires_at": (datetime.now(UTC) + timedelta(seconds=3600)).strftime(_ISO), + "scopes": None, + "as_issuer": ISSUER, + "audience": AUDIENCE, + "created": datetime.now(UTC).strftime(_ISO), + "last_refreshed": None, + } - result = asyncio.run(_run()) + # Pre-lock read returns the rejected token; the under-lock re-read returns + # a DIFFERENT token (a concurrent waiter re-minted while we held-waited). + reads = [_fresh_row("rejected-at"), _fresh_row("peer-reminted-at")] + with patch.object(state.mcp_token_store, "get_user_token", side_effect=reads): + + async def _run() -> Any: + return await get_obo_access_token_classified( + app_state=state, user_id=USER, server_name=SERVER, force_refresh=True + ) + + result = asyncio.run(_run()) assert result.kind == "token" - assert result.token == "winner-minted-at" - assert client.post.call_count == 0 + assert result.token == "peer-reminted-at" # reused the peer's fresh token + assert client.post.call_count == 0 # no redundant redemption + + def test_force_refresh_remints_when_cache_still_holds_rejected_token( + self, storage: SQLiteBackend + ) -> None: + """The other half of the identity gate: when the under-lock row still + holds the SAME token the caller came in with (no peer re-minted), a + force_refresh must RE-MINT — never re-serve the just-rejected bearer. + A mint-time gate at 1-second ``created`` granularity would wrongly + re-serve a token minted in the same second as the retry.""" + from unittest.mock import patch + + _seed_obo_server(storage) + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock( + return_value=_mk_response( + 200, {"access_token": "genuinely-reminted", "expires_in": 3600} + ) + ) + state = _make_app_state(storage, http_client=client, oidc_config=_make_oidc_config()) + _seed_credential(state) + + rejected = { + "user_id": USER, + "server_name": SERVER, + "access_token": "rejected-at", + "refresh_token": None, + "expires_at": (datetime.now(UTC) + timedelta(seconds=3600)).strftime(_ISO), + "scopes": None, + "as_issuer": ISSUER, + "audience": AUDIENCE, + "created": datetime.now(UTC).strftime(_ISO), + "last_refreshed": None, + } + # Both the pre-lock and under-lock reads return the SAME (rejected) token. + with patch.object( + state.mcp_token_store, "get_user_token", side_effect=[rejected, rejected] + ): + + async def _run() -> Any: + return await get_obo_access_token_classified( + app_state=state, user_id=USER, server_name=SERVER, force_refresh=True + ) + + result = asyncio.run(_run()) + + assert result.kind == "token" + assert result.token == "genuinely-reminted" # re-minted, NOT re-served + assert client.post.call_count == 1 def test_rotation_persist_failure_does_not_break_the_mint(self, storage: SQLiteBackend) -> None: """Review finding: a storage error inside the rotation-persist callback diff --git a/turnstone/console/server.py b/turnstone/console/server.py index ca5b5f49..171ae42c 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -9889,6 +9889,22 @@ def _enforce_oauth_obo_requirements( }, status_code=400, ) + if not getattr(oidc_config, "capture_user_credential", False): + # The mint redeems the user's CAPTURED IdP refresh token; with capture + # off, login persists nothing, so every dispatch returns kind="missing" + # and the "sign in again" remedy can never succeed — a permanent + # misconfig this choke point exists to reject. (Enabling capture also + # requires the encryption key, checked at boot.) + return JSONResponse( + { + "error": ( + "auth_type=oauth_obo requires [oidc] capture_user_credential=true " + "so each user's sign-in credential is captured for minting; it is " + "currently disabled, so this server could never obtain a token." + ) + }, + status_code=400, + ) profile = _obo_profile(request) from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES @@ -10551,22 +10567,36 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse: ): del updates[_noop_key] - # Clear the OAuth columns the target auth_type does NOT use (shared - # ``_oauth_columns_to_clear`` policy). ``oauth_client_secret_ct`` is owned by - # a dedicated write path (see below). - if is_flip: - target_auth = new_auth # non-None: is_flip requires new_auth is not None - if is_user_scoped_auth(target_auth): - # FLIP into a user-scoped type: recompute the semantic columns from - # the body — present → that value, absent → NULL — so neither the - # old audience nor the old scopes can leak across the semantic - # boundary. The top-of-handler loop already copied any body value - # into ``updates``; forcing the key present here NULLs it when the - # body omitted it (e.g. an API PUT of just {"auth_type": ...}, or the - # console form clearing the field on the auth-type switch). - updates["oauth_audience"] = updates.get("oauth_audience") - updates["oauth_scopes"] = updates.get("oauth_scopes") - updates.update(_oauth_columns_to_clear(target_auth)) + # Scrub the OAuth columns the target auth_type does NOT use — on EVERY write, + # not just a flip. This is a SECURITY invariant, not a flip nicety: a + # same-type edit can still inject a column the type doesn't use (e.g. an API + # PUT of {"auth_type":"static","oauth_authorization_server_url":"…attacker…"} + # onto a static row), which — because oauth_user legitimately USES that + # column — would then survive a later flip to oauth_user and redirect every + # consenting user's OAuth traffic. The persisted OAuth columns must be a pure + # function of the target auth_type (the create handler enforces the same via + # ``_oauth_columns_to_clear``). ``oauth_client_secret_ct`` is owned by a + # dedicated write path (see below). + target_auth = new_auth if new_auth is not None else old_auth + if is_flip and new_auth is not None and is_user_scoped_auth(new_auth): + # FLIP into a user-scoped type: the columns whose MEANING differs across + # oauth_user↔oauth_obo must be recomputed from the body — present → that + # value, absent → NULL — never inherited from the old model's row. For + # oauth_audience/oauth_scopes this is the semantic-boundary rule; for a + # flip INTO oauth_user the oauth_user-only columns (client_id / + # registration / AS-URL) are also recomputed, so a stale/injected value + # left on the pre-flip static/none/obo row cannot carry in (those models + # don't use these columns, so the operator supplies them fresh here). + updates["oauth_audience"] = updates.get("oauth_audience") + updates["oauth_scopes"] = updates.get("oauth_scopes") + if new_auth == "oauth_user": + for _user_col in ( + "oauth_client_id", + "oauth_registration_mode", + "oauth_authorization_server_url", + ): + updates[_user_col] = updates.get(_user_col) + updates.update(_oauth_columns_to_clear(target_auth)) # Renaming a pool-backed row needs the same per-user-token purge as # delete: the tokens are keyed on the OLD ``server_name`` and a row # later created with that old name (with attacker-controlled URL) diff --git a/turnstone/core/mcp_client.py b/turnstone/core/mcp_client.py index 2ecef140..4598250a 100644 --- a/turnstone/core/mcp_client.py +++ b/turnstone/core/mcp_client.py @@ -7279,7 +7279,14 @@ def _pool_error_detail(server_row: dict[str, Any], situation: str, *, kind: str card; everything else is treated as ``oauth_user``. """ model = "oauth_obo" if server_row.get("auth_type") == "oauth_obo" else "oauth_user" - return _POOL_ERROR_DETAIL[(model, situation)].format(kind=kind, kind_cap=kind.capitalize()) + template = _POOL_ERROR_DETAIL[(model, situation)] + # Only the insufficient_scope rows carry {kind}/{kind_cap}; the rest are + # plain strings. Format ONLY when a kind is supplied so a future message + # containing a literal brace (a scope/JSON example) can't raise inside this + # error-rendering path and turn a clean structured error into a 500. + if kind: + return template.format(kind=kind, kind_cap=kind.capitalize()) + return template def _consent_missing_detail(server_row: dict[str, Any]) -> str: diff --git a/turnstone/core/mcp_oauth.py b/turnstone/core/mcp_oauth.py index 920eef9b..54247f66 100644 --- a/turnstone/core/mcp_oauth.py +++ b/turnstone/core/mcp_oauth.py @@ -1419,6 +1419,13 @@ class _RefreshBackoffState: last_failure_monotonic: float = 0.0 ambiguous_streak: int = 0 + # True when the failure that armed the current cooldown was a PERMANENT + # dead-grant (only the oauth_obo path arms a cooldown on permanent, because + # its shared credential survives the per-server revoke). The in-cooldown + # short-circuit reads this so it surfaces the honest permanent classification + # (re-login / admin remedy) instead of a misleading "retry" transient for the + # whole window. Reset to False whenever a transient/ambiguous failure arms. + last_failure_permanent: bool = False def _refresh_backoff_state(app_state: Any, user_id: str, server_name: str) -> _RefreshBackoffState: @@ -1435,7 +1442,9 @@ def _refresh_backoff_state(app_state: Any, user_id: str, server_name: str) -> _R return state -def _arm_cooldown(app_state: Any, user_id: str, server_name: str) -> _RefreshBackoffState: +def _arm_cooldown( + app_state: Any, user_id: str, server_name: str, *, permanent: bool = False +) -> _RefreshBackoffState: """Stamp the per-(user, server) transient-failure cooldown clock to now. Single definition of the "back off this pair" operation (previously written @@ -1444,9 +1453,16 @@ def _arm_cooldown(app_state: Any, user_id: str, server_name: str) -> _RefreshBac missed site can't silently keep hammering the AS/IdP on that path. Returns the backoff state so a caller that also mutates the ambiguous streak reuses the same object instead of re-fetching it. + + ``permanent`` records whether the failure that armed the cooldown was a + dead-grant (obo only): the in-cooldown short-circuit reads it to surface the + honest permanent vs. transient classification. A transient/ambiguous arm + resets it to False so a later transient window can't inherit a stale + permanent flag. """ state = _refresh_backoff_state(app_state, user_id, server_name) state.last_failure_monotonic = time.monotonic() + state.last_failure_permanent = permanent return state @@ -1701,8 +1717,10 @@ async def _handle_refresh_failure( ) if arm_cooldown_on_permanent: # _revoke_after_refresh_failure cleared the backoff; re-arm the - # cooldown as the terminal backstop for the surviving credential. - _arm_cooldown(app_state, user_id, server_name) + # cooldown as the terminal backstop for the surviving credential, and + # mark it permanent so the in-cooldown short-circuit surfaces the + # honest dead-grant classification (not a misleading "retry"). + _arm_cooldown(app_state, user_id, server_name, permanent=True) return result # Transient or ambiguous: keep the token (a blip must never revoke a # user's consent) and arm the cooldown so a down AS isn't hit on @@ -1724,7 +1742,7 @@ async def _handle_refresh_failure( streak=backoff.ambiguous_streak, error=str(exc), ) - return await _revoke_after_refresh_failure( + escalation_result = await _revoke_after_refresh_failure( app_state, token_store, user_id, @@ -1732,6 +1750,16 @@ async def _handle_refresh_failure( server_id_for_audit, reason=escalation_reason, ) + if arm_cooldown_on_permanent: + # Same shared-credential backstop as the PERMANENT branch: an + # escalation is a treated-as-dead grant, but the revoke + # cleared the cooldown, and for obo the credential survives — + # so without re-arming, the very next dispatch immediately + # re-mints against the still-failing IdP and re-escalates + # each cycle. Re-arm (marked permanent for the honest + # in-cooldown classification). + _arm_cooldown(app_state, user_id, server_name, permanent=True) + return escalation_result # Background priming: an UNCLASSIFIABLE sustained rejection is # exactly where a bulk prime of servers the user may not be # using must not revoke consent. Defer the escalation-revoke to @@ -2319,8 +2347,6 @@ async def get_obo_access_token_classified( log.debug("mcp_server.oauth.token_store_unconfigured") return TokenLookupResult(kind="missing") - t_lock_request_started = datetime.now(UTC).replace(microsecond=0) - # Resolve the server row + audience FIRST (callers on the dispatch/priming # path pass server_row, so this is normally no SQL) — the fast-path cache # serve must validate the row's audience against the CURRENT one, so it can't @@ -2363,6 +2389,14 @@ async def get_obo_access_token_classified( # still-fresh cache must fall through to the locked re-read so it can pick up # a token a cluster-mate just minted, rather than fail transient in-cooldown. if not fresh and _refresh_in_cooldown(app_state, user_id, server_name): + # Surface the classification that armed the cooldown: obo arms it on a + # PERMANENT dead-grant too (its credential survives the per-server + # revoke), and reporting that as a retryable "transient" for the whole + # window would tell the user to retry a permanently-broken server and + # flap against the honest re-login/admin affordance the mint returned. + backoff = _refresh_backoff_state(app_state, user_id, server_name) + if backoff.last_failure_permanent: + return TokenLookupResult(kind="refresh_failed") return TokenLookupResult(kind="refresh_failed_transient") if oidc_config is not None and not getattr(oidc_config, "enabled", False): # A node that booted during a transient IdP outage carries @@ -2435,13 +2469,18 @@ async def get_obo_access_token_classified( if _is_fresh_obo_cache_row(plain2, audience, scopes) and plain2 is not None: if not force_refresh: return _token_result(app_state, user_id, server_name, plain2["access_token"]) - # Obo cache rows are delete+create'd per mint and never touched by - # update_user_token_after_refresh, so ``created`` IS the mint time - # (``last_refreshed`` stays NULL on this path — the oauth_user - # gate's column would never fire here, and every serialized - # force_refresh waiter would run its own redundant IdP redemption). - minted_at = _parse_iso_to_utc(plain2.get("created") or "") - if minted_at is not None and minted_at >= t_lock_request_started: + # force_refresh means the caller's bearer was rejected; serialized + # waiters must single-flight the re-mint (avoid N redundant IdP + # redemptions) WITHOUT re-serving the very token that was just + # rejected. Distinguish by token IDENTITY, not mint time: the pre-lock + # ``plain`` is the token this caller came in with (the rejected one); + # if the under-lock row now holds a DIFFERENT token, a concurrent + # waiter re-minted while we waited — reuse it. If it is the SAME + # token, nothing has changed, so fall through and re-mint. (Mint time + # can't distinguish these at the cache row's 1-second ``created`` + # granularity — a same-second own-mint would read as "fresh".) + pre_lock_token = plain["access_token"] if plain is not None else None + if plain2["access_token"] != pre_lock_token: return _token_result(app_state, user_id, server_name, plain2["access_token"]) # Re-read the credential under the lock — a concurrent mint for a @@ -3577,6 +3616,10 @@ async def _handle_mcp_oauth_list_connections_inner(request: Request) -> Response # "Disconnect" that silently undid itself — the row deletes, then # session-start priming re-mints from the surviving captured credential — # so the connections list shows only rows the user can actually revoke. + # Classify by the authoritative server ``auth_type`` (one read on this cold + # settings-page path) rather than inferring obo from a NULL refresh token: + # the auth_type is the source of truth, and a token-shape heuristic would + # silently hide any oauth_user row that ever lacked a refresh token. # Fail open on a server-list read error: worst case an obo row renders # and the revoke endpoint below still refuses it honestly. storage = _get_storage(request.app.state)