diff --git a/README.md b/README.md index 00a1f40..f32f50d 100644 --- a/README.md +++ b/README.md @@ -183,7 +183,7 @@ alone remain replayable within their TTL. | Flow state | Encrypted into | TTL | |---|---|---| -| Client registration | `client_id` | 7d (configurable via `CLIENT_REGISTRATION_TTL`) | +| Client registration | `client_id` | 7d (configurable via `CLIENT_REGISTRATION_TTL`; `0` = never expires) | | Authorize session | IdP `state` parameter | 10min | | Authorization code | `code` parameter | 60s | | Access token | Opaque bearer | 1h | diff --git a/config/config.go b/config/config.go index 6d60fef..43cf0cb 100644 --- a/config/config.go +++ b/config/config.go @@ -375,14 +375,21 @@ func Load() (*Config, error) { // Hard cap at 90d — a longer envelope materially extends the // window an exfiltrated client_id (which is unauthenticated // metadata) can be reused. + // + // Set to 0 to disable expiry entirely (emits client_id_expires_at=0, + // "never expires" — mirroring RFC 7591 §3.2.1 client_secret_expires_at). + // Opt-in escape hatch for fleets whose MCP clients never re-run DCR + // on invalid_client (e.g. Azure APIM connectors) and would otherwise + // break on every TTL window. Trades away the reuse-window bound — see + // docs/runbooks/client-registration-expired.md. c.ClientRegistrationTTL = 7 * 24 * time.Hour if raw := os.Getenv("CLIENT_REGISTRATION_TTL"); raw != "" { d, err := time.ParseDuration(raw) if err != nil { - return nil, fmt.Errorf("CLIENT_REGISTRATION_TTL must be a Go duration (e.g. 168h, 24h, 720h): %w", err) + return nil, fmt.Errorf("CLIENT_REGISTRATION_TTL must be a Go duration (e.g. 168h, 24h, 720h, or 0 to never expire): %w", err) } - if d <= 0 { - return nil, fmt.Errorf("CLIENT_REGISTRATION_TTL must be positive, got %s", d) + if d < 0 { + return nil, fmt.Errorf("CLIENT_REGISTRATION_TTL must not be negative (use 0 to never expire), got %s", d) } if d > 90*24*time.Hour { return nil, fmt.Errorf("CLIENT_REGISTRATION_TTL exceeds 90d cap, got %s", d) diff --git a/config/config_test.go b/config/config_test.go index 318d02b..e5fc2d7 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -565,12 +565,32 @@ func TestLoad_ClientRegistrationTTL_Custom(t *testing.T) { } } -func TestLoad_ClientRegistrationTTL_RejectsNonPositive(t *testing.T) { +// 0 is the documented opt-in for "never expires" (client_id_expires_at=0), +// not an error: it must load cleanly as a zero duration. Both the +// bare "0" (the natural operator input the docs imply) and "0s" are +// accepted by time.ParseDuration and must both reach the sentinel. +func TestLoad_ClientRegistrationTTL_ZeroMeansNever(t *testing.T) { + for _, raw := range []string{"0", "0s"} { + t.Run(raw, func(t *testing.T) { + setAllRequired(t) + t.Setenv("CLIENT_REGISTRATION_TTL", raw) + cfg, err := Load() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.ClientRegistrationTTL != 0 { + t.Errorf("ClientRegistrationTTL = %v, want 0 (never expires)", cfg.ClientRegistrationTTL) + } + }) + } +} + +func TestLoad_ClientRegistrationTTL_RejectsNegative(t *testing.T) { setAllRequired(t) - t.Setenv("CLIENT_REGISTRATION_TTL", "0s") + t.Setenv("CLIENT_REGISTRATION_TTL", "-1h") _, err := Load() - if err == nil || !strings.Contains(err.Error(), "positive") { - t.Fatalf("want positive rejection, got %v", err) + if err == nil || !strings.Contains(err.Error(), "negative") { + t.Fatalf("want negative rejection, got %v", err) } } diff --git a/docs/configuration.md b/docs/configuration.md index 6920e69..3c0eb6d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -45,7 +45,7 @@ secure production posture (`PROD_MODE=true`); flags listed here as |---|---|---| | `TOKEN_SIGNING_SECRETS_PREVIOUS` | (empty) | Whitespace-separated retired signing secrets accepted on Open during a rolling rotation. New seals always use the primary `TOKEN_SIGNING_SECRET`; Open tries primary first, then each previous. See [`runbooks/key-rotation.md`](./runbooks/key-rotation.md). | | `REVOKE_BEFORE` | (empty) | RFC3339 timestamp. Bulk revocation cutoff: tokens with `iat` before this are rejected. Applies to access AND refresh tokens. | -| `CLIENT_REGISTRATION_TTL` | `168h` (7d) | Lifetime of a sealed `client_id` minted by `POST /register`. Default matches the 7-day refresh-token TTL so a client holding a still-valid refresh can always exchange it; a shorter value silently kills long-running MCP clients (which treat DCR as one-shot at startup) the moment their access token first expires. Go duration syntax (`168h`, `720h`, …); capped at 90d. **Rolling-deploy note:** the TTL is sealed into each `client_id` at registration time, so bumping this env var only affects newly-issued client_ids — existing registrations stay on whatever TTL was in effect when they were minted. See [`runbooks/client-registration-expired.md`](./runbooks/client-registration-expired.md). | +| `CLIENT_REGISTRATION_TTL` | `168h` (7d) | Lifetime of a sealed `client_id` minted by `POST /register`. Default matches the 7-day refresh-token TTL so a client holding a still-valid refresh can always exchange it; a shorter value silently kills long-running MCP clients (which treat DCR as one-shot at startup) the moment their access token first expires. Go duration syntax (`168h`, `720h`, …); capped at 90d. **Set to `0` to disable expiry entirely** (`client_id_expires_at=0`, "never expires" — mirroring RFC 7591 §3.2.1 `client_secret_expires_at`) — opt-in escape hatch for clients that never re-run DCR on `invalid_client` (e.g. Azure APIM connectors); trades away the reuse-window bound. **Rolling-deploy note:** the TTL is sealed into each `client_id` at registration time, so bumping this env var only affects newly-issued client_ids — existing registrations stay on whatever TTL was in effect when they were minted. See [`runbooks/client-registration-expired.md`](./runbooks/client-registration-expired.md). | ## Replay store (Redis) @@ -158,6 +158,21 @@ sum(mcp_auth_consent_decisions_total{decision="approved"}) - `state_missing` — `/authorize` without state in strict mode. - `refresh_family_revoked` / `refresh_concurrent_submit` — refresh-rotation outcomes. + - `client_id_missing` — `/authorize` received a request with no + `client_id`. (`/token` folds a missing `client_id` into its generic + `invalid_request` "missing required parameters", unmetered.) + - `client_id_invalid` — `/authorize` or `/token` could not decode + the sealed `client_id` (tampered, truncated, or wrong key). + - `client_typ_mismatch` — the blob decoded but is not a client + registration (sealed-type-confusion defense; should not fire + absent tampering). + - `client_audience_mismatch` — sealed `client_id` was issued for a + different proxy deployment (distinct from `audience_mismatch`, + which is the access-token binding). + - `client_registration_expired` — sealed `client_id` outlived + `CLIENT_REGISTRATION_TTL`. Sustained counts = a client that + never re-runs DCR; see + [`runbooks/client-registration-expired.md`](./runbooks/client-registration-expired.md). - `mcp_auth_replay_detected_total{kind}` — `code` / `refresh` / `consent` / `callback_state` replays caught by the Redis-backed store. The `consent` kind answers with a re-rendered consent page diff --git a/docs/runbooks/client-registration-expired.md b/docs/runbooks/client-registration-expired.md index 8ca0fbe..18c91e0 100644 --- a/docs/runbooks/client-registration-expired.md +++ b/docs/runbooks/client-registration-expired.md @@ -14,14 +14,31 @@ re-register and the user sees the error directly. ## Signals -- Per-client log line: `client_registration_expired` - (`handlers/helpers.go`, fired by `openAndValidateClient`). -- Counter: `mcp_auth_access_denied_total{reason="invalid_client"}` - with `error_description="client registration expired"` in the - log line that accompanies it. +- Counter: `mcp_auth_access_denied_total{reason="client_registration_expired"}` + — incremented on every rejection by both `/authorize` and + `/token` (`handlers/authorize.go`, `handlers/helpers.go` + `openAndValidateClient`). A tight, sustained rise = a stuck + client (e.g. an Azure APIM connector) looping on the 400 instead + of re-running DCR. +- Log line: `access_denied_client_registration_expired` at WARN with + the client's `internal_id` and `expired_at` (same `access_denied_*` + family as every other denial — catchable by a `level>=warn` or + `event=~"access_denied_.*"` filter). +- HTTP access log: repeated `GET /authorize` (and/or `POST + /token`) at **status 400** with a **77-byte** response body — + the 76-byte JSON + `{"error":"invalid_client","error_description":"client registration expired"}` + plus the encoder's trailing newline (`resp_bytes=77` on the wire). - User report: "MCP server stopped working after a few days of uptime, restart fixes it." +> **Log volume:** the `access_denied_*` WARN lines are volume-bounded +> by the default per-IP rate limiting plus zap's production sampling. +> A stuck single-client storm is capped well before it reaches the +> handler. If you run with `RATE_LIMIT_ENABLED=false` **and** the dev +> (TTY) logger config (no sampling), an attacker- or bug-driven +> rejection loop logs unbounded — keep rate limiting on in production. + ## Why it happens The sealed `client_id` returned by `POST /register` has a @@ -33,6 +50,19 @@ rejects with `invalid_client: client registration expired`. The lifetime is the `CLIENT_REGISTRATION_TTL` env var, default **7 days** (matches `refreshTokenTTL` so a client holding a still-valid refresh can always exchange it). Cap is 90 days. +Setting it to **`0`** disables expiry entirely (never expires). + +## Known client-side root cause + +This is the server-side symptom of a widespread MCP client bug: +the client caches its DCR `client_id` and **does not re-run DCR +when it gets `invalid_client`**. The proxy is spec-correct — it +advertises `client_id_expires_at` (an RFC 7591 extension field) and +rejects a lapsed handle — but most clients ignore the field and never +re-register. Notably **Azure APIM-backed connectors** loop on the +400 indefinitely instead of reconnecting. Tracked across Claude +Code, Cursor, LibreChat, et al. The MCP spec is itself moving away +from DCR toward URL-based client IDs to retire this failure mode. ## Response @@ -57,7 +87,16 @@ suggests they should: actual cause may be the token cutoff. Logs disambiguate. 3. **Lengthen the TTL** for long-running deployments by setting `CLIENT_REGISTRATION_TTL=720h` (30d) or up to the 90d cap. -4. **Consider Option 4** (auto-extend `client_id` on each +4. **Disable expiry** with `CLIENT_REGISTRATION_TTL=0` when the + client provably never re-registers (e.g. Azure APIM connectors + that loop on the 400). This emits `client_id_expires_at=0` and + skips the expiry check on every validation path. It is the + correct fix when the alternative is a permanently-broken + connector — at the cost of the reuse-window bound below. + Reversible: unset the env var to restore the 7d default (only + newly-issued client_ids are affected, per the rolling-deploy + note). +5. **Consider Option 4** (auto-extend `client_id` on each `/token` use) — see `misc/next-steps.md`. Not yet implemented as of this writing. @@ -76,11 +115,20 @@ no matter what the env var says now. Plan accordingly: ## What NOT to do -- **Don't disable `CLIENT_REGISTRATION_TTL` checks.** The TTL - bounds the residual reach of an exfiltrated `client_id` +- **Don't reach for `CLIENT_REGISTRATION_TTL=0` by default.** The + TTL bounds the residual reach of an exfiltrated `client_id` (which is unauthenticated metadata sent in the clear on - `/authorize`). A 0 or near-infinite value silently extends - that window. + `/authorize`). `0` removes that bound — a leaked `client_id` + stays openable forever. It is a deliberate opt-in (see Response + step 4), justified only when a client provably never + re-registers; reserve it for that case rather than as a blanket + fix. Note a leaked `client_id` alone grants nothing: a full + OAuth flow with IdP consent is still required. Under `0` the only + per-`client_id` revocation lever is rotating `TOKEN_SIGNING_SECRET`, + which invalidates **every** sealed blob fleet-wide (all clients, + codes, and refresh tokens) — there is no per-client deny-list, so + losing the TTL backstop means a single bad client can only be + evicted with a fleet-wide rotation. - **Don't try to extend an existing `client_id` server-side.** The sealed payload is immutable. The only way to extend is re-issuing a fresh `client_id`. diff --git a/handlers/authorize.go b/handlers/authorize.go index 450264c..97e6b44 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -103,28 +103,39 @@ func Authorize(tm *token.Manager, logger *zap.Logger, baseURL string, oauth2Cfg // not receive an `error=` redirect — we'd be forwarding to whatever // host an attacker chose. JSON 400 keeps these errors visible to // the resource owner instead. + // Error shapes here differ from the /token path (invalid_client / + // invalid_request, not invalid_grant) per RFC 6749 §4.1.2.1, so + // the block stays inline rather than sharing openAndValidateClient; + // recordClientDenial keeps the metric+log observable (see its doc). if clientIDStr == "" { + recordClientDenial(logger, "client_id_missing") writeOAuthError(w, http.StatusBadRequest, "invalid_request", "client_id is required") return } var client sealedClient if err := tm.OpenJSON(clientIDStr, &client, token.PurposeClient); err != nil { + recordClientDenial(logger, "client_id_invalid", zap.Error(err)) writeOAuthError(w, http.StatusBadRequest, "invalid_client", "unknown client_id") return } if client.Typ != token.PurposeClient { + recordClientDenial(logger, "client_typ_mismatch") writeOAuthError(w, http.StatusBadRequest, "invalid_client", "unknown client_id") return } if client.Audience != baseURL { + recordClientDenial(logger, "client_audience_mismatch", zap.String("internal_id", client.ID)) writeOAuthError(w, http.StatusBadRequest, "invalid_client", "client registered for a different audience") return } - if time.Now().After(client.ExpiresAt) { + // clientExpired treats a zero ExpiresAt as never-expiring + // (CLIENT_REGISTRATION_TTL=0); shared with the /token path. + if clientExpired(&client) { + recordClientDenial(logger, "client_registration_expired", zap.String("internal_id", client.ID), zap.Time("expired_at", client.ExpiresAt)) writeOAuthError(w, http.StatusBadRequest, "invalid_client", "client registration expired") return } diff --git a/handlers/handlers_test.go b/handlers/handlers_test.go index bea6faf..a0d0373 100644 --- a/handlers/handlers_test.go +++ b/handlers/handlers_test.go @@ -262,8 +262,8 @@ func TestRegister_Success(t *testing.T) { if resp.ClientIDIssuedAt == 0 { t.Error("client_id_issued_at should be non-zero") } - // RFC 7591 §3.2.1 OPTIONAL `client_id_expires_at` — surfaced so - // clients know when the sealed handle stops opening (default 24h). + // `client_id_expires_at` (RFC 7591 extension field) — surfaced so + // clients know when the sealed handle stops opening (default 7d). if resp.ClientIDExpiresAt <= resp.ClientIDIssuedAt { t.Errorf("client_id_expires_at=%d must be > client_id_issued_at=%d", resp.ClientIDExpiresAt, resp.ClientIDIssuedAt) @@ -293,6 +293,95 @@ func TestRegister_Success(t *testing.T) { } } +// CLIENT_REGISTRATION_TTL=0 is the opt-in for non-expiring client_ids: +// the response must carry client_id_expires_at=0, +// the sealed ExpiresAt must be the zero time, and the shared validator +// (openAndValidateClient, used by both /token grants and mirrored by +// /authorize) must NOT reject it as "client registration expired". +func TestRegister_NeverExpires(t *testing.T) { + tm := newTestTokenManager(t) + body := `{"redirect_uris":["https://app.example.com/callback"],"client_name":"forever-app"}` + + req := httptest.NewRequest(http.MethodPost, "/register", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + // clientTTL = 0 → never expires. + Register(tm, zap.NewNop(), testBaseURL, 0)(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var resp registerResponse + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.ClientIDExpiresAt != 0 { + t.Errorf("client_id_expires_at = %d, want 0 (never expires)", resp.ClientIDExpiresAt) + } + + var sc sealedClient + if err := tm.OpenJSON(resp.ClientID, &sc, token.PurposeClient); err != nil { + t.Fatalf("client_id is not a valid sealed client: %v", err) + } + if !sc.ExpiresAt.IsZero() { + t.Errorf("sealed ExpiresAt = %v, want zero time (never expires)", sc.ExpiresAt) + } + + // The expiry guard must let a zero-ExpiresAt client through even + // though time.Now() is "after" the zero time. + vr := httptest.NewRecorder() + if got := openAndValidateClient(vr, tm, zap.NewNop(), resp.ClientID, testBaseURL); got == nil { + t.Fatalf("non-expiring client_id rejected: %d %s", vr.Code, vr.Body.String()) + } +} + +// Pins the AccessDenied{reason} increment for every client-validation +// rejection on the shared /token-grant validator. A stuck client +// looping on the 400 is invisible without the metric. The wrong-purpose +// branch is the type-confusion defense (its own label, not folded into +// client_id_invalid), so its observability matters most. +func TestOpenAndValidateClient_RejectionMetrics(t *testing.T) { + tm := newTestTokenManager(t) + cb := []string{"https://app.example.com/callback"} + seal := func(sc sealedClient) string { + id, err := tm.SealJSON(sc, token.PurposeClient) + if err != nil { + t.Fatalf("SealJSON: %v", err) + } + return id + } + future := time.Now().Add(time.Hour) + past := time.Now().Add(-time.Hour) + + cases := []struct { + name string + clientID string + reason string + }{ + {"undecodable", "not-a-sealed-blob", "client_id_invalid"}, + {"wrong_typ", seal(sealedClient{ID: "x", Typ: token.PurposeCode, Audience: testBaseURL, RedirectURIs: cb, ExpiresAt: future}), "client_typ_mismatch"}, + {"audience", seal(sealedClient{ID: "x", Typ: token.PurposeClient, Audience: "https://other.example", RedirectURIs: cb, ExpiresAt: future}), "client_audience_mismatch"}, + {"expired", seal(sealedClient{ID: "x", Typ: token.PurposeClient, Audience: testBaseURL, RedirectURIs: cb, ExpiresAt: past}), "client_registration_expired"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + before := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues(tc.reason)) + rr := httptest.NewRecorder() + if got := openAndValidateClient(rr, tm, zap.NewNop(), tc.clientID, testBaseURL); got != nil { + t.Fatal("expected rejection (nil), got a client") + } + if rr.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", rr.Code) + } + if after := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues(tc.reason)); after-before != 1 { + t.Errorf("AccessDenied{%s} delta = %v, want 1", tc.reason, after-before) + } + }) + } +} + func TestRegister_UnsupportedAuthMethod(t *testing.T) { tm := newTestTokenManager(t) // Client requests client_secret_post, but discovery only advertises "none" @@ -741,6 +830,9 @@ func TestAuthorize_ExpiredClient(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/authorize?"+params.Encode(), nil) rr := httptest.NewRecorder() + // Pin the metric on the /authorize path too: the guard here is a + // twin of openAndValidateClient and must stay observable. + before := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues("client_registration_expired")) Authorize(tm, logger, testBaseURL, testOAuth2Config(), AuthorizeConfig{PKCERequired: true})(rr, req) if rr.Code != http.StatusBadRequest { @@ -754,6 +846,100 @@ func TestAuthorize_ExpiredClient(t *testing.T) { if oauthErr.Error != "invalid_client" { t.Errorf("expected error 'invalid_client', got %q", oauthErr.Error) } + if after := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues("client_registration_expired")); after-before != 1 { + t.Errorf("AccessDenied{client_registration_expired} delta = %v, want 1", after-before) + } +} + +// The /authorize never-expire (zero ExpiresAt) branch is a twin of the +// openAndValidateClient guard; without this test, inverting or dropping +// the !IsZero() short-circuit in authorize.go would reject every +// non-expiring client_id at /authorize and no test would fail. +func TestAuthorize_NeverExpires(t *testing.T) { + tm := newTestTokenManager(t) + redirectURI := "https://app.example.com/callback" + // Zero ExpiresAt = never expires (CLIENT_REGISTRATION_TTL=0). + encClientID, err := tm.SealJSON(sealedClient{ + ID: "never-expires-authz", + RedirectURIs: []string{redirectURI}, + ClientName: "forever", + Typ: token.PurposeClient, + Audience: testBaseURL, + }, token.PurposeClient) + if err != nil { + t.Fatalf("SealJSON: %v", err) + } + + params := url.Values{ + "response_type": {"code"}, + "client_id": {encClientID}, + "redirect_uri": {redirectURI}, + "code_challenge": {pkceChallenge("verifier")}, + "code_challenge_method": {"S256"}, + "state": {"user-state-123"}, + } + req := httptest.NewRequest(http.MethodGet, "/authorize?"+params.Encode(), nil) + rr := httptest.NewRecorder() + + Authorize(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), AuthorizeConfig{PKCERequired: true})(rr, req) + + if rr.Code != http.StatusFound { + t.Fatalf("non-expiring client_id rejected at /authorize: expected 302, got %d: %s", rr.Code, rr.Body.String()) + } +} + +// Pins the AccessDenied{reason} increment for every client-validation +// rejection on the /authorize path. The feature's purpose is +// observability; a silent rejection here is a regression. +func TestAuthorize_ClientValidation_EmitsMetric(t *testing.T) { + tm := newTestTokenManager(t) + redirectURI := "https://app.example.com/callback" + seal := func(sc sealedClient) string { + id, err := tm.SealJSON(sc, token.PurposeClient) + if err != nil { + t.Fatalf("SealJSON: %v", err) + } + return id + } + future := time.Now().Add(time.Hour) + + cases := []struct { + name string + clientID string // empty → omit param entirely + reason string + }{ + {"missing", "", "client_id_missing"}, + {"undecodable", "not-a-sealed-blob", "client_id_invalid"}, + {"wrong_typ", seal(sealedClient{ID: "x", Typ: token.PurposeCode, Audience: testBaseURL, RedirectURIs: []string{redirectURI}, ExpiresAt: future}), "client_typ_mismatch"}, + {"audience", seal(sealedClient{ID: "x", Typ: token.PurposeClient, Audience: "https://other.example", RedirectURIs: []string{redirectURI}, ExpiresAt: future}), "client_audience_mismatch"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + params := url.Values{ + "response_type": {"code"}, + "redirect_uri": {redirectURI}, + "code_challenge": {pkceChallenge("verifier")}, + "code_challenge_method": {"S256"}, + "state": {"s"}, + } + if tc.clientID != "" { + params.Set("client_id", tc.clientID) + } + req := httptest.NewRequest(http.MethodGet, "/authorize?"+params.Encode(), nil) + rr := httptest.NewRecorder() + + before := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues(tc.reason)) + Authorize(tm, zap.NewNop(), testBaseURL, testOAuth2Config(), AuthorizeConfig{PKCERequired: true})(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + if after := testutil.ToFloat64(metrics.AccessDenied.WithLabelValues(tc.reason)); after-before != 1 { + t.Errorf("AccessDenied{%s} delta = %v, want 1", tc.reason, after-before) + } + }) + } } // --- Token: authorization_code grant --- diff --git a/handlers/helpers.go b/handlers/helpers.go index c917d14..b10a2a4 100644 --- a/handlers/helpers.go +++ b/handlers/helpers.go @@ -8,7 +8,9 @@ import ( "strings" "time" + "github.com/babs/mcp-auth-proxy/metrics" "github.com/babs/mcp-auth-proxy/token" + "go.uber.org/zap" ) // OAuthError represents an RFC 6749 error response. @@ -249,6 +251,32 @@ func hasOverlap(userGroups, allowed []string) bool { return false } +// recordClientDenial records a client_id-validation rejection: it +// increments AccessDenied{reason} and emits a matching +// access_denied_ WARN log — the repo's denial-logging +// convention (see callback.go / authorize.go state path). Pairing them +// in one call keeps the metric series and the log in lockstep; the +// caller still writes the (path-specific) RFC 6749 OAuth error and +// returns. writeOAuthError is otherwise silent, so without this a stuck +// client looping on e.g. expired registration shows up only as raw 4xx +// access logs. Under a retry storm the log volume is bounded by the +// per-IP rate limiter (main.go) plus zap's production sampling — keep +// both enabled. +func recordClientDenial(logger *zap.Logger, reason string, fields ...zap.Field) { + metrics.AccessDenied.WithLabelValues(reason).Inc() + logger.Warn("access_denied_"+reason, fields...) +} + +// clientExpired reports whether a sealed client_id has passed its TTL. +// A zero ExpiresAt means "never expires" (CLIENT_REGISTRATION_TTL=0) and +// is never stale — the 0=never semantic mirrors RFC 7591 §3.2.1 +// client_secret_expires_at applied to our client_id_expires_at extension +// field. Single source for this rule, shared by /authorize and /token so +// the two validation sites cannot diverge. +func clientExpired(c *sealedClient) bool { + return !c.ExpiresAt.IsZero() && time.Now().After(c.ExpiresAt) +} + // openAndValidateClient decodes a client_id sealed payload and runs // the four invariants both /token grant handlers need: AAD purpose // match, belt-and-braces typ discriminator, audience binding, and TTL. @@ -257,21 +285,25 @@ func hasOverlap(userGroups, allowed []string) bool { // Factoring this out removes the near-identical 20-line block from // each grant handler; error shapes are preserved verbatim so the // existing test matrix against both paths keeps exercising them. -func openAndValidateClient(w http.ResponseWriter, tm *token.Manager, clientIDStr, audience string) *sealedClient { +func openAndValidateClient(w http.ResponseWriter, tm *token.Manager, logger *zap.Logger, clientIDStr, audience string) *sealedClient { var client sealedClient if err := tm.OpenJSON(clientIDStr, &client, token.PurposeClient); err != nil { + recordClientDenial(logger, "client_id_invalid", zap.Error(err)) writeOAuthError(w, http.StatusBadRequest, "invalid_grant", "invalid client_id") return nil } if client.Typ != token.PurposeClient { + recordClientDenial(logger, "client_typ_mismatch") writeOAuthError(w, http.StatusBadRequest, "invalid_grant", "invalid client_id") return nil } if client.Audience != audience { + recordClientDenial(logger, "client_audience_mismatch", zap.String("internal_id", client.ID)) writeOAuthError(w, http.StatusBadRequest, "invalid_client", "client registered for a different audience") return nil } - if time.Now().After(client.ExpiresAt) { + if clientExpired(&client) { + recordClientDenial(logger, "client_registration_expired", zap.String("internal_id", client.ID), zap.Time("expired_at", client.ExpiresAt)) writeOAuthError(w, http.StatusBadRequest, "invalid_client", "client registration expired") return nil } diff --git a/handlers/register.go b/handlers/register.go index 4531325..fbf7a4c 100644 --- a/handlers/register.go +++ b/handlers/register.go @@ -44,11 +44,13 @@ type registerResponse struct { ClientID string `json:"client_id"` ClientIDIssuedAt int64 `json:"client_id_issued_at"` // ClientIDExpiresAt is the UNIX timestamp at which the sealed - // client_id stops opening (RFC 7591 §3.2.1 OPTIONAL). Published - // so clients can proactively re-register before hitting a 400 on - // /authorize once the sealed TTL lapses (default 24h, see - // clientTTL). Value of 0 per §3.2.1 would mean "never expires"; - // we always emit a real timestamp. + // client_id stops opening. It is an RFC 7591 extension field (the + // spec defines client_secret_expires_at, not client_id_expires_at); + // we publish it so clients can proactively re-register before hitting + // a 400 on /authorize once the sealed TTL lapses (default 7d, see + // clientTTL). Emitted as 0 ("never expires", mirroring §3.2.1 + // client_secret_expires_at) when the operator sets + // CLIENT_REGISTRATION_TTL=0. ClientIDExpiresAt int64 `json:"client_id_expires_at"` RedirectURIs []string `json:"redirect_uris"` // ClientName is echoed only when the client actually submitted @@ -201,13 +203,22 @@ func Register(tm *token.Manager, logger *zap.Logger, audience string, clientTTL } now := time.Now() + // clientTTL of 0 means "never expires" (CLIENT_REGISTRATION_TTL=0; + // config rejects negatives, so 0 is the only non-positive value + // that reaches here): seal the zero time so the validation paths + // (openAndValidateClient / authorize) skip the expiry check, and + // emit client_id_expires_at=0 below. + var expiresAt time.Time + if clientTTL > 0 { + expiresAt = now.Add(clientTTL) + } sc := sealedClient{ ID: uuid.New().String(), RedirectURIs: req.RedirectURIs, ClientName: req.ClientName, Typ: token.PurposeClient, Audience: audience, - ExpiresAt: now.Add(clientTTL), + ExpiresAt: expiresAt, } clientID, err := tm.SealJSON(sc, token.PurposeClient) @@ -221,14 +232,21 @@ func Register(tm *token.Manager, logger *zap.Logger, audience string, clientTTL logger.Info("client_registered", zap.String("internal_id", sc.ID), zap.String("client_name", req.ClientName)) // RFC 7591 examples mark client information responses non-cacheable. - // Our client_id is a 24h bearer-like registration handle, so keep it + // Our client_id is a bearer-like registration handle (7d default + // TTL, or non-expiring when CLIENT_REGISTRATION_TTL=0), so keep it // out of shared caches even though POST responses are rarely cached. w.Header().Set("Cache-Control", "no-store") w.Header().Set("Pragma", "no-cache") + // 0 = never expires; zero time must not be rendered via .Unix() + // (which yields a large negative epoch). + var clientIDExpiresAt int64 + if !sc.ExpiresAt.IsZero() { + clientIDExpiresAt = sc.ExpiresAt.Unix() + } writeJSON(w, http.StatusCreated, registerResponse{ ClientID: clientID, ClientIDIssuedAt: now.Unix(), - ClientIDExpiresAt: sc.ExpiresAt.Unix(), + ClientIDExpiresAt: clientIDExpiresAt, RedirectURIs: req.RedirectURIs, ClientName: req.ClientName, TokenEndpointAuthMethod: authMethod, diff --git a/handlers/token.go b/handlers/token.go index 746be86..f1a3201 100644 --- a/handlers/token.go +++ b/handlers/token.go @@ -165,7 +165,7 @@ func handleAuthorizationCode(w http.ResponseWriter, r *http.Request, tm *token.M return } - client := openAndValidateClient(w, tm, clientIDStr, audience) + client := openAndValidateClient(w, tm, logger, clientIDStr, audience) if client == nil { return } @@ -395,7 +395,7 @@ func handleRefreshToken(w http.ResponseWriter, r *http.Request, tm *token.Manage return } - client := openAndValidateClient(w, tm, clientIDStr, audience) + client := openAndValidateClient(w, tm, logger, clientIDStr, audience) if client == nil { return } diff --git a/specs.md b/specs.md index 301d0e6..698530d 100644 --- a/specs.md +++ b/specs.md @@ -257,7 +257,7 @@ PKCE-only proxy: no client secrets are validated. `scopes_supported` is an expli - OAuth 2.1 §2.3.1: each `redirect_uri` must use HTTPS, or HTTP when pointing at a loopback host. Loopback is recognized via `net.ParseIP().IsLoopback()` (covers the full 127/8 range, `::1`, `::ffff:127.0.0.1`, `::0.0.0.1`) plus the literal `localhost` / `localhost.`. Non-http(s) schemes (e.g. `ftp://`, `ldap://`, `file://`, custom app schemes) are rejected unconditionally even when the host is loopback - Generate an internal UUID for the client - Encrypt the whole `{ id, redirect_uris, client_name, expires_at }` with AES-GCM → this is the returned `client_id` -- TTL embedded in the encrypted blob: 7d default, configurable via `CLIENT_REGISTRATION_TTL` (Go duration, capped at 90d). The 7d default matches `refreshTokenTTL` so a client holding a still-valid refresh token can always exchange it — a shorter TTL silently kills long-running MCP clients (which treat DCR as one-shot at startup) the moment their access token first expires. +- TTL embedded in the encrypted blob: 7d default, configurable via `CLIENT_REGISTRATION_TTL` (Go duration, capped at 90d; `0` = never expires, sealing a zero `expires_at`). The 7d default matches `refreshTokenTTL` so a client holding a still-valid refresh token can always exchange it — a shorter TTL silently kills long-running MCP clients (which treat DCR as one-shot at startup) the moment their access token first expires. `0` is the opt-in escape hatch for clients that never re-run DCR on `invalid_client` (e.g. Azure APIM connectors); it trades away the reuse-window bound on an exfiltrated `client_id`. - Request body limited to 1 MB (`MaxBytesReader`) **Response 201 JSON:** @@ -274,7 +274,7 @@ Headers: `Cache-Control: no-store`, `Pragma: no-cache`. } ``` -`client_id_expires_at` (RFC 7591 §3.2.1) is the UNIX timestamp at which the sealed `client_id` stops opening (default `client_id_issued_at + 7d`, configurable via `CLIENT_REGISTRATION_TTL`). Clients that cache the handle should re-register before this time to avoid a 400 on `/authorize` and on `/token` refresh-token rotations. +`client_id_expires_at` (an RFC 7591 extension field — the spec defines `client_secret_expires_at`, not `client_id_expires_at`) is the UNIX timestamp at which the sealed `client_id` stops opening (default `client_id_issued_at + 7d`, configurable via `CLIENT_REGISTRATION_TTL`). Clients that cache the handle should re-register before this time to avoid a 400 on `/authorize` and on `/token` refresh-token rotations. When `CLIENT_REGISTRATION_TTL=0`, `client_id_expires_at` is emitted as `0` ("never expires", mirroring RFC 7591 §3.2.1 `client_secret_expires_at`) and the expiry check is skipped on every validation path. Error responses use RFC 7591 §3.2.2 codes: `invalid_redirect_uri` for any redirect_uri-shape defect (missing, over-count, over-length, malformed, opaque, hostless, fragment-bearing, userinfo-bearing, or non-https-non-loopback); `invalid_client_metadata` for unsupported `token_endpoint_auth_method`, over-length `client_name`, or `client_name` containing control bytes (NUL/CR/LF/TAB) or the X-User-Groups delimiter `,` (the field is sealed into the returned `client_id` and emitted to logs; raw control bytes would smuggle past zap's JSON-escaping when downstream code unsealed and parsed it); `invalid_request` for structural problems: 400 with `"invalid JSON body"` for a malformed body, 413 with `"request body exceeds the 1 MB cap"` when the body crosses `MaxBytesReader`.