diff --git a/docs/configuration.md b/docs/configuration.md index e9228e5..8eec471 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -84,26 +84,27 @@ Opt-in support for the Model Context Protocol authorization flow ([issue #86](ht - app.example.com ``` -| Option | Type | Default | Description | -| ------------------------------------------------------- | -------- | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | -| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | -| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | -| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | -| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | -| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | -| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | -| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | -| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | -| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | -| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | -| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | -| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | -| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | -| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | -| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | -| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | -| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | +| Option | Type | Default | Description | +| ------------------------------------------------------- | --------------- | --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `mcp.enabled` | boolean | `false` | Master switch for the MCP OAuth endpoints | +| `mcp.issuer` | string | (none) | Authorization-server URI advertised in AS metadata. **Required when `mcp.enabled`** — startup fails otherwise, to prevent a Host-header-driven `iss`/`aud` | +| `mcp.resource` | string | `/mcp` | Canonical resource URI advertised in PRM (RFC 9728) and validated as `aud` on issued tokens (RFC 8707). Optional override; defaults safely from the pinned `issuer` | +| `mcp.providers` | string[] | (all providers) | Subset of upstream providers eligible for the MCP auth flow. **v1 requires exactly one** resolved provider — set this when more than one provider is configured globally, otherwise `/oauth/mcp/authorize` returns `server_error` | +| `mcp.dynamicClientRegistration.enabled` | boolean | `true` | Enable the `/register` endpoint when `mcp.enabled` is true | +| `mcp.dynamicClientRegistration.initialAccessToken` | string | (none) | If set, registration requires `Authorization: Bearer `. Otherwise open per RFC 7591 | +| `mcp.dynamicClientRegistration.allowedRedirectUriHosts` | string[] | (none) | Allowlist for redirect_uri hosts. Localhost always allowed per RFC 8252 | +| `mcp.clientIdMetadataDocuments.enabled` | boolean | `true` | Enable CIMD resolution for URL-shaped `client_id` values. Disable with `false` to reject all CIMD clients; see [Client ID Metadata Documents](./mcp-oauth.md#client-id-metadata-documents-cimd) | +| `mcp.clientIdMetadataDocuments.allowedHosts` | string[] | (none) | If set, only CIMD `client_id` URLs whose hostname is in this list are resolved. Others are silently rejected (`invalid_client`) without revealing the allowlist | +| `mcp.clientIdMetadataDocuments.fetchTimeoutMs` | number | `5000` | Deadline for CIMD document retrieval covering DNS, connect, and body read (milliseconds). Non-finite or non-positive values fall back to the default | +| `mcp.clientIdMetadataDocuments.maxDocumentBytes` | number | `65536` | Maximum CIMD document size in bytes (64 KB default). Responses exceeding this limit are rejected. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.enabled` | boolean | `false` | Enable the RFC 7523 `client_credentials` grant (`private_key_jwt`, EdDSA) for headless agents. Explicit opt-in; requires a non-empty `mcp.clientIdMetadataDocuments.allowedHosts` allowlist, CIMD enabled, and an `https:` `mcp.issuer` (RFC 6749 §3.2 — the token endpoint must be TLS; `http:` is permitted only for loopback development issuers) — all enforced at startup. See [Headless agents](./mcp-oauth.md#headless-agents-client_credentials) | +| `mcp.clientCredentials.accessTokenTtl` | number | `300` | Access-token lifetime in seconds for the `client_credentials` grant. No refresh token is ever issued — agents re-mint on 401. Non-finite or non-positive values fall back to the default | +| `mcp.clientCredentials.rateLimit` | number \| false | `30` | Issuance rate limit in requests/minute per `client_id` (token bucket, per node, and in fact per worker thread — N threads means an N× effective ceiling; a shared counter table would be a replication hot-write anti-pattern, and the assertion replay guard + 60s window bound cross-node abuse). Over-limit requests get `429` with `error: "slow_down"` and a `Retry-After` header. `false` or `0` disables; non-finite/non-positive values fall back to the default. CIMD metadata fetches are separately limited at a fixed 10 attempts/min per client_id URL (not configurable) | +| `mcp.signingKeyPem` | string | (generated) | PEM-encoded RS256 private key (PKCS#8) used to sign access tokens. When set, this key **always** wins as the signer — it is found in the key set by material match, or persisted on first use (under a deterministic kid so concurrent cluster nodes are idempotent). When unset, a UUID-kid keypair is generated on first boot. Because all persisted keys are published in the JWKS, tokens signed by any node verify everywhere — pinning is **recommended** for clusters but not strictly required | +| `mcp.keyRotationInterval` | number | `0` (disabled) | Signing-key rotation period in seconds. When `> 0`, a fresh UUID-kid keypair is generated at token-mint time once the current signer is older than this interval. Old keys are kept in the JWKS and deleted lazily once `2 × accessTokenTtl` has passed since their immediate successor was created (covering replication lag). Rotation is skipped while `signingKeyPem` is set — setting both emits a startup warning | +| `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm. Only `RS256` is supported in v1 (reserved for a future EdDSA option) | +| `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | +| `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | Sensitive leaves inside `mcp` support `${ENV_VAR}` expansion (e.g., `initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN}`), the same way provider credentials do. diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 25fae99..bf52cf9 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -636,6 +636,7 @@ mcp: clientCredentials: enabled: true accessTokenTtl: 300 # default; agents re-mint on 401 + rateLimit: 30 # default; issuance requests/min per client_id, false disables ``` Agents don't register. Each agent's `client_id` is an HTTPS URL to a CIMD @@ -707,6 +708,25 @@ is the document-declared `scope`; a `scope` parameter on the token request is not honored (a client can never escalate past its registered scope, and RFC 6749 §3.3 downscoping-on-request is future work). +Issuance is **rate-limited per `client_id`** (`mcp.clientCredentials.rateLimit`, +default 30 requests/min, `false` disables): over-limit requests receive `429` +with `error: "slow_down"` and a `Retry-After` header (seconds until a retry can +succeed). The limit is debited **after** the client assertion is verified, so it +counts only authenticated issuance — a caller cannot drain a real agent's quota +by replaying the agent's public `client_id` URL with a bogus assertion (those +fail verification with `401` and never touch the bucket). Pre-auth work is +bounded separately: CIMD metadata fetches are limited at a fixed 10 attempts/min +per `client_id` URL (cache hits don't consume, so only failing documents +repeat), and resolution/DNS concurrency is capped globally. +Both limits are per-node token buckets (a replicated counter would be a +hot-write anti-pattern; the assertion replay guard and ≤60s window bound +cross-node abuse). The bucket state is per worker thread: if the plugin runs +across N HTTP worker threads on a node, the effective ceiling is N × the +configured limit per `client_id` (as with the CIMD fetch cache and concurrency +caps, which are likewise per-thread). This is intentional for a defense-in-depth +control — treat the configured value as a per-thread floor, not a hard node-wide +cap. + Key rotation / revocation semantics: the fleet rotates a key by updating the agent's metadata document. The change takes effect within the CIMD cache TTL (up to 24 h, typically 1 h — bound it with `Cache-Control: max-age` on the diff --git a/src/lib/mcp/authorize.ts b/src/lib/mcp/authorize.ts index fc88b5a..0038db8 100644 --- a/src/lib/mcp/authorize.ts +++ b/src/lib/mcp/authorize.ts @@ -48,8 +48,9 @@ import { import { resolveIssuer, resolveResource } from './wellKnown.ts'; type ErrorJSON = { - status: 400 | 500; + status: 400 | 429 | 500; body: { error: string; error_description?: string }; + headers?: Record; }; type Redirect = { @@ -384,10 +385,12 @@ export async function handleAuthorize( client = await resolveClient(query.client_id, mcpConfig, logger); } catch (error) { if (error instanceof CimdClientError) { - return { - status: 400, + const json: ErrorJSON = { + status: error.statusCode === 429 ? 429 : 400, body: { error: error.oauthError, error_description: error.message }, }; + if (error.retryAfterSeconds !== undefined) json.headers = { 'Retry-After': String(error.retryAfterSeconds) }; + return json; } logger?.error?.('MCP authorize: client lookup failed:', error instanceof Error ? error.message : String(error)); return { diff --git a/src/lib/mcp/cimd.ts b/src/lib/mcp/cimd.ts index ea4a639..1aadf11 100644 --- a/src/lib/mcp/cimd.ts +++ b/src/lib/mcp/cimd.ts @@ -54,8 +54,10 @@ * protection (an attacker's document must not be able to force a fetch * per authorize request). * - Failures are NOT cached (the CIMD draft forbids caching error responses - * and invalid documents); repeated fetches of bad documents are left to - * rate limiting (#163). + * and invalid documents); repeated fetch attempts are instead rate-limited + * per client_id URL (fixed 10/min token bucket, #163) — legit clients hit + * the cache after their first success, so only failures repeat. Issuance + * itself is separately rate-limited at the client_credentials grant. * - Cached records are revalidated against the live redirect-host policy on * every hit, so tightening `allowedRedirectUriHosts` takes effect * immediately instead of after cache expiry (up to 24 h). @@ -76,6 +78,7 @@ import { request as httpsRequest } from 'node:https'; import { Readable } from 'node:stream'; import type { Logger, MCPClientIdMetadataDocumentsConfig, MCPClientRecord, MCPConfig } from '../../types.ts'; import { MCPClientStore } from './clientStore.ts'; +import { createRateLimiter } from './rateLimit.ts'; import { validateGrantTypes, validateRedirectUri, @@ -117,19 +120,46 @@ const cimdCache = new Map(); // requests for one uncached URL trigger a single fetch (thundering-herd guard). const inFlightResolutions = new Map>(); +// Per-URL fetch-attempt rate limit (#163) — fixed policy, no config knob: +// legit clients hit the cache after their first success (only failures +// repeat, and failures are never cached per the CIMD draft). Per-node +// semantics: see rateLimit.ts module header. +const CIMD_FETCH_ATTEMPTS_PER_MINUTE = 10; +const cimdFetchLimiter = createRateLimiter({ + capacity: CIMD_FETCH_ATTEMPTS_PER_MINUTE, + refillPerMinute: CIMD_FETCH_ATTEMPTS_PER_MINUTE, +}); + +/** + * Upper bound on a client_id length, shared by every entry point that uses a + * caller-supplied client_id as a lookup or rate-limiter map key. Same + * defense-in-depth family as the repo's 2048-char request-path cap. + */ +export const MAX_CLIENT_ID_LENGTH = 2048; + /** * Thrown when CIMD resolution fails due to a client-side issue (bad URL, * invalid document, unsupported auth method, allowedHosts policy). * Callers should surface this as the given `oauthError` with the `message` - * as the `error_description`. + * as the `error_description`. Throttle rejections additionally carry a + * `statusCode` (429) and `retryAfterSeconds` so callers can emit a proper + * `Retry-After` instead of a misleading auth error. */ export class CimdClientError extends Error { readonly oauthError: string; - - constructor(oauthError: string, message: string, options?: { cause?: unknown }) { + readonly statusCode?: number; + readonly retryAfterSeconds?: number; + + constructor( + oauthError: string, + message: string, + options?: { cause?: unknown; statusCode?: number; retryAfterSeconds?: number } + ) { super(message, options); this.name = 'CimdClientError'; this.oauthError = oauthError; + this.statusCode = options?.statusCode; + this.retryAfterSeconds = options?.retryAfterSeconds; } } @@ -210,9 +240,12 @@ export function _setFetch(fn: CimdFetch | null): void { _fetch = fn ?? pinnedHttpsFetch; } -/** Clear the CIMD cache (for testing). @internal */ +/** Clear the CIMD cache AND the per-URL fetch limiter (for testing) — tests + * that loop many resolution attempts against one URL rely on the limiter + * resetting alongside the cache. @internal */ export function _clearCimdCache(): void { cimdCache.clear(); + cimdFetchLimiter._reset(); } // --- SSRF guard helpers --- @@ -846,8 +879,25 @@ export async function resolveCimdClient( if (existing) return existing; // Total-concurrency bound: the in-flight map IS the counter (no await sits // between this check and the set below, so the cap cannot be raced past). + // Checked BEFORE the rate-limit take so a capacity reject doesn't spend a + // token without a fetch (keeps "only actual fetch attempts consume" true). if (inFlightResolutions.size >= MAX_CONCURRENT_RESOLUTIONS) { - throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly'); + throw new CimdClientError('temporarily_unavailable', 'CIMD resolution capacity reached; retry shortly', { + statusCode: 429, + }); + } + // Per-URL fetch rate limit (#163): only actual fetch attempts consume — + // cache hits returned above, dedup'd joiners, and capacity rejects never + // reach this. Fixed policy (no knob): legit clients are served from the + // cache after their first success; repeated attempts are the bad-document + // amplification vector this closes. + const rateLimit = cimdFetchLimiter.tryTake(clientId); + if (!rateLimit.allowed) { + logger?.warn?.(`CIMD: fetch rate limit reached for ${clientId}`); + throw new CimdClientError('slow_down', 'CIMD resolution rate limit reached; retry shortly', { + statusCode: 429, + retryAfterSeconds: rateLimit.retryAfterSeconds, + }); } const pending = fetchAndValidateCimd(clientId, cimdConfig, allowedRedirectUriHosts, logger).finally(() => inFlightResolutions.delete(clientId) @@ -977,8 +1027,9 @@ async function fetchAndValidateCimd( return record; } catch (err) { // Failures are never cached — the CIMD draft forbids caching error - // responses and invalid documents. Fetch amplification from repeated - // bad requests is rate limiting's job (#163). + // responses and invalid documents. Amplification from repeated bad + // requests is bounded by the per-URL fetch rate limit in + // `resolveCimdClient` (#163) instead. if (err instanceof CimdClientError) { logger?.warn?.(`CIMD: rejected client ${clientId}: ${err.message}`); throw err; @@ -1002,6 +1053,9 @@ export async function resolveClient( mcpConfig: MCPConfig | undefined, logger?: Logger ): Promise { + // An over-length client_id is never a real client — reject before it becomes + // a CIMD fetch-limiter key or a store lookup (unknown-client null, no leak). + if (clientId.length > MAX_CLIENT_ID_LENGTH) return null; const cimdConfig = mcpConfig?.clientIdMetadataDocuments; // CIMD is enabled by default when mcp.enabled; disabled only by explicit `enabled: false`. const cimdEnabled = cimdConfig?.enabled !== false; diff --git a/src/lib/mcp/rateLimit.ts b/src/lib/mcp/rateLimit.ts new file mode 100644 index 0000000..b6a357d --- /dev/null +++ b/src/lib/mcp/rateLimit.ts @@ -0,0 +1,103 @@ +/** + * Per-node in-memory token-bucket rate limiter (#163). + * + * Deliberately PER-NODE, not replicated: Harper replication makes a shared + * counter table a hot-write anti-pattern, and the surfaces this limits are + * already bounded cross-node — the client_credentials grant by the assertion + * replay guard and its ≤60s `exp` window, CIMD fetches by the concurrency + * caps. A node-local bucket is the right defense-in-depth without turning + * every token request into a replicated write. + * + * Buckets refill continuously (elapsed-time based — no interval timers), so a + * limit of N/min admits a burst of up to N and then one request per 60/N + * seconds. Keys may be ATTACKER-CHOSEN strings (client_ids, URLs), so: (1) the + * key space is LRU-bounded — at `maxKeys` the least-recently-used bucket is + * evicted (an evicted bucket forgets that key's spend history, acceptable + * because eviction requires `maxKeys` distinct keys inside one window and the + * global concurrency caps still bound aggregate work); and (2) the key is + * stored as a fixed-size SHA-256 fingerprint, so a flood of long distinct keys + * bounds map memory to `maxKeys` × a constant, not × the raw key length. + */ + +import { createHash } from 'node:crypto'; + +type BucketState = { tokens: number; lastRefill: number }; + +export type RateLimitResult = { allowed: true } | { allowed: false; retryAfterSeconds: number }; + +export type RateLimiter = { + tryTake(key: string): RateLimitResult; + /** Drop all bucket state (for testing). @internal */ + _reset(): void; +}; + +const DEFAULT_MAX_KEYS = 10_000; + +// Cap the advertised Retry-After. A tiny configured rate (e.g. 0.001/min) +// would otherwise yield an absurd multi-year value; 2,147,483 s (≈24.8 days, +// int32-max milliseconds expressed as whole seconds) is a sane ceiling for any +// client parsing the header. +const MAX_RETRY_AFTER_SECONDS = 2_147_483; + +export function createRateLimiter(options: { + /** Bucket capacity (maximum burst). */ + capacity: number; + /** Continuous refill rate, tokens per minute. */ + refillPerMinute: number; + /** LRU bound on distinct keys tracked. Default 10 000. */ + maxKeys?: number; + /** Clock override (for testing). Default Date.now. */ + now?: () => number; +}): RateLimiter { + // A token bucket takes 1 token per request, so a burst ceiling below 1 could + // never admit anyone — clamp it up. The refill rate is left untouched (a + // sub-1/min limit still means "one request, then one per 60/rate seconds"), + // except that a non-finite or non-positive rate — which would make the + // bucket never refill and the retry-after math divide by zero/negative — + // falls back to the (already ≥1) capacity so the limiter stays well-defined. + const capacity = Math.max(1, options.capacity); + const refillPerMinute = + Number.isFinite(options.refillPerMinute) && options.refillPerMinute > 0 ? options.refillPerMinute : capacity; + const maxKeys = options.maxKeys ?? DEFAULT_MAX_KEYS; + const now = options.now ?? Date.now; + const buckets = new Map(); + + return { + tryTake(key: string): RateLimitResult { + // Fixed-size fingerprint: the raw key may be an attacker-chosen URL of + // arbitrary length, and it is retained in the map until evicted. + const mapKey = createHash('sha256').update(key).digest('base64url'); + const at = now(); + let bucket = buckets.get(mapKey); + if (bucket) { + const elapsedMs = at - bucket.lastRefill; + if (elapsedMs > 0) { + bucket.tokens = Math.min(capacity, bucket.tokens + (elapsedMs / 60_000) * refillPerMinute); + bucket.lastRefill = at; + } + // Delete now; the set below re-inserts so Map order tracks recency (LRU). + buckets.delete(mapKey); + } else { + bucket = { tokens: capacity, lastRefill: at }; + if (buckets.size >= maxKeys) { + const oldest = buckets.keys().next().value; + if (oldest !== undefined) buckets.delete(oldest); + } + } + let result: RateLimitResult; + if (bucket.tokens >= 1) { + bucket.tokens -= 1; + result = { allowed: true }; + } else { + const deficit = 1 - bucket.tokens; + const retryAfterSeconds = Math.min(MAX_RETRY_AFTER_SECONDS, Math.ceil((deficit * 60) / refillPerMinute)); + result = { allowed: false, retryAfterSeconds }; + } + buckets.set(mapKey, bucket); + return result; + }, + _reset(): void { + buckets.clear(); + }, + }; +} diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index 7b00eca..c60125e 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -16,9 +16,10 @@ import type { Logger, MCPClientRecord, MCPConfig, Request } from '../../types.ts import { emitMCPAuditEvent } from './audit.ts'; import { MCPAssertionJtiStore } from './assertionJtiStore.ts'; import { MCPAuthCodeStore } from './authCodeStore.ts'; -import { CimdClientError, resolveClient } from './cimd.ts'; +import { CimdClientError, MAX_CLIENT_ID_LENGTH, resolveClient } from './cimd.ts'; import { CLIENT_ASSERTION_TYPE_JWT_BEARER, verifyClientAssertion } from './clientAssertion.ts'; import { MCPKeyStore } from './keyStore.ts'; +import { createRateLimiter, type RateLimiter } from './rateLimit.ts'; import { hashRefreshToken, makeRefreshToken, @@ -57,6 +58,20 @@ function errorResponse(status: number, error: string, description?: string): Tok }; } +/** + * Map a CimdClientError to a token-endpoint response. A throttle rejection + * carries `statusCode` (429) and, for the fetch rate limit, `retryAfterSeconds` + * — surface those (with a `Retry-After` header) instead of the default 401, so + * a rate-limited client backs off rather than treating it as an auth failure. + */ +function cimdErrorResponse(err: CimdClientError): TokenResponse { + const response = errorResponse(err.statusCode ?? 401, err.oauthError, err.message); + if (err.retryAfterSeconds !== undefined) { + response.headers = { ...response.headers, 'Retry-After': String(err.retryAfterSeconds) }; + } + return response; +} + function nowSeconds(): number { return Math.floor(Date.now() / 1000); } @@ -72,6 +87,44 @@ function coerceTtl(value: unknown, fallback: number): number { return Number.isFinite(n) && n > 0 ? n : fallback; } +// --- client_credentials issuance rate limiting (#163) --- + +const RATE_LIMIT_DEFAULT_PER_MINUTE = 30; + +/** + * Resolve `mcp.clientCredentials.rateLimit` to requests/minute or `false` + * (disabled). `false`/`0` (and their env-expanded string forms) disable the + * limiter explicitly; anything non-finite/non-positive falls back to the + * default rather than failing open — mirrors `coerceTtl`. + */ +function resolveRateLimit(value: unknown): number | false { + if (value === false || value === 0 || value === 'false' || value === '0') return false; + if (value === undefined || value === null) return RATE_LIMIT_DEFAULT_PER_MINUTE; + const n = typeof value === 'number' ? value : Number(value); + return Number.isFinite(n) && n > 0 ? n : RATE_LIMIT_DEFAULT_PER_MINUTE; +} + +// Per-node bucket keyed by client_id; memoized on the configured rate so a +// live config change rebuilds it (dropping state — acceptable, the limiter is +// defense-in-depth, not an accounting ledger). Per-node rationale: see +// rateLimit.ts module header. +let grantLimiter: RateLimiter | undefined; +let grantLimiterRate: number | undefined; + +function getGrantLimiter(ratePerMinute: number): RateLimiter { + if (!grantLimiter || grantLimiterRate !== ratePerMinute) { + grantLimiter = createRateLimiter({ capacity: ratePerMinute, refillPerMinute: ratePerMinute }); + grantLimiterRate = ratePerMinute; + } + return grantLimiter; +} + +/** Drop grant-limiter state (for testing). @internal */ +export function _resetGrantRateLimiter(): void { + grantLimiter = undefined; + grantLimiterRate = undefined; +} + /** Does the client's registered grant_types permit refresh tokens? Defaults to true when unspecified (DCR default includes refresh_token). */ function allowsRefresh(client: MCPClientRecord): boolean { return !client.grant_types || client.grant_types.includes('refresh_token'); @@ -132,7 +185,7 @@ async function authenticateClient( client = await resolveClient(clientId, mcpConfig, logger); } catch (err) { if (err instanceof CimdClientError) { - return { error: errorResponse(401, err.oauthError, err.message) }; + return { error: cimdErrorResponse(err) }; } logger?.error?.('MCP token: client lookup failed:', err instanceof Error ? err.message : String(err)); return { error: errorResponse(500, 'server_error', 'Client lookup failed') }; @@ -457,6 +510,13 @@ async function handleClientCredentialsGrant( if (!clientId) { return errorResponse(400, 'invalid_request', 'client_id is required'); } + // Cap the client_id length before it becomes a rate-limiter map key + // (attacker-chosen, retained up to maxKeys entries) — same defense-in-depth + // family as the repo's request-path and assertion-length caps. 2048 covers + // any legitimate CIMD URL or DCR id. + if (clientId.length > MAX_CLIENT_ID_LENGTH) { + return errorResponse(400, 'invalid_request', 'client_id exceeds the maximum length'); + } if (assertionType !== CLIENT_ASSERTION_TYPE_JWT_BEARER) { return errorResponse(400, 'invalid_request', `client_assertion_type must be ${CLIENT_ASSERTION_TYPE_JWT_BEARER}`); } @@ -480,7 +540,7 @@ async function handleClientCredentialsGrant( client = await resolveClient(clientId, mcpConfig, logger); } catch (err) { if (err instanceof CimdClientError) { - return errorResponse(401, err.oauthError, err.message); + return cimdErrorResponse(err); } logger?.error?.('MCP token: client lookup failed:', err instanceof Error ? err.message : String(err)); return errorResponse(500, 'server_error', 'Client lookup failed'); @@ -516,6 +576,23 @@ async function handleClientCredentialsGrant( return errorResponse(401, 'invalid_client', `client_assertion verification failed: ${result.reason}`); } + // Issuance rate limit (#163, #159 req 5): applied AFTER proof-of-possession, + // keyed by the now-verified client_id. Running it pre-auth would let any + // caller drain a real agent's quota by replaying the agent's PUBLIC CIMD + // client_id URL with a bogus assertion (429 the victim before its valid + // assertion is even checked). Pre-auth work is bounded elsewhere: CIMD + // fetches by the per-URL fetch limiter + resolution/DNS concurrency caps, + // signature verification by Node's request concurrency (no I/O, no jti burn). + const ratePerMinute = resolveRateLimit(mcpConfig.clientCredentials?.rateLimit); + if (ratePerMinute !== false) { + const limit = getGrantLimiter(ratePerMinute).tryTake(clientId); + if (!limit.allowed) { + const response = errorResponse(429, 'slow_down', 'Token issuance rate limit reached for this client'); + response.headers = { ...response.headers, 'Retry-After': String(limit.retryAfterSeconds) }; + return response; + } + } + // RFC 8707 resource binding: exact match against the canonical MCP // resource, fail closed — no prefix or wildcard comparisons (#159 req 3). // Checked BEFORE the jti is consumed: a recoverable request-param mistake diff --git a/src/types.ts b/src/types.ts index 27bed09..a7d22b6 100644 --- a/src/types.ts +++ b/src/types.ts @@ -125,6 +125,13 @@ export interface MCPClientCredentialsConfig { * agents re-mint on 401; no refresh token is ever issued. */ accessTokenTtl?: number; + /** + * Issuance rate limit in requests/minute per client_id (per node — see + * rateLimit.ts). Default: 30. `false` or `0` disables the limiter; + * non-finite/non-positive values fall back to the default. Over-limit + * requests get 429 + `error: "slow_down"` + `Retry-After`. + */ + rateLimit?: number | false; } /** diff --git a/test/lib/mcp/authorize.test.js b/test/lib/mcp/authorize.test.js index c577b23..797f50c 100644 --- a/test/lib/mcp/authorize.test.js +++ b/test/lib/mcp/authorize.test.js @@ -616,6 +616,24 @@ describe('handleAuthorize — CIMD interstitial', () => { resource: 'https://app.example.com/mcp', }; + it('surfaces 429 + Retry-After when the CIMD fetch rate limit trips', async () => { + // A doc whose client_id doesn't match the URL fails validation and is not + // cached, so each attempt re-fetches and spends a fetch-limiter token + // (fixed 10/min per URL). The 11th is throttled and must reach the client + // as a 429 slow_down with a Retry-After header, not a 400/401. + _setFetch(makeCimdFetch(makeCimdDoc('https://mcp-client.example.com/mismatch.json'))); + const { entries } = makeProviderRegistry('github'); + const target = makeTarget(CIMD_QUERY); + for (let i = 0; i < 10; i++) { + const r = await handleAuthorize(makeRequest(), target, { enabled: true }, entries); + assert.equal(r.status, 400, `attempt ${i + 1} fails validation (token consumed)`); + } + const limited = await handleAuthorize(makeRequest(), target, { enabled: true }, entries); + assert.equal(limited.status, 429); + assert.equal(limited.body.error, 'slow_down'); + assert.ok(limited.headers?.['Retry-After'], 'carries a Retry-After header'); + }); + it('returns 200 HTML interstitial page for a CIMD client (not 302)', async () => { _setFetch(makeCimdFetch(makeCimdDoc(CIMD_CLIENT_ID))); const { entries } = makeProviderRegistry('github'); diff --git a/test/lib/mcp/cimd.test.js b/test/lib/mcp/cimd.test.js index bbb0d75..78e8184 100644 --- a/test/lib/mcp/cimd.test.js +++ b/test/lib/mcp/cimd.test.js @@ -872,6 +872,88 @@ describe('resolveCimdClient — cache', () => { }); }); +describe('resolveCimdClient — per-URL fetch rate limit (#163)', () => { + beforeEach(() => _clearCimdCache()); + afterEach(() => { + _setDnsLookup(null); + _setFetch(null); + }); + + // Fails validation (client_id mismatch) → never cached → every attempt fetches. + const BAD_DOC = { client_id: 'https://elsewhere.example.com/x.json', client_name: 'x', redirect_uris: [] }; + + it('rejects the 11th fetch attempt for one URL within the window', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 10; i++) { + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => err instanceof CimdClientError && err.oauthError === 'invalid_client' + ); + } + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => { + // Throttle surfaces as slow_down + 429 + Retry-After, mirroring the + // issuance limiter — not the old auth-flavoured temporarily_unavailable. + assert.equal(err.oauthError, 'slow_down'); + assert.equal(err.statusCode, 429); + assert.ok(err.retryAfterSeconds >= 1, 'carries a Retry-After hint'); + assert.match(err.message, /rate limit reached/); + return true; + } + ); + }); + + it('is per-URL — one throttled URL does not affect another', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 10; i++) { + await resolveCimdClient(VALID_URL, undefined).catch(() => {}); + } + await assert.rejects(() => resolveCimdClient(VALID_URL, undefined), /rate limit reached/); + const OTHER_URL = 'https://other.example.com/client.json'; + _setFetch( + makeOkFetch({ client_id: OTHER_URL, client_name: 'Other', redirect_uris: ['https://other.example.com/cb'] }) + ); + const record = await resolveCimdClient(OTHER_URL, undefined); + assert.ok(record, 'a different client_id URL has its own bucket'); + }); + + it('cache hits do not consume fetch-attempt tokens', async () => { + _setDnsLookup(makeDnsOk()); + _setFetch(makeOkFetch(VALID_DOC)); + await resolveCimdClient(VALID_URL, undefined); + for (let i = 0; i < 30; i++) { + const record = await resolveCimdClient(VALID_URL, undefined); + assert.ok(record, 'cached resolutions are never rate-limited'); + } + }); + + it('deduped concurrent resolutions consume a single attempt', async () => { + _setDnsLookup(makeDnsOk()); + let release; + const gate = new Promise((resolve) => (release = resolve)); + _setFetch(async (url, init) => { + await gate; + return makeOkFetch(BAD_DOC)(url, init); + }); + const batch = Array.from({ length: 5 }, () => resolveCimdClient(VALID_URL, undefined).catch((err) => err)); + release(); + await Promise.all(batch); + // The 5-caller batch consumed exactly one attempt → 9 more pass the + // limiter (failing validation), and the 11th is rate-limited. + _setFetch(makeOkFetch(BAD_DOC)); + for (let i = 0; i < 9; i++) { + await assert.rejects( + () => resolveCimdClient(VALID_URL, undefined), + (err) => err instanceof CimdClientError && err.oauthError === 'invalid_client' + ); + } + await assert.rejects(() => resolveCimdClient(VALID_URL, undefined), /rate limit reached/); + }); +}); + describe('resolveClient — routing', () => { let originalDatabases; let storedClients; diff --git a/test/lib/mcp/rateLimit.test.js b/test/lib/mcp/rateLimit.test.js new file mode 100644 index 0000000..5d22a0f --- /dev/null +++ b/test/lib/mcp/rateLimit.test.js @@ -0,0 +1,130 @@ +/** + * Tests for the per-node token-bucket rate limiter (#163). + * The clock is injected — no test sleeps. + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { createRateLimiter } from '../../../dist/lib/mcp/rateLimit.js'; + +function makeClock(startMs = 0) { + let t = startMs; + return { now: () => t, advance: (ms) => (t += ms) }; +} + +describe('createRateLimiter', () => { + it('admits up to capacity, then blocks with seconds until one token refills', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 2, refillPerMinute: 60, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 1); // 60/min = one token per second + }); + + it('refills continuously with elapsed time', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 60, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, false); + clock.advance(500); // half a token — still blocked + assert.equal(limiter.tryTake('k').allowed, false); + clock.advance(500); // a full token has refilled + assert.equal(limiter.tryTake('k').allowed, true); + }); + + it('caps refill at capacity — idle time never accrues an unbounded burst', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 2, refillPerMinute: 60, now: clock.now }); + limiter.tryTake('k'); + clock.advance(3_600_000); // an hour idle + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, false, 'burst is bounded by capacity, not elapsed time'); + }); + + it('isolates keys — a drained key does not starve another', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, now: clock.now }); + assert.equal(limiter.tryTake('noisy').allowed, true); + assert.equal(limiter.tryTake('noisy').allowed, false); + assert.equal(limiter.tryTake('quiet').allowed, true); + }); + + it('LRU-bounds the key space — a unique-key flood evicts the oldest bucket', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, maxKeys: 2, now: clock.now }); + assert.equal(limiter.tryTake('a').allowed, true); + assert.equal(limiter.tryTake('a').allowed, false); // 'a' drained + limiter.tryTake('b'); + limiter.tryTake('c'); // map at maxKeys → evicts 'a' (least recently used) + assert.equal(limiter.tryTake('a').allowed, true, 'an evicted key returns with a fresh bucket'); + }); + + it('retryAfterSeconds reflects the deficit at slow refill rates', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 2, now: clock.now }); + limiter.tryTake('k'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 30); // 2/min = one token per 30 s + }); + + it('clamps a sub-1 capacity up to 1 so a slow rate still admits the first request', () => { + const clock = makeClock(); + // rate 0.5/min → capacity would be 0.5, below the 1 token a take needs; + // without the clamp this bucket could never admit anyone. + const limiter = createRateLimiter({ capacity: 0.5, refillPerMinute: 0.5, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true, 'first request admitted despite sub-1 rate'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 120); // 0.5/min = one token per 120 s + clock.advance(120_000); + assert.equal(limiter.tryTake('k').allowed, true, 'refills at the configured slow rate'); + }); + + it('bounds memory under a long, unique-key flood — keys are fingerprinted, LRU still evicts', () => { + const clock = makeClock(); + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1, maxKeys: 3, now: clock.now }); + // Keys of arbitrary length must not grow the map beyond maxKeys, and each + // distinct key still gets its own bucket (fingerprints don't collide). + const long = (n) => `https://attacker.example.com/${'x'.repeat(4000)}/${n}`; + for (let i = 0; i < 1000; i++) assert.equal(limiter.tryTake(long(i)).allowed, true); + // The three most-recent keys retain state; older ones were evicted, so a + // re-request of a recent drained key is still blocked (state survived). + assert.equal(limiter.tryTake(long(999)).allowed, false, 'recent key keeps its drained bucket'); + assert.equal(limiter.tryTake(long(0)).allowed, true, 'long-evicted key returns fresh'); + }); + + it('caps retryAfterSeconds at a sane upper bound for tiny rates', () => { + const clock = makeClock(); + // 1e-5/min would compute a ~6,000,000 s wait; capped to the int32-second max. + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 0.00001, now: clock.now }); + limiter.tryTake('k'); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.equal(blocked.retryAfterSeconds, 2_147_483); + }); + + it('stays well-defined when constructed with a non-positive refill rate', () => { + const clock = makeClock(); + // A 0 rate would divide-by-zero the retry-after math; the guard falls back + // to capacity so the limiter still admits the burst and yields a finite wait. + const limiter = createRateLimiter({ capacity: 3, refillPerMinute: 0, now: clock.now }); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + assert.equal(limiter.tryTake('k').allowed, true); + const blocked = limiter.tryTake('k'); + assert.equal(blocked.allowed, false); + assert.ok(Number.isFinite(blocked.retryAfterSeconds) && blocked.retryAfterSeconds >= 1); + }); + + it('_reset drops all bucket state', () => { + const limiter = createRateLimiter({ capacity: 1, refillPerMinute: 1 }); + limiter.tryTake('k'); + assert.equal(limiter.tryTake('k').allowed, false); + limiter._reset(); + assert.equal(limiter.tryTake('k').allowed, true); + }); +}); diff --git a/test/lib/mcp/token.test.js b/test/lib/mcp/token.test.js index 92f19b4..49d6961 100644 --- a/test/lib/mcp/token.test.js +++ b/test/lib/mcp/token.test.js @@ -7,7 +7,7 @@ import { describe, it, before, after, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { createHash, generateKeyPairSync, randomBytes, sign } from 'node:crypto'; -import { handleToken } from '../../../dist/lib/mcp/token.js'; +import { handleToken, _resetGrantRateLimiter } from '../../../dist/lib/mcp/token.js'; import { resetMCPAssertionJtisTableCache } from '../../../dist/lib/mcp/assertionJtiStore.js'; import { resetMCPAuthCodesTableCache } from '../../../dist/lib/mcp/authCodeStore.js'; import { _clearCimdCache, _setDnsLookup, _setFetch } from '../../../dist/lib/mcp/cimd.js'; @@ -711,6 +711,7 @@ describe('handleToken — client_credentials grant (#162)', () => { resetMCPKeysTableCache(); resetMCPAssertionJtisTableCache(); _clearCimdCache(); + _resetGrantRateLimiter(); hookEvents = []; clients = new Map(); @@ -916,4 +917,49 @@ describe('handleToken — client_credentials grant (#162)', () => { assert.equal(res.status, 400); assert.equal(res.body.error, 'unauthorized_client'); }); + + it('rate-limits issuance per client — 429 + slow_down + Retry-After (#163)', async () => { + const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 2 } }; + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + const blocked = await handleToken({ headers: {} }, grantBody(), limited); + assert.equal(blocked.status, 429); + assert.equal(blocked.body.error, 'slow_down'); + assert.ok(Number(blocked.headers['Retry-After']) >= 1, 'Retry-After carries seconds until a token refills'); + }); + + it('rejects an over-length client_id before it becomes a rate-limiter key', async () => { + const longId = 'https://agents.example.com/' + 'a'.repeat(2100) + '.json'; + const res = await handleToken({ headers: {} }, grantBody({ client_id: longId }), ccConfig); + assert.equal(res.status, 400); + assert.equal(res.body.error, 'invalid_request'); + assert.match(res.body.error_description, /client_id exceeds the maximum length/); + }); + + it('rateLimit: false disables the issuance limiter', async () => { + const unlimited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: false } }; + for (let i = 0; i < 5; i++) { + assert.equal((await handleToken({ headers: {} }, grantBody(), unlimited)).status, 200); + } + }); + + it('bogus assertions cannot drain a real client’s issuance quota (limiter is post-auth)', async () => { + // The limiter is keyed by the client_id but debited only AFTER + // proof-of-possession. An attacker who knows the victim’s public CIMD + // client_id but not its key can flood the endpoint; every request fails + // verification (401) and none touches the victim’s bucket. + const limited = { ...ccConfig, clientCredentials: { ...ccConfig.clientCredentials, rateLimit: 2 } }; + const rogue = generateKeyPairSync('ed25519'); + for (let i = 0; i < 5; i++) { + const forged = await handleToken( + { headers: {} }, + grantBody({ client_assertion: signAssertion({ key: rogue.privateKey }) }), + limited + ); + assert.equal(forged.status, 401, 'forged assertion rejected'); + } + // The genuine agent’s quota is untouched: two valid mints still succeed. + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + assert.equal((await handleToken({ headers: {} }, grantBody(), limited)).status, 200); + }); });