feat: coalesce concurrent CIMD document fetches for the same client_id

docs/cimd.md

@@ -269,21 +269,73 @@ 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.
 - **SSRF.** The document fetch cannot be turned into a probe of internal/metadata addresses — the guard is the same audited implementation used elsewhere in ZeroID, applied at dial time against the *resolved* IP (DNS-rebinding-safe).
 - **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.
 
 ---
 

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 (

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

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")
+	}
+}