mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
main
26 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 metaclassing
|
9adde920d4 |
feat(models): per-alias backend auth via Entra OBO and app identity (#898)
Adds a per-alias `auth_mode` on model definitions so a model backend can authenticate to an Entra-fronted gateway with a per-request minted token instead of one shared static API key, letting the gateway attribute calls to the actual user or to the app as a machine identity. - `static` (default, unchanged) sends the stored `api_key`. - `entra_obo` mints a per-user On-Behalf-Of token for `obo_audience` from the caller's captured refresh credential. - `entra_app` mints an app-identity token via the client-credentials grant, and covers userless turns that OBO cannot. Reuses the existing OBO grant legs, refresh-token rotation CAS, cluster advisory lock and the `mcp_user_tokens` mint-cache, keyed under synthetic `__model_obo__:<audience>` / `__model_app__:<audience>` rows. The token binds at the call site through `client.with_options(api_key=...)` so each SDK emits it on its own auth path rather than through header injection. Migration 068 adds `auth_mode` and `obo_audience`. Both are additive and existing rows default to `static`, so behaviour is unchanged unless an alias opts in. Operator controls: `model.auth_audience_allowlist` is an exact-match allow-list that gates which audiences may be configured and denies all by default, and changing a mode or audience requires `admin.mcp`. `model.auth_fail_closed` decides whether a failed mint may fall back to an explicitly configured static key. A delegated call with no user, or a dynamic alias with no real static key, always refuses. Two changes here apply regardless of whether any alias opts in: - Storage and app state are now wired into the console MCP client manager. This fixes per-user `oauth_user` / `oauth_obo` dispatch for coordinator-hosted sessions, which previously raised `RuntimeError` on first call because `set_app_state` was only ever called on the node. - Unattended watch restores and `--resume` resolve the persisted workstream owner instead of constructing the session under an empty principal. A workstream with no owner is now a permanent refusal rather than an anonymous, auto-approved run. |
||
|
Patrick Buckley
|
984a10307e |
feat(coordinator): MCP tool surface for coordinator sessions (#725)
Coordinator-kind workstreams get the same MCP surface as interactive sessions — tools, resources, and prompts (read_resource/use_prompt go dual-kind) — gated per-persona exactly like interactive, with no separate feature flag. The console hosts its manager with node parity end to end: boot calls create_mcp_client inline (same catalog resolution: DB rows, then mcp.config_path, then this host's config.toml), the admin reload fan-out lazily constructs and reconciles it under a lock (the node's unlocked equivalent is #873), per-server refresh/reconnect and the admin MCP status view cover it under the collector's console pseudo-node id, and shutdown follows LIFO teardown. Sessions read the live manager through a per-construction getter — the console counterpart of the node factory's mcp_ref[0] read; client presence is the session-level contract, and the kind-aware tool assembly runs the same listener/prime/rebind skeleton as interactive. bind_acting_user re-scopes listeners and per-user pools, which is security-critical for multi-sender coordinators. The wire-safety status projections move verbatim to core/mcp_utils so both hosts present one schema (node endpoint bodies byte-identical); the console's per-server action classification is a pinned COPY of the node endpoints', with a parity test driving both sides across the outcome matrix that fails if either drifts. The shared MCP error card (consent / re-consent / forbidden / operator) moves to mcp_error.js + mcp_error.css, linked by all three card hosts and pinned by className→rule and host→link parity tests; the module joins the whole-file sink-scan and var-ratchet lists. Reload reporting is honest about the console entry: excluded from the unreached-node warning's list and denominator, and the toast claims "+ console" only for a real reconcile, with an explicit note on failure. The pending-consent badge (#874's console half) ships too: the console defines the same onConsentDetected seam the node dashboard exposes — lighting up the shared pane host's existing bridge for hosted interactive panes — and the coordinator pane threads its card's detections through the single MCP-error helper. The badge rides the Admin > MCP Servers rail row, hydrates at boot from the Phase 9 pending-consent endpoint the console already serves, re-syncs to DB truth when the operator views the MCP panel, and the rail-less standalone page carries a status-bar chip instead. A coordinator that hits a consent wall unattended now has a persistent, glanceable signal. Pre-existing bugs fixed along the way: create_mcp_client returned None on pool-only installs, leaving any host managerless after restart until the next admin MCP write; admin_import_mcp_config never scheduled the reload fan-out (stale catalogs after import); the admin settings UI rendered the coordinator settings section unordered and unlabeled. Follow-ups: #873 (node reload double-construct race); #874 narrows to the admin-MCP-view per-server indicator. |
||
|
Patrick Buckley
|
86aeb43120 |
fix(mcp): review round 6 — close the refresh-outcome reporting residuals
Three residual gaps in the round-5 skip-outcome threading, all in _refresh_all's other reconnect branches plus the endpoint ordering: - The disconnected-server reconnect DEFERRAL (_ensure_static_connected returns None: a sibling call in flight on the old stack, lock not held) returned None without stamping 'skipped', so the endpoint and pill read the STALE prior 'ok' and reported a never-run refresh as current. Now stamps 'skipped' like every other skip branch. - A server removed from config between the top-of-loop session check and the cfg lookup fell through to with results[name] UNSET, omitting it from the returned dict — an operator refreshing that one server saw a bare 'refresh complete' with no line. Now reports None so it renders. - internal_mcp_refresh_one checked 'skipped' BEFORE the error pill, so a skip on a server carrying a live error returned a benign 202 instead of 500 — a status-code-keyed caller would treat an erroring server as healthy-but-busy. Error is now checked first. - _reap_bounded swallowed an external CancelledError (shutdown / an operator cancel of the refresh runner) — it now re-raises after a best-effort exception retrieval, honouring the cancel. Dropped the unneeded asyncio.shield in the process. Tests: deferral stamps skipped, removed-mid-pass reported not omitted, endpoint error-beats-skip → 500, reap re-raises external cancel. Suite 9403 green. Refs #839 |
||
|
Patrick Buckley
|
748f670fe8 |
fix(mcp): review round 5 — thread the refresh outcome to every operator surface
The 'skipped'/None refresh sentinel added in round 4 was only half
threaded: consumers still misreported it. Unify all operator surfaces
on ONE source of truth — the per-server last_refresh_outcome ('ok' /
'skipped' / 'error:<Class>') — exposed via a new last_refresh_outcome()
accessor:
- _refresh_all returns None (not ([], [])) for a FAILURE too, so a
failed refresh is never rendered as 'no changes' (the pre-#839 lie
the sentinel exists to close); None is disambiguated skipped-vs-failed
by the outcome. ([], []) now strictly means 'ran, no changes'.
- /mcp refresh renders skip ('skipped — retry scheduled') and failure
('refresh failed (error:X)') distinctly from 'no changes'.
- The node-internal refresh endpoint returns 202 'skipped' instead of a
misleading 200 'ok' for a refresh that never ran (the busy-lock skip);
it reads the outcome from the manager accessor because the public
status projection deliberately whitelists last_refresh_outcome out.
- admin.js paints 'skipped' with a neutral info pill
(.mcp-refresh-pill-skip), not the error-red any-non-'ok' used to get.
- _admit_list_changed rolls back BOTH the coalesce marker and the
debounce stamp when scheduling raises, so a same-kind push in the
window afterward isn't debounced against a refresh that never spawned
(the pool path has no on_debounce_drop recovery).
Tests: endpoint 202-skip, CLI skip/failure render, _refresh_all
failure→None + outcome, spawn-failure stamp+marker rollback. Suite
9399 green.
NOTE filed #843: the admin refresh pill's data (last_refresh_at/outcome)
is stripped by BOTH status projections and never reaches admin.js — a
pre-existing latent bug (the pill has never rendered); the admin.js
color fix here is correct-when-reachable. Out of #839 scope (the read
projection strips it for a privacy reason that needs its own coarsening
decision).
Refs #839
|
||
|
Patrick Buckley
|
b2b8b6f65e |
fix(mcp): schedule node reload after admin write instead of blocking on it
The auto-notify added in the prior commit awaited _notify_nodes_mcp_reload
inline in create/update/delete, coupling each admin write's latency — and
success — to cluster reachability: on a large cluster with slow/unreachable
nodes the write could hang up to ceil(nodes/fan_out_limit)*30s behind the
fan-out, and a post-commit fan-out error would 500 a write that already landed.
Schedule the fan-out as a BackgroundTask that runs AFTER the 200 instead — the
"trigger, not drain" contract already used by _cascade_cancel_to_children — so
the write's response is never blocked on, nor failed by, the fan-out. The
pre-existing registry-install path is converted the same way for consistency.
There is no periodic node->DB reconcile, so a node that misses the reload serves
a stale MCP catalog until the next POST /reload. The background _run therefore
logs any unreached node (or a systemic fan-out fault) at WARNING — visible at
the default INFO level — rather than swallowing it; the per-node status view
also surfaces the divergence. A non-2xx reply from a node's reload/action
endpoint now counts as a failure (raise_for_status) rather than a reached node,
so neither the WARNING nor the operator /reload results miss a 5xx node.
Revert the getattr None-guard on _notify_nodes_mcp_reload: it turned the
operator-triggered POST /reload into a silent success ({} with 200) when the
fan-out infra was absent — a fail-loudly violation — and diverged from the
unguarded sibling _notify_nodes_mcp_action. The helper is drain-style again,
awaited only by /reload (which must surface fan-out failures); writes go through
the best-effort scheduler.
Tests: assert the reload is NOT scheduled on a delete/update 404 or a create
secret-store 503; that an unreached-node, raising, or non-2xx fan-out is logged
at WARNING / recorded as an error; and that operator POST /reload fails loudly
(500) without fan-out infra.
|
||
|
Patrick Buckley
|
b3cd91f1a0 |
fix(mcp): auto-notify nodes on admin create/update/delete
admin_create/update/delete_mcp_server wrote to the DB but never told nodes to reconcile — only the registry-install path and the explicit /reload did — so a programmatic create/edit/delete was inert on nodes until a manual reload (and the mid-session re-prime self-heal never fired). Call _notify_nodes_mcp_reload after each write, mirroring registry-install; also make that helper best-effort (skip when the cluster fan-out infra is absent) so a write can't 500 on it. |
||
|
Patrick Buckley
|
ec079f0df3 |
fix(oidc/console): unblock obo edits when OIDC off; latch config-invalid rediscovery
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). |
||
|
Patrick Buckley
|
af56170be6 |
fix(oidc/mcp): make runtime OIDC rediscovery actually work; preserve oauth_user paths
Round-7 review follow-up. - The runtime OIDC re-discovery feature was dead code: discover_oidc PRESERVES the input config's `enabled` flag on success (only load_oidc_config ever sets it True), and maybe_rediscover_oidc always probed from the disabled boot config, so a successful rediscovery still returned enabled=False and the config swap was unreachable — the whole boot-outage auto-heal never worked. It now probes with enabled forced on so the flag is a reliable success signal. The unit test that "covered" this was mocking discover_oidc to return enabled=True, masking the bug; it now drives the real discover_oidc through a mocked HTTP discovery GET. - The console never runs runtime rediscovery, so a transient discovery failure at console boot made every oauth_obo server un-editable and un-disable-able. The write gate now accepts a discovery_retryable config (OIDC configured, discovery transiently down) and rejects only a genuinely absent OIDC. - The first rediscovery probe was suppressed for ~60s after host boot because the "last probe" timestamp defaulted to 0.0; it now uses a None sentinel for "never probed". - Two behavior-preservation fixes for the pre-existing oauth_user path: the shared hardened token-POST no longer escalates oauth_user oversized error bodies (that status-based classification is opt-in for the obo legs only), and the token_revoked audit fires unconditionally for oauth_user again (a refresh failure means a real grant died) while staying delete-gated for obo to avoid revocation rows for tokens that never existed. Cleanups: drop a throwaway set allocation in the pool-emptiness check, compute the create handler's cleaned OAuth text once, remove a dead no-op pop with a false comment, and simplify the cleared-map prune to two non-overlapping passes. |
||
|
Patrick Buckley
|
09aa50b7a1 |
fix(mcp): close obo auth-column leak, capture gate, and cooldown classification
Round-5 review follow-up — three CONFIRMED (one security) plus two correctness issues, all traceable to earlier fixes in this branch. SECURITY: the round-2 redesign gated the "scrub OAuth columns this auth_type doesn't use" on is_flip, replacing the old unconditional scrub. A same-type static/none/obo edit could then inject an oauth_authorization_server_url that survived a later flip to oauth_user (which uses that column) and redirected every consenting user's OAuth traffic to an attacker AS. The scrub is now applied on EVERY write, and a flip into oauth_user recomputes the oauth_user-only columns from the request so a stale value can't carry in — the persisted OAuth columns are once again a pure function of the target auth_type. - The oauth_obo write gate now also requires capture_user_credential to be enabled: without it, login persists no credential and every dispatch returns "missing" with a remedy that can never succeed — the permanent misconfig the gate exists to reject. - A permanent obo mint failure arms the cooldown (its shared credential survives the per-server revoke), but the in-cooldown short-circuit reported it as a retryable transient for the whole window, flapping against the honest re-login/admin affordance. The backoff state now records whether the arming failure was permanent, and the short-circuit surfaces the matching classification. - The ambiguous-escalation revoke cleared the cooldown without re-arming; for obo (surviving credential) that let the next dispatch immediately re-mint against the still-failing IdP. It now re-arms the same terminal backstop the permanent branch has. - The force-refresh reuse gate keyed on the cache row's 1-second `created` time, which couldn't tell a concurrent peer's fresh mint from the caller's own just-rejected token minted in the same second — so a retry could re-serve the rejected bearer. It now decides by token identity (the under-lock row differs from the pre-lock one), preserving the single-flight reuse while never re-serving a rejected token. Also: guard _pool_error_detail's str.format so placeholder-free copy can't raise inside the error renderer, and note why the connections-list classifies obo rows by authoritative auth_type on that cold path. |
||
|
Patrick Buckley
|
d2e69ca527 |
fix(mcp): coherent obo auth-type carry-over + honest error affordances
Round-2 review follow-up. The headline is a redesign of the OAuth column carry-over so scopes/audience can no longer leak or vanish across an auth-type flip: - oauth_audience and oauth_scopes keep their meaning only WITHIN an auth type (a resource indicator vs. an IdP app id; AS-consent scopes vs. an rfc8693 exchange scope). On any oauth_user<->oauth_obo flip they are now recomputed from the request (present -> value, absent -> NULL) and never carried from the old row. A shared _oauth_columns_to_clear policy drives both the create and update handlers. No-op normalization of a re-sent equal value applies only to same-type edits. - The console form clears both semantic fields when the auth type changes and always submits the visible values; the previous "omit unchanged scopes" logic collided with the backend's flip handling and could silently drop or carry scopes. Write-time validation now rejects oauth_obo rows that can never mint — OIDC disabled/unconfigured, or an invalid obo_grant_profile — instead of letting them surface per-dispatch as a retryable transient that never heals. Honest failure affordances for sign-in passthrough (no per-server consent flow exists): - the token_revoked audit fires only when a row was actually deleted, so a permanent mint rejection against a surviving credential no longer appends a bogus revocation on every post-cooldown dispatch/prime; - the 403 insufficient-scope detail and the chat error card's action button are now auth-model-aware — obo errors point at the administrator rather than a dead-end re-consent, and the Connect button renders only when a real consent URL is present; - the read-side freshness gate now enforces scopes as well as audience, so an rfc8693 scope narrowing takes effect on the next dispatch even if the best-effort admin cache purge failed. Cleanups: the five decrypt-failure result constructions collapse into _decrypt_failure_result; the cleared-pairs TTL bookkeeping into _mark_pending_consent_cleared; drop the dead USER_SCOPED_AUTH_TYPES re-export from mcp_oauth; correct the now-bidirectional oidc<->mcp_oauth lazy-import note. Docs updated for the flip semantics and the OIDC prerequisite. |
||
|
Patrick Buckley
|
32c76499fa |
fix(mcp): harden obo mint path and admin lifecycle after review
Mint engine: guard the credential-rotation persist so a storage blip cannot escape the classified-result contract mid-mint (and cannot brick the user's other obo servers on strict-rotation IdPs); stop borrowing the login flow's httpx client across event loops — mints use a transient per-request client (obo_http_client remains as a test seam); retry OIDC discovery at runtime (cooldown-gated, single-flight) so a node that booted during an IdP outage can mint again without a restart; key the under-lock force-refresh reuse gate on created, which delete+create makes the mint time (obo rows never set last_refreshed, so the copied oauth_user gate never fired and serialized waiters each re-redeemed). Cross-node consent badges: the cleared-pairs set becomes a TTL map with bounded growth, so a badge written by another node after this node's last clear self-heals within one TTL window instead of surviving until a restart. Admin lifecycle: purge the mint cache when oauth_scopes changes on an obo row (an rfc8693 privilege reduction now applies immediately, like audience changes); normalize no-op scope/audience re-sends out of updates — the admin form re-submits pre-filled fields on every save, which both re-triggered purges and made entra-profile rows with legacy scopes un-editable; make flip-into-obo scope handling grant-profile aware (entra clears the carry-over, rfc8693 honors the request); clear obo-era audience/scopes when flipping back to oauth_user (the IdP-side app identifier is not a resource indicator); mirror the same column policy in the create handler. Revocation honesty: hide obo mint-cache rows from the user connections list and refuse the per-server disconnect with 409 — deleting the row returned 204, audited token_revoked, and then session-start priming silently re-minted from the surviving captured credential. Console form: keep the audience-from-URL autofill off for sign-in passthrough (the audience there is an IdP application identifier, and the prefilled URL passed every validation layer then failed every mint); clear the autofill artifact when switching modes; omit unchanged scopes from submissions. Dispatchers: route tool/resource/prompt through one shared lookup-error mapping and an auth-model-aware 401-exhausted detail (obo users are no longer pointed at a consent flow that does not exist). The consent-url audit count drops 13 → 7: the three per-dispatcher mapping copies collapsed into _pool_lookup_error. Priming: skip all obo servers for users with no captured credential via one existence SELECT (previously three reads per server per session). Also: USER_SCOPED_AUTH_TYPES now lives in storage._protocol so the backend SQL predicates share the application layer's set; docs describe the actual purge-on-transition behavior (the orphan-and-reactivate claims were wrong); the entra e2e setup script no longer aborts silently under set -e with suppressed stderr. |
||
|
Patrick Buckley
|
44e9d46e40 |
fix(mcp): address pre-push review — obo scope/audience/priming defects
Frontend↔backend interaction bugs the backend-only rounds couldn't see: - flip oauth_user->oauth_obo: the admin form re-submits the pre-filled oauth_user scopes, so the flip-clear (gated on 'oauth_scopes' not in body) was skipped -> rfc8693 mints broke permanently. Clear now compares to the existing value, robust to the re-send. - entra edit-lockout: update validated the MERGED scopes, so a pre-existing scoped obo row under the entra profile became un-editable (every PUT 400'd). Reject only when the request actually SETS scopes. - flush-cache button never rendered: consented_users_count is now populated for oauth_obo rows too, not just oauth_user. Mint engine + priming: - audience guard: a cached token minted for a since-narrowed audience is no longer served (extracted _is_fresh_obo_cache_row, used pre/post-lock, checks refresh-less + audience-match + fresh). _persist_obo_cache_row now delete+creates so the row's audience column tracks the mint (a plain update kept the stale audience -> re-mint loop). - obo session priming passes revoke_ambiguous_escalation=False (new param threaded through get_obo_...), so an IdP wobble during a bulk prime can't escalate-revoke obo cache rows cluster-wide. Cross-node + lifecycle: - pending-consent success-clear now clears once-per-failure-cycle via a _pending_consent_cleared set (was gated on 'we wrote it' -> never fired cross-node/after-restart -> stale badge). Still no per-call SQL. - identity-unlink cache purge: per-server try/except so one failure doesn't leave other servers' bearers un-purged. - entra ignored-scopes: warn once per audience (was per-mint flood -> downgraded to debug -> no signal on a profile switch). - entra_setup.sh writes single-quoted .env values (secret may contain $). +6 regression tests. 1892 mcp/oidc/console tests green; mypy clean. Refs #551. |
||
|
Patrick Buckley
|
e5f8453e1a |
fix(mcp): complete oauth_obo revocation lifecycle + fix hot-path regression (follow-up review)
Addresses the high follow-up review of the first fix round: Revocation lifecycle (the review's dominant theme): - identity-unlink now purges the user's minted obo cache rows in addition to revoking the credential, and the response/audit report the actual effect (credential + N cache rows) instead of a blanket revoked=true; warmed-session residual (bounded by token TTL) documented - bulk-revoke on obo is now an honest cache-FLUSH: distinct audit event (obo_cache_flushed) + response effect=cache_flush_remints, since the shared credential survives and the next dispatch re-mints (oauth_user keeps its durable revoke semantics) - changing oauth_audience on a pool-backed row now purges cached tokens (audience is the token binding), like URL/name/auth_type changes - flipping oauth_user->oauth_obo now clears the stale AS-consent scopes (else rfc8693 sends them -> invalid_scope loop); write path rejects oauth_scopes under the entra profile (it mints <audience>/.default) - a cache row bearing a refresh token is never served as an obo token (guards the cross-node purge-vs-refresh race) Self-inflicted regression: - _clear_pending_consent_sync is now gated on an in-memory _pending_consent_written hint, so the common successful-dispatch path issues ZERO SQL (was an unconditional per-dispatch DELETE) Observability + cleanups: - restore the obo_mint_rejected log carrying the IdP error text (the shared-helper unification dropped it); event names passed as whole literals so alerting can grep them - persist_rotation typed Callable[[str], Awaitable[None]] (was Any) - _prime_one branches on _obo_server_names (no pre-lookup SQL for oauth_user) - removed now-dead any_oauth_user_mcp_servers (3 impls + tests) +13 regression tests. Full mcp/oidc/console suite 1888 green; mypy clean. Refs #551. |
||
|
Patrick Buckley
|
d84393c25c |
fix(console): widen admin MCP surface for oauth_obo (P0/P1 per review)
- write-time validation (_enforce_oauth_obo_requirements): reject an oauth_obo row with no oauth_audience (400) or no encryption key (503, else it SystemExits the cluster at next boot) — at the save choke point, not per-dispatch (findings 10137/10163) - update handler no longer nulls oauth_audience/oauth_scopes for oauth_obo (it needs them); clears only the oauth_user-only columns (10344) - auth_type-transition purge now covers every pool-backed transition, including oauth_user->oauth_obo (was skipped: old per-server-AS refresh tokens leaked into the mint cache + left a live grant at the old AS unrevoked) and oauth_obo->static/none (10326) - URL-change purge + https enforcement + client-secret clear now apply to oauth_obo, not just oauth_user (10339) - bulk-revoke accepts oauth_obo — the documented remediation for the stale rows a flip leaves behind (10705) +6 console tests (obo audience/key required, happy path, flip-purge, obo bulk-revoke). Refs #551. |
||
|
Patrick Buckley
|
b28e8bac80 |
feat(mcp): admin-scoped aggregate view for oauth_user server status
Resolves the one regression the user-scoping in
|
||
|
Patrick Buckley
|
4db7d9c6cf |
feat(mcp): per-(user, server) ClientSession pool with OAuth dispatch
Phase 5 of OAuth-MCP — adds a per-(user, MCP-server) ClientSession
pool to MCPClientManager alongside the existing static-server path,
gated entirely on the per-server `auth_type='oauth_user'` config.
Pool architecture:
- `_user_pool_entries: dict[(user_id, server_name), PoolEntryState]`
with lazy connect on first dispatch, per-key asyncio.Lock allocated
on the mcp-loop, idle eviction coroutine (default 600s TTL, LRU cap
200), and an `in_flight` counter as the eviction interlock so live
calls can never be torn down mid-flight.
- `_dispatch_pool` runs the token-state machine: missing token →
`mcp_consent_required`; key-rotation decrypt failure →
`mcp_token_undecryptable_key_unknown` with NO consent prompt and NO
auto-delete; expired token → silent refresh under per-(user, server)
advisory lock; refresh failure → revoke + consent.
- `_classify_failure` separates transport (trips breaker) from auth
401/403 (does NOT trip breaker — server-only invariant) from
protocol (no breaker change).
- `entry.open_lock` held only across connect-or-reuse and released
before the `await session.call_tool` so concurrent calls from one
user against one server overlap (validated by Spike 1 scenario 2).
Auth-class failures are fail-soft in Phase 5: any 401/403 surfaced by
the SDK propagates to the agent as a tool error and the next dispatch
reconnects on a fresh refresh. Real introspection of upstream 401/403
is a Phase 6 concern — the MCP SDK's `streamable_http` post_writer
swallows `httpx.HTTPStatusError` upstream, so detecting status from
the response chain requires `McpError(CONNECTION_CLOSED)` payload
parsing or a custom httpx middleware around `streamablehttp_client`.
The mid-flight 401 refresh-retry path and the `mcp_insufficient_scope`
structured error for 403 step-up land together in Phase 6, gated by
an integration test that drives a real upstream 401/403 (the unit-
test injection of `HTTPStatusError` is what masked the production gap
on the first apply-findings pass — the integration test is the
structural gate so the gap can't reopen). RFC §1.5 steps 4-5 and the
phase table in §Implementation phases reflect this scope split.
Multi-node refresh contention:
- New `StorageBackend.acquire_advisory_lock_sync` Protocol method.
SQLite returns nullcontext (single-node, in-process asyncio.Lock
is sufficient). Postgres uses `pg_try_advisory_xact_lock` with
retry on a fresh per-attempt connection, so waiters don't pin pool
connections during the AS roundtrip. Inner try/except + nested
finally ensures conn is always returned to the pool, even when
begin / execute / yield / commit raises mid-body.
- Lock ordering: pg_advisory outer, asyncio.Lock inner. Re-read after
lock collapses cluster-wide contention to one HTTP roundtrip per
(user, server) per refresh window.
- `_PgRefreshLock` enter/exit pinned to a single-worker
ThreadPoolExecutor so SQLAlchemy connection state stays
thread-affine across cancellations.
Token storage refactor:
- `get_user_access_token_classified` returns a tagged TokenLookupResult
(Token / MissingToken / DecryptFailure / RefreshFailed) so the
dispatcher maps each state to the right user-facing error.
- `get_user_access_token` is now a thin wrapper around the classified
variant; the previous duplicated state machine is gone.
Security:
- Pool dispatch + admin endpoints reject `http://` URLs for
`auth_type='oauth_user'` servers (only exact loopback hostnames are
exempt — `*.localhost` is intentionally NOT honored because RFC 6761
localhost-zone resolution is configuration-dependent and could route
bearers to non-loopback IPs via custom resolvers / hosts file /
Docker overlays). Validated at three layers:
`_dispatch_pool` (structured `mcp_oauth_url_insecure` error),
`_connect_one_pool` (defensive ValueError), and
`admin_create_mcp_server` / `admin_update_mcp_server` (400 before
storage write).
- Admin URL change on an oauth_user row purges per-user OAuth tokens
bound to the old URL: bearers are bound (via OAuth resource /
audience) to the URL active at consent time, so silently rebinding
them to a new URL is a token-binding violation. Re-consent forces
fresh issuance for the new resource.
- Encryption-key fingerprints stay in audit logs only; no longer
surfaced in agent-facing error payloads.
User_id thread-through:
- `MCPClientManager.call_tool_sync(..., user_id=None)` (additive;
default None preserves the static path byte-identically).
- `ChatSession._exec_mcp_tool` passes `self._user_id or None`.
- `set_app_state(app_state)` setter wires OAuth state at lifespan
startup, called from both turnstone-server and turnstone-console.
Performance:
- LRU cap eviction iterates `_user_pool_entries` (not
`_user_pool_last_used`) so pre-dispatch entries are eligible.
- Eviction batch closes via `asyncio.gather` instead of serial await.
- `_resolve_pool_target` returns the resolved server row to
`_dispatch_pool` to eliminate the second DB lookup.
- Production reachability of pool dispatch is gated on Phase 7
(catalog scoping) wiring pool tools into `_tool_map`; until then
pool dispatch is reachable only via direct `call_tool_sync` with a
prefixed name (the path the new pool tests exercise).
Hardening parity preserved:
- Static path (auth_type ∈ {none, static}) byte-identical; PR #296
hardening (SDK #2147 mitigations, anyio cancel-scope, stale-session-
and-stack guard, server-only circuit breaker) intact.
- `test_reconnect_preserves_static_state_identity` unchanged + green.
- `MCPTokenStore.get_user_token` does not auto-delete on
MCPTokenDecryptError (key-rotation safety).
- Notification debounce stays manager-level.
- Connect-failure cleanup factored into
`_safe_teardown_on_connect_failure` shared by both connect paths.
Tests: 5475 → 5493 (+18). New file `tests/test_mcp_user_pool.py`
plus additions to test_mcp_oauth_refresh.py, test_mcp_admin_api.py,
and test_mcp_client.py covering: pool data structures, lazy connect,
eviction TTL + LRU + lock interlock, dispatch state machine (token
states), failure classification, http-rejection at dispatch and
admin layers, URL-change-purges-tokens (sec), concurrent dispatch on
one (user, server), pg_advisory lock parity, and user_id threading.
Phase exit criterion (synthetic load test 50 users × 3 servers × LRU
30 × 1000 calls × 200 evictions) deferred to a post-Phase-5 fitness
spike that runs against a staging deployment with real FDs and real
network behaviour, not a CI mock — same shape as Spike 1's
pre-Phase-0 SDK validation.
Out-of-scope for Phase 5 (Phase 6+): SDK-level 401 refresh-retry +
403 `mcp_insufficient_scope` (Phase 6), per-user catalog scoping
(Phase 7), consent UX SSE event + dashboard renderer (Phase 8),
admin UI status indicators (Phase 9).
|
||
|
Patrick Buckley
|
62bbc332af |
fix(mcp): pin OAuth return_url + sanitise read-scope status
Addresses ten findings on the Phase 4 OAuth-MCP commit: four from the PR #478 review surface, plus six surfaced by a follow-up multi-stage review of the first round of fixes. Two of the latter were genuine security regressions in the very code that claimed to close those holes. Security -------- - _validate_return_url now pins return_url same-origin against the configured oidc_config.redirect_base instead of request.url. Behind a permissive front proxy that did not normalise Host, an attacker could spoof Host and provide a matching absolute return_url to mint an open redirect off /api/mcp/oauth/start. Same fix pattern as PR #476 OIDC. - Reject return_url values containing literal backslashes or starting with `//` up front. urlparse leaves backslashes inside `path`, so a value like `/\evil.example/foo` slipped through the path-only branch and became the protocol-relative `//evil.example/foo` after WHATWG- conformant browsers normalised the backslash — re-introducing the open redirect the same-origin pin was meant to close. - internal_mcp_status (read-scoped) projects through a new _strip_server_status_for_read helper that drops the verbose `error` text and replaces it with a coarse `has_error` boolean. The error string is built as `f"{type(exc).__name__}: {exc}"` and so carries stdio binary paths (FileNotFoundError) or internal MCP URLs (httpx.ConnectError) — equivalent to leaking command/url, which this same patch deliberately strips. Approve-scoped refresh and reconnect callers continue to receive the full `error` text via the existing _strip_server_status helper. - internal_mcp_status now returns the projected (sanitised) entries for every server in mcp_mgr.get_all_server_status() instead of emitting the un-sanitised dict that included `command` (stdio argv) and `url` (remote MCP endpoint). Sibling refresh/reconnect endpoints already used _public_server_status to strip these. - internal_mcp_status docstring documents the trust boundary — server enumeration to read scope is intentional so dashboards can render per-server indicators; verbose error detail and command/url remain approve-scoped. Correctness / UX ---------------- - _validate_return_url comparison normalises (scheme, host, port) before equality. Lowercases hostname and collapses the scheme's default port, so `https://App.Example.COM/x` and `https://app.example.com:443/x` are recognised as same-origin with `redirect_base = https://app.example.com` instead of being silently downgraded to the `/` fallback. - mcp_crypto startup-gate error message now names both `mcp_token_encryption_keys` (rotation list) and `mcp_token_encryption_key` (single) so an operator using rotation isn't misled into thinking only the singular form is valid. Cleanup ------- - Delete the unused _KNOWN_TRUSTED_ENDPOINT_HOSTS legacy re-export shim in oidc.py (zero callers — a no-op that survived the Phase 4 oauth_ssrf extraction). Sphinx :data: docstring reference at validate_discovered_endpoint updated to point at turnstone.core.oauth_ssrf.KNOWN_TRUSTED_OAUTH_ENDPOINT_HOSTS directly. The Google multi-origin allowlist is unaffected — it lives at the canonical name and is read from oauth_ssrf.py:164. - test_mcp_oauth_handlers TestValidateReturnUrl imports _validate_return_url at module level instead of repeating the import inside each test method. - test_server_lifespan_mcp_crypto replaces a fragile `messages.count("mcp_token_encryption_key") >= 2` substring trick with `re.search(r"mcp_token_encryption_key(?!s)", messages)` — asserts the singular form directly via negative lookahead. Tests ----- 5448 pass (+13 vs the prior tip): - TestValidateReturnUrl gains backslash-bypass, protocol-relative, default-port, uppercase-host, and explicit-port-mismatch cases alongside the original same-origin / cross-origin / scheme- mismatch / path-only cases. - TestInternalMcpStatusEndpoint asserts the `error` text never reaches the read-scope wire (binary-path FileNotFoundError no longer appears anywhere in the rendered response) and that the coarse `has_error` boolean lights up correctly on the failed server. - TestInternalMcpStatusEndpoint also pins the no-mcp-client path to `{"servers": {}}`. - _routes_with_internal extended to include the /api/_internal/mcp-status route so the new tests can exercise it through TestClient. - Existing test_startup_aborts_with_oauth_user_row_and_no_key strengthened to require both singular and plural key names appear in the error log. |
||
|
Patrick Buckley
|
29c42c1427 |
feat(mcp): per-(user, server) OAuth 2.1 + PKCE flow
Lands the OAuth flow that uses the token-at-rest store from the prior
commit: discovery (RFC 9728 PRM + RFC 8414 AS metadata with operator-
override precedence), PKCE S256 (mandatory — refuse AS without it),
RFC 8707 resource indicator on every authorize and token request,
RFC 7591 minimal one-shot dynamic client registration, authorization-
code exchange, refresh-token grant with re-read-after-acquire single-
flight lock, and the /v1/api/mcp/oauth/{start,callback} endpoints
mounted on both server and console.
Refactored:
- validate_url_no_ssrf, validate_discovered_endpoint, is_localhost,
effective_port, sanitize_log_text moved out of oidc.py into a shared
oauth_ssrf module; oidc.py re-exports for compatibility. The shared
helpers also expose async wrappers (validate_url_no_ssrf_async,
validate_discovered_endpoint_async) so OAuth-MCP discovery — invoked
from async handlers — does not block the event loop on the
synchronous socket.getaddrinfo call.
- MCPTokenStore.get_oauth_client_secret reader path added (the prior
commit was write-only)
- Storage protocol gains create/pop/cleanup_*_mcp_oauth_pending_state
and get_mcp_oauth_client_secret_ct (mirror OIDC pending-state
pattern: SQLite BEGIN IMMEDIATE select-then-delete, Postgres atomic
DELETE...RETURNING)
Refresh-grant correctness:
- When the AS omits refresh_token (RFC 6749 §6 — MAY rotate), the
existing refresh value is preserved at the OAuth-flow layer rather
than cleared, so production ASes (Google, Auth0 default, Okta) don't
force re-consent every hour
- expires_in accepts int, float, str-with-decimal — earlier int-coerce
through str() failed on float and silently dropped expiry tracking
- The refresh-grant `resource=` parameter (RFC 8707) is the canonical
MCP server URL, not the audience. Audience and resource are distinct
concepts; using audience as resource would mismatch the AS RS
allowlist.
Audience handling:
- _validate_token_audience accepts str or tuple; the callback resolves
accepted_audiences = {server_url, oauth_audience} and validates
against the set, so Auth0-style ASes that honor `audience=` (not
RFC 8707 `resource=`) issue tokens that pass audience-bound
validation
- build_authorize_url emits both `resource=` (RFC 8707) and
`audience=` (Auth0-style) per server config; comment documents which
AS implementations need which form
Security hardening:
- redirect_uri pinned to oidc_config.redirect_base instead of the
request Host header — closes the same Host-header injection PR #476
fixed for OIDC. Both /start and /callback return 503 with operator-
actionable hint when redirect_base is unset
- DCR registration runs under per-server asyncio.Lock with re-fetch
inside the lock, so concurrent /start callers don't both register
and overwrite each other's client_id (the second user's code is no
longer rejected on callback)
- /callback error branch pops the pending state row before redirecting
so a leaked state can't be replayed against a separately-obtained
code in the 60s cleanup window
- WWW-Authenticate Bearer parser handles RFC 7235 quoted-string
escapes (\" and \\) instead of the naive [^"]+ regex
- AS-controlled response bodies and error_description query params go
through sanitize_log_text before reaching exception messages or
audit details. AS error responses are parsed for the standard
RFC 6749 fields (error, error_description, error_uri), each
capped at 80 chars and run through redact_credentials to defend
against ASes that echo the request body back into their error
payload.
- oauth_as_issuer_cached is re-validated against the SSRF guard on
read; on rejection the column is cleared and PRM rediscovery runs
- DCR / token-endpoint / refresh-endpoint response bodies cap at 64
KiB (PRM/AS metadata cap stays at 256 KiB) so a hostile or
malfunctioning AS can't exhaust client memory.
- oauth_client_secret operator input capped at 1024 chars at the
admin-form boundary; longer plaintext rejected with 400.
- /start and /callback responses stamp `X-Frame-Options: DENY` so the
redirected pages can't be framed by attacker sites.
- delete_user cascades to mcp_user_tokens and mcp_oauth_pending so
user deletion no longer leaves dangling per-user OAuth state.
- Renaming or deleting an oauth_user MCP server purges per-user
tokens and pending OAuth state for the previous server name
(delete_mcp_oauth_rows_by_server_name). The OAuth tables key on the
mutable server_name; without this purge, a future server with the
same name (and an attacker-controlled URL) would silently rebind
prior user tokens. A future schema migration will replace the
server_name key with a server_id FK + ON DELETE CASCADE.
- get_user_access_token catches MCPTokenDecryptError (raised when no
installed key can decrypt the row, e.g. after key rotation) and
falls through to None so dispatch surfaces a re-consent rather than
crashing.
- oauth_user MCP server rows are skipped in the static auto-connect
path. Auto-connecting them at startup with empty headers fails the
AS check and trips the circuit breaker; per-user tokens come online
lazily once the user has consented.
Audit (mcp_server.oauth.* prefix):
- consent_started, consent_completed, consent_failed, token_refreshed,
token_revoked, dcr_registered. _audit_event is async and wraps
record_audit in asyncio.to_thread so the audit write doesn't block
the event loop. resource_id on the audit row is the immutable
server_id (PK UUID) so admin-driven server renames don't break
event correlation; server_name is exposed in detail for cross-
reference. dcr_registered detail.has_secret reflects whether the
DCR-issued secret was actually persisted (the prior code reported
has_secret=true even on persistence failure).
- _admin_mcp_action audits the immutable server_id, not the mutable
server_name (which is what the column is — the table's PK was
always server_id).
- All OAuth-flow log keys use the mcp_server.oauth.* prefix to match
the audit-action taxonomy.
Lifespan close-order in turnstone.server and turnstone.console.server
is reversed (LIFO) — mcp_oauth → mcp_crypto → oidc — to match init
order.
Deferred until the upcoming per-user pool integration:
- Multi-node refresh-lock contention via pg_advisory_lock
- DCR re-register on token-endpoint 401 (the dispatch path surfaces
those 401s)
- TTL-LRU caching of decrypted plaintext access tokens
- DNS-rebinding hardening (httpx Transport pin) — documented as
limitation in oauth_ssrf module docstring
Tests: 7 new test files / ~85 new tests covering discovery precedence
+ PRM quoted-string parsing, PKCE round-trip, SSRF helper extraction,
authorize/callback handlers including 503-on-no-redirect-base + DCR
concurrency + JWT audience polymorphism + callback-error-pops-pending,
refresh single-flight lock, refresh resource-vs-audience regression,
decrypt-error fallthrough, _db_servers_to_config skipping oauth_user,
pending-state CRUD round-trip.
|
||
|
Patrick Buckley
|
7f132e7230 |
feat(mcp): token-at-rest encryption layer for OAuth-MCP
Phase 3 of docs/design/oauth-mcp.md. Adds the Fernet/MultiFernet wrapper, [security] config loader with rotation support, MCPTokenStore CRUD facade, typed MCPTokenDecryptError that maps to the RFC's mcp_token_undecryptable_ key_unknown class, and a startup gate that fails loud when auth_type= 'oauth_user' rows exist without a configured encryption key. Crypto module (turnstone/core/mcp_crypto.py): - MCPTokenCipher wraps cryptography.fernet.Fernet + MultiFernet for rotation; encrypt with first key, decrypt by trying each in order - load_mcp_token_cipher_config reads [security] mcp_token_encryption_keys (plural list) or mcp_token_encryption_key (singular), validates each key is base64-decodable to exactly 32 bytes - MCPTokenCipherConfig is repr=False with custom __repr__ that redacts raw key bytes (defense in depth against accidental log/traceback leak) - _key_fingerprint produces an 8-hex-char SHA-256 prefix for audit attribution without exposing the key - MCPTokenStore handles encrypt-on-write / decrypt-on-read for mcp_user_tokens and mcp_servers.oauth_client_secret_ct - get_user_token MUST NOT auto-delete the row on MCPTokenDecryptError (test_get_user_token_with_wrong_key_raises_decrypt_error verifies the row stays intact across a key-mismatch read) - initialize_mcp_crypto_state / close_mcp_crypto_state lifespan helpers shared between server and console Storage protocol (5 new ciphertext-only methods): - set_mcp_oauth_client_secret_ct (dedicated writer; deliberately NOT added to MCP_SERVER_MUTABLE so generic update_mcp_server cannot write the secret column) - create_mcp_user_token, get_mcp_user_token, update_mcp_user_token_after_refresh, delete_mcp_user_token Server + console lifespans (turnstone/server.py + console/server.py): - after OIDC init, count auth_type='oauth_user' rows; if any exist and no encryption key is configured, log an actionable error and raise SystemExit(1) - without oauth_user rows, missing key is fine (lazy validation; admin flip without restart returns 503 from the admin handler) - app.state.mcp_token_cipher / .mcp_token_store populated when key configured; None otherwise Admin handlers: - _require_token_store_for_oauth_secret pre-mutation gate validates token_store availability and oauth_client_secret type BEFORE storage.create_mcp_server / update_mcp_server runs, so a 503 from a missing key never leaves an orphan row or partial-update state - _apply_oauth_client_secret encapsulates the encrypt + audit write used after the storage mutation; rolled out across both create and update handlers - 503 message references both mcp_token_encryption_key (singular) and mcp_token_encryption_keys (plural for rotation) - non-string oauth_client_secret payloads (false / 0 / lists / dicts) are rejected with 400 instead of being str()-coerced - when auth_type transitions away from oauth_user, the encrypted secret column is cleared in the same admin call (with audit), so flipping back doesn't silently resurrect a stale credential Audit events (mcp_server.oauth.* per audit.py taxonomy; RFC's mcp.oauth.* renamed for consistency): - mcp_server.oauth.client_secret_set fired from admin handlers with cleared:bool and key_fingerprint - mcp_server.oauth.token_decrypt_failure fired from MCPTokenStore .get_user_token when no installed key can decrypt; carries key_fingerprints_attempted Tests: 35 new tests across test_mcp_crypto, test_mcp_token_store, test_server_lifespan_mcp_crypto, plus 6 admin-API tests covering the no-orphan-row, no-partial-update, secret-clear-on-transition, and non-string-secret-rejection invariants. Suite at 5337 (Phase 3 added ~50 tests including the rebase-imported skill suite). cryptography>=42 promoted from transitive (lacme[tls]) to direct dep since the encryption layer is now core, not optional. Phase 4 (OAuth flow) wires the actual callers; Phase 3 adds only the crypto layer and is exercised entirely by tests. |
||
|
Patrick Buckley
|
d675b237a3 |
feat(mcp): oauth schema + minimum admin form
Adds the data model and admin UI surface required by the OAuth-MCP flow.
Phase 2 of the per-user delegation initiative.
Schema:
- migration 049 creates mcp_user_tokens (PK user_id, server_name) and
mcp_oauth_pending (PK state, indexed by created_at)
- eight new columns on mcp_servers: auth_type ('none' / 'static' /
'oauth_user', NOT NULL DEFAULT 'static') plus six oauth_* config
fields and oauth_as_issuer_cached
- post-upgrade UPDATE normalises auth_type to 'none' for streamable-http
rows whose headers are NULL/empty/'{}'; stdio rows are left at the
'static' default (auth_type is HTTP-auth-only)
- _schema.py kept in lockstep with the migration so metadata.create_all
and alembic upgrade produce identical shapes
- mcp_user_tokens / mcp_oauth_pending TypedDicts in _protocol.py for
Phase 3/4 use (no CRUD methods yet)
Storage / API:
- create_mcp_server gains the eight kwargs across protocol + sqlite +
postgresql
- MCP_SERVER_MUTABLE picks up auth_type and the six text oauth_* fields;
oauth_client_secret_ct is intentionally NOT in the whitelist — Phase 3
will own ciphertext writes via a dedicated method
- McpServerInfo + Create/Update Pydantic schemas extended; oauth_client_secret
accepted as plaintext input but discarded (Phase 3 wires encryption)
Admin handlers:
- _parse_auth_type validates against {'none', 'static', 'oauth_user'} and
rejects empty / unknown values; shared between create and update
- when auth_type changes away from 'oauth_user', the oauth_* config
columns are explicitly nulled in the same UPDATE so the row stays
consistent
- _clean_oauth_text caps text fields at 512 chars (URLs at 2048) to bound
admin write surface
- _mask_mcp_secrets now masks oauth_client_secret_ct to '***' regardless
of reveal=true (write-only field)
- audit detail dict redacts oauth_client_secret if present
Frontend:
- new "Multitenant Authorization" fieldset on the MCP-server modal with
three radio buttons (None / Shared / Per-user OAuth 2.1)
- conditional OAuth subform: AS URL, registration mode (preregistered /
dcr; cimd is future), client ID, client secret, scopes, audience
- secret input is autocomplete=off and never round-trips on edit
- audience auto-populates from the MCP server URL on blur
- headers textarea hidden and submitted as {} when auth_type is 'none' or
'oauth_user' so flipping the radio cleans up server-side state
Tests: storage round-trip for the new columns, oauth_pending table smoke,
migration 049 upgrade/downgrade with stdio-vs-http normalisation, four
admin-API tests for auth_type validation and oauth_*-clear-on-flip-away.
Suite passes 5284 (matched pre-Phase-2 baseline 5267 + 17 new).
Stacks on Phase 0; no behavioural change for existing rows.
|
||
|
Patrick Buckley
|
eb2a119da9 |
refactor(mcp): remove periodic refresh, add manual refresh/reconnect controls
Deletes the _periodic_refresh task and its supporting state
(_refresh_task, _refresh_failures, _refresh_backoff_until,
_REFRESH_BACKOFF_BASE/MAX, _DEFAULT_REFRESH_INTERVAL, refresh_interval
kwarg) from MCPClientManager. Push notifications and operator-driven
manual refresh now cover all catalog-update needs; the long-running
4-hour timer was dead complexity that obscured the per-user pool
work to come.
Catalog freshness on auto-reconnect is preserved by scheduling an
unblocking _refresh_server task on the mcp-loop after _connect_one
succeeds; the calling thread returns immediately so half-open
recovery latency does not double. Adds MCPClientManager.reconnect_sync
(clears the circuit, closes any existing session, calls _connect_one,
clears stale catalog on failure).
Wires a new pair of operator endpoints —
POST /v1/api/admin/mcp-servers/{name}/refresh and
/v1/api/admin/mcp-servers/{name}/reconnect — that fan out to all
nodes through the existing _internal route family, with per-row
"Refresh" and "Reconnect" buttons in the MCP Servers admin tab.
The new node-internal paths /api/_internal/mcp-{refresh,reconnect}/
are gated to the approve scope to prevent direct unprivileged
reconnects bypassing the console's admin.mcp gate. Internal
endpoints return generic error messages and a filtered status
payload (no command/url) to keep transport details admin-gated.
Drops the [mcp] refresh_interval setting, the
--mcp-refresh-interval CLI flag, and the matching config-mapping
entry; updates docs/architecture.md, docs/tools.md,
docs/settings.md, and the three PlantUML diagrams that referenced
the periodic loop.
Tradeoffs (intentional):
- Idle nodes will not auto-rejoin a recovered MCP server until
traffic arrives or an operator clicks Reconnect. The previous
background reconnection loop is gone by design — push
notifications + operator controls replace it.
- Console fan-out blocks on the slowest node (existing pattern);
not changed here.
This is Phase 1 of the OAuth-MCP series — feature subtraction
ahead of per-user state.
|
||
|
Patrick Buckley
|
f63b2915cc |
review: address copilot feedback on user_id trust check
Remove console-proxy from trusted_sources — end-user tokens via the console proxy already carry the real user_id in the JWT, so they must not be able to override it via the request body (impersonation risk). Only bridge and console service identities are trusted to forward user_id on behalf of users. Add 5 tests covering the trust boundary. |
||
|
Patrick Buckley
|
ada8b80509 |
test: add MCP reload and reconcile endpoint integration tests (#141)
* test: add MCP reload and reconcile endpoint integration tests 11 new tests covering POST /v1/api/admin/mcp-servers/reload (console) and POST /v1/api/_internal/mcp-reload (node). Verifies reconcile_sync invocation, fan-out results, permission checks, missing storage handling, and mixed node error propagation. * fix: address review — lazy-import internal_mcp_reload to avoid heavy module load Move turnstone.server import inside _routes_with_internal() helper so the full server module (which reads UI static assets) is only loaded when node-side endpoint tests actually run, not during test collection. |
||
|
Patrick Buckley
|
414eb52d67 |
feat: raise scaling limits for 1000-node clusters (#129)
* feat: raise scaling limits for 1000-node clusters Raise hardcoded limits throughout the codebase so clusters up to 1000 nodes work without configuration changes. Scaling limits: - max_workstreams default 10 → 50 (configurable via settings) - Console fan-out concurrency 50 → 200 (configurable: cluster.node_fan_out_limit) - MCP max servers 50 → 200 (configurable: cluster.mcp_max_servers) - Console SSE queue 500 → 2000, server global SSE queue 500 → 1000 - httpx proxy pool: explicit max_connections on both proxy clients - PostgreSQL pool 5+10 → 2+3 per process (right-sized for short-burst queries) - Redis pool: explicit max_connections=200 on both sync and async brokers Performance optimizations: - Redis list_nodes(): replace N+1 SCAN+GET with SCAN+MGET - Collector poll: raise thread pool to 200 (matches fan-out limit) - Server SSE: dedicated ThreadPoolExecutor(200) for queue polling - Fan-out: new get_all_nodes() removes hardcoded limit=1000 ceiling Bug fixes: - Settings reload notification was silently failing (called .get() on tuple) - Watch fan-out only queried 500 nodes instead of full cluster New cluster settings (configurable via admin Settings tab): - cluster.node_fan_out_limit (default 200, range 10-1000) - cluster.mcp_max_servers (default 200, range 1-2000) Adds docs/pgbouncer.md for PostgreSQL connection pooling at scale. Adds ddgStressCluster compose profile (100 nodes, 10 groups of 10). Updates architecture, console, docker, settings, and API reference docs. * fix: add image tag to compose anchors to avoid redundant builds All cluster/stress services inherit `build:` from the anchor, causing Docker to attempt 200+ separate builds. Adding `image: turnstone:local` means Docker builds once and all services reuse the cached image. * fix: address Copilot review feedback on scaling PR - Remove magic number in get_all_nodes (limit=None instead of 2**31) - Size httpx proxy pool from fan-out limit setting (not hardcoded 250) - Cap cluster.node_fan_out_limit max_value to 500, mark restart_required - Convert _publish_config_change from sync to async (was blocking event loop) - Use shutdown(wait=True, cancel_futures=True) for SSE executor * fix: add PostgreSQL env vars to cluster bridge anchor Bridges initialize storage for auth/migrations but the bridge anchor was missing TURNSTONE_DB_BACKEND and TURNSTONE_DB_URL, causing all bridges to fall back to SQLite. With 100 bridges sharing the same volume, concurrent SQLite migrations corrupt the database. * fix: address Copilot round 2 + PG connection exhaustion at startup Copilot feedback: - Raise cluster.node_fan_out_limit max_value to 1000 (matches target) - Cache fan-out limit on app.state at startup instead of re-reading DB per request (pool and semaphore now use the same value consistently) - Remove unused params from _publish_config_change Stress cluster fix: - Raise PG max_connections to 300 (configurable via POSTGRES_MAX_CONNECTIONS) to handle 200 processes connecting simultaneously at startup - Bump PG shared_buffers to 128MB and memory limit to 1G to match - Add DB env vars to production bridge service * fix readme * fix: startup resilience for large clusters Server no longer crashes when LLM backend is unreachable at startup. detect_model() accepts fatal=False, returning (None, None) so the server starts in degraded mode with circuit breaker open. The health monitor will detect when the backend becomes available. Migration runner retries with jittered exponential backoff (up to 10 attempts) when PostgreSQL rejects connections during startup stampedes. Collector httpx pool sized to match poll workers (was using default of 100 connections with 200 workers). Also addresses Copilot round 2: - Raise cluster.node_fan_out_limit max_value to 1000 - Cache fan-out limit on app.state at startup - Remove unused params from _publish_config_change - Add DB env vars to production bridge service * fix: replace silent error suppression with structured logging Audit and fix 30+ instances of silently swallowed exceptions across 8 files. No-raise contracts are preserved — all changes add logging while keeping the same return-value behavior. memory.py (26 changes): Every storage operation now logs on failure. Previously the entire persistence facade had zero logging — messages, workstream state, and structured memories could silently stop being saved. server.py: Usage recording failures now log at warning (was pass). Global SSE fan-out errors log at debug (was pass). console/server.py: Config reload notification logs per-node failures at warning. Settings read fallbacks log at warning with the default value used. auth.py: User existence check logs at warning (was pass). Setup rollback failures log at error (was suppress). OIDC state cleanup logs at debug (was suppress). mcp_client.py: DB-managed MCP server list failure logs at warning (was pass). collector.py: Node poll failure upgraded from debug to warning with exc_info. Health fetch failure logs at debug with exc_info (was silent). bridge.py: Best-effort plan rejection logs at warning (was suppress). Malformed SSE data logs at debug (was suppress). session.py: Tool output UI callback failure logs at debug (was suppress). * fix: stagger collector poll with deterministic per-node jitter Each node gets a stable offset within the first half of the poll interval, derived from hashing the node_id against a Mersenne prime (2^31 - 1). This spreads HTTP requests across the cycle instead of firing all 100+ at the same instant. Also raises poll interval from 10s to 15s and HTTP timeout from 5s to 30s for large-cluster resilience. * fix: add startup jitter to bridge heartbeat and health monitor probe Bridge heartbeat: deterministic per-node jitter (from node_id hash) spreads initial registration across the first quarter of the heartbeat TTL. At 100 bridges with 60s TTL, heartbeats spread across 15s instead of all firing at T=0. Health monitor probe: deterministic per-process jitter (from PID hash) spreads initial LLM backend probes across half the probe interval. At 100 servers with 30s interval, probes spread across 15s instead of all hitting the LLM at T=30. Both use the same Mersenne prime hashing approach as the collector poll jitter for consistency. * fix: split collector httpx timeout and raise keepalive pool Use separate connect/read/write/pool timeouts instead of a single 30s for all phases. Raise keepalive connections from 50 to 200 so the collector reuses TCP connections across poll cycles instead of constantly tearing down and re-establishing them. * fix: narrow detect_model return type for CLI and eval callers detect_model() now returns tuple[str | None, int | None] to support fatal=False. CLI and eval always use fatal=True (the default), which guarantees a non-None model or SystemExit. Add assert to narrow the type for mypy. |
||
|
Patrick Buckley
|
1efcbcf2ba |
perf: parallelize _collect_mcp_status and _notify_nodes_mcp_reload wi… (#73)
* perf: parallelize _collect_mcp_status and _notify_nodes_mcp_reload with asyncio.gather Both functions queried cluster nodes sequentially, making latency O(N × timeout). Use asyncio.gather to query all nodes concurrently, matching the existing admin_list_watches pattern. Also reuse the shared proxy_client instead of creating throwaway httpx clients per node, and add debug logging on MCP status fetch failures. * perf: bound node fan-out concurrency and improve debug logging Add _NODE_FAN_OUT_LIMIT (50) semaphore to all three gather fan-out sites (_collect_mcp_status, _notify_nodes_mcp_reload, admin_list_watches) to cap concurrent outbound connections below the httpx pool limit, leaving headroom for other proxy traffic at 1000-node scale. Add exc_info=True to all debug log calls for actionable diagnostics. * test: add unit tests for _collect_mcp_status and _notify_nodes_mcp_reload 11 tests covering success, non-200, missing URL, exceptions, empty cluster, and mixed multi-node scenarios for both fan-out helpers. |
||
|
Patrick Buckley
|
19abc0cc65 |
feat: admin MCP Servers tab — database-backed MCP server management w… (#62)
* feat: admin MCP Servers tab — database-backed MCP server management with live status Add MCP Servers admin tab (14th tab, System group) for managing MCP server definitions via the database instead of static JSON config files. Storage: `mcp_servers` table (migration 016), 6 CRUD methods on both SQLite and PostgreSQL backends, `MCP_SERVER_MUTABLE` field allowlist. Config priority chain: DB rows (if any enabled) → CLI `--mcp-config` → `mcp.config_path` setting → none. Nodes auto-load from DB on startup via `load_mcp_config(storage=)`. Hot-reload: `reconcile_sync(storage)` diffs running servers against DB — adds missing, removes stale, reconnects changed. `_db_managed` set tracks DB-sourced servers so config-file servers (MCP_CONFIG env) are never removed by reconcile. Per-server `AsyncExitStack` for clean teardown. Reload pattern: console writes to DB then signals nodes via `POST /_internal/mcp-reload` (update by reference, no config payload). Console admin API: 7 endpoints under `/v1/api/admin/mcp-servers` (CRUD + reload + import), `admin.mcp` permission, secret masking (env/headers replaced with *** unless ?reveal=true), audit log sanitization. Unified view: tab merges DB-managed servers with config-sourced servers detected on nodes. Config servers shown as read-only rows with "config" badge — no edit/delete. Admin UI: 7-column grid with magenta status dots, transport badges, single-column create/edit modal, paste-based JSON import (mcpServers format), detail modal with per-node status. Mobile 3-column collapse, reduced-motion support, backdrop-click dismiss, focus trapping. SDKs: 7 methods on Python (async+sync) and TypeScript SDKs. Also fixes: Settings tab permission gate (admin.users → admin.settings), _ALL_PERMISSIONS list in governance.js (5 missing permissions added), _internal/mcp-reload added to APPROVE_PATHS. Docs: architecture.md (14 tabs), api-reference.md (7 endpoints), 20-mcp-architecture.puml updated with admin-driven lifecycle. 66 new tests (2232 total). * fix: address Copilot review feedback on MCP admin PR - Docs: fix "merges both sources" → "first-match-wins priority" (architecture.md) - Validation: require command for stdio, url for streamable-http transport - Validation: check args/headers/env types in import handler before storing - Schema: add transport/command/url to McpServerStatus, source to McpServerDetail - Thread safety: move all remove_server_sync mutations onto MCP event loop thread - Regenerate OpenAPI JSON snapshots for TypeScript SDK |