diff --git a/CHANGELOG.md b/CHANGELOG.md index bb25c73..aad49db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,40 @@ ## Unreleased +### Security + +- **Alg-downgrade defense relaxed for Keycloak-shape IdPs (v2.1.0 pre-tag fix).** + Pre-fix, the IdP-bind alg-downgrade check at `internal/auth/oidc/service.go` + refused to load any OIDC provider whose discovery doc advertised HS256 / + HS384 / HS512 / `none` in `id_token_signing_alg_values_supported` — + even if RS256 was ALSO advertised. This broke binding against + Keycloak 26.x (and a handful of other real IdPs) which list every alg + the codebase is capable of in their discovery doc, regardless of which + one the realm actually signs with. The v2.1.0 Phase-10 live-IdP smoke + surfaced the regression: 6 testcontainers-Keycloak integration tests + failed with `oidc: IdP advertises weak signing algorithms (HS*/none); refusing to use as defense against downgrade attacks: HS256`. + **Fix:** the check now refuses only when the intersection of advertised + vs `DefaultAllowedAlgs` is EMPTY — an IdP advertising HS256 alongside + RS256 binds successfully, but an IdP advertising HS-only / none-only + still fails closed. The per-token alg pin at sig-verify time + (`isDisallowedAlg`, service.go ~L1177) remains the load-bearing defense + against the actual algorithm-confusion attack (forged HS256 token + signed with the IdP's RS256 pubkey as HMAC secret) — go-oidc/v3's + verifier rejects any token whose `alg` header isn't in the configured + allow-list, regardless of what the discovery doc claims. Updates: + `Service.getOrLoad` alg-check loop rewritten to compute intersection; + `ErrIdPDowngradeAdvertised` docstring reflects new semantics; + `TestDiscovery` dry-run validator surfaces HS*/none alongside RS* as + an informational note (not a hard fail); `docs/operator/auth-threat-model.md` + alg-allow-list section updated to call out the load-bearing-defense + hierarchy. Tests: `TestService_IdPDowngradeDefense_RS256PlusHS256_BindsSuccessfully` + (positive — Keycloak-shape) + `TestService_IdPDowngradeDefense_RejectsHSOnlyAdvertised` + (negative — pathological intersection-empty case) + + `TestService_RefreshKeys_CatchesPostLoadDowngrade` updated to assert + intersection-empty post-rotation; `TestTestDiscovery_AlgDowngrade_HS256AlongsideRS256_BindsWithNote` + + `TestTestDiscovery_AlgDowngrade_HSOnly_StillTrips_HardFail` pin the + dry-run validator's new behavior. + ### Tests - **Vitest coverage for the 2026-05-10/11 GUI batch (Audit 2026-05-11 Fix 12).** diff --git a/docs/operator/auth-threat-model.md b/docs/operator/auth-threat-model.md index 90a268f..1b1452a 100644 --- a/docs/operator/auth-threat-model.md +++ b/docs/operator/auth-threat-model.md @@ -169,17 +169,27 @@ constant, router-level no-rbacGate-wraps-protocol-paths). - **Algorithm allow-list, never `none`, never HMAC.** The service- layer pinning lives in `internal/auth/oidc/service.go::disallowedAlgs` - and the IdP-downgrade-attack defense in - `Service.guardAdvertisedAlgs`. At provider creation AND on every - `RefreshKeys`, the IdP's advertised - `id_token_signing_alg_values_supported` is intersected with the - allow-list (RS256 / RS512 / ES256 / ES384 / EdDSA). If the IdP - advertises HS256/HS384/HS512 or `none` AT ALL, provider creation - is rejected - the IdP has not yet signed a single token, but the - service refuses to trust an IdP that COULD sign one with a weak - alg. coreos/go-oidc additionally enforces the allow-list per-token - at verify time as defense-in-depth against an upstream library - regression. + + `isDisallowedAlg`. The per-token alg check at sig-verify time + (`isDisallowedAlg`, line ~1177) is the load-bearing defense — every + ID token whose JWS header carries an alg outside the allow-list + (RS256 / RS512 / ES256 / ES384 / EdDSA) is rejected with + `ErrAlgRejected`. coreos/go-oidc additionally enforces the allow-list + per-token at verify time as defense-in-depth against an upstream + library regression. The IdP-downgrade-attack secondary defense at + provider creation / `RefreshKeys` (v2.1.0-relaxed semantics) + intersects the IdP's advertised `id_token_signing_alg_values_supported` + with the allow-list and rejects only when the intersection is EMPTY + — i.e., the IdP advertises NO acceptable alg. Pre-v2.1.0 the check + strict-denied on ANY HS*/`none` advertisement; that broke against + Keycloak 26.x (which lists every alg it's capable of in its discovery + doc, including HS*, even when the realm only signs with RS256). The + relaxation is safe because the per-token alg pin already prevents + a real algorithm-confusion attack — a forged HS256 token using the + IdP's RS256 pubkey as HMAC secret is rejected at sig-verify regardless + of what the discovery doc advertises. Operators worried about a + compromised IdP rotating to weak algs without rotating its certctl + provider config get defense-in-depth from `JWKSStatus` + the alert + hooks in the GUI panel. - **Exact `iss` match.** ID-token `iss` claim must equal the configured `OIDCProvider.IssuerURL` byte-for-byte (sentinel `ErrIssuerMismatch`). A token from a different IdP - even one diff --git a/internal/auth/oidc/coverage_fill_test.go b/internal/auth/oidc/coverage_fill_test.go index 4ebbe55..10302ee 100644 --- a/internal/auth/oidc/coverage_fill_test.go +++ b/internal/auth/oidc/coverage_fill_test.go @@ -124,11 +124,13 @@ func TestTestDiscovery_HappyPath_AgainstMockIdP(t *testing.T) { } } -// TestTestDiscovery_AlgDowngradeDetected runs against a stub IdP that -// advertises HS256 in id_token_signing_alg_values_supported. The -// function MUST flag the downgrade attack vector in res.Errors but -// MUST NOT short-circuit (per-leg observability is the contract). -func TestTestDiscovery_AlgDowngradeDetected(t *testing.T) { +// TestTestDiscovery_AlgDowngrade_HS256AlongsideRS256_BindsWithNote runs +// against a stub IdP that advertises both HS256 + RS256 (Keycloak-shape). +// Under v2.1.0-relaxed semantics this must SUCCEED (DiscoverySucceeded=true, +// JWKSReachable=true) and surface only an informational note about the +// weak-alg advertisement — NOT a hard "alg-downgrade defense tripped" error. +// The per-token alg pin at sig-verify time remains the load-bearing defense. +func TestTestDiscovery_AlgDowngrade_HS256AlongsideRS256_BindsWithNote(t *testing.T) { svc := newServiceForUnitTest(t) mux := http.NewServeMux() srv := httptest.NewServer(mux) @@ -156,15 +158,60 @@ func TestTestDiscovery_AlgDowngradeDetected(t *testing.T) { if !res.DiscoverySucceeded { t.Errorf("expected DiscoverySucceeded=true; got Errors=%v", res.Errors) } + // The Keycloak-shape advertisement must NOT trip the hard fail. + for _, e := range res.Errors { + if strings.Contains(e, "alg-downgrade defense tripped") { + t.Errorf("v2.1.0-relaxed semantics: HS256+RS256 must NOT trip hard fail; got %q", e) + } + } + // Informational note must be present. + noteFound := false + for _, e := range res.Errors { + if strings.Contains(e, "note:") && strings.Contains(e, "HS256") { + noteFound = true + } + } + if !noteFound { + t.Errorf("expected informational note about HS256 in errors; got %v", res.Errors) + } +} + +// TestTestDiscovery_AlgDowngrade_HSOnly_StillTrips_HardFail asserts the +// pathological intersection-empty case still hard-fails. +func TestTestDiscovery_AlgDowngrade_HSOnly_StillTrips_HardFail(t *testing.T) { + svc := newServiceForUnitTest(t) + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + defer srv.Close() + + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "jwks_uri": srv.URL + "/jwks", + "id_token_signing_alg_values_supported": []string{"HS256", "HS384"}, // no RS + }) + }) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"keys":[]}`)) + }) + + res, err := svc.TestDiscovery(context.Background(), srv.URL) + if err != nil { + t.Fatalf("TestDiscovery: %v", err) + } found := false for _, e := range res.Errors { - if strings.Contains(e, "alg-downgrade defense tripped") && strings.Contains(e, "HS256") { + if strings.Contains(e, "alg-downgrade defense tripped") && strings.Contains(e, "only weak algorithms") { found = true break } } if !found { - t.Errorf("expected alg-downgrade-tripped:HS256 in errors; got %v", res.Errors) + t.Errorf("expected hard-fail for HS-only IdP; got %v", res.Errors) } } diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index 3953b48..755fac0 100644 --- a/internal/auth/oidc/service.go +++ b/internal/auth/oidc/service.go @@ -52,9 +52,16 @@ import ( // 6. IdP downgrade-attack defense: at provider creation / // RefreshKeys, the discovery doc's // `id_token_signing_alg_values_supported` is intersected with -// the allow-list. If the IdP advertises HS* / none AT ALL, the -// provider is rejected with an actionable error so a future -// compromised IdP can't downgrade. +// the allow-list. The provider is rejected only when the +// intersection is EMPTY (i.e., the IdP advertises NO acceptable +// signing alg). Real IdPs like Keycloak advertise the full list +// of algorithms they're capable of including HS*, but actually +// sign with RS256; the per-token alg check at sig-verify time +// (line ~1177 `isDisallowedAlg`) is the load-bearing defense +// against an actual algorithm-confusion attack. Pre-v2.1.0 this +// check was strict-deny on any HS* advertisement; v2.1.0 relaxed +// to the intersection-empty form so Keycloak / other real IdPs +// bind successfully. // 7. JWKS handling delegated to coreos/go-oidc/v3; on JWKS fetch // failure during a key rotation the service returns // ErrJWKSUnreachable (HTTP 503), existing sessions untouched, @@ -299,10 +306,16 @@ var ( // allow-list. HTTP 400. ErrAlgRejected = errors.New("oidc: ID token signed with disallowed algorithm") - // ErrIdPDowngradeAdvertised: provider's discovery doc advertises - // HS* or `none` algorithms. Provider creation / refresh rejects. - // HTTP 400. - ErrIdPDowngradeAdvertised = errors.New("oidc: IdP advertises weak signing algorithms (HS*/none); refusing to use as defense against downgrade attacks") + // ErrIdPDowngradeAdvertised: provider's discovery doc lists NO + // alg from DefaultAllowedAlgs in `id_token_signing_alg_values_ + // supported`. v2.1.0-relaxed semantics: an IdP that advertises + // HS* / none ALONGSIDE RS256+ binds successfully; only an IdP + // that advertises ZERO acceptable algs fails closed. The + // per-token alg check at sig-verify time (isDisallowedAlg) is + // the load-bearing defense against an actual algorithm-confusion + // attack. Provider creation / refresh rejects only when the + // intersection is empty. HTTP 400. + ErrIdPDowngradeAdvertised = errors.New("oidc: IdP advertises no acceptable signing algorithms (intersection of advertised vs DefaultAllowedAlgs is empty)") // ErrJWKSUnreachable: JWKS endpoint fetch failed during a // rotation. The in-flight login fails 503; existing sessions @@ -367,8 +380,11 @@ var ( // DefaultAllowedAlgs is the operator-default ID-token signing algorithm // allow-list. Configurable per-provider but the union must be a subset // of this set. HMAC algorithms (HS256/HS384/HS512) and `none` are -// NEVER in the default set; the IdP downgrade defense rejects any -// provider that advertises them in discovery. +// NEVER in the default set; sig-verify rejects any token whose `alg` +// header is outside this list. v2.1.0-relaxed: the IdP downgrade defense +// at provider creation only rejects a provider whose advertised list +// intersects this allow-list to the EMPTY set — Keycloak / Auth0 / etc. +// that advertise HS* alongside RS* bind successfully. var DefaultAllowedAlgs = []string{ gooidc.RS256, gooidc.RS512, gooidc.ES256, gooidc.ES384, @@ -1064,11 +1080,36 @@ func (s *Service) getOrLoad(ctx context.Context, providerID string) (*providerEn if cerr := provider.Claims(&advertised); cerr != nil { return nil, fmt.Errorf("oidc: discovery claims: %w", cerr) } + // Alg-downgrade defense (v2.1.0 relaxation — keycloak-compat). + // Pre-v2.1.0 this loop refused to bind if the IdP advertised ANY + // HS*/none in `id_token_signing_alg_values_supported`. That was + // too strict for real-world IdPs: Keycloak 26.x (and several others) + // list every alg they're CAPABLE of, not the ones the realm is + // actively signing with — so a realm signing exclusively with RS256 + // still advertises HS256/HS384/HS512/PS*/etc. in its discovery doc. + // The actual algorithm-confusion attack (forged HS256 token with the + // RS256 pubkey as HMAC secret) is caught at sig-verify time by the + // per-token alg check (isDisallowedAlg, ~L1177) AND by go-oidc/v3's + // verifier-side allow-list pin to DefaultAllowedAlgs. So a Keycloak + // that *says* it supports HS256 but only ever signs with RS256 is + // safe to bind to. + // New semantics: refuse only if the IdP advertises NO acceptable alg + // (intersection of advertised vs DefaultAllowedAlgs is empty). A + // pathological HS-only IdP still fails closed. + allowedSet := make(map[string]struct{}, len(DefaultAllowedAlgs)) + for _, a := range DefaultAllowedAlgs { + allowedSet[a] = struct{}{} + } + hasAcceptable := false for _, a := range advertised.IDTokenSigningAlgValuesSupported { - if _, deny := disallowedAlgs[a]; deny { - return nil, fmt.Errorf("%w: %s", ErrIdPDowngradeAdvertised, a) + if _, ok := allowedSet[a]; ok { + hasAcceptable = true + break } } + if len(advertised.IDTokenSigningAlgValuesSupported) > 0 && !hasAcceptable { + return nil, fmt.Errorf("%w: advertised=%v", ErrIdPDowngradeAdvertised, advertised.IDTokenSigningAlgValuesSupported) + } // Compute the effective allow-list: intersection of the default // allow-list AND any operator-configured restriction (currently diff --git a/internal/auth/oidc/service_test.go b/internal/auth/oidc/service_test.go index fcd0f82..f60e150 100644 --- a/internal/auth/oidc/service_test.go +++ b/internal/auth/oidc/service_test.go @@ -576,32 +576,40 @@ func TestService_ATHash_UnknownAlgReturnsFalse(t *testing.T) { } } -// Test 11: IdP downgrade-attack defense. A provider whose discovery doc -// advertises HS256 in id_token_signing_alg_values_supported is REJECTED -// by the cache load with ErrIdPDowngradeAdvertised. -func TestService_IdPDowngradeDefense_RejectsHSAdvertised(t *testing.T) { +// Test 11: IdP downgrade-attack defense (v2.1.0-relaxed semantics). +// Pre-v2.1.0 ANY HS* advertisement refused. Real IdPs like Keycloak +// 26.x advertise the full alg list (HS+RS+ES+PS) but actually sign +// with RS256 — they failed the old check + caused legitimate-IdP +// integration tests to red. New contract: bind when the intersection +// of advertised vs DefaultAllowedAlgs is non-empty; reject only when +// the IdP advertises NO acceptable alg. Per-token alg pin at sig-verify +// (isDisallowedAlg) still catches an actual algorithm-confusion attack. +func TestService_IdPDowngradeDefense_RS256PlusHS256_BindsSuccessfully(t *testing.T) { idp := newMockIdP(t) - idp.advertisedAlgs = []string{"RS256", "HS256"} // HS256 is the downgrade vector + idp.advertisedAlgs = []string{"RS256", "HS256"} // Keycloak-shape — both advertised - svc, _ := newServiceWithProvider(t, idp.URL(), "op-bad-idp") + svc, _ := newServiceWithProvider(t, idp.URL(), "op-keycloak-shape") - _, err := svc.getOrLoad(context.Background(), "op-bad-idp") - if !errors.Is(err, ErrIdPDowngradeAdvertised) { - t.Errorf("err = %v; want ErrIdPDowngradeAdvertised", err) + entry, err := svc.getOrLoad(context.Background(), "op-keycloak-shape") + if err != nil { + t.Fatalf("RS256+HS256 advertisement must bind successfully (v2.1.0 relaxation); got %v", err) + } + if entry.provider == nil || entry.verifier == nil { + t.Errorf("expected non-nil provider+verifier on successful bind") } } -// Test 12: IdP downgrade-attack defense — `none` advertisement also -// triggers rejection. -func TestService_IdPDowngradeDefense_RejectsNoneAdvertised(t *testing.T) { +// Test 12: IdP downgrade-attack defense — HS-only advertisement is +// still rejected (the pathological case the defense protects against). +func TestService_IdPDowngradeDefense_RejectsHSOnlyAdvertised(t *testing.T) { idp := newMockIdP(t) - idp.advertisedAlgs = []string{"RS256", "none"} + idp.advertisedAlgs = []string{"HS256", "HS384", "HS512"} // no RS/ES — pathological - svc, _ := newServiceWithProvider(t, idp.URL(), "op-none-idp") + svc, _ := newServiceWithProvider(t, idp.URL(), "op-hs-only-idp") - _, err := svc.getOrLoad(context.Background(), "op-none-idp") + _, err := svc.getOrLoad(context.Background(), "op-hs-only-idp") if !errors.Is(err, ErrIdPDowngradeAdvertised) { - t.Errorf("err = %v; want ErrIdPDowngradeAdvertised", err) + t.Errorf("err = %v; want ErrIdPDowngradeAdvertised (intersection is empty)", err) } } @@ -624,7 +632,9 @@ func TestService_GetOrLoad_AcceptsCleanIdP(t *testing.T) { // Test 14: RefreshKeys evicts the cache + re-fetches discovery, which // re-runs the downgrade defense. If the IdP rotated to advertising -// HS256 between loads, RefreshKeys catches it. +// ONLY weak algs between loads (intersection-empty case), RefreshKeys +// catches it. Pre-v2.1.0 this test asserted the strict-deny "any HS*" +// behavior; v2.1.0-relaxed asserts the intersection-empty behavior. func TestService_RefreshKeys_CatchesPostLoadDowngrade(t *testing.T) { idp := newMockIdP(t) svc, _ := newServiceWithProvider(t, idp.URL(), "op-rotate") @@ -633,11 +643,11 @@ func TestService_RefreshKeys_CatchesPostLoadDowngrade(t *testing.T) { t.Fatalf("initial load: %v", err) } - // IdP rotates to advertising HS256. - idp.advertisedAlgs = []string{"RS256", "HS256"} + // IdP rotates to advertising ONLY HS algs — pathological case. + idp.advertisedAlgs = []string{"HS256", "HS384"} err := svc.RefreshKeys(context.Background(), "op-rotate") if !errors.Is(err, ErrIdPDowngradeAdvertised) { - t.Errorf("RefreshKeys err = %v; want ErrIdPDowngradeAdvertised", err) + t.Errorf("RefreshKeys err = %v; want ErrIdPDowngradeAdvertised (intersection-empty)", err) } } diff --git a/internal/auth/oidc/test_discovery.go b/internal/auth/oidc/test_discovery.go index bf8cbff..151ee73 100644 --- a/internal/auth/oidc/test_discovery.go +++ b/internal/auth/oidc/test_discovery.go @@ -76,14 +76,37 @@ func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDis res.JWKSURI = advertised.JWKSURI res.UserInfoEndpoint = advertised.UserInfoEndpoint - // Step 3 — alg-downgrade defense. The IdP MUST NOT advertise HS* - // or none in the signing-alg list (operators that bind certctl to - // an IdP advertising these are at risk of a forged-token attack). - // Same check applied in getOrLoad's production path. + // Step 3 — alg-downgrade defense (v2.1.0-relaxed semantics). + // Pre-v2.1.0 this loop appended an error for ANY HS*/none in the + // IdP's advertised list. That was strict-deny but incompatible with + // real IdPs like Keycloak 26.x which list every alg they're capable + // of, even though the realm only signs with RS256. + // New semantics: only flag the IdP if the intersection of advertised + // vs DefaultAllowedAlgs is empty (a pathological all-weak IdP). Each + // HS*/none advertisement is still surfaced as an informational note + // so operators can ask their IdP team to tighten the list, but it's + // no longer a hard fail. The per-token alg check at sig-verify time + // (isDisallowedAlg in service.go ~L1177) is the load-bearing defense. + allowedSet := make(map[string]struct{}, len(DefaultAllowedAlgs)) + for _, a := range DefaultAllowedAlgs { + allowedSet[a] = struct{}{} + } + hasAcceptable := false + weak := []string{} for _, a := range advertised.IDTokenSigningAlgValuesSupported { - if _, deny := disallowedAlgs[a]; deny { - res.Errors = append(res.Errors, fmt.Sprintf("alg-downgrade defense tripped: IdP advertises %s in id_token_signing_alg_values_supported", a)) + if _, ok := allowedSet[a]; ok { + hasAcceptable = true } + if _, deny := disallowedAlgs[a]; deny { + weak = append(weak, a) + } + } + if len(advertised.IDTokenSigningAlgValuesSupported) > 0 && !hasAcceptable { + res.Errors = append(res.Errors, fmt.Sprintf("alg-downgrade defense tripped: IdP advertises only weak algorithms (%v) — no acceptable alg from %v present", advertised.IDTokenSigningAlgValuesSupported, DefaultAllowedAlgs)) + } else if len(weak) > 0 { + // Informational only — RS/ES present alongside HS, so the + // IdP binds successfully but the operator should know. + res.Errors = append(res.Errors, fmt.Sprintf("note: IdP advertises weak algorithms %v alongside acceptable ones — verifier-side alg pin prevents downgrade, but tightening the IdP's advertised list is recommended", weak)) } // Step 4 — JWKS reachability. The go-oidc Verifier defers JWKS