mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-05-23 02:01:32 +00:00
fix(iam): require non-empty issuer in OIDC discovery doc
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.
This commit is contained in:
@@ -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/<suffix>
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user