Skip to content

auth: JWKS refresh cooldown, fetch timeout and shared backoff, operator-configured metadata resource, no-usable-keys 503 (see Consiliency/pmcp#231, Consiliency/pmcp#320) - #325

Merged
ViperJuice merged 2 commits into
mainfrom
fix/231-auth-hardening
Oct 1, 2026

Conversation

@ViperJuice

@ViperJuice ViperJuice commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Implements the merged plan .consiliency/plans/detailed-231-auth-surface-hardening-20260922-1625.md (#284) for S-07, S-08 and S-10 (see #231), and maps a JWKS with no usable keys to the existing 503 path (see #320). S-09 was already merged in #281 and is not touched here.

What changed

src/pmcp/auth.py — AsyncJWKS

  • New keyword params with defaults: forced_refresh_cooldown_seconds=10.0, fetch_timeout_seconds=5.0, refresh_failure_backoff_seconds=5.0.
  • get() follows the plan's order. There is a pre-lock fast path (_serve_cache_instead). Then, under _refresh_lock: (1) an authoritative re-check of cache and cooldown; (2) the S-08 backoff gate, which returns the unexpired cache or raises ResourceServerJWKSUnavailable and never stamps the cooldown; (3) _last_forced_refresh = now if forced, which is the only place it is stamped; (4) _fetch(), where a failure stamps _last_refresh_failure and a success resets it.
  • _fetch(): aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=self._fetch_timeout_seconds)).
  • _select_jwk_key: jwt.PyJWKSetError → ResourceServerJWKSUnavailable("JWKS contains no usable signing keys."). The text is fixed, so no pyjwt message, JWKS content or token is ever included. It propagates through validate_resource_server_token's except ResourceServerAuthError: raise into handle_mcp's existing 503 branch.

src/pmcp/transport/http.py

  • New closure helper _canonical_resource(). It returns resource_server_audience if set; otherwise f"{scheme}://{netloc}/mcp" from the normalized auth_metadata.protected_resource_metadata_url (its netloc has no userinfo and keeps IPv6 brackets); otherwise "". It never reads request.
  • handle_protected_resource_metadata returns "resource": _canonical_resource().

Docs: CHANGELOG [Unreleased] has three Security bullets (S-07/S-08/S-10) and one Fixed bullet (#320). The README resource-server paragraph and the SECURITY.md async-JWKS sentence are extended, with no [C-NN] citation added.

Plan assumptions that changed or were adapted

  1. Re-anchoring. perf(auth): match the redactor's keyword rule in one linear pass #303 (876fd33) and feat(redaction): additive shape-based redaction on top of the existing redactor (see Consiliency/pmcp#234) #310 (7680445) changed only the diagnostic sanitiser and redaction in auth.py. The plan's edits never call that code, so every plan anchor still holds, shifted by a few lines (AsyncJWKS is now at auth.py:382, the metadata handler at http.py:469). The pyjwt 2.15.1 bump (fix(deps): raise pyjwt to 2.15 for the advisories pip-audit reports against 2.13 #318) is what turns both auth: a JWKS with no usable keys returns 500 instead of the 503 key-set-unavailable response #320 cases into PyJWKSetError. The floor is >=2.15.0, so jwt.PyJWKSetError always exists.
  2. The S-08 waiter-bound test asserts a fetch count instead of wall-clock time. The plan's test asserted elapsed < 2*FT over a 0.2 s sleeping fetch. That depends on timing, and this task requires deterministic concurrency tests. The new test holds the first fetch on an asyncio.Event until all N=5 waiters are queued on the lock, then lets it fail, and asserts fetches == 1. Every fetch is already bounded by the timeout, so the number of fetches is what decides "about one timeout" versus "N × timeout". The plan's mutation (delete the failure stamp) gives fetches=5.
  3. The S-08 timeout test uses fetch_timeout_seconds=0.5 with a 5 s outer wait_for, not the default 5 s with an 8 s outer bound, which saves about 5 s per run. A separate assertion pins the default (_fetch_timeout_seconds == 5.0). The total=60 mutation still goes red on the outer bound.
  4. No new setting for S-10. The task asked for docs on "the new operator-configured resource setting", but the merged plan adds none. The published resource comes from the existing resource_server_audience (--oauth-audience / PMCP_OAUTH_AUDIENCE) and falls back to the configured metadata URL's origin. The docs describe that behaviour, and no flag or env var was invented. The three new AsyncJWKS knobs are constructor-only with defaults and are not exposed on the CLI.
  5. auth: a JWKS with no usable keys returns 500 instead of the 503 key-set-unavailable response #320 and S-08 backoff. The backoff does not apply to auth: a JWKS with no usable keys returns 500 instead of the 503 key-set-unavailable response #320: the fetch succeeded and returned a well-formed but useless key set, which is cached for its TTL. Each request then forces a refresh (no kid can match), and the S-07 cooldown limits that to one per 10 s. I mapped the error at the selection site only. Rejecting such a set in _fetch would also change caching semantics, which goes beyond the issue.
  6. _make_app in tests/test_transport_http.py gained a resource_server_audience kwarg, an option the plan offers, for the audience-first S-10 test.

Tests added

  • tests/test_auth.py: test_s07_random_unknown_kids_do_not_each_force_a_fetch, test_s07_concurrent_unknown_kids_share_one_forced_refresh, test_s07_s08_recovery_after_failed_forced_refresh_waits_out_the_cooldown[immediate|timeout], test_s07_s08_backoff_rejection_does_not_consume_the_cooldown, test_s08_fetch_is_bounded_by_a_timeout, test_s08_concurrent_get_bounds_the_last_waiter, test_jwks_no_usable_keys_is_unavailable_not_a_crash[empty|malformed_only].
  • tests/test_transport_http.py: test_s10_metadata_resource_does_not_reflect_forged_host, test_s10_metadata_resource_uses_configured_audience, test_s10_metadata_resource_fallback_keeps_ipv6_brackets.
  • tests/test_auth_origin_wiring.py: TestJWKSWithoutUsableKeys::test_jwks_no_usable_keys_is_503_value_free[empty|malformed_only]. This test exercises the real AsyncJWKS and validator path end to end, with only _fetch patched. It asserts 503, error="temporarily_unavailable", and that the response body, headers and captured DEBUG logs contain no JWKS content and no token.
  • --collect-only | grep -E "s07|s08|s10" gives the plan's 10 node ids.
  • The concurrency tests use no timed sleeps. They use asyncio.Event plus sleep(0) drains, or a fake clock patched onto pmcp.auth.time.

Measured mutations

Each mutation was run the same way: save a copy, apply only the mutation, check git diff --stat (against the committed fix), run the named tests, restore from the saved copy, then cmp (identical every time) and confirm git diff --stat is empty.

# Mutation (reverts one fix) git diff --stat Result
M1 S-07: delete the _last_forced_refresh = now stamp auth.py | 2 +- red: unknown kids each forced a fetch: calls=3
M2 S-07 concurrency: in-lock re-check → None if force_refresh else self._cached(now) (pre-lock check left alone) auth.py | 2 +- burst red: concurrent unknown kids amplified the refresh: fetches=6 (1 initial + 5 forced for 5 concurrent requests); sequential S-07 test green (1 failed, 1 passed), which is why both tests exist
M3 S-07×S-08: stamp moved to after a successful _fetch() auth.py | 4 ++-- [immediate] red: a forced fetch ran at t=9.0s, before max(C, d+B)=10.0s: fetches=2; [timeout] green (1 failed, 1 passed), as the plan predicts
M4 S-08 timeout: ClientTimeout(total=60) auth.py | 2 +- red: asyncio.exceptions.TimeoutError from the 5 s outer wait_for
M5 S-08 waiter bound: delete _last_refresh_failure = time.monotonic() auth.py | 1 - red: last waiter not bounded: each waiter re-attempted the failed fetch: fetches=5 for 5 waiters
M6 S-07×S-08 composition: stamp the cooldown on the backoff-rejected branch auth.py | 2 ++ red: rotated key not accepted at backoff expiry: fetches=1
M7 S-10: resource back to str(request.url_for("mcp")) http.py | 2 +- all 3 S-10 tests red: metadata reflected attacker Host: resource='http://evil.example/mcp', 'http://evil.example/mcp' == 'https://canonical.example/mcp', fallback resource wrong: resource='http://evil.example/mcp'
M8 S-10 IPv6: rebuild from hostname/port http.py | 2 +- red: fallback resource wrong: resource='https://2606:4700::1:8443/mcp'
M9 #320: remove the PyJWKSetError mapping auth.py | 12 +----------- unit red with uncaught jwt.exceptions.PyJWKSetError (both cases); HTTP red: assert 500 == 503 (both cases)
M10 F1: decode guard catches nothing (except () as exc) auth.py | 2 +- red, all 24 matrix tests (12 unit + 12 HTTP): unit forged tokens not mapped to invalid_token: ['RS256/kid: TypeError', …, 'ES256/kid: InvalidKeyError', …]; HTTP forged tokens not answered 401: ['ES256/kid: 500', 'ES256/no-kid: 500']
M11 F1: generic InvalidTokenError description back to str(exc) auth.py | 2 +- 12 unit tests red: token/key content in the error description: ['crit-unknown']; 12 HTTP tests green (12 failed, 12 passed), because the HTTP layer never renders descriptions, so only the unit level catches this one
M12 F3: remove the generic except Exception in get() (except () as exc) auth.py | 2 +- red: a non-JWKSUnavailable fetch failure escaped: ['RecursionError']
M13 F3: _fetch JSON parse catches (ValueError,) only auth.py | 2 +- red: RecursionError: maximum recursion depth exceeded while decoding a JSON array from a unicode string
M14 F2: except BaseException in get() (cancellation opens the backoff) auth.py | 2 +- red: a cancelled fetch opened the backoff: waiter got ResourceServerJWKSUnavailable('JWKS refresh recently failed for https://issuer.example/jwks.json; backing off.')
M15 F4: keep the default port (normalisation line → pass) http.py | 2 +- red: fallback resource not canonical: resource='https://gw.example:443/mcp'

Round 1 review follow-ups (F1–F4)

F1: a forged token could cause an unauthenticated 500 (this predates the PR, and main has it too). The token's header picks the algorithm and its kid picks the key, so a caller can pair any allowed algorithm with any published key. On a mismatch, pyjwt or cryptography raised TypeError ("Expecting a PEM-formatted key."), ValueError, or jwt.InvalidKeyError while preparing the key. InvalidKeyError is a PyJWTError but not an InvalidTokenError, so nothing caught any of them.

  • Fix: jwt.decode now runs inside _decode_with_key. It lets InvalidTokenError through to the caller's existing mapping and maps (jwt.PyJWTError, TypeError, ValueError) to ResourceServerAuthError("invalid_token", "Token could not be verified with the published key.").

  • Why the boundary is here: the try wraps exactly one call, and its only variable inputs are the attacker's token and a published key. Key preparation and verification are where pyjwt and cryptography signal "this key cannot verify this token" with these built-in types. A TypeError/ValueError from PMCP's own code anywhere else (key selection, scope handling, the HTTP handler) still escapes, and so does anything outside that class (AttributeError, KeyError, RecursionError and so on). The PyJWKSetError mapping from auth: a JWKS with no usable keys returns 500 instead of the 503 key-set-unavailable response #320 stays at the selection site, so a broken key set is still a 503 and not a 401.

  • Also changed: generic error descriptions are now fixed text. For a generic InvalidTokenError, the description is now "Invalid token.". pyjwt quotes an unknown crit extension name back in its message; the matrix test caught that. Claim errors whose pyjwt text is fixed (expired, not-before, issuer, iat, missing required claim, disallowed algorithm) keep their text, so the existing claim tests are unchanged. The HTTP layer never rendered descriptions anyway; this keeps the exception value-free for anyone embedding the validator.

  • Cases tested. They follow what the code does with token input before and during jwt.decode (get_unverified_header, kid selection, PyJWKSet, decode):

    • all 17 algorithm values (every RS/PS/ES/EdDSA variant, plus HS256, none, NONE, an unknown value and the sentinel), each with and without the key's kid, against RSA, EC P-256, EC P-384, OKP Ed25519 and oct keys, and against a mixed set;
    • under both the default allow-list and a wide one (every public-key algorithm pyjwt supports);
    • a kid that is an int, list or object; alg given as a list or missing; a 180 KB header; crit naming an unknown extension, crit that is not a list, crit with b64; jku, x5u and an embedded jwk;
    • a header that is an array, a string, not JSON, not UTF-8, or nested 5,000 levels deep;
    • a payload that is an array, a string, not JSON, or nested 5,000 levels deep;
    • two or four segments, bad base64, and empty segments.

    test_forged_tokens_are_invalid_token_and_value_free (12 parametrizations, unit level) asserts every case ends in invalid_token and that the sentinel never appears in str(exc) or .description. TestForgedTokenMatrixOverHTTP (12 parametrizations) sends the same cases through the real app, AsyncJWKS and validator, with only _fetch patched. It asserts every case is a 401 and never a 500. It also asserts that the sentinel (planted in the payload, the signature, header fields and every key's kid) and the token itself never appear in the body, the headers, or logs captured at DEBUG.

F2: cancellation. I decided a cancelled fetch does not open the backoff. Cancellation means the caller went away (server shutdown or an embedding caller's timeout), not that the endpoint failed. Opening the backoff would turn one caller's disappearance into 503s for every other request for B seconds. State stays consistent:

  • the lock is released by async with;
  • _jwks/_expires_at are untouched;
  • _last_refresh_failure is unchanged;
  • the cooldown stamp stays, because the attempt was made (the plan's "stamp every attempted forced fetch" rule).

The next waiter fetches. The fetch inside get() catches Exception, never BaseException, so CancelledError propagates unchanged. test_s08_cancelled_fetch_does_not_open_the_backoff pins this: the waiter gets the fetched keys, fetches == 2, _last_refresh_failure == -inf, and the lock is free. The S-07×S-08 composition is unchanged: the backoff branch still never stamps the cooldown, and M6 is still red.

F3: any fetch failure opens the backoff. Besides ResourceServerJWKSUnavailable, get()'s fetch wrapper now catches Exception. It stamps _last_refresh_failure and raises ResourceServerJWKSUnavailable("JWKS fetch failed for <redacted url>."). _fetch's JSON parse now catches (ValueError, RecursionError); ValueError covers JSONDecodeError and UnicodeDecodeError. test_s08_any_fetch_exception_opens_the_shared_backoff holds the first fetch and then raises a RecursionError carrying a sentinel. All 5 waiters get ResourceServerJWKSUnavailable, with fetches == 1 and no sentinel. test_s08_deeply_nested_jwks_body_is_unavailable serves a real 200 KB [[[…]]] body over loopback and expects Invalid JWKS JSON.

F4: S-10 fallback. I chose to make the fallback correct and document the rule, following how the app is mounted. create_http_app routes MCP at /mcp and the metadata route at the metadata URL's literal path, on the same app. So https://gw.example/tenant/a/.well-known/oauth-protected-resource is served at /tenant/a/... while MCP is still served at /mcp. A prefix in the metadata URL is therefore not a prefix of MCP, and origin + /mcp is right; carrying the prefix over would publish a URL the app does not serve. A scheme-default port (:443 for https, :80 for http) is now dropped, so https://gw.example:443/... publishes https://gw.example/mcp. The README states the rule and says to set --oauth-audience when MCP is reachable at a different public URL. The new test is test_s10_metadata_resource_fallback_drops_default_port_and_prefix.

Gates (read from logs)

Round 1 head (4083989):

  • uv run ruff check src/ tests/ → All checks passed!
  • uv run ruff format --check src/ tests/ → 171 files already formatted
  • uv run mypy src/ → Success: no issues found in 52 source files
  • uv run pytest tests/test_auth.py tests/test_transport_http.py tests/test_auth_origin_wiring.py --cov-fail-under=0 -q → 218 passed in 6.25s
  • uv run pytest tests/test_auth.py -k "s07 or s08" ×20 → 20 × "10 passed, 142 deselected"
  • Full suite -m 'not live and not slow' (with npm_config_cache, npm_config_store_dir, pnpm_config_store_dir unset, run detached on dev0) → 4928 passed, 3 skipped, 80 deselected in 740.54s (0:12:20), rc=0
  • python3 scripts/check_security_claims.py → OK ... SECURITY.md: 129 cited node id(s)
  • python3 scripts/check_plan_consistency.py plans/phase-plan-v13-*.md → blocking inconsistencies: 0

Initial head (3c043d7): module tests 190 passed in 1.88s; full suite 4900 passed, 3 skipped, 80 deselected in 656.73s (0:10:56), rc=0.

Do not merge until the panel review is done.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RiabTgWZRfuqiUQ9siEegF

ViperJuice and others added 2 commits September 30, 2026 22:53
…operator-configured metadata resource, no-usable-keys 503

S-07: forced (unknown-kid) JWKS refreshes are limited to one per cooldown
window (default 10 s). The cooldown is checked before the refresh lock as
a fast path and re-checked authoritatively under it, so a concurrent burst
of unknown kids shares one forced fetch. The window is stamped only when a
fetch is actually attempted.

S-08: the JWKS fetch has a total aiohttp ClientTimeout (default 5 s), and
a failed refresh opens a shared backoff (default 5 s) so waiters queued
behind it fail at once instead of each re-attempting it. A
backoff-rejected attempt does not consume the S-07 window.

S-10: /.well-known/oauth-protected-resource publishes
resource_server_audience, else the normalized metadata URL's origin plus
/mcp (netloc kept, so IPv6 brackets survive); never the request Host.

A JWKS with no usable keys (PyJWKSetError) now maps to
ResourceServerJWKSUnavailable (503) with a fixed, value-free message.

See #231 and #320.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiabTgWZRfuqiUQ9siEegF
…ckoff, canonical metadata fallback

Round-1 review follow-ups on the auth hardening.

F1: jwt.decode runs inside a narrow guard (_decode_with_key) that maps
PyJWTError (non-InvalidTokenError), TypeError and ValueError from key
preparation or verification to invalid_token with fixed text. An
unauthenticated alg/key-type mismatch (ES256 against an RSA key) used to
escape as a 500. Generic InvalidTokenError descriptions are now fixed
text; claim errors whose pyjwt text is fixed keep it.

F2/F3: any non-cancellation exception from the JWKS fetch opens the
shared backoff and becomes ResourceServerJWKSUnavailable (503); _fetch
maps RecursionError from a deeply nested body. Cancellation does not open
the backoff (the caller went away, not the endpoint).

F4: the metadata resource fallback drops a scheme-default port; a path
prefix is not carried over because MCP is served at /mcp (documented).

See #231 and #320.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiabTgWZRfuqiUQ9siEegF
@ViperJuice ViperJuice closed this Oct 1, 2026
@ViperJuice ViperJuice reopened this Oct 1, 2026
@ViperJuice
ViperJuice merged commit 89559db into main Oct 1, 2026
25 of 27 checks passed
@ViperJuice
ViperJuice deleted the fix/231-auth-hardening branch October 1, 2026 07:09
ViperJuice added a commit that referenced this pull request Oct 1, 2026
…dler errors described; broken stdio frames never shown (see #297)

Answers the round-9 board on #314 @ a449dd9:
- codex P1 (blocking): at DEBUG the MCP SDK logs each JSON-RPC message
  its transports receive and send, after the envelope is accepted and
  before pmcp validates the payload. install_log_scrubber now makes every
  rendering of an SDK message object (the four JSONRPC classes,
  SessionMessage and its metadata) structural, and the scrubber describes
  dict-shaped messages in log arguments. This takes over the SDK-traffic
  part of #315.
- Found by the new valid-envelope sweep (pre-existing): a request
  handler's escaping validation error reached the caller as pydantic's
  text, and a nameless catalog entry was logged by its content. Both are
  now structural.
- claude N1-r9: the stdio reader stays in "describe" mode after a
  rejected frame (or a frame head behind a banner) until a line parses.
- claude N2-r9: banner shapes now described are listed.

Repro: 12 of 12 scenarios leak on rev 9, 0 of 12 on rev 10. Main moved to
89559db (#325, #327) during the revision;
merged, and the 40 patches are embedded against 89559db and proven
cmp-identical to wip/297-code@6078419. Full suite: 5356 passed. Mutants:
64/64 killed; NO_STATIC 62/64 (M22, M35 by design).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiabTgWZRfuqiUQ9siEegF
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant