mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 19:01:34 +00:00
harden(oidc): relax alg-downgrade IdP-bind check to intersection-empty (Keycloak compat)
Phase-10 live-IdP smoke (Keycloak 26.x via testcontainers-go) revealed
the IdP-bind alg-downgrade check was too strict for real-world IdPs.
6 of the integration tests in internal/auth/oidc/integration_keycloak*_test.go
were failing with:
oidc: IdP advertises weak signing algorithms (HS*/none);
refusing to use as defense against downgrade attacks: HS256
Keycloak 26.x (and several other real-world IdPs — Auth0 when HS-mode is
enabled, some Authentik configs) advertise EVERY alg they're capable of
in the discovery doc's id_token_signing_alg_values_supported field, even
when the realm only signs with RS256 in practice. Pre-fix the IdP-bind
check refused on ANY HS* or 'none' advertisement → no real Keycloak deploy
could ever bind a provider row, hence the integration-test failures.
The strict-deny check was defense-in-depth on top of the load-bearing
per-token alg-pin at sig-verify time (isDisallowedAlg, service.go L1177):
that check rejects every ID token whose JWS header carries an alg outside
DefaultAllowedAlgs, regardless of what the discovery doc advertises.
A forged HS256 token signed with the IdP's RS256 pubkey as HMAC secret
is rejected at sig-verify time → the actual algorithm-confusion attack
is closed by the per-token pin, NOT by the discovery-doc check.
Fix: relax the IdP-bind check to refuse only when the intersection of
advertised vs DefaultAllowedAlgs is EMPTY (the pathological all-weak-alg
IdP case). Keycloak (RS256 + HS256 advertised) now binds successfully;
an HS-only IdP still fails closed.
Changes:
- internal/auth/oidc/service.go: rewrite the alg-check loop at L1067 in
getOrLoad / RefreshKeys to compute the intersection set; refuse only
when no acceptable alg is advertised. ErrIdPDowngradeAdvertised
docstring updated to reflect new contract. DefaultAllowedAlgs
docstring + the package-level design-comment block at L40-72 updated
with v2.1.0-relaxed semantics callouts.
- internal/auth/oidc/test_discovery.go: TestDiscovery dry-run validator
rewritten to surface HS*/none alongside RS* as an informational note
('note: IdP advertises weak algorithms %v alongside acceptable ones')
rather than a hard-fail error. HS-only / none-only still hard-fails.
- internal/auth/oidc/service_test.go: TestService_IdPDowngradeDefense_*
tests updated. Renamed:
- RejectsHSAdvertised → RS256PlusHS256_BindsSuccessfully (positive)
- RejectsNoneAdvertised → RejectsHSOnlyAdvertised (intersection-empty)
- RefreshKeys_CatchesPostLoadDowngrade rotated to HS-only post-load
- internal/auth/oidc/coverage_fill_test.go: TestTestDiscovery_AlgDowngradeDetected
split into _HS256AlongsideRS256_BindsWithNote (positive, asserts note
but no hard-fail) + _HSOnly_StillTrips_HardFail (intersection-empty).
- docs/operator/auth-threat-model.md: OIDC token-validation alg-allow-list
section rewritten to call out the load-bearing-defense hierarchy
(per-token pin first, IdP-bind check defense-in-depth) and document
the v2.1.0 relaxation rationale.
- CHANGELOG.md: ### Security entry under Unreleased.
Verify: go test ./internal/auth/oidc/ -short PASS; gofmt clean; go vet
clean. The Keycloak integration tests should now pass when the operator
re-runs 'make keycloak-integration-test'.
This commit is contained in:
@@ -2,6 +2,40 @@
|
|||||||
|
|
||||||
## Unreleased
|
## 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
|
### Tests
|
||||||
|
|
||||||
- **Vitest coverage for the 2026-05-10/11 GUI batch (Audit 2026-05-11 Fix 12).**
|
- **Vitest coverage for the 2026-05-10/11 GUI batch (Audit 2026-05-11 Fix 12).**
|
||||||
|
|||||||
@@ -169,17 +169,27 @@ constant, router-level no-rbacGate-wraps-protocol-paths).
|
|||||||
|
|
||||||
- **Algorithm allow-list, never `none`, never HMAC.** The service-
|
- **Algorithm allow-list, never `none`, never HMAC.** The service-
|
||||||
layer pinning lives in `internal/auth/oidc/service.go::disallowedAlgs`
|
layer pinning lives in `internal/auth/oidc/service.go::disallowedAlgs`
|
||||||
and the IdP-downgrade-attack defense in
|
+ `isDisallowedAlg`. The per-token alg check at sig-verify time
|
||||||
`Service.guardAdvertisedAlgs`. At provider creation AND on every
|
(`isDisallowedAlg`, line ~1177) is the load-bearing defense — every
|
||||||
`RefreshKeys`, the IdP's advertised
|
ID token whose JWS header carries an alg outside the allow-list
|
||||||
`id_token_signing_alg_values_supported` is intersected with the
|
(RS256 / RS512 / ES256 / ES384 / EdDSA) is rejected with
|
||||||
allow-list (RS256 / RS512 / ES256 / ES384 / EdDSA). If the IdP
|
`ErrAlgRejected`. coreos/go-oidc additionally enforces the allow-list
|
||||||
advertises HS256/HS384/HS512 or `none` AT ALL, provider creation
|
per-token at verify time as defense-in-depth against an upstream
|
||||||
is rejected - the IdP has not yet signed a single token, but the
|
library regression. The IdP-downgrade-attack secondary defense at
|
||||||
service refuses to trust an IdP that COULD sign one with a weak
|
provider creation / `RefreshKeys` (v2.1.0-relaxed semantics)
|
||||||
alg. coreos/go-oidc additionally enforces the allow-list per-token
|
intersects the IdP's advertised `id_token_signing_alg_values_supported`
|
||||||
at verify time as defense-in-depth against an upstream library
|
with the allow-list and rejects only when the intersection is EMPTY
|
||||||
regression.
|
— 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
|
- **Exact `iss` match.** ID-token `iss` claim must equal the
|
||||||
configured `OIDCProvider.IssuerURL` byte-for-byte (sentinel
|
configured `OIDCProvider.IssuerURL` byte-for-byte (sentinel
|
||||||
`ErrIssuerMismatch`). A token from a different IdP - even one
|
`ErrIssuerMismatch`). A token from a different IdP - even one
|
||||||
|
|||||||
@@ -124,11 +124,13 @@ func TestTestDiscovery_HappyPath_AgainstMockIdP(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestTestDiscovery_AlgDowngradeDetected runs against a stub IdP that
|
// TestTestDiscovery_AlgDowngrade_HS256AlongsideRS256_BindsWithNote runs
|
||||||
// advertises HS256 in id_token_signing_alg_values_supported. The
|
// against a stub IdP that advertises both HS256 + RS256 (Keycloak-shape).
|
||||||
// function MUST flag the downgrade attack vector in res.Errors but
|
// Under v2.1.0-relaxed semantics this must SUCCEED (DiscoverySucceeded=true,
|
||||||
// MUST NOT short-circuit (per-leg observability is the contract).
|
// JWKSReachable=true) and surface only an informational note about the
|
||||||
func TestTestDiscovery_AlgDowngradeDetected(t *testing.T) {
|
// 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)
|
svc := newServiceForUnitTest(t)
|
||||||
mux := http.NewServeMux()
|
mux := http.NewServeMux()
|
||||||
srv := httptest.NewServer(mux)
|
srv := httptest.NewServer(mux)
|
||||||
@@ -156,15 +158,60 @@ func TestTestDiscovery_AlgDowngradeDetected(t *testing.T) {
|
|||||||
if !res.DiscoverySucceeded {
|
if !res.DiscoverySucceeded {
|
||||||
t.Errorf("expected DiscoverySucceeded=true; got Errors=%v", res.Errors)
|
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
|
found := false
|
||||||
for _, e := range res.Errors {
|
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
|
found = true
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if !found {
|
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)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -52,9 +52,16 @@ import (
|
|||||||
// 6. IdP downgrade-attack defense: at provider creation /
|
// 6. IdP downgrade-attack defense: at provider creation /
|
||||||
// RefreshKeys, the discovery doc's
|
// RefreshKeys, the discovery doc's
|
||||||
// `id_token_signing_alg_values_supported` is intersected with
|
// `id_token_signing_alg_values_supported` is intersected with
|
||||||
// the allow-list. If the IdP advertises HS* / none AT ALL, the
|
// the allow-list. The provider is rejected only when the
|
||||||
// provider is rejected with an actionable error so a future
|
// intersection is EMPTY (i.e., the IdP advertises NO acceptable
|
||||||
// compromised IdP can't downgrade.
|
// 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
|
// 7. JWKS handling delegated to coreos/go-oidc/v3; on JWKS fetch
|
||||||
// failure during a key rotation the service returns
|
// failure during a key rotation the service returns
|
||||||
// ErrJWKSUnreachable (HTTP 503), existing sessions untouched,
|
// ErrJWKSUnreachable (HTTP 503), existing sessions untouched,
|
||||||
@@ -299,10 +306,16 @@ var (
|
|||||||
// allow-list. HTTP 400.
|
// allow-list. HTTP 400.
|
||||||
ErrAlgRejected = errors.New("oidc: ID token signed with disallowed algorithm")
|
ErrAlgRejected = errors.New("oidc: ID token signed with disallowed algorithm")
|
||||||
|
|
||||||
// ErrIdPDowngradeAdvertised: provider's discovery doc advertises
|
// ErrIdPDowngradeAdvertised: provider's discovery doc lists NO
|
||||||
// HS* or `none` algorithms. Provider creation / refresh rejects.
|
// alg from DefaultAllowedAlgs in `id_token_signing_alg_values_
|
||||||
// HTTP 400.
|
// supported`. v2.1.0-relaxed semantics: an IdP that advertises
|
||||||
ErrIdPDowngradeAdvertised = errors.New("oidc: IdP advertises weak signing algorithms (HS*/none); refusing to use as defense against downgrade attacks")
|
// 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
|
// ErrJWKSUnreachable: JWKS endpoint fetch failed during a
|
||||||
// rotation. The in-flight login fails 503; existing sessions
|
// rotation. The in-flight login fails 503; existing sessions
|
||||||
@@ -367,8 +380,11 @@ var (
|
|||||||
// DefaultAllowedAlgs is the operator-default ID-token signing algorithm
|
// DefaultAllowedAlgs is the operator-default ID-token signing algorithm
|
||||||
// allow-list. Configurable per-provider but the union must be a subset
|
// allow-list. Configurable per-provider but the union must be a subset
|
||||||
// of this set. HMAC algorithms (HS256/HS384/HS512) and `none` are
|
// of this set. HMAC algorithms (HS256/HS384/HS512) and `none` are
|
||||||
// NEVER in the default set; the IdP downgrade defense rejects any
|
// NEVER in the default set; sig-verify rejects any token whose `alg`
|
||||||
// provider that advertises them in discovery.
|
// 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{
|
var DefaultAllowedAlgs = []string{
|
||||||
gooidc.RS256, gooidc.RS512,
|
gooidc.RS256, gooidc.RS512,
|
||||||
gooidc.ES256, gooidc.ES384,
|
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 {
|
if cerr := provider.Claims(&advertised); cerr != nil {
|
||||||
return nil, fmt.Errorf("oidc: discovery claims: %w", cerr)
|
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 {
|
for _, a := range advertised.IDTokenSigningAlgValuesSupported {
|
||||||
if _, deny := disallowedAlgs[a]; deny {
|
if _, ok := allowedSet[a]; ok {
|
||||||
return nil, fmt.Errorf("%w: %s", ErrIdPDowngradeAdvertised, a)
|
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
|
// Compute the effective allow-list: intersection of the default
|
||||||
// allow-list AND any operator-configured restriction (currently
|
// allow-list AND any operator-configured restriction (currently
|
||||||
|
|||||||
@@ -576,32 +576,40 @@ func TestService_ATHash_UnknownAlgReturnsFalse(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Test 11: IdP downgrade-attack defense. A provider whose discovery doc
|
// Test 11: IdP downgrade-attack defense (v2.1.0-relaxed semantics).
|
||||||
// advertises HS256 in id_token_signing_alg_values_supported is REJECTED
|
// Pre-v2.1.0 ANY HS* advertisement refused. Real IdPs like Keycloak
|
||||||
// by the cache load with ErrIdPDowngradeAdvertised.
|
// 26.x advertise the full alg list (HS+RS+ES+PS) but actually sign
|
||||||
func TestService_IdPDowngradeDefense_RejectsHSAdvertised(t *testing.T) {
|
// 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 := 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")
|
entry, err := svc.getOrLoad(context.Background(), "op-keycloak-shape")
|
||||||
if !errors.Is(err, ErrIdPDowngradeAdvertised) {
|
if err != nil {
|
||||||
t.Errorf("err = %v; want ErrIdPDowngradeAdvertised", err)
|
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
|
// Test 12: IdP downgrade-attack defense — HS-only advertisement is
|
||||||
// triggers rejection.
|
// still rejected (the pathological case the defense protects against).
|
||||||
func TestService_IdPDowngradeDefense_RejectsNoneAdvertised(t *testing.T) {
|
func TestService_IdPDowngradeDefense_RejectsHSOnlyAdvertised(t *testing.T) {
|
||||||
idp := newMockIdP(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) {
|
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
|
// Test 14: RefreshKeys evicts the cache + re-fetches discovery, which
|
||||||
// re-runs the downgrade defense. If the IdP rotated to advertising
|
// 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) {
|
func TestService_RefreshKeys_CatchesPostLoadDowngrade(t *testing.T) {
|
||||||
idp := newMockIdP(t)
|
idp := newMockIdP(t)
|
||||||
svc, _ := newServiceWithProvider(t, idp.URL(), "op-rotate")
|
svc, _ := newServiceWithProvider(t, idp.URL(), "op-rotate")
|
||||||
@@ -633,11 +643,11 @@ func TestService_RefreshKeys_CatchesPostLoadDowngrade(t *testing.T) {
|
|||||||
t.Fatalf("initial load: %v", err)
|
t.Fatalf("initial load: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// IdP rotates to advertising HS256.
|
// IdP rotates to advertising ONLY HS algs — pathological case.
|
||||||
idp.advertisedAlgs = []string{"RS256", "HS256"}
|
idp.advertisedAlgs = []string{"HS256", "HS384"}
|
||||||
err := svc.RefreshKeys(context.Background(), "op-rotate")
|
err := svc.RefreshKeys(context.Background(), "op-rotate")
|
||||||
if !errors.Is(err, ErrIdPDowngradeAdvertised) {
|
if !errors.Is(err, ErrIdPDowngradeAdvertised) {
|
||||||
t.Errorf("RefreshKeys err = %v; want ErrIdPDowngradeAdvertised", err)
|
t.Errorf("RefreshKeys err = %v; want ErrIdPDowngradeAdvertised (intersection-empty)", err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -76,14 +76,37 @@ func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDis
|
|||||||
res.JWKSURI = advertised.JWKSURI
|
res.JWKSURI = advertised.JWKSURI
|
||||||
res.UserInfoEndpoint = advertised.UserInfoEndpoint
|
res.UserInfoEndpoint = advertised.UserInfoEndpoint
|
||||||
|
|
||||||
// Step 3 — alg-downgrade defense. The IdP MUST NOT advertise HS*
|
// Step 3 — alg-downgrade defense (v2.1.0-relaxed semantics).
|
||||||
// or none in the signing-alg list (operators that bind certctl to
|
// Pre-v2.1.0 this loop appended an error for ANY HS*/none in the
|
||||||
// an IdP advertising these are at risk of a forged-token attack).
|
// IdP's advertised list. That was strict-deny but incompatible with
|
||||||
// Same check applied in getOrLoad's production path.
|
// 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 {
|
for _, a := range advertised.IDTokenSigningAlgValuesSupported {
|
||||||
if _, deny := disallowedAlgs[a]; deny {
|
if _, ok := allowedSet[a]; ok {
|
||||||
res.Errors = append(res.Errors, fmt.Sprintf("alg-downgrade defense tripped: IdP advertises %s in id_token_signing_alg_values_supported", a))
|
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
|
// Step 4 — JWKS reachability. The go-oidc Verifier defers JWKS
|
||||||
|
|||||||
Reference in New Issue
Block a user