fix(users): key OIDC JWKS cache by (issuer, jwks_uri) (bookshelf-mfli8) #1413

Merged

zombor merged 3 commits from bd-bookshelf-mfli8 into main 2026-08-10 01:40:29 +00:00

Conversation 2 Commits 3 Files changed 2 +97 -12

zombor commented 2026-08-10 00:37:41 +00:00

Owner

Copy link

Summary

Non-blocking code-review MINOR follow-up from #1405: internal/users/oidc_cache.go's oidcJWKSCache was keyed by jwksURI alone, so two distinct trusted issuers sharing a jwks_uri could collide and share a cache entry (a hit for issuer B could silently serve JWKS bytes fetched/trusted under issuer A). Harmless today since a cache hit never re-dials, but a latent footgun.

Keys the cache entries map (and the singleflight dedup key) by (issuer, jwksURI) via a new jwksCacheKey struct. Public Fetch/Refresh signatures are unchanged.

Test plan

• Added a black-box test asserting two issuers sharing a jwks_uri each trigger their own fetch (2 calls, not 1).
• go test ./internal/users/... passes, 100% coverage on the modified oidc_cache.go.
• golangci-lint run ./internal/users/... clean.

Docs: N/A because this is an internal cache-correctness fix with no user-observable behavior change.

Closes bead bookshelf-mfli8 on merge.

## Summary Non-blocking code-review MINOR follow-up from #1405: `internal/users/oidc_cache.go`'s `oidcJWKSCache` was keyed by `jwksURI` alone, so two distinct trusted issuers sharing a `jwks_uri` could collide and share a cache entry (a hit for issuer B could silently serve JWKS bytes fetched/trusted under issuer A). Harmless today since a cache hit never re-dials, but a latent footgun. Keys the cache entries map (and the singleflight dedup key) by `(issuer, jwksURI)` via a new `jwksCacheKey` struct. Public `Fetch`/`Refresh` signatures are unchanged. ## Test plan - Added a black-box test asserting two issuers sharing a `jwks_uri` each trigger their own fetch (2 calls, not 1). - `go test ./internal/users/...` passes, 100% coverage on the modified `oidc_cache.go`. - `golangci-lint run ./internal/users/...` clean. Docs: N/A because this is an internal cache-correctness fix with no user-observable behavior change. Closes bead bookshelf-mfli8 on merge.

zombor added 1 commit 2026-08-10 00:37:42 +00:00

fix(users): key OIDC JWKS cache by (issuer, jwks_uri), not jwks_uri alone ... 64ecf13260

oidcJWKSCache.entries was keyed by jwksURI only, so two distinct trusted
issuers advertising the same jwks_uri would share a cache entry — a cache
hit for issuer B could silently serve JWKS bytes fetched (and trusted)
under issuer A's identity. Harmless today since a hit never re-dials, but
a latent footgun.

Key the cache (and its singleflight dedup key) by (issuer, jwksURI) via a
new jwksCacheKey struct. Fetch/Refresh/get signatures are unchanged.

Adds a black-box test asserting two issuers sharing a jwks_uri each
trigger their own fetch instead of sharing a cache entry.

Closes bead bookshelf-mfli8 on merge.

zombor commented 2026-08-10 00:53:43 +00:00

Author

Owner

Copy link

Security review of PR #1413 (fix(users): key OIDC JWKS cache by (issuer, jwks_uri)).

Scope reviewed: internal/users/oidc_cache.go, internal/users/oidc_cache_test.go, and the call sites in internal/users/oidc_service.go that invoke fetchJWKS/refreshJWKS.

Analysis

• The issuer value threaded into oidcJWKSCache.get/Refresh originates exclusively from cfg.Issuer — the admin-configured, trusted OIDC issuer (internal/users/oidc_service.go lines 294, 302, and all other call sites). It is never sourced from request input, the ID token's own iss claim, or any other attacker-influenceable value. The new cache key jwksCacheKey{issuer, jwksURI} therefore keys on a trusted value, which is the correct precondition for this fix to actually close the collision it targets.
• The change correctly fixes the described collision: previously the cache map was map[string]jwksEntry keyed by jwksURI alone, so if two configured issuers (or a misconfigured re-point of cfg.Issuer at a different provider sharing infrastructure) advertised the same jwks_uri, a fetch cached under issuer A could be served for issuer B without a corresponding fetch — a latent (if narrow, given the app is presently single-tenant OIDC) trust-boundary blur between issuers. Keying by (issuer, jwks_uri) closes that.
• The singleflight key was updated in parallel (issuer + "|" + jwksURI), preventing two concurrent fetches for different issuers sharing a jwks_uri from collapsing into a single shared HTTP call and result. Good — this mirrors the map-key fix and avoids reintroducing the same collision via the dedup path.
• Stale-cache-on-refetch-error behavior (if hit { serve e.raw }) still keys off the new composite key's hit/e, so the "serve stale on transient error" fallback continues to only serve an entry that was cached under the matching issuer — no regression there.
• No secrets logged: the added "issuer", issuer log fields log only the issuer URL (already logged elsewhere in this file/package), not JWKS key material or tokens.
• Test coverage (oidc_cache_test.go) directly exercises the fixed scenario: two issuers sharing one jwks_uri each trigger their own fetch (asserts calls == 2), which is the right regression test for this class of bug.

Minor observation (not a functional flaw given current trusted-input guarantees):

internal/users/oidc_cache.go:71 — sfKey := issuer + "|" + jwksURI is a naive string-concat key. In principle issuer="a|" + jwksURI="b" collides with issuer="a" + jwksURI="|b". Both inputs are trusted (admin-configured issuer / discovery-document-derived URI, not attacker input), so this is not currently exploitable, but a struct-keyed singleflight (or a delimiter guaranteed absent from URLs, or a hash) would remove the ambiguity entirely and match the map key's type-safety. Given trusted inputs this is cosmetic, not a vulnerability.

---

[MINOR] internal/users/oidc_cache.go:71 — singleflight key uses string concatenation with a | delimiter instead of a typed/struct key
issuer + "|" + jwksURI could theoretically collide if either trusted value contained a literal | (neither currently can, since both are admin-configured/discovery-derived URLs, so this is not exploitable today). Prefer a struct key (mirroring jwksCacheKey) or a delimiter guaranteed not to appear in a URL, for defense-in-depth consistency with the map-key fix this PR just made.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Security review of PR #1413 (`fix(users): key OIDC JWKS cache by (issuer, jwks_uri)`). **Scope reviewed:** `internal/users/oidc_cache.go`, `internal/users/oidc_cache_test.go`, and the call sites in `internal/users/oidc_service.go` that invoke `fetchJWKS`/`refreshJWKS`. **Analysis** - The `issuer` value threaded into `oidcJWKSCache.get`/`Refresh` originates exclusively from `cfg.Issuer` — the admin-configured, trusted OIDC issuer (`internal/users/oidc_service.go` lines 294, 302, and all other call sites). It is never sourced from request input, the ID token's own `iss` claim, or any other attacker-influenceable value. The new cache key `jwksCacheKey{issuer, jwksURI}` therefore keys on a trusted value, which is the correct precondition for this fix to actually close the collision it targets. - The change correctly fixes the described collision: previously the cache map was `map[string]jwksEntry` keyed by `jwksURI` alone, so if two configured issuers (or a misconfigured re-point of `cfg.Issuer` at a different provider sharing infrastructure) advertised the same `jwks_uri`, a fetch cached under issuer A could be served for issuer B without a corresponding fetch — a latent (if narrow, given the app is presently single-tenant OIDC) trust-boundary blur between issuers. Keying by `(issuer, jwks_uri)` closes that. - The singleflight key was updated in parallel (`issuer + "|" + jwksURI`), preventing two concurrent fetches for different issuers sharing a `jwks_uri` from collapsing into a single shared HTTP call and result. Good — this mirrors the map-key fix and avoids reintroducing the same collision via the dedup path. - Stale-cache-on-refetch-error behavior (`if hit { serve e.raw }`) still keys off the new composite key's `hit`/`e`, so the "serve stale on transient error" fallback continues to only serve an entry that was cached under the *matching* issuer — no regression there. - No secrets logged: the added `"issuer", issuer` log fields log only the issuer URL (already logged elsewhere in this file/package), not JWKS key material or tokens. - Test coverage (`oidc_cache_test.go`) directly exercises the fixed scenario: two issuers sharing one `jwks_uri` each trigger their own fetch (asserts `calls == 2`), which is the right regression test for this class of bug. **Minor observation (not a functional flaw given current trusted-input guarantees):** `internal/users/oidc_cache.go:71` — `sfKey := issuer + "|" + jwksURI` is a naive string-concat key. In principle `issuer="a|"` + `jwksURI="b"` collides with `issuer="a"` + `jwksURI="|b"`. Both inputs are trusted (admin-configured issuer / discovery-document-derived URI, not attacker input), so this is not currently exploitable, but a struct-keyed singleflight (or a delimiter guaranteed absent from URLs, or a hash) would remove the ambiguity entirely and match the map key's type-safety. Given trusted inputs this is cosmetic, not a vulnerability. --- [MINOR] internal/users/oidc_cache.go:71 — singleflight key uses string concatenation with a `|` delimiter instead of a typed/struct key `issuer + "|" + jwksURI` could theoretically collide if either trusted value contained a literal `|` (neither currently can, since both are admin-configured/discovery-derived URLs, so this is not exploitable today). Prefer a struct key (mirroring `jwksCacheKey`) or a delimiter guaranteed not to appear in a URL, for defense-in-depth consistency with the map-key fix this PR just made. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-10 00:55:11 +00:00

Author

Owner

Copy link

Code review — PR #1413 (bd-bookshelf-mfli8), diff-review only (CI green).

[MAJOR] internal/users/oidc_cache.go:189 — singleflight key built by naive string concatenation can collide across distinct (issuer, jwksURI) pairs
sfKey := issuer + "|" + jwksURI has no delimiter-collision protection: two different (issuer, jwksURI) pairs can produce the identical sfKey whenever either string contains the literal | (e.g. issuer="a|b", jwksURI="c" vs issuer="a", jwksURI="b|c" both yield "a|b|c"). jwksURI in particular is sourced from meta.JWKSURI in the OIDC discovery document — i.e. content returned over the network by the (trusted, but externally-hosted) IdP, not a value under this codebase's control — so a crafted/misconfigured discovery response can produce a |-containing jwks_uri. net/url and Go's HTTP client do not reject an unescaped | in a URL, so this is reachable, not just theoretical.
Why it matters: unlike the map (correctly keyed by the jwksCacheKey struct, immune to this), singleflight.Group.Do takes a bare string key. If two concurrent get() calls for different (issuer, jwksURI) pairs collide on sfKey, only one fetch runs and both callers receive the same raw bytes — and each caller then writes those bytes into its own (distinct, correctly-scoped) map entry. That reintroduces, one layer down, the exact cross-issuer JWKS confusion this PR sets out to eliminate (issuer B's cache entry ends up populated with issuer A's fetched key material).
Fix: derive the singleflight key unambiguously from the same struct that keys the map — e.g. fmt.Sprintf("%d:%s|%s", len(issuer), issuer, jwksURI) (length-prefixing the first field removes the ambiguity) or hash the struct (sha256 over a \x00-joined encoding). Do not rely on an arbitrary separator character being absent from attacker/IdP-influenced input.

Everything else checked out:

• internal/users/oidc_cache.go:117-121 — jwksCacheKey{issuer, jwksURI} map key is a genuine struct, correctly scoping the cache (not just the log line); no path still keys the map by jwksURI alone.
• internal/users/wire.go:184-196 (origin/bd-bookshelf-mfli8) — Fetch/Refresh call sites are unchanged method-value references; the pre-existing (ctx, jwksURI, issuer) signature was already threading issuer through, so no caller needed updating for this change.
• internal/users/oidc_backchannel.go:107-146 — the backchannel-logout jwksCache is a genuinely separate, single-entry cache instantiated once per OIDCBackchannelLogout closure (internal/users/oidc_backchannel.go:355), tied to one cfg from a single getConfig call. Pergamum currently supports exactly one configured OIDC provider (OIDCConfig is singular, one Issuer field throughout oidc_service.go), so this cache has no multi-issuer surface today — the agent's claim that it needs no equivalent fix is correct, not overclaimed.
• internal/users/oidc_cache_test.go:328-359 — new black-box test (package users_test, via users.ExportNewOIDCJWKSCache) asserts two issuers sharing a jwks_uri produce two fetches (calls.Load() == 2), directly covering the fixed collision. Existing singleflight-dedup and TTL/Refresh tests are otherwise untouched and still pass with the new key type.
• Logging: "jwks_uri", jwksURI, "issuer", issuer added consistently to both the stale-serve warning and the cache-populated info log — no secrets logged (JWKS is public key material).

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

Code review — PR #1413 (bd-bookshelf-mfli8), diff-review only (CI green). [MAJOR] internal/users/oidc_cache.go:189 — singleflight key built by naive string concatenation can collide across distinct (issuer, jwksURI) pairs `sfKey := issuer + "|" + jwksURI` has no delimiter-collision protection: two *different* (issuer, jwksURI) pairs can produce the identical `sfKey` whenever either string contains the literal `|` (e.g. issuer=`"a|b"`, jwksURI=`"c"` vs issuer=`"a"`, jwksURI=`"b|c"` both yield `"a|b|c"`). `jwksURI` in particular is sourced from `meta.JWKSURI` in the OIDC discovery document — i.e. content returned over the network by the (trusted, but externally-hosted) IdP, not a value under this codebase's control — so a crafted/misconfigured discovery response can produce a `|`-containing jwks_uri. `net/url` and Go's HTTP client do not reject an unescaped `|` in a URL, so this is reachable, not just theoretical. Why it matters: unlike the map (correctly keyed by the `jwksCacheKey` struct, immune to this), `singleflight.Group.Do` takes a bare `string` key. If two concurrent `get()` calls for different (issuer, jwksURI) pairs collide on `sfKey`, only one fetch runs and *both* callers receive the same `raw` bytes — and each caller then writes those bytes into its own (distinct, correctly-scoped) map entry. That reintroduces, one layer down, the exact cross-issuer JWKS confusion this PR sets out to eliminate (issuer B's cache entry ends up populated with issuer A's fetched key material). Fix: derive the singleflight key unambiguously from the same struct that keys the map — e.g. `fmt.Sprintf("%d:%s|%s", len(issuer), issuer, jwksURI)` (length-prefixing the first field removes the ambiguity) or hash the struct (`sha256` over a `\x00`-joined encoding). Do not rely on an arbitrary separator character being absent from attacker/IdP-influenced input. Everything else checked out: - internal/users/oidc_cache.go:117-121 — `jwksCacheKey{issuer, jwksURI}` map key is a genuine struct, correctly scoping the *cache* (not just the log line); no path still keys the map by `jwksURI` alone. - internal/users/wire.go:184-196 (origin/bd-bookshelf-mfli8) — `Fetch`/`Refresh` call sites are unchanged method-value references; the pre-existing `(ctx, jwksURI, issuer)` signature was already threading issuer through, so no caller needed updating for this change. - internal/users/oidc_backchannel.go:107-146 — the backchannel-logout `jwksCache` is a genuinely separate, single-entry cache instantiated once per `OIDCBackchannelLogout` closure (internal/users/oidc_backchannel.go:355), tied to one `cfg` from a single `getConfig` call. Pergamum currently supports exactly one configured OIDC provider (`OIDCConfig` is singular, one `Issuer` field throughout oidc_service.go), so this cache has no multi-issuer surface today — the agent's claim that it needs no equivalent fix is correct, not overclaimed. - internal/users/oidc_cache_test.go:328-359 — new black-box test (`package users_test`, via `users.ExportNewOIDCJWKSCache`) asserts two issuers sharing a jwks_uri produce two fetches (`calls.Load() == 2`), directly covering the fixed collision. Existing singleflight-dedup and TTL/Refresh tests are otherwise untouched and still pass with the new key type. - Logging: `"jwks_uri", jwksURI, "issuer", issuer` added consistently to both the stale-serve warning and the cache-populated info log — no secrets logged (JWKS is public key material). REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor added 1 commit 2026-08-10 00:57:47 +00:00

fix(oidc): collision-safe singleflight JWKS cache key (review MAJOR, bookshelf-mfli8) ... 761d4138f4

The singleflight dedup key in oidcJWKSCache.get() was built by naively
concatenating issuer + "|" + jwksURI. Since jwksURI comes from the IdP's
discovery document (network-sourced, IdP-influenced), a literal "|" in it
could make two distinct (issuer, jwksURI) pairs collide:
  - ("x|y", "z") -> "x|y|z"
  - ("x", "y|z") -> "x|y|z"

This would cause one issuer's JWKS to be cached/deduplicated under another
issuer's singleflight entry, reintroducing the cross-issuer confusion that
the struct-keyed cache correctly prevents.

Fix: use length-prefix format for the singleflight key:
  fmt.Sprintf("%d:%s|%d:%s", len(issuer), issuer, len(jwksURI), jwksURI)

This makes collisions impossible regardless of "|" in either value.

Add test demonstrating the collision safety: two (issuer, jwksURI) pairs
that would collide under naive concat now correctly trigger separate fetches.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016tRKybTpfjQ4SxmNdVFLHi

zombor referenced this pull request 2026-08-10 01:15:48 +00:00

Hygiene: adopt db.RunInTx/InTx at all hand-rolled BeginTx sites (bookshelf-dhlj4.1) #1419

zombor added 1 commit 2026-08-10 01:30:14 +00:00

Merge branch 'main' into bd-bookshelf-mfli8 9e65efd057

zombor merged commit 33382e4103 into main 2026-08-10 01:40:29 +00:00

zombor referenced this pull request from a commit 2026-08-10 01:40:35 +00:00

Merge pull request 'fix(users): key OIDC JWKS cache by (issuer, jwks_uri) (bookshelf-mfli8)' (#1413) from bd-bookshelf-mfli8 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1413

No description provided.