mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-08 20:38:53 +00:00
fefeccfa59
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'.
149 lines
6.4 KiB
Go
149 lines
6.4 KiB
Go
package oidc
|
|
|
|
// Audit 2026-05-10 MED-5 closure — dry-run validator for OIDC provider
|
|
// configuration. Lets operators verify discovery + JWKS reachability +
|
|
// alg-downgrade defense BEFORE persisting a provider row. Mirrors the
|
|
// non-persistence-touching subset of getOrLoad.
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"net/http"
|
|
|
|
gooidc "github.com/coreos/go-oidc/v3/oidc"
|
|
)
|
|
|
|
// TestDiscoveryResult is the report TestDiscovery returns. The HTTP
|
|
// layer marshals this verbatim. Each field is independently observable
|
|
// so the GUI can render a per-check status row.
|
|
//
|
|
// `Errors` collects every leg that failed; a partial-success case
|
|
// (e.g. discovery OK but alg-downgrade tripped) returns
|
|
// DiscoverySucceeded=true + a non-empty Errors slice.
|
|
type TestDiscoveryResult struct {
|
|
DiscoverySucceeded bool `json:"discovery_succeeded"`
|
|
JWKSReachable bool `json:"jwks_reachable"`
|
|
SupportedAlgValues []string `json:"supported_alg_values"`
|
|
IssParamSupported bool `json:"iss_param_supported"`
|
|
IssuerEcho string `json:"issuer_echo,omitempty"` // the iss value the IdP advertised
|
|
AuthorizationURL string `json:"authorization_url,omitempty"`
|
|
TokenURL string `json:"token_url,omitempty"`
|
|
JWKSURI string `json:"jwks_uri,omitempty"`
|
|
UserInfoEndpoint string `json:"userinfo_endpoint,omitempty"`
|
|
Errors []string `json:"errors,omitempty"`
|
|
}
|
|
|
|
// TestDiscovery runs the read-only subset of getOrLoad against a
|
|
// candidate issuer URL: fetches the discovery doc, runs the
|
|
// alg-downgrade defense, parses the RFC 9207 iss-parameter advert,
|
|
// then fetches the JWKS once to confirm reachability.
|
|
//
|
|
// The function NEVER persists anything; the caller is the
|
|
// /api/v1/auth/oidc/test endpoint that the GUI uses for dry-runs.
|
|
//
|
|
// Service-layer entry point so the handler stays HTTP-shaped only.
|
|
func (s *Service) TestDiscovery(ctx context.Context, issuerURL string) (*TestDiscoveryResult, error) {
|
|
res := &TestDiscoveryResult{}
|
|
|
|
// Step 1 — discovery. gooidc.NewProvider fetches
|
|
// `<issuer>/.well-known/openid-configuration` and runs the iss
|
|
// match check internally; on failure it returns a fmt-style
|
|
// wrapped error.
|
|
provider, err := gooidc.NewProvider(ctx, issuerURL)
|
|
if err != nil {
|
|
res.Errors = append(res.Errors, fmt.Sprintf("discovery fetch failed: %v", err))
|
|
return res, nil // Non-fatal at this layer; the response carries the per-leg failure.
|
|
}
|
|
res.DiscoverySucceeded = true
|
|
res.IssuerEcho = issuerURL
|
|
endpoint := provider.Endpoint()
|
|
res.AuthorizationURL = endpoint.AuthURL
|
|
res.TokenURL = endpoint.TokenURL
|
|
|
|
// Step 2 — parse the claims we care about from the discovery doc.
|
|
var advertised struct {
|
|
IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"`
|
|
AuthorizationResponseIssParamSupported bool `json:"authorization_response_iss_parameter_supported"`
|
|
JWKSURI string `json:"jwks_uri"`
|
|
UserInfoEndpoint string `json:"userinfo_endpoint"`
|
|
}
|
|
if cerr := provider.Claims(&advertised); cerr != nil {
|
|
res.Errors = append(res.Errors, fmt.Sprintf("discovery claims: %v", cerr))
|
|
return res, nil
|
|
}
|
|
res.SupportedAlgValues = advertised.IDTokenSigningAlgValuesSupported
|
|
res.IssParamSupported = advertised.AuthorizationResponseIssParamSupported
|
|
res.JWKSURI = advertised.JWKSURI
|
|
res.UserInfoEndpoint = advertised.UserInfoEndpoint
|
|
|
|
// 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 _, 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
|
|
// fetch until first token-verify; for the dry-run we explicitly
|
|
// HEAD/GET the JWKS endpoint to confirm network reachability.
|
|
if advertised.JWKSURI == "" {
|
|
res.Errors = append(res.Errors, "discovery doc omits jwks_uri")
|
|
} else if ok, herr := jwksReachable(ctx, advertised.JWKSURI); !ok {
|
|
if herr != nil {
|
|
res.Errors = append(res.Errors, fmt.Sprintf("JWKS fetch failed: %v", herr))
|
|
} else {
|
|
res.Errors = append(res.Errors, "JWKS endpoint returned non-200")
|
|
}
|
|
} else {
|
|
res.JWKSReachable = true
|
|
}
|
|
|
|
return res, nil
|
|
}
|
|
|
|
// jwksReachable issues a GET against the JWKS URI and returns ok=true
|
|
// when the response status is 2xx. Used by TestDiscovery for the
|
|
// reachability leg of the dry-run.
|
|
//
|
|
// Kept distinct from go-oidc's internal JWKS fetcher because we want
|
|
// to surface the HTTP status to the operator without requiring a
|
|
// token-verify round-trip.
|
|
var jwksReachable = func(ctx context.Context, jwksURI string) (bool, error) {
|
|
req, err := http.NewRequestWithContext(ctx, http.MethodGet, jwksURI, nil)
|
|
if err != nil {
|
|
return false, err
|
|
}
|
|
resp, err := http.DefaultClient.Do(req)
|
|
if err != nil {
|
|
return false, err
|
|
}
|
|
defer resp.Body.Close()
|
|
return resp.StatusCode >= 200 && resp.StatusCode < 300, nil
|
|
}
|