diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e986ea..098adbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,27 @@ ### Security +- **Strict pre-login UA/IP binding (Audit 2026-05-11 A-6).** + The MED-16 closure left a request-side empty-header bypass: when the + pre-login row carried a User-Agent or client-IP binding but the + `/auth/oidc/callback` request omitted the corresponding value, the + binding check was silently skipped. `curl` doesn't send User-Agent + by default; many programmatic clients omit it. An attacker who + acquired a pre-login cookie could replay it without the bound + header and bypass the RFC 9700 §4.7.1 defense. The check is now + strict-when-stored — an empty request-side value with a non-empty + stored binding rejects with HTTP 400 and the new audit failure + categories `prelogin_ua_missing` / `prelogin_ip_missing` (distinct + from the existing `*_mismatch` categories so SIEM rules can alert + specifically on bypass attempts). **Operator advisory:** environments + where the User-Agent is stripped in transit (some debug proxies, a + handful of CDN configurations) must set + `CERTCTL_OIDC_PRELOGIN_REQUIRE_UA=false` to keep logins working; + symmetric `CERTCTL_OIDC_PRELOGIN_REQUIRE_IP=false` exists for the + IP-side. The legacy-row compat window — pre-migration rows with no + stored binding — still passes through unchecked, but that window is + bounded by the 10-minute pre-login TTL. + - **Pre-login cookie Path widened from `/auth/oidc/` to `/` (Audit MED-14 follow-on).** Required to satisfy the `__Host-` prefix's `Path=/` rule. The cookie lifetime is unchanged (10 minutes) and only the callback handler diff --git a/internal/api/handler/auth_session_oidc.go b/internal/api/handler/auth_session_oidc.go index 3faf223..ea680e2 100644 --- a/internal/api/handler/auth_session_oidc.go +++ b/internal/api/handler/auth_session_oidc.go @@ -1006,11 +1006,11 @@ func (h *AuthSessionOIDCHandler) TestProvider(w http.ResponseWriter, r *http.Req } h.recordAudit(r.Context(), "auth.oidc_provider_tested", caller.ActorID, caller.ActorType, "", map[string]interface{}{ - "issuer_url": req.IssuerURL, - "discovery_succeeded": res.DiscoverySucceeded, - "jwks_reachable": res.JWKSReachable, - "iss_param_supported": res.IssParamSupported, - "error_count": len(res.Errors), + "issuer_url": req.IssuerURL, + "discovery_succeeded": res.DiscoverySucceeded, + "jwks_reachable": res.JWKSReachable, + "iss_param_supported": res.IssParamSupported, + "error_count": len(res.Errors), }) writeJSON(w, http.StatusOK, res) } @@ -1267,6 +1267,14 @@ func classifyOIDCFailure(err error) string { return "prelogin_ua_mismatch" case errors.Is(err, oidcsvc.ErrPreLoginIPMismatch): return "prelogin_ip_mismatch" + // Audit 2026-05-11 A-6 — strict-when-stored. Distinguishes the + // new "request omitted the bound header" reject path from the + // existing "header was supplied but didn't match" path so SIEM + // rules can alert specifically on attempted bypasses. + case errors.Is(err, oidcsvc.ErrPreLoginUAMissing): + return "prelogin_ua_missing" + case errors.Is(err, oidcsvc.ErrPreLoginIPMissing): + return "prelogin_ip_missing" } msg := strings.ToLower(err.Error()) switch { diff --git a/internal/api/handler/auth_session_oidc_test.go b/internal/api/handler/auth_session_oidc_test.go index 55fe134..58cb16e 100644 --- a/internal/api/handler/auth_session_oidc_test.go +++ b/internal/api/handler/auth_session_oidc_test.go @@ -1217,6 +1217,17 @@ func TestClassifyOIDCFailure(t *testing.T) { // 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"}, + // Audit 2026-05-11 A-6 — strict-when-stored. Distinguishes the + // new request-omitted-binding reject path from the existing + // mismatch leg. Wrapped variants must round-trip through + // errors.Is so the audit category remains stable even when + // the service layer adds context wrapping. + {oidcsvc.ErrPreLoginUAMismatch, "prelogin_ua_mismatch"}, + {oidcsvc.ErrPreLoginIPMismatch, "prelogin_ip_mismatch"}, + {oidcsvc.ErrPreLoginUAMissing, "prelogin_ua_missing"}, + {oidcsvc.ErrPreLoginIPMissing, "prelogin_ip_missing"}, + {fmt.Errorf("upstream: %w", oidcsvc.ErrPreLoginUAMissing), "prelogin_ua_missing"}, + {fmt.Errorf("upstream: %w", oidcsvc.ErrPreLoginIPMissing), "prelogin_ip_missing"}, {errors.New("some other error"), "unspecified"}, } for _, tc := range cases { diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index c5531db..cc2fff3 100644 --- a/internal/auth/oidc/service.go +++ b/internal/auth/oidc/service.go @@ -240,6 +240,26 @@ var ( // CERTCTL_OIDC_PRELOGIN_REQUIRE_IP=false to disable. ErrPreLoginIPMismatch = errors.New("oidc: pre-login row client IP does not match callback request") + // ErrPreLoginUAMissing: the pre-login row carries a User-Agent + // binding (MED-16) but the /auth/oidc/callback request omitted + // the User-Agent header. Audit 2026-05-11 A-6 closure — the + // original MED-16 logic short-circuited the compare when the + // request side was empty, which let an attacker bypass the + // binding by sending a callback with no User-Agent (trivial: + // curl, many programmatic clients omit the header by default). + // Distinguished from ErrPreLoginUAMismatch so the audit row + // can tell a binding violation apart from a missing-header + // bypass attempt. HTTP 400. Operators on enterprise proxies + // that strip the User-Agent header in transit can disable the + // check with CERTCTL_OIDC_PRELOGIN_REQUIRE_UA=false. + ErrPreLoginUAMissing = errors.New("oidc: pre-login row has User-Agent binding but callback omitted User-Agent header") + + // ErrPreLoginIPMissing: symmetric to ErrPreLoginUAMissing for + // the source IP binding. Reachable when XFF-trust gating zeros + // the resolved client IP for a request whose pre-login row + // captured one. Audit 2026-05-11 A-6 closure. + ErrPreLoginIPMissing = errors.New("oidc: pre-login row has client-IP binding but callback request had no resolvable client IP") + // ErrAudienceMismatch: ID token `aud` doesn't include the // configured client_id. HTTP 400. ErrAudienceMismatch = errors.New("oidc: audience mismatch") @@ -508,12 +528,33 @@ func (s *Service) HandleCallback( // 000044 have NULL → empty string), and (c) the incoming request // carries a non-empty value too. Constant-time compares for both // legs to avoid leaking UA/IP length differences via timing. - if s.preLoginRequireUA && storedUA != "" && userAgent != "" { + // Audit 2026-05-11 A-6 — strict-when-stored. The original MED-16 + // closure short-circuited the compare when the request side was + // empty (`userAgent != ""` / `ip != ""`), which was an + // attacker-controllable bypass: an attacker forging a callback + // request can simply omit the User-Agent header (curl does this by + // default; many programmatic HTTP clients omit it) and the binding + // check skips silently. Now: when the pre-login row carries a + // binding, the request MUST present a matching value; an empty + // request value with a non-empty stored value rejects with + // ErrPreLoginUA{IP}Missing (distinct from the mismatch leg so the + // audit row can tell them apart). Legacy-row compat — pre-migration + // rows with `storedUA == ""` / `storedIP == ""` — still passes + // unchecked; that window is bounded by the 10-minute pre-login TTL, + // so within 10 minutes of the MED-16 deploy the strict path is + // universal. + if s.preLoginRequireUA && storedUA != "" { + if userAgent == "" { + return nil, ErrPreLoginUAMissing + } if subtle.ConstantTimeCompare([]byte(userAgent), []byte(storedUA)) != 1 { return nil, ErrPreLoginUAMismatch } } - if s.preLoginRequireIP && storedIP != "" && ip != "" { + if s.preLoginRequireIP && storedIP != "" { + if ip == "" { + return nil, ErrPreLoginIPMissing + } if subtle.ConstantTimeCompare([]byte(ip), []byte(storedIP)) != 1 { return nil, ErrPreLoginIPMismatch } diff --git a/internal/auth/oidc/service_test.go b/internal/auth/oidc/service_test.go index ab77c74..e8ba90f 100644 --- a/internal/auth/oidc/service_test.go +++ b/internal/auth/oidc/service_test.go @@ -2063,6 +2063,132 @@ func TestService_HandleCallback_MED16_RequireUAFalse_AllowsMismatch(t *testing.T } } +// ============================================================================= +// Audit 2026-05-11 A-6 — strict-when-stored. The MED-16 closure short- +// circuited the UA/IP compare when the request-side value was empty, +// which was an attacker-controllable bypass (omit User-Agent → check +// skipped). The strict-when-stored fix rejects request-empty when the +// pre-login row carries a binding, distinguishing the new reject path +// from the existing mismatch leg via dedicated sentinels: +// ErrPreLoginUAMissing + ErrPreLoginIPMissing. +// ============================================================================= + +// TestService_HandleCallback_MED16_A6_UAStoredButRequestEmpty_Rejects +// pins the load-bearing bypass-closure leg. Pre-login row has a stored +// User-Agent; the callback request omits the User-Agent header. Pre-A-6 +// this passed silently (the `userAgent != ""` short-circuit). Post-A-6 +// it rejects with ErrPreLoginUAMissing. +func TestService_HandleCallback_MED16_A6_UAStoredButRequestEmpty_Rejects(t *testing.T) { + idp := newMockIdP(t) + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-a6-ua-empty") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-a6-ua-empty", "a6-ua-state", "test-nonce-fixed", + "verifier-a6uaemptyxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "10.0.0.1", "MozillaLogin/1.0") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + // Empty userAgent on the consume-side mirrors an attacker forging + // a callback request without a User-Agent header (curl default). + _, err = svc.HandleCallback(context.Background(), cookie, "code", "a6-ua-state", "", "10.0.0.1", "") + if !errors.Is(err, ErrPreLoginUAMissing) { + t.Fatalf("err = %v; want ErrPreLoginUAMissing (the A-6 bypass closure)", err) + } +} + +// TestService_HandleCallback_MED16_A6_IPStoredButRequestEmpty_Rejects +// is symmetric for source IP. Reachable when XFF-trust gating zeros the +// resolved IP for a request whose pre-login row captured one. +func TestService_HandleCallback_MED16_A6_IPStoredButRequestEmpty_Rejects(t *testing.T) { + idp := newMockIdP(t) + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-a6-ip-empty") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-a6-ip-empty", "a6-ip-state", "test-nonce-fixed", + "verifier-a6ipemptyxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "10.0.0.1", "Mozilla/5.0") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + _, err = svc.HandleCallback(context.Background(), cookie, "code", "a6-ip-state", "", "", "Mozilla/5.0") + if !errors.Is(err, ErrPreLoginIPMissing) { + t.Fatalf("err = %v; want ErrPreLoginIPMissing", err) + } +} + +// TestService_HandleCallback_MED16_A6_LegacyRowEmptyStoredStillPasses +// pins the legacy-row compat: pre-migration rows (storedUA / storedIP +// both empty) still pass through unchecked, irrespective of what the +// callback request supplies. Within 10 minutes of the MED-16 deploy +// every legacy row expires; afterwards the strict path is universal. +func TestService_HandleCallback_MED16_A6_LegacyRowEmptyStoredStillPasses(t *testing.T) { + idp := newMockIdP(t) + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-a6-legacy") + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-a6-legacy", "a6-leg-state", "test-nonce-fixed", + "verifier-a6legacyxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "", "") // legacy: pre-migration row has no binding + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + // Request supplies a UA + IP — these are NOT compared because the + // stored row has nothing to compare against. + res, err := svc.HandleCallback(context.Background(), cookie, "code", "a6-leg-state", "", "10.0.0.1", "Mozilla/5.0") + if err != nil { + t.Fatalf("HandleCallback (legacy empty stored): %v", err) + } + if res == nil { + t.Fatal("CallbackResult nil on legacy-row compat path") + } +} + +// TestService_HandleCallback_MED16_A6_ToggleOff_AllowsBypass pins +// the operator escape hatch. With CERTCTL_OIDC_PRELOGIN_REQUIRE_UA=false, +// even an A-6-bypass attempt (stored UA, empty request UA) passes +// silently. The persistence side still captures the binding so +// retroactive audit forensics remain possible. +func TestService_HandleCallback_MED16_A6_ToggleOff_AllowsBypass(t *testing.T) { + idp := newMockIdP(t) + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-a6-toggle-ua") + svc.SetPreLoginBindingRequirements(false, true) // UA off, IP on + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-a6-toggle-ua", "a6-tog-state", "test-nonce-fixed", + "verifier-a6togglexxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "10.0.0.1", "Mozilla/5.0") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + // UA gate disabled → empty request UA passes despite stored UA. + res, err := svc.HandleCallback(context.Background(), cookie, "code", "a6-tog-state", "", "10.0.0.1", "") + if err != nil { + t.Fatalf("HandleCallback (UA toggle off, empty request UA): %v", err) + } + if res == nil { + t.Fatal("CallbackResult nil with UA toggle off") + } +} + +// TestService_HandleCallback_MED16_A6_ToggleOff_IP_AllowsBypass is +// the symmetric IP-side escape-hatch pin. +func TestService_HandleCallback_MED16_A6_ToggleOff_IP_AllowsBypass(t *testing.T) { + idp := newMockIdP(t) + svc, pl := newServiceWithProviderAndPL(t, idp.URL(), "op-a6-toggle-ip") + svc.SetPreLoginBindingRequirements(true, false) // UA on, IP off + + cookie, _, err := pl.CreatePreLogin(context.Background(), "op-a6-toggle-ip", "a6-togip-state", "test-nonce-fixed", + "verifier-a6togipxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + "10.0.0.1", "Mozilla/5.0") + if err != nil { + t.Fatalf("CreatePreLogin: %v", err) + } + res, err := svc.HandleCallback(context.Background(), cookie, "code", "a6-togip-state", "", "", "Mozilla/5.0") + if err != nil { + t.Fatalf("HandleCallback (IP toggle off, empty request IP): %v", err) + } + if res == nil { + t.Fatal("CallbackResult nil with IP toggle off") + } +} + // 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;