diff --git a/docs/cimd.md b/docs/cimd.md index 60f1bb2..52eeab8 100644 --- a/docs/cimd.md +++ b/docs/cimd.md @@ -269,6 +269,56 @@ CIMD-aware clients (MCP 2025-11-25, and any client that walks the draft's discov --- +## Specification revision and deviations + +**Implemented against:** [`draft-ietf-oauth-client-id-metadata-document-02`](https://datatracker.ietf.org/doc/draft-ietf-oauth-client-id-metadata-document/), adopted by the OAuth WG in October 2025. + +This is a **draft at revision 02 and it will change.** The section exists so that when it does, the next reader can tell which behaviours were the spec, which were our choices, and which were deliberately left unbuilt — rather than reverse-engineering that from the validator. + +### Where ZeroID is deliberately STRICTER than the draft + +Both of these will reject a document some other implementation accepts. That is intended, but it is also the part most likely to age badly: a later revision can bless what we refuse, and then we are rejecting valid clients for a reason nobody remembers choosing. Revisit both on each draft bump. + +| Rule | Draft says | ZeroID does | Why | +|---|---|---|---| +| Query string in `client_id` | SHOULD NOT | **Rejects** | The `client_id` a client presents must stay byte-identical to the URL the document was fetched from — that is exactly what the §4 self-reference check compares. Stripping or tolerating a query gives one document two spellings. | +| `client_name` | RECOMMENDED | **Required, non-empty** | It is the string a human is asked to trust on a consent screen. Absent, the prompt degrades to a raw URL — and for a URL an attacker chose, that is actively misleading. The publisher is anonymous by construction (no registration, no secret), so this label is most of what consent has to work with. | + +### Deliberately not implemented + +- **Confidential CIMD clients** — `token_endpoint_auth_method: private_key_jwt` with a published `jwks_uri`. ZeroID accepts no `private_key_jwt` token-endpoint auth for any client; CIMD is public-PKCE-only. +- **`software_statement`** — signed metadata is not consumed. CIMD trust here is domain-ownership based. + +Both are areas the draft is more likely to move in than the core resolution rules, which is part of why they are not built on. + +### The change most likely to break silently + +A rename of the discovery field **`client_id_metadata_document_supported`**. + +If a later draft renames it, nothing errors. The authorization server keeps advertising a key clients no longer look for, every client falls back to `registration_endpoint` and DCR, and the whole flow keeps working — via exactly the row-per-client path CIMD exists to remove. + +Tests do not help here. They assert the server *emits* that field, so they stay green while no client can see it. The only guard is tracking the draft. + +### What makes drift survivable + +Two properties worth knowing about before a spec change forces a decision: + +- **DCR is retained as a fallback.** `registration_endpoint` stays advertised alongside CIMD deliberately, so a client that cannot complete CIMD still has a path. +- **Registry-first resolution is a pinning mechanism.** A client registered under its CIMD URL overrides the document (see step 1 of resolution). So a specific client caught by a spec change can be pinned by registering it, without waiting on a ZeroID release. + +### Deployment note: the cache is per process + +`CIMDService` holds its resolution cache in memory, so a deployment running N replicas has N independent caches. Concurrent resolutions of the same `client_id` are coalesced **within** a process (singleflight), not across them. + +Two consequences worth planning around: + +- **Fan-out** is up to N fetches per document per TTL, and negative caching is likewise per replica — so replaying a dead URL costs N times as much. Bounded in practice by edge rate limiting, which is required for an open deployment anyway. +- **Staleness is non-uniform.** Two replicas can serve different versions of one document for up to the TTL, so a client that removes a `redirect_uri` may find the change effective on some replicas and not others, with no way to tell which served it. + +A shared cache (Redis) would fix the fan-out and make replicas *consistently* stale — it would not make them *fresher*. Revocation latency is governed by the TTL and by the document's `Cache-Control` (ZeroID takes the shorter, floored at 60s), independent of where the cache lives. Note also that a shared cache holds `redirect_uris`, the primary anti-impersonation control, so write access to it becomes equivalent to choosing where authorization codes are delivered. + +--- + ## Security considerations - **Redirect-URI allow-list is the load-bearing control.** Copying a `client_id` is useless without control of a host in its `redirect_uris`. PKCE binds the code to the verifier on top of that. @@ -276,14 +326,16 @@ CIMD-aware clients (MCP 2025-11-25, and any client that walks the draft's discov - **Localhost redirects.** Loopback `redirect_uris` are matched port-agnostically (RFC 8252) so native/CLI callbacks work, but any process on the user's machine can bind a loopback port. Treat localhost CIMD clients with the same caution as any native public client. - **DNS / TLS trust.** CIMD's integrity rests on the same DNS + TLS + certificate-transparency assumptions as any HTTPS-based trust model. **`allowed_domains` is the hard boundary** — see [Production hardening](#production-hardening--set-allowed_domains). - **No persistence, no secret.** CIMD clients are ephemeral and hold no symmetric secret at the broker — there is nothing to leak from ZeroID's side. -- **Fetch abuse.** The resolution surface is unauthenticated (`/oauth2/authorize` is public), so a request-supplied `client_id` triggers an outbound fetch. It is deliberately hard to abuse — bounded cache (1000 entries), negative caching of failures (10 s fetch / 60 s validation), 5 KiB response cap, 5 s timeout, no redirect following, and the SSRF blocklist — but the **primary control is `allowed_domains`**, which rejects unknown hosts before any fetch. Deployers running fully open CIMD (empty allowlist) should additionally rate-limit `/oauth2/authorize` at the edge like any public endpoint. +- **Fetch abuse.** The resolution surface is unauthenticated (`/oauth2/authorize` is public, and client resolution runs *before* the principal chain), so a request-supplied `client_id` triggers an outbound fetch. It is deliberately hard to abuse — concurrent resolutions of the same `client_id` are **coalesced into one fetch** (singleflight), plus a bounded cache (1000 entries), negative caching of failures (10 s fetch / 60 s validation), 5 KiB response cap, 5 s timeout, no redirect following, and the SSRF blocklist. + + Be precise about what each control bounds. The caps bound the cost of *one* fetch. Coalescing bounds *duplicate concurrent* work for one `client_id`. Neither bounds a caller cycling **distinct** URLs: each unique path is a fresh flight, misses the cache, walks past negative caching, and churns cache eviction. So the **primary control is `allowed_domains`**, which rejects unknown hosts before any fetch — and a deployment running fully open (empty allowlist) **must** rate-limit `/oauth2/authorize` at the edge. That is the deployer's job; no in-process control substitutes for it. --- ## Limitations / future work -- **Confidential CIMD clients** (`token_endpoint_auth_method: private_key_jwt` with a published `jwks_uri`) are not supported — ZeroID does not yet accept `private_key_jwt` token-endpoint auth for any client. v1 is public-PKCE-only. -- **`software_statement`** (signed metadata) — not consumed; CIMD trust is domain-ownership based. +- **Unimplemented draft features** — confidential CIMD clients (`private_key_jwt` + `jwks_uri`) and `software_statement`. Both are covered under [Specification revision and deviations](#deliberately-not-implemented), which is the single place that records what is and is not built against the draft; they are not repeated here so the two cannot drift apart. +- **Cross-replica cache coherence** — the resolution cache is per process, so N replicas hold N caches. See [Deployment note: the cache is per process](#deployment-note-the-cache-is-per-process) for the fan-out and staleness consequences, and why a shared cache addresses the first but not the second. --- diff --git a/go.mod b/go.mod index 5e83093..3c6cba7 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.44.0 go.opentelemetry.io/otel/trace v1.44.0 golang.org/x/crypto v0.55.0 + golang.org/x/sync v0.22.0 ) require ( diff --git a/internal/service/cimd.go b/internal/service/cimd.go index 37be36f..17b87af 100644 --- a/internal/service/cimd.go +++ b/internal/service/cimd.go @@ -49,6 +49,8 @@ import ( "sync" "time" + "golang.org/x/sync/singleflight" + "github.com/rs/zerolog/log" "github.com/highflame-ai/zeroid/domain" @@ -171,6 +173,23 @@ type CIMDService struct { mu sync.Mutex cache map[string]cimdCacheEntry + + // flights collapses concurrent resolutions of the SAME client_id into one + // outbound fetch. The cache only helps once a fetch has COMPLETED; until + // then every arriving request was a miss and started its own fetch, so N + // simultaneous first-time requests for one client_id meant N DNS + // resolutions, N TLS handshakes, and N × up-to-5s of request occupancy for + // a document that is identical every time. + // + // That is the amplification an unauthenticated endpoint should not offer: + // client resolution runs BEFORE the principal chain, so no credential is + // needed to trigger it, and concurrency was free to the caller. + // + // This bounds duplicate work per client_id. It does NOT bound distinct-URL + // abuse — a caller cycling unique paths gets a fresh flight each time, and + // walks past negative caching too. Only an edge rate limit closes that; + // see the "Fetch abuse" note in docs/cimd.md. + flights singleflight.Group // maxCacheEntries is maxCIMDCacheEntries in production; overridable in // tests to exercise eviction without inserting a thousand entries. maxCacheEntries int @@ -341,6 +360,49 @@ func (s *CIMDService) ResolveClient(ctx context.Context, clientID string) (*doma return client, cachedErr } + // Cache miss. Collapse concurrent misses for this client_id into one + // fetch; every other caller waits on that result rather than issuing its + // own. See CIMDService.flights. + // + // The returned client is cloned PER CALLER below: singleflight hands the + // same value to every waiter, and callers get a mutable *domain.OAuthClient + // — the same reason cachedResult returns a copy. Sharing one instance + // across waiters would let any of them mutate what the others hold. + res, err, _ := s.flights.Do(clientID, func() (any, error) { + // Re-check under the flight. A fetch may have completed and populated + // the cache between the miss above and this call, in which case + // starting a fetch would be duplicate work the flight cannot see. + if client, cachedErr, hit := s.cachedResult(clientID); hit { + if cachedErr != nil { + return nil, cachedErr + } + + return client, nil + } + + return s.resolveUncached(ctx, clientID) + }) + if err != nil { + return nil, err + } + + client, ok := res.(*domain.OAuthClient) + if !ok || client == nil { + // Unreachable: every non-error path above returns a non-nil + // *domain.OAuthClient. Kept because singleflight erases the type, so a + // future edit that returns something else would otherwise surface as a + // nil-pointer panic inside the authorize handler rather than an error. + return nil, fmt.Errorf("%w: resolution returned no client", ErrCIMDInvalidDocument) + } + + return cloneCIMDClient(client), nil +} + +// resolveUncached performs the actual fetch, validation, synthesis and caching +// for one client_id. Split out of ResolveClient so the singleflight callback +// stays readable; it must only be called from inside a flight, because it +// assumes the caller has established there is no usable cache entry. +func (s *CIMDService) resolveUncached(ctx context.Context, clientID string) (*domain.OAuthClient, error) { doc, docTTL, err := s.fetch(ctx, clientID) if err != nil { // Negative-cache the failure so replaying a dead URL can't force a diff --git a/internal/service/cimd_test.go b/internal/service/cimd_test.go index 6d6ed00..11bb27d 100644 --- a/internal/service/cimd_test.go +++ b/internal/service/cimd_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "sync/atomic" "testing" "time" @@ -556,3 +557,96 @@ func TestCIMDPositiveCacheTTL(t *testing.T) { } } } + +// TestCIMDResolveClient_CoalescesConcurrentFetches pins that N simultaneous +// first-time resolutions of one client_id perform ONE outbound fetch. +// +// The cache alone cannot provide this: it is only populated once a fetch has +// completed, so before that every arriving request is a miss and starts its +// own fetch. N concurrent requests for the same new client_id meant N DNS +// resolutions, N TLS handshakes, and N × up-to-5s of request occupancy for a +// document that is byte-identical every time — reachable without a credential, +// since client resolution runs ahead of the principal chain. +// +// The handler blocks until every caller has arrived, so the test fails if the +// implementation serialises rather than coalesces: without singleflight all +// concurrent callers reach the handler and the barrier releases, and the fetch +// count is N. +func TestCIMDResolveClient_CoalescesConcurrentFetches(t *testing.T) { + const callers = 8 + + var docURL string + + release := make(chan struct{}) + arrived := make(chan struct{}, callers) + + svc, base, hits := newCIMDTestServer(t, CIMDConfig{Enabled: true, CacheTTL: time.Hour}, + func(w http.ResponseWriter, _ *http.Request) { + // Hold the first fetch open so the other callers are guaranteed to + // be inside ResolveClient while it is still in flight. Without + // this the first could complete and populate the cache before the + // others start, and the test would pass for the wrong reason. + arrived <- struct{}{} + <-release + fmt.Fprint(w, docJSON(docURL, "")) + }) + docURL = base + "/client.json" + + var wg sync.WaitGroup + + results := make([]*domain.OAuthClient, callers) + errs := make([]error, callers) + + for i := range callers { + wg.Add(1) + + go func() { + defer wg.Done() + + results[i], errs[i] = svc.ResolveClient(context.Background(), docURL) + }() + } + + // Exactly one caller should reach the handler; let it finish. + <-arrived + close(release) + wg.Wait() + + if got := atomic.LoadInt32(hits); got != 1 { + t.Errorf("concurrent resolutions performed %d fetches, want 1 — "+ + "they are not being coalesced, so an unauthenticated caller can "+ + "multiply outbound work by simply issuing requests in parallel", got) + } + + for i := range callers { + if errs[i] != nil { + t.Fatalf("caller %d: %v", i, errs[i]) + } + + if results[i] == nil { + t.Fatalf("caller %d got a nil client", i) + } + + if results[i].ClientID != docURL { + t.Errorf("caller %d: client_id = %q, want %q", i, results[i].ClientID, docURL) + } + } + + // Every waiter must hold its OWN client. singleflight hands the same value + // to all of them, so without a per-caller clone one caller mutating its + // result would corrupt what the others are holding — the same reason + // cachedResult returns a copy. + for i := 1; i < callers; i++ { + if results[i] == results[0] { + t.Fatalf("caller %d shares a *domain.OAuthClient with caller 0; "+ + "singleflight results must be cloned per caller", i) + } + } + + results[0].RedirectURIs[0] = "https://mutated.example/cb" + + if results[1].RedirectURIs[0] == "https://mutated.example/cb" { + t.Error("mutating one caller's RedirectURIs changed another's — the " + + "slice is shared, so the clone is shallow where it must be deep") + } +}