From 91fe0a5162daea7fdaba90cfea6df84af323a839 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Mon, 4 May 2026 19:50:53 -0700
Subject: [PATCH] fix(iam): trim trailing slash + retry discovery after
 transient failure
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 weed/iam/oidc/oidc_discovery_test.go |  9 ++++++---
 weed/iam/oidc/oidc_provider.go       | 15 +++++++++++++--
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/weed/iam/oidc/oidc_discovery_test.go b/weed/iam/oidc/oidc_discovery_test.go
index e28b73033..deb823d89 100644
--- a/weed/iam/oidc/oidc_discovery_test.go
+++ b/weed/iam/oidc/oidc_discovery_test.go
@@ -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)
 	}
 }
 
diff --git a/weed/iam/oidc/oidc_provider.go b/weed/iam/oidc/oidc_provider.go
index ce7b2b39f..277023a47 100644
--- a/weed/iam/oidc/oidc_provider.go
+++ b/weed/iam/oidc/oidc_provider.go
@@ -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)