From ec079f0df32a3e1721050f66db4bb4e94a3ad25c Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sun, 12 Jul 2026 09:04:35 -0700 Subject: [PATCH] fix(oidc/console): unblock obo edits when OIDC off; latch config-invalid rediscovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-8 review follow-up — two correctness follow-ons from the round-7 rediscovery/console-gate fixes, plus two cleanups. - The console obo write gate ran the OIDC-deployment checks on EVERY update, so once OIDC was operator-disabled any edit of an existing oauth_obo server — including the natural remedy of setting enabled=false — was rejected 400, leaving DELETE as the only way out. The deployment-level checks (encryption key, OIDC enabled/configured, capture opt-in, valid grant profile) now run only when a write is a NEW obo enablement (create or flip INTO obo); a same-type edit keeps only the per-server validity checks (audience required, entra-scope reject), so an operator can always disable or edit an existing obo server. - Probing rediscovery with enabled forced True carried the retryable boot flag into discover_oidc, whose config-error branches returned enabled= False without clearing it, so a config-invalid IdP (an endpoint failing SSRF/same-origin validation) re-probed every 60s forever. The config- error branches now latch discovery_retryable=False (terminal), and maybe_rediscover installs that terminal config so the node stops probing; the transient fetch/degraded branches keep retrying. Cleanups: fold the obo missing-expires_in fallback into _expires_at_from_response via a default_ttl_seconds param (one owner of the stored-expiry format), and drop the redundant audience-change inequality already guaranteed by the no-op normalization (matching the sibling scopes_changing). --- tests/test_mcp_admin_api.py | 33 ++++++++- tests/test_oidc.py | 48 ++++++++++++ turnstone/console/server.py | 143 +++++++++++++++++++++--------------- turnstone/core/mcp_oauth.py | 33 +++++---- turnstone/core/oidc.py | 26 +++++-- 5 files changed, 202 insertions(+), 81 deletions(-) diff --git a/tests/test_mcp_admin_api.py b/tests/test_mcp_admin_api.py index 1a4e8569..a23b6989 100644 --- a/tests/test_mcp_admin_api.py +++ b/tests/test_mcp_admin_api.py @@ -896,7 +896,10 @@ class TestUpdateMcpServer: assert r.status_code == 200, r.text assert r.json()["enabled"] is False - # A genuinely-absent OIDC (neither enabled nor retryable) still rejects. + # Even with OIDC fully operator-disabled (neither flag set), a same-type + # edit of the EXISTING obo server is still allowed — the deployment + # checks only fire on create / flip-into-obo, so an operator is never + # locked out of disabling or editing a server (review finding R8-1). client.app.state.oidc_config = SimpleNamespace( enabled=False, issuer="", @@ -908,8 +911,32 @@ class TestUpdateMcpServer: "/v1/api/admin/mcp-servers/obo-retry-id", json={"enabled": True}, ) - assert r2.status_code == 400 - assert "OIDC" in r2.json()["error"] + assert r2.status_code == 200, r2.text + + def test_create_new_obo_still_rejected_when_oidc_operator_disabled(self, client): + """The deployment gate still fires for a NEW obo enablement: creating a + fresh oauth_obo server (or flipping one into obo) while OIDC is fully + operator-disabled is rejected — only same-type edits of an existing obo + server skip the deployment checks.""" + client.app.state.oidc_config = SimpleNamespace( + enabled=False, + issuer="", + obo_grant_profile="entra", + capture_user_credential=True, + discovery_retryable=False, + ) + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "obo-new-nooidc", + "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 "OIDC" in r.json()["error"] def test_create_obo_rejected_on_invalid_grant_profile(self, client): """Review finding: a typo'd deployment obo_grant_profile leaves the mint diff --git a/tests/test_oidc.py b/tests/test_oidc.py index 6767c56e..c1988123 100644 --- a/tests/test_oidc.py +++ b/tests/test_oidc.py @@ -2560,6 +2560,54 @@ class TestRuntimeRediscovery: # The healed config no longer advertises a retryable failure. assert state.oidc_config.discovery_retryable is False + def test_rediscover_latches_terminal_on_config_error_and_stops_probing(self): + """Review finding: probing with enabled forced True carries the + retryable boot flag into discover_oidc, whose config-error branches must + latch discovery_retryable=False (terminal) — and maybe_rediscover must + INSTALL that terminal config — or a config-invalid IdP (endpoint failing + SSRF/same-origin) re-probes every cooldown window forever. Drives the + real discover_oidc: the discovered token_endpoint is on a foreign host, + so validation rejects it as a config error.""" + from turnstone.core.oidc import maybe_rediscover_oidc + + state = self._disabled_retryable_state() # issuer=https://idp.example.com + bad_doc = { + "authorization_endpoint": "https://idp.example.com/authorize", + "token_endpoint": "https://attacker.example/token", # foreign host + "userinfo_endpoint": "https://idp.example.com/userinfo", + "jwks_uri": "https://idp.example.com/.well-known/jwks.json", + } + mock_response = MagicMock() + mock_response.json.return_value = bad_doc + mock_response.raise_for_status = MagicMock() + + probes = {"n": 0} + + async def _get(url): + probes["n"] += 1 + return mock_response + + async def _run(): + client = _mock_async_client(_get) + with ( + patch("socket.getaddrinfo", return_value=[(2, 1, 6, "", ("93.184.216.34", 0))]), + patch("httpx.AsyncClient", return_value=client), + ): + await maybe_rediscover_oidc(state) + # Same window: cooldown already gates a second probe. + await maybe_rediscover_oidc(state) + # Force the cooldown open — but the config is now terminal, so + # the retryable guard should short-circuit before any probe. + state.oidc_rediscover_last = None + await maybe_rediscover_oidc(state) + + asyncio.run(_run()) + + # Still disabled, but LATCHED terminal (not retryable) — one probe only. + assert state.oidc_config.enabled is False + assert state.oidc_config.discovery_retryable is False + assert probes["n"] == 1 + def test_rediscover_cooldown_gates_repeat_probes(self): from turnstone.core.oidc import maybe_rediscover_oidc diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 5f913303..09dcb286 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -9842,10 +9842,27 @@ def _oauth_columns_to_clear(auth_type: str | None) -> dict[str, None]: def _enforce_oauth_obo_requirements( - request: Request, auth_type: str, *, audience: str | None, scopes: str | None + request: Request, + auth_type: str, + *, + audience: str | None, + scopes: str | None, + check_oidc_deployment: bool = True, ) -> JSONResponse | None: """Write-time validation for ``oauth_obo`` rows (issue #551). + ``check_oidc_deployment`` splits the checks by what they guard. The + DEPLOYMENT-level checks (token-encryption key, OIDC configured/enabled, + capture opt-in, valid grant profile) reject configuring a NEW obo mint that + could never work — they run on create and on a flip INTO obo. They are + SKIPPED for a same-type edit of an existing obo server (``False``): OIDC + being operator-disabled is a deployment state, not a per-server one, so + blocking every edit — including the natural remedy of setting + ``enabled=false`` — would only lock the operator out (the row can still be + DELETEd, but not disabled). The PER-SERVER validity checks (audience + required; ``oauth_scopes`` rejected under the entra profile) always run so a + same-type edit can't leave the row itself invalid. + Rejects at the write choke point what would otherwise fail per-dispatch at runtime (or worse, at the next boot): @@ -9872,61 +9889,60 @@ def _enforce_oauth_obo_requirements( """ if auth_type != "oauth_obo": return None - if getattr(request.app.state, "mcp_token_store", None) is None: - return JSONResponse({"error": _OAUTH_TOKEN_STORE_503_MSG}, status_code=503) - oidc_config = getattr(request.app.state, "oidc_config", None) - # Accept a config that is enabled OR merely transiently un-discovered - # (``discovery_retryable`` — the IdP was unreachable at this process's boot). - # OIDC is still CONFIGURED there (issuer set); rejecting it would make every - # oauth_obo server un-editable/un-disable-able on the console — which never - # runs the runtime rediscovery that heals server nodes — until a manual - # restart. Reject only a genuinely absent / operator-disabled OIDC (neither - # flag set: no issuer configured). - oidc_configured = oidc_config is not None and ( - getattr(oidc_config, "enabled", False) or getattr(oidc_config, "discovery_retryable", False) - ) - if not oidc_configured: - return JSONResponse( - { - "error": ( - "auth_type=oauth_obo requires OIDC sign-in to be configured and " - "enabled (it mints per-server tokens from each user's Turnstone " - "sign-in). Configure [oidc] and capture_user_credential, or use " - "auth_type=oauth_user for per-server consent." - ) - }, - 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 - - if profile not in OBO_GRANT_PROFILES: - return JSONResponse( - { - "error": ( - f"[oidc] obo_grant_profile={profile!r} is not a valid grant profile " - f"({', '.join(sorted(OBO_GRANT_PROFILES))}); fix the deployment " - "config before configuring oauth_obo servers" - ) - }, - status_code=400, + if check_oidc_deployment: + if getattr(request.app.state, "mcp_token_store", None) is None: + return JSONResponse({"error": _OAUTH_TOKEN_STORE_503_MSG}, status_code=503) + oidc_config = getattr(request.app.state, "oidc_config", None) + # Accept a config that is enabled OR merely transiently un-discovered + # (``discovery_retryable`` — the IdP was unreachable at this process's + # boot). OIDC is still CONFIGURED there (issuer set); reject only a + # genuinely absent / operator-disabled OIDC (neither flag set). + oidc_configured = oidc_config is not None and ( + getattr(oidc_config, "enabled", False) + or getattr(oidc_config, "discovery_retryable", False) ) + if not oidc_configured: + return JSONResponse( + { + "error": ( + "auth_type=oauth_obo requires OIDC sign-in to be configured and " + "enabled (it mints per-server tokens from each user's Turnstone " + "sign-in). Configure [oidc] and capture_user_credential, or use " + "auth_type=oauth_user for per-server consent." + ) + }, + 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, + ) + from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES + + if profile not in OBO_GRANT_PROFILES: + return JSONResponse( + { + "error": ( + f"[oidc] obo_grant_profile={profile!r} is not a valid grant profile " + f"({', '.join(sorted(OBO_GRANT_PROFILES))}); fix the deployment " + "config before configuring oauth_obo servers" + ) + }, + status_code=400, + ) if not (audience or "").strip(): return JSONResponse( { @@ -10637,12 +10653,11 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse: # minted obo bearers (and oauth_user tokens) are audience-scoped, so cached # rows for the OLD audience must be purged or a privilege reduction silently # doesn't take effect until they expire. (auth_type flips already purge, so - # only guard the same-type edit here.) + # only guard the same-type edit here.) Like ``scopes_changing`` below, the + # same-type no-op normalization above already dropped an equal re-sent value + # from ``updates``, so "in updates" already means a genuine change. audience_changing = ( - is_user_scoped_auth(old_auth) - and not is_flip - and "oauth_audience" in updates - and (existing.get("oauth_audience") or "") != (updates["oauth_audience"] or "") + is_user_scoped_auth(old_auth) and not is_flip and "oauth_audience" in updates ) # Changing oauth_scopes on an oauth_obo row is the same token-binding # change under the rfc8693 profile: the exchange scope shapes the minted @@ -10686,8 +10701,16 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse: # pre-filled field, so every unrelated save would 400. audience_now = updates.get("oauth_audience", existing.get("oauth_audience")) scopes_being_set = updates.get("oauth_scopes") if "oauth_scopes" in updates else None + # Run the DEPLOYMENT-level OIDC checks only when this write is a NEW obo + # enablement (a flip INTO obo); a same-type edit of an existing obo server + # keeps only the per-server validity checks, so an operator can still + # disable / edit an obo server after OIDC was operator-disabled. err_resp = _enforce_oauth_obo_requirements( - request, auth_type_now, audience=audience_now, scopes=scopes_being_set + request, + auth_type_now, + audience=audience_now, + scopes=scopes_being_set, + check_oidc_deployment=is_flip, ) if err_resp is not None: return err_resp diff --git a/turnstone/core/mcp_oauth.py b/turnstone/core/mcp_oauth.py index 48644509..89b91ab4 100644 --- a/turnstone/core/mcp_oauth.py +++ b/turnstone/core/mcp_oauth.py @@ -2609,15 +2609,14 @@ async def get_obo_access_token_classified( # expiry is never NULL for an obo row (see _OBO_DEFAULT_TTL_SECONDS): a # missing expires_in falls back to a conservative default so the # freshness gate can never serve a short-lived minted token forever. - obo_expires_at = _expires_at_from_response(tokens) or ( - datetime.now(UTC) + timedelta(seconds=_OBO_DEFAULT_TTL_SECONDS) - ).strftime("%Y-%m-%dT%H:%M:%S") await _persist_obo_cache_row( token_store, user_id, server_name, access_token=access_token, - expires_at=obo_expires_at, + expires_at=_expires_at_from_response( + tokens, default_ttl_seconds=_OBO_DEFAULT_TTL_SECONDS + ), scopes=scopes, issuer=issuer, audience=audience, @@ -2763,20 +2762,26 @@ async def _refresh_and_persist( return new_access, rotated_refresh, new_expires_at -def _expires_at_from_response(tokens: dict[str, Any]) -> str | None: +def _expires_at_from_response( + tokens: dict[str, Any], *, default_ttl_seconds: int | None = None +) -> str | None: """Convert an AS ``expires_in`` to an ISO timestamp. Accepts int, float, or string-serialised numerics — some real ASes return ``"3600"`` (string), some return ``3600.0`` (float). Returns - ``None`` when the field is missing, malformed, or non-positive. + ``None`` when the field is missing, malformed, or non-positive — UNLESS + *default_ttl_seconds* is given, in which case that fallback lifetime is + used (the obo mint path passes ``_OBO_DEFAULT_TTL_SECONDS`` so a minted + row is never cached with a NULL, read-as-never-expiring expiry). One owner + of the stored-expiry timestamp format. """ expires_in = tokens.get("expires_in") - seconds: int + seconds: int | None if isinstance(expires_in, bool): # ``bool`` is a subclass of ``int`` — reject explicitly so True # doesn't silently parse as 1 second. - return None - if isinstance(expires_in, int): + seconds = None + elif isinstance(expires_in, int): seconds = expires_in elif isinstance(expires_in, float): seconds = int(expires_in) @@ -2784,11 +2789,13 @@ def _expires_at_from_response(tokens: dict[str, Any]) -> str | None: try: seconds = int(float(expires_in)) except (TypeError, ValueError): - return None + seconds = None else: - return None - if seconds <= 0: - return None + seconds = None + if seconds is None or seconds <= 0: + if default_ttl_seconds is None: + return None + seconds = default_ttl_seconds return (datetime.now(UTC) + timedelta(seconds=seconds)).strftime("%Y-%m-%dT%H:%M:%S") diff --git a/turnstone/core/oidc.py b/turnstone/core/oidc.py index b4249ecf..ea431d84 100644 --- a/turnstone/core/oidc.py +++ b/turnstone/core/oidc.py @@ -449,8 +449,14 @@ async def discover_oidc( setup across calls; when ``None`` a transient client is used (the legacy shape, kept so tests don't need lifecycle management). """ + # Config-error branches force ``discovery_retryable=False`` (terminal): they + # reflect a bad CONFIG (no issuer, an SSRF-rejected URL), not a transient IdP + # outage, so a runtime re-probe would only fail identically forever. This is + # explicit rather than preserved-from-input because ``maybe_rediscover_oidc`` + # probes with the retryable boot config (``discovery_retryable=True``); left + # preserved, a config-invalid IdP would re-probe every cooldown window. if not config.issuer: - return dataclasses.replace(config, enabled=False) + return dataclasses.replace(config, enabled=False, discovery_retryable=False) try: issuer_parsed = _validate_url_no_ssrf( @@ -458,7 +464,7 @@ async def discover_oidc( ) except OIDCError as exc: log.warning("OIDC issuer URL rejected: %s", exc) - return dataclasses.replace(config, enabled=False) + return dataclasses.replace(config, enabled=False, discovery_retryable=False) url = config.issuer.rstrip("/") + "/.well-known/openid-configuration" try: @@ -517,8 +523,10 @@ async def discover_oidc( allow_private=config.allow_private_network, ) except OIDCError as exc: + # Config error (discovered endpoint fails SSRF/validation), not a + # transient outage — latch terminal so rediscovery stops re-probing. log.warning("OIDC discovered %s rejected (url=%s): %s", name, endpoint_url, exc) - return dataclasses.replace(config, enabled=False) + return dataclasses.replace(config, enabled=False, discovery_retryable=False) if userinfo_endpoint: try: @@ -535,7 +543,8 @@ async def discover_oidc( userinfo_endpoint, exc, ) - return dataclasses.replace(config, enabled=False) + # Config error — latch terminal (see the issuer-rejection branch). + return dataclasses.replace(config, enabled=False, discovery_retryable=False) log.info("OIDC discovery complete: %s", config.issuer) return dataclasses.replace( @@ -746,7 +755,14 @@ async def maybe_rediscover_oidc(app_state: Any) -> None: # failure clears it back to False). fresh = await discover_oidc(dataclasses.replace(cfg, enabled=True)) if not fresh.enabled: - return # still failing — next probe after the cooldown lapses + if not fresh.discovery_retryable: + # Discovery now fails for a CONFIG reason (bad issuer, an + # SSRF-rejected endpoint), not a transient outage — latch that + # terminal config onto app_state so this node stops re-probing + # every cooldown window. Without installing it, app_state would + # keep the retryable flag and re-probe an IdP that can never heal. + app_state.oidc_config = fresh + return # still disabled (transient → retry next window; terminal → latched) app_state.oidc_config = dataclasses.replace(fresh, discovery_retryable=False) log.info("OIDC discovery recovered at runtime: %s", fresh.issuer) finally: