From 641bea825dec6ca0e4bcc44dfc23ba0f7f44eed4 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 4 May 2026 21:47:42 -0700 Subject: [PATCH] fix(iam): require non-empty issuer in OIDC discovery doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous "doc.Issuer != "" && ..." guard let a discovery document that omitted the issuer field bypass the issuer-mismatch check entirely, letting the doc steer fetchJWKS at any URL it provided. OIDC Discovery 1.0 §3 mandates the issuer field; treat missing as a hard failure same as mismatched. Trailing-slash equivalence still applies. Adds TestDiscoveryRejectsMissingIssuer alongside the existing TestDiscoveryRejectsIssuerMismatch via a new omitDiscoveryIssuer toggle on fakeIDP. --- weed/iam/oidc/oidc_discovery_test.go | 38 +++++++++++++++++++++++++--- weed/iam/oidc/oidc_provider.go | 16 +++++++----- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/weed/iam/oidc/oidc_discovery_test.go b/weed/iam/oidc/oidc_discovery_test.go index deb823d89..66e4d75e9 100644 --- a/weed/iam/oidc/oidc_discovery_test.go +++ b/weed/iam/oidc/oidc_discovery_test.go @@ -19,6 +19,7 @@ type fakeIDP struct { disableDiscovery bool discoveryStatusCode int discoveryIssuer string + omitDiscoveryIssuer bool // when true, the discovery doc omits the "issuer" field entirely customJWKSPathSuffix string // optional suffix that fakeIDP serves at /custom/ jwks JWKS } @@ -46,10 +47,11 @@ func newFakeIDP(t *testing.T) *fakeIDP { if idp.customJWKSPathSuffix != "" { jwksURI = idp.server.URL + "/custom/" + idp.customJWKSPathSuffix } - _ = json.NewEncoder(w).Encode(map[string]string{ - "issuer": issuer, - "jwks_uri": jwksURI, - }) + body := map[string]string{"jwks_uri": jwksURI} + if !idp.omitDiscoveryIssuer { + body["issuer"] = issuer + } + _ = json.NewEncoder(w).Encode(body) }) mux.HandleFunc("/discovered/jwks", func(w http.ResponseWriter, r *http.Request) { idp.jwksHits.Add(1) @@ -165,3 +167,31 @@ func TestDiscoveryRejectsIssuerMismatch(t *testing.T) { t.Fatalf("expected 1 fallback JWKS hit, got %d", got) } } + +// TestDiscoveryRejectsMissingIssuer: a discovery document that omits the +// issuer field entirely must be treated the same as one that supplies a +// mismatched issuer. Otherwise an attacker who can intercept the discovery +// response can strip the issuer field and the comparison silently passes, +// letting the document point fetchJWKS at any URL it pleases. +func TestDiscoveryRejectsMissingIssuer(t *testing.T) { + idp := newFakeIDP(t) + idp.omitDiscoveryIssuer = true + p := newProviderForIDP(t, idp, "") + + if err := p.fetchJWKS(context.Background()); err != nil { + t.Fatalf("fetchJWKS should fall back to /.well-known/jwks.json on issuer-missing discovery: %v", err) + } + if got := idp.discoveryHits.Load(); got != 1 { + t.Fatalf("expected 1 discovery probe, got %d", got) + } + // The discovery document was rejected; the JWKS that ultimately served + // us must be the fallback one, not the discovered URI. The fakeIDP + // counts both hits under jwksHits since they share a counter; what + // matters is that customJWKSHits stayed zero. + if got := idp.customJWKSHits.Load(); got != 0 { + t.Fatalf("custom JWKS endpoint must not have been used, got %d hits", got) + } + if got := idp.jwksHits.Load(); got != 1 { + t.Fatalf("expected 1 fallback JWKS hit, got %d", got) + } +} diff --git a/weed/iam/oidc/oidc_provider.go b/weed/iam/oidc/oidc_provider.go index 277023a47..c9f289aa6 100644 --- a/weed/iam/oidc/oidc_provider.go +++ b/weed/iam/oidc/oidc_provider.go @@ -683,12 +683,16 @@ func (p *OIDCProvider) fetchDiscoveryJWKSUri(ctx context.Context, discoveryURL s return "", fmt.Errorf("discovery document missing jwks_uri") } - // Issuer must match: a discovery doc that points to a different issuer is - // 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, "/") { + // Issuer must be present and match: a discovery doc that points to a + // different issuer is either a misconfiguration or an attack against + // issuer-confusion, and a doc that omits the issuer field entirely + // would have bypassed the previous check (doc.Issuer != "" guard) and + // silently accepted whatever JWKS URI the document supplied. OIDC + // Discovery 1.0 §3 mandates the issuer field, so treat missing as a + // hard failure. Compare after trimming a single trailing slash on each + // side because real IdPs disagree on whether the configured issuer + // has one. + if 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) }