diff --git a/CHANGELOG.md b/CHANGELOG.md index cdea33b..b9ae390 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,18 @@ cookie lifetime is unchanged (10 minutes) and only the callback handler consumes it; the wider path scope is harmless. +- **RFC 9207 `iss` URL parameter check on OIDC callback (Audit 2026-05-10 + MED-17).** When the matched IdP's discovery doc advertises + `authorization_response_iss_parameter_supported: true`, certctl now requires + the `iss` query parameter on `/auth/oidc/callback` and enforces a + constant-time compare against the configured provider's `IssuerURL`. Mismatch + rejects with HTTP 400; the audit row's `failure_category` distinguishes + `iss_param_missing` / `iss_param_mismatch` (RFC 9207 leg) from the existing + `id_token_iss_mismatch` (in-token iss claim leg). Closes the mix-up-attack + defense for modern Keycloak, Authentik, and public-trust CAs that ship + RFC-9207 discovery. Providers that don't advertise support (the majority + today) keep pre-fix behavior — back-compat is preserved. + ## v2.1.0 - Auth Bundles 1 + 2: RBAC primitive + OIDC SSO + sessions ⚠️ > **SECURITY: AUDIT YOUR API KEYS.** diff --git a/internal/api/handler/auth_session_oidc.go b/internal/api/handler/auth_session_oidc.go index 5227d93..b87414a 100644 --- a/internal/api/handler/auth_session_oidc.go +++ b/internal/api/handler/auth_session_oidc.go @@ -56,7 +56,11 @@ import ( // consumes. Phase 3's *oidc.Service satisfies this directly. type OIDCAuthHandshaker interface { HandleAuthRequest(ctx context.Context, providerID string) (authURL, cookieValue, preLoginID string, err error) - HandleCallback(ctx context.Context, preLoginCookie, code, callbackState, ip, userAgent string) (*oidcsvc.CallbackResult, error) + // Audit 2026-05-10 MED-17 — callbackIss carries the value of the + // RFC 9207 `iss` query parameter on /auth/oidc/callback (empty + // string when the IdP doesn't send it). The service enforces the + // check only when the provider's discovery doc advertised support. + HandleCallback(ctx context.Context, preLoginCookie, code, callbackState, callbackIss, ip, userAgent string) (*oidcsvc.CallbackResult, error) RefreshKeys(ctx context.Context, providerID string) error } @@ -272,6 +276,12 @@ func (h *AuthSessionOIDCHandler) LoginCallback(w http.ResponseWriter, r *http.Re q := r.URL.Query() code := strings.TrimSpace(q.Get("code")) state := strings.TrimSpace(q.Get("state")) + // Audit 2026-05-10 MED-17 — RFC 9207 iss URL parameter. NOT + // trimmed; preserved exactly as sent so the service-layer compare + // against the matched provider's IssuerURL is byte-strict. The IdP + // emits this only when advertised in its discovery doc; the + // service-layer check is a no-op otherwise. + callbackIss := q.Get("iss") if code == "" || state == "" { Error(w, http.StatusBadRequest, "missing code or state query parameter") return @@ -286,7 +296,7 @@ func (h *AuthSessionOIDCHandler) LoginCallback(w http.ResponseWriter, r *http.Re clientIP := clientIPFromRequest(r) userAgent := r.UserAgent() - res, err := h.oidcSvc.HandleCallback(r.Context(), preLoginCookie.Value, code, state, clientIP, userAgent) + res, err := h.oidcSvc.HandleCallback(r.Context(), preLoginCookie.Value, code, state, callbackIss, clientIP, userAgent) if err != nil { // Audit 2026-05-10 HIGH-7 — instead of a blank 400, redirect // to /login?error=oidc_failed&reason=. The LoginPage @@ -1152,10 +1162,30 @@ func clientIPFromRequest(r *http.Request) string { // classifyOIDCFailure maps an OIDC service error to a stable audit // category string. Used for the failure_category audit detail; the // wire stays uniform 400. +// +// Audit 2026-05-10 MED-17 — the three iss-related sentinel errors are +// dispatched via errors.Is BEFORE the substring fall-through so they +// stay distinguishable in the audit row: +// - ErrIssParamMissing → iss_param_missing +// - ErrIssParamMismatch → iss_param_mismatch +// - ErrIssuerMismatch → id_token_iss_mismatch +// +// errors.Is is used for the iss family because all three error +// strings contain "iss" and substring matching would either collapse +// them or order-dependently mis-classify. func classifyOIDCFailure(err error) string { if err == nil { return "ok" } + // Audit 2026-05-10 MED-17 — typed dispatch for the iss family. + switch { + case errors.Is(err, oidcsvc.ErrIssParamMissing): + return "iss_param_missing" + case errors.Is(err, oidcsvc.ErrIssParamMismatch): + return "iss_param_mismatch" + case errors.Is(err, oidcsvc.ErrIssuerMismatch): + return "id_token_iss_mismatch" + } msg := strings.ToLower(err.Error()) switch { case strings.Contains(msg, "pre-login"): diff --git a/internal/api/handler/auth_session_oidc_test.go b/internal/api/handler/auth_session_oidc_test.go index be15953..9017016 100644 --- a/internal/api/handler/auth_session_oidc_test.go +++ b/internal/api/handler/auth_session_oidc_test.go @@ -46,7 +46,7 @@ type stubOIDCSvc struct { func (s *stubOIDCSvc) HandleAuthRequest(_ context.Context, _ string) (string, string, string, error) { return s.authURL, s.cookie, s.preLoginID, s.authReqErr } -func (s *stubOIDCSvc) HandleCallback(_ context.Context, _, _, _, _, _ string) (*oidcsvc.CallbackResult, error) { +func (s *stubOIDCSvc) HandleCallback(_ context.Context, _, _, _, _, _, _ string) (*oidcsvc.CallbackResult, error) { return s.callbackRes, s.callbackErr } func (s *stubOIDCSvc) RefreshKeys(_ context.Context, _ string) error { return s.refreshErr } @@ -1197,6 +1197,15 @@ func TestClassifyOIDCFailure(t *testing.T) { {errors.New("oidc: groups did not match any configured mapping"), "unmapped_groups"}, {errors.New("oidc: configured groups claim missing or malformed"), "groups_missing"}, {errors.New("oidc: jwks unreachable"), "jwks_unreachable"}, + // Audit 2026-05-10 MED-17 — typed dispatch beats the substring + // fallthrough because all three iss-family sentinels contain + // "iss" in their message and would otherwise mis-classify. + {oidcsvc.ErrIssParamMissing, "iss_param_missing"}, + {oidcsvc.ErrIssParamMismatch, "iss_param_mismatch"}, + {oidcsvc.ErrIssuerMismatch, "id_token_iss_mismatch"}, + // Wrapped variants must round-trip through errors.Is. + {fmt.Errorf("upstream: %w", oidcsvc.ErrIssParamMissing), "iss_param_missing"}, + {fmt.Errorf("upstream: %w", oidcsvc.ErrIssParamMismatch), "iss_param_mismatch"}, {errors.New("some other error"), "unspecified"}, } for _, tc := range cases { diff --git a/internal/auth/oidc/bench_test.go b/internal/auth/oidc/bench_test.go index 0670f94..fe636d2 100644 --- a/internal/auth/oidc/bench_test.go +++ b/internal/auth/oidc/bench_test.go @@ -100,7 +100,7 @@ func BenchmarkOIDC_SteadyState(b *testing.B) { } start := time.Now() - _, err = svc.HandleCallback(ctx, cookie, "bench-code", "bench-state", "10.0.0.1", "bench/1.0") + _, err = svc.HandleCallback(ctx, cookie, "bench-code", "bench-state", "", "10.0.0.1", "bench/1.0") elapsed := time.Since(start) if err != nil { b.Fatalf("HandleCallback: %v", err) diff --git a/internal/auth/oidc/integration_keycloak_test.go b/internal/auth/oidc/integration_keycloak_test.go index 7f3988e..7dca852 100644 --- a/internal/auth/oidc/integration_keycloak_test.go +++ b/internal/auth/oidc/integration_keycloak_test.go @@ -432,7 +432,7 @@ func TestKeycloakIntegration_AuthCodeFlow_HappyPath(t *testing.T) { code, state := driveAuthCodeFlow(t, authURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) // Complete the OIDC handshake. - res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "10.0.0.1", "integration-test/1.0") + res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "", "10.0.0.1", "integration-test/1.0") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -491,7 +491,7 @@ func TestKeycloakIntegration_LogoutRevokesSession(t *testing.T) { t.Fatalf("HandleAuthRequest: %v", err) } code, state := driveAuthCodeFlow(t, authURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "ip", "ua") + res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "", "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -534,7 +534,7 @@ func TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey(t *testing.T) t.Fatalf("pre-rotate HandleAuthRequest: %v", err) } preCode, preState := driveAuthCodeFlow(t, preAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, "ip", "ua"); err != nil { + if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, "", "ip", "ua"); err != nil { t.Fatalf("pre-rotate HandleCallback: %v", err) } @@ -553,7 +553,7 @@ func TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey(t *testing.T) t.Fatalf("post-rotate HandleAuthRequest: %v", err) } postCode, postState := driveAuthCodeFlow(t, postAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - if _, err := svc.HandleCallback(ctx, postCookie, postCode, postState, "ip", "ua"); err != nil { + if _, err := svc.HandleCallback(ctx, postCookie, postCode, postState, "", "ip", "ua"); err != nil { t.Fatalf("post-rotate HandleCallback: %v (rotation broke validation?)", err) } } @@ -578,7 +578,7 @@ func TestKeycloakIntegration_UnmappedGroupsFailsClosed(t *testing.T) { t.Fatalf("HandleAuthRequest: %v", err) } code, state := driveAuthCodeFlow(t, authURL, testfixtures.ViewerUser, testfixtures.ViewerPassword) - _, err = svc.HandleCallback(ctx, preCookie, code, state, "ip", "ua") + _, err = svc.HandleCallback(ctx, preCookie, code, state, "", "ip", "ua") if !errors.Is(err, oidc.ErrGroupsUnmapped) { t.Errorf("HandleCallback err = %v, want ErrGroupsUnmapped (fail-closed for unmapped groups)", err) } diff --git a/internal/auth/oidc/logging_test.go b/internal/auth/oidc/logging_test.go index ee82eb5..10e23f9 100644 --- a/internal/auth/oidc/logging_test.go +++ b/internal/auth/oidc/logging_test.go @@ -92,7 +92,7 @@ func TestLoggingHygiene_HandleCallback_LeaksNothing(t *testing.T) { defer restore() authCode := "secret-auth-code-do-not-leak" - res, err := svc.HandleCallback(context.Background(), cookie, authCode, "the-state", "10.0.0.1", "Mozilla") + res, err := svc.HandleCallback(context.Background(), cookie, authCode, "the-state", "", "10.0.0.1", "Mozilla") if err != nil { t.Fatalf("HandleCallback: %v", err) } diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index 16f78f5..b3dc657 100644 --- a/internal/auth/oidc/service.go +++ b/internal/auth/oidc/service.go @@ -98,6 +98,15 @@ type providerEntry struct { oauthConfig *oauth2.Config allowedAlgs []string // intersected: domain config ∩ allow-list ∩ IdP-advertised plaintext []byte // decrypted client secret; held for token exchange + + // Audit 2026-05-10 MED-17 — RFC 9207 iss-URL-parameter support. + // Populated from the discovery doc's + // `authorization_response_iss_parameter_supported` claim during + // getOrLoad. When true, HandleCallback REQUIRES a non-empty + // callback iss URL param and compares it against the provider's + // IssuerURL. When false (the default for most IdPs that haven't + // rolled RFC 9207 yet), the check is skipped. + issParamSupported bool } // OIDCProviderLookup is a narrow read-side projection of @@ -160,8 +169,30 @@ var ( // ErrIssuerMismatch: ID token `iss` doesn't match the configured // provider issuer_url. HTTP 400. + // + // Audit 2026-05-10 MED-17 — also returned when the RFC 9207 iss + // URL parameter check fails (provider advertises + // authorization_response_iss_parameter_supported=true but the + // callback iss is missing or mismatched). The handler's + // classifyOIDCFailure breaks the two cases apart by audit + // failure_category (iss_param_missing / iss_param_mismatch / + // id_token_iss_mismatch). ErrIssuerMismatch = errors.New("oidc: issuer mismatch") + // ErrIssParamMissing: provider's discovery doc advertises + // authorization_response_iss_parameter_supported=true but the + // callback URL had no `iss` query parameter. Per RFC 9207 §2.4 the + // client MUST reject the response in this case. HTTP 400. Pre-fix, + // the callback path ignored the parameter entirely. + ErrIssParamMissing = errors.New("oidc: provider advertises iss-parameter support but callback omitted it") + + // ErrIssParamMismatch: provider's discovery doc advertises + // authorization_response_iss_parameter_supported=true and the + // callback supplied an `iss` query parameter, but it doesn't match + // the matched provider's issuer URL. Mixed-up attack defense per + // RFC 9207 §2.3. HTTP 400. + ErrIssParamMismatch = errors.New("oidc: callback iss parameter does not match provider issuer URL") + // ErrAudienceMismatch: ID token `aud` doesn't include the // configured client_id. HTTP 400. ErrAudienceMismatch = errors.New("oidc: audience mismatch") @@ -383,9 +414,18 @@ type CallbackResult struct { } // HandleCallback completes the OIDC flow. +// +// Audit 2026-05-10 MED-17 — `callbackIss` is the value of the `iss` +// query parameter on /auth/oidc/callback, exactly as sent by the IdP. +// When the matched provider's discovery doc advertises +// authorization_response_iss_parameter_supported=true (RFC 9207 §3), +// we require this parameter and verify it equals the provider's +// IssuerURL. When the provider doesn't advertise support (the default +// for most IdPs that haven't rolled RFC 9207 yet), the parameter is +// ignored — preserving back-compat with the pre-fix call path. func (s *Service) HandleCallback( ctx context.Context, - preLoginCookie, code, callbackState, ip, userAgent string, + preLoginCookie, code, callbackState, callbackIss, ip, userAgent string, ) (*CallbackResult, error) { // Step 1: consume the pre-login row (single-use). providerID, storedState, storedNonce, verifier, err := s.preLogin.LookupAndConsume(ctx, preLoginCookie) @@ -403,6 +443,23 @@ func (s *Service) HandleCallback( return nil, err } + // Step 2.5 — Audit 2026-05-10 MED-17 — RFC 9207 iss URL parameter + // check. Only enforced when the provider advertised support in its + // discovery doc. Compares against the matched provider's + // IssuerURL (which is what go-oidc's gooidc.NewProvider verified + // the discovery doc's own iss against during getOrLoad). Mismatch + // is the load-bearing defense against mix-up attacks where the + // honest IdP returns the auth code to the wrong endpoint because + // of a malicious co-tenant relying-party. + if entry.issParamSupported { + if callbackIss == "" { + return nil, ErrIssParamMissing + } + if subtle.ConstantTimeCompare([]byte(callbackIss), []byte(entry.cfgRow.IssuerURL)) != 1 { + return nil, ErrIssParamMismatch + } + } + // Step 3: exchange the auth code for tokens (with PKCE verifier). token, err := entry.oauthConfig.Exchange(ctx, code, oauth2.VerifierOption(verifier)) if err != nil { @@ -755,8 +812,14 @@ func (s *Service) getOrLoad(ctx context.Context, providerID string) (*providerEn // IdP downgrade-attack defense. The discovery doc's // id_token_signing_alg_values_supported MUST NOT include any // disallowed alg. + // + // Audit 2026-05-10 MED-17 — we also read + // `authorization_response_iss_parameter_supported` from the same + // claims call to drive the RFC 9207 iss-URL-parameter check in + // HandleCallback. var advertised struct { - IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` + IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` + AuthorizationResponseIssParamSupported bool `json:"authorization_response_iss_parameter_supported"` } if cerr := provider.Claims(&advertised); cerr != nil { return nil, fmt.Errorf("oidc: discovery claims: %w", cerr) @@ -794,12 +857,13 @@ func (s *Service) getOrLoad(ctx context.Context, providerID string) (*providerEn } entry = &providerEntry{ - cfgRow: cfgRow, - provider: provider, - verifier: verifier, - oauthConfig: oauthConfig, - allowedAlgs: allowed, - plaintext: plaintext, + cfgRow: cfgRow, + provider: provider, + verifier: verifier, + oauthConfig: oauthConfig, + allowedAlgs: allowed, + plaintext: plaintext, + issParamSupported: advertised.AuthorizationResponseIssParamSupported, } s.mu.Lock() diff --git a/internal/auth/oidc/service_test.go b/internal/auth/oidc/service_test.go index 7d3df46..971270e 100644 --- a/internal/auth/oidc/service_test.go +++ b/internal/auth/oidc/service_test.go @@ -67,6 +67,12 @@ type mockIdP struct { // downgrade-attack defense. advertisedAlgs []string + // advertiseIssParameterSupported controls whether the discovery + // doc emits `authorization_response_iss_parameter_supported: true`. + // Audit 2026-05-10 MED-17 — drives the RFC 9207 iss URL parameter + // check in HandleCallback. + advertiseIssParameterSupported bool + // omitUserinfoEndpoint suppresses listing the userinfo endpoint in // the discovery doc. Used to test the "userinfo fallback configured // but provider has no userinfo endpoint" branch in fetchUserinfoGroups. @@ -140,6 +146,12 @@ func newMockIdPWithTB(t testing.TB) *mockIdP { if !idp.omitUserinfoEndpoint { doc["userinfo_endpoint"] = base + "/userinfo" } + // Audit 2026-05-10 MED-17 — only emit the iss-parameter claim + // when explicitly requested so default tests stay back-compat + // with pre-fix behavior. + if idp.advertiseIssParameterSupported { + doc["authorization_response_iss_parameter_supported"] = true + } w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(doc) }) @@ -469,7 +481,7 @@ func TestService_StateReplayDeniedByConsumeOnce(t *testing.T) { // Test 3: forged pre-login cookie returns ErrPreLoginNotFound. func TestService_HandleCallback_RejectsForgedPreLoginCookie(t *testing.T) { svc := newServiceForUnitTest(t) - _, err := svc.HandleCallback(context.Background(), "bogus-cookie", "any-code", "any-state", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), "bogus-cookie", "any-code", "any-state", "", "ip", "ua") if !errors.Is(err, ErrPreLoginNotFound) { t.Errorf("err = %v; want ErrPreLoginNotFound", err) } @@ -479,7 +491,7 @@ func TestService_HandleCallback_RejectsForgedPreLoginCookie(t *testing.T) { func TestService_HandleCallback_RejectsStateMismatch(t *testing.T) { svc, pl := newServiceForUnitTestWithPL(t) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-test", "real-state", "real-nonce", "verifier-xxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "wrong-state", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "wrong-state", "", "ip", "ua") if !errors.Is(err, ErrStateMismatch) { t.Errorf("err = %v; want ErrStateMismatch", err) } @@ -635,7 +647,7 @@ func TestService_HandleCallback_HappyPath(t *testing.T) { t.Fatalf("CreatePreLogin: %v", err) } - res, err := svc.HandleCallback(context.Background(), cookie, "test-code", "happy-state", "10.0.0.1", "Mozilla/5.0") + res, err := svc.HandleCallback(context.Background(), cookie, "test-code", "happy-state", "", "10.0.0.1", "Mozilla/5.0") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -657,7 +669,7 @@ func TestService_HandleCallback_RejectsWrongAudience(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-aud") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-aud", "s", "test-nonce-fixed", "v-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") // gooidc.Verify catches this first; its wrap reaches us as a wrapped error. // Either ErrAudienceMismatch (our re-check) OR a wrapped verify error is acceptable. if err == nil { @@ -673,7 +685,7 @@ func TestService_HandleCallback_RejectsNonceMismatch(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-nonce") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-nonce", "s", "expected-nonce", "v-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrNonceMismatch) { t.Errorf("err = %v; want ErrNonceMismatch", err) } @@ -686,7 +698,7 @@ func TestService_HandleCallback_RejectsExpiredToken(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-exp") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-exp", "s", "test-nonce-fixed", "v-cccccccccccccccccccccccccccccccccccccccccc") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") // Either ErrTokenExpired (our re-check) or a wrapped verify error is fine. if err == nil { t.Errorf("expected non-nil err for expired token") @@ -703,7 +715,7 @@ func TestService_HandleCallback_RejectsIATTooOld(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iat") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-iat", "s", "test-nonce-fixed", "v-dddddddddddddddddddddddddddddddddddddddddd") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrIATTooOld) { t.Errorf("err = %v; want ErrIATTooOld", err) } @@ -716,7 +728,7 @@ func TestService_HandleCallback_RejectsGroupsMissing(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-grp") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-grp", "s", "test-nonce-fixed", "v-eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsMissing) { t.Errorf("err = %v; want ErrGroupsMissing", err) } @@ -729,7 +741,7 @@ func TestService_HandleCallback_RejectsGroupsUnmapped(t *testing.T) { svc, pl := newServiceWithProviderAndPLNoMappings(t, idp.URL(), "op-unmap") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-unmap", "s", "test-nonce-fixed", "v-ffffffffffffffffffffffffffffffffffffffffff") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsUnmapped) { t.Errorf("err = %v; want ErrGroupsUnmapped", err) } @@ -889,7 +901,7 @@ func TestService_UpsertUser_UpdateExistingPath(t *testing.T) { // First login creates the user. cookie1, _, _ := pl.CreatePreLogin(context.Background(), "op-upd", "s1", "test-nonce-fixed", "v-1aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") - res1, err := svc.HandleCallback(context.Background(), cookie1, "code", "s1", "ip", "ua") + res1, err := svc.HandleCallback(context.Background(), cookie1, "code", "s1", "", "ip", "ua") if err != nil { t.Fatalf("first HandleCallback: %v", err) } @@ -903,7 +915,7 @@ func TestService_UpsertUser_UpdateExistingPath(t *testing.T) { // Second login by same subject: update path, no new user row. cookie2, _, _ := pl.CreatePreLogin(context.Background(), "op-upd", "s2", "test-nonce-fixed", "v-2aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") idp.overrideEmail = "user-renamed@example.com" - res2, err := svc.HandleCallback(context.Background(), cookie2, "code2", "s2", "ip", "ua") + res2, err := svc.HandleCallback(context.Background(), cookie2, "code2", "s2", "", "ip", "ua") if err != nil { t.Fatalf("second HandleCallback: %v", err) } @@ -1171,7 +1183,7 @@ func TestService_BootstrapHook_GrantsAdminOnMatch(t *testing.T) { }) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-bootstrap", "s", "test-nonce-fixed", "v-bootstrapxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "10.0.0.1", "Mozilla/5.0") + res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "10.0.0.1", "Mozilla/5.0") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -1194,7 +1206,7 @@ func TestService_BootstrapHook_NoMatchPreservesEmptyMappingFailClosed(t *testing }) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-no-match", "s", "test-nonce-fixed", "v-nomatchxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsUnmapped) { t.Errorf("err = %v; want ErrGroupsUnmapped (no bootstrap match + empty mappings)", err) } @@ -1215,7 +1227,7 @@ func TestService_BootstrapHook_AdminAlreadyExistsFallsThroughToNormalMapping(t * }) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-existing-admin", "s", "test-nonce-fixed", "v-existingxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -1237,7 +1249,7 @@ func TestService_BootstrapHook_ErrorWraps(t *testing.T) { return false, fmt.Errorf("simulated AdminExists probe failure") }) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-hook-err", "s", "test-nonce-fixed", "v-errxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "admin bootstrap") { t.Errorf("err = %v; want admin bootstrap wrap", err) } @@ -1258,7 +1270,7 @@ func TestService_BootstrapHook_IdempotentWhenAdminAlreadyMapped(t *testing.T) { }) cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-idem", "s", "test-nonce-fixed", "v-idempxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -1313,7 +1325,7 @@ func TestService_HandleCallback_AZPRequired_OnMultiAud(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-azp-req") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-azp-req", "s", "test-nonce-fixed", "v-azpreqxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrAZPRequired) { t.Errorf("err = %v; want ErrAZPRequired", err) } @@ -1327,7 +1339,7 @@ func TestService_HandleCallback_AZPMismatch(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-azp-mis") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-azp-mis", "s", "test-nonce-fixed", "v-azpmisxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrAZPMismatch) { t.Errorf("err = %v; want ErrAZPMismatch", err) } @@ -1345,7 +1357,7 @@ func TestService_HandleCallback_ATHashMismatch(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-ath-mis") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ath-mis", "s", "test-nonce-fixed", "v-athmisxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrATHashMismatch) { t.Errorf("err = %v; want ErrATHashMismatch", err) } @@ -1362,7 +1374,7 @@ func TestService_HandleCallback_ATHashRequired_WhenAccessTokenPresent(t *testing svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-ath-req") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ath-req", "s", "test-nonce-fixed", "v-athreqxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrATHashRequired) { t.Errorf("err = %v; want ErrATHashRequired", err) } @@ -1378,7 +1390,7 @@ func TestService_HandleCallback_IATInFuture(t *testing.T) { svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iat-fut") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-iat-fut", "s", "test-nonce-fixed", "v-iatfutxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrIATInFuture) { t.Errorf("err = %v; want ErrIATInFuture", err) } @@ -1396,7 +1408,7 @@ func TestService_HandleCallback_MappingsMapError(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-map-err", "s", "test-nonce-fixed", "v-mapxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "group-role mapping") { t.Errorf("err = %v; want group-role mapping wrap", err) } @@ -1414,7 +1426,7 @@ func TestService_HandleCallback_SessionMintError(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-mint-err", "s", "test-nonce-fixed", "v-mintxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "session mint") { t.Errorf("err = %v; want session mint wrap", err) } @@ -1433,7 +1445,7 @@ func TestService_HandleCallback_UserCreateError(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-uc-err", "s", "test-nonce-fixed", "v-ucxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "upsert user") { t.Errorf("err = %v; want upsert user wrap", err) } @@ -1453,7 +1465,7 @@ func TestService_HandleCallback_GetByOIDCSubjectNonNotFoundError(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-get-err", "s", "test-nonce-fixed", "v-getxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "simulated query failure") { t.Errorf("err = %v; want simulated query failure unwrap", err) } @@ -1474,7 +1486,7 @@ func TestService_UpsertUser_DisplayNameFallsBackToEmail(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-name-fb", "s", "test-nonce-fixed", "v-namxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -1500,7 +1512,7 @@ func TestService_FetchUserinfoGroups_HappyPath_OnEmptyIDTokenGroups(t *testing.T svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ui-ok", "s", "test-nonce-fixed", "v-uioxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + res, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -1525,7 +1537,7 @@ func TestService_FetchUserinfoGroups_ReturnsErrGroupsMissing_WhenUserinfoAlsoEmp svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ui-empty", "s", "test-nonce-fixed", "v-uixxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsMissing) { t.Errorf("err = %v; want ErrGroupsMissing", err) } @@ -1547,7 +1559,7 @@ func TestService_FetchUserinfoGroups_ReturnsErrGroupsMissing_WhenEndpointMissing svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ui-noendpoint", "s", "test-nonce-fixed", "v-uixxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsMissing) { t.Errorf("err = %v; want ErrGroupsMissing", err) } @@ -1691,7 +1703,7 @@ func TestService_HandleCallback_RejectsTokenResponseMissingIDToken(t *testing.T) svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-no-idtok") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-no-idtok", "s", "test-nonce-fixed", "v-noidxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "missing id_token") { t.Errorf("err = %v; want missing id_token error", err) } @@ -1714,7 +1726,7 @@ func TestService_FetchUserinfoGroups_ReturnsErrGroupsMissing_WhenUserinfoFails(t svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-ui-500", "s", "test-nonce-fixed", "v-uifxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if !errors.Is(err, ErrGroupsMissing) { t.Errorf("err = %v; want ErrGroupsMissing", err) } @@ -1760,6 +1772,103 @@ func TestService_AlgPinning_HeaderAlgValueUnterminatedString(t *testing.T) { } } +// ============================================================================= +// MED-17 regression tests — RFC 9207 iss URL parameter check. +// +// HandleCallback REQUIRES the `iss` callback URL parameter when the +// provider's discovery doc advertises +// authorization_response_iss_parameter_supported=true. Pre-fix the +// parameter was ignored; mix-up attacks could route the auth code to +// the wrong relying-party endpoint without detection. +// ============================================================================= + +// TestService_HandleCallback_MED17_NoSupport_AnyIssAccepted pins the +// back-compat case: providers that don't advertise iss-parameter +// support (the majority today) get the same behavior as before — +// callback iss is not required and an arbitrary value is ignored. +func TestService_HandleCallback_MED17_NoSupport_AnyIssAccepted(t *testing.T) { + idp := newMockIdP(t) + // advertiseIssParameterSupported deliberately left false. + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iss-back-compat") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-iss-back-compat", "iss-bc-state", "test-nonce-fixed", "v-issbcxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + + // Pass a callbackIss value the provider didn't advertise support + // for — the service must ignore it. + res, err := svc.HandleCallback(context.Background(), cookie, "code", "iss-bc-state", "https://malicious.example/", "ip", "ua") + if err != nil { + t.Fatalf("HandleCallback (no-support, arbitrary callbackIss): %v; want nil (parameter must be ignored)", err) + } + if res == nil { + t.Fatalf("CallbackResult nil for back-compat happy path") + } +} + +// TestService_HandleCallback_MED17_SupportButMissing rejects with +// ErrIssParamMissing when the provider advertised support but the +// callback URL omitted the iss query parameter. +func TestService_HandleCallback_MED17_SupportButMissing(t *testing.T) { + idp := newMockIdP(t) + idp.advertiseIssParameterSupported = true + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iss-missing") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-iss-missing", "iss-miss-state", "test-nonce-fixed", "v-issmsxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + _, err = svc.HandleCallback(context.Background(), cookie, "code", "iss-miss-state", "", "ip", "ua") + if !errors.Is(err, ErrIssParamMissing) { + t.Fatalf("err = %v; want ErrIssParamMissing", err) + } +} + +// TestService_HandleCallback_MED17_SupportButMismatch rejects with +// ErrIssParamMismatch when the provider advertised support and the +// callback URL supplied an iss query parameter but the value doesn't +// match the matched provider's IssuerURL. This is the load-bearing +// mix-up-attack defense. +func TestService_HandleCallback_MED17_SupportButMismatch(t *testing.T) { + idp := newMockIdP(t) + idp.advertiseIssParameterSupported = true + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iss-mismatch") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-iss-mismatch", "iss-mm-state", "test-nonce-fixed", "v-issmmxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + // Supply an honest-looking but wrong iss — the impersonator's URL + // instead of the matched provider's IssuerURL. + _, err = svc.HandleCallback(context.Background(), cookie, "code", "iss-mm-state", "https://attacker.example/", "ip", "ua") + if !errors.Is(err, ErrIssParamMismatch) { + t.Fatalf("err = %v; want ErrIssParamMismatch", err) + } +} + +// TestService_HandleCallback_MED17_SupportAndCorrect succeeds when the +// callback iss exactly matches the matched provider's IssuerURL — +// the success path that proves the gate isn't over-eager. +func TestService_HandleCallback_MED17_SupportAndCorrect(t *testing.T) { + idp := newMockIdP(t) + idp.advertiseIssParameterSupported = true + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-iss-ok") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-iss-ok", "iss-ok-state", "test-nonce-fixed", "v-issokxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + // The matched provider's IssuerURL is the mockIdP server URL. + res, err := svc.HandleCallback(context.Background(), cookie, "code", "iss-ok-state", idp.URL(), "ip", "ua") + if err != nil { + t.Fatalf("HandleCallback (correct iss): %v", err) + } + if res == nil { + t.Fatalf("CallbackResult nil for happy iss path") + } +} + // TestService_UpsertUser_ValidateErrorOnEmptyEmail pins the // User.Validate failure path. The IdP returns an empty email (missing // claim); the upsertUser display-name fallback resolves to "" too; @@ -1776,7 +1885,7 @@ func TestService_UpsertUser_ValidateErrorOnEmptyEmail(t *testing.T) { svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "") cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-validate-err", "s", "test-nonce-fixed", "v-valxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx") - _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "ip", "ua") + _, err := svc.HandleCallback(context.Background(), cookie, "code", "s", "", "ip", "ua") if err == nil || !strings.Contains(err.Error(), "validate") { t.Errorf("err = %v; want validate wrap", err) }