mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-12 17:48:57 +00:00
harden(oidc): strict UA/IP binding (A-6) — close request-empty bypass in MED-16
The MED-16 closure (2a1a0b3) 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.
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user