From 99aff1dcccc20094db36e8057cc23b1852a45e98 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 13:56:05 +0800 Subject: [PATCH 1/7] fix(keys): let an unknown kid force a JWKS refetch with the default options JWKSIssuerKeySource, ClientJWKSSource and ephemeral.ClientKeySource rate-limited the unknown-kid forced refetch against the last fetch of any kind, with the interval defaulting to the cache TTL. Every TTL refresh reset the limit, so with the defaults an unknown kid never forced a refetch and a rotated-in key was refused until the cache expired (OIDC Core 10.1.1 expects the verifier to go back to the jwks_uri on an unfamiliar kid). The limit now counts only forced refetches, and defaults to min(cache TTL, 1 minute). A miss on a set fetched since the request arrived is negative-cached instead of refetched, so a cold start with an unknown kid still costs one fetch. Tests now cover the default options too. Co-Authored-By: Claude Opus 5.5 --- GETTING_STARTED.md | 5 +- keys/client_jwks.go | 6 +- keys/client_jwks_internal_test.go | 54 +++++-- keys/ephemeral/clientkeysource.go | 40 +++-- .../ephemeral/clientkeysource_refresh_test.go | 51 ++++--- keys/ephemeral/ephemeral_test.go | 30 ++-- keys/jwkscache.go | 53 +++++-- keys/jwkscache_internal_test.go | 2 +- keys/jwksissuer.go | 13 +- keys/jwksissuer_internal_test.go | 139 ++++++++++-------- 10 files changed, 246 insertions(+), 147 deletions(-) diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index eb1dc1f9..8d7c8b58 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -1747,8 +1747,9 @@ in the right order: for your JWKS — this module's own `keys.NewJWKSIssuerKeySource` (a client resolving your keys) refetches on an unrecognized kid at most once per its minimum refresh interval (`keys.WithMinRefreshInterval`, - by default its cache TTL), so it picks up a new key within one TTL, - and you may have consumers you don't control caching longer. + by default a minute, or the cache TTL if shorter), but you may have + consumers you don't control that only refetch once their cache + expires. When in doubt, wait longer than you think you need to; this step costs nothing but time. 4. Cut `Sign`/`PublicKey` over to the new key. New ID tokens, JWT diff --git a/keys/client_jwks.go b/keys/client_jwks.go index a10f190e..d80d7179 100644 --- a/keys/client_jwks.go +++ b/keys/client_jwks.go @@ -91,8 +91,10 @@ func WithClientJWKSCacheTTL(d time.Duration) ClientJWKSOption { // WithClientJWKSMinRefreshInterval bounds how often a request naming a // kid a client's cached set lacks may force a refetch of that client's -// set, as WithMinRefreshInterval does for JWKSIssuerKeySource. Defaults -// to the cache TTL; a zero or negative duration means that default. +// set, as WithMinRefreshInterval does for JWKSIssuerKeySource: at most +// one forced refetch per interval, counted from the last forced +// refetch. Defaults to min(cache TTL, 1 minute); a zero or negative +// duration means that default. func WithClientJWKSMinRefreshInterval(d time.Duration) ClientJWKSOption { return func(s *ClientJWKSSource) { s.timing.minRefreshInterval = d } } diff --git a/keys/client_jwks_internal_test.go b/keys/client_jwks_internal_test.go index ab4a1597..cfb0858c 100644 --- a/keys/client_jwks_internal_test.go +++ b/keys/client_jwks_internal_test.go @@ -194,32 +194,62 @@ func TestClientJWKSSourceCachesPerClient(t *testing.T) { } // TestClientJWKSSourceRefetchesOnRotation: a kid the cached set lacks -// (the client rotated keys) forces a refetch, once the minimum refresh -// interval has passed, and the new key then resolves. +// (the client rotated keys) forces a refetch at once, however long the +// cache TTL; a second rotation within the minimum refresh interval of +// that forced refetch waits for the interval, then resolves. func TestClientJWKSSourceRefetchesOnRotation(t *testing.T) { doc, _ := es256JWKS(t, "old") src, srv, _ := newURISource(t, map[fapi.ClientID][]byte{"a": doc}, WithClientJWKSCacheTTL(time.Hour), WithClientJWKSMinRefreshInterval(time.Minute)) clock := newManualClock() src.now = clock.Now + assertClientRotations(t, src, srv, clock, time.Minute) +} + +// TestClientJWKSSourceDefaultRefetchesOnRotation: the same with the +// default options, whose 5-minute cache TTL used to rate-limit forced +// refetches out of existence (every TTL refresh reset the limit). +func TestClientJWKSSourceDefaultRefetchesOnRotation(t *testing.T) { + doc, _ := es256JWKS(t, "old") + src, srv, _ := newURISource(t, map[fapi.ClientID][]byte{"a": doc}) + clock := newManualClock() + src.now = clock.Now + assertClientRotations(t, src, srv, clock, defaultMinRefreshInterval) +} +// assertClientRotations drives two key rotations of client "a" against +// src, whose minimum refresh interval is interval. +func assertClientRotations(t *testing.T, src *ClientJWKSSource, srv *jwksServer, clock *manualClock, interval time.Duration) { + t.Helper() if got, err := resolveES256(t, src, "a", "old"); err != nil || len(got) != 1 { t.Fatalf("old kid: got %d keys, err %v", len(got), err) } rotated, pubs := es256JWKS(t, "new") srv.set("/a", rotated) + clock.Advance(time.Second) + assertClientKey(t, src, "new", pubs["new"]) + if n := srv.fetchCount("/a"); n != 2 { + t.Fatalf("fetches after the first rotation = %d, want 2", n) + } - // Within the minimum refresh interval of the first fetch, the miss - // doesn't refetch. - if got, err := resolveES256(t, src, "a", "new"); err != nil || len(got) != 0 { - t.Fatalf("new kid within the interval: got %d keys, err %v; want none", len(got), err) + rotatedAgain, pubsAgain := es256JWKS(t, "newer") + srv.set("/a", rotatedAgain) + clock.Advance(interval / 2) + if got, err := resolveES256(t, src, "a", "newer"); err != nil || len(got) != 0 { + t.Fatalf("newer kid within the interval: got %d keys, err %v; want none", len(got), err) } - clock.Advance(2 * time.Minute) - got, err := resolveES256(t, src, "a", "new") - if err != nil || len(got) != 1 || !got[0].PublicKey.(*ecdsa.PublicKey).Equal(pubs["new"]) { - t.Fatalf("new kid after the interval: got %+v, err %v; want the rotated key", got, err) + clock.Advance(interval / 2) + assertClientKey(t, src, "newer", pubsAgain["newer"]) + if n := srv.fetchCount("/a"); n != 3 { + t.Fatalf("fetches after the second rotation = %d, want 3", n) } - if n := srv.fetchCount("/a"); n != 2 { - t.Fatalf("fetches = %d, want 2", n) +} + +// assertClientKey asserts that kid resolves for client "a" to want. +func assertClientKey(t *testing.T, src *ClientJWKSSource, kid string, want *ecdsa.PublicKey) { + t.Helper() + got, err := resolveES256(t, src, "a", kid) + if err != nil || len(got) != 1 || !got[0].PublicKey.(*ecdsa.PublicKey).Equal(want) { + t.Fatalf("kid %q: got %+v, err %v; want the rotated key", kid, got, err) } } diff --git a/keys/ephemeral/clientkeysource.go b/keys/ephemeral/clientkeysource.go index 2fc3214a..7a69104f 100644 --- a/keys/ephemeral/clientkeysource.go +++ b/keys/ephemeral/clientkeysource.go @@ -19,6 +19,10 @@ import ( // WithCacheTTL. const defaultClientJWKSCacheTTL = 5 * time.Minute +// defaultMinRefreshInterval caps how often, by default, a request naming +// a kid a client's cached JWKS lacks may force a refetch of it. +const defaultMinRefreshInterval = time.Minute + // ClientKeySpec is one registered client's key material — // either an inline static JWKS or a JWKS URI to fetch live. Deliberately // carries only what ClientKeySource needs (not a storage.RegisteredClient @@ -47,11 +51,13 @@ func WithCacheTTL(d time.Duration) Option { // WithMinRefreshInterval bounds how often a request naming a kid the // cached JWKS doesn't have may force a live refetch of that client's -// JWKS. Defaults to the cache TTL, as keys.JWKSIssuerKeySource's own -// option does: a rotated key is still picked up within one TTL, while a -// caller sending distinct unknown kids (taken from an unverified JWT -// header, so attacker-controlled) forces at most one fetch per client -// per interval. A zero or negative duration means that default. +// JWKS. Defaults to min(cache TTL, 1 minute), as +// keys.JWKSIssuerKeySource's own option does: a rotated key is picked up +// within one interval of its first use, while a caller sending distinct +// unknown kids (taken from an unverified JWT header, so +// attacker-controlled) forces at most one fetch per client per +// interval. Only forced refetches count against it, not the TTL ones. A +// zero or negative duration means that default. func WithMinRefreshInterval(d time.Duration) Option { return func(s *ClientKeySource) { s.minRefreshInterval = d } } @@ -82,6 +88,7 @@ type clientKeyEntry struct { cached *clientKeys cachedAt time.Time lastAttempt time.Time // the last fetch, successful or not + lastForced time.Time // the last fetch made while the cached set was fresh lastErr error // the last fetch's error, nil if it succeeded // fetchMu serializes fetches, so concurrent callers that all need @@ -133,7 +140,7 @@ func NewClientKeySource(fetcher *fapihttp.Client, specs []ClientKeySpec, opts .. return nil, fmt.Errorf("ephemeral: cache TTL must be positive") } if s.minRefreshInterval <= 0 { - s.minRefreshInterval = s.cacheTTL + s.minRefreshInterval = min(s.cacheTTL, defaultMinRefreshInterval) } for _, spec := range specs { if spec.ClientID == "" { @@ -235,21 +242,23 @@ func (s *ClientKeySource) ResolveEncryptionKeys(ctx context.Context, req keys.Cl // while fresh, refetched once stale, with the same stale-key handling // keys.JWKSIssuerKeySource applies: a fresh set without wantKeyID (a // client that may have rotated keys since the last fetch) is refetched -// too, but at most once per minRefreshInterval, since wantKeyID comes -// from an unverified JWT header. Fetches are serialized per client, and +// too, but at most once per minRefreshInterval of the last such forced +// refetch, since wantKeyID comes from an unverified JWT header, and not +// if the set was fetched since the request arrived. Fetches are serialized per client, and // a failed one isn't retried for refreshBackoff — its error is // returned instead, or the cached set while it is still fresh. func (s *ClientKeySource) current(ctx context.Context, entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string) (clientKeys, error) { if entry.jwksURL == nil { return entry.static, nil } - if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids); done { + arrived := s.now() + if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids, arrived); done { return keys, err } entry.fetchMu.Lock() defer entry.fetchMu.Unlock() // Another caller may have fetched while this one waited. - if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids); done { + if keys, done, err := s.cachedOrRateLimited(entry, wantKeyID, kids, arrived); done { return keys, err } return s.refetch(ctx, entry) @@ -258,9 +267,11 @@ func (s *ClientKeySource) current(ctx context.Context, entry *clientKeyEntry, wa // cachedOrRateLimited reports (done=true) the answer current gives // without fetching: the cached set when it is fresh and has wantKeyID // (or none was asked for), or when a fetch for a missing kid isn't -// allowed yet; the last fetch's error while it is within +// allowed yet (the set was fetched since the request arrived at +// arrived, or the last forced refetch was within minRefreshInterval); +// the last fetch's error while it is within // refreshBackoff. done=false means current should fetch now. -func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string) (clientKeys, bool, error) { +func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID string, kids func(clientKeys) []string, arrived time.Time) (clientKeys, bool, error) { entry.mu.Lock() defer entry.mu.Unlock() now := s.now() @@ -275,7 +286,7 @@ func (s *ClientKeySource) cachedOrRateLimited(entry *clientKeyEntry, wantKeyID s } return clientKeys{}, true, entry.lastErr } - if fresh && sinceAttempt < s.minRefreshInterval { + if fresh && (!entry.cachedAt.Before(arrived) || now.Sub(entry.lastForced) < s.minRefreshInterval) { return *entry.cached, true, nil } return clientKeys{}, false, nil @@ -293,6 +304,9 @@ func (s *ClientKeySource) refetch(ctx context.Context, entry *clientKeyEntry) (c entry.mu.Lock() defer entry.mu.Unlock() entry.lastAttempt = s.now() + if entry.cached != nil && entry.lastAttempt.Sub(entry.cachedAt) < s.cacheTTL { + entry.lastForced = entry.lastAttempt + } entry.lastErr = err if err != nil { return clientKeys{}, err diff --git a/keys/ephemeral/clientkeysource_refresh_test.go b/keys/ephemeral/clientkeysource_refresh_test.go index 372c1522..cde22b11 100644 --- a/keys/ephemeral/clientkeysource_refresh_test.go +++ b/keys/ephemeral/clientkeysource_refresh_test.go @@ -73,7 +73,7 @@ func (h refreshHarness) resolve(kid string) (keys.VerificationKeySet, error) { // TestClientKeySourceBoundsUnknownKeyIDRefetches: a kid is taken from an // unverified JWT header, so a stream of distinct unknown kids within -// the minimum refresh interval forces no fetch beyond the first. +// the minimum refresh interval forces one refetch, and no more. func TestClientKeySourceBoundsUnknownKeyIDRefetches(t *testing.T) { h := newRefreshHarness(t) if _, err := h.resolve("kid-1"); err != nil { @@ -86,8 +86,8 @@ func TestClientKeySourceBoundsUnknownKeyIDRefetches(t *testing.T) { t.Fatalf("resolve(unknown kid) = %+v, %v; want no keys, no error", set.Keys, err) } } - if got := h.fetches.Load(); got != 1 { - t.Fatalf("fetches = %d after 50 unknown kids within the interval, want 1", got) + if got := h.fetches.Load(); got != 2 { + t.Fatalf("fetches = %d after 50 unknown kids within the interval, want 2 (the first lookup plus one forced refetch)", got) } } @@ -169,27 +169,30 @@ func TestNewClientKeySourceRejectsNonPositiveCacheTTL(t *testing.T) { } } -// TestClientKeySourceMinRefreshIntervalDefaultsToCacheTTL: without -// WithMinRefreshInterval, an unknown kid forces at most one refetch per -// cache TTL, as keys.JWKSIssuerKeySource's own default does. -func TestClientKeySourceMinRefreshIntervalDefaultsToCacheTTL(t *testing.T) { - h := newRefreshHarness(t, WithCacheTTL(time.Minute)) - if _, err := h.resolve("kid-1"); err != nil { - t.Fatalf("resolve(kid-1): %v", err) - } - *h.now = h.now.Add(59 * time.Second) - if _, err := h.resolve("unknown"); err != nil { - t.Fatalf("resolve(unknown): %v", err) - } - if got := h.fetches.Load(); got != 1 { - t.Fatalf("fetches = %d for an unknown kid within the cache TTL, want 1", got) - } - *h.now = h.now.Add(2 * time.Second) - if _, err := h.resolve("unknown"); err != nil { - t.Fatalf("resolve(unknown): %v", err) - } - if got := h.fetches.Load(); got != 2 { - t.Fatalf("fetches = %d once the cache TTL has passed, want 2", got) +// TestClientKeySourceMinRefreshIntervalDefault: without +// WithMinRefreshInterval, an unknown kid forces a refetch at once, then +// at most one more per minute however long the cache TTL, as +// keys.JWKSIssuerKeySource's own default does. +func TestClientKeySourceMinRefreshIntervalDefault(t *testing.T) { + h := newRefreshHarness(t, WithCacheTTL(time.Hour)) + steps := []struct { + advance time.Duration + kid string + fetches int32 + }{ + {0, "kid-1", 1}, + {time.Second, "unknown-1", 2}, + {59 * time.Second, "unknown-2", 2}, + {time.Second, "unknown-3", 3}, + } + for _, step := range steps { + *h.now = h.now.Add(step.advance) + if _, err := h.resolve(step.kid); err != nil { + t.Fatalf("resolve(%s): %v", step.kid, err) + } + if got := h.fetches.Load(); got != step.fetches { + t.Fatalf("fetches = %d after resolve(%s), want %d", got, step.kid, step.fetches) + } } } diff --git a/keys/ephemeral/ephemeral_test.go b/keys/ephemeral/ephemeral_test.go index 2272fd6a..b0f63f74 100644 --- a/keys/ephemeral/ephemeral_test.go +++ b/keys/ephemeral/ephemeral_test.go @@ -264,8 +264,9 @@ func TestClientKeySourceCachesWithinTTL(t *testing.T) { // TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh covers // current's stale-key handling: a client that has rotated keys since the // last fetch isn't stuck with a cached set that lacks the newly -// requested kid until the TTL expires — it is refetched once the minimum -// refresh interval has passed, and not before. +// requested kid until the TTL expires — it is refetched at once, and a +// further unknown kid forces another refetch only once the minimum +// refresh interval has passed. func TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh(t *testing.T) { priv1, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if err != nil { @@ -321,28 +322,33 @@ func TestClientKeySourceRefetchesOnUnknownKeyIDEvenWhenCacheFresh(t *testing.T) mu.Lock() rotated = true mu.Unlock() - resolveKid2 := func() keys.VerificationKeySet { + resolve := func(kid string) keys.VerificationKeySet { t.Helper() set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ - ClientID: "client-1", Algorithm: fapi.ES256, KeyID: "kid-2", + ClientID: "client-1", Algorithm: fapi.ES256, KeyID: kid, }) if err != nil { - t.Fatalf("ResolveVerificationKeys(kid-2): %v", err) + t.Fatalf("ResolveVerificationKeys(%s): %v", kid, err) } return set } + // The rotated-in kid forces a refetch at once, well within the TTL. + now = now.Add(time.Second) + if set := resolve("kid-2"); len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-2" || fetches != 2 { + t.Fatalf("rotated kid: keys = %+v, fetches = %d; want kid-2 and 2 (rotation should force a refetch)", set.Keys, fetches) + } + + // Another unknown kid within the minimum refresh interval of that + // forced refetch doesn't force another. now = now.Add(5 * time.Second) - if set := resolveKid2(); len(set.Keys) != 0 || fetches != 1 { - t.Fatalf("within the minimum refresh interval: keys = %+v, fetches = %d; want none and 1", set.Keys, fetches) + if set := resolve("kid-3"); len(set.Keys) != 0 || fetches != 2 { + t.Fatalf("within the minimum refresh interval: keys = %+v, fetches = %d; want none and 2", set.Keys, fetches) } now = now.Add(6 * time.Second) - if set := resolveKid2(); len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-2" { - t.Fatalf("Keys = %+v, want one key with kid-2 (rotation should force a refetch)", set.Keys) - } - if fetches != 2 { - t.Fatalf("fetches = %d, want 2 (an unknown kid forces a refetch within TTL once the interval has passed)", fetches) + if set := resolve("kid-3"); len(set.Keys) != 0 || fetches != 3 { + t.Fatalf("after the interval: keys = %+v, fetches = %d; want none and 3 (an unknown kid forces a refetch once the interval has passed)", set.Keys, fetches) } } diff --git a/keys/jwkscache.go b/keys/jwkscache.go index 8342e523..0be48cdb 100644 --- a/keys/jwkscache.go +++ b/keys/jwkscache.go @@ -15,7 +15,12 @@ import ( // refetch, at most once per minRefreshInterval, with a miss for that // kid negative-cached for as long, since the kid comes from an // unverified JWT header and so is attacker-controlled. Concurrent -// fetches are single-flighted. +// fetches are single-flighted. The forced-refetch rate limit counts +// only forced refetches, never the plain TTL refreshes, so a rotated-in +// kid is picked up within minRefreshInterval of its first use whatever +// the TTL (OIDC Core §10.1.1: "The verifier knows to go back to the +// jwks_uri location to re-retrieve the keys when it sees an unfamiliar +// kid value"). // // It is shared by JWKSIssuerKeySource (one cache per source) and // ClientJWKSSource (one per client), and stays unexported: each source @@ -32,7 +37,7 @@ type jwksCache[V any] struct { cached V haveCached bool cachedAt time.Time - lastAttempt time.Time + lastForced time.Time lastRefreshErr error lastRefreshErrAt time.Time inflight *refreshCall[V] @@ -55,6 +60,13 @@ type refreshCall[V any] struct { // occasional extra refresh, which minRefreshInterval already bounds. const maxNegativeKIDs = 1024 +// defaultMinRefreshInterval is how often, by default, an unknown kid +// may force a refetch of a JWK Set: short enough that a signer's key +// rotation is picked up within a minute of the new kid's first use, +// long enough that a flood of random kids costs the JWKS endpoint at +// most one extra fetch a minute per set. +const defaultMinRefreshInterval = time.Minute + // jwksCacheTiming is a jwksCache's three intervals. type jwksCacheTiming struct { cacheTTL time.Duration @@ -63,10 +75,11 @@ type jwksCacheTiming struct { } // withDefaults fills t's unset intervals: minRefreshInterval defaults to -// cacheTTL, refreshBackoff to min(cacheTTL, 5s). +// min(cacheTTL, defaultMinRefreshInterval), refreshBackoff to +// min(cacheTTL, 5s). func (t jwksCacheTiming) withDefaults() jwksCacheTiming { if t.minRefreshInterval <= 0 { - t.minRefreshInterval = t.cacheTTL + t.minRefreshInterval = min(t.cacheTTL, defaultMinRefreshInterval) } if t.refreshBackoff <= 0 { t.refreshBackoff = min(t.cacheTTL, 5*time.Second) @@ -89,6 +102,7 @@ func newJWKSCache[V any](timing jwksCacheTiming, now func() time.Time, fetch fun // is set and matches nothing, it forces one refresh (rate-limited and // single-flighted, see jwksCache) before giving up with an empty result. func resolveFromJWKSCache[V, K any](ctx context.Context, c *jwksCache[V], kid string, match func(V) []K) ([]K, error) { + arrived := c.now() current, err := c.current(ctx) if err != nil { return nil, err @@ -97,7 +111,7 @@ func resolveFromJWKSCache[V, K any](ctx context.Context, c *jwksCache[V], kid st if len(matched) > 0 || kid == "" { return matched, nil } - if !c.shouldForceRefresh(kid) { + if !c.reserveForcedRefresh(kid, arrived) { return nil, nil } fresh, err := c.refreshSingleflight(ctx) @@ -135,20 +149,30 @@ func (c *jwksCache[V]) current(ctx context.Context) (V, error) { return c.refreshSingleflight(ctx) } -// shouldForceRefresh reports whether an unknown-kid miss on kid may -// force a live refresh right now: it may not if kid is still -// negative-cached, or if the last refresh attempt was within -// minRefreshInterval. -func (c *jwksCache[V]) shouldForceRefresh(kid string) bool { +// reserveForcedRefresh reports whether an unknown-kid miss on kid, in +// a request that arrived at arrived, may force a live refresh right +// now, and if so records it as the last forced refresh. It may not if +// kid is still negative-cached, or if the last forced refresh was +// within minRefreshInterval. A plain TTL refresh doesn't count against +// that limit (if it did, a minRefreshInterval as long as cacheTTL would +// let a kid miss force nothing at all), but a set fetched since the +// request arrived is already as fresh as a forced refresh would make +// it, so a miss on it is negative-cached instead of refetched. +func (c *jwksCache[V]) reserveForcedRefresh(kid string, arrived time.Time) bool { c.mu.Lock() defer c.mu.Unlock() now := c.now() if retryAt, ok := c.negativeKIDs[kid]; ok && now.Before(retryAt) { return false } - if !c.lastAttempt.IsZero() && now.Sub(c.lastAttempt) < c.minRefreshInterval { + if c.haveCached && !c.cachedAt.Before(arrived) { + c.rememberMissLocked(kid, now) + return false + } + if !c.lastForced.IsZero() && now.Sub(c.lastForced) < c.minRefreshInterval { return false } + c.lastForced = now return true } @@ -159,7 +183,11 @@ func (c *jwksCache[V]) shouldForceRefresh(kid string) bool { func (c *jwksCache[V]) rememberMiss(kid string) { c.mu.Lock() defer c.mu.Unlock() - now := c.now() + c.rememberMissLocked(kid, c.now()) +} + +// rememberMissLocked is rememberMiss with c.mu held. +func (c *jwksCache[V]) rememberMissLocked(kid string, now time.Time) { if len(c.negativeKIDs) >= maxNegativeKIDs { for k, retryAt := range c.negativeKIDs { if !now.Before(retryAt) { @@ -185,7 +213,6 @@ func (c *jwksCache[V]) refreshSingleflight(ctx context.Context) (V, error) { } call := &refreshCall[V]{done: make(chan struct{})} c.inflight = call - c.lastAttempt = c.now() c.mu.Unlock() go func() { diff --git a/keys/jwkscache_internal_test.go b/keys/jwkscache_internal_test.go index f2e7d0f6..8cea8b69 100644 --- a/keys/jwkscache_internal_test.go +++ b/keys/jwkscache_internal_test.go @@ -21,7 +21,7 @@ func TestJWKSCacheBoundsNegativeKeyIDs(t *testing.T) { if n := len(c.negativeKIDs); n != maxNegativeKIDs { t.Fatalf("negative cache holds %d kids, want %d", n, maxNegativeKIDs) } - if c.shouldForceRefresh("kid-0") { + if c.reserveForcedRefresh("kid-0", clock.Now()) { t.Fatalf("a negative-cached kid may force a refresh") } clock.Advance(2 * time.Hour) diff --git a/keys/jwksissuer.go b/keys/jwksissuer.go index 0ceafdb5..2f287759 100644 --- a/keys/jwksissuer.go +++ b/keys/jwksissuer.go @@ -47,12 +47,13 @@ type JWKSIssuerKeySource struct { type JWKSOption func(*JWKSIssuerKeySource) // WithMinRefreshInterval bounds how often an unknown-kid miss may force -// a live refetch, independent of CacheTTL. Defaults to CacheTTL, which -// means an unknown kid can force at most one extra fetch per TTL -// window — a real key rotation is still picked up within one TTL, the -// same worst case as plain TTL expiry, while an attacker sending -// requests with distinct unknown kids cannot force more than one -// upstream fetch per interval. +// a live refetch, independent of CacheTTL: at most one forced refetch +// per interval, counted from the last forced refetch (the plain TTL +// refreshes don't count). Defaults to min(CacheTTL, 1 minute), so a +// real key rotation is picked up within a minute of the new kid's first +// use (OIDC Core §10.1.1), while an attacker sending requests with +// distinct unknown kids cannot force more than one upstream fetch per +// interval. A zero or negative duration means that default. func WithMinRefreshInterval(d time.Duration) JWKSOption { return func(s *JWKSIssuerKeySource) { s.timing.minRefreshInterval = d } } diff --git a/keys/jwksissuer_internal_test.go b/keys/jwksissuer_internal_test.go index fbf26d1f..9bfbd2f7 100644 --- a/keys/jwksissuer_internal_test.go +++ b/keys/jwksissuer_internal_test.go @@ -87,16 +87,17 @@ func TestUnknownKIDBurstCoalescesToSingleFetch(t *testing.T) { if err != nil { t.Fatalf("NewJWKSIssuerKeySource: %v", err) } + // A stopped clock: every call arrives no later than the fetch. + src.now = newManualClock().Now // No priming: the cache starts empty, so every one of these // concurrent, distinct-unknown-kid calls independently discovers a - // cache miss and races to populate it via currentKeys' own + // cache miss and races to populate it via current's own // refreshSingleflight call — this is the single-flight coalescing - // this test exercises. Once that first fetch completes, each call's - // own forced-refresh attempt (the kid still won't match) is in turn - // suppressed by minRefreshInterval, since it was just satisfied by - // the same fetch a moment ago — so the total across all 50 must - // still be exactly one fetch. + // this test exercises. Once that first fetch completes, no call + // forces another: the set each one misses in was fetched since it + // arrived — so the total across all 50 must still be exactly one + // fetch. const burst = 50 var wg sync.WaitGroup wg.Add(burst) @@ -139,39 +140,37 @@ func TestUnknownKIDRateLimited(t *testing.T) { src.now = clock.Now // No priming: the cache starts empty, so this first unknown-kid - // lookup does the initial population fetch itself (via - // currentKeys), which also stamps lastAttempt. - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-1", - }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-1): %v", err) - } - if got := atomic.LoadInt32(&fetches); got != 1 { - t.Fatalf("fetches after first miss = %d, want 1", got) - } + // lookup does the initial population fetch itself (via current); + // the set it misses in was fetched for it, so no forced refresh. + assertFetchesAfterMiss(t, src, "unknown-1", &fetches, 1) - // A second, different unknown kid within minRefreshInterval must not - // force another fetch. + // Another unknown kid, on a set fetched before it arrived, forces + // one refetch: a plain fetch doesn't count against the limit. clock.Advance(30 * time.Second) - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-2", - }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-2): %v", err) - } - if got := atomic.LoadInt32(&fetches); got != 1 { - t.Fatalf("fetches after second miss within interval = %d, want 1 (rate limit not enforced)", got) - } + assertFetchesAfterMiss(t, src, "unknown-2", &fetches, 2) + + // A third, different unknown kid within minRefreshInterval of that + // forced refetch must not force another. + clock.Advance(59 * time.Second) + assertFetchesAfterMiss(t, src, "unknown-3", &fetches, 2) // Once minRefreshInterval has elapsed, a further unknown kid is // allowed to force a new fetch. - clock.Advance(31 * time.Second) + clock.Advance(time.Second) + assertFetchesAfterMiss(t, src, "unknown-4", &fetches, 3) +} + +// assertFetchesAfterMiss resolves the unknown kid against src and +// asserts that the upstream has then been fetched want times. +func assertFetchesAfterMiss(t *testing.T, src *JWKSIssuerKeySource, kid string, fetches *int32, want int32) { + t.Helper() if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "unknown-3", + Algorithm: fapi.ES256, KeyID: kid, }); err != nil { - t.Fatalf("ResolveIssuerKeys(unknown-3): %v", err) + t.Fatalf("ResolveIssuerKeys(%s): %v", kid, err) } - if got := atomic.LoadInt32(&fetches); got != 2 { - t.Fatalf("fetches after interval elapsed = %d, want 2", got) + if got := atomic.LoadInt32(fetches); got != want { + t.Fatalf("fetches after a miss on %s = %d, want %d", kid, got, want) } } @@ -207,15 +206,34 @@ func TestNegativeCacheSuppressesRefetch(t *testing.T) { } } +// TestRotationStillResolvesAfterInterval: a rotated-in kid forces a +// refetch at once, however long the cache TTL; a second rotation within +// the minimum refresh interval of that forced refetch waits for the +// interval, then resolves. func TestRotationStillResolvesAfterInterval(t *testing.T) { - var newKID atomic.Bool + assertIssuerRotations(t, time.Hour, time.Minute, WithMinRefreshInterval(time.Minute)) +} + +// TestRotationResolvesWithDefaultOptions: the same with no options, +// where the minimum refresh interval used to default to the cache TTL +// and every TTL refresh reset it, so an unknown kid never forced a +// refetch (OIDC Core §10.1.1). +func TestRotationResolvesWithDefaultOptions(t *testing.T) { + assertIssuerRotations(t, 15*time.Minute, defaultMinRefreshInterval) +} + +// assertIssuerRotations drives two key rotations against a +// JWKSIssuerKeySource built with cacheTTL and opts, whose minimum +// refresh interval is interval. +func assertIssuerRotations(t *testing.T, cacheTTL, interval time.Duration, opts ...JWKSOption) { + t.Helper() + var kid atomic.Value + kid.Store("kid-original") + var fetches atomic.Int32 ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fetches.Add(1) w.Header().Set("Content-Type", "application/json") - if newKID.Load() { - fmt.Fprint(w, testP256JWK(t, "kid-rotated")) - } else { - fmt.Fprint(w, testP256JWK(t, "kid-original")) - } + fmt.Fprint(w, testP256JWK(t, kid.Load().(string))) })) defer ts.Close() @@ -224,42 +242,39 @@ func TestRotationStillResolvesAfterInterval(t *testing.T) { t.Fatalf("ParseEndpointURL: %v", err) } clock := newManualClock() - src, err := NewJWKSIssuerKeySource(testFetcher(t, ts), jwksURI, time.Hour, WithMinRefreshInterval(time.Minute)) + src, err := NewJWKSIssuerKeySource(testFetcher(t, ts), jwksURI, cacheTTL, opts...) if err != nil { t.Fatalf("NewJWKSIssuerKeySource: %v", err) } src.now = clock.Now - if _, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-original", - }); err != nil { - t.Fatalf("prime: %v", err) + assertIssuerKeyCount(t, src, "kid-original", 1) + kid.Store("kid-rotated") + clock.Advance(time.Second) + assertIssuerKeyCount(t, src, "kid-rotated", 1) + if n := fetches.Load(); n != 2 { + t.Fatalf("fetches after the first rotation = %d, want 2", n) } - // The priming resolve above already stamped lastAttempt at clock's - // current value, so this immediate query for the (now rotated) kid - // is still within minRefreshInterval and must not force a refresh. - newKID.Store(true) - set, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-rotated", - }) - if err != nil { - t.Fatalf("ResolveIssuerKeys(kid-rotated, within interval): %v", err) - } - if len(set.Keys) != 0 { - t.Fatalf("Keys = %+v, want none yet (forced refresh should still be rate-limited)", set.Keys) + kid.Store("kid-rotated-again") + clock.Advance(interval / 2) + assertIssuerKeyCount(t, src, "kid-rotated-again", 0) + clock.Advance(interval / 2) + assertIssuerKeyCount(t, src, "kid-rotated-again", 1) + if n := fetches.Load(); n != 3 { + t.Fatalf("fetches after the second rotation = %d, want 3", n) } +} - // After minRefreshInterval elapses, the same kid resolves. - clock.Advance(2 * time.Minute) - set, err = src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{ - Algorithm: fapi.ES256, KeyID: "kid-rotated", - }) +// assertIssuerKeyCount asserts that src resolves want ES256 keys for kid. +func assertIssuerKeyCount(t *testing.T, src *JWKSIssuerKeySource, kid string, want int) { + t.Helper() + set, err := src.ResolveIssuerKeys(context.Background(), IssuerKeyRequest{Algorithm: fapi.ES256, KeyID: kid}) if err != nil { - t.Fatalf("ResolveIssuerKeys(kid-rotated, after interval): %v", err) + t.Fatalf("ResolveIssuerKeys(%s): %v", kid, err) } - if len(set.Keys) != 1 || set.Keys[0].KeyID != "kid-rotated" { - t.Fatalf("Keys = %+v, want one key with kid-rotated", set.Keys) + if len(set.Keys) != want { + t.Fatalf("ResolveIssuerKeys(%s) = %+v, want %d keys", kid, set.Keys, want) } } From 191824d5cab80d66aa926feb06d9c38c195a994a Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 13:57:39 +0800 Subject: [PATCH 2/7] fix(jose): compare a JWT typ case-insensitively in ASCII only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TypeIs lowercased typ with strings.ToLower, whose Unicode case mapping folds the Kelvin sign U+212A onto "k", so "trust-marK+jwt" matched "trust-mark+jwt". RFC 7515 4.1.9 compares typ as an RFC 2045 media type, case-insensitive in ASCII. Use canonical.LowerASCII, as the host and scheme comparisons already do. Co-Authored-By: Claude Opus 5.5 --- internal/jose/typ.go | 12 ++++++++---- internal/jose/typ_test.go | 5 +++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/jose/typ.go b/internal/jose/typ.go index 08895104..73e16c5d 100644 --- a/internal/jose/typ.go +++ b/internal/jose/typ.go @@ -4,6 +4,8 @@ import ( "errors" "slices" "strings" + + "github.com/idfoundry/fapigo/internal/canonical" ) // ErrOtherExplicitType reports a JWT whose "typ" names a different, @@ -15,15 +17,17 @@ var ErrOtherExplicitType = errors.New("jose: header typ names another kind of JW var ErrUnexpectedType = errors.New(`jose: header typ is neither absent, "JWT", nor the expected media type`) // TypeIs reports whether typ names the media type want, comparing as -// RFC 7515 §4.1.9 asks: case-insensitively (RFC 2045), and with -// "application/" implied for a value containing no '/'. So -// "application/at+jwt" and "AT+JWT" are both TypeIs(_, "at+jwt"). +// RFC 7515 §4.1.9 asks: case-insensitively (RFC 2045), in ASCII only, +// and with "application/" implied for a value containing no '/'. So +// "application/at+jwt" and "AT+JWT" are both TypeIs(_, "at+jwt"), but +// a non-ASCII letter that Unicode folds onto an ASCII one (the Kelvin +// sign U+212A onto "k") doesn't match it. func TypeIs(typ, want string) bool { return mediaType(typ) == mediaType(want) } func mediaType(typ string) string { - typ = strings.ToLower(typ) + typ = canonical.LowerASCII(typ) if !strings.Contains(typ, "/") { typ = "application/" + typ } diff --git a/internal/jose/typ_test.go b/internal/jose/typ_test.go index af152a26..29331d2d 100644 --- a/internal/jose/typ_test.go +++ b/internal/jose/typ_test.go @@ -20,6 +20,11 @@ func TestTypeIs(t *testing.T) { {"text/at+jwt", "at+jwt", false}, {"at+jwt ", "at+jwt", false}, {"dpop+jwt", "at+jwt", false}, + // Unicode folds the Kelvin sign U+212A onto "k"; the media type + // comparison is ASCII-only. + {"trust-mar\u212a+jwt", "trust-mark+jwt", false}, + {"jw\u212a-set+jwt", "jwk-set+jwt", false}, + {"TRUST-MARK+JWT", "trust-mark+jwt", true}, } { if got := TypeIs(tc.typ, tc.want); got != tc.is { t.Errorf("TypeIs(%q, %q) = %v, want %v", tc.typ, tc.want, got, tc.is) From de97ac30f887ec2e57487e137d690968f089f607 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:11:53 +0800 Subject: [PATCH 3/7] fix(server,client)!: refuse a configured issuer with a query v0.65.0 made fapi.ParseIssuerURL refuse an issuer with a query, as RFC 8414 section 2 and OpenID Connect Core section 2 require, but server.New and client.New accepted any fapi.URL as Config.Issuer. An issuer built with fapi.ParseEndpointURL, which allows a query, was still accepted in production, and the server published and signed it. Both constructors now refuse an issuer with a query, even an empty "?", at either assurance level. BREAKING CHANGE: server.New and client.New refuse a Config.Issuer with a query, including an empty "?". Parse the issuer with fapi.ParseIssuerURL and move any tenant selector into the path. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 29 ++++++++++++++++++++++++++++ client/client.go | 3 +++ client/config.go | 4 +++- client/issuer_query_test.go | 31 ++++++++++++++++++++++++++++++ server/config.go | 4 +++- server/issuer_query_test.go | 38 +++++++++++++++++++++++++++++++++++++ server/server.go | 3 +++ 7 files changed, 110 insertions(+), 2 deletions(-) create mode 100644 client/issuer_query_test.go create mode 100644 server/issuer_query_test.go diff --git a/UPGRADING.md b/UPGRADING.md index ebabbccb..8a3adc37 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -164,6 +164,35 @@ result, err := srv.IssueAccessToken(ctx, server.IssueAccessTokenRequest{ }) ``` +### `server.New` and `client.New` refuse an issuer with a query (server, client) + +**Affects:** a server's or a client's `Config.Issuer` with a query, +even an empty `?`. `fapi.ParseIssuerURL` has refused one since v0.65.0, +so this only reaches an issuer built another way, such as with +`fapi.ParseEndpointURL`, which allows a query. + +**Why:** RFC 8414 §2 and OpenID Connect Core §2 define an issuer as a +URL with no query or fragment. v0.65.0 made `fapi.ParseIssuerURL` +refuse a query, but `New` accepted any `fapi.URL` as the issuer, so a +server could still publish and sign (`iss` in ID tokens, authorization +responses and JARM) an issuer conformant clients refuse. + +**What fails:** `New` returns an error ending "issuer must not contain +a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL", at either +assurance level. + +**What to change:** parse the issuer with `fapi.ParseIssuerURL` and +give it without a query. An issuer that needs to tell tenants apart +puts the tenant in the path: + +```go +issuer, err := fapi.ParseIssuerURL("https://as.example/tenants/a") // not ParseEndpointURL("https://as.example/?tenant=a") +if err != nil { + return err +} +cfg.Issuer = issuer +``` + ## v0.68.0 ### `DefaultAttesterLeafPolicy` refuses more signing roles (server) diff --git a/client/client.go b/client/client.go index 587aadb0..63c6de9b 100644 --- a/client/client.go +++ b/client/client.go @@ -215,6 +215,9 @@ func validateRequiredConfig(cfg Config) error { if cfg.Issuer.IsZero() { return fmt.Errorf("client: config: issuer is required") } + if u := cfg.Issuer.URL(); u.RawQuery != "" || u.ForceQuery { + return fmt.Errorf("client: config: issuer must not contain a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL") + } if cfg.ClientID == "" { return fmt.Errorf("client: config: client ID is required") } diff --git a/client/config.go b/client/config.go index 719764b8..11a22a2f 100644 --- a/client/config.go +++ b/client/config.go @@ -447,7 +447,9 @@ type Config struct { // Issuer is the authorization server this client talks to — the // audience client assertions and request objects are addressed to, // and the issuer authorization responses and ID tokens must come - // from. + // from. Parse it with fapi.ParseIssuerURL: New refuses one with a + // query, even an empty one, which RFC 8414 §2 and OpenID Connect + // Core §2 rule out. Issuer fapi.URL // ClientID is this client's identifier at the authorization server: diff --git a/client/issuer_query_test.go b/client/issuer_query_test.go new file mode 100644 index 00000000..1336c200 --- /dev/null +++ b/client/issuer_query_test.go @@ -0,0 +1,31 @@ +package client_test + +import ( + "strings" + "testing" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/client" +) + +// TestNewRefusesIssuerWithQuery: an issuer has no query component (RFC +// 8414 §2, OpenID Connect Core §2), so New refuses one even when it was +// built with a parser that allows a query, under either assurance level. +func TestNewRefusesIssuerWithQuery(t *testing.T) { + for _, assurance := range []client.AssuranceLevel{client.AssuranceDevelopment, client.AssuranceProduction} { + for _, suffix := range []string{"?tenant=a", "?"} { + t.Run(suffix, func(t *testing.T) { + cfg := validConfig(t) + cfg.Assurance = assurance + issuer, err := fapi.ParseEndpointURL(cfg.Issuer.String() + suffix) + if err != nil { + t.Fatalf("ParseEndpointURL: %v", err) + } + cfg.Issuer = issuer + if _, err := client.New(cfg, validDependencies(t)); err == nil || !strings.Contains(err.Error(), "query") { + t.Fatalf("New(issuer %q, assurance %v) = %v, want the query refused", issuer, assurance, err) + } + }) + } + } +} diff --git a/server/config.go b/server/config.go index dfd56cc5..947e193e 100644 --- a/server/config.go +++ b/server/config.go @@ -404,7 +404,9 @@ type Config struct { // Issuer is this server's issuer identifier — the audience client // assertions and request objects must be addressed to, and every // access token's audience unless the token is for resources (RFC - // 8707 resource indicators; see Resources). + // 8707 resource indicators; see Resources). Parse it with + // fapi.ParseIssuerURL: New refuses one with a query, even an empty + // one, which RFC 8414 §2 and OpenID Connect Core §2 rule out. Issuer fapi.URL Endpoints Endpoints diff --git a/server/issuer_query_test.go b/server/issuer_query_test.go new file mode 100644 index 00000000..8ecccc68 --- /dev/null +++ b/server/issuer_query_test.go @@ -0,0 +1,38 @@ +package server_test + +import ( + "strings" + "testing" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/server" +) + +// TestNewRefusesIssuerWithQuery: an issuer has no query component (RFC +// 8414 §2, OpenID Connect Core §2), so New refuses one even when it was +// built with a parser that allows a query, under either assurance level. +func TestNewRefusesIssuerWithQuery(t *testing.T) { + prodCfg, prodDeps := productionConfigAndDeps(t) + configs := map[string]struct { + cfg server.Config + deps server.Dependencies + }{ + "development": {validConfig(t), validDependencies()}, + "production": {prodCfg, prodDeps}, + } + for level, c := range configs { + for _, suffix := range []string{"?tenant=a", "?"} { + t.Run(level+" "+suffix, func(t *testing.T) { + cfg := c.cfg + issuer, err := fapi.ParseEndpointURL(cfg.Issuer.String() + suffix) + if err != nil { + t.Fatalf("ParseEndpointURL: %v", err) + } + cfg.Issuer = issuer + if _, err := server.New(cfg, c.deps); err == nil || !strings.Contains(err.Error(), "query") { + t.Fatalf("New(issuer %q) = %v, want the query refused", issuer, err) + } + }) + } + } +} diff --git a/server/server.go b/server/server.go index c1e27aee..7f729a49 100644 --- a/server/server.go +++ b/server/server.go @@ -173,6 +173,9 @@ func validateRequiredEndpoints(cfg Config) error { if cfg.Issuer.IsZero() { return fmt.Errorf("server: config: issuer is required") } + if u := cfg.Issuer.URL(); u.RawQuery != "" || u.ForceQuery { + return fmt.Errorf("server: config: issuer must not contain a query (RFC 8414 §2): parse it with fapi.ParseIssuerURL") + } if cfg.Endpoints.Token.IsZero() { return fmt.Errorf("server: config: endpoints.token is required") } From f3a006baefb11ff712f8e7242ec59caff645e63e Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:12:32 +0800 Subject: [PATCH 4/7] fix!: refuse an empty fragment in issuer, endpoint and redirect URIs RFC 3986 section 3.5 makes everything after the first "#" a fragment component, an empty one included, and RFC 8414 section 2 and RFC 6749 section 3.1.2 rule one out of an issuer and a redirect URI. The fapi.Parse*URL parsers refused a non-empty fragment but parsed a bare "#" away, silently rewriting the value, and storage.NewRegisteredClient checked a web client's redirect URIs for no fragment at all. The parsers now refuse any "#", and NewRegisteredClient refuses a redirect URI containing one. BREAKING CHANGE: fapi.ParseIssuerURL, ParseEndpointURL and ParseRedirectURL refuse a URL ending in a bare "#", and storage.NewRegisteredClient (and so sqlstore) refuses a registered redirect URI containing "#". Drop the "#" from configuration and from each client's registered redirect URIs. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 34 ++++++++++++++++++++++++++++++++++ storage/client_repository.go | 18 ++++++++++++++++-- storage/client_subject.go | 9 ++++++--- storage/redirect_uris_test.go | 30 ++++++++++++++++++++++++++++++ url.go | 23 ++++++++++++++++------- url_test.go | 19 +++++++++++++++++++ 6 files changed, 121 insertions(+), 12 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 8a3adc37..dd5b82ee 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -193,6 +193,40 @@ if err != nil { cfg.Issuer = issuer ``` +### An empty fragment (`#`) is refused in issuer, endpoint and redirect URIs (all packages, storage) + +**Affects:** + +- a URL ending in a bare `#` given to `fapi.ParseIssuerURL`, + `fapi.ParseEndpointURL` or `fapi.ParseRedirectURL`, including an + issuer a client discovers and a `redirect_uri` a client sends to a + server; +- a client registered with `storage.NewRegisteredClient` (and so + `sqlstore`) whose `RedirectURIs` has a URI containing `#`, even a + web client's. + +**Why:** RFC 3986 §3.5 makes everything after the first `#` a fragment +component, an empty one included. RFC 8414 §2 rules one out of an +issuer and RFC 6749 §3.1.2 out of a redirect URI. The parsers refused +a non-empty fragment but parsed a bare `#` away, silently rewriting the +value. `NewRegisteredClient` checked a web client's redirect URIs for +no fragment at all, and registered one that could only match a request +spelled the same way, which the server's own parser then refused. + +**What fails:** the parsers return an error ending "URL must not +contain a fragment". `NewRegisteredClient` returns an error saying the +redirect URI "has a fragment, which a redirect URI must not (RFC 6749 +§3.1.2)". A client already stored in `sqlstore` with such a URI fails +to resolve with `sqlstore.ErrClientConfig`, so requests naming it are +refused until it is stored again without the `#`. + +**What to change:** drop the `#`, from the configuration and from each +client's registered redirect URIs: + +```go +cfg.RedirectURIs = []fapi.RegisteredRedirectURI{"https://rp.example/cb"} // not "https://rp.example/cb#" +``` + ## v0.68.0 ### `DefaultAttesterLeafPolicy` refuses more signing roles (server) diff --git a/storage/client_repository.go b/storage/client_repository.go index 870123e6..dd072ed7 100644 --- a/storage/client_repository.go +++ b/storage/client_repository.go @@ -249,7 +249,8 @@ type RegisteredClientConfig struct { // kept byte for byte and the authorization response's parameters // added after it (RFC 6749 §3.1.2), so a query naming one of them — // code, state, iss, error, error_description, error_uri or - // response — is refused. + // response — is refused, as is a URI with a fragment, even an empty + // "#" (RFC 6749 §3.1.2). RedirectURIs []fapi.RegisteredRedirectURI // ApplicationType is ApplicationTypeWeb (the zero value) or @@ -944,6 +945,19 @@ func (c RegisteredClient) ApplicationType() ApplicationType { return c.applicati // iss, and JARM's response. var authorizationResponseParameters = []string{"code", "state", "iss", "error", "error_description", "error_uri", "response"} +// checkRedirectURIFragments refuses a redirect URI with a fragment +// component, even an empty one ("https://rp.example/cb#"): RFC 6749 +// §3.1.2 says "The endpoint URI MUST NOT include a fragment component", +// and RFC 3986 §3.5 makes everything after the first "#" one. +func checkRedirectURIFragments(cfg RegisteredClientConfig) error { + for _, uri := range cfg.RedirectURIs { + if strings.Contains(string(uri), "#") { + return fmt.Errorf("storage: client %q: redirect URI %q has a fragment, which a redirect URI must not (RFC 6749 §3.1.2)", cfg.ID, uri) + } + } + return nil +} + // checkRedirectURIQueries refuses a redirect URI whose query names an // authorization response parameter (authorizationResponseParameters). // RFC 6749 §3.1.2 has the server retain the registered query and add @@ -1033,7 +1047,7 @@ func withoutLoopbackPort(uri string) (string, bool) { return "", false } u, err := url.Parse(uri) - if err != nil || u.User != nil || u.Fragment != "" || !strings.HasPrefix(uri[len(prefix):], u.Host) { + if err != nil || u.User != nil || strings.Contains(uri, "#") || !strings.HasPrefix(uri[len(prefix):], u.Host) { return "", false } if ip := net.ParseIP(u.Hostname()); ip == nil || !ip.IsLoopback() { diff --git a/storage/client_subject.go b/storage/client_subject.go index 001f9ea4..d0808384 100644 --- a/storage/client_subject.go +++ b/storage/client_subject.go @@ -25,10 +25,13 @@ func (c RegisteredClient) SubjectType() fapi.SubjectType { return c.subjectType // can't have pairwise subjects). func (c RegisteredClient) Sector() string { return c.sector } -// checkApplicationTypeAndSector runs checkRedirectURIQueries and -// checkApplicationType, then clientSector, whose sector derivation -// depends on the application type. +// checkApplicationTypeAndSector runs checkRedirectURIFragments, +// checkRedirectURIQueries and checkApplicationType, then clientSector, +// whose sector derivation depends on the application type. func checkApplicationTypeAndSector(cfg RegisteredClientConfig) (string, error) { + if err := checkRedirectURIFragments(cfg); err != nil { + return "", err + } if err := checkRedirectURIQueries(cfg); err != nil { return "", err } diff --git a/storage/redirect_uris_test.go b/storage/redirect_uris_test.go index 4d0d5452..91c94294 100644 --- a/storage/redirect_uris_test.go +++ b/storage/redirect_uris_test.go @@ -97,3 +97,33 @@ func TestNewRegisteredClientRefusesRedirectQueryNamingResponseParameter(t *testi }) } } + +// RFC 6749 §3.1.2: a redirect URI "MUST NOT include a fragment +// component", and "#" alone is one (RFC 3986 §3.5). NewRegisteredClient +// refuses it for a web or a native client, rather than registering a +// URI that matches only a request spelled with the same bare "#". +func TestNewRegisteredClientRefusesRedirectFragment(t *testing.T) { + for _, tc := range []struct { + uri string + native bool + }{ + {"https://rp.example/cb#", false}, + {"https://rp.example/cb#frag", false}, + {"https://rp.example/cb?a=1#", false}, + {"http://127.0.0.1/cb#", true}, + {"com.example.app:/cb#", true}, + } { + t.Run(tc.uri, func(t *testing.T) { + cfg := RegisteredClientConfig{ + ID: "c", RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(tc.uri)}, + ClientAssertionAlgorithm: fapi.ES256, AllowedScopes: []string{"openid"}, + } + if tc.native { + cfg.ApplicationType = ApplicationTypeNative + } + if c, err := NewRegisteredClient(cfg); err == nil { + t.Fatalf("NewRegisteredClient(%q) accepted it; HasRedirectURI = %v", tc.uri, c.HasRedirectURI(tc.uri)) + } + }) + } +} diff --git a/url.go b/url.go index 89641d34..ebca22d1 100644 --- a/url.go +++ b/url.go @@ -16,7 +16,7 @@ import ( // ParseIssuerURL, ParseEndpointURL or ParseRedirectURL, which enforce: // absolute, HTTPS (except an explicitly enabled loopback exception, or a // native app's private-use scheme for a redirect), no embedded -// credentials, no fragment, a host in ASCII (an internationalized +// credentials, no fragment (not even an empty "#"), a host in ASCII (an internationalized // domain name given as its xn-- A-label), and a normalized scheme and // host (their ASCII letters lowercased). type URL struct { @@ -65,7 +65,7 @@ func ParseRedirectURL(raw string, opts ...URLOption) (URL, error) { } if o.allowPrivateUseScheme { if parsed, err := url.Parse(raw); err == nil && parsed.Scheme != "http" && parsed.Scheme != "https" && parsed.Scheme != "" { - u, err := parsePrivateUseURL(parsed) + u, err := parsePrivateUseURL(raw, parsed) if err != nil { return URL{}, fmt.Errorf("fapi: parse redirect URL: %w", err) } @@ -79,9 +79,9 @@ func ParseRedirectURL(raw string, opts ...URLOption) (URL, error) { return u, nil } -// parsePrivateUseURL validates parsed as a private-use URI scheme -// redirect (RFC 8252 §7.1, §8.4). -func parsePrivateUseURL(parsed *url.URL) (URL, error) { +// parsePrivateUseURL validates parsed, raw's parse, as a private-use +// URI scheme redirect (RFC 8252 §7.1, §8.4). +func parsePrivateUseURL(raw string, parsed *url.URL) (URL, error) { if !isReverseDomainScheme(parsed.Scheme) { return URL{}, fmt.Errorf("private-use scheme %q must be a reverse-order domain name, such as com.example.app", parsed.Scheme) } @@ -91,12 +91,21 @@ func parsePrivateUseURL(parsed *url.URL) (URL, error) { if parsed.Opaque != "" || parsed.Host != "" || parsed.User != nil || !strings.HasPrefix(parsed.Path, "/") || strings.HasPrefix(parsed.Path, "//") || strings.HasPrefix(parsed.String(), parsed.Scheme+"://") { return URL{}, fmt.Errorf("private-use redirect URI must be %s:/path, with a single slash after the scheme (RFC 8252 §7.1), not %s://…", parsed.Scheme, parsed.Scheme) } - if parsed.Fragment != "" { + if hasFragment(raw) { return URL{}, fmt.Errorf("URL must not contain a fragment") } return URL{value: *parsed}, nil } +// hasFragment reports whether raw has a fragment component, even an +// empty one: RFC 3986 §3.5's fragment is everything after the first +// "#", so "https://rp.example/cb#" has one. net/url parses that "#" +// away, leaving url.URL.Fragment empty, and a URL that kept it would +// drop it from its string form, silently rewriting the value. +func hasFragment(raw string) bool { + return strings.Contains(raw, "#") +} + // isReverseDomainScheme reports whether scheme has the form RFC 8252 // §7.1 requires of a private-use URI scheme: a domain name in reverse // order, at least two dot-separated labels, each of letters, digits and @@ -184,7 +193,7 @@ func parseSecureURL(raw string, opts []URLOption) (URL, error) { if parsed.User != nil { return URL{}, fmt.Errorf("URL must not contain embedded credentials") } - if parsed.Fragment != "" { + if hasFragment(raw) { return URL{}, fmt.Errorf("URL must not contain a fragment") } diff --git a/url_test.go b/url_test.go index 117da517..8ea9fc2f 100644 --- a/url_test.go +++ b/url_test.go @@ -258,6 +258,25 @@ func TestParseURLRefusesNonASCIIHost(t *testing.T) { } } +// TestParseURLRefusesEmptyFragment: "#" alone is a fragment component +// (RFC 3986 §3.5), which an issuer (RFC 8414 §2) and a redirect URI (RFC +// 6749 §3.1.2) must not have, so every parser refuses it rather than +// parsing it away; a private-use redirect too. +func TestParseURLRefusesEmptyFragment(t *testing.T) { + for name, parse := range parsers { + for _, raw := range []string{ + "https://as.example.com/#", + "https://as.example.com#", + "https://client.example.com/cb?a=1#", + "com.example.app:/cb#", + } { + if u, err := parse(raw); err == nil { + t.Errorf("%s(%q) = %q, want an error", name, raw, u.String()) + } + } + } +} + // TestParseURLLowercasesASCIIHost: a mixed-case ASCII host or scheme is // lowercased, and an internationalized name in its A-label form is // accepted. From 4555c67116bb03e6ee519b33234b5aa2ced0cc9a Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:17:24 +0800 Subject: [PATCH 5/7] fix(fapihttp): accept Structured Field parameters on a forwarded Client-Cert RFC 9440 section 2.2 makes Client-Cert a Byte Sequence, and RFC 9651 section 2.3 lets every Item carry Parameters as an extensibility mechanism, discouraging a field from treating an unrecognized one as an error. ForwardedClientCertificate refused any value with parameters, so a TLS-terminating proxy adding one had every mTLS request refused. It now parses the value as an RFC 9651 Item: a Byte Sequence followed by any valid parameters, which are ignored. Anything else is still ErrMalformedForwardedCertificate. The RFC 8941 citations now name RFC 9651, which obsoletes it. Co-Authored-By: Claude Opus 5.5 --- fapihttp/forwarded_certificate.go | 42 +++-- fapihttp/forwarded_certificate_fuzz_test.go | 58 +++++- fapihttp/forwarded_certificate_test.go | 14 +- fapihttp/structured_field.go | 196 ++++++++++++++++++++ 4 files changed, 285 insertions(+), 25 deletions(-) create mode 100644 fapihttp/structured_field.go diff --git a/fapihttp/forwarded_certificate.go b/fapihttp/forwarded_certificate.go index 65360fc0..456b9e56 100644 --- a/fapihttp/forwarded_certificate.go +++ b/fapihttp/forwarded_certificate.go @@ -58,13 +58,15 @@ var ( // certificate, authenticating as that client and presenting its // certificate-bound tokens. // -// The value is RFC 8941 §3.3.5's Byte Sequence: the DER certificate in -// base64 between colons. A header occurring more than once (RFC 9440 -// §2.2 makes it a singleton), a value longer than -// MaxForwardedCertificateBytes, anything but one Byte Sequence -// (parameters included: RFC 9440 defines none), or bytes that aren't -// exactly one X.509 certificate is ErrMalformedForwardedCertificate. -// Missing "=" padding is accepted, as RFC 8941 §4.2.7 asks. A forwarded +// The value is RFC 9651 §3.3.5's Byte Sequence: the DER certificate in +// base64 between colons. RFC 9440 defines no parameters, but RFC 9651 +// §2.3 lets every Item carry some, so any that follow the Byte Sequence +// (":…:;a=1") are accepted and ignored. A header occurring more than +// once (RFC 9440 §2.2 makes it a singleton), a value longer than +// MaxForwardedCertificateBytes, anything but one Byte Sequence and its +// parameters, or bytes that aren't exactly one X.509 certificate is +// ErrMalformedForwardedCertificate. Missing "=" padding is accepted, as +// RFC 9651 §4.2.7 asks. A forwarded // PEM or URL-encoded certificate, which some proxies send in their own // headers, isn't RFC 9440's encoding and is refused: configure the proxy // to send Client-Cert instead. @@ -97,21 +99,29 @@ func ForwardedClientCertificate(r *http.Request, header string, trusted func(*ht return cert, nil } -// parseByteSequence decodes a header field value that is one RFC 8941 -// Byte Sequence (§3.3.5, parsed per §4.2 and §4.2.7): optional spaces, -// ":", base64, ":", optional spaces, and nothing else. +// parseByteSequence decodes a header field value that is one RFC 9651 +// Item whose bare item is a Byte Sequence (§3.3.5, parsed per §4.2, +// §4.2.3 and §4.2.7): optional spaces, ":", base64, ":", any +// parameters (ignored), optional spaces, and nothing else. func parseByteSequence(value string) ([]byte, error) { if len(value) > MaxForwardedCertificateBytes { return nil, fmt.Errorf("the value is longer than %d bytes", MaxForwardedCertificateBytes) } - // RFC 8941 §4.2: leading and trailing SP are discarded (net/http has + // RFC 9651 §4.2: leading and trailing SP are discarded (net/http has // already removed any other optional whitespace). value = strings.Trim(value, " ") - if len(value) < 2 || value[0] != ':' || value[len(value)-1] != ':' { + end := -1 + if value != "" && value[0] == ':' { + end = strings.IndexByte(value[1:], ':') + } + if end < 0 { return nil, errors.New("the value isn't a byte sequence (:base64:)") } - body := value[1 : len(value)-1] - // Up to two "=" of padding, only at the end; RFC 8941 §4.2.7 asks a + body := value[1 : 1+end] + if err := skipParameters(value[2+end:]); err != nil { + return nil, err + } + // Up to two "=" of padding, only at the end; RFC 9651 §4.2.7 asks a // parser not to fail when it is missing. unpadded := strings.TrimRight(body, "=") if unpadded == "" { @@ -121,9 +131,9 @@ func parseByteSequence(value string) ([]byte, error) { if len(body)-len(unpadded) > 2 || strings.ContainsRune(unpadded, '=') { return nil, errors.New("the byte sequence's padding is malformed") } - // encoding/base64 skips line breaks, which RFC 8941's base64 doesn't + // encoding/base64 skips line breaks, which RFC 9651's base64 doesn't // allow, so the alphabet is checked first. Non-zero pad bits are - // accepted, as RFC 8941 §4.2.7 asks. + // accepted, as RFC 9651 §4.2.7 asks. if strings.IndexFunc(unpadded, notBase64) >= 0 { return nil, errors.New("the byte sequence isn't base64") } diff --git a/fapihttp/forwarded_certificate_fuzz_test.go b/fapihttp/forwarded_certificate_fuzz_test.go index 0e7349f7..14b6f98b 100644 --- a/fapihttp/forwarded_certificate_fuzz_test.go +++ b/fapihttp/forwarded_certificate_fuzz_test.go @@ -11,19 +11,23 @@ import ( "errors" "math/big" "net/http" + "net/url" + "regexp" "strings" "testing" "time" + "unicode/utf8" ) // FuzzParseByteSequence checks parseByteSequence, which reads the RFC // 9440 Client-Cert header a proxy forwards, against an independent -// reading of RFC 8941 §3.3.5/§4.2.7: beyond never panicking, +// reading of RFC 9651 §3.3.5/§4.2.3/§4.2.7: beyond never panicking, // // - it accepts exactly a value that, once leading and trailing spaces // are dropped, is ":", base64 characters, at most two "=" at the -// end, ":", within MaxForwardedCertificateBytes, whose base64 decodes -// to at least one byte; +// end, ":", then zero or more RFC 9651 Parameters, within +// MaxForwardedCertificateBytes, whose base64 decodes to at least one +// byte; // - what it accepts re-encodes, canonically padded, to a Byte Sequence // it reads back to the same bytes, whose base64 is the input's own // up to pad bits. @@ -31,6 +35,8 @@ func FuzzParseByteSequence(f *testing.F) { for _, seed := range []string{ ":AQID:", " :AQID: ", ":AQI=:", ":AQ==:", ":AQ:", ":AR==:", ":=:", ":==:", "::", ":", "", ":AQ===:", ":A=Q=:", ":AQ\nID:", ":AQ-_:", "AQID", ":AQID:;p=1", "\t:AQID:", ":A:", + ":AQID:;a", ":AQID:; a=?1;b=-12.5", `:AQID:;a="q\"";b=%"%c3%a9"`, ":AQID:;a=@-1;b=t:/x", + ":AQID: ;a", ":AQID:;A", ":AQID:;a=1.", ":AQID:;a=%\"%c3\"", ":AQID:;a=:A=:", ":AQID:;", } { f.Add(seed) } @@ -47,17 +53,52 @@ func FuzzParseByteSequence(f *testing.F) { }) } -// byteSequenceOracle is RFC 8941's Byte Sequence grammar as +// sfParameter is one RFC 9651 §4.2.3.2 parameter as a regular +// expression: ";", optional spaces, a key (§4.2.3.3), and optionally "=" +// and a bare item: a Decimal or Integer (§4.2.4), a String (§4.2.5), a +// Token (§4.2.6), a Byte Sequence (§4.2.7), a Boolean (§4.2.8), a Date +// (§4.2.9) or a Display String (§4.2.10), whose escaped content is the +// one submatch, for parametersOracle to check it decodes to UTF-8. +var sfParameter = regexp.MustCompile(`^; *[a-z*][a-z0-9_\-.*]*(?:=(?:` + + `-?[0-9]{1,12}\.[0-9]{1,3}|-?[0-9]{1,15}|` + + `"(?:[\x20\x21\x23-\x5b\x5d-\x7e]|\\["\\])*"|` + + "[A-Za-z*][!#$%&'*+\\-.^_`|~0-9A-Za-z:/]*|" + + `:[A-Za-z0-9+/=]*:|\?[01]|@-?[0-9]{1,15}|` + + `%"((?:[\x20\x21\x23\x24\x26-\x7e]|%[0-9a-f]{2})*)"))?`) + +// parametersOracle reports whether s is RFC 9651 Parameters: zero or +// more of sfParameter, and nothing else. +func parametersOracle(s string) bool { + for s != "" { + m := sfParameter.FindStringSubmatchIndex(s) + if m == nil { + return false + } + if m[2] >= 0 { + decoded, err := url.PathUnescape(s[m[2]:m[3]]) + if err != nil || !utf8.ValidString(decoded) { + return false + } + } + s = s[m[1]:] + } + return true +} + +// byteSequenceOracle is RFC 9651's Byte Sequence Item grammar as // parseByteSequence documents it, written without its code. func byteSequenceOracle(value string) bool { if len(value) > MaxForwardedCertificateBytes { return false } v := strings.Trim(value, " ") - if !strings.HasPrefix(v, ":") || !strings.HasSuffix(v, ":") || len(v) < 2 { + if !strings.HasPrefix(v, ":") { + return false + } + body, params, closed := strings.Cut(v[1:], ":") + if !closed || !parametersOracle(params) { return false } - body := v[1 : len(v)-1] pad := len(body) - len(strings.TrimRight(body, "=")) chars := body[:len(body)-pad] if pad > 2 { @@ -85,9 +126,10 @@ func checkByteSequenceRoundTrip(t *testing.T, value string, der []byte) { if err != nil || !bytes.Equal(again, der) { t.Fatalf("parseByteSequence(%q) = %x, but its canonical form %q reads back as %x, %v", value, der, canonical, again, err) } - body := strings.TrimRight(strings.Trim(strings.Trim(value, " "), ":"), "=") + body, _, _ := strings.Cut(strings.TrimPrefix(strings.Trim(value, " "), ":"), ":") + body = strings.TrimRight(body, "=") reencoded := base64.RawStdEncoding.EncodeToString(der) - // Only the last character may differ, in pad bits RFC 8941 §4.2.7 + // Only the last character may differ, in pad bits RFC 9651 §4.2.7 // asks a parser to ignore. if len(body) != len(reencoded) || body[:len(body)-1] != reencoded[:len(reencoded)-1] { t.Fatalf("parseByteSequence(%q) = %x, which re-encodes as %q", value, der, reencoded) diff --git a/fapihttp/forwarded_certificate_test.go b/fapihttp/forwarded_certificate_test.go index 7830669d..808d37b7 100644 --- a/fapihttp/forwarded_certificate_test.go +++ b/fapihttp/forwarded_certificate_test.go @@ -106,7 +106,19 @@ func TestForwardedClientCertificateEncodings(t *testing.T) { "padding missing": {[]string{":" + raw + ":"}, true}, "twice": {[]string{":" + b64 + ":", ":" + b64 + ":"}, false}, "a list": {[]string{":" + b64 + ":, :" + b64 + ":"}, false}, - "parameters": {[]string{":" + b64 + ":;a=1"}, false}, + "a parameter": {[]string{":" + b64 + ":;a=1"}, true}, + "a boolean parameter": {[]string{":" + b64 + ":;grease"}, true}, + "parameters of every type": {[]string{":" + b64 + `:;a=-1.5;b="x \" y";c=tok/en:x;d=:AQID:;e=?0;f=@1700000000;g=%"caf%c3%a9";*h;i`}, true}, + "spaces after a semicolon": {[]string{":" + b64 + ":; a=1 "}, true}, + "a space before a parameter": {[]string{":" + b64 + ": ;a=1"}, false}, + "an uppercase key": {[]string{":" + b64 + ":;A=1"}, false}, + "a parameter with no value": {[]string{":" + b64 + ":;a="}, false}, + "a semicolon alone": {[]string{":" + b64 + ":;"}, false}, + "a four-digit fraction": {[]string{":" + b64 + ":;a=1.2345"}, false}, + "an unterminated string": {[]string{":" + b64 + `:;a="x`}, false}, + "a decimal date": {[]string{":" + b64 + ":;a=@1.5"}, false}, + "an invalid display string": {[]string{":" + b64 + `:;a=%"%ff"`}, false}, + "an inner list parameter": {[]string{":" + b64 + ":;a=(1 2)"}, false}, "no colons": {[]string{b64}, false}, "one colon": {[]string{":" + b64}, false}, "empty": {[]string{"::"}, false}, diff --git a/fapihttp/structured_field.go b/fapihttp/structured_field.go new file mode 100644 index 00000000..bb6e4756 --- /dev/null +++ b/fapihttp/structured_field.go @@ -0,0 +1,196 @@ +package fapihttp + +import ( + "errors" + "strings" + "unicode/utf8" +) + +// This file reads just enough of RFC 9651 (Structured Field Values for +// HTTP, which obsoletes RFC 8941) to skip an Item's Parameters: RFC 9440 +// §2.2 makes Client-Cert a Byte Sequence, and RFC 9651 §2.3 has every +// Item allow Parameters "as an extensibility mechanism", discouraging a +// field from treating an unrecognized one as an error. Each function +// checks one RFC 9651 §4.2 production at the start of s and returns +// what follows it, so a value that isn't a valid Item is still refused. + +var errMalformedParameters = errors.New("the byte sequence's parameters are malformed") + +// skipParameters checks that s is zero or more Parameters (RFC 9651 +// §4.2.3.2: ";", optional spaces, a key, optionally "=" and a bare item) +// and nothing else. Their keys and values are ignored. +func skipParameters(s string) error { + for s != "" { + if s[0] != ';' { + return errors.New("the value isn't a byte sequence (:base64:)") + } + rest, ok := skipKey(strings.TrimLeft(s[1:], " ")) + if !ok { + return errMalformedParameters + } + if rest != "" && rest[0] == '=' { + if rest, ok = skipBareItem(rest[1:]); !ok { + return errMalformedParameters + } + } + s = rest + } + return nil +} + +// skipKey skips a key (RFC 9651 §4.2.3.3): a lowercase letter or "*", +// then lowercase letters, digits, "_", "-", "." and "*". +func skipKey(s string) (string, bool) { + if s == "" || !isLCAlpha(s[0]) && s[0] != '*' { + return s, false + } + i := 1 + for i < len(s) && (isLCAlpha(s[i]) || isDigit(s[i]) || strings.IndexByte("_-.*", s[i]) >= 0) { + i++ + } + return s[i:], true +} + +// skipBareItem skips one bare item (RFC 9651 §4.2.3.1), whichever of +// its types s starts with. +func skipBareItem(s string) (string, bool) { + if s == "" { + return s, false + } + switch c := s[0]; { + case c == '-' || isDigit(c): + rest, _, ok := skipNumber(s) + return rest, ok + case c == '"': + return skipString(s[1:]) + case c == '*' || isAlpha(c): + return skipToken(s[1:]), true + case c == ':': + return skipByteSequence(s[1:]) + case c == '?': + return skipBoolean(s[1:]) + case c == '@': + rest, decimal, ok := skipNumber(s[1:]) + return rest, ok && !decimal + case c == '%': + return skipDisplayString(s[1:]) + } + return s, false +} + +// skipNumber skips an Integer or Decimal (RFC 9651 §4.2.4): an optional +// "-", then at most 15 digits, or at most 12 digits, "." and one to +// three digits. decimal reports which. +func skipNumber(s string) (rest string, decimal bool, ok bool) { + i := 0 + if i < len(s) && s[i] == '-' { + i++ + } + start := i + for i < len(s) && isDigit(s[i]) { + i++ + } + whole := i - start + if whole == 0 { + return s, false, false + } + if i == len(s) || s[i] != '.' { + return s[i:], false, whole <= 15 + } + i++ + fracStart := i + for i < len(s) && isDigit(s[i]) { + i++ + } + fraction := i - fracStart + return s[i:], true, whole <= 12 && fraction >= 1 && fraction <= 3 +} + +// skipString skips the rest of a String (RFC 9651 §4.2.5) after its +// opening quote: visible ASCII and spaces, "\"" and "\\" escapes, then +// the closing quote. +func skipString(s string) (string, bool) { + for i := 0; i < len(s); i++ { + switch c := s[i]; { + case c == '"': + return s[i+1:], true + case c == '\\': + i++ + if i == len(s) || s[i] != '"' && s[i] != '\\' { + return s, false + } + case c < 0x20 || c > 0x7e: + return s, false + } + } + return s, false +} + +// skipToken skips the rest of a Token (RFC 9651 §4.2.6) after its first +// character: tchar (RFC 9110 §5.6.2), ":" and "/". +func skipToken(s string) string { + i := 0 + for i < len(s) && (isAlpha(s[i]) || isDigit(s[i]) || strings.IndexByte("!#$%&'*+-.^_`|~:/", s[i]) >= 0) { + i++ + } + return s[i:] +} + +// skipByteSequence skips the rest of a Byte Sequence (RFC 9651 §4.2.7) +// after its opening colon: base64 characters, then the closing colon. +func skipByteSequence(s string) (string, bool) { + end := strings.IndexByte(s, ':') + if end < 0 || strings.IndexFunc(s[:end], func(c rune) bool { return notBase64(c) && c != '=' }) >= 0 { + return s, false + } + return s[end+1:], true +} + +// skipBoolean skips the rest of a Boolean (RFC 9651 §4.2.8) after its +// "?": "0" or "1". +func skipBoolean(s string) (string, bool) { + if s == "" || s[0] != '0' && s[0] != '1' { + return s, false + } + return s[1:], true +} + +// skipDisplayString skips the rest of a Display String (RFC 9651 +// §4.2.10) after its "%": a quote, visible ASCII and spaces with "%" +// escaping two lowercase hex digits, a closing quote, and the decoded +// bytes valid UTF-8. +func skipDisplayString(s string) (string, bool) { + if s == "" || s[0] != '"' { + return s, false + } + var decoded []byte + for i := 1; i < len(s); i++ { + switch c := s[i]; { + case c == '"': + return s[i+1:], utf8.Valid(decoded) + case c == '%': + if i+2 >= len(s) || !isLCHex(s[i+1]) || !isLCHex(s[i+2]) { + return s, false + } + decoded = append(decoded, lcHexValue(s[i+1])<<4|lcHexValue(s[i+2])) + i += 2 + case c < 0x20 || c > 0x7e: + return s, false + default: + decoded = append(decoded, c) + } + } + return s, false +} + +func isDigit(c byte) bool { return '0' <= c && c <= '9' } +func isLCAlpha(c byte) bool { return 'a' <= c && c <= 'z' } +func isAlpha(c byte) bool { return isLCAlpha(c) || 'A' <= c && c <= 'Z' } +func isLCHex(c byte) bool { return isDigit(c) || 'a' <= c && c <= 'f' } + +func lcHexValue(c byte) byte { + if isDigit(c) { + return c - '0' + } + return c - 'a' + 10 +} From da49bdf661085507ce2f8af3a9a5b09ef5d3d41f Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 14:19:06 +0800 Subject: [PATCH 6/7] fix(keys): match a kid-less client key by its RFC 7638 thumbprint A VerificationKey registered without a KeyID, given to NewStaticClientKeys or found in a client's JWK Set, matched only a request naming no kid. NewKeyManagerFromSigners derives a kid (the key's RFC 7638 thumbprint) for a key given without one, so a client assertion signed by such a key manager never matched the same key registered without a kid, and failed with invalid_client. A kid-less key now also matches a request naming its thumbprint, and no other kid. VerificationKey.KeyID now documents the matching rule. Co-Authored-By: Claude Opus 5.5 --- keys/client_static.go | 35 +++++++++++++++++++++----- keys/client_static_test.go | 50 ++++++++++++++++++++++++++++++++++++++ keys/verification.go | 6 +++++ 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/keys/client_static.go b/keys/client_static.go index 2f99e06b..e97bd098 100644 --- a/keys/client_static.go +++ b/keys/client_static.go @@ -36,12 +36,30 @@ type ClientKeyMaterial struct { } // clientKeySet is one client's validated keys: those that verify its -// signatures and those to encrypt to it. +// signatures and those to encrypt to it. thumbprints[i] is the RFC 7638 +// JWK thumbprint of verification[i] when that key has no KeyID, the kid +// it also answers to (see VerificationKey.KeyID), and "" otherwise. type clientKeySet struct { verification []VerificationKey + thumbprints []string encryption []ClientEncryptionKey } +// addVerificationKey appends k, an already validated key, to set, +// with its thumbprint if it has no KeyID. +func (set *clientKeySet) addVerificationKey(k VerificationKey) error { + thumbprint := "" + if k.KeyID == "" { + var err error + if thumbprint, err = keyIDFor("", k.PublicKey, k.Algorithm); err != nil { + return err + } + } + set.verification = append(set.verification, k) + set.thumbprints = append(set.thumbprints, thumbprint) + return nil +} + // StaticClientKeys is a ClientKeySource and ClientEncryptionKeySource // over keys registered up front, client by client — for a deployment // that keeps its clients' public keys (their registered "jwks", or keys @@ -99,7 +117,9 @@ func validateClientKeyMaterial(material ClientKeyMaterial) (clientKeySet, error) if err := jose.ValidateCallerVerificationKey(k.PublicKey, k.Algorithm); err != nil { return clientKeySet{}, fmt.Errorf("verification key %d (kid %q): %w", i, k.KeyID, err) } - set.verification = append(set.verification, k) + if err := set.addVerificationKey(k); err != nil { + return clientKeySet{}, fmt.Errorf("verification key %d: %w", i, err) + } } for i, k := range material.EncryptionKeys { if err := jose.ValidateCallerEncryptionKey(k.PublicKey, k.Algorithm); err != nil { @@ -143,11 +163,12 @@ func (s *StaticClientKeys) Capabilities() KeySourceCapabilities { } // matchVerificationKeys is set's verification keys for alg (and kid, if -// set), in a new slice. +// set), in a new slice. A key registered without a KeyID matches a kid +// that is its RFC 7638 thumbprint, and no other. func matchVerificationKeys(set clientKeySet, alg fapi.SignatureAlgorithm, kid string) []VerificationKey { var matched []VerificationKey - for _, k := range set.verification { - if k.Algorithm == alg && (kid == "" || k.KeyID == kid) { + for i, k := range set.verification { + if k.Algorithm == alg && (kid == "" || k.KeyID == kid || k.KeyID == "" && set.thumbprints[i] == kid) { matched = append(matched, k) } } @@ -185,7 +206,9 @@ func parseClientJWKS(body []byte) (clientKeySet, error) { } var out clientKeySet for _, k := range verification { - out.verification = append(out.verification, VerificationKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}) + if err := out.addVerificationKey(VerificationKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}); err != nil { + return clientKeySet{}, err + } } for _, k := range encryption { out.encryption = append(out.encryption, ClientEncryptionKey{KeyID: k.KeyID, Algorithm: k.Algorithm, PublicKey: k.PublicKey}) diff --git a/keys/client_static_test.go b/keys/client_static_test.go index b7ed8446..9b350ed6 100644 --- a/keys/client_static_test.go +++ b/keys/client_static_test.go @@ -8,6 +8,7 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/rsa" + "encoding/base64" "math/big" "strings" "testing" @@ -77,6 +78,55 @@ func TestStaticClientKeysServesKeysGivenDirectly(t *testing.T) { } } +// TestStaticClientKeysMatchesThumbprintOfKeyWithoutKeyID: a key +// registered without a KeyID matches a request naming its RFC 7638 +// thumbprint, the kid NewKeyManagerFromSigners derives for the same key +// given without one, so the two sides interoperate. It matches no other +// kid, and a key with a KeyID still matches only that kid. +func TestStaticClientKeysMatchesThumbprintOfKeyWithoutKeyID(t *testing.T) { + rp, other := newP256(t), newP256(t) + km, err := keys.NewKeyManagerFromSigners([]keys.SignerSpec{{Purpose: keys.ClientAuthentication, Algorithm: fapi.ES256, Signer: rp}}) + if err != nil { + t.Fatalf("NewKeyManagerFromSigners: %v", err) + } + signing, err := km.PublicKey(context.Background(), keys.ClientAuthentication, fapi.ES256) + if err != nil || signing.KeyID == "" { + t.Fatalf("PublicKey: %+v, %v; want a derived kid", signing, err) + } + point, err := rp.PublicKey.Bytes() // 0x04 || X || Y + if err != nil { + t.Fatalf("Bytes: %v", err) + } + src, err := keys.NewStaticClientKeys(map[fapi.ClientID]keys.ClientKeyMaterial{ + "rp": {VerificationKeys: []keys.VerificationKey{{Algorithm: fapi.ES256, PublicKey: &rp.PublicKey}}}, + "named": {VerificationKeys: []keys.VerificationKey{{KeyID: "named", Algorithm: fapi.ES256, PublicKey: &rp.PublicKey}}}, + "other": {VerificationKeys: []keys.VerificationKey{{Algorithm: fapi.ES256, PublicKey: &other.PublicKey}}}, + "jwks": {JWKS: []byte(`{"keys":[{"kty":"EC","crv":"P-256","alg":"ES256","use":"sig","x":"` + base64.RawURLEncoding.EncodeToString(point[1:33]) + `","y":"` + base64.RawURLEncoding.EncodeToString(point[33:]) + `"}]}`)}, + }) + if err != nil { + t.Fatalf("NewStaticClientKeys: %v", err) + } + for _, tc := range []struct { + client fapi.ClientID + kid string + want int + }{ + {"rp", signing.KeyID, 1}, + {"rp", "", 1}, + {"rp", "some-other-kid", 0}, + {"named", signing.KeyID, 0}, + {"named", "named", 1}, + {"other", signing.KeyID, 0}, + {"jwks", signing.KeyID, 1}, + {"jwks", "some-other-kid", 0}, + } { + set, err := src.ResolveVerificationKeys(context.Background(), keys.ClientKeyRequest{ClientID: tc.client, Purpose: keys.ClientAssertionVerification, Algorithm: fapi.ES256, KeyID: tc.kid}) + if err != nil || len(set.Keys) != tc.want { + t.Errorf("client %q, kid %q: got %d keys, err %v; want %d", tc.client, tc.kid, len(set.Keys), err, tc.want) + } + } +} + func TestStaticClientKeysResolvesEncryptionKeys(t *testing.T) { priv, err := ecdh.P256().GenerateKey(rand.Reader) if err != nil { diff --git a/keys/verification.go b/keys/verification.go index b60d9199..27d2eddf 100644 --- a/keys/verification.go +++ b/keys/verification.go @@ -48,6 +48,12 @@ type ClientKeyRequest struct { // type, so an external implementation of ClientKeySource never needs to // depend on this module's internal JWK representation. type VerificationKey struct { + // KeyID is the key's "kid". A request naming no kid matches any + // key of its algorithm; one naming a kid matches a key whose KeyID + // is that kid. A key given without one (to NewStaticClientKeys, or + // in a client's JWK Set) matches a request naming its RFC 7638 JWK + // thumbprint, the kid NewKeyManagerFromSigners and SignerSpec derive + // for a key given without one, and no other kid. KeyID string Algorithm fapi.SignatureAlgorithm PublicKey crypto.PublicKey From bce8360e1143896a08d3e8c16e4be82d11e107c2 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 2 Nov 2026 15:08:16 +0800 Subject: [PATCH 7/7] test(fapihttp): cover each refused Client-Cert parameter value One case per way a parameter's bare item can be malformed: a sign with no digits, a bad or control character in a String, a non-base64 or unterminated Byte Sequence, a Boolean other than 0 or 1, and a Display String without its quote, with an uppercase or unterminated escape, or with a control character. Co-Authored-By: Claude Opus 5.5 --- fapihttp/forwarded_certificate_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fapihttp/forwarded_certificate_test.go b/fapihttp/forwarded_certificate_test.go index 808d37b7..8ead28ff 100644 --- a/fapihttp/forwarded_certificate_test.go +++ b/fapihttp/forwarded_certificate_test.go @@ -119,6 +119,16 @@ func TestForwardedClientCertificateEncodings(t *testing.T) { "a decimal date": {[]string{":" + b64 + ":;a=@1.5"}, false}, "an invalid display string": {[]string{":" + b64 + `:;a=%"%ff"`}, false}, "an inner list parameter": {[]string{":" + b64 + ":;a=(1 2)"}, false}, + "a sign with no digits": {[]string{":" + b64 + ":;a=-"}, false}, + "a bad string escape": {[]string{":" + b64 + `:;a="\x"`}, false}, + "a tab in a string": {[]string{":" + b64 + ":;a=\"a\tb\""}, false}, + "a non-base64 parameter": {[]string{":" + b64 + ":;a=:A$:"}, false}, + "an open byte sequence": {[]string{":" + b64 + ":;a=:AQID"}, false}, + "a boolean of 2": {[]string{":" + b64 + ":;a=?2"}, false}, + "a display string, no quote": {[]string{":" + b64 + ":;a=%x"}, false}, + "an uppercase escape": {[]string{":" + b64 + `:;a=%"%C3%A9"`}, false}, + "a tab in a display string": {[]string{":" + b64 + ":;a=%\"a\tb\""}, false}, + "an open display string": {[]string{":" + b64 + `:;a=%"abc`}, false}, "no colons": {[]string{b64}, false}, "one colon": {[]string{":" + b64}, false}, "empty": {[]string{"::"}, false},