Repository navigation
docs(plan): auth-surface hardening — S-07, S-08, S-10 of #231 - #284
Merged
Merged
Conversation
…07/S-08/S-10) Cooldown-bound the unknown-kid JWKS forced refresh (S-07), add a total ClientTimeout to the JWKS fetch held under the refresh lock (S-08), and return an operator-configured (non-Host-derived) resource from the protected-resource-metadata endpoint (S-10). Acceptance probes were run red against the current tree and deleted; see #231. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
The appended entry recorded both `handoff_ref` and `reflection_ref` as ABSOLUTE paths inside the plan worktree, and the reflection had since moved, so one pointer already dangled and both would dangle the moment the worktree is removed after merge. Rewritten to the convention every other `type=detailed` entry uses -- a repo-relative handoff path and a `~`-prefixed reflection path -- and validated before commit: both now resolve on disk, slugs are unique, schema_version is intact. A manifest is an index; an index that points nowhere is worse than no index, and it is the same shape as the other "claims that do not match reality" this session has been clearing out. See #231. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
Address the panel block (codex): a total JWKS fetch timeout bounds each lock holder, not each request — after a failed refresh with an expired cache, N concurrent waiters time out sequentially (~N*timeout). Add a shared failure-backoff so the last waiter is bounded to ~1 timeout, state that bound explicitly, and add a concurrent get() acceptance test (measured red: 1.004s ~= N*FT on the current tree). Also: node ids carry s07/s08/s10 for the -k filters, add the S-10 audience-first-branch criterion, reframe the SECURITY.md point as spirit-only (not a ledger violation), and correct the CHANGELOG "event loop" wording. See #231. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
…plan Round-3 block (codex): stamping the S-07 forced-refresh cooldown BEFORE the get() call lets an attempt the S-08 backoff rejects still consume a cooldown window, delaying a rotated key by up to one extra cooldown past backoff expiry. Fix: advance _last_forced_refresh only inside get(), past the backoff gate, right before _fetch() — never on the rejected branch. State the rotation-recovery bound (~B+T after recovery, independent of C). Add an integration acceptance test over get_for_token (unexpired cache, failed forced refresh, overlapping windows, recovery), measured with an executable model of the buggy-vs-fixed variant. Also fold in grok's nits: S-10 tests assert exact values, audience test builds create_http_app directly (not _make_app), and _canonical_resource parses the normalized metadata URL via hostname/port so userinfo cannot surface. See #231. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
…ecovery bounds (#231) Round-4 panel findings on the auth-surface plan: - BLOCKING 1 (codex): N concurrent unknown-kid requests each queued a forced refresh because the cooldown was checked only before the lock. The cooldown is now enforced in get(): a pre-lock fast path plus the authoritative re-check under _refresh_lock, before the backoff gate and the stamp. Measured on a real spike (reverted byte-identical): current tree fetches=6, round-3 variant fetches=6, fix fetches=2. - BLOCKING 2 (codex): the "~B+T, independent of C" recovery claim was wrong. Recovery = max(C, d + B) + T' after a failed forced fetch of duration d; with defaults ~10.5-15 s, and C governs whenever C - d > B. Both cases restated with arithmetic; a parametrized recovery test at the default relationship (C > B) is measured red on the current tree and on the stamp-on-success variant. - grok: the S-10 fallback takes the origin from the normalized URL's netloc (already userinfo-free and IPv6-bracketed) instead of rebuilding from hostname/port, which un-brackets IPv6. Node count 6 -> 10; every new test carries s07/s08/s10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
# Conflicts: # plans/manifest.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Planning artifact only — no
src/or test change. Plans the remainder of #231 (see #231); S-09 is already fixed in #281 and is untouched.The S-10 decision inverts the obvious fix, and that is the point
The obvious remedy is "make the metadata route call
_host_rejectedlikehandle_mcpdoes." That fails open in the default configuration._host_rejectedreturnsFalsewhenallowed_origins is None— the local-first default — so rejecting-outright would still reflect an attacker-supplied Host in exactly the deployment most users run. I verified this againsttransport/http.py.The plan instead returns a non-Host-derived
resource: the operator-configured audience when set, else the metadata-URL origin. That fixes it unconditionally rather than only when an allowlist happens to be configured.On the #211 constraint I asked the planner to check: #211 (
56a555c, "stop vouching for URLs a downstream server supplied") did not touch this handler — the Host-derived value predates it. #211 is a principle: never advertise an attacker-supplied value as vouched-for, remedied there by relay-but-mark-unverified. This fix is stronger, because the untrusted Host is never emitted at all, so no "verified" implication can be reintroduced.One characterisation I'd soften: the plan says the metadata path "violated" the existing SECURITY.md claim. That sentence is specifically about the audience being bound to the operator-configured canonical resource and never Host-derived; the metadata
resourcefield is a related but distinct value. The fix aligns them, which is good — but I'd call it an inconsistency in spirit rather than a claim violation.S-07 — why not the two obvious options
kidnegative cache: randomkids defeat it, since every new one is unknown by construction. That is precisely the attacker's input.rate_limit_rpm > 0, so it bounds nothing in the default config.Instead: a global forced-refresh cooldown (default 10 s). The first unknown
kidper window still forces one refresh, so a legitimate key rotation is picked up within one window (10 s ≪ the 300 s TTL) and can never be locked out — that was the fail-closed-in-the-wrong-direction risk I asked about. Akidunknown during an active window gets a transient, self-healing 401.S-08 — the timeout is the lock-hold bound
A total
ClientTimeout(default 5 s), not connect-only, because a slow-drip body under the size cap must also be bounded._fetchruns entirely insideget()'s_refresh_lock, so bounding_fetchbounds every waiter. On timeout the cache is untouched: waiters serve stale-valid keys if present, else503.All three measured red on the current tree
resource='http://evil.example/mcp'— the forged Host reflectedEach has a named post-fix mutation for the implementer. The probe was deleted and the tree left clean.
Stated as unconfirmed
No SECURITY.md ledger claim ID covers the metadata
resourcefield; the relevant prose is uncited. The plan instructs the implementer to runcheck_security_claims.pyafter any SECURITY.md edit and to add a[C-NN]citation only alongside a matching real test node.check_security_claims.pyOK (129 cited ids);check_plan_consistency.py0 blocking.🤖 Generated with Claude Code
https://claude.ai/code/session_013G8iK4noj1iNbSTqJVFEX6
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.