Repository navigation
fix(plugin-auth): the MCP access-token verifier follows the signing keyring instead of holding its own copy of the key set for five minutes - #24225
Conversation
…ough the signer's keyring reader, with one bounded fresh read for an unknown key Claude-Session: https://claude.ai/code/session_01Nk17Z26X5DB59iQztDBADH Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nk17Z26X5DB59iQztDBADH Co-authored-by: Claude <noreply@anthropic.com>
…p-verifier-jwks-seam
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 16 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 5201d74975817aa264c6fdad71f7d057a7a47035 && git checkout 5201d74975817aa264c6fdad71f7d057a7a47035
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b9ac35cbfe60afcea88947ed4eaf8c50e3efc214 90849befc10c379cb4c37b1c2c086cc9783b2edd && git checkout -B drift-repro b9ac35cbfe60afcea88947ed4eaf8c50e3efc214 && git merge --no-ff 90849befc10c379cb4c37b1c2c086cc9783b2edd
node scripts/docs-audit/affected-docs.mjs --json b9ac35cbfe60afcea88947ed4eaf8c50e3efc214
|
席位核对ACCEPT ·
Generated by Claude Code |
Fixes #24186
Clause-②: no
The MCP access-token verifier (
AuthManager.verifyMcpAccessToken) no longer holds its own copy of the signing key set on a 5-minute TTL. It reads the key set on every call through the keyring reader that PR #24185 added for the signer (signing-key-read-cache.ts). So the set it verifies against follows both halves of #11633's cross-request caching rule: an observedsys_jwkswrite retires it, and a TTL bounds what that seam cannot see. A token signed by a key the set does not carry gets one bounded fresh read before it is refused.The two consequences, and what each pin holds
Stated abstractly, as the card does:
Reach, measured on
origin/main99e9d2957e/api/v1/mcpand its environment-scoped form). It setsacceptOAuthAccessToken(packages/runtime/src/http-dispatcher.ts:736), andresolveExecutionContexthands a compact-JWS Bearer toauthService.verifyMcpAccessToken(packages/runtime/src/security/resolve-execution-context.ts:159). REST, GraphQL and/auth/me/*do not accept OAuth access tokens.isMcpServerEnabled()defaults to on (packages/types/src/env.ts:594).resolveOidcProviderEnabledfollows it unlessOS_OIDC_PROVIDER_ENABLEDorplugins.oidcProvidersays otherwise.isMcpOAuthEnabled()is then true whenever the canonical origin is TLS, or plain HTTP on a loopback or private address. A default deployment over HTTPS runs this verifier.sys_jwkstoday. Only better-auth's jwt plugin, which mints a key on first use when the keyring holds no live key. On a host without Ed25519, it mints a fresh ES256 key once and hides the EdDSA rows. Nojwks.rotationIntervalis configured, so keys carry no expiry and nothing rotates them on a schedule.sys_jwksdeclaresapiEnabled: falseandapiMethods: [], and has no Setup nav entry. No CLI command, setting or admin route removes or rotates a key.Which route, and why: the verifier reads through PR #24185's reader
The seat's premise measures true. The verifier's refresh was already
api.getJwks(), better-auth'sgetJwksendpoint. Its keyring read isgetJwksAdapter().getAllKeys(), which callsoptions.adapter.getJwks, and that iscreateSigningKeyReader(better-auth@1.7.3dist/plugins/jwt/adapter.mjsandindex.mjs). It reads the samesys_jwksrows, uses the same per-engine write counters and the same TTL. The verifier only held the endpoint's answer for five more minutes on top. So the fix removes that held copy (jwksCache,JWKS_CACHE_TTL_MS) and reads through on every call. One seam and one bound now cover the signer,/jwksand the verifier.alg,crv, the parsedpublicKey,kid), and only that is handed to jose. The row read beneath it is the shared reader's, held for the signer. That read includes theprivate_keyreadback (internal-field-readback.ts), as it did on the verifier's refresh path before this change. With a warm reader the verifier causes no keyring read at all.signing-key-read-cache.ts:retireHeldKeyrings(engine). It records a forced re-read as a keyring write that entered and settled, so the reader's existing rule retires every held entry and refuses to hold a read that was in flight. The reader needs no second case. AlsoUNKNOWN_SIGNING_KEY_REREAD_FLOOR_MS. Neither is reachable from the package entry (the module is not re-exported fromindex.ts).The bound on the fresh read
A token whose verification fails with jose's
JWKSNoMatchingKey(checked withinstanceof errors.JWKSNoMatchingKeyon the installedjose@6.2.12, codeERR_JWKS_NO_MATCHING_KEY) earns one fresh read. Every other failure is refused at once, because a re-read cannot change it.UNKNOWN_SIGNING_KEY_REREAD_FLOOR_MS, measured from when the last forced read started). Inside the floor, the token is refused as it would have been without the retry. A failed read still spends the interval.SIGNING_KEY_CACHE_TTL_MS). A stream of tokens naming keys that do not exist therefore at most doubles the keyring read rate the TTL already allows. While such a stream runs, a key added where this node cannot see it is verified no later than the TTL alone would allow. It is alsojose's defaultcooldownDurationfor the same mechanism in its remote key set, measured in the installeddist/webapi/jwks/remote.js: a reload on an unmatched key, at most once per cooldown. It is a constant, not a configuration key.The multi-node bound
A
sys_jwkswrite is observed only by the node whose engine it enters. On every other node:Clause-② measurement: built entry declarations, BASE
99e9d2957evs this head@objectstack/plugin-authdist/index.d.tsanddist/index.d.mts, built at both ends (rate-limit-storage.d.ts/.d.mts: byte-identical,diffexit 0). With comments stripped (TypeScript printer,removeComments), the only declaration lines that change are fourprivatemembers ofclass AuthManager, which print without types:private jwksCache?;andprivate static readonly JWKS_CACHE_TTL_MS;private unknownKeyReread?;andprivate rereadKeyringForUnknownKey;The rest of the raw diff is doc-comment prose. No public member, export, type, configuration key or environment variable changes. The two removed members are the defect itself, so no correct version of this change leaves them in place. The claim's
Clause-②: noholds on the clause's own question: no accepted set is widened and no public surface grows. The seat reads this; see the acceptance notes.Tests (at head
90849befc, which mergesorigin/mainb9ac35cbf; none of the four incoming commits touchesplugin-auth)src/auth-manager.mcp-verifier-keyring.test.ts. A realAuthManagerruns on a realObjectQLover@objectstack/driver-sql+ better-sqlite3, with its embedded authorization server on and tokens signed by the jwt plugin's ownsignJWT. Keyring reads are counted at the driver.pnpm --filter @objectstack/plugin-auth exec vitest run --maxWorkers=2(whole package): 158 files, 3091 passed, 10 skipped, 0 failed.pnpm --filter @objectstack/plugin-auth typecheckexit 0. That coverstsc --noEmit, the examples project andcheck:test-typecheck, and--listFilesOnly -p tsconfig.test.jsonlists the new file.Ablations, one per pin, through
scripts/ablation-replace.mjs(anchor hit once, blob changed, restore proven blob == HEAD andgit diff HEADempty)The subject is imported by relative path from
src/, so there is nodist/leg.undefinedfalseReverse leg: the BASE
auth-manager.ts(git restore --source=99e9d2957e, blob verified) turns all seven red. The removed-key pin reads "expected a principal to be null", and the unseen-key pin reads "expected null to match a principal". Restored to HEAD and verified.Cost, measured
3,000 sequential verifications of a valid token on the rig above, warm. Two runs each, on a shared box, so read the ratio: BASE 372 and 385 µs per call, head 696 and 765 µs per call, about 1.9x. Both make zero keyring reads in the loop. The added cost is CPU only: the in-process
getJwksendpoint call per verification, replacing the 5-minute held copy.Gates
Local, at head
90849befc.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, with no paths (merge baseb9ac35cbf, 4 paths, +472 / -16), derives 67 families. They are the same 67 the seat derived at99e9d2957e.--ranreconciliation: 67 derived, 67 run, 0 NOT-MEASURED, 0 UNRUN.6902fdd09, before the merge, hadpnpm check:dual-build-cjs-loadsexit 3 (PREREQUISITE NOT MET: not every package had adist/yet). On the merged head it measured: 111 published require entry points across 69 packages load.pnpm --filter @objectstack/spec check:generatedexits 0.origin/main's side movedpackages/spec; this diff does not touch it.plugin-authclosure was rebuilt on the merged head, and the head'sdist/index.d.tsis byte-identical to the pre-merge head's (md5 equal).test:repoand the rest of the CI battery are CI's; the PR's check-runs are the reading for those.Acceptance notes
yes. The only changed lines are the fourprivatemembers above, which carry no type and are unreachable from outside the class. I kept the claim'sno, because ayeswould declare a widening this diff does not make and force aminorgrade. If the seat rules the other way, the edit is this body's and the changeset'sClause-②line toyes, and the changeset grade tominor.Generated by Claude Code