gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-07 17:02:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge Fix 06 (HIGH A-6): strict UA/IP binding — close request-empty bypass in MED-16

Browse Source 
# Conflicts:
#	CHANGELOG.md
#	internal/api/handler/auth_session_oidc.go
#	internal/api/handler/auth_session_oidc_test.go
This commit is contained in:
shankar0123
2026-05-11 11:19:04 +00:00
parent 8ed8a94242 e0ac659d5e
commit e6ae81f478
5 changed files with 209 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+21
View File
@@ -123,6 +123,27 @@
inputs render an explicit decode-error fallback — silent failure on
the payload preview is what produced this bug in the first place.
- **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
+8
View File
@@ -1275,6 +1275,14 @@ func classifyOIDCFailure(err error) string {
// detail subject to change.
case errors.Is(err, oidcsvc.ErrUserDeactivated):
return "user_deactivated"
// 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
+11
View File
@@ -1222,6 +1222,17 @@ func TestClassifyOIDCFailure(t *testing.T) {
// round-trip).
{oidcsvc.ErrUserDeactivated, "user_deactivated"},
{fmt.Errorf("upstream: %w", oidcsvc.ErrUserDeactivated), "user_deactivated"},
// 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
+43 -2
View File
@@ -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")
@@ -523,12 +543,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
+126
View File
@@ -2229,6 +2229,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;