fix(iam): trim trailing slash + retry discovery after transient failure

Two OIDC discovery edge cases reviewers flagged:

1. Issuer comparison was sensitive to trailing slashes. resolveJWKSUri
   trims them when building the discovery URL, but the doc.Issuer ↔
   p.config.Issuer check did not, so an IDP whose issuer claim drops or
   adds the slash relative to the configured value would be falsely
   rejected. Trim a single trailing slash on each side before comparing.

2. discoveryFailed flipped to true on any error and stayed there for the
   process lifetime. A transient 5xx at startup permanently locked the
   provider into the /.well-known/jwks.json fallback. Reset the flag at
   the top of fetchJWKSLocked when no URI has been cached yet, so each
   JWKS refresh (typically once per TTL = 1h) reattempts discovery.
   Successful discovery remains cached via resolvedJWKSUri so we don't
   pay the discovery RTT on every refresh.

Addresses gemini security-medium + medium reviews on PR #9318.
This commit is contained in:
Chris Lu
2026-05-04 22:06:19 -07:00
parent 64a60607c6
commit 91fe0a5162
2 changed files with 19 additions and 5 deletions
+6 -3
View File
@@ -120,12 +120,15 @@ func TestDiscoveryFallback404(t *testing.T) {
t.Fatalf("expected 1 JWKS hit at fallback uri, got %d", got)
}
// Subsequent fetches skip discovery — discoveryFailed is sticky.
// Subsequent fetches retry discovery — discoveryFailed resets at the top
// of fetchJWKSLocked when no URI was cached, so a transient 5xx at
// startup doesn't lock the provider into the fallback path forever.
// Retry rate is bounded by the JWKS TTL (one retry per refresh cycle).
if err := p.fetchJWKS(context.Background()); err != nil {
t.Fatalf("fetchJWKS second: %v", err)
}
if got := idp.discoveryHits.Load(); got != 1 {
t.Fatalf("discovery probe should not retry after failure, got %d hits", got)
if got := idp.discoveryHits.Load(); got != 2 {
t.Fatalf("discovery probe should retry while no URI is cached, got %d hits", got)
}
}
+13 -2
View File
@@ -684,8 +684,11 @@ func (p *OIDCProvider) fetchDiscoveryJWKSUri(ctx context.Context, discoveryURL s
}
// Issuer must match: a discovery doc that points to a different issuer is
// either a misconfiguration or an attack against issuer-confusion.
if doc.Issuer != "" && doc.Issuer != p.config.Issuer {
// either a misconfiguration or an attack against issuer-confusion. Compare
// after trimming a single trailing slash on each side; OIDC Discovery
// 1.0 is silent on slash equivalence and real IdPs disagree on whether
// the configured issuer has one.
if doc.Issuer != "" && strings.TrimSuffix(doc.Issuer, "/") != strings.TrimSuffix(p.config.Issuer, "/") {
return "", fmt.Errorf("discovery issuer %q does not match configured issuer %q", doc.Issuer, p.config.Issuer)
}
@@ -704,7 +707,15 @@ func (p *OIDCProvider) fetchJWKS(ctx context.Context) error {
// fetchJWKSLocked fetches the JWKS from the provider. The caller must hold
// p.mu (write lock); the function writes p.jwksCache and p.jwksFetchedAt
// without taking the lock itself.
//
// Each fetch reattempts discovery if the previous attempt failed: a
// transient 5xx that flipped discoveryFailed at startup shouldn't lock the
// provider into the fallback path forever. The retry rate is bounded by
// the JWKS TTL (typically 1h), so the discovery RTT cost is amortized.
func (p *OIDCProvider) fetchJWKSLocked(ctx context.Context) error {
if p.config.JWKSUri == "" && p.resolvedJWKSUri == "" {
p.discoveryFailed = false
}
jwksURL, err := p.resolveJWKSUriLocked(ctx)
if err != nil {
return fmt.Errorf("resolve JWKS URI: %v", err)