harden(oidc): strict UA/IP binding (A-6) — close request-empty bypass in MED-16

The MED-16 closure (cb73547) added the RFC 9700 §4.7.1 pre-login
UA/IP binding but the consume-side compare at
internal/auth/oidc/service.go was gated by:

  if s.preLoginRequireUA && storedUA != "" && userAgent != "" {
      ... constant-time compare ...
  }
  if s.preLoginRequireIP && storedIP != "" && ip != "" {
      ... constant-time compare ...
  }

The `userAgent != ""` and `ip != ""` arms were intended as
rolling-deploy / headless-proxy compat ("if the request didn't supply
a value, don't try to compare against nothing"). They achieve that —
and they ALSO short-circuit the compare whenever the **attacker**
controls the request side, which is always at /auth/oidc/callback.

Threat model:
  1. Attacker acquires a pre-login cookie (HMAC-protected; requires
     RNG break OR transit leak — not implausible, that's why the
     binding exists in the first place).
  2. Attacker replays the cookie at /auth/oidc/callback from their
     own user-agent.
  3. Attacker OMITS the User-Agent header. curl doesn't send one by
     default. Many programmatic HTTP clients omit it.

Pre-A-6, step 3 trivially bypassed the binding check. The whole
RFC 9700 §4.7.1 defense was theatre against the realistic threat —
silent-allow when the attacker abandons the header they don't want
checked.

Fix: flipped to strict-when-stored. When the pre-login row carries a
binding value (storedUA != "" or storedIP != ""), the request MUST
present a matching value. An empty request side with a non-empty
stored side now rejects with two new sentinels:

  ErrPreLoginUAMissing  — request omitted User-Agent header
  ErrPreLoginIPMissing  — request had no resolvable client IP

Distinguished from the existing *Mismatch sentinels so the audit
row can tell apart "binding violation" (operator mis-configured the
proxy) from "missing-header bypass attempt" (active exploit indicator).
The handler-side classifyOIDCFailure adds typed errors.Is dispatch:

  ErrPreLoginUAMissing → "prelogin_ua_missing"
  ErrPreLoginIPMissing → "prelogin_ip_missing"

SIEM rules can now alert specifically on the bypass-attempt category
distinctly from operator config drift.

Legacy-row compat preserved: pre-migration rows where storedUA == ""
/ storedIP == "" still pass through unchecked. That window is
bounded by the 10-minute pre-login TTL — within 10 minutes of the
MED-16 deploy every legacy row has expired and the strict path is
universal.

Operator escape hatches preserved: CERTCTL_OIDC_PRELOGIN_REQUIRE_UA=false
(symmetric for IP) bypasses both the *Mismatch AND the new *Missing
reject paths. Required for environments where a proxy strips the
User-Agent header in transit (rare but documented in the operator
advisory).

Regression coverage:

  service_test.go (5 new tests under
  `Audit 2026-05-11 A-6 — strict-when-stored` block):
    TestService_HandleCallback_MED16_A6_UAStoredButRequestEmpty_Rejects
      — the load-bearing bypass-closure leg
    TestService_HandleCallback_MED16_A6_IPStoredButRequestEmpty_Rejects
      — symmetric for IP
    TestService_HandleCallback_MED16_A6_LegacyRowEmptyStoredStillPasses
      — legacy-row compat preserved
    TestService_HandleCallback_MED16_A6_ToggleOff_AllowsBypass
      — UA toggle off allows the bypass (operator escape hatch)
    TestService_HandleCallback_MED16_A6_ToggleOff_IP_AllowsBypass
      — IP toggle off allows the bypass

  auth_session_oidc_test.go::TestClassifyOIDCFailure extended:
    ErrPreLoginUAMismatch → prelogin_ua_mismatch (new explicit pin)
    ErrPreLoginIPMismatch → prelogin_ip_mismatch (new explicit pin)
    ErrPreLoginUAMissing → prelogin_ua_missing
    ErrPreLoginIPMissing → prelogin_ip_missing
    fmt.Errorf wrapped variants of the *Missing sentinels round-trip
    through errors.Is (defense against future context-wrapping in
    the service layer)

Verify gate green: gofmt clean, go vet clean, all 10 MED-16 tests
+ extended TestClassifyOIDCFailure pass; full short-mode test run
across internal/auth/oidc + internal/api/handler also green.

Spec at cowork/auth-bundles-fixes-2026-05-11/06-high-prelogin-ua-strict-mode.md.
Audit doc: MED-16 row in cowork/auth-bundles-audit-2026-05-10.md
appended with the A-6 follow-up closure annotation; status table
row updated to "CLOSED + A-6 follow-up CLOSED 2026-05-11".
Operator advisory in CHANGELOG.md v2.1.0 release notes covers the
two operator-visible behaviour changes: (1) callback requests
without User-Agent now reject when a binding was stored, and (2)
the CERTCTL_OIDC_PRELOGIN_REQUIRE_UA=false escape hatch is the
documented path for environments where the proxy strips the header.

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

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 {

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 {

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
 		}

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;