diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a031f3d..6e5ef6263 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id ### Added +- **Client ID Metadata Documents (`--enable-client-id-metadata-document`)**: accepts an HTTPS-URL `client_id` that resolves to a JSON document describing the client ([CIMD draft](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-client-id-metadata-document-00)), so a client with no pre-registration can authenticate. This is what makes the OAuth path to `/mcp` reachable: without it Claude Code refuses the server outright ("Incompatible auth server: does not support dynamic client registration"). The document is fetched through the SSRF-hardened client (one-shot DNS, dial pinned to the validated IP), its `client_id` must equal its own URL, `redirect_uri` is validated against the document's list, and responses are size-capped and cached with a clamped TTL. Advertised as `client_id_metadata_document_supported` only when enabled. Optional host allow-list via `--client-id-metadata-allowed-domains`. Off by default — it changes the authorization endpoint's trust model for every client, not just MCP. +- **Dynamic Client Registration (`--enable-dynamic-client-registration`)**: implements [RFC 7591](https://www.rfc-editor.org/rfc/rfc7591.html) at `POST /oauth/register` and advertises `registration_endpoint`, for MCP clients that cannot yet use CIMD — which today includes Claude Code, whose released version reads `client_id_metadata_document_supported` and still refuses without a registration endpoint. CIMD remains preferred: the MCP client priority order is pre-registered → CIMD → DCR, so enabling this cannot downgrade a CIMD-capable client. Registers PUBLIC clients only (`token_endpoint_auth_method` must be `none`; `client_credentials` is refused), limits `redirect_uris` to https or loopback http, requires S256 PKCE at `/authorize` and refuses implicit response types, shows the same consent screen CIMD clients get ([RFC 7591 §5](https://www.rfc-editor.org/rfc/rfc7591.html#section-5) asks for exactly that warning), and bounds abuse with the per-IP rate limiter plus a registry ceiling. RFC 7592 client management is not implemented. **Off by default** — it is an unauthenticated write endpoint, matching how Keycloak, Ory Hydra and Auth0 all ship it. +- **Consent screen for self-registered clients**: a CIMD client asserts its own identity — it chooses its `client_name`, and anyone who can host a JSON file can claim any name — so `/authorize` now shows a consent page for those clients before issuing a code. It leads with the **redirect host**, the only fact about such a client the server has verified, and warns explicitly when a client's redirect URIs are loopback-only (any local process can bind the same port and present the same document; the MCP spec requires both). Pre-registered and first-party clients are unaffected and keep today's silent approval. Consent is single-use, pinned to the session it was shown to, and a refusal redirects to the client with `access_denied` per RFC 6749 §4.1.2.1. + - **Remote MCP server (`--mcp-enabled`)**: Authorizer serves its MCP tool surface over Streamable HTTP at `POST /mcp`, acting as an OAuth 2.1 resource server for itself. Implements RFC 9728 protected resource metadata at `/.well-known/oauth-protected-resource/mcp`, answers an unauthenticated or expired credential with `401` + `WWW-Authenticate: Bearer resource_metadata="…"` (the header MCP clients follow to discover the authorization server, and the status they refresh on), and enforces RFC 8707 audience binding — a token is accepted only when its `aud` is this deployment's canonical `/mcp`, which is the audience every other Authorizer endpoint rejects. Each request carries its own bearer token, so one server serves every caller under their own identity. The surface runs on a dedicated in-process gRPC server that accepts no cookie, no admin secret and no admin operation, so no token can cross between MCP and GraphQL/gRPC/REST. **Requires `--url`**; startup refuses the combination without it, because the audience comparison must not take input from the caller. Off by default. - **RFC 8252 §7.3 loopback redirect URIs**: `redirect_uri` validation now ignores the port when both the registered and presented URIs are loopback (`127.0.0.1`, `[::1]`, `localhost`). Native apps bind an ephemeral port at run time and cannot register it in advance, so exact matching made loopback redirects unusable. Scheme, host, path and query must still match exactly, and non-loopback redirects are unchanged. - **Unified OAuth Client registry (machine & agent identity foundation)**: All clients (human, machine, agent) are registered in a single `authorizer_clients` table with a `kind` discriminator (`interactive` | `service_account`). Service accounts can use the `client_credentials` grant for machine-to-machine authentication, while agents can participate in delegation chains. Admin GraphQL/gRPC operations manage clients with secret generation (32-byte crypto/rand, bcrypt-12 at rest), scope-subset enforcement, and one-time secret reveal ([#648](https://github.com/authorizerdev/authorizer/pull/648)). @@ -100,7 +104,7 @@ Targets the 2.4.0 release. Significant additions include enterprise SSO (SAML Id ### Removed - **`authorizer_client_id_not_found_total`**: replaced by **`authorizer_client_id_header_missing_total`**, which matches the actual behavior (header omitted, request still allowed). Update dashboards and alerts accordingly. -- **OIDC Discovery — `registration_endpoint`**: previously pointed to the signup UI rather than an RFC 7591 dynamic client registration endpoint. It will return when RFC 7591 is implemented. +- **OIDC Discovery — `registration_endpoint`**: previously pointed to the signup UI rather than an RFC 7591 dynamic client registration endpoint. (It returns in 2.4.0, pointing at a real RFC 7591 endpoint and only when `--enable-dynamic-client-registration` is set.) ## [2.2.1-rc.0] - 2026-04-06 diff --git a/ROADMAP_V2.md b/ROADMAP_V2.md index 118d95232..affae7a55 100644 --- a/ROADMAP_V2.md +++ b/ROADMAP_V2.md @@ -291,10 +291,11 @@ - [ ] **Authorization Server Metadata (RFC 8414)** - `/.well-known/oauth-authorization-server` endpoint - Publishes supported grant types, scopes, response types, token endpoint auth methods -- [ ] **Dynamic Client Registration (RFC 7591)** +- [x] **Dynamic Client Registration (RFC 7591)** -- delivered in 2.4.0 behind `--enable-dynamic-client-registration` (off by default) - `POST /oauth/register` -- MCP clients register programmatically - - Returns `client_id` (and optionally `client_secret` for confidential clients) - - Registration access token for subsequent client management + - Returns `client_id` only: PUBLIC clients exclusively, so no `client_secret` is ever issued to an anonymous caller + - Registration access tokens (RFC 7592 client management) deliberately NOT implemented -- a self-registered client is disposable + - CIMD remains the preferred mechanism; this exists for clients that predate it - [ ] **Resource Indicators (RFC 8707)** - `resource` parameter in authorization and token requests - Token audience (`aud`) set to the target MCP server URL @@ -366,7 +367,7 @@ Human approval and safe third-party access. - *Unlocks:* human-in-the-loop, agents calling Google/Slack/etc. on a user's behalf. ### Wave 4 — Enterprise hardening -- [x] **MCP authorization** (OAuth 2.1 + RFC 9728 + RFC 8707) (4.1) — delivered in 2.4.0 as `--mcp-enabled`: Authorizer is both the authorization server and the resource server for its own MCP surface. Still open: RFC 7591 dynamic client registration / CIMD for zero-touch client onboarding (and the `/authorize` consent screen CIMD requires), plus **ID-JAG / Cross-App Access** for enterprise-managed MCP. +- [x] **MCP authorization** (OAuth 2.1 + RFC 9728 + RFC 8707) (4.1) — delivered in 2.4.0 as `--mcp-enabled`: Authorizer is both the authorization server and the resource server for its own MCP surface. Zero-touch client onboarding also landed in 2.4.0: CIMD (`--enable-client-id-metadata-document`, preferred) and RFC 7591 DCR (`--enable-dynamic-client-registration`, for clients that predate CIMD), both gated behind the `/authorize` consent screen. Still open: **ID-JAG / Cross-App Access** for enterprise-managed MCP. - [ ] **JIT / time-bound grants** (TTL tuples), **per-agent guardrails** (spend/rate limits), **consent management**. - *Unlocks:* enterprise-managed agent deployments at scale. @@ -558,7 +559,7 @@ Phase 5 (can partially parallelize with Phase 3-4) |---|---|---| | OAuth 2.1 (draft) | 4 | Modern OAuth baseline (PKCE mandatory, no implicit) | | RFC 7636 (PKCE) | Done | Proof Key for Code Exchange | -| RFC 7591 (DCR) | 4 | Dynamic Client Registration for MCP | +| RFC 7591 (DCR) | Done | Dynamic Client Registration for MCP (opt-in; CIMD preferred) | | RFC 8414 (AS Metadata) | 4 | Authorization Server discovery | | RFC 8693 (Token Exchange) | 5 | Agent delegation, cross-service tokens | | RFC 8707 (Resource Indicators) | 4 | Audience-restricted tokens for MCP | diff --git a/cmd/root.go b/cmd/root.go index b37350157..87a4ebed9 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -19,6 +19,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/authenticators" "github.com/authorizerdev/authorizer/internal/authenticators/webauthn" + "github.com/authorizerdev/authorizer/internal/clientmetadata" "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/email" @@ -143,6 +144,18 @@ func init() { // as the OAuth metadata clients discover it through, and mounting it on the // main router gives it the existing CORS, security-header, rate-limit and // logging middleware. + f.BoolVar(&rootArgs.config.EnableClientIDMetadataDocument, "enable-client-id-metadata-document", false, + "Accept HTTPS-URL client_ids that resolve to a Client ID Metadata Document (CIMD), so clients "+ + "with no prior relationship can authenticate — required for the OAuth path to the MCP surface. "+ + "Clients registered this way are self-asserted, so a consent screen is shown for them. Off by default") + f.StringSliceVar(&rootArgs.config.ClientIDMetadataAllowedDomains, "client-id-metadata-allowed-domains", nil, + "Restrict which hosts may serve a client metadata document (e.g. claude.ai). Empty accepts any HTTPS host") + f.BoolVar(&rootArgs.config.EnableDynamicClientRegistration, "enable-dynamic-client-registration", false, + "Serve the RFC 7591 dynamic client registration endpoint at POST /oauth/register, for MCP "+ + "clients that cannot use CIMD. This is an UNAUTHENTICATED write endpoint: prefer "+ + "--enable-client-id-metadata-document where the client supports it. Clients registered this "+ + "way are self-asserted, so a consent screen is shown for them. Off by default") + f.BoolVar(&rootArgs.config.MCPEnabled, "mcp-enabled", false, "Serve the MCP tool surface over HTTP at POST /mcp as an OAuth 2.1 resource server. "+ "Requires --url: tokens are accepted only when their audience equals /mcp, and that "+ @@ -701,20 +714,32 @@ func runRoot(c *cobra.Command, args []string) { log.Fatal().Err(err).Msg("failed to create service provider") } + // CIMD resolver. nil when disabled, which is what switches the feature off + // everywhere downstream — the authorize handler and the client-auth resolver + // both treat a nil provider as "URL client_ids are not a thing here". + var clientMetadataProvider *clientmetadata.Provider + if rootArgs.config.EnableClientIDMetadataDocument { + clientMetadataProvider = clientmetadata.New(&log, rootArgs.config.ClientIDMetadataAllowedDomains, + rootArgs.config.Env == constants.E2EEnv) + log.Info().Strs("allowed_domains", rootArgs.config.ClientIDMetadataAllowedDomains). + Msg("Client ID Metadata Documents enabled") + } + httpProvider, err := http_handlers.New(&rootArgs.config, &http_handlers.Dependencies{ - Log: &log, - AuditProvider: auditProvider, - AuthenticatorProvider: authenticatorProvider, - EmailProvider: emailProvider, - EventsProvider: eventsProvider, - MemoryStoreProvider: memoryStoreProvider, - SMSProvider: smsProvider, - StorageProvider: storageProvider, - TokenProvider: tokenProvider, - OAuthProvider: oauthProvider, - RateLimitProvider: rateLimitProvider, - ServiceProvider: serviceProvider, - AuthzEngine: authzEngine, + ClientMetadataProvider: clientMetadataProvider, + Log: &log, + AuditProvider: auditProvider, + AuthenticatorProvider: authenticatorProvider, + EmailProvider: emailProvider, + EventsProvider: eventsProvider, + MemoryStoreProvider: memoryStoreProvider, + SMSProvider: smsProvider, + StorageProvider: storageProvider, + TokenProvider: tokenProvider, + OAuthProvider: oauthProvider, + RateLimitProvider: rateLimitProvider, + ServiceProvider: serviceProvider, + AuthzEngine: authzEngine, }) if err != nil { log.Fatal().Err(err).Msg("failed to create http provider") diff --git a/e2e-playground/docker-compose.yml b/e2e-playground/docker-compose.yml index b7c0b5330..f75ff02f2 100644 --- a/e2e-playground/docker-compose.yml +++ b/e2e-playground/docker-compose.yml @@ -21,10 +21,19 @@ services: # presented at /mcp is checked against /mcp, so the server refuses # to start with --mcp-enabled and no --url. - "--mcp-enabled" + # Accept https client_ids that resolve to a Client ID Metadata Document, + # so a client with no pre-registration can authenticate. + - "--enable-client-id-metadata-document" + # RFC 7591 self-registration, for clients that predate CIMD. Both are on + # here so tests/dcr.spec.ts can prove they coexist: a client that supports + # CIMD selects it first (the MCP priority order is pre-registered → CIMD → + # DCR), so advertising the registration endpoint cannot downgrade one. + # Off by default in the product — it is an unauthenticated write endpoint. + - "--enable-dynamic-client-registration" - "--app-cookie-secure=false" - "--admin-cookie-secure=false" - "--app-cookie-same-site=lax" - - "--allowed-origins=http://localhost:8080,http://authorizer:8080" + - "--allowed-origins=http://localhost:8080,http://authorizer:8080,https://cimd-client:4300" - "--google-client-id=mock-client-id" - "--google-client-secret=mock-client-secret" - "--github-client-id=mock-client-id" @@ -64,12 +73,22 @@ services: # correctness. - "--rate-limit-rps=1000" - "--rate-limit-burst=1000" + environment: + # Go reads the system trust store from SSL_CERT_FILE. The bundle appends + # our e2e CA to the public roots (see mocks/tls-certs/generate.sh) rather + # than replacing them, so this adds trust for the CIMD mock without + # removing it for anything else. + SSL_CERT_FILE: /certs/bundle.crt + volumes: + - certs:/certs:ro depends_on: mock-oauth: { condition: service_started } mock-saml-idp: { condition: service_started } mailpit: { condition: service_started } sms-sink: { condition: service_started } webhook-sink: { condition: service_started } + tls-certs: { condition: service_completed_successfully } + cimd-client: { condition: service_started } healthcheck: test: ["CMD", "wget", "-qO-", "http://localhost:8080/healthz"] interval: 2s @@ -559,6 +578,24 @@ services: build: ./mocks/sms-sink ports: ["4100:4100"] + # One-shot: generates a CA and a leaf certificate into the shared `certs` + # volume, then exits. Everything that needs TLS waits on it completing. + tls-certs: + build: ./mocks/tls-certs + volumes: + - certs:/certs + + # Hosts a Client ID Metadata Document over HTTPS. A compose service rather + # than a fixture file because the AUTHORIZER SERVER fetches the client_id URL + # — that fetch is what CIMD is. + cimd-client: + build: ./mocks/cimd-client + depends_on: + tls-certs: { condition: service_completed_successfully } + volumes: + - certs:/certs + ports: ["4300:4300"] + webhook-sink: build: ./mocks/webhook-sink ports: ["4200:4200"] @@ -590,6 +627,7 @@ services: AUTHORIZER_REPLICA_A_BASE_URL: http://authorizer-replica-a:8080 AUTHORIZER_REPLICA_B_BASE_URL: http://authorizer-replica-b:8080 MOCK_OAUTH_BASE_URL: http://mock-oauth:4000 + CIMD_CLIENT_BASE_URL: https://cimd-client:4300 # https, not http: mock-saml-idp terminates TLS (see its server.ts) so # Authorizer's idp_sso_url validation (https-only, no test bypass) # accepts the stored SSO URL. Callers must ignore the self-signed cert @@ -686,3 +724,6 @@ services: mailpit: { condition: service_started } sms-sink: { condition: service_started } webhook-sink: { condition: service_started } + +volumes: + certs: diff --git a/e2e-playground/mocks/cimd-client/Dockerfile b/e2e-playground/mocks/cimd-client/Dockerfile new file mode 100644 index 000000000..7432ff577 --- /dev/null +++ b/e2e-playground/mocks/cimd-client/Dockerfile @@ -0,0 +1,7 @@ +FROM node:22-alpine +WORKDIR /app +COPY package.json ./ +RUN npm install +COPY server.ts tsconfig.json ./ +EXPOSE 4300 +CMD ["npm", "start"] diff --git a/e2e-playground/mocks/cimd-client/package.json b/e2e-playground/mocks/cimd-client/package.json new file mode 100644 index 000000000..7b9e9c388 --- /dev/null +++ b/e2e-playground/mocks/cimd-client/package.json @@ -0,0 +1,8 @@ +{ + "name": "cimd-client", + "private": true, + "version": "1.0.0", + "scripts": { "start": "ts-node server.ts" }, + "dependencies": { "express": "^4.21.0" }, + "devDependencies": { "ts-node": "^10.9.2", "typescript": "^5.6.0", "@types/express": "^4.17.21", "@types/node": "^22.7.0" } +} diff --git a/e2e-playground/mocks/cimd-client/server.ts b/e2e-playground/mocks/cimd-client/server.ts new file mode 100644 index 000000000..3aa105fac --- /dev/null +++ b/e2e-playground/mocks/cimd-client/server.ts @@ -0,0 +1,55 @@ +// Stands in for an MCP client that identifies itself with a Client ID Metadata +// Document instead of a pre-registered client_id. +// +// Serves HTTPS, and that is not incidental: the spec requires a CIMD client_id +// to use the https scheme, and the AUTHORIZER SERVER — not the test process — +// is what fetches it. So this needs a certificate the server trusts, which the +// tls-certs service generates into a shared volume. +// +// `client_id` MUST equal the URL it is served from. That equality is the spec's +// central requirement and the only thing stopping any host from serving a +// document claiming to be some other client. +import express from 'express'; +import https from 'node:https'; +import fs from 'node:fs'; + +const BASE = process.env.SELF_BASE_URL || 'https://cimd-client:4300'; +const app = express(); + +app.get('/client.json', (_req, res) => { + res.json({ + client_id: `${BASE}/client.json`, + client_name: 'E2E Playground Client', + client_uri: BASE, + redirect_uris: [`${BASE}/callback`], + grant_types: ['authorization_code'], + response_types: ['code'], + token_endpoint_auth_method: 'none', + }); +}); + +// A document whose client_id names a DIFFERENT URL — the impersonation case. +// Accepting this would let any host claim to be any client. +app.get('/mismatched.json', (_req, res) => { + res.json({ + client_id: `${BASE}/client.json`, + client_name: 'Impersonator', + redirect_uris: [`${BASE}/callback`], + }); +}); + +// Where the authorization code lands. Echoes the query so the test can read it. +app.get('/callback', (req, res) => { + // HTML, not JSON: this is a NAVIGATION target, and a browser handles a + // document far more predictably than an application/json body. + res.type('html').send(`callback
${JSON.stringify(req.query)}
`); +}); + +app.get('/healthz', (_req, res) => res.sendStatus(204)); + +https + .createServer( + { key: fs.readFileSync('/certs/server.key'), cert: fs.readFileSync('/certs/server.crt') }, + app, + ) + .listen(4300, () => console.log(`cimd-client listening on ${BASE}`)); diff --git a/e2e-playground/mocks/cimd-client/tsconfig.json b/e2e-playground/mocks/cimd-client/tsconfig.json new file mode 100644 index 000000000..82d61996e --- /dev/null +++ b/e2e-playground/mocks/cimd-client/tsconfig.json @@ -0,0 +1,12 @@ +{ + "compilerOptions": { + "target": "ES2022", + "module": "commonjs", + "moduleResolution": "node", + "strict": true, + "esModuleInterop": true, + "skipLibCheck": true, + "resolveJsonModule": true, + "types": ["node"] + } +} diff --git a/e2e-playground/mocks/tls-certs/Dockerfile b/e2e-playground/mocks/tls-certs/Dockerfile new file mode 100644 index 000000000..a66e3cce1 --- /dev/null +++ b/e2e-playground/mocks/tls-certs/Dockerfile @@ -0,0 +1,14 @@ +# One-shot certificate authority for the e2e stack. +# +# CIMD requires an https client_id and the AUTHORIZER SERVER is what fetches it, +# so the mock client host must serve TLS with a certificate the server trusts. +# This generates both halves into a shared volume: the leaf for cimd-client, and +# a CA bundle the server is pointed at. +# +# Generated at run time, never committed — a private key in the repository would +# be a real key regardless of what it is labelled. +FROM alpine:3.20 +RUN apk add --no-cache openssl ca-certificates +COPY generate.sh /generate.sh +RUN chmod +x /generate.sh +ENTRYPOINT ["/generate.sh"] diff --git a/e2e-playground/mocks/tls-certs/generate.sh b/e2e-playground/mocks/tls-certs/generate.sh new file mode 100755 index 000000000..6595dd03e --- /dev/null +++ b/e2e-playground/mocks/tls-certs/generate.sh @@ -0,0 +1,35 @@ +#!/bin/sh +# Generates a CA and a leaf certificate for the CIMD mock host. +# +# Idempotent: compose may start this more than once across a run, and +# regenerating would invalidate the bundle a running server already loaded. +set -eu +OUT=/certs +if [ -f "$OUT/bundle.crt" ] && [ -f "$OUT/server.key" ]; then + echo "certs already present, leaving them alone" + exit 0 +fi +mkdir -p "$OUT" + +openssl req -x509 -newkey rsa:2048 -sha256 -days 3650 -nodes \ + -keyout "$OUT/ca.key" -out "$OUT/ca.crt" \ + -subj "/CN=authorizer-e2e-local-ca" >/dev/null 2>&1 + +# SAN, not CN: Go's TLS stack ignores CN entirely, so a CN-only certificate +# would fail verification with a confusing "certificate is not valid for any +# names" rather than an obvious misconfiguration. +openssl req -newkey rsa:2048 -sha256 -nodes \ + -keyout "$OUT/server.key" -out "$OUT/server.csr" \ + -subj "/CN=cimd-client" >/dev/null 2>&1 +printf "subjectAltName=DNS:cimd-client,DNS:localhost\nextendedKeyUsage=serverAuth\n" > "$OUT/san.cnf" +openssl x509 -req -in "$OUT/server.csr" -CA "$OUT/ca.crt" -CAkey "$OUT/ca.key" \ + -CAcreateserial -out "$OUT/server.crt" -days 3650 -sha256 \ + -extfile "$OUT/san.cnf" >/dev/null 2>&1 + +# The bundle APPENDS to the public roots rather than replacing them: Go's +# SSL_CERT_FILE overrides the system pool wholesale, so shipping only our CA +# would silently break every other TLS dial the server makes. +cat /etc/ssl/certs/ca-certificates.crt "$OUT/ca.crt" > "$OUT/bundle.crt" + +chmod 644 "$OUT/bundle.crt" "$OUT/server.crt" "$OUT/server.key" +echo "generated CA + leaf for cimd-client" diff --git a/e2e-playground/tests/cimd.spec.ts b/e2e-playground/tests/cimd.spec.ts new file mode 100644 index 000000000..8a25100c7 --- /dev/null +++ b/e2e-playground/tests/cimd.spec.ts @@ -0,0 +1,132 @@ +// e2e-playground/tests/cimd.spec.ts +// +// Client ID Metadata Documents: a client identifies itself with an https URL +// pointing at a JSON document, instead of a client_id registered in advance. It +// is what lets a client with no prior relationship authenticate at all — +// without it Claude Code refuses the server outright ("Incompatible auth +// server: does not support dynamic client registration"). +// +// This is the ONLY test that drives the browser leg. The Go tests cover the +// resolver and the consent handler's rules; only a real browser proves a user +// is shown the consent page and that approving it yields a code. +// +// It needs real TLS: the spec requires an https client_id and the SERVER +// fetches it, so the document host must present a certificate the server +// trusts. The tls-certs service generates a CA into a shared volume and the +// server is pointed at it via SSL_CERT_FILE — so the certificate path is +// exercised, not bypassed. +import { test, expect, type Page } from '@playwright/test'; +import { GraphQLClient, gql } from 'graphql-request'; +import crypto from 'node:crypto'; + +const BASE_URL = process.env.AUTHORIZER_BASE_URL || 'http://localhost:8080'; +const CIMD_BASE = process.env.CIMD_CLIENT_BASE_URL || 'https://cimd-client:4300'; + +// The client_id IS this URL, and the document served there must claim the same +// value — the equality that stops any host claiming to be any client. +const CLIENT_ID = `${CIMD_BASE}/client.json`; +const REDIRECT_URI = `${CIMD_BASE}/callback`; + +const client = new GraphQLClient(`${BASE_URL}/graphql`, { headers: { Origin: BASE_URL } }); + +function authorizeURL(clientID: string, state: string) { + const u = new URL('/authorize', BASE_URL); + u.searchParams.set('response_type', 'code'); + u.searchParams.set('client_id', clientID); + u.searchParams.set('redirect_uri', REDIRECT_URI); + u.searchParams.set('scope', 'openid'); + u.searchParams.set('state', state); + u.searchParams.set('response_mode', 'query'); + const verifier = crypto.randomBytes(32).toString('base64url'); + u.searchParams.set('code_challenge', crypto.createHash('sha256').update(verifier).digest('base64url')); + u.searchParams.set('code_challenge_method', 'S256'); + return u.toString(); +} + +async function signupAndReach(page: Page, url: string) { + const email = `cimd-${crypto.randomUUID()}@example.com`; + const password = 'Str0ngPassw0rd!'; + await client.request( + gql`mutation ($params: SignUpRequest!) { signup(params: $params) { message } }`, + { params: { email, password, confirm_password: password } }, + ); + await page.goto(url); + await page.locator('#authorizer-login-email-or-phone-number').fill(email); + await page.locator('#authorizer-login-password').fill(password); + await page.locator('form[name="authorizer-login-form"] button[type="submit"]').click(); + // First login for a new user hits the optional MFA-setup offer. + await page.getByRole('button', { name: 'Skip for now' }).click({ timeout: 10_000 }).catch(() => {}); +} + +// Assert on the redirect the SERVER issues, captured as the browser attempts it, +// rather than on the browser landing successfully. +// +// The callback host serves TLS signed by the e2e CA, which Chromium does not +// trust — and it should not have to: what is under test is that Authorizer sends +// an authorization code to the registered redirect URI, which is fully +// determined by the moment the request is made. Waiting for the navigation to +// COMPLETE would additionally require the browser to trust our test CA, making +// the assertion depend on something irrelevant to the behaviour. +function awaitCallback(page: Page) { + return page.waitForRequest((req) => req.url().includes('/callback'), { timeout: 20_000 }); +} + +// The mock's certificate is signed by the e2e CA, which Chromium does not +// trust. The SERVER's trust is what this suite is about and is configured +// properly via SSL_CERT_FILE; the browser only has to survive the final +// redirect to the callback. +test.use({ + ignoreHTTPSErrors: true, + launchOptions: { args: ['--ignore-certificate-errors'] }, +}); + +// SCOPE — this file asserts only what a browser is uniquely good for: that the +// consent page renders, names the client, shows the redirect host, and is +// clickable. +// +// The security property — approving issues a code to the registered redirect, +// declining returns access_denied and no code — is asserted in Go, by +// TestCIMDConsentEndToEnd (internal/integration_tests/cimd_flow_test.go), which +// drives the same four requests over http.Client with a cookie jar. +// +// That split is deliberate. The consent page is plain HTML with no JavaScript, +// so a browser adds nothing to an assertion about status codes and Location +// headers — and trying to make it do so failed for a reason unrelated to the +// feature: the flow ends in a cross-origin redirect into an https host whose +// certificate is signed by a throwaway CA, and Chromium cancels that navigation +// (net::ERR_ABORTED, canceled=true, no response event) even with +// ignoreHTTPSErrors AND --ignore-certificate-errors. Measured, not assumed: +// a DIRECT navigation to the same host returns 200, so it is the redirect into +// it that Chromium refuses. Reading the Location in Go sidesteps the whole +// question. +test.describe('CIMD — self-registered clients', () => { + test('the authorization server advertises the capability', async ({ request }) => { + // Anthropic documents Claude selecting CIMD only when BOTH hold — the + // second because its CIMD client authenticates as a public client. A server + // advertising one without the other silently falls back to DCR, which this + // server deliberately does not implement. + const res = await request.get('/.well-known/oauth-authorization-server'); + expect(res.status()).toBe(200); + const doc = await res.json(); + expect(doc.client_id_metadata_document_supported).toBe(true); + expect(doc.token_endpoint_auth_methods_supported).toContain('none'); + }); + + test('the consent page names the client and shows the redirect host', async ({ page }) => { + await signupAndReach(page, authorizeURL(CLIENT_ID, crypto.randomUUID())); + + // A pre-registered client would have gone straight to the callback. The + // page appearing at all is the gate working. + await expect(page.getByRole('heading', { name: /E2E Playground Client/ })).toBeVisible(); + + // The redirect HOST is the only verified fact about a self-asserted client, + // so it must be presented as its own field. Targeted by class rather than + // text, because the host also appears inside the full client_id below and + // matching either would pass without proving it is shown on its own. + await expect(page.locator('.host')).toHaveText('cimd-client:4300'); + + // Both decisions must be offered; which one produces what is asserted in Go. + await expect(page.getByRole('button', { name: 'Allow access' })).toBeEnabled(); + await expect(page.getByRole('button', { name: 'Cancel' })).toBeEnabled(); + }); +}); diff --git a/e2e-playground/tests/dcr.spec.ts b/e2e-playground/tests/dcr.spec.ts new file mode 100644 index 000000000..3e107056f --- /dev/null +++ b/e2e-playground/tests/dcr.spec.ts @@ -0,0 +1,340 @@ +// e2e-playground/tests/dcr.spec.ts +// +// RFC 7591 dynamic client registration: a client with no prior relationship +// POSTs its own metadata and gets a client_id back, then runs a normal OAuth +// flow with it. CIMD is the preferred mechanism (see cimd.spec.ts) — this path +// exists for clients that predate it, which today includes Claude Code. +// +// Two properties can ONLY be proven here, not in the Go tests: +// +// 1. The registration POST survives the real middleware chain. It carries no +// Origin and no Referer, because it comes from a CLI rather than a page, +// and the CSRF middleware rejects exactly that shape unless the endpoint is +// exempted. Every Go test calls the handler directly and so never sees the +// middleware — which is how the equivalent bug reached the consent form. +// 2. A real user is shown the consent screen for a self-registered client, +// rendered by the real template after a real login, and approving it yields +// a code for the registered redirect. The Go tests assert the handler's +// rules against a fabricated request; only this proves the page a person +// actually sees carries the client's name, the redirect host, the warning +// and a working form. +// +// The final hop into the client's own callback is asserted from the redirect the +// server issues rather than by letting the browser follow it — see the note at +// the approval step for why Chromium refuses that hop in this topology. +// +// The callback is a loopback URI rather than a path on the authorizer, and that +// is forced rather than chosen: the server refuses to register a plain-http +// redirect_uri unless it is loopback (MCP requires redirect URIs to be localhost +// or https), so http://authorizer:8080/... — the same-origin trick mcp.spec.ts +// uses with a pre-registered client — is correctly rejected for a DCR client. +// Loopback is also what a real MCP client binds. +import { test, expect } from "@playwright/test"; +import { GraphQLClient, gql } from "graphql-request"; +import crypto from "node:crypto"; + +const BASE_URL = process.env.AUTHORIZER_BASE_URL || "http://localhost:8080"; +const MCP_RESOURCE = `${BASE_URL}/mcp`; + +const client = new GraphQLClient(`${BASE_URL}/graphql`, { + headers: { Origin: BASE_URL }, +}); + +const MCP_HEADERS = { + "Content-Type": "application/json", + Accept: "application/json, text/event-stream", +}; + +const INITIALIZE_RPC = { + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2025-06-18", + capabilities: {}, + clientInfo: { name: "e2e-dcr", version: "1.0" }, + }, +}; + +// The loopback callback this client registers. Nothing listens on it, and +// nothing needs to: the flow below asserts the redirect the SERVER issues rather +// than following it into the client's process (see the note at the approval +// step). A high, arbitrary port is deliberate — an MCP client binds an ephemeral +// one, which is precisely the case the server's port-agnostic loopback matching +// (RFC 8252 §7.3) exists for, and which no operator could allow-list in advance. +const redirectURI = "http://127.0.0.1:47821/callback"; + +async function register( + request: import("@playwright/test").APIRequestContext, + body: unknown, +) { + // No Origin, no Referer, no cookies — the shape a CLI sends. If the CSRF + // exemption regresses this returns 403 and every case below fails loudly. + return request.post("/oauth/register", { + headers: { "Content-Type": "application/json" }, + data: body, + }); +} + +test.describe("DCR — dynamically registered clients", () => { + test("the authorization server advertises the registration endpoint", async ({ + request, + }) => { + // This exact field is what a client looks for before it will attempt DCR; + // its absence is the "Incompatible auth server: does not support dynamic + // client registration" refusal that made the MCP surface unreachable. + const res = await request.get("/.well-known/oauth-authorization-server"); + expect(res.status()).toBe(200); + const doc = await res.json(); + expect(doc.registration_endpoint).toBe(`${BASE_URL}/oauth/register`); + // Both self-registration mechanisms are offered; a client that supports + // CIMD picks it first and never reaches the DCR path. + expect(doc.client_id_metadata_document_supported).toBe(true); + expect(doc.token_endpoint_auth_methods_supported).toContain("none"); + }); + + test("registration succeeds without an Origin header and yields a public client", async ({ + request, + }) => { + const res = await register(request, { + client_name: "E2E DCR Client", + redirect_uris: [redirectURI], + grant_types: ["authorization_code", "refresh_token"], + token_endpoint_auth_method: "none", + }); + expect(res.status(), await res.text()).toBe(201); + + const body = await res.json(); + expect(body.client_id).toBeTruthy(); + expect(body.token_endpoint_auth_method).toBe("none"); + expect(body.redirect_uris).toEqual([redirectURI]); + // No secret is issued: an anonymous caller must never be able to create a + // confidential client. + expect(body).not.toHaveProperty("client_secret"); + expect(body).not.toHaveProperty("client_secret_expires_at"); + }); + + test("a confidential registration is refused", async ({ request }) => { + const res = await register(request, { + client_name: "Wants A Secret", + redirect_uris: [redirectURI], + token_endpoint_auth_method: "client_secret_basic", + }); + expect(res.status()).toBe(400); + expect((await res.json()).error).toBe("invalid_client_metadata"); + }); + + test("a non-loopback http redirect is refused", async ({ request }) => { + // MCP: "All redirect URIs MUST be either localhost or use HTTPS." Without + // this every code issued to the client would cross the network in the clear. + const res = await register(request, { + client_name: "Cleartext", + redirect_uris: ["http://app.example.com/cb"], + }); + expect(res.status()).toBe(400); + expect((await res.json()).error).toBe("invalid_redirect_uri"); + }); + + test("a self-registered client is consented to, then reaches the MCP surface", async ({ + page, + request, + }) => { + const regRes = await register(request, { + client_name: "E2E DCR Browser Client", + redirect_uris: [redirectURI], + grant_types: ["authorization_code", "refresh_token"], + token_endpoint_auth_method: "none", + }); + expect(regRes.status(), await regRes.text()).toBe(201); + const clientId = (await regRes.json()).client_id as string; + + const email = `dcr-${crypto.randomUUID()}@example.com`; + const password = "Str0ngPassw0rd!"; + await client.request( + gql` + mutation ($params: SignUpRequest!) { + signup(params: $params) { + message + } + } + `, + { params: { email, password, confirm_password: password } }, + ); + + const codeVerifier = crypto.randomBytes(32).toString("base64url"); + const codeChallenge = crypto + .createHash("sha256") + .update(codeVerifier) + .digest("base64url"); + + const authorizeUrl = new URL("/authorize", BASE_URL); + authorizeUrl.searchParams.set("response_type", "code"); + authorizeUrl.searchParams.set("client_id", clientId); + authorizeUrl.searchParams.set("redirect_uri", redirectURI); + authorizeUrl.searchParams.set("scope", "openid offline_access"); + authorizeUrl.searchParams.set("state", crypto.randomUUID()); + authorizeUrl.searchParams.set("response_mode", "query"); + authorizeUrl.searchParams.set("code_challenge", codeChallenge); + authorizeUrl.searchParams.set("code_challenge_method", "S256"); + // RFC 8707: binds the issued token's `aud` to the MCP server. + authorizeUrl.searchParams.set("resource", MCP_RESOURCE); + + await page.goto(authorizeUrl.toString()); + await page.locator("#authorizer-login-email-or-phone-number").fill(email); + await page.locator("#authorizer-login-password").fill(password); + await page + .locator('form[name="authorizer-login-form"] button[type="submit"]') + .click(); + // First login for a new user hits the optional MFA-setup offer. + await page + .getByRole("button", { name: "Skip for now" }) + .click({ timeout: 10_000 }) + .catch(() => {}); + + // --- the consent screen ------------------------------------------------ + // RFC 7591 §5 warns that "a rogue client might use the name and logo of a + // legitimate client" and tells servers to warn users about dynamically + // registered clients. The name below was chosen by the caller at + // registration and verified by nobody, which is what the page must say. + await expect( + page.getByRole("heading", { name: /E2E DCR Browser Client/ }), + ).toBeVisible(); + // The redirect host is the only fact about this client the server verified. + await expect(page.locator(".host")).toHaveText(new URL(redirectURI).host); + await expect(page.getByText(/runs on your own computer/i)).toBeVisible(); + + // The form must be wired to submit the single-use id the server issued — + // the page carries nothing else, so a broken action, method or hidden field + // is the difference between consent working and silently doing nothing. + const form = page.locator('form[action="/authorize/consent"]'); + await expect(form).toHaveAttribute("method", /post/i); + await expect( + page.getByRole("button", { name: "Allow access" }), + ).toBeEnabled(); + const consentID = await form + .locator('input[name="consent_id"]') + .inputValue(); + expect(consentID).toBeTruthy(); + + // Approve through the PAGE's own request context, which shares this + // browser's cookies, and walk the redirects one hop at a time. + // + // The click itself cannot be used to reach the callback here: Chromium + // aborts a navigation when a redirect chain that began on a private-network + // origin (authorizer:8080 resolves to a private address) targets a loopback + // address — its Private Network Access rule. A direct navigation to the same + // loopback URL succeeds, and the server log shows POST /authorize/consent + // 302 → GET /authorize 302 exactly as intended, so the abort is a browser + // policy about redirect chains rather than anything this server does. A real + // MCP client starts the flow from localhost and never trips it. Neither + // page.route nor waitForResponse can observe past the abort, so the hops are + // driven explicitly instead of pretending the browser completed them. + const consentRes = await page.request.post("/authorize/consent", { + form: { consent_id: consentID, action: "approve" }, + maxRedirects: 0, + }); + expect(consentRes.status(), await consentRes.text()).toBe(302); + + const resumed = await page.request.get( + new URL(consentRes.headers()["location"], BASE_URL).toString(), + { maxRedirects: 0 }, + ); + expect(resumed.status(), await resumed.text()).toBe(302); + + // The code must go to the URI this client registered, and nowhere else. + const landed = new URL(resumed.headers()["location"]); + expect(landed.origin + landed.pathname).toBe(redirectURI); + const code = landed.searchParams.get("code")!; + expect(code).toBeTruthy(); + + // --- redeem it as a public client -------------------------------------- + // No client_secret anywhere: PKCE alone binds the code to the instance that + // started the flow. + const tokenRes = await request.post("/oauth/token", { + form: { + grant_type: "authorization_code", + code, + client_id: clientId, + redirect_uri: redirectURI, + code_verifier: codeVerifier, + resource: MCP_RESOURCE, + }, + }); + expect(tokenRes.status(), await tokenRes.text()).toBe(200); + const tokens = await tokenRes.json(); + expect(tokens.access_token).toBeTruthy(); + expect(tokens.refresh_token).toBeTruthy(); + + // --- the token reaches the tool surface -------------------------------- + const init = await request.post("/mcp", { + headers: { + ...MCP_HEADERS, + Authorization: `Bearer ${tokens.access_token}`, + }, + data: INITIALIZE_RPC, + }); + expect(init.status(), await init.text()).toBe(200); + const sessionId = init.headers()["mcp-session-id"]; + expect(sessionId).toBeTruthy(); + + const tools = await request.post("/mcp", { + headers: { + ...MCP_HEADERS, + Authorization: `Bearer ${tokens.access_token}`, + "Mcp-Session-Id": sessionId, + }, + data: { jsonrpc: "2.0", id: 2, method: "tools/list" }, + }); + expect(tools.status()).toBe(200); + expect(await tools.text()).toContain("profile"); + + // --- the refresh a long-lived connection depends on -------------------- + // Claude Code registers refresh_token, so this runs on every rotation. The + // resource binding must survive it, or connections die at the first refresh + // rather than at connect time. + const refreshRes = await request.post("/oauth/token", { + form: { + grant_type: "refresh_token", + refresh_token: tokens.refresh_token, + client_id: clientId, + }, + }); + expect(refreshRes.status(), await refreshRes.text()).toBe(200); + const refreshed = await refreshRes.json(); + expect(refreshed.access_token).toBeTruthy(); + expect(refreshed.access_token).not.toBe(tokens.access_token); + + const afterRefresh = await request.post("/mcp", { + headers: { + ...MCP_HEADERS, + Authorization: `Bearer ${refreshed.access_token}`, + "Mcp-Session-Id": sessionId, + }, + data: { jsonrpc: "2.0", id: 3, method: "tools/list" }, + }); + expect(afterRefresh.status(), await afterRefresh.text()).toBe(200); + }); + + test("PKCE is required of a self-registered client", async ({ request }) => { + // RFC 9700 §2.1.1 "Public clients MUST use PKCE"; OAuth 2.1 §4.1.1 has the + // authorization server MUST enforce it. Refused at /authorize so the user is + // never asked to log in and approve a request that could never complete. + const regRes = await register(request, { + client_name: "No PKCE Client", + redirect_uris: [redirectURI], + }); + expect(regRes.status()).toBe(201); + const clientId = (await regRes.json()).client_id as string; + + const u = new URL("/authorize", BASE_URL); + u.searchParams.set("response_type", "code"); + u.searchParams.set("client_id", clientId); + u.searchParams.set("redirect_uri", redirectURI); + u.searchParams.set("scope", "openid"); + u.searchParams.set("state", crypto.randomUUID()); + + const res = await request.get(u.toString(), { maxRedirects: 0 }); + expect(res.status()).toBe(400); + expect(await res.text()).toContain("code_challenge is required"); + }); +}); diff --git a/internal/clientmetadata/clientmetadata.go b/internal/clientmetadata/clientmetadata.go new file mode 100644 index 000000000..45b19136c --- /dev/null +++ b/internal/clientmetadata/clientmetadata.go @@ -0,0 +1,363 @@ +// Package clientmetadata resolves OAuth Client ID Metadata Documents (CIMD): +// client identifiers that are HTTPS URLs pointing at a JSON document describing +// the client, rather than opaque strings registered ahead of time. +// +// It exists because the MCP authorization spec (2025-11-25) makes CIMD the +// recommended registration mechanism — authorization servers SHOULD support it, +// and MAY support RFC 7591 dynamic client registration, which the spec keeps +// only "for backwards compatibility with earlier versions". Without one of the +// two, a client with no prior relationship to this server cannot authenticate at +// all: Claude Code, for instance, refuses an authorization server that offers +// neither rather than prompting for a client id. +// +// CIMD is the better half of that choice for a self-hosted product. DCR is an +// open, unauthenticated write endpoint that mints a row per registration — and +// per Anthropic's own guidance, clients register afresh on every connection, so +// every operator would accumulate client rows without bound. CIMD adds no +// endpoint, no rows and no schema change: the document lives on the client's own +// server and this package reads it. +// +// Specs: +// - https://datatracker.ietf.org/doc/html/draft-ietf-oauth-client-id-metadata-document-00 +// - https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization +package clientmetadata + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "sync" + "time" + + "github.com/rs/zerolog" + + "github.com/authorizerdev/authorizer/internal/validators" +) + +const ( + // maxDocumentBytes caps how much of a client's document is read. The body is + // attacker-controlled: without a cap, one authorization request could pull an + // unbounded stream into memory. + maxDocumentBytes = 64 << 10 + + // fetchTimeout bounds a single document fetch. It sits inside the + // authorization request, so a slow or hanging client server must not hold a + // request open — the caller gets a clean rejection instead. + fetchTimeout = 5 * time.Second + + // minCacheTTL / maxCacheTTL clamp whatever the client's Cache-Control says. + // The spec says to respect cache headers, but the header is written by the + // party being validated: a hostile max-age of 0 turns every authorization + // into a fetch, and a hostile max-age of a year pins a document that may + // later be corrected or revoked. + minCacheTTL = 1 * time.Minute + maxCacheTTL = 1 * time.Hour + + // maxCacheEntries bounds the cache so distinct client_id URLs cannot grow it + // without limit. On overflow the cache is cleared rather than evicted + // per-entry: this is a small cache of public documents, and a correctness- + // neutral flush is cheaper than maintaining LRU bookkeeping. + maxCacheEntries = 512 +) + +// Document is the subset of a Client ID Metadata Document this server uses. +type Document struct { + // ClientID MUST equal the URL the document was fetched from. + ClientID string `json:"client_id"` + // ClientName is displayed on the consent screen. Self-asserted, so it is + // shown as a claim and never as an identity — the redirect URI hostname is + // what the consent screen relies on. + ClientName string `json:"client_name"` + // RedirectURIs is the allow-list the presented redirect_uri is checked + // against. + RedirectURIs []string `json:"redirect_uris"` + ClientURI string `json:"client_uri"` + LogoURI string `json:"logo_uri"` + Scope string `json:"scope"` +} + +// IsMetadataClientID reports whether a client_id is in URL form and should be +// resolved as a metadata document rather than looked up in the registry. +// +// The two forms cannot collide: a registered client_id is an opaque string +// (a UUID in this codebase), and the spec requires a CIMD client_id to be an +// https URL WITH a path component. Requiring the path is not cosmetic — it is +// what keeps a bare origin from being mistaken for a client identifier. +// A registered client_id can never collide: the admin API does not accept one +// (CreateClientRequest has no client_id field — it is server-generated), so the +// only string an operator controls is --client-id. Callers pass that through +// reservedClientID so a deployment whose --client-id happens to look like a +// document URL keeps resolving from the registry rather than silently switching +// to a fetch. +func IsMetadataClientID(clientID string) bool { + if !strings.HasPrefix(clientID, "https://") { + return false + } + u, err := url.Parse(clientID) + if err != nil || u.Scheme != "https" || u.Host == "" { + return false + } + return u.Path != "" && u.Path != "/" +} + +// Provider resolves and caches client metadata documents. +type Provider struct { + log *zerolog.Logger + // allowPrivate switches the outbound fetch to + // validators.SafeHTTPClientAllowPrivate. Set ONLY when + // Config.Env == constants.E2EEnv (--env=e2e, never true in production). + // + // That function's doc comment asks for careful review before adding a third + // caller, so here it is. CIMD is defined by the SERVER fetching a URL the + // client supplies, so any test of it needs a document host the server can + // reach — and every host on a docker-compose network is a private address + // the guard refuses unconditionally. The alternative was to leave the + // browser flow untested, or to relax the https requirement under e2e, which + // would put an environment-dependent branch inside a security check and mean + // the thing under test is no longer the thing that runs in production. + // + // What is NOT relaxed: the scheme allow-list, the one-shot DNS resolution + // and dial pinning that defeat rebinding, and TLS verification. The e2e mock + // serves real HTTPS with a certificate from a CA generated into the stack, + // so the certificate path is exercised rather than bypassed — this widens + // which ADDRESSES are reachable, nothing else. + allowPrivate bool + // allowedDomains, when non-empty, restricts which hosts may serve a metadata + // document (the spec's optional domain trust policy). Empty accepts any + // HTTPS host, which is what a public MCP server wants. + allowedDomains map[string]struct{} + + // httpClient, when set, replaces the SSRF-hardened client built per request. + // It is the same seam fetchViaClient exposes and exists for the same reason: + // the guard refuses loopback by design, so a test cannot point this at an + // httptest server without it. Never set outside tests — New does not accept + // one, so production always builds the hardened client. + httpClient *http.Client + + mu sync.RWMutex + cache map[string]cacheEntry +} + +type cacheEntry struct { + doc *Document + expiresAt time.Time +} + +// New builds a Provider. allowedDomains is an optional host allow-list; an empty +// slice accepts any HTTPS host. +func New(log *zerolog.Logger, allowedDomains []string, allowPrivate bool) *Provider { + allowed := make(map[string]struct{}, len(allowedDomains)) + for _, d := range allowedDomains { + if d = strings.ToLower(strings.TrimSpace(d)); d != "" { + allowed[d] = struct{}{} + } + } + return &Provider{log: log, allowedDomains: allowed, allowPrivate: allowPrivate, cache: map[string]cacheEntry{}} +} + +// Resolve fetches and validates the metadata document named by clientID. +// +// Every failure returns an error rather than a partial document: a client whose +// identity cannot be established must not reach the consent screen, where a +// half-validated `client_name` would be shown to a user as if it meant something. +func (p *Provider) Resolve(ctx context.Context, clientID string) (*Document, error) { + if !IsMetadataClientID(clientID) { + return nil, fmt.Errorf("client_id is not a metadata document URL") + } + u, err := url.Parse(clientID) + if err != nil { + return nil, fmt.Errorf("client_id is not a valid URL") + } + // A fragment would make two spellings of one identifier, and the document's + // own client_id could then never match both. + if u.Fragment != "" || u.User != nil { + return nil, fmt.Errorf("client_id must not contain a fragment or userinfo") + } + if len(p.allowedDomains) > 0 { + if _, ok := p.allowedDomains[strings.ToLower(u.Hostname())]; !ok { + return nil, fmt.Errorf("client_id host is not in the allowed domain list") + } + } + + if doc := p.cached(clientID); doc != nil { + return doc, nil + } + + doc, ttl, err := p.fetch(ctx, clientID) + if err != nil { + return nil, err + } + p.store(clientID, doc, ttl) + return doc, nil +} + +func (p *Provider) cached(clientID string) *Document { + p.mu.RLock() + defer p.mu.RUnlock() + e, ok := p.cache[clientID] + if !ok || time.Now().After(e.expiresAt) { + return nil + } + return e.doc +} + +func (p *Provider) store(clientID string, doc *Document, ttl time.Duration) { + p.mu.Lock() + defer p.mu.Unlock() + if len(p.cache) >= maxCacheEntries { + p.cache = map[string]cacheEntry{} + } + p.cache[clientID] = cacheEntry{doc: doc, expiresAt: time.Now().Add(ttl)} +} + +// fetch retrieves and validates the document. The URL is attacker-supplied, so +// the request goes through validators.SafeHTTPClient: it resolves the host once +// and pins the dial to the validated IP, so a DNS-rebinding TOCTOU cannot make +// this server reach a private address after the check passed. Without that, CIMD +// would hand any caller an SSRF primitive against everything the server can +// reach — the risk the spec's security considerations lead with. +func (p *Provider) fetch(ctx context.Context, clientID string) (*Document, time.Duration, error) { + if p.httpClient != nil { + return p.fetchViaClient(ctx, clientID, p.httpClient) + } + newClient := validators.SafeHTTPClient + if p.allowPrivate { + newClient = validators.SafeHTTPClientAllowPrivate + } + client, err := newClient(ctx, clientID, fetchTimeout) + if err != nil { + return nil, 0, fmt.Errorf("client_id URL is not fetchable: %w", err) + } + return p.fetchViaClient(ctx, clientID, client) +} + +// fetchViaClient performs the request and validates the document. +// +// Split from fetch so the document-validation rules can be tested against a real +// server without the SSRF-hardened dialer, which refuses httptest's loopback +// address by design. Production has exactly one caller — fetch — so the safe +// client is never bypassed outside tests. Do not add another. +func (p *Provider) fetchViaClient(ctx context.Context, clientID string, client *http.Client) (*Document, time.Duration, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, clientID, nil) + if err != nil { + return nil, 0, fmt.Errorf("client_id URL is not fetchable") + } + req.Header.Set("Accept", "application/json") + + resp, err := client.Do(req) + if err != nil { + return nil, 0, fmt.Errorf("could not fetch client metadata document") + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + return nil, 0, fmt.Errorf("client metadata document returned status %d", resp.StatusCode) + } + + // LimitReader with one extra byte so an oversized body is detected rather + // than silently truncated into something that might still parse. + body, err := io.ReadAll(io.LimitReader(resp.Body, maxDocumentBytes+1)) + if err != nil { + return nil, 0, fmt.Errorf("could not read client metadata document") + } + if len(body) > maxDocumentBytes { + return nil, 0, fmt.Errorf("client metadata document is too large") + } + + var doc Document + if err := json.Unmarshal(body, &doc); err != nil { + return nil, 0, fmt.Errorf("client metadata document is not valid JSON") + } + + // The spec's central requirement: the document must claim the identity it + // was found at. Without this any host could serve a document claiming to be + // any other client, and the URL would stop meaning anything. + if doc.ClientID != clientID { + return nil, 0, fmt.Errorf("client metadata document client_id does not match its URL") + } + if strings.TrimSpace(doc.ClientName) == "" { + return nil, 0, fmt.Errorf("client metadata document is missing client_name") + } + if len(doc.RedirectURIs) == 0 { + return nil, 0, fmt.Errorf("client metadata document is missing redirect_uris") + } + for _, r := range doc.RedirectURIs { + ru, err := url.Parse(r) + if err != nil || !ru.IsAbs() { + return nil, 0, fmt.Errorf("client metadata document has an invalid redirect_uri") + } + // OAuth 2.1 §1.5: redirect URIs are HTTPS or loopback. An http:// URI on + // a public host would send an authorization code over cleartext. + if ru.Scheme != "https" { + switch ru.Hostname() { + case "127.0.0.1", "::1", "localhost": + default: + return nil, 0, fmt.Errorf("client metadata document redirect_uri must be https or loopback") + } + } + // RFC 6749 §3.1.2 forbids a fragment on the redirection endpoint, and + // userinfo is the phishing shape ("https://evil.com@app.example.com/cb" + // reads as evil.com to a human skimming the consent screen). Rejected + // here as well as in redirectURIMatches so a document cannot register + // one at all — otherwise the consent page could display a host that is + // not where the code actually lands. + if ru.Fragment != "" || ru.User != nil { + return nil, 0, fmt.Errorf("client metadata document redirect_uri must not contain a fragment or userinfo") + } + } + + return &doc, cacheTTL(resp.Header.Get("Cache-Control")), nil +} + +// cacheTTL derives a cache lifetime from Cache-Control, clamped. The header is +// written by the party being validated, so it is a hint, not an instruction: an +// unclamped max-age of 0 makes every authorization request refetch, and a +// max-age of a year pins a document that may later be corrected or revoked. +func cacheTTL(header string) time.Duration { + ttl := minCacheTTL + for _, part := range strings.Split(header, ",") { + part = strings.ToLower(strings.TrimSpace(part)) + if v, ok := strings.CutPrefix(part, "max-age="); ok { + var secs int + if _, err := fmt.Sscanf(v, "%d", &secs); err == nil && secs > 0 { + ttl = time.Duration(secs) * time.Second + } + } + } + if ttl < minCacheTTL { + ttl = minCacheTTL + } + if ttl > maxCacheTTL { + ttl = maxCacheTTL + } + return ttl +} + +// SetHTTPClientForTest injects the client used to fetch metadata documents. +// +// Exported solely so integration tests can point the resolver at an +// httptest.NewTLSServer: CIMD requires an https client_id, and the SSRF guard +// refuses loopback, so there is otherwise no way to exercise the flow without +// either weakening the guard or standing up public infrastructure. +// +// It is not reachable from New, so no production path can call it. +func (p *Provider) SetHTTPClientForTest(c *http.Client) { p.httpClient = c } + +// IsMetadataClientIDFor is IsMetadataClientID with the deployment's own reserved +// client_id excluded. +// +// --client-id is free-form, so an operator could set it to something that parses +// as a document URL. Without this, that deployment's reserved client would be +// resolved by fetching a URL instead of from the registry — a silent change of +// identity source triggered by configuration. Cheap to exclude, and it makes the +// precedence explicit rather than accidental. +func IsMetadataClientIDFor(clientID, reservedClientID string) bool { + if clientID != "" && clientID == strings.TrimSpace(reservedClientID) { + return false + } + return IsMetadataClientID(clientID) +} diff --git a/internal/clientmetadata/clientmetadata_test.go b/internal/clientmetadata/clientmetadata_test.go new file mode 100644 index 000000000..9627c4026 --- /dev/null +++ b/internal/clientmetadata/clientmetadata_test.go @@ -0,0 +1,328 @@ +package clientmetadata + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testProvider(t *testing.T, allowed ...string) *Provider { + t.Helper() + log := zerolog.Nop() + return New(&log, allowed, false) +} + +func TestIsMetadataClientID(t *testing.T) { + // The discriminator between "look this up in the client registry" and "go + // fetch this URL". It has to be exact in both directions: too loose and an + // ordinary client_id triggers an outbound fetch; too strict and CIMD never + // engages. + cases := []struct { + clientID string + want bool + }{ + {"https://app.example.com/client.json", true}, + {"https://app.example.com/oauth/metadata", true}, + + {"", false}, + {"a4b66000-a396-44fe-b8bf-efe9806a911d", false}, // a real registered client_id + {"local-client", false}, + {"http://app.example.com/client.json", false}, // http is not https + {"https://app.example.com", false}, // no path component (spec MUST) + {"https://app.example.com/", false}, // bare root is not a path + {"ftp://app.example.com/client.json", false}, + {"https:///client.json", false}, // no host + } + for _, tc := range cases { + t.Run(tc.clientID, func(t *testing.T) { + assert.Equal(t, tc.want, IsMetadataClientID(tc.clientID)) + }) + } +} + +// The loopback-only predicate that drives the consent warning moved to +// internal/http_handlers with the consent screen itself, so that "is this +// loopback?" has one implementation shared with the RFC 8252 redirect matcher. +// Its table test moved with it: TestConsentClientIsLoopbackOnly. + +// docServer serves a metadata document whose client_id is its own URL, which is +// what a well-behaved client hosts. +func docServer(t *testing.T, body func(selfURL string) string, headers map[string]string) (*httptest.Server, *int32) { + t.Helper() + var hits int32 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + for k, v := range headers { + w.Header().Set(k, v) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body(r.Header.Get("X-Self-URL")))) + })) + t.Cleanup(srv.Close) + return srv, &hits +} + +// TestResolveRejectsUnfetchableAndPrivateHosts is the SSRF guard. +// +// The client_id is an arbitrary attacker-supplied URL that this server fetches, +// which the CIMD spec names as its primary risk: without a check, any caller +// could aim the authorization server at internal addresses it can reach and use +// the error/latency as an oracle. validators.SafeHTTPClient resolves once and +// pins the dial to the validated IP, so a hostile DNS record cannot rebind +// between validation and connection. +func TestResolveRejectsUnfetchableAndPrivateHosts(t *testing.T) { + p := testProvider(t) + ctx := context.Background() + + // Assert on the SPECIFIC rejection, not merely that an error occurred. + // + // An earlier version of this test only required an error, and passed even + // with the SSRF guard swapped for SafeHTTPClientAllowPrivate — because the + // request then failed TLS verification against httptest's self-signed cert + // instead. That is a different failure with different security properties, + // and against a real attacker-controlled host with a valid certificate there + // would have been no error at all. + const wantRejection = "private/internal networks are not allowed" + + // httptest binds loopback, which is exactly what must be refused. + srv, hits := docServer(t, func(string) string { return `{}` }, nil) + _, err := p.Resolve(ctx, srv.URL+"/client.json") + require.Error(t, err) + assert.Contains(t, err.Error(), wantRejection, + "a loopback client_id URL must be refused by the SSRF guard, not by some later failure") + assert.Zero(t, *hits, "the refusal must happen before the request is made") + + for _, bad := range []string{ + "https://169.254.169.254/latest/meta-data", // cloud metadata service + "https://10.0.0.1/client.json", // RFC 1918 + "https://192.168.1.1/client.json", // RFC 1918 + "https://[::1]/client.json", // ipv6 loopback + "https://127.0.0.1/client.json", // ipv4 loopback + } { + _, err := p.Resolve(ctx, bad) + require.Error(t, err, "must be refused: %s", bad) + assert.Contains(t, err.Error(), wantRejection, + "%s must be refused by the SSRF guard specifically", bad) + } +} + +func TestResolveValidation(t *testing.T) { + // These run against a document server the provider will refuse to dial + // (loopback), so they assert the checks that happen BEFORE any network call. + p := testProvider(t) + ctx := context.Background() + + t.Run("a non-URL client_id is not a metadata document", func(t *testing.T) { + _, err := p.Resolve(ctx, "some-registered-client") + require.Error(t, err) + assert.Contains(t, err.Error(), "not a metadata document URL") + }) + + t.Run("a fragment is rejected", func(t *testing.T) { + // Two spellings of one identifier: the document's own client_id could + // not match both, so the identity check would be ambiguous. + _, err := p.Resolve(ctx, "https://app.example.com/client.json#x") + require.Error(t, err) + assert.Contains(t, err.Error(), "fragment") + }) + + t.Run("userinfo is rejected", func(t *testing.T) { + _, err := p.Resolve(ctx, "https://evil.com@app.example.com/client.json") + require.Error(t, err) + }) + + t.Run("a host outside the allow-list is rejected", func(t *testing.T) { + restricted := testProvider(t, "trusted.example.com") + _, err := restricted.Resolve(ctx, "https://other.example.com/client.json") + require.Error(t, err) + assert.Contains(t, err.Error(), "allowed domain list") + }) +} + +func TestCacheTTLIsClamped(t *testing.T) { + // Cache-Control is written by the party being validated, so it is a hint. + // Unclamped, max-age=0 makes every authorization request refetch (a DoS + // amplifier pointed at this server), and max-age=1yr pins a document that + // may later be corrected or revoked. + cases := []struct { + header string + want time.Duration + }{ + {"", minCacheTTL}, + {"max-age=0", minCacheTTL}, + {"max-age=1", minCacheTTL}, + {"no-store", minCacheTTL}, + {"max-age=31536000", maxCacheTTL}, + {"public, max-age=600", 600 * time.Second}, + {"MAX-AGE=600", 600 * time.Second}, + } + for _, tc := range cases { + t.Run(tc.header, func(t *testing.T) { + assert.Equal(t, tc.want, cacheTTL(tc.header)) + }) + } +} + +func TestCacheIsBounded(t *testing.T) { + // Distinct client_id URLs are attacker-controlled, so an unbounded cache is + // a memory-growth primitive. + p := testProvider(t) + for i := 0; i < maxCacheEntries+10; i++ { + p.store(fmt.Sprintf("https://app.example.com/%d.json", i), &Document{}, time.Hour) + } + p.mu.RLock() + defer p.mu.RUnlock() + assert.LessOrEqual(t, len(p.cache), maxCacheEntries) +} + +func TestCacheIsUsed(t *testing.T) { + p := testProvider(t) + const id = "https://app.example.com/client.json" + p.store(id, &Document{ClientID: id, ClientName: "cached"}, time.Hour) + + // Resolving must not attempt a fetch — this host does not exist, so a cache + // miss would surface as a DNS error rather than a document. + doc, err := p.Resolve(context.Background(), id) + require.NoError(t, err) + assert.Equal(t, "cached", doc.ClientName) + + t.Run("an expired entry is not served", func(t *testing.T) { + p.store(id, &Document{ClientID: id, ClientName: "stale"}, -time.Second) + assert.Nil(t, p.cached(id)) + }) +} + +// TestFetchValidatesDocumentContent covers the checks applied to a document that +// was successfully retrieved. It drives fetch() through a real TLS server via a +// provider whose SSRF guard is bypassed for the test only — the guard itself is +// covered by TestResolveRejectsUnfetchableAndPrivateHosts. +func TestFetchValidatesDocumentContent(t *testing.T) { + cases := []struct { + name string + body string + wantErr string + }{ + { + name: "client_id must match the URL it was fetched from", + body: `{"client_id":"https://someone-else.example.com/c.json","client_name":"n","redirect_uris":["https://a/cb"]}`, + wantErr: "does not match its URL", + }, + { + name: "client_name is required", + body: `{"client_id":"%[1]s","redirect_uris":["https://a/cb"]}`, + wantErr: "missing client_name", + }, + { + name: "redirect_uris is required", + body: `{"client_id":"%[1]s","client_name":"n"}`, + wantErr: "missing redirect_uris", + }, + { + name: "a plaintext non-loopback redirect_uri is rejected", + body: `{"client_id":"%[1]s","client_name":"n","redirect_uris":["http://app.example.com/cb"]}`, + wantErr: "https or loopback", + }, + { + name: "malformed JSON is rejected", + body: `not json`, + wantErr: "not valid JSON", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var selfURL string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + body := tc.body + if strings.Contains(body, "%[1]s") { + body = fmt.Sprintf(body, selfURL) + } + _, _ = w.Write([]byte(body)) + })) + defer srv.Close() + selfURL = srv.URL + "/client.json" + + p := testProvider(t) + // fetchViaClient exercises the validation independently of the + // SSRF-hardened dialer, which refuses httptest's loopback address. + _, _, err := p.fetchViaClient(context.Background(), selfURL, srv.Client()) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + }) + } + + t.Run("a valid document is accepted", func(t *testing.T) { + var selfURL string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"client_id":%q,"client_name":"Example Client","redirect_uris":["http://127.0.0.1/callback"]}`, selfURL) + })) + defer srv.Close() + selfURL = srv.URL + "/client.json" + + doc, ttl, err := testProvider(t).fetchViaClient(context.Background(), selfURL, srv.Client()) + require.NoError(t, err) + assert.Equal(t, "Example Client", doc.ClientName) + // The redirect list is what the consent screen's loopback warning is + // computed from (see TestConsentClientIsLoopbackOnly); assert it survives + // the fetch intact rather than re-testing the predicate here. + assert.Equal(t, []string{"http://127.0.0.1/callback"}, doc.RedirectURIs) + assert.Equal(t, minCacheTTL, ttl) + }) + + t.Run("an oversized document is rejected rather than truncated", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"padding":"` + strings.Repeat("a", maxDocumentBytes+100) + `"}`)) + })) + defer srv.Close() + + _, _, err := testProvider(t).fetchViaClient(context.Background(), srv.URL+"/c.json", srv.Client()) + require.Error(t, err) + assert.Contains(t, err.Error(), "too large") + }) + + t.Run("a non-200 response is rejected", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + _, _, err := testProvider(t).fetchViaClient(context.Background(), srv.URL+"/c.json", srv.Client()) + require.Error(t, err) + assert.Contains(t, err.Error(), "status 404") + }) +} + +// TestIsMetadataClientIDForExcludesTheReservedClient guards against a +// configuration-triggered change of identity source. +// +// --client-id is free-form. If an operator set it to something that parses as a +// document URL, the reserved client would stop being resolved from the registry +// and start being resolved by FETCHING that URL — silently, and with whoever +// controls that URL then describing the deployment's own primary client. +// +// No attacker path exists (the admin API does not accept a client_id; it is +// server-generated), which is exactly why this is worth a cheap guard rather +// than an argument about likelihood. +func TestIsMetadataClientIDForExcludesTheReservedClient(t *testing.T) { + const url = "https://app.example.com/client.json" + + assert.True(t, IsMetadataClientIDFor(url, "some-normal-client-id"), + "an ordinary deployment must still resolve URL client_ids as documents") + + assert.False(t, IsMetadataClientIDFor(url, url), + "the deployment's own reserved client_id must never be resolved by fetching it") + assert.False(t, IsMetadataClientIDFor(url, " "+url+" "), + "whitespace in --client-id must not defeat the exclusion") + + assert.False(t, IsMetadataClientIDFor("normal-client", "normal-client"), + "a non-URL reserved client_id was never a document anyway") +} diff --git a/internal/config/config.go b/internal/config/config.go index 18b085140..3fb42b7a1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -67,6 +67,44 @@ type Config struct { GRPCTLSCert string GRPCTLSKey string + // EnableClientIDMetadataDocument accepts HTTPS-URL client_ids that resolve to + // a Client ID Metadata Document (CIMD), letting a client with no prior + // relationship to this server authenticate. It is what makes the OAuth path + // to /mcp usable by clients that will not otherwise register. + // + // Off by default because it changes the authorization endpoint's trust model + // for EVERY client, not just MCP: identity becomes self-asserted, which is + // why enabling it also turns on a consent screen for those clients. + EnableClientIDMetadataDocument bool + // ClientIDMetadataAllowedDomains optionally restricts which hosts may serve a + // metadata document (the spec's domain trust policy). Empty accepts any HTTPS + // host, which is what a public MCP server wants; an allow-list is for + // locked-down deployments. + ClientIDMetadataAllowedDomains []string + + // EnableDynamicClientRegistration serves the RFC 7591 registration endpoint + // at POST /oauth/register and advertises `registration_endpoint`. + // + // Off by default, and it should stay off wherever CIMD suffices. The MCP + // authorization spec (2025-11-25) has authorization servers SHOULD support + // Client ID Metadata Documents and MAY support DCR, which it keeps "for + // backwards compatibility with earlier versions". This flag exists because + // clients in the field have not caught up: they look for + // `registration_endpoint` and give up when it is absent, even where + // `client_id_metadata_document_supported` is advertised. + // + // Turning it on opens an UNAUTHENTICATED write endpoint — RFC 7591 §5 permits + // exactly that ("SHOULD allow registration requests with no authorization") + // but pairs it with rate limiting and with warning the user about + // dynamically registered clients. Both hold here: the endpoint sits behind + // the global per-IP limiter, the registry has a hard ceiling, and a + // self-registered client always goes through the consent screen. + // + // Enabling it does NOT downgrade CIMD-capable clients: the spec's client + // priority order is pre-registered → CIMD → DCR, so DCR only catches clients + // that cannot do CIMD. + EnableDynamicClientRegistration bool + // MCPEnabled serves Authorizer's MCP tool surface over HTTP at POST /mcp on // the main listener, as an OAuth 2.1 resource server. Off by default: it is a // new internet-facing authenticated surface, and an auth server should not diff --git a/internal/constants/client_registry.go b/internal/constants/client_registry.go index 031ca9225..3fe557ec7 100644 --- a/internal/constants/client_registry.go +++ b/internal/constants/client_registry.go @@ -9,8 +9,24 @@ const ( // ClientKindServiceAccount is a machine/workload client using the // client_credentials grant or workload identity federation. ClientKindServiceAccount = "service_account" + + // ClientKindDynamic is an interactive client that registered ITSELF through + // the RFC 7591 endpoint, with no operator involvement. Behaviourally it is an + // interactive client; the separate kind exists because its identity is + // self-asserted, which is what drives the consent screen and the mandatory + // PKCE check. A distinct Kind value rather than a new column keeps this off + // the schema-change path across all six storage backends — every existing + // Kind test is a positive `== ClientKindServiceAccount` comparison, so a new + // value cannot silently widen an existing grant. + ClientKindDynamic = "dynamic" ) +// IsSelfRegistered reports whether a client asserted its own identity rather +// than being vouched for by an operator. Such clients get the consent screen and +// are required to use PKCE. CIMD clients are the other self-asserted kind; they +// never reach the registry, so they are detected by their URL client_id instead. +func IsSelfRegistered(kind string) bool { return kind == ClientKindDynamic } + // Client.TokenEndpointAuthMethod values (RFC 7591 §2 / OIDC Core §9). const ( // TokenEndpointAuthMethodClientSecretBasic sends client_id/client_secret in diff --git a/internal/e2e/smoke_test.go b/internal/e2e/smoke_test.go index 905df4479..2e0d3d7a3 100644 --- a/internal/e2e/smoke_test.go +++ b/internal/e2e/smoke_test.go @@ -17,11 +17,15 @@ package e2e import ( "bufio" "context" + "crypto/sha256" + "encoding/base64" "encoding/json" "fmt" + "io" "net" "net/http" "net/http/cookiejar" + "net/url" "os" "os/exec" "path/filepath" @@ -52,14 +56,22 @@ const ( fgaModelDSL = "model\n schema 1.1\ntype user\ntype document\n relations\n define viewer: [user]" ) -// TestReleaseSmoke is the release gate: one scenario, four surfaces. +// TestReleaseSmoke is the release gate: one scenario across every public +// surface. // // 1. Build the binary and boot it (sqlite storage; FGA auto-derives onto the // same sqlite file so the MCP subprocess can share it later). // 2. Seed via GraphQL: admin login, FGA model + tuple, user signup. // 3. Assert the same check_permissions / list_permissions decision on // GraphQL, REST, and gRPC, plus REST fail-closed and validation paths. -// 4. Stop the server and drive the `authorizer mcp` stdio subcommand through +// 4. Run the OAuth 2.1 authorization-code + PKCE round trip — /authorize to +// /oauth/token to /userinfo, including code single-use. Everything else +// here authenticates with a token minted directly by signup, so without +// this the authorization endpoint, the code store, the PKCE comparison and +// the token endpoint could all break without a single failure. +// 5. Provision a user over inbound SCIM 2.0, the one surface authenticated by +// a per-org bearer token rather than a session or the admin secret. +// 6. Stop the server and drive the `authorizer mcp` stdio subcommand through // a real MCP handshake with the minted bearer token. func TestReleaseSmoke(t *testing.T) { bin := buildBinary(t) @@ -252,6 +264,176 @@ func TestReleaseSmoke(t *testing.T) { assert.NotEmpty(t, res.AdminMeta.Roles) }) + // --- Surface 6: the OAuth 2.1 authorization code + PKCE round trip ---- + // The flow every browser-based integration depends on, driven end to end + // against the booted binary: /authorize issues a code, /oauth/token + // exchanges it with the PKCE verifier, and the resulting access token is + // accepted at /userinfo. + // + // Worth a release gate of its own because everything else here authenticates + // with a token minted directly by signup — none of it would notice + // /authorize, the code store, the PKCE comparison or the token endpoint + // breaking. This is also the only place the flow runs with the real route + // table, real middleware (CSRF, CORS, rate limiting) and real cookies. + t.Run("oauth authorization code + PKCE", func(t *testing.T) { + verifier := "smoke-code-verifier-0123456789abcdefghijklmnop" + sum := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(sum[:]) + redirectURI := baseURL + "/smoke-callback" + + // Its OWN user and cookie jar, deliberately. /authorize rolls the + // session over on success, which invalidates the token minted at signup + // — so running this against the shared user would silently break every + // later subtest that still holds that token. It did exactly that to the + // MCP stdio case before this was split out. + oauthGQL := newGraphQLClient(t, baseURL) + const oauthEmail = "smoke-oauth@test.dev" + oauthSignup := oauthGQL.mutate(t, `mutation { signup(params:{email:"`+oauthEmail+`", password:"`+smokeUserPassword+`", confirm_password:"`+smokeUserPassword+`"}) { user { id } } }`) + oauthUserID := oauthSignup["signup"].(map[string]any)["user"].(map[string]any)["id"].(string) + require.NotEmpty(t, oauthUserID) + + // The session cookie from that signup is what makes /authorize issue a + // code without a login round trip; it rides on that client's jar. + q := url.Values{} + q.Set("response_type", "code") + q.Set("client_id", smokeClientID) + q.Set("redirect_uri", redirectURI) + q.Set("scope", "openid profile email") + q.Set("state", "smoke-state") + q.Set("response_mode", "query") + q.Set("code_challenge", challenge) + q.Set("code_challenge_method", "S256") + + req, err := http.NewRequest(http.MethodGet, baseURL+"/authorize?"+q.Encode(), nil) + require.NoError(t, err) + // Do NOT follow the redirect: its Location IS the result under test. + noRedirect := &http.Client{ + Jar: oauthGQL.client.Jar, + Timeout: 15 * time.Second, + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + resp, err := noRedirect.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + body, _ := io.ReadAll(resp.Body) + require.Equal(t, http.StatusFound, resp.StatusCode, "authorize must redirect with a code: %s", body) + + loc, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err) + code := loc.Query().Get("code") + require.NotEmpty(t, code, "authorize must mint an authorization code") + assert.Equal(t, "smoke-state", loc.Query().Get("state"), "state must round-trip unmodified") + + // Exchange. The smoke client is confidential, so it authenticates with + // its secret AND supplies the PKCE verifier. + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("redirect_uri", redirectURI) + form.Set("client_id", smokeClientID) + form.Set("client_secret", smokeClientSecret) + form.Set("code_verifier", verifier) + + tokenResp, err := http.Post(baseURL+"/oauth/token", + "application/x-www-form-urlencoded", strings.NewReader(form.Encode())) + require.NoError(t, err) + defer func() { _ = tokenResp.Body.Close() }() + tokenBody, _ := io.ReadAll(tokenResp.Body) + require.Equal(t, http.StatusOK, tokenResp.StatusCode, "token exchange failed: %s", tokenBody) + + var tokens struct { + AccessToken string `json:"access_token"` + IDToken string `json:"id_token"` + TokenType string `json:"token_type"` + } + require.NoError(t, json.Unmarshal(tokenBody, &tokens)) + require.NotEmpty(t, tokens.AccessToken) + require.NotEmpty(t, tokens.IDToken, "an openid scope must yield an id_token") + assert.Equal(t, "Bearer", tokens.TokenType) + + // A second exchange of the same code must fail: authorization codes are + // single-use (RFC 6749 §4.1.2), and a replay is the classic attack. + replay, err := http.Post(baseURL+"/oauth/token", + "application/x-www-form-urlencoded", strings.NewReader(form.Encode())) + require.NoError(t, err) + defer func() { _ = replay.Body.Close() }() + assert.NotEqual(t, http.StatusOK, replay.StatusCode, "an authorization code must not be redeemable twice") + + // The token works where a real client would use it. + uiReq, err := http.NewRequest(http.MethodGet, baseURL+"/userinfo", nil) + require.NoError(t, err) + uiReq.Header.Set("Authorization", "Bearer "+tokens.AccessToken) + ui, err := http.DefaultClient.Do(uiReq) + require.NoError(t, err) + defer func() { _ = ui.Body.Close() }() + uiBody, _ := io.ReadAll(ui.Body) + require.Equal(t, http.StatusOK, ui.StatusCode, "userinfo rejected the freshly issued token: %s", uiBody) + var claims map[string]any + require.NoError(t, json.Unmarshal(uiBody, &claims)) + assert.Equal(t, oauthUserID, claims["sub"], "userinfo must describe the user who authorized") + }) + + // --- Surface 7: inbound SCIM 2.0 ------------------------------------- + // Provisioning is how enterprise customers create users, and it is the one + // surface authenticated by a per-org bearer token rather than a session or + // the admin secret — a route-group or middleware change can break it + // without touching anything else in this file. + t.Run("scim provisioning", func(t *testing.T) { + org := gql.mutate(t, `mutation { _create_organization(params:{name:"smoke-org", display_name:"Smoke Org"}) { id } }`) + orgID := org["_create_organization"].(map[string]any)["id"].(string) + require.NotEmpty(t, orgID) + + created := gql.mutate(t, `mutation { _create_scim_endpoint(params:{org_id:"`+orgID+`"}) { token scim_endpoint { id enabled } } }`) + scimToken := created["_create_scim_endpoint"].(map[string]any)["token"].(string) + require.NotEmpty(t, scimToken, "the token is returned once at creation and never again") + + scimReq := func(t *testing.T, method, path, bearer, payload string) (int, []byte) { + t.Helper() + var rdr io.Reader + if payload != "" { + rdr = strings.NewReader(payload) + } + req, err := http.NewRequest(method, baseURL+path, rdr) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/scim+json") + if bearer != "" { + req.Header.Set("Authorization", "Bearer "+bearer) + } + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + b, _ := io.ReadAll(resp.Body) + return resp.StatusCode, b + } + + // Fail closed first: an unauthenticated call must never provision. + status, _ := scimReq(t, http.MethodGet, "/scim/v2/Users", "", "") + assert.Equal(t, http.StatusUnauthorized, status, "SCIM must refuse an unauthenticated caller") + + // Provision a user the way an IdP does. + scimEmail := "scim-smoke@test.dev" + payload := `{"schemas":["urn:ietf:params:scim:schemas:core:2.0:User"],` + + `"userName":"` + scimEmail + `","active":true,` + + `"emails":[{"value":"` + scimEmail + `","primary":true}]}` + status, body := scimReq(t, http.MethodPost, "/scim/v2/Users", scimToken, payload) + require.Equal(t, http.StatusCreated, status, "SCIM create failed: %s", body) + + var createdUser struct { + ID string `json:"id"` + UserName string `json:"userName"` + Active bool `json:"active"` + } + require.NoError(t, json.Unmarshal(body, &createdUser)) + require.NotEmpty(t, createdUser.ID) + assert.Equal(t, scimEmail, createdUser.UserName) + assert.True(t, createdUser.Active) + + // And read it back through the same surface. + status, body = scimReq(t, http.MethodGet, "/scim/v2/Users/"+createdUser.ID, scimToken, "") + require.Equal(t, http.StatusOK, status, "SCIM get failed: %s", body) + assert.Contains(t, string(body), scimEmail) + }) + // --- Surface 4: MCP over HTTP ---------------------------------------- // Runs against the REAL binary and the REAL route table, which is the only // place the --mcp-enabled route registration is actually exercised: every diff --git a/internal/http_handlers/app.go b/internal/http_handlers/app.go index a3c64b63d..ba6808ec1 100644 --- a/internal/http_handlers/app.go +++ b/internal/http_handlers/app.go @@ -7,7 +7,6 @@ import ( "github.com/gin-gonic/gin" "github.com/authorizerdev/authorizer/internal/parsers" - "github.com/authorizerdev/authorizer/internal/validators" ) // State is the struct that holds authorizer url and redirect url @@ -42,8 +41,32 @@ func (h *httpProvider) AppHandler() gin.HandlerFunc { if redirectURI == "" { redirectURI = hostname + "/app" } else { - // validate redirect url with allowed origins - if !validators.IsValidRedirectURI(redirectURI, h.Config.AllowedOrigins, hostname) { + // Validate against the CLIENT, not just the global allow-list. + // + // /authorize hands this page the redirect_uri it already accepted, + // so applying a different rule here rejects flows that were valid one + // redirect earlier. That is what happened while this checked + // AllowedOrigins alone: any client whose registered redirect_uri was + // not also an allowed origin got "invalid redirect url" on the login + // page and could never sign in. Every fixture had allow-listed its + // callback origin, so nothing caught it until a client used an + // EPHEMERAL loopback port — which no operator can allow-list in + // advance, and which is exactly what an MCP client binds. + // + // The check is not weakened: with no client_id it is still the + // allow-list, and a client with registered URIs is held to an exact + // match against them. See checkClientRedirectURI. + clientID := strings.TrimSpace(c.Query("client_id")) + check, err := h.checkClientRedirectURI(c.Request.Context(), clientID, redirectURI, hostname) + if err != nil { + // Could not CHECK the client (storage down, or a metadata + // document that would not resolve) — never fall back to the + // laxer rule on the strength of a failure. + log.Warn().Err(err).Str("client_id", clientID).Msg("could not verify redirect url against the client") + c.JSON(400, gin.H{"error": "invalid redirect url"}) + return + } + if !check.Valid { log.Debug().Msg("Invalid redirect url") c.JSON(400, gin.H{"error": "invalid redirect url"}) return diff --git a/internal/http_handlers/app_redirect_uri_test.go b/internal/http_handlers/app_redirect_uri_test.go new file mode 100644 index 000000000..383edc441 --- /dev/null +++ b/internal/http_handlers/app_redirect_uri_test.go @@ -0,0 +1,144 @@ +package http_handlers + +import ( + "encoding/json" + "html/template" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/constants" + inmemorystore "github.com/authorizerdev/authorizer/internal/memory_store/in_memory" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// newAppTestProvider builds an /app handler backed by the same client stub the +// authorize tests use, with a DELIBERATELY narrow AllowedOrigins so the +// allow-list and the client's registered URIs cannot be confused for each other. +func newAppTestProvider(t *testing.T, client *schemas.Client) *httpProvider { + t.Helper() + logger := zerolog.Nop() + cfg := &config.Config{ + AllowedOrigins: []string{"https://console.example.com"}, + EnableLoginPage: true, + } + ms, err := inmemorystore.NewInMemoryProvider(cfg, &inmemorystore.Dependencies{Log: &logger}) + require.NoError(t, err) + return &httpProvider{ + Config: cfg, + Dependencies: Dependencies{ + Log: &logger, + StorageProvider: &redirectURIClientStore{client: client}, + MemoryStoreProvider: ms, + }, + } +} + +func getApp(h *httpProvider, clientID, redirectURI string) *httptest.ResponseRecorder { + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + router := gin.New() + // The real template, so a success is a real render rather than a 200 from a + // stub. app.tmpl uses the `json` FuncMap the production router registers, and + // SetFuncMap must precede LoadHTMLFiles or parsing fails on the unknown + // function. + router.SetFuncMap(template.FuncMap{ + "json": func(v any) template.JS { + a, _ := json.Marshal(v) + return template.JS(strings.ReplaceAll(string(a), " 0 { - validRedirect = false - for _, r := range registered { - if redirectURIMatches(r, redirectURI) { - validRedirect = true - break - } - } - } + // Shared with /app, which re-renders this same redirect_uri on the + // login page. Both must apply the identical rule; when they did not, + // a client whose registered redirect was outside AllowedOrigins + // passed here and was refused there. + check, cErr := h.checkClientRedirectURI(gc.Request.Context(), clientID, redirectURI, hostname) + switch { + case errors.Is(cErr, errClientUnresolvable): + log.Debug().Err(cErr).Str("client_id", clientID).Msg("could not resolve client metadata document") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_client", + "error_description": "could not resolve the client metadata document for this client_id", + }) + return + case cErr != nil: + log.Warn().Err(cErr).Str("client_id", clientID). + Msg("client lookup failed; refusing rather than falling back to the origin allow-list") + gc.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "temporarily_unavailable", + "error_description": "could not verify the client; please retry", + }) + return } - if !validRedirect { + // A client that registered ITSELF through RFC 7591 is remembered so + // the consent gate and PKCE check further down do not repeat the + // lookup. Operator-created clients are left false: they were vouched + // for by a human who entered their redirect URIs, which is the whole + // basis for not interrupting their users. + selfRegistered = check.SelfRegistered + if !check.Valid { log.Debug().Msg("Invalid redirect URI") gc.JSON(http.StatusBadRequest, gin.H{ "error": "invalid_request", @@ -309,6 +328,76 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { } } + // PKCE is MANDATORY for a self-asserted client — one that registered + // itself via RFC 7591 or presented a Client ID Metadata Document. Both + // are public clients, and for public clients PKCE is not advisory: + // + // RFC 9700 §2.1.1: "Public clients MUST use PKCE". + // OAuth 2.1 §4.1.1: clients "MUST use code_challenge and code_verifier + // and authorization servers MUST enforce their use". + // MCP authorization (2025-11-25): clients MUST implement PKCE and MUST + // use S256 when technically capable. + // + // Enforced HERE rather than left to the token endpoint. Without a secret, + // PKCE is the only thing binding the code to the instance that started + // the flow, so a request that omits it is unauthenticatable from the + // start; the token endpoint's "code_verifier or client_secret" rule would + // eventually refuse it, but only after the user had already logged in and + // approved — and with an error naming client_secret, which a public + // client can never supply. Failing at /authorize keeps the refusal + // truthful and free of user interaction. + // + // Answered as 400 rather than redirected to the client, matching the + // neighbouring PKCE validations above and the MCP authorization spec's + // error table, which maps a malformed authorization request to 400. + if selfRegistered || (h.ClientMetadataProvider != nil && clientmetadata.IsMetadataClientIDFor(clientID, h.Config.ClientID)) { + // A self-asserted client may not use an implicit response type at + // all, so PKCE below is unconditional rather than carved out for + // flows that issue no code. + // + // OAuth 2.1 removes the implicit grant, and MCP mandates OAuth 2.1. + // Independently of that, implicit hands a bearer token straight to + // the redirect URI in a URL fragment with nothing binding it to the + // requester — for a client whose identity nobody verified, that is + // the worst available combination. Both registration paths declare + // response_types ["code"] anyway (the registration endpoint refuses + // anything else, and the CIMD spec's example does the same), so this + // enforces at /authorize what the client already said it would do. + if isImplicit { + metrics.RecordSecurityEvent("self_registered_client_implicit_rejected", "authorize_endpoint") + log.Debug().Str("client_id", clientID).Str("response_type", responseType). + Msg("rejected: self-registered client requested an implicit response type") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "unsupported_response_type", + "error_description": "this client may only use response_type=code", + }) + return + } + if codeChallenge == "" { + metrics.RecordSecurityEvent("self_registered_client_missing_pkce", "authorize_endpoint") + log.Debug().Str("client_id", clientID).Msg("rejected: self-registered client omitted code_challenge") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_request", + "error_description": "code_challenge is required for this client; use PKCE with code_challenge_method=S256", + }) + return + } + // S256 only, independent of OAuth21Strict. "plain" carries the + // verifier in the same authorization request as the challenge, so it + // protects nothing against an attacker who can read that request — + // which is the threat model for a client with no secret. + if codeChallengeMethod != "S256" { + metrics.RecordSecurityEvent("self_registered_client_weak_pkce", "authorize_endpoint") + log.Debug().Str("client_id", clientID).Str("method", codeChallengeMethod). + Msg("rejected: self-registered client used a non-S256 code_challenge_method") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_request", + "error_description": "code_challenge_method must be S256 for this client", + }) + return + } + } + // OAuth 2.1 strict mode: the implicit grant is removed. Reject EVERY // response type that delivers a bearer access token into the URL // fragment — not just "token" and "id_token token" but also the @@ -609,6 +698,103 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { return } + // Consent gate for self-asserted (CIMD) clients. Placed here, after the + // session is validated and the user resolved, for two reasons: the page + // names the account being granted, and an unauthenticated visitor must + // reach the login UI first rather than being asked to approve something + // on behalf of nobody. + // + // Skipped once ConsentHandler has replayed the request — it sets the + // marker only after its own store lookup and session check pass, so this + // cannot be short-circuited by anything the client sends. + isCIMDClient := h.ClientMetadataProvider != nil && clientmetadata.IsMetadataClientIDFor(clientID, h.Config.ClientID) + if isCIMDClient || selfRegistered { + // Consume the grant recorded by ConsentHandler. Single-use and keyed + // to (user, client), so a consent authorizes one authorization + // request — not every subsequent one until the store expires it. + // Keyed to THIS request's parameters and consumed atomically, so a + // grant authorizes the one request it was given for and cannot be + // redeemed twice by concurrent callers. + grantKey := consentGrantKey(user.ID, clientID, originalParams(gc).Encode()) + granted, gErr := h.MemoryStoreProvider.GetAndRemoveState(grantKey) + if gErr != nil || granted == "" { + // Both kinds of self-asserted client reach the same page, because + // the user-facing fact is the same either way: nobody at this + // deployment vouched for the name being displayed. RFC 7591 §5 + // asks for exactly this — it warns that "a rogue client might use + // the name and logo of a legitimate client" and tells servers to + // "present warning messages to end-users about dynamically + // registered clients". + // + // Where the name and redirect list come from is the only + // difference: a document fetched from the client's own URL, or + // the row it wrote at registration. + var clientName string + var redirectURIs []string + if isCIMDClient { + doc, dErr := h.ClientMetadataProvider.Resolve(gc.Request.Context(), clientID) + if dErr != nil { + log.Debug().Err(dErr).Msg("could not resolve client metadata document for consent") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_client", + "error_description": "could not resolve the client metadata document for this client_id", + }) + return + } + clientName, redirectURIs = doc.ClientName, doc.RedirectURIs + } else { + client, cErr := h.StorageProvider.GetClientByClientID(gc.Request.Context(), clientID) + // GetClientByClientID is the documented (nil, nil)-on-absent + // exception, so absent and unavailable arrive differently and + // must be answered differently: invalid_client tells a caller + // their client_id is permanently wrong, which during an + // outage is both false and non-retryable. Neither outcome + // skips consent — reaching this point already established the + // client is self-asserted. + switch { + case cErr != nil: + log.Warn().Err(cErr).Msg("client lookup failed while preparing consent") + gc.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "temporarily_unavailable", + "error_description": "could not load the client; please retry", + }) + return + case client == nil: + // The row was there during redirect validation and is + // gone now — deleted mid-flight. + log.Debug().Msg("registered client disappeared between redirect validation and consent") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_client", + "error_description": "could not load the client for this client_id", + }) + return + } + clientName, redirectURIs = client.Name, client.ParsedRedirectURIs() + } + // OIDC Core §3.1.2.1: with prompt=none the authorization server + // "MUST NOT display any authentication or consent user interface + // pages". A self-asserted client still requires consent, and the + // two cannot both be satisfied — so the request fails with + // consent_required and the client decides whether to retry + // interactively. + // + // The two prompt=none guards above only cover the UNAUTHENTICATED + // case. Without this, a caller with a live session would be shown + // a consent page in response to a request that forbids one. + if prompt == "none" { + redirectErrorToRP(gc, responseMode, redirectURI, state, "consent_required", + "prompt=none was requested but this client requires consent") + return + } + h.renderConsent(gc, consentClient{ + ClientID: clientID, + ClientName: clientName, + RedirectURIs: redirectURIs, + }, redirectURI, user.ID, refs.StringValue(user.Email), scope) + return + } + } + sessionKey := user.ID if claims.LoginMethod != "" { sessionKey = claims.LoginMethod + ":" + user.ID diff --git a/internal/http_handlers/authorize_redirect_uri_test.go b/internal/http_handlers/authorize_redirect_uri_test.go index 52af6b0fa..e4ca23a0c 100644 --- a/internal/http_handlers/authorize_redirect_uri_test.go +++ b/internal/http_handlers/authorize_redirect_uri_test.go @@ -2,7 +2,6 @@ package http_handlers import ( "context" - "errors" "net/http" "net/http/httptest" "testing" @@ -26,9 +25,15 @@ type redirectURIClientStore struct { client *schemas.Client } +// GetClientByClientID follows the documented exception for this one method: an +// absent row is (nil, nil), NOT an error. The stub used to return an error here, +// which made it disagree with all six real providers — and that mattered as soon +// as the handler started distinguishing "no such client" from "could not check", +// because a stub reporting an outage for every unregistered client_id turned +// ordinary requests into 503s in tests only. func (s *redirectURIClientStore) GetClientByClientID(_ context.Context, clientID string) (*schemas.Client, error) { if s.client == nil || s.client.ClientID != clientID { - return nil, errors.New("not found") + return nil, nil } return s.client, nil } diff --git a/internal/http_handlers/client_redirect.go b/internal/http_handlers/client_redirect.go new file mode 100644 index 000000000..201a5f191 --- /dev/null +++ b/internal/http_handlers/client_redirect.go @@ -0,0 +1,110 @@ +package http_handlers + +import ( + "context" + "errors" + + "github.com/authorizerdev/authorizer/internal/clientmetadata" + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/validators" +) + +// redirectCheck is the outcome of validating a presented redirect_uri against +// the client that presented it. +type redirectCheck struct { + // Valid reports whether the URI may be redirected to. + Valid bool + // SelfRegistered reports whether the client asserted its own identity + // (RFC 7591 self-registration). Callers use it to decide whether consent and + // mandatory PKCE apply. Meaningless when Valid is false. + SelfRegistered bool +} + +// errClientUnavailable means the client could not be CHECKED — a storage +// failure or an unresolvable metadata document — as opposed to a client whose +// redirect_uri simply did not match. Callers must fail closed on it rather than +// falling back to the origin allow-list. +var errClientUnavailable = errors.New("could not verify the client") + +// errClientUnresolvable distinguishes the metadata-document case, which is the +// client's fault (a bad client_id) rather than this server's, and so maps to +// invalid_client / 400 instead of temporarily_unavailable / 503. +var errClientUnresolvable = errors.New("could not resolve the client metadata document") + +// checkClientRedirectURI decides whether redirectURI is acceptable for clientID. +// +// It exists as one function called from BOTH /authorize and /app because those +// two are halves of a single decision. /authorize validates the redirect and +// then, for an unauthenticated visitor, hands the SAME redirect_uri to /app to +// render the login page. When /app applied a different (AllowedOrigins-only) +// rule, every client whose registered redirect_uri was not also an allowed +// origin passed the first check and failed the second — the login page returned +// "invalid redirect url" and the flow died before the user could type anything. +// +// That gap was invisible in testing for a long time because every fixture +// allow-listed its callback origin. It cannot be papered over that way in +// production: an MCP client binds an EPHEMERAL loopback port, so there is no +// origin an operator could have added in advance. +// +// The precedence deliberately mirrors RFC 6749 §3.1.2.3 / RFC 9700: a client +// that registered exact redirect URIs is held to an exact match against them — +// never a prefix or origin match, which alone would let any path under an +// allowed host through, including a suffix appended to another client's +// callback. AllowedOrigins remains the fallback only for clients that have +// registered nothing, which is what the deployment's own reserved client relies +// on. +func (h *httpProvider) checkClientRedirectURI(ctx context.Context, clientID, redirectURI, hostname string) (redirectCheck, error) { + // No client_id: this is a non-OAuth use of the login page. The global + // allow-list is the only rule that could apply. + if clientID == "" { + return redirectCheck{Valid: validators.IsValidRedirectURI(redirectURI, h.Config.AllowedOrigins, hostname)}, nil + } + + // A Client ID Metadata Document client carries its redirect_uris in a + // document at its own client_id URL rather than in the registry. A + // resolution failure is fatal rather than a fall-through to the + // AllowedOrigins fallback: falling through would let an unresolvable + // client_id inherit a LAXER check than a resolved one, which is backwards. + if h.ClientMetadataProvider != nil && clientmetadata.IsMetadataClientIDFor(clientID, h.Config.ClientID) { + doc, err := h.ClientMetadataProvider.Resolve(ctx, clientID) + if err != nil { + return redirectCheck{}, errClientUnresolvable + } + return redirectCheck{Valid: matchesAny(doc.RedirectURIs, redirectURI)}, nil + } + + client, err := h.StorageProvider.GetClientByClientID(ctx, clientID) + // A storage error is NOT "no such client". Collapsing the two silently + // downgrades the request to the laxer AllowedOrigins check AND leaves a + // self-asserted client looking operator-registered, so it would skip + // consent. GetClientByClientID is the documented (nil, nil)-on-absent + // exception precisely so absent and unavailable stay distinguishable here. + if err != nil { + return redirectCheck{}, errClientUnavailable + } + if client == nil { + // Legacy path: no registry row, so the global allow-list stands. + return redirectCheck{Valid: validators.IsValidRedirectURI(redirectURI, h.Config.AllowedOrigins, hostname)}, nil + } + + out := redirectCheck{SelfRegistered: constants.IsSelfRegistered(client.Kind)} + if registered := client.ParsedRedirectURIs(); len(registered) > 0 { + out.Valid = matchesAny(registered, redirectURI) + return out, nil + } + // A registry row that registered no redirect URIs still falls back, which is + // what the reserved interactive client depends on. + out.Valid = validators.IsValidRedirectURI(redirectURI, h.Config.AllowedOrigins, hostname) + return out, nil +} + +// matchesAny reports whether presented satisfies any registered URI, using the +// same matcher everywhere so RFC 8252 loopback rules apply identically. +func matchesAny(registered []string, presented string) bool { + for _, r := range registered { + if redirectURIMatches(r, presented) { + return true + } + } + return false +} diff --git a/internal/http_handlers/consent.go b/internal/http_handlers/consent.go new file mode 100644 index 000000000..e18b0e9ab --- /dev/null +++ b/internal/http_handlers/consent.go @@ -0,0 +1,418 @@ +package http_handlers + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "net/http" + "net/url" + "strings" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + + "github.com/authorizerdev/authorizer/internal/metrics" +) + +// Pending consents are held in the state store, which applies its own expiry — +// 10 minutes on Redis (stateTTL), 600 seconds read-side on the DB store. That is +// the right order of magnitude here: long enough for a human to read the page, +// short enough that an abandoned tab is not a standing authorization waiting to +// be submitted later. There is no per-key TTL on the SetState interface, so this +// is inherited rather than chosen; if that ever needs to differ for consent, the +// interface has to grow a TTL argument rather than this file working around it. + +// pendingConsent is the authorization request held while the user decides. +// +// It exists because consent splits one logical authorization into two HTTP +// requests, and everything the second request needs must survive the gap +// without being re-derived from parameters the browser could have altered in +// between. Only the consent_id travels through the page; every value below is +// read back from the store, so a tampered form cannot widen scope, redirect +// elsewhere, or swap the client. +type pendingConsent struct { + ClientID string `json:"client_id"` + ClientName string `json:"client_name"` + RedirectURI string `json:"redirect_uri"` + // Scopes is rendered on the page. It is NOT read back on submit — the + // resumed request carries scope in Query, which is the value actually + // enforced. Kept so the stored record fully describes what was shown. + Scopes []string `json:"scopes"` + // Query is the original /authorize parameter set, re-encoded and replayed on + // approval so the resumed request carries exactly what was consented to. + // + // Built from the parsed FORM, not from URL.RawQuery: /authorize is registered + // for POST as well as GET (RFC 6749 §3.1 / OIDC Core §3.1.2.1 permit it), and + // a POST carries its parameters in the body. Storing only the query string + // left Query empty for those, so approval replayed a parameterless request + // and the user was dropped on "response_type is required" after having + // approved — while the client waited on a callback that never came. + Query string `json:"query"` + // UserID pins the consent to the session that saw the page. A consent + // approved in one account must never mint a code for another. + UserID string `json:"user_id"` +} + +// consentClient is the minimum a self-asserted client must supply to be +// described on the consent page. +// +// It exists so the page does not depend on WHERE the client's claims came from. +// The two sources — a Client ID Metadata Document fetched from the client's own +// URL, and a row written by the RFC 7591 registration endpoint — differ in +// plumbing and not in trust: in both cases the name is chosen by the client and +// verified by nobody. Passing a *clientmetadata.Document here instead would have +// forced the registration path to fabricate one, which would have read as if a +// document had been fetched when none had. +type consentClient struct { + ClientID string + ClientName string + // RedirectURIs is the client's full registered list, used only to decide + // whether the loopback warning applies. The URI actually being authorized is + // passed separately and is what the page displays. + RedirectURIs []string +} + +// isLoopbackOnly reports whether every registered redirect URI is a loopback +// address. +// +// Such a client cannot be distinguished from a local impostor: any process on +// the user's machine can bind a port and claim the same metadata document, or +// register itself under the same name. Both specs call this out and say the +// authorization server SHOULD warn about it, because it is not solvable +// server-side. The consent screen uses this to say so out loud. +// +// Shares isLoopbackHost with the RFC 8252 redirect matcher and the registration +// validator, so "is this loopback?" has exactly one answer in this codebase. +func (c consentClient) isLoopbackOnly() bool { + if len(c.RedirectURIs) == 0 { + return false + } + for _, raw := range c.RedirectURIs { + u, err := url.Parse(raw) + if err != nil || !isLoopbackHost(u.Hostname()) { + return false + } + } + return true +} + +// renderConsent stores the pending authorization and shows the consent page. +// +// Consent is required for self-asserted clients and NOT for operator-registered +// ones, and the asymmetry is the whole point. A registered client was vouched +// for by an operator who entered its redirect URIs by hand. A self-asserted +// client — one presenting a Client ID Metadata Document, or one that registered +// itself through RFC 7591 — chose its own `client_name`, and anyone who can host +// a JSON file or POST to /oauth/register can claim any name. The only fact about +// it this server has verified is the redirect host, which is precisely what the +// page leads with. +// +// The MCP authorization spec requires the authorization server to display the +// redirect URI hostname and to warn about loopback-only clients, because a local +// impostor can bind the same port and present the legitimate client's metadata. +// RFC 7591 §5 asks for the same warning for dynamically registered clients. +func (h *httpProvider) renderConsent(gc *gin.Context, client consentClient, redirectURI, userID, userEmail string, scopes []string) { + log := h.Log.With().Str("func", "renderConsent").Logger() + + consentID := uuid.NewString() + payload, err := json.Marshal(pendingConsent{ + ClientID: client.ClientID, + ClientName: client.ClientName, + RedirectURI: redirectURI, + Scopes: scopes, + Query: originalParams(gc).Encode(), + UserID: userID, + }) + if err != nil { + log.Debug().Err(err).Msg("failed to encode pending consent") + h.consentError(gc, http.StatusInternalServerError, + "Something went wrong", + "We could not start the approval for this application.", + "Please return to the application and try connecting again.") + return + } + if err := h.MemoryStoreProvider.SetState(consentKey(consentID), string(payload)); err != nil { + log.Debug().Err(err).Msg("failed to store pending consent") + h.consentError(gc, http.StatusInternalServerError, + "Something went wrong", + "We could not start the approval for this application.", + "Please return to the application and try connecting again.") + return + } + + redirectHost := redirectURI + if u, err := url.Parse(redirectURI); err == nil && u.Host != "" { + redirectHost = u.Host + } + + // The consent form's submission legitimately ends at the client's + // redirect_uri, so the CSP must permit that destination — otherwise the + // button does nothing. + // + // A browser enforces `form-action` across the ENTIRE redirect chain of a + // form submission, not just its immediate target. Approving POSTs to + // /authorize/consent → 302 /authorize → 302 the client's redirect_uri, so + // the default `form-action 'self'` aborts the navigation at the last hop. + // The user is left staring at the consent page with no error, assumes the + // click missed, clicks again — and the second POST fails with "expired or + // already used", because the first one already consumed the consent. + // + // This server already hit the same rule twice: setFormPostCSP relaxes + // form-action for OIDC form_post, and samlIDPSSOCSP omits it because + // "form-action 'self' would silently break every SAML IdP login". The + // consent page is the third instance of the same shape. + // + // Scoped to the ONE origin being approved rather than the `form-action *` + // that form_post uses: the redirect_uri has already been validated against + // this client's registered list, so the exact destination is known here and + // nothing wider needs allowing. + setConsentCSP(gc, redirectURI) + + metrics.RecordSecurityEvent("cimd_consent_shown", "authorize") + // The page carries a single-use consent_id and names the signed-in user, so + // it must not sit in a shared or back-button cache: a cached copy would show + // one user's email to the next person on the machine, and re-submitting a + // stale page is a guaranteed "expired or already used" dead end. + gc.Header("Cache-Control", "no-store, no-cache, must-revalidate") + gc.Header("Pragma", "no-cache") + gc.HTML(http.StatusOK, "consent.tmpl", gin.H{ + "client_name": client.ClientName, + "client_id": client.ClientID, + // The host, not the full URI: it is the part that decides where the + // authorization code actually lands, and the part a person can judge. + "redirect_host": redirectHost, + "user_email": userEmail, + "scopes": scopes, + "loopback_only": client.isLoopbackOnly(), + "consent_id": consentID, + "organization_name": h.Config.OrganizationName, + "organization_logo": h.Config.OrganizationLogo, + }) +} + +// consentError renders a consent failure as a page rather than a JSON body. +// +// Everything that reaches this handler is a browser following a form the server +// itself rendered, so the response is read by a person, not a program. It used +// to return raw JSON — which is what a user actually saw after clicking +// "Allow access" twice: `{"error":"invalid_request","error_description":"this +// consent request has expired or was already used"}`, with no indication of +// what to do next. +// +// The OAuth error code stays in the logs and metrics; it is not something the +// person in front of the screen can act on. What they can act on is the hint. +func (h *httpProvider) consentError(gc *gin.Context, status int, title, message, hint string) { + gc.Header("Cache-Control", "no-store, no-cache, must-revalidate") + gc.Header("Pragma", "no-cache") + gc.HTML(status, "consent_error.tmpl", gin.H{ + "title": title, + "message": message, + "hint": hint, + "organization_name": h.Config.OrganizationName, + "organization_logo": h.Config.OrganizationLogo, + }) +} + +// ConsentHandler processes the approve/deny decision. +// +// On approval it replays the ORIGINAL /authorize query rather than rebuilding +// one from form fields. That is deliberate: rebuilding would make every +// parameter — scope, redirect_uri, code_challenge, resource — attacker-editable +// between the page render and the submit, so a user could be shown one request +// and made to approve another. +func (h *httpProvider) ConsentHandler() gin.HandlerFunc { + return func(gc *gin.Context) { + log := h.Log.With().Str("func", "ConsentHandler").Logger() + + consentID := strings.TrimSpace(gc.PostForm("consent_id")) + if consentID == "" { + h.consentError(gc, http.StatusBadRequest, + "This approval link is not valid", + "The request is missing the identifier that ties it to an approval.", + "Please return to the application and try connecting again.") + return + } + + // Atomic: a read-then-delete lets two concurrent submissions of the same + // consent_id both pass the single-use check, and this repo already ships + // GetAndRemoveState precisely because "returning state on the strength of + // the read alone would hand the same code to every racer". + raw, err := h.MemoryStoreProvider.GetAndRemoveState(consentKey(consentID)) + if err != nil || raw == "" { + // Expired, already used, or never existed — all indistinguishable to + // the caller on purpose, so this is not an oracle for guessing ids. + log.Debug().Msg("consent rejected: no pending request for this consent_id") + h.consentError(gc, http.StatusBadRequest, + "This approval has already been used", + "Approvals can only be used once, and they expire after a few minutes.", + "Nothing has been shared. Return to the application and connect again to get a fresh approval.") + return + } + var pending pendingConsent + if err := json.Unmarshal([]byte(raw), &pending); err != nil { + log.Debug().Err(err).Msg("consent rejected: corrupt pending state") + h.consentError(gc, http.StatusBadRequest, + "This approval is no longer valid", + "The saved approval could not be read.", + "Please return to the application and try connecting again.") + return + } + + // The approving session must be the one that was shown the page. + // Without this, a consent page rendered for one user could be submitted + // in another user's browser and mint a code against their account. + tokenData, err := h.TokenProvider.GetUserIDFromSessionOrAccessToken(gc) + if err != nil || tokenData == nil || tokenData.UserID != pending.UserID { + metrics.RecordSecurityEvent("cimd_consent_session_mismatch", "authorize") + log.Warn().Msg("consent rejected: submitted by a different session than it was issued to") + h.consentError(gc, http.StatusUnauthorized, + "Please sign in again", + "This approval was created for a different sign-in session, so it was not applied.", + "Return to the application and connect again to approve as the account you are signed in with.") + return + } + + if gc.PostForm("action") != "approve" { + // RFC 6749 §4.1.2.1: a refusal is reported to the client at its + // registered redirect_uri, not rendered here — the client is waiting + // on that callback and would otherwise hang. + metrics.RecordSecurityEvent("cimd_consent_denied", "authorize") + // redirectErrorToRP, not a hand-rolled query-string redirect: the + // original request may have asked for fragment, form_post or + // web_message, and delivering a query-string 302 to a client that + // asked for form_post leaves it waiting on a response it will never + // parse. The helper is what every other /authorize error path uses. + orig, _ := url.ParseQuery(pending.Query) + redirectErrorToRP(gc, orig.Get("response_mode"), pending.RedirectURI, + orig.Get("state"), "access_denied", + "the user declined to authorize this application") + return + } + + // Approved. Record the grant server-side and REDIRECT the browser back to + // /authorize with the original query, rather than invoking the handler + // in-process. + // + // Calling it directly does not work: gin caches parsed query parameters + // on the Context at first access, so rewriting Request.URL.RawQuery + // afterwards leaves the handler reading the POST's (empty) query and + // failing with "response_type is required". Rewriting gin's internals to + // force a re-parse would be a worse dependency than a redirect. + // + // A redirect is also the more honest shape: it is exactly what the client + // would see from any other authorization server, and it re-enters + // /authorize through the front door with every middleware applied. + metrics.RecordSecurityEvent("cimd_consent_approved", "authorize") + if err := h.MemoryStoreProvider.SetState( + consentGrantKey(pending.UserID, pending.ClientID, pending.Query), "1"); err != nil { + log.Debug().Err(err).Msg("failed to record consent grant") + h.consentError(gc, http.StatusInternalServerError, + "Something went wrong", + "Your approval could not be recorded.", + "Please return to the application and try connecting again.") + return + } + gc.Redirect(http.StatusFound, "/authorize?"+pending.Query) + } +} + +// consentGrantKey names the single-use marker that tells the authorize handler +// this user has consented to this client FOR THIS EXACT REQUEST. +// +// The request is part of the key, not just the user and client. Keying on +// (user, client) alone meant a grant that was never redeemed — the tab closed, +// the browser went back, the network dropped — sat in the store for its whole +// TTL and would then satisfy ANY later /authorize for that pair: a different +// scope, a different redirect_uri from the document's list, a different PKCE +// challenge. The user would have been shown one request and a materially +// different one would execute, which is exactly what storing the full parameter +// set in pendingConsent exists to prevent. Hashing it in closes that. +// +// SHA-256 of the encoded parameters rather than the parameters themselves: +// the key goes into a shared store, and a redirect_uri or login_hint in a key is +// needless exposure. +func consentGrantKey(userID, clientID, query string) string { + sum := sha256.Sum256([]byte(query)) + return "cimd_consent_granted:" + userID + ":" + clientID + ":" + hex.EncodeToString(sum[:]) +} + +// originalParams returns the /authorize parameters as sent, from whichever place +// they arrived — query string for GET, body for POST. Mirrors how the authorize +// handler itself reads them (gc.Request.FormValue). +func originalParams(gc *gin.Context) url.Values { + _ = gc.Request.ParseForm() + out := url.Values{} + for k, v := range gc.Request.Form { + if len(v) > 0 { + out.Set(k, v[0]) + } + } + return out +} + +func consentKey(id string) string { return "cimd_consent:" + id } + +// setConsentCSP writes the consent page's Content-Security-Policy, widening +// form-action to include the redirect_uri's origin and nothing else. +// +// It mirrors the default policy (internal/http_handlers/security_headers.go) +// rather than deriving from it, for the same reason setFormPostCSP does: the +// header is replaced wholesale, so it has to be complete. Keep the two in step. +// +// The origin is appended, never substituted — 'self' must stay, because the +// form posts to /authorize/consent on this origin before it redirects anywhere. +func setConsentCSP(gc *gin.Context, redirectURI string) { + formAction := "'self'" + // A relative redirect (the "/app" default, reachable when a metadata-document + // client omits redirect_uri) has no origin to add and needs none. + // + // The host is checked before it is embedded. url.Parse rejects spaces and + // control characters in a host, but it ACCEPTS ";" — and ";" separates CSP + // directives, so a client registering https://evil.com;x/cb would terminate + // this directive and start another. Nothing exploitable follows from it (a + // directive needs a space before its values, and form-action is last here), + // but a value the client chose does not belong in a security header + // unexamined. A host that fails this is not a real host; the origin is + // dropped rather than the request refused, because this function's job is to + // write a header, not to adjudicate the redirect — which redirectURIMatches + // already did. + if u, err := url.Parse(redirectURI); err == nil && u.Scheme != "" && isCSPSafeHost(u.Host) { + formAction += " " + u.Scheme + "://" + u.Host + } + gc.Writer.Header().Set("Content-Security-Policy", + "default-src 'self'; "+ + "script-src 'self'; "+ + "style-src 'self' 'unsafe-inline'; "+ + // `https:` matches the default policy, and it is load-bearing here: + // --organization-logo is usually hosted somewhere else entirely, so + // restricting to 'self' renders the operator's own branding as a + // broken image on the one page where branding is the trust signal. + "img-src 'self' data: https:; "+ + "font-src 'self' data:; "+ + "connect-src 'self'; "+ + "frame-ancestors 'none'; "+ + "base-uri 'self'; "+ + "form-action "+formAction+";") +} + +// isCSPSafeHost reports whether host consists only of characters that can +// legitimately appear in an authority and carry no meaning in a CSP policy. +// +// Deliberately an allow-list. A deny-list of ";" alone would pass whatever the +// next CSP revision makes significant, and the set of legal host characters is +// far smaller and far more stable than the set of dangerous ones. Brackets and +// the colon are permitted for IPv6 literals and ports. +func isCSPSafeHost(host string) bool { + if host == "" { + return false + } + for _, r := range host { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '.', r == '-', r == ':', r == '[', r == ']': + default: + return false + } + } + return true +} diff --git a/internal/http_handlers/consent_client_test.go b/internal/http_handlers/consent_client_test.go new file mode 100644 index 000000000..7c5109b63 --- /dev/null +++ b/internal/http_handlers/consent_client_test.go @@ -0,0 +1,94 @@ +package http_handlers + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestConsentClientIsLoopbackOnly drives a consent-screen warning both specs ask +// for, so a wrong answer is user-visible: a false negative hides a real risk, a +// false positive cries wolf on a normal web client. +// +// Moved here from internal/clientmetadata when the predicate stopped being a +// property of a metadata document — a client that registered itself via RFC 7591 +// needs the identical warning and has no document. +func TestConsentClientIsLoopbackOnly(t *testing.T) { + cases := []struct { + name string + uris []string + want bool + }{ + {"all loopback", []string{"http://127.0.0.1/cb", "http://localhost:1/cb"}, true}, + {"ipv6 loopback", []string{"http://[::1]/cb"}, true}, + {"mixed", []string{"http://127.0.0.1/cb", "https://app.example.com/cb"}, false}, + {"public only", []string{"https://app.example.com/cb"}, false}, + {"empty", nil, false}, + {"lookalike host is not loopback", []string{"https://127.0.0.1.evil.com/cb"}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, consentClient{RedirectURIs: tc.uris}.isLoopbackOnly()) + }) + } +} + +// TestValidateRegistrationRedirectURI pins what an anonymous caller may store as +// a redirect target. Every rejection here is a place a code could otherwise be +// delivered somewhere the user did not intend. +func TestValidateRegistrationRedirectURI(t *testing.T) { + cases := []struct { + name string + uri string + ok bool + why string + }{ + {"https", "https://app.example.com/cb", true, "the normal case"}, + {"loopback http", "http://127.0.0.1:5599/cb", true, "RFC 8252 §7.3 native clients"}, + {"localhost http", "http://localhost:1234/cb", true, "same, by name"}, + {"ipv6 loopback http", "http://[::1]:1234/cb", true, "same, over IPv6"}, + {"public http", "http://app.example.com/cb", false, "codes would cross the network in the clear"}, + {"custom scheme", "myapp://cb", false, "the server cannot tell which local app receives the code"}, + {"fragment", "https://app.example.com/cb#f", false, "RFC 6749 §3.1.2 forbids a fragment"}, + {"relative", "/cb", false, "a redirect_uri must be absolute"}, + {"empty", "", false, "nothing to redirect to"}, + {"no host", "https:///cb", false, "an https URI with no host is not a destination"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateRegistrationRedirectURI(tc.uri) + if tc.ok { + assert.NoError(t, err, tc.why) + return + } + assert.Error(t, err, tc.why) + }) + } +} + +// TestIsCSPSafeHost guards the one place a client-chosen value reaches a +// security header. url.Parse rejects spaces and control characters in a host +// but accepts ";", which separates CSP directives — so a registered +// https://evil.com;x/cb would otherwise terminate form-action and open a new +// directive. +func TestIsCSPSafeHost(t *testing.T) { + for _, tc := range []struct { + host string + ok bool + why string + }{ + {"app.example.com", true, "ordinary host"}, + {"127.0.0.1:5599", true, "loopback with a port"}, + {"[::1]:3000", true, "IPv6 literal with a port"}, + {"sub-domain.example.com", true, "hyphens are legal in hostnames"}, + {"evil.com;x", false, "semicolon starts a new CSP directive"}, + {"evil.com,other.com", false, "comma separates policies"}, + {"evil.com'", false, "quote could close a source expression"}, + {"evil.com *", false, "a space would let a value be injected"}, + {"", false, "nothing to allow"}, + } { + t.Run(tc.host, func(t *testing.T) { + assert.Equal(t, tc.ok, isCSPSafeHost(tc.host), tc.why) + }) + } +} diff --git a/internal/http_handlers/csrf.go b/internal/http_handlers/csrf.go index 313fd82b7..04c445bdc 100644 --- a/internal/http_handlers/csrf.go +++ b/internal/http_handlers/csrf.go @@ -50,6 +50,20 @@ func (h *httpProvider) CSRFMiddleware() gin.HandlerFunc { return } + // Exempt RFC 7591 dynamic client registration. The endpoint is + // unauthenticated by design (RFC 7591 §5: the server "SHOULD allow + // registration requests with no authorization"), so there is no ambient + // credential for a cross-site request to abuse — which is the only thing + // CSRF protection defends. The caller is a CLI or SDK making a + // programmatic POST with no Origin or Referer, so the generic check + // would reject every legitimate request and none of the illegitimate + // ones. Abuse is bounded by the per-IP rate limiter and the registry + // ceiling instead; see RegisterClientHandler. + if c.Request.URL.Path == "/oauth/register" { + c.Next() + return + } + // Exempt the inbound SCIM 2.0 surface. SCIM requests are machine-to- // machine, authenticated by a per-org bearer token (never cookies), so // CSRF does not apply — same rationale as /oauth/token above. @@ -58,6 +72,30 @@ func (h *httpProvider) CSRFMiddleware() gin.HandlerFunc { return } + // Exempt the consent form POST. + // + // This is NOT a hole, because the flow already carries a stronger + // synchronizer token than the generic middleware would check. The form's + // consent_id is a random UUID that exists only inside a page this server + // rendered for one specific session; it is single-use, and + // ConsentHandler additionally verifies the submitting session is the one + // it was issued to. An attacker cannot forge a submission without first + // reading a value they have no way to obtain — which is precisely the + // property CSRF protection exists to create. + // + // The exemption is necessary rather than convenient: the generic check + // demands `Content-Type: application/json` or `X-Requested-With`, and a + // plain HTML form can send neither. The alternative would be to drive the + // form with JavaScript, which would make consent — the one screen a user + // must be able to read and trust — silently fail with JS disabled. + // + // Found by the e2e browser test; every unit-level test called the handler + // directly and so never saw the middleware. + if c.Request.URL.Path == "/authorize/consent" { + c.Next() + return + } + // Exempt the MCP surface. Same rationale, and it holds structurally // rather than by convention: MCPAuthMiddleware authenticates only a // bearer token whose audience names this MCP server, the interceptor diff --git a/internal/http_handlers/openid_config.go b/internal/http_handlers/openid_config.go index 9e3df45ff..1a4178116 100644 --- a/internal/http_handlers/openid_config.go +++ b/internal/http_handlers/openid_config.go @@ -85,35 +85,54 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { // RFC 8707 resource indicators are honored on the authorization_code // flow (resource query param → access token aud) and token-exchange. "resource_indicators_supported": true, - // NO `registration_endpoint`, and that is deliberate — please do not - // "fix" it by adding RFC 7591 dynamic client registration. + // Advertised only when enabled. Anthropic documents Claude selecting + // CIMD when this is true AND "none" appears in + // token_endpoint_auth_methods_supported (it authenticates as a public + // client); both hold here when the flag is on. + "client_id_metadata_document_supported": h.Config.EnableClientIDMetadataDocument, + // `registration_endpoint` (RFC 7591) is advertised ONLY when the + // feature is switched on, and it ships off by default. Everything + // below is why it is opt-in rather than absent, and why it is opt-in + // rather than on. // - // The MCP authorization spec (2025-11-25) demoted DCR: authorization + // The MCP authorization spec (2025-11-25) demotes DCR: authorization // servers **SHOULD** support Client ID Metadata Documents and **MAY** - // support DCR, which it keeps only "for backwards compatibility with + // support DCR, which it keeps "for backwards compatibility with // earlier versions of the MCP authorization spec". CIMD is the - // recommended path, and the client priority order is pre-registered → - // CIMD → DCR → prompt the user. + // recommended path and remains this server's preferred one. // https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization // - // DCR is also an open, unauthenticated write endpoint. Auth0 ships it - // Enterprise-only, disabled by default, and requires tenant ACLs or a - // reverse proxy in front, citing resource depletion, security probing, - // unvetted misconfigured clients and audit gaps — then recommends CIMD - // instead for production. - // https://auth0.com/ai/docs/mcp/guides/registering-your-mcp-client-application/dynamic-client-registration + // This endpoint exists anyway because the spec's client priority + // order — pre-registered → CIMD → DCR → prompt the user — is only as + // good as client support for step 2, and shipping clients are still + // on step 3. Claude Code reads this document, finds + // `client_id_metadata_document_supported` true, and still refuses + // with "Incompatible auth server: does not support dynamic client + // registration" because its released version predates CIMD. Without + // `registration_endpoint` those clients cannot connect at all. + // + // That same priority order is what makes advertising it safe: a + // CIMD-capable client picks CIMD first and never reaches DCR, so + // enabling this cannot downgrade a client that could have done + // better. // - // Anthropic's own connector guidance points the same way: DCR makes a - // client register afresh on every connection, so a self-hosted - // deployment accumulates client rows without bound. + // It stays OFF by default because it is an open, unauthenticated + // write endpoint. Auth0 ships DCR Enterprise-only and disabled, + // requiring tenant ACLs or a reverse proxy, citing resource + // depletion, security probing, unvetted clients and audit gaps — + // then recommends CIMD for production. + // https://auth0.com/ai/docs/mcp/guides/registering-your-mcp-client-application/dynamic-client-registration + // Keycloak disables anonymous registration by default and gates it + // behind registration policies; Ory Hydra hides it behind + // `oidc.dynamic_client_registration.enabled`. Anthropic's connector + // guidance notes DCR clients re-register on every connection, so rows + // accumulate — which is what maxRegisteredClients bounds. // https://claude.com/docs/connectors/building/authentication // - // `client_id_metadata_document_supported` is NOT advertised yet either, - // because advertising a capability that is not implemented is worse - // than omitting it — a client would select CIMD and then fail. Adding - // CIMD is tracked as the follow-up to the MCP transport work; it needs - // the URL-form client_id resolver plus an /authorize consent screen, - // which the spec makes mandatory once client identity is self-asserted. + // Both self-registration capabilities are advertised only when + // actually enabled: advertising one that is off would make a client + // select it and then fail, which is worse than omitting it and + // letting the client fall back. "revocation_endpoint": issuer + "/oauth/revoke", "revocation_endpoint_auth_methods_supported": []string{"client_secret_basic", "client_secret_post"}, "introspection_endpoint": issuer + "/oauth/introspect", @@ -126,6 +145,14 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { "request_uri_parameter_supported": false, } + // Added rather than set to "" when disabled: a client that sees the key + // at all will POST to it, so an empty or absent-but-present value is + // worse than no key. RFC 8414 §2 treats an omitted metadata field as + // "not supported". + if h.Config.EnableDynamicClientRegistration { + resp["registration_endpoint"] = issuer + "/oauth/register" + } + // Discovery metadata changes infrequently; allow caching. c.Header("Cache-Control", "public, max-age=300") c.JSON(200, resp) diff --git a/internal/http_handlers/provider.go b/internal/http_handlers/provider.go index a11efa311..8610c4ae7 100644 --- a/internal/http_handlers/provider.go +++ b/internal/http_handlers/provider.go @@ -7,6 +7,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/authenticators" "github.com/authorizerdev/authorizer/internal/authorization/engine" + "github.com/authorizerdev/authorizer/internal/clientmetadata" "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/email" "github.com/authorizerdev/authorizer/internal/events" @@ -41,6 +42,11 @@ type Dependencies struct { StorageProvider storage.Provider // TokenProvider is used to generate tokens TokenProvider token.Provider + // ClientMetadataProvider resolves Client ID Metadata Documents (CIMD): + // client_ids that are HTTPS URLs. nil disables the feature entirely, which + // is the default — it changes the authorization endpoint's trust model for + // every client, so it is opt-in via --enable-client-id-metadata-document. + ClientMetadataProvider *clientmetadata.Provider // OAuthProvider is used to register oauth providers OAuthProvider oauth.Provider // RateLimitProvider is used for per-IP rate limiting @@ -66,6 +72,10 @@ func New(cfg *config.Config, deps *Dependencies) (Provider, error) { Log: deps.Log, StorageProvider: deps.StorageProvider, MemoryStoreProvider: deps.MemoryStoreProvider, + // Same resolver instance the authorize handler uses, so a document + // validated at /authorize is served from cache at /oauth/token and + // the two cannot disagree about a client mid-flow. + ClientMetadataProvider: deps.ClientMetadataProvider, }), } return g, nil @@ -135,6 +145,11 @@ type Provider interface { // MCPAuthMiddleware authenticates /mcp and issues the RFC 9728 §5.1 // WWW-Authenticate challenge when it cannot. MCPAuthMiddleware() gin.HandlerFunc + // ConsentHandler processes the approve/deny decision for a CIMD client. + ConsentHandler() gin.HandlerFunc + // RegisterClientHandler implements RFC 7591 dynamic client registration. + // Only routed when EnableDynamicClientRegistration is set. + RegisterClientHandler() gin.HandlerFunc // PlaygroundHandler is the main handler that handels all the playground requests PlaygroundHandler() gin.HandlerFunc // RevokeRefreshTokenHandler is the main handler that handels all the revoke refresh token requests diff --git a/internal/http_handlers/register_client.go b/internal/http_handlers/register_client.go new file mode 100644 index 000000000..5c3dc3e89 --- /dev/null +++ b/internal/http_handlers/register_client.go @@ -0,0 +1,311 @@ +package http_handlers + +import ( + "errors" + "net/http" + "net/url" + "strings" + "unicode/utf8" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/metrics" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// maxRegisteredClients is a hard ceiling on the number of rows in the client +// registry, checked before a self-registration is accepted. +// +// RFC 7591 §5 allows an unauthenticated registration endpoint but expects it to +// be "rate-limited or otherwise limited to prevent a denial-of-service attack on +// the client registration endpoint". The per-IP limiter bounds the RATE; this +// bounds the STOCK, which is the part that actually persists — Anthropic +// documents DCR clients re-registering on every connection, so a busy +// deployment accumulates rows without one. Keycloak's initial-access-token count +// limit exists for the same reason. +// +// It counts EVERY client, not just self-registered ones: there is no filtered +// count on the storage interface and adding one would mean a method across all +// six backends for a guard rail. A deployment with more clients than this that +// also wants anonymous registration should pre-register instead. +const maxRegisteredClients = 1000 + +// maxRedirectURIs bounds the list an anonymous caller can store in one row. +const maxRedirectURIs = 10 + +// maxClientNameLength bounds the self-asserted name that the consent screen +// renders. Templates escape it, so this is about layout and log volume, not +// injection. +const maxClientNameLength = 200 + +// clientRegistrationRequest is the RFC 7591 §2 client metadata this server +// accepts. Unknown members are ignored, which §3.1 requires ("the authorization +// server MUST ignore any client metadata sent by the client that it does not +// understand"). +// Fields the client may send that are deliberately absent here — client_uri, +// logo_uri, scope, contacts — are not merely unstored but unrendered: the +// consent screen shows a name and a redirect host and nothing else, because +// every one of those values is self-asserted and a logo is the most effective +// way to impersonate a known product. Adding a field here means deciding where +// it is displayed and what it would let a rogue client claim. +type clientRegistrationRequest struct { + RedirectURIs []string `json:"redirect_uris"` + ClientName string `json:"client_name"` + GrantTypes []string `json:"grant_types"` + ResponseTypes []string `json:"response_types"` + TokenEndpointAuthMethod string `json:"token_endpoint_auth_method"` +} + +// RegisterClientHandler implements OAuth 2.0 Dynamic Client Registration +// (RFC 7591) for MCP clients that cannot use Client ID Metadata Documents. +// +// Deliberately narrow. This endpoint registers PUBLIC, interactive clients and +// nothing else: +// +// - token_endpoint_auth_method MUST be "none". RFC 7591 §2 defaults the field +// to client_secret_basic when omitted, but issuing a secret to an anonymous +// caller would create a confidential client nobody vouched for, and a +// confidential client is exactly what must not be self-service. Omitted is +// therefore read as "none" rather than as the RFC default, and any other +// value is refused. +// - grant_types are limited to authorization_code (+ refresh_token). In +// particular client_credentials is refused: it issues a token with no user +// and no consent, and the Kind assigned here (dynamic, not service_account) +// means the token endpoint would reject it anyway — this makes the refusal +// explicit at registration time instead of at first use. +// +// RFC 7592 client management (registration_access_token, registration_client_uri) +// is NOT implemented, which RFC 7591 §3.2.1 permits — neither field is required +// in the response. Nothing here can be read back, modified or deleted through +// this endpoint; a self-registered client is disposable by design. +func (h *httpProvider) RegisterClientHandler() gin.HandlerFunc { + return func(gc *gin.Context) { + log := h.Log.With().Str("func", "RegisterClientHandler").Logger() + + // Defence in depth: the route is only mounted when the flag is on, so + // this should be unreachable. It is here because an unauthenticated + // write endpoint is the wrong place to rely on registration order. + if !h.Config.EnableDynamicClientRegistration { + gc.JSON(http.StatusNotFound, gin.H{ + "error": "invalid_request", + "error_description": "dynamic client registration is not enabled on this server", + }) + return + } + + var req clientRegistrationRequest + if err := gc.ShouldBindJSON(&req); err != nil { + log.Debug().Err(err).Msg("malformed registration request") + registrationError(gc, "invalid_client_metadata", "the request body must be a JSON object of client metadata") + return + } + + // RFC 7591 §2: "If unspecified or omitted, the default is + // client_secret_basic". That default is refused rather than honoured — + // see the doc comment. An explicit "none" is the only accepted value. + if m := strings.TrimSpace(req.TokenEndpointAuthMethod); m != "" && m != constants.TokenEndpointAuthMethodNone { + log.Debug().Str("token_endpoint_auth_method", m).Msg("registration refused: confidential client requested") + registrationError(gc, "invalid_client_metadata", + "only public clients may register dynamically; token_endpoint_auth_method must be \"none\"") + return + } + + // RFC 7591 §2 defaults grant_types to authorization_code when omitted. + grantTypes := req.GrantTypes + if len(grantTypes) == 0 { + grantTypes = []string{constants.GrantTypeAuthorizationCode} + } + // Normalised in place: the value that is VALIDATED must be the value that + // is STORED, or a padded " authorization_code" passes here and is + // persisted with the space, ready to fail a later exact comparison. + for i, gt := range grantTypes { + grantTypes[i] = strings.TrimSpace(gt) + switch grantTypes[i] { + case constants.GrantTypeAuthorizationCode, constants.GrantTypeRefreshToken: + default: + log.Debug().Str("grant_type", gt).Msg("registration refused: unsupported grant type") + registrationError(gc, "invalid_client_metadata", + "only the authorization_code and refresh_token grant types may be registered dynamically") + return + } + } + + // RFC 7591 §2 defaults response_types to ["code"] when omitted, which is + // the only value supported here. Refused rather than silently narrowed: + // §3.2.1 does let the server return metadata different from what was + // requested, but a client that asked for a token in the fragment and is + // handed back "code" would discover the difference only when its callback + // receives something it cannot parse. /authorize enforces the same rule. + for _, rt := range req.ResponseTypes { + if strings.TrimSpace(rt) != "code" { + log.Debug().Str("response_type", rt).Msg("registration refused: unsupported response type") + registrationError(gc, "invalid_client_metadata", + "only response_type \"code\" may be registered dynamically") + return + } + } + + // redirect_uris is required: every grant accepted here is redirect-based, + // and RFC 7591 §2 makes the field required for such clients. + if len(req.RedirectURIs) == 0 { + registrationError(gc, "invalid_redirect_uri", "redirect_uris is required") + return + } + if len(req.RedirectURIs) > maxRedirectURIs { + registrationError(gc, "invalid_redirect_uri", "too many redirect_uris") + return + } + normalized := make([]string, 0, len(req.RedirectURIs)) + for _, raw := range req.RedirectURIs { + u := strings.TrimSpace(raw) + if err := validateRegistrationRedirectURI(u); err != nil { + log.Debug().Str("redirect_uri", u).Err(err).Msg("registration refused: invalid redirect_uri") + registrationError(gc, "invalid_redirect_uri", err.Error()) + return + } + normalized = append(normalized, u) + } + + // Stock ceiling. Checked before the write and deliberately not atomic + // with it: two racing registrations can both pass at the boundary, which + // overshoots by the number of concurrent requests and not by more. A + // transaction here would need a storage-interface change across six + // backends to prevent an overshoot the per-IP limiter already bounds. + if _, page, err := h.StorageProvider.ListClients(gc.Request.Context(), &model.Pagination{Limit: 1}); err != nil { + log.Warn().Err(err).Msg("could not count clients before registration") + gc.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "temporarily_unavailable", + "error_description": "could not process the registration; please retry", + }) + return + } else if page != nil && page.Total >= maxRegisteredClients { + metrics.RecordSecurityEvent("dcr_registration_limit_reached", "register") + log.Warn().Int64("total", page.Total).Msg("registration refused: client registry is full") + gc.JSON(http.StatusForbidden, gin.H{ + "error": "access_denied", + "error_description": "this server is not accepting new client registrations", + }) + return + } + + clientName := strings.TrimSpace(req.ClientName) + if clientName == "" { + clientName = "Unnamed client" + } + // Truncated on a RUNE boundary, not a byte one. Slicing bytes can split a + // multi-byte character and leave invalid UTF-8, which SQLite stores + // happily and Postgres rejects outright ("invalid byte sequence for + // encoding UTF8") — a registration that works on one backend and 500s on + // another, from a name an anonymous caller chose. + if utf8.RuneCountInString(clientName) > maxClientNameLength { + clientName = string([]rune(clientName)[:maxClientNameLength]) + } + + // The client_id is a server-generated opaque UUID, never anything the + // caller supplied: RFC 7591 §2 has no client_id request field, and + // honouring one would let a caller claim an existing client's identifier. + clientID := uuid.NewString() + created, err := h.StorageProvider.AddClient(gc.Request.Context(), &schemas.Client{ + ClientID: clientID, + Kind: constants.ClientKindDynamic, + Name: clientName, + // ClientSecret stays empty. This is a public client; the token + // endpoint skips secret verification when none is presented and PKCE + // is what binds the code (see clientauth.ResolveClient). + RedirectURIs: strings.Join(normalized, ","), + GrantTypes: strings.Join(grantTypes, ","), + TokenEndpointAuthMethod: constants.TokenEndpointAuthMethodNone, + IsActive: true, + }) + if err != nil { + log.Warn().Err(err).Msg("could not persist dynamically registered client") + gc.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "temporarily_unavailable", + "error_description": "could not process the registration; please retry", + }) + return + } + + metrics.RecordSecurityEvent("dcr_client_registered", "register") + log.Info().Str("client_id", created.ClientID).Str("client_name", clientName). + Msg("dynamically registered a new public client") + + // RFC 7591 §3.2.1: 201 Created, no-store, and the registered metadata + // echoed back. client_secret and client_secret_expires_at are absent + // because no secret was issued. + gc.Header("Cache-Control", "no-store") + gc.Header("Pragma", "no-cache") + gc.JSON(http.StatusCreated, gin.H{ + "client_id": created.ClientID, + "client_id_issued_at": created.CreatedAt, + "client_name": clientName, + "redirect_uris": normalized, + "grant_types": grantTypes, + "response_types": []string{"code"}, + "token_endpoint_auth_method": constants.TokenEndpointAuthMethodNone, + }) + } +} + +// registrationError writes an RFC 7591 §3.2.2 registration error response. +func registrationError(gc *gin.Context, code, description string) { + gc.Header("Cache-Control", "no-store") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": code, + "error_description": description, + }) +} + +// validateRegistrationRedirectURI enforces the MCP authorization spec's rule +// that "all redirect URIs MUST be either localhost or use HTTPS", which is also +// what RFC 8252 §7.3 and RFC 9700 require of native and public clients. +// +// The check is on the value being STORED. /authorize separately matches the +// presented redirect_uri against this list with redirectURIMatches, so a URI +// that gets past here still cannot be widened later. +func validateRegistrationRedirectURI(raw string) error { + if raw == "" { + return errors.New("redirect_uri must not be empty") + } + // Comma is the storage encoding for the list (Client.ParsedRedirectURIs), so + // a URI containing one would be silently split into two registered redirect + // targets — a redirect-target injection. Commas are legal in a URI query, so + // this is a real input rather than a theoretical one. Checked before parsing + // because it is a property of the stored string, not of the URL. + if strings.Contains(raw, ",") { + return errors.New("redirect_uri must not contain a comma") + } + u, err := url.Parse(raw) + if err != nil { + return errors.New("redirect_uri is not a valid URI") + } + if !u.IsAbs() { + return errors.New("redirect_uri must be absolute") + } + // RFC 6749 §3.1.2: the endpoint URI "MUST NOT include a fragment component". + if u.Fragment != "" || strings.Contains(raw, "#") { + return errors.New("redirect_uri must not contain a fragment") + } + switch strings.ToLower(u.Scheme) { + case "https": + if u.Hostname() == "" { + return errors.New("redirect_uri must have a host") + } + return nil + case "http": + // Loopback only. http://evil.example.com would otherwise be registrable + // and every code issued to it would cross the network in the clear. + if !isLoopbackHost(u.Hostname()) { + return errors.New("an http redirect_uri is only allowed for loopback addresses; use https") + } + return nil + default: + // Custom schemes (myapp://) are refused: this server cannot tell which + // application the OS will hand the code to, and an MCP client has no + // need for one. + return errors.New("redirect_uri must use https, or http on a loopback address") + } +} diff --git a/internal/integration_tests/cimd_consent_test.go b/internal/integration_tests/cimd_consent_test.go new file mode 100644 index 000000000..93598c874 --- /dev/null +++ b/internal/integration_tests/cimd_consent_test.go @@ -0,0 +1,172 @@ +package integration_tests + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/clientmetadata" + "github.com/authorizerdev/authorizer/internal/constants" +) + +// TestCIMDConsentFlow covers ONE property: that a client_id which cannot be +// resolved is refused outright rather than falling through to the +// AllowedOrigins check, which would give an unresolvable client a laxer +// redirect_uri check than a resolved one. +// +// It is deliberately narrow. The end-to-end consent flow — page shown, decision +// honoured, code issued or access_denied returned — is covered by +// TestCIMDConsentEndToEnd in cimd_flow_test.go, which serves a real TLS document +// host. An earlier version of this comment claimed that coverage while the test +// only asserted the refusal, which is the kind of overstatement that makes a +// suite look stronger than it is. +func TestCIMDConsentFlow(t *testing.T) { + cfg := getTestConfig() + cfg.AuthorizerURL = "https://auth.example.com" + cfg.EnableClientIDMetadataDocument = true + ts := initTestSetup(t, cfg) + + // No document host is stood up: app.example.com does not resolve, so + // resolution fails and the refusal below is what is under test. The resolved + // path needs a real TLS host and lives in cimd_flow_test.go. + const clientID = "https://app.example.com/client.json" + const redirectURI = "http://localhost:3000/callback" + + router := gin.New() + // Only the consent template: LoadHTMLGlob would also parse app.tmpl, which + // uses the `json` FuncMap the real router registers and this one does not. + router.LoadHTMLFiles( + "../../web/templates/consent.tmpl", + "../../web/templates/consent_error.tmpl", + // The shared shell defines au_styles/au_brand; without it the pages + // render EMPTY rather than failing loudly. + "../../web/templates/au_shell.tmpl", + ) + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + router.POST("/authorize/consent", ts.HttpProvider.ConsentHandler()) + + authorizeQuery := func(state string) url.Values { + qs := url.Values{} + qs.Set("response_type", "code") + qs.Set("client_id", clientID) + qs.Set("redirect_uri", redirectURI) + qs.Set("state", state) + qs.Set("response_mode", "query") + qs.Set("scope", "openid") + qs.Set("code_challenge", s256Challenge("a-verifier-long-enough-to-be-valid-00000000000")) + qs.Set("code_challenge_method", "S256") + return qs + } + + _, sessionToken := mcpSession(t, ts, []string{"openid"}) + + t.Run("an unresolvable client_id is refused, not silently downgraded", func(t *testing.T) { + // app.example.com does not serve a document, so resolution fails. The + // request must be rejected rather than falling through to the + // AllowedOrigins check — falling through would give an unresolvable + // client a LAXER redirect_uri check than a resolved one. + w := doAuthorizeGET(router, authorizeQuery("st"), sessionToken) + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + + var body map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, "invalid_client", body["error"]) + }) + + t.Run("IsMetadataClientID gates the whole path", func(t *testing.T) { + // The discriminator between registry lookup and document fetch. An + // ordinary client_id must never trigger an outbound request. + assert.True(t, clientmetadata.IsMetadataClientID(clientID)) + assert.False(t, clientmetadata.IsMetadataClientID(cfg.ClientID)) + }) +} + +// TestConsentHandlerSecurityProperties pins the properties that make splitting +// one authorization across two requests safe. Each is asserted on its own +// because each closes a distinct hole. +func TestConsentHandlerSecurityProperties(t *testing.T) { + cfg := getTestConfig() + cfg.EnableClientIDMetadataDocument = true + ts := initTestSetup(t, cfg) + + router := gin.New() + router.LoadHTMLFiles( + "../../web/templates/consent.tmpl", + "../../web/templates/consent_error.tmpl", + // The shared shell defines au_styles/au_brand; without it the pages + // render EMPTY rather than failing loudly. + "../../web/templates/au_shell.tmpl", + ) + router.POST("/authorize/consent", ts.HttpProvider.ConsentHandler()) + + post := func(form url.Values, sessionToken string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/authorize/consent", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if sessionToken != "" { + req.AddCookie(&http.Cookie{Name: constants.AppCookieName + "_session", Value: sessionToken}) + } + router.ServeHTTP(w, req) + return w + } + + t.Run("a missing consent_id is rejected", func(t *testing.T) { + assert.Equal(t, http.StatusBadRequest, post(url.Values{"action": {"approve"}}, "").Code) + }) + + t.Run("an unknown consent_id is rejected", func(t *testing.T) { + // Expired, already used and never-existed are deliberately + // indistinguishable, so this is not an oracle for guessing ids. + f := url.Values{"consent_id": {uuid.NewString()}, "action": {"approve"}} + w := post(f, "") + require.Equal(t, http.StatusBadRequest, w.Code) + // Asserted on the text a PERSON sees: this endpoint is only ever reached + // by a browser submitting a form this server rendered, so the response is + // a page, not a machine-readable error body. + assert.Contains(t, w.Body.String(), "already been used") + }) + + t.Run("a consent is single-use", func(t *testing.T) { + // A replayed approval must not mint a second authorization code, so the + // pending record is removed before the decision is acted on. + id := uuid.NewString() + payload := `{"client_id":"https://app.example.com/c.json","client_name":"n",` + + `"redirect_uri":"http://localhost:3000/cb","query":"state=x","user_id":"someone"}` + require.NoError(t, ts.MemoryStoreProvider.SetState("cimd_consent:"+id, payload)) + + // First use: rejected on the session check (no cookie), but it must + // still have consumed the record. + first := post(url.Values{"consent_id": {id}, "action": {"approve"}}, "") + assert.Equal(t, http.StatusUnauthorized, first.Code) + + second := post(url.Values{"consent_id": {id}, "action": {"approve"}}, "") + require.Equal(t, http.StatusBadRequest, second.Code) + assert.Contains(t, second.Body.String(), "already been used", + "the pending consent must be consumed on first use, whatever the outcome") + }) + + t.Run("a consent approved by a different session is refused", func(t *testing.T) { + // Without this, a consent page rendered for one user could be submitted + // in another user's browser and mint a code against their account. + _, otherSession := mcpSession(t, ts, []string{"openid"}) + id := uuid.NewString() + payload := `{"client_id":"https://app.example.com/c.json","client_name":"n",` + + `"redirect_uri":"http://localhost:3000/cb","query":"state=x","user_id":"a-different-user"}` + require.NoError(t, ts.MemoryStoreProvider.SetState("cimd_consent:"+id, payload)) + + w := post(url.Values{"consent_id": {id}, "action": {"approve"}}, otherSession) + require.Equal(t, http.StatusUnauthorized, w.Code) + assert.Contains(t, w.Body.String(), "sign in again", + "the refusal must be legible to the person in front of the screen") + assert.NotContains(t, w.Body.String(), "a-different-user", + "the page must not disclose whose consent it was") + }) +} diff --git a/internal/integration_tests/cimd_flow_test.go b/internal/integration_tests/cimd_flow_test.go new file mode 100644 index 000000000..e8dd1f5e1 --- /dev/null +++ b/internal/integration_tests/cimd_flow_test.go @@ -0,0 +1,264 @@ +package integration_tests + +import ( + "fmt" + "io" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +// TestCIMDConsentEndToEnd drives the whole consent flow over real HTTP with a +// cookie jar: /authorize → consent page → POST the decision → resumed +// /authorize → the redirect the client receives. +// +// This is deliberately NOT a browser test. The consent page is plain HTML with +// no JavaScript, so a browser adds nothing to the property that matters — +// "approving issues a code to the registered redirect URI, declining does not" — +// which is entirely determined by HTTP status, cookies and Location headers. +// +// It also avoids a real problem the browser attempt hit: the flow ends in a +// cross-origin redirect to an https host whose certificate is signed by a +// throwaway CA, and Chromium cancels that navigation even with +// ignoreHTTPSErrors and --ignore-certificate-errors set. That fight is with the +// test fixture, not the feature. Here the redirect is simply read, not followed. +func TestCIMDConsentEndToEnd(t *testing.T) { + cfg := getTestConfig() + cfg.EnableClientIDMetadataDocument = true + ts := initTestSetup(t, cfg) + require.NotNil(t, ts.ClientMetadataProvider, "the feature must be wired, or this test proves nothing") + + // The client's metadata document, served over TLS because the spec requires + // an https client_id. The resolver is pointed at this server's client so the + // self-signed certificate is trusted without weakening anything in + // production — see SetHTTPClientForTest. + var docURL string + docSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "mismatched") { + // Claims to be /client.json while being served at /mismatched.json — + // the impersonation case the spec's identity check exists to catch. + _, _ = fmt.Fprintf(w, `{"client_id":"%s/client.json","client_name":"Impersonator","redirect_uris":["%s/callback"]}`, docURL, docURL) + return + } + // client_id MUST equal the URL this document is served from. + _, _ = fmt.Fprintf(w, `{"client_id":"%s%s","client_name":"E2E Test Client","redirect_uris":["%s/callback"],"token_endpoint_auth_method":"none"}`, docURL, r.URL.Path, docURL) + })) + defer docSrv.Close() + docURL = docSrv.URL + clientID := docURL + "/client.json" + redirectURI := docURL + "/callback" + ts.ClientMetadataProvider.SetHTTPClientForTest(docSrv.Client()) + + router := gin.New() + router.LoadHTMLFiles( + "../../web/templates/consent.tmpl", + "../../web/templates/consent_error.tmpl", + // The shared shell defines au_styles/au_brand; without it the pages + // render EMPTY rather than failing loudly. + "../../web/templates/au_shell.tmpl", + ) + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + router.POST("/authorize/consent", ts.HttpProvider.ConsentHandler()) + srv := httptest.NewServer(router) + defer srv.Close() + + _, sessionToken := mcpSession(t, ts, []string{"openid"}) + + // A jar so the session cookie rides along exactly as a browser would send + // it — the flow spans four requests and breaks without it. + jar, err := cookiejar.New(nil) + require.NoError(t, err) + client := &http.Client{ + Jar: jar, + // Do NOT follow redirects: the final Location IS the assertion, and + // following it would leave the test depending on the mock host. + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + base, err := url.Parse(srv.URL) + require.NoError(t, err) + jar.SetCookies(base, []*http.Cookie{{Name: constants.AppCookieName + "_session", Value: sessionToken}}) + + authorizeURL := func(cid string) string { + q := url.Values{} + q.Set("response_type", "code") + q.Set("client_id", cid) + q.Set("redirect_uri", redirectURI) + q.Set("scope", "openid") + q.Set("state", "st-1") + q.Set("response_mode", "query") + q.Set("code_challenge", s256Challenge("a-verifier-long-enough-to-be-valid-00000000000")) + q.Set("code_challenge_method", "S256") + return srv.URL + "/authorize?" + q.Encode() + } + + consentIDRe := regexp.MustCompile(`name="consent_id" value="([^"]+)"`) + + // Reaching the consent page is the precondition for both decisions, so it is + // a helper rather than a separate case. + reachConsent := func(t *testing.T) string { + t.Helper() + resp, err := client.Get(authorizeURL(clientID)) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + body := readAll(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, "expected the consent page: %s", body) + assert.Contains(t, body, "E2E Test Client", "the page must name the client from its document") + assert.Contains(t, body, strings.TrimPrefix(docURL, "https://"), + "the page must show the redirect host — the only verified fact about a self-asserted client") + m := consentIDRe.FindStringSubmatch(body) + require.Len(t, m, 2, "the page must carry a consent_id") + return m[1] + } + + decide := func(t *testing.T, consentID, action string) *http.Response { + t.Helper() + form := url.Values{"consent_id": {consentID}, "action": {action}} + resp, err := client.PostForm(srv.URL+"/authorize/consent", form) + require.NoError(t, err) + return resp + } + + t.Run("approving issues a code to the registered redirect", func(t *testing.T) { + consentID := reachConsent(t) + + resp := decide(t, consentID, "approve") + defer func() { _ = resp.Body.Close() }() + require.Equal(t, http.StatusFound, resp.StatusCode, "approval must redirect back to /authorize") + + // Follow the one hop back into /authorize; its Location is what the + // client actually receives. + resumed, err := client.Get(srv.URL + resp.Header.Get("Location")) + require.NoError(t, err) + defer func() { _ = resumed.Body.Close() }() + require.Equal(t, http.StatusFound, resumed.StatusCode, + "the resumed authorization must redirect to the client, body: %s", readAll(t, resumed)) + + loc, err := url.Parse(resumed.Header.Get("Location")) + require.NoError(t, err) + assert.Equal(t, redirectURI, loc.Scheme+"://"+loc.Host+loc.Path, + "the code must go to the redirect_uri from the client's own document") + assert.NotEmpty(t, loc.Query().Get("code"), "approval must produce an authorization code") + assert.Equal(t, "st-1", loc.Query().Get("state")) + }) + + t.Run("PKCE is required for a metadata-document client", func(t *testing.T) { + // A CIMD client is a public client by construction — the spec's own + // example sets token_endpoint_auth_method "none" — so RFC 9700 §2.1.1 + // ("Public clients MUST use PKCE") applies to it exactly as it does to a + // client that registered itself. Asserted here because every other case + // in this file supplies PKCE, which would leave the CIMD arm of the check + // passing for the wrong reason. + q := url.Values{} + q.Set("response_type", "code") + q.Set("client_id", clientID) + q.Set("redirect_uri", redirectURI) + q.Set("scope", "openid") + q.Set("state", "st-nopkce") + q.Set("response_mode", "query") + + resp, err := client.Get(srv.URL + "/authorize?" + q.Encode()) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + body := readAll(t, resp) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, + "a public client with no code_challenge must be refused at /authorize: %s", body) + assert.Contains(t, body, "code_challenge is required") + assert.NotContains(t, body, "consent_id", + "the refusal must come before the user is asked to approve anything") + }) + + t.Run("declining redirects with access_denied and no code", func(t *testing.T) { + consentID := reachConsent(t) + + resp := decide(t, consentID, "deny") + defer func() { _ = resp.Body.Close() }() + require.Equal(t, http.StatusFound, resp.StatusCode) + + // RFC 6749 §4.1.2.1: the refusal goes to the client, which is blocked on + // its callback — not to a page only the user sees. + loc, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err) + assert.Equal(t, "access_denied", loc.Query().Get("error")) + assert.Empty(t, loc.Query().Get("code"), "a refusal must not also mint a code") + }) + + t.Run("prompt=none returns consent_required instead of showing the page", func(t *testing.T) { + // OIDC Core §3.1.2.1: prompt=none forbids displaying any authentication + // OR CONSENT user interface. A self-asserted client requires consent, so + // the two cannot both be satisfied and the request must fail to the + // client rather than render a page it asked not to see. + // + // The pre-existing prompt=none guards only cover the unauthenticated + // case, so this is reachable with a perfectly valid session. + u := authorizeURL(clientID) + "&prompt=none" + resp, err := client.Get(u) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + require.Equal(t, http.StatusFound, resp.StatusCode, + "the error belongs at the client's redirect_uri, body: %s", readAll(t, resp)) + loc, pErr := url.Parse(resp.Header.Get("Location")) + require.NoError(t, pErr) + assert.Equal(t, "consent_required", loc.Query().Get("error")) + assert.Empty(t, loc.Query().Get("code"), "a refused request must not mint a code") + }) + + t.Run("a grant does not authorize a different request", func(t *testing.T) { + // The grant marker is keyed to the exact parameter set that was shown. + // + // Keyed on (user, client) alone, a grant that was never redeemed — tab + // closed, browser back, network drop — would sit in the store for its + // whole TTL and then satisfy ANY later /authorize for that pair: a wider + // scope, a different redirect_uri from the document's list, a different + // PKCE challenge. The user approves one request and a materially + // different one executes, which is precisely what storing the full + // parameter set exists to prevent. + consentID := reachConsent(t) + resp := decide(t, consentID, "approve") + defer func() { _ = resp.Body.Close() }() + require.Equal(t, http.StatusFound, resp.StatusCode) + + // Deliberately do NOT follow the redirect: the grant is now outstanding, + // exactly as it would be if the user's browser never completed the hop. + // A benign parameter change: it alters the request the user would be + // consenting to without diverting the flow (prompt=login, for instance, + // would force re-authentication and never reach the gate). + widened := authorizeURL(clientID) + "&login_hint=someone-else%40example.com" + other, err := client.Get(widened) + require.NoError(t, err) + defer func() { _ = other.Body.Close() }() + + require.Equal(t, http.StatusOK, other.StatusCode, + "a different request must be shown consent again, not silently approved") + assert.Contains(t, readAll(t, other), "consent_id", + "the outstanding grant must not authorize a request the user never saw") + }) + + t.Run("a document whose client_id does not match its URL is refused", func(t *testing.T) { + // Never reaches consent: the client cannot be established, so there is + // nothing honest to show the user. + resp, err := client.Get(authorizeURL(docURL + "/mismatched.json")) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Contains(t, readAll(t, resp), "invalid_client") + }) +} + +// readAll returns a response body as a string for assertion messages. +func readAll(t *testing.T, resp *http.Response) string { + t.Helper() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return string(b) +} diff --git a/internal/integration_tests/dcr_flow_test.go b/internal/integration_tests/dcr_flow_test.go new file mode 100644 index 000000000..eb0967d5e --- /dev/null +++ b/internal/integration_tests/dcr_flow_test.go @@ -0,0 +1,499 @@ +package integration_tests + +import ( + "encoding/json" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +const dcrVerifier = "a-verifier-long-enough-to-be-valid-00000000000" + +// dcrRouter mounts the routes a dynamically registered client actually uses. +func dcrRouter(ts *testSetup) *gin.Engine { + router := gin.New() + router.LoadHTMLFiles( + "../../web/templates/consent.tmpl", + "../../web/templates/consent_error.tmpl", + // The shared shell defines au_styles/au_brand; without it the pages + // render EMPTY rather than failing loudly. + "../../web/templates/au_shell.tmpl", + ) + router.POST("/oauth/register", ts.HttpProvider.RegisterClientHandler()) + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + router.POST("/authorize/consent", ts.HttpProvider.ConsentHandler()) + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + return router +} + +// registerClient POSTs an RFC 7591 registration and returns the decoded body. +func registerClient(t *testing.T, router http.Handler, body string) (int, map[string]any) { + t.Helper() + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/oauth/register", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + router.ServeHTTP(w, req) + var out map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &out) + return w.Code, out +} + +// TestDynamicClientRegistrationEndToEnd drives the whole RFC 7591 path: a client +// registers itself, is sent through consent because nobody vouched for it, and +// redeems the resulting code as a public client using PKCE alone. +// +// The point of running it end to end rather than per-handler is that the parts +// only work together: registration writes a row whose Kind drives the consent +// gate, and whose empty secret is what makes the PKCE-only token exchange legal. +func TestDynamicClientRegistrationEndToEnd(t *testing.T) { + cfg := getTestConfig() + cfg.EnableDynamicClientRegistration = true + ts := initTestSetup(t, cfg) + + router := dcrRouter(ts) + srv := httptest.NewServer(router) + defer srv.Close() + + redirectURI := "http://127.0.0.1:5599/callback" + + code, body := registerClient(t, router, + `{"client_name":"Dynamic Test Client","redirect_uris":["`+redirectURI+`"],"token_endpoint_auth_method":"none"}`) + require.Equal(t, http.StatusCreated, code, "registration must return 201 Created: %v", body) + clientID, _ := body["client_id"].(string) + require.NotEmpty(t, clientID, "registration must issue a client_id") + + t.Run("the response is a public client and carries no secret", func(t *testing.T) { + // RFC 7591 §3.2.1. A secret here would mean an anonymous caller had just + // created a CONFIDENTIAL client, which is the thing this endpoint must + // never do. + assert.Equal(t, "none", body["token_endpoint_auth_method"]) + assert.NotContains(t, body, "client_secret") + assert.NotContains(t, body, "client_secret_expires_at") + assert.NotEmpty(t, body["client_id_issued_at"]) + assert.NotEqual(t, "Dynamic Test Client", clientID, + "the client_id must be server-generated, never derived from caller-supplied metadata") + }) + + t.Run("the registered client is stored as self-registered", func(t *testing.T) { + // The Kind is what routes this client through consent later; if it were + // stored as an ordinary interactive client the consent gate would be + // silently skipped and the rest of this test would still pass. + stored, err := ts.StorageProvider.GetClientByClientID(t.Context(), clientID) + require.NoError(t, err) + require.NotNil(t, stored) + assert.Equal(t, constants.ClientKindDynamic, stored.Kind) + assert.Empty(t, stored.ClientSecret, "a dynamically registered client must be public") + assert.True(t, stored.IsActive) + }) + + authorizeQuery := func() url.Values { + q := url.Values{} + q.Set("response_type", "code") + q.Set("client_id", clientID) + q.Set("redirect_uri", redirectURI) + q.Set("scope", "openid") + q.Set("state", "dcr-state") + q.Set("response_mode", "query") + q.Set("code_challenge", s256Challenge(dcrVerifier)) + q.Set("code_challenge_method", "S256") + return q + } + + _, sessionToken := mcpSession(t, ts, []string{"openid"}) + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + client := &http.Client{ + Jar: jar, + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + base, err := url.Parse(srv.URL) + require.NoError(t, err) + jar.SetCookies(base, []*http.Cookie{{Name: constants.AppCookieName + "_session", Value: sessionToken}}) + + consentIDRe := regexp.MustCompile(`name="consent_id" value="([^"]+)"`) + + t.Run("a self-registered client is sent through consent", func(t *testing.T) { + // RFC 7591 §5: "a rogue client might use the name and logo of a + // legitimate client", so servers should "present warning messages to + // end-users about dynamically registered clients". The name below was + // chosen by the caller and verified by nobody. + resp, gErr := client.Get(srv.URL + "/authorize?" + authorizeQuery().Encode()) + require.NoError(t, gErr) + defer func() { _ = resp.Body.Close() }() + page := readAll(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, "expected the consent page: %s", page) + assert.Contains(t, page, "Dynamic Test Client", "the page must name the client it registered as") + assert.Contains(t, page, "127.0.0.1:5599", + "the page must show the redirect host — the only verified fact about a self-asserted client") + assert.Contains(t, page, "runs on your own computer", + "a loopback-only client must carry the impersonation warning") + + // The page's CSP must permit the form submission to end at the client's + // redirect_uri. This is not a hardening nicety — it is what makes the + // button work at all. + // + // Approving POSTs to /authorize/consent, which redirects to /authorize, + // which redirects to the client's redirect_uri. A browser enforces + // form-action across that WHOLE redirect chain, so the default + // `form-action 'self'` aborts the navigation at the last hop: the user + // stays on the consent page, assumes the click missed, clicks again, and + // the second POST fails with "expired or already used" because the first + // one already consumed the consent. That is precisely what a real user + // hit, and no test caught it because every automated check either drove + // the redirects itself or asserted the Location header instead of + // letting a browser follow it. + csp := resp.Header.Get("Content-Security-Policy") + require.NotEmpty(t, csp, "the consent page must still carry a CSP") + assert.Contains(t, csp, "http://127.0.0.1:5599", + "form-action must allow the redirect_uri being approved, or the browser blocks the redirect after approval") + assert.NotContains(t, csp, "form-action 'self';", + "form-action 'self' alone silently breaks approval for any off-origin redirect_uri") + }) + + t.Run("approving issues a code redeemable with PKCE and no secret", func(t *testing.T) { + resp, gErr := client.Get(srv.URL + "/authorize?" + authorizeQuery().Encode()) + require.NoError(t, gErr) + defer func() { _ = resp.Body.Close() }() + m := consentIDRe.FindStringSubmatch(readAll(t, resp)) + require.Len(t, m, 2, "the page must carry a consent_id") + + approved, pErr := client.PostForm(srv.URL+"/authorize/consent", + url.Values{"consent_id": {m[1]}, "action": {"approve"}}) + require.NoError(t, pErr) + defer func() { _ = approved.Body.Close() }() + require.Equal(t, http.StatusFound, approved.StatusCode) + + resumed, rErr := client.Get(srv.URL + approved.Header.Get("Location")) + require.NoError(t, rErr) + defer func() { _ = resumed.Body.Close() }() + require.Equal(t, http.StatusFound, resumed.StatusCode, + "the resumed authorization must redirect to the client: %s", readAll(t, resumed)) + + loc, lErr := url.Parse(resumed.Header.Get("Location")) + require.NoError(t, lErr) + authCode := loc.Query().Get("code") + require.NotEmpty(t, authCode, "approval must produce an authorization code") + + // The exchange that proves the whole point: no client_secret anywhere, + // PKCE alone binding the code to the instance that started the flow. + form := url.Values{ + "grant_type": {"authorization_code"}, + "code": {authCode}, + "redirect_uri": {redirectURI}, + "client_id": {clientID}, + "code_verifier": {dcrVerifier}, + } + tokenResp, tErr := http.Post(srv.URL+"/oauth/token", + "application/x-www-form-urlencoded", strings.NewReader(form.Encode())) + require.NoError(t, tErr) + defer func() { _ = tokenResp.Body.Close() }() + tokenBody := readAll(t, tokenResp) + require.Equal(t, http.StatusOK, tokenResp.StatusCode, + "a public client must redeem its code with PKCE alone: %s", tokenBody) + assert.Contains(t, tokenBody, "access_token") + }) + + t.Run("PKCE is required, not optional", func(t *testing.T) { + // RFC 9700 §2.1.1 "Public clients MUST use PKCE"; OAuth 2.1 §4.1.1 has + // the authorization server MUST enforce it. Rejected at /authorize rather + // than at the token endpoint so the user is never asked to log in and + // approve a request that was never completable. + q := authorizeQuery() + q.Del("code_challenge") + q.Del("code_challenge_method") + w := doAuthorizeGET(router, q, sessionToken) + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "code_challenge is required") + }) + + t.Run("a public client can refresh with client_id alone", func(t *testing.T) { + // Not hypothetical: Claude Code registers grant_types + // ["authorization_code","refresh_token"], so this path runs on every + // long-lived connection. RFC 6749 §6 has a public client refresh with its + // client_id and no secret, and the resource binding must survive the + // rotation — an earlier bug in this area dropped the RFC 8707 resource on + // refresh, which killed MCP connections at the first token rotation + // rather than at connect time. + q := authorizeQuery() + q.Set("scope", "openid offline_access") + q.Set("resource", "http://localhost:8099/mcp") + q.Set("state", "dcr-refresh") + + // Consent first — a self-registered client always passes through it. + page, gErr := client.Get(srv.URL + "/authorize?" + q.Encode()) + require.NoError(t, gErr) + defer func() { _ = page.Body.Close() }() + m := consentIDRe.FindStringSubmatch(readAll(t, page)) + require.Len(t, m, 2) + + approved, pErr := client.PostForm(srv.URL+"/authorize/consent", + url.Values{"consent_id": {m[1]}, "action": {"approve"}}) + require.NoError(t, pErr) + defer func() { _ = approved.Body.Close() }() + resumed, rErr := client.Get(srv.URL + approved.Header.Get("Location")) + require.NoError(t, rErr) + defer func() { _ = resumed.Body.Close() }() + loc, lErr := url.Parse(resumed.Header.Get("Location")) + require.NoError(t, lErr) + authCode := loc.Query().Get("code") + require.NotEmpty(t, authCode, "body: %s", readAll(t, resumed)) + + exchange := url.Values{ + "grant_type": {"authorization_code"}, + "code": {authCode}, + "redirect_uri": {redirectURI}, + "client_id": {clientID}, + "code_verifier": {dcrVerifier}, + "resource": {"http://localhost:8099/mcp"}, + } + tr, tErr := http.Post(srv.URL+"/oauth/token", + "application/x-www-form-urlencoded", strings.NewReader(exchange.Encode())) + require.NoError(t, tErr) + defer func() { _ = tr.Body.Close() }() + var tok map[string]any + require.NoError(t, json.Unmarshal([]byte(readAll(t, tr)), &tok)) + require.Equal(t, http.StatusOK, tr.StatusCode, "body: %v", tok) + refreshToken, _ := tok["refresh_token"].(string) + require.NotEmpty(t, refreshToken, "offline_access must yield a refresh token, or this test proves nothing") + + // The refresh itself: client_id only, no secret. + refresh := url.Values{ + "grant_type": {"refresh_token"}, + "refresh_token": {refreshToken}, + "client_id": {clientID}, + } + rr, rfErr := http.Post(srv.URL+"/oauth/token", + "application/x-www-form-urlencoded", strings.NewReader(refresh.Encode())) + require.NoError(t, rfErr) + defer func() { _ = rr.Body.Close() }() + var rotated map[string]any + require.NoError(t, json.Unmarshal([]byte(readAll(t, rr)), &rotated)) + require.Equal(t, http.StatusOK, rr.StatusCode, + "a public client must refresh with client_id alone: %v", rotated) + + access, _ := rotated["access_token"].(string) + require.NotEmpty(t, access) + claims, cErr := ts.TokenProvider.ParseJWTToken(access) + require.NoError(t, cErr) + assert.Equal(t, "http://localhost:8099/mcp", claims["aud"], + "the rotated token must stay bound to the resource the user consented to") + }) + + t.Run("an implicit response type is refused", func(t *testing.T) { + // OAuth 2.1 removes the implicit grant and MCP mandates OAuth 2.1. + // Independently: implicit delivers a bearer token into the URL fragment + // with nothing binding it to the requester, which for a client nobody + // vouched for is the worst available combination. It is also what the + // client itself registered — response_types ["code"] — so this is + // enforcement, not a new restriction. + q := authorizeQuery() + q.Set("response_type", "id_token token") + q.Set("response_mode", "fragment") + w := doAuthorizeGET(router, q, sessionToken) + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "unsupported_response_type") + }) + + t.Run("PKCE plain is refused even outside strict mode", func(t *testing.T) { + // "plain" carries the verifier in the same request as the challenge, so + // it protects nothing against an attacker who can read that request — + // which is exactly the threat model for a client with no secret. + require.False(t, cfg.OAuth21Strict, "this case is only meaningful with strict mode off") + q := authorizeQuery() + q.Set("code_challenge", "a-plain-challenge-value-000000000000000000000") + q.Set("code_challenge_method", "plain") + w := doAuthorizeGET(router, q, sessionToken) + require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "must be S256") + }) +} + +// TestDynamicClientRegistrationRefusals pins what the endpoint will not accept. +// Each case is a way an anonymous caller could otherwise widen what it gets. +func TestDynamicClientRegistrationRefusals(t *testing.T) { + cfg := getTestConfig() + cfg.EnableDynamicClientRegistration = true + ts := initTestSetup(t, cfg) + router := dcrRouter(ts) + + cases := []struct { + name, body, wantError, why string + }{ + { + name: "a confidential client may not self-register", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb"],"token_endpoint_auth_method":"client_secret_basic"}`, + wantError: "invalid_client_metadata", + why: "issuing a secret to an anonymous caller creates a confidential client nobody vouched for", + }, + { + name: "the RFC default auth method is refused, not honoured", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb"],"grant_types":["authorization_code"]}`, + wantError: "", + why: "RFC 7591 §2 defaults the omitted field to client_secret_basic; omitted must mean public here", + }, + { + name: "client_credentials may not be registered", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb"],"grant_types":["client_credentials"]}`, + wantError: "invalid_client_metadata", + why: "it issues a token with no user and no consent", + }, + { + name: "redirect_uris is required", + body: `{"client_name":"c"}`, + wantError: "invalid_redirect_uri", + why: "every grant accepted here is redirect-based", + }, + { + name: "a non-loopback http redirect is refused", + body: `{"client_name":"c","redirect_uris":["http://app.example.com/cb"]}`, + wantError: "invalid_redirect_uri", + why: "MCP requires redirect URIs to be localhost or https; otherwise codes cross the network in the clear", + }, + { + name: "a custom scheme is refused", + body: `{"client_name":"c","redirect_uris":["myapp://callback"]}`, + wantError: "invalid_redirect_uri", + why: "the server cannot tell which local application the OS will hand the code to", + }, + { + name: "a fragment is refused", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb#x"]}`, + wantError: "invalid_redirect_uri", + why: "RFC 6749 §3.1.2: the endpoint URI MUST NOT include a fragment", + }, + { + name: "a comma is refused", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb?a=1,2"]}`, + wantError: "invalid_redirect_uri", + why: "comma is the storage encoding for the list, so one would split into two registered URIs", + }, + { + name: "a non-code response type is refused", + body: `{"client_name":"c","redirect_uris":["https://app.example.com/cb"],"response_types":["token"]}`, + wantError: "invalid_client_metadata", + why: "silently narrowing to code would surface only when the client's callback got something it cannot parse", + }, + { + name: "a non-object body is refused", + body: `"not-an-object"`, + wantError: "invalid_client_metadata", + why: "the endpoint is unauthenticated; it must not fall through on malformed input", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status, body := registerClient(t, router, tc.body) + if tc.wantError == "" { + // The "omitted auth method" case: accepted, but pinned to public. + require.Equal(t, http.StatusCreated, status, "%s — body: %v", tc.why, body) + assert.Equal(t, "none", body["token_endpoint_auth_method"], tc.why) + return + } + require.Equal(t, http.StatusBadRequest, status, "%s — body: %v", tc.why, body) + assert.Equal(t, tc.wantError, body["error"], tc.why) + }) + } +} + +// TestDynamicClientRegistrationDisabled pins that the feature is inert when off. +func TestDynamicClientRegistrationDisabled(t *testing.T) { + cfg := getTestConfig() + // Deliberately NOT setting EnableDynamicClientRegistration. + ts := initTestSetup(t, cfg) + + t.Run("the handler refuses even if it is somehow routed", func(t *testing.T) { + // The route is only mounted when the flag is on, so this is defence in + // depth. It is asserted because an unauthenticated write endpoint is the + // wrong place to depend on registration order staying correct. + router := gin.New() + router.POST("/oauth/register", ts.HttpProvider.RegisterClientHandler()) + status, body := registerClient(t, router, `{"redirect_uris":["https://app.example.com/cb"]}`) + assert.Equal(t, http.StatusNotFound, status, "body: %v", body) + }) + + t.Run("registration_endpoint is not advertised", func(t *testing.T) { + // RFC 8414 §2 treats an omitted field as "not supported". Advertising an + // endpoint that is not mounted would send clients into a 404 at the one + // step they cannot recover from. + w := httptest.NewRecorder() + r := gin.New() + r.GET("/.well-known/openid-configuration", ts.HttpProvider.OpenIDConfigurationHandler()) + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/.well-known/openid-configuration", nil)) + require.Equal(t, http.StatusOK, w.Code) + + var meta map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &meta)) + assert.NotContains(t, meta, "registration_endpoint") + }) +} + +// TestDynamicClientRegistrationAdvertised is the other half of the pair above: +// when enabled, discovery must point at the endpoint that is actually mounted. +func TestDynamicClientRegistrationAdvertised(t *testing.T) { + cfg := getTestConfig() + cfg.EnableDynamicClientRegistration = true + ts := initTestSetup(t, cfg) + + w := httptest.NewRecorder() + r := gin.New() + r.GET("/.well-known/openid-configuration", ts.HttpProvider.OpenIDConfigurationHandler()) + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/.well-known/openid-configuration", nil)) + require.Equal(t, http.StatusOK, w.Code) + + var meta map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &meta)) + // This exact field is what a client looks for before it will attempt DCR; + // its absence is the "does not support dynamic client registration" refusal. + assert.Contains(t, meta, "registration_endpoint") + endpoint, _ := meta["registration_endpoint"].(string) + assert.True(t, strings.HasSuffix(endpoint, "/oauth/register"), "got %q", endpoint) +} + +// TestOperatorRegisteredClientIsUnaffected is the backward-compatibility guard. +// +// Everything added for self-registered clients — the consent interstitial and +// mandatory S256 PKCE — must be invisible to clients that already worked. An +// operator-created client was vouched for by a human, which is the entire basis +// for not interrupting its users, and it may legitimately predate PKCE. +func TestOperatorRegisteredClientIsUnaffected(t *testing.T) { + cfg := getTestConfig() + cfg.EnableDynamicClientRegistration = true + ts := initTestSetup(t, cfg) + + // The deployment's own reserved client: interactive, operator-configured. + _, sessionToken := mcpSession(t, ts, []string{"openid"}) + router := dcrRouter(ts) + + q := url.Values{} + q.Set("response_type", "code") + q.Set("client_id", cfg.ClientID) + // The reserved client registers no redirect URIs of its own, so it falls + // back to the AllowedOrigins allow-list — which is exactly the legacy path + // that must not regress. + q.Set("redirect_uri", "http://localhost:3000/callback") + q.Set("scope", "openid") + q.Set("state", "bc-state") + q.Set("response_mode", "query") + // No code_challenge at all — the case that must keep working. + + w := doAuthorizeGET(router, q, sessionToken) + require.Equal(t, http.StatusFound, w.Code, + "an operator-registered client must still complete without PKCE and without consent: %s", w.Body.String()) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + assert.NotEmpty(t, loc.Query().Get("code"), "the pre-existing flow must still mint a code") + assert.NotContains(t, w.Body.String(), "consent_id", + "an operator-registered client must never be sent through the consent screen") +} diff --git a/internal/integration_tests/oauth_standards_compliance_test.go b/internal/integration_tests/oauth_standards_compliance_test.go index 60d6446f2..cf103cfad 100644 --- a/internal/integration_tests/oauth_standards_compliance_test.go +++ b/internal/integration_tests/oauth_standards_compliance_test.go @@ -125,9 +125,14 @@ func TestOpenIDDiscoveryCompliance(t *testing.T) { t.Run("OIDC_Discovery_registration_endpoint_absent", func(t *testing.T) { // We previously advertised registration_endpoint=/app, which is the // signup UI, not an RFC 7591 dynamic client registration endpoint. - // Spec-compliant OIDC clients interpret this field as RFC 7591 - // and will fail when they receive HTML. Until we actually implement - // RFC 7591, the field MUST be absent. + // Spec-compliant OIDC clients interpret this field as RFC 7591 and will + // fail when they receive HTML. + // + // RFC 7591 IS implemented now, but it is opt-in and this config leaves + // it off — so the field must still be absent here. That is the property + // under test today: advertising an endpoint that is not mounted sends + // clients into a 404 at the one step they cannot recover from. The + // enabled case is asserted by TestDynamicClientRegistrationAdvertised. w := httptest.NewRecorder() req, _ := http.NewRequest("GET", "/.well-known/openid-configuration", nil) req.Host = "localhost" @@ -142,7 +147,7 @@ func TestOpenIDDiscoveryCompliance(t *testing.T) { _, present := body["registration_endpoint"] assert.False(t, present, - "registration_endpoint MUST NOT be advertised until RFC 7591 is implemented") + "registration_endpoint MUST NOT be advertised while dynamic client registration is disabled") }) } diff --git a/internal/integration_tests/test_helper.go b/internal/integration_tests/test_helper.go index 55eb1e6e5..2df8123ef 100644 --- a/internal/integration_tests/test_helper.go +++ b/internal/integration_tests/test_helper.go @@ -22,6 +22,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/authenticators" "github.com/authorizerdev/authorizer/internal/authenticators/webauthn" + "github.com/authorizerdev/authorizer/internal/clientmetadata" "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/email" @@ -43,10 +44,13 @@ import ( type testSetup struct { GraphQLProvider graphql.Provider HttpProvider http_handlers.Provider - HttpServer *httptest.Server - Config *config.Config - Logger *zerolog.Logger - GinContext *gin.Context + // ClientMetadataProvider is exposed so a CIMD test can point the resolver at + // an httptest TLS server; nil unless the config enabled the feature. + ClientMetadataProvider *clientmetadata.Provider + HttpServer *httptest.Server + Config *config.Config + Logger *zerolog.Logger + GinContext *gin.Context // Used for specific tests where we need to access the storage StorageProvider storage.Provider MemoryStoreProvider memory_store.Provider @@ -343,7 +347,12 @@ func initTestSetup(t *testing.T, cfg *config.Config) *testSetup { // Create dependencies struct httpDeps := &http_handlers.Dependencies{ - Log: &logger, + Log: &logger, + // Built exactly as cmd/root.go does: nil unless the flag is on, so a + // test that does not opt in sees the feature switched off, and one that + // does gets the same wiring production has. + ClientMetadataProvider: newTestClientMetadataProvider(cfg, &logger), + AuditProvider: auditProvider, AuthenticatorProvider: authProvider, EmailProvider: emailProvider, @@ -391,19 +400,20 @@ func initTestSetup(t *testing.T, cfg *config.Config) *testSetup { }) return &testSetup{ - GraphQLProvider: gqlProvider, - HttpProvider: httpProvider, - HttpServer: server, - Config: cfg, - Logger: &logger, - GinContext: ctx, - StorageProvider: storageProvider, - MemoryStoreProvider: memoryStoreProvider, - AuthenticatorProvider: authProvider, - WebAuthnProvider: webAuthnProvider, - TokenProvider: tokenProvider, - ServiceProvider: serviceProvider, - DNSResolver: dnsResolver, + GraphQLProvider: gqlProvider, + HttpProvider: httpProvider, + ClientMetadataProvider: httpDeps.ClientMetadataProvider, + HttpServer: server, + Config: cfg, + Logger: &logger, + GinContext: ctx, + StorageProvider: storageProvider, + MemoryStoreProvider: memoryStoreProvider, + AuthenticatorProvider: authProvider, + WebAuthnProvider: webAuthnProvider, + TokenProvider: tokenProvider, + ServiceProvider: serviceProvider, + DNSResolver: dnsResolver, } } @@ -471,3 +481,12 @@ func latestMfaSessionCookie(s *testSetup) string { func newAdminSessionToken(ts *testSetup) (string, error) { return ts.TokenProvider.NewAdminSession() } + +// newTestClientMetadataProvider mirrors cmd/root.go: nil unless the flag is on, +// so a test that does not opt in sees the feature switched off. +func newTestClientMetadataProvider(cfg *config.Config, logger *zerolog.Logger) *clientmetadata.Provider { + if !cfg.EnableClientIDMetadataDocument { + return nil + } + return clientmetadata.New(logger, cfg.ClientIDMetadataAllowedDomains, cfg.Env == constants.E2EEnv) +} diff --git a/internal/server/http_routes.go b/internal/server/http_routes.go index 62ca3ed78..5e4f16c87 100644 --- a/internal/server/http_routes.go +++ b/internal/server/http_routes.go @@ -140,9 +140,23 @@ func (s *server) NewRouter() *gin.Engine { router.GET("/logout", s.Dependencies.HTTPProvider.LogoutHandler()) router.POST("/logout", s.Dependencies.HTTPProvider.LogoutHandler()) router.POST("/oauth/token", s.Dependencies.HTTPProvider.TokenHandler()) + // Consent decision for a self-asserted (CIMD) client. Registered + // unconditionally: the handler is only ever reached from a page this server + // rendered, and a 400 for an unknown consent_id is a better answer than a + // 404 that varies with configuration. + router.POST("/authorize/consent", s.Dependencies.HTTPProvider.ConsentHandler()) router.POST("/oauth/revoke", s.Dependencies.HTTPProvider.RevokeRefreshTokenHandler()) router.POST("/oauth/introspect", s.Dependencies.HTTPProvider.IntrospectHandler()) + // RFC 7591 dynamic client registration. Mounted ONLY when enabled, which + // matters more here than for a read-only endpoint: this one writes to the + // registry without authenticating the caller, so an unconfigured deployment + // must not have it reachable at all. `registration_endpoint` is advertised + // under the same condition, so discovery and reality cannot disagree. + if s.Dependencies.AppConfig != nil && s.Dependencies.AppConfig.EnableDynamicClientRegistration { + router.POST("/oauth/register", s.Dependencies.HTTPProvider.RegisterClientHandler()) + } + // Inbound SCIM 2.0 (per-org user provisioning). Its own route group with a // bearer-token auth middleware; the org is derived only from the token, so // there is no org segment in the path (design §4.4 H6). CSRF is exempted for diff --git a/internal/service/clientauth/clientauth.go b/internal/service/clientauth/clientauth.go index b593821fd..99bc6852d 100644 --- a/internal/service/clientauth/clientauth.go +++ b/internal/service/clientauth/clientauth.go @@ -16,6 +16,7 @@ import ( "github.com/rs/zerolog" "golang.org/x/crypto/bcrypt" + "github.com/authorizerdev/authorizer/internal/clientmetadata" "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/memory_store" @@ -137,6 +138,9 @@ type ResolveParams struct { type Dependencies struct { Log *zerolog.Logger StorageProvider storage.Provider + // ClientMetadataProvider resolves CIMD client_ids (HTTPS URLs). nil disables + // the path, which is the default. + ClientMetadataProvider *clientmetadata.Provider // MemoryStoreProvider backs the shared, cross-instance caches used by the // client_assertion path: the single-use jti/replay markers and the per-trust- // row JWKS cache. Optional for the secret-only paths. @@ -231,6 +235,48 @@ func (p *provider) ResolveClient(ctx context.Context, params ResolveParams) (*sc log.Debug().Msg("client_id missing") return nil, ErrMissingClientID } + // Client ID Metadata Document clients are not in the registry — their + // client_id IS the URL of a document describing them. Resolve and validate + // it, then represent the client synthetically for the rest of this request. + // + // They are PUBLIC clients by construction: the spec's own example sets + // token_endpoint_auth_method "none", and there is nowhere to put a secret + // that both sides would know. Two consequences are enforced here rather than + // assumed downstream: + // + // - client_credentials is refused outright (RequireSecret). A grant that + // issues a token with no user and no secret must never be reachable by a + // client that registered itself by hosting a JSON file. + // - a presented secret is refused rather than ignored. Accepting it would + // tell a caller their credential "worked" when nothing verified it. + // + // PKCE still gates the authorization_code exchange, unchanged: that is what + // binds the code to the client instance that started the flow. + if p.ClientMetadataProvider != nil && clientmetadata.IsMetadataClientIDFor(clientID, p.Config.ClientID) { + if params.RequireSecret || params.RequireServiceAccountKind { + log.Debug().Msg("metadata-document client is public; client_credentials is not available to it") + return nil, ErrUnauthorizedClient + } + if secret != "" { + log.Debug().Msg("metadata-document client presented a secret; it is a public client") + return nil, ErrInvalidClient + } + doc, dErr := p.ClientMetadataProvider.Resolve(ctx, clientID) + if dErr != nil { + log.Debug().Err(dErr).Msg("could not resolve client metadata document") + return nil, ErrInvalidClient + } + return &schemas.Client{ + ID: doc.ClientID, + ClientID: doc.ClientID, + Name: doc.ClientName, + Kind: constants.ClientKindInteractive, + TokenEndpointAuthMethod: constants.TokenEndpointAuthMethodNone, + RedirectURIs: strings.Join(doc.RedirectURIs, ","), + IsActive: true, + }, nil + } + secretPresented := secret != "" // doVerify decides whether the secret is checked at all: always for // client_credentials (RequireSecret), only-when-present for authorization_code @@ -282,6 +328,29 @@ func (p *provider) ResolveClient(ctx context.Context, params ResolveParams) (*sc return client, ErrUnauthorizedClient } + // A registered PUBLIC client must not be authenticated by a secret, whatever + // it presented. Without this the request still fails — bcrypt compares the + // presented secret against an empty stored hash and errors — so this changes + // the REASON rather than the outcome: the refusal becomes a declared property + // of the client's registration instead of an accident of the stored hash + // being empty. It also stops a caller being told their credential "worked" + // if a secret ever gets written to a row registered as public. + // + // Mirrors what the CIMD branch above does explicitly, and matches how Ory + // Hydra drives behaviour from the registered token_endpoint_auth_method + // rather than from what the caller chose to send. No RFC compels this — RFC + // 9700 does not cover it and OAuth 2.1 §2.4 only forbids using more than one + // method per request — so it is hygiene, not a compliance fix. + // + // The dummy compare preserves the cost of the real one, so this branch does + // not become a timing oracle distinguishing a public client from a + // confidential one with a wrong secret. + if secretPresented && client.TokenEndpointAuthMethod == constants.TokenEndpointAuthMethodNone { + log.Debug().Str("client_id", clientID).Msg("public client presented a secret") + performDummyCompare(secret) + return client, ErrInvalidClient + } + // bcrypt.CompareHashAndPassword is itself constant-time with respect to the // secret; running it before the IsActive check keeps a wrong-secret and an // inactive-account rejection timing-indistinguishable. diff --git a/internal/service/clientauth/clientauth_test.go b/internal/service/clientauth/clientauth_test.go index 7ef543a5a..0a3b4129f 100644 --- a/internal/service/clientauth/clientauth_test.go +++ b/internal/service/clientauth/clientauth_test.go @@ -184,6 +184,33 @@ func TestResolveClient_InactiveClient(t *testing.T) { require.NotNil(t, got) } +func TestResolveClient_PublicClientPresentingSecretIsRefused(t *testing.T) { + // A client REGISTERED as public must not be authenticated by a secret, + // whatever it sends. This already failed before the check existed — bcrypt + // compared the presented secret against an empty stored hash and errored — + // so what is pinned here is that the refusal is a declared property of the + // registration rather than an accident of the stored hash being empty. If a + // secret were ever written to a row registered as public, the accident would + // stop holding and this test is what notices. + public := &schemas.Client{ + ID: "id-public", + ClientID: "public-app", + Kind: constants.ClientKindDynamic, + TokenEndpointAuthMethod: constants.TokenEndpointAuthMethodNone, + ClientSecret: hashSecret(t, "a-secret-that-should-never-be-honoured"), + IsActive: true, + } + r := newResolver(t, map[string]*schemas.Client{"public-app": public}) + got, err := r.ResolveClient(context.Background(), ResolveParams{ + BodyClientID: "public-app", + BodySecret: "a-secret-that-should-never-be-honoured", // the CORRECT secret + VerifyPresentedSecret: true, + }) + assert.ErrorIs(t, err, ErrInvalidClient, + "a client registered with token_endpoint_auth_method=none must not authenticate with a secret, even a matching one") + require.NotNil(t, got, "the resolved client is still returned so the caller can attribute an audit event") +} + func TestResolveClient_PublicClientNoSecret(t *testing.T) { // A public client (token_endpoint_auth_method == "none") presents no secret; // authorization_code sets VerifyPresentedSecret=true but with no secret there diff --git a/web/templates/au_shell.tmpl b/web/templates/au_shell.tmpl new file mode 100644 index 000000000..0871d8474 --- /dev/null +++ b/web/templates/au_shell.tmpl @@ -0,0 +1,250 @@ +{{/* + Shared chrome for the server-rendered pages in the authorization flow + (consent, and the consent error page). + + These pages sit INSIDE the login journey: a user reaches them straight from + the hosted login UI at /app, so anything that looks different reads as a + different site — exactly the wrong signal on a screen whose entire job is to + help someone judge whether to trust a client. + + The tokens below are copied from web/app/src/index.css rather than imported: + that file is bundled by Vite into the app build and is not servable to a + standalone template. Keep the two in step — the values here are the app's, + including the note about why the primary blue is one shade past the logo's. +*/}} + +{{define "au_styles"}} + +{{end}} + +{{/* + The same brand header the hosted login UI renders (web/app/src/App.tsx): + logo when one is configured, organization name underneath. Both are + operator-configured values, never anything the client supplied — which is the + point on the consent screen, where every other name on the page is + self-asserted. +*/}} +{{define "au_brand"}} +
+ {{if .organization_logo}} + {{.organization_name}} + {{end}} +

{{.organization_name}}

+
+{{end}} diff --git a/web/templates/consent.tmpl b/web/templates/consent.tmpl new file mode 100644 index 000000000..201266d04 --- /dev/null +++ b/web/templates/consent.tmpl @@ -0,0 +1,66 @@ + + + + + + Authorize {{.client_name}} · {{.organization_name}} + {{template "au_styles" .}} + + +
+ {{template "au_brand" .}} + +
+

Authorize {{.client_name}}

+

+ {{.client_name}} wants to access your account as + {{.user_email}}. +

+ +
+
Sends you back to
+
{{.redirect_host}}
+ +
Identified by
+
{{.client_id}}
+ + {{if .scopes}} +
Requests permission to
+
+
    + {{range .scopes}} +
  • {{.}}
    • + {{end}} +
+
+ {{end}} +
+ + {{if .loopback_only}} +
+ This application runs on your own computer. + Any program on this machine could claim to be + {{.client_name}}. Only continue if you just + started it yourself. +
+ {{end}} + +

+ This application registered itself and its name has not been + verified by {{.organization_name}}. The address above is where your + access will actually be sent — check that you recognise it. +

+ +
+ + + +
+
+
+ + diff --git a/web/templates/consent_error.tmpl b/web/templates/consent_error.tmpl new file mode 100644 index 000000000..bc47858c6 --- /dev/null +++ b/web/templates/consent_error.tmpl @@ -0,0 +1,44 @@ + + + + + + {{.title}} · {{.organization_name}} + {{template "au_styles" .}} + + +
+ {{template "au_brand" .}} + +
+ + +

{{.title}}

+

{{.message}}

+ + {{/* + No retry button. Every error that lands here has already + consumed or invalidated the request, so a retry could only + fail again — and the flow has to restart from the client, + which is the one place that can mint a fresh authorization + request with its own PKCE challenge. + */}} +

+ {{.hint}} +

+
+
+ +