Don't backdate a fresh MFA to a stale kc_login cookie (INFD-IN-0005)
completeAuthorization reused an existing kc_login session's IssuedAt as authTime whenever one existed for the same user, even at the two call sites that had just validated an MFA token in this exact request. A platform-root sign-in that stepped up with fresh MFA while an older browser session cookie was still around got that older timestamp stamped onto the new authentication, understating freshness for every downstream binding-grade check (flex-auth's 900s review window denied informed-decision's infd-20260927-b01 approval as a result). Add a freshMFA parameter so only the carried-forward-assurance call site (no MFA challenge in this request) reuses the prior session's IssuedAt; the two call sites following a successful MFA.ValidateMFAToken always stamp authTime to now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Assistant: claude-code Assistant-Model: sonnet Assistant-Process: 169987@bnt-lap001 Assistant-Session: 322ef1ef-9048-4021-8570-b6d6f6347999
This commit is contained in:
parent
8352f4bd37
commit
911e9de969
3 changed files with 134 additions and 8 deletions
|
|
@ -384,12 +384,12 @@ func (h *AuthorizeHandler) ServeHTTPCallback(w http.ResponseWriter, r *http.Requ
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
h.pending.Delete(state)
|
h.pending.Delete(state)
|
||||||
h.completeAuthorization(w, r, ps, result.Username, true)
|
h.completeAuthorization(w, r, ps, result.Username, true, true)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
h.pending.Delete(state)
|
h.pending.Delete(state)
|
||||||
h.completeAuthorization(w, r, ps, result.Username, decision.MFAVerified)
|
h.completeAuthorization(w, r, ps, result.Username, decision.MFAVerified, false)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (h *AuthorizeHandler) decideAssurance(ctx context.Context, ps *PendingState, username string, login *LoginSession) (domain.AssuranceDecision, error) {
|
func (h *AuthorizeHandler) decideAssurance(ctx context.Context, ps *PendingState, username string, login *LoginSession) (domain.AssuranceDecision, error) {
|
||||||
|
|
@ -470,22 +470,30 @@ func (h *AuthorizeHandler) serveMFASubmission(w http.ResponseWriter, r *http.Req
|
||||||
// Reached only after ValidateMFAToken succeeded above -- MFA was
|
// Reached only after ValidateMFAToken succeeded above -- MFA was
|
||||||
// required and passed, unlike the callback path where mfaRequired may
|
// required and passed, unlike the callback path where mfaRequired may
|
||||||
// be false.
|
// be false.
|
||||||
h.completeAuthorization(w, r, ps, ps.AuthenticatedUser, true)
|
h.completeAuthorization(w, r, ps, ps.AuthenticatedUser, true, true)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (h *AuthorizeHandler) completeAuthorization(w http.ResponseWriter, r *http.Request, ps *PendingState, username string, mfaVerified bool) {
|
func (h *AuthorizeHandler) completeAuthorization(w http.ResponseWriter, r *http.Request, ps *PendingState, username string, mfaVerified bool, freshMFA bool) {
|
||||||
level := domain.AssuranceAAL1
|
level := domain.AssuranceAAL1
|
||||||
if mfaVerified {
|
if mfaVerified {
|
||||||
level = domain.AssuranceAAL2
|
level = domain.AssuranceAAL2
|
||||||
}
|
}
|
||||||
|
|
||||||
// When an existing browser session for this same user carried the
|
// When an existing browser session for this same user carried the
|
||||||
// authorization, the authentication happened when that session was
|
// authorization -- i.e. no MFA challenge happened in this exact request
|
||||||
// issued, not now. Creating the new login session below resets IssuedAt,
|
// (freshMFA is false) -- the authentication happened when that session
|
||||||
|
// was issued, not now. But when MFA was just validated in this request
|
||||||
|
// (freshMFA is true, from a callback or submission that itself called
|
||||||
|
// MFA.ValidateMFAToken), that proof is the current instant regardless of
|
||||||
|
// any older session cookie for the same user: reusing the prior time
|
||||||
|
// here understated freshness for every binding-grade check downstream
|
||||||
|
// (INFD-IN-0005). Creating the new login session below resets IssuedAt,
|
||||||
// so the original instant has to be read before that.
|
// so the original instant has to be read before that.
|
||||||
authTime := time.Now()
|
authTime := time.Now()
|
||||||
if prior := h.Logins.fromRequest(r); prior != nil && prior.Username == username && !prior.IssuedAt.IsZero() {
|
if !freshMFA {
|
||||||
authTime = prior.IssuedAt
|
if prior := h.Logins.fromRequest(r); prior != nil && prior.Username == username && !prior.IssuedAt.IsZero() {
|
||||||
|
authTime = prior.IssuedAt
|
||||||
|
}
|
||||||
}
|
}
|
||||||
if login := h.Logins.Create(username, level); login != nil {
|
if login := h.Logins.Create(username, level); login != nil {
|
||||||
writeLoginCookie(w, login, issuerIsHTTPS(h.Issuer))
|
writeLoginCookie(w, login, issuerIsHTTPS(h.Issuer))
|
||||||
|
|
|
||||||
|
|
@ -616,6 +616,61 @@ func TestAuthorizeCallback_MFASubmission_ValidToken_RedirectsWithCode(t *testing
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// INFD-IN-0005: a browser that already carried an older kc_login cookie for
|
||||||
|
// the same user must not have that stale IssuedAt stamped onto a session
|
||||||
|
// that just passed a fresh MFA challenge. Downstream freshness checks (e.g.
|
||||||
|
// flex-auth's 900-second review window) read AuthTime as proof of when MFA
|
||||||
|
// actually happened; silently backdating it made real, just-completed MFA
|
||||||
|
// look stale.
|
||||||
|
func TestAuthorizeCallback_MFASubmission_FreshEvenWithStalePriorLoginCookie(t *testing.T) {
|
||||||
|
mfa := &mockMFAProvider{required: true}
|
||||||
|
emitter := &captureEmitter{}
|
||||||
|
|
||||||
|
sessions := oidc.NewSessionStore()
|
||||||
|
logins := oidc.NewLoginSessionStore()
|
||||||
|
stale := logins.Create("alice", domain.AssuranceAAL2)
|
||||||
|
stale.IssuedAt = time.Now().Add(-4 * time.Hour)
|
||||||
|
|
||||||
|
h := &oidc.AuthorizeHandler{
|
||||||
|
ClientConfig: testClient(),
|
||||||
|
Auth: &mockAuthProvider{},
|
||||||
|
MFA: mfa,
|
||||||
|
Sessions: sessions,
|
||||||
|
Logins: logins,
|
||||||
|
Emitter: emitter,
|
||||||
|
}
|
||||||
|
|
||||||
|
h.PendingStates().Store("random-state", &oidc.PendingState{
|
||||||
|
ClientID: "test-client",
|
||||||
|
RedirectURI: "https://app.example.com/callback",
|
||||||
|
PKCEChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
|
||||||
|
PKCEChallengeMethod: "S256",
|
||||||
|
State: "random-state",
|
||||||
|
Scopes: []string{"openid"},
|
||||||
|
ExpiresAt: time.Now().Add(5 * time.Minute),
|
||||||
|
AuthenticatedUser: "alice",
|
||||||
|
})
|
||||||
|
|
||||||
|
form := url.Values{"state": {"random-state"}, "mfa_token": {"123456"}}
|
||||||
|
req := httptest.NewRequest(http.MethodPost, "/authorize/callback", strings.NewReader(form.Encode()))
|
||||||
|
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||||
|
req.AddCookie(&http.Cookie{Name: "kc_login", Value: stale.ID})
|
||||||
|
w := httptest.NewRecorder()
|
||||||
|
h.ServeHTTPCallback(w, req)
|
||||||
|
|
||||||
|
if w.Code != http.StatusFound {
|
||||||
|
t.Fatalf("expected 302 redirect, got %d (body: %s)", w.Code, w.Body.String())
|
||||||
|
}
|
||||||
|
loc, _ := url.Parse(w.Header().Get("Location"))
|
||||||
|
sess, ok := sessions.Get(loc.Query().Get("code"))
|
||||||
|
if !ok {
|
||||||
|
t.Fatal("expected a PKCE session for the issued code")
|
||||||
|
}
|
||||||
|
if time.Since(sess.AuthTime) > time.Minute {
|
||||||
|
t.Errorf("AuthTime = %v, want close to now: a fresh MFA submission must not inherit the stale kc_login cookie's IssuedAt", sess.AuthTime)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestAuthorizeCallback_MFANotRequired_SessionRecordsMFAVerifiedFalse(t *testing.T) {
|
func TestAuthorizeCallback_MFANotRequired_SessionRecordsMFAVerifiedFalse(t *testing.T) {
|
||||||
auth := &mockAuthProvider{
|
auth := &mockAuthProvider{
|
||||||
callbackResult: &domain.AuthResult{Username: "alice"},
|
callbackResult: &domain.AuthResult{Username: "alice"},
|
||||||
|
|
|
||||||
63
workplans/ADHOC-2026-09-27.md
Normal file
63
workplans/ADHOC-2026-09-27.md
Normal file
|
|
@ -0,0 +1,63 @@
|
||||||
|
---
|
||||||
|
id: ADHOC-2026-09-27
|
||||||
|
type: workplan
|
||||||
|
title: "Fresh MFA no longer backdated to a stale kc_login cookie (INFD-IN-0005)"
|
||||||
|
domain: infotech
|
||||||
|
repo: key-cape
|
||||||
|
status: finished
|
||||||
|
owner: claude
|
||||||
|
created: "2026-09-27"
|
||||||
|
updated: "2026-09-27"
|
||||||
|
---
|
||||||
|
|
||||||
|
## Stop completeAuthorization from reusing a stale prior session's IssuedAt after real MFA
|
||||||
|
|
||||||
|
```task
|
||||||
|
id: ADHOC-2026-09-27-T01
|
||||||
|
status: done
|
||||||
|
priority: high
|
||||||
|
```
|
||||||
|
|
||||||
|
`informed-decision` intake `INFD-IN-0005` (open, `origin_ref: SECRETS-WP-0010`):
|
||||||
|
platform-root's approval of `infd-20260927-b01` was refused by flex-auth's
|
||||||
|
`compact_sitting` policy with `policy_denied`, despite platform-root having just
|
||||||
|
completed a fresh key-cape MFA sign-in. flex-auth's 900-second review-freshness
|
||||||
|
check reads `assurance.at`, which key-cape derives from the PKCE session's
|
||||||
|
`AuthTime`.
|
||||||
|
|
||||||
|
Root cause in `src/internal/server/oidc/authorize.go`,
|
||||||
|
`AuthorizeHandler.completeAuthorization`: whenever a `kc_login` cookie already
|
||||||
|
existed for the same username, its `IssuedAt` was unconditionally reused as
|
||||||
|
`authTime` — even on the two call sites that had just called
|
||||||
|
`MFA.ValidateMFAToken` successfully in this exact request (the callback's
|
||||||
|
`mfaToken != ""` branch and `serveMFASubmission`). A user who had an older
|
||||||
|
browser session and then stepped up with a fresh MFA challenge got the *old*
|
||||||
|
session's timestamp stamped onto the *new*, just-proven authentication —
|
||||||
|
understating freshness for every downstream binding-grade check, exactly as
|
||||||
|
`INFD-IN-0005` describes ("completeAuthorization currently copies a prior
|
||||||
|
timestamp... only after actual reauthentication").
|
||||||
|
|
||||||
|
The reuse itself is correct when MFA was **not** just performed in this
|
||||||
|
request (`decision.MFAVerified` carried forward from an existing AAL2 session,
|
||||||
|
via `domain.DecideAssurance`) — reporting mint time there would overstate
|
||||||
|
freshness (`TestAssuranceReportsAuthenticationTimeNotMintTime`). The two cases
|
||||||
|
were conflated under a single `mfaVerified` bool.
|
||||||
|
|
||||||
|
Fix: added a `freshMFA bool` parameter to `completeAuthorization`, `true` only
|
||||||
|
at the two call sites that just validated an MFA token in this request, `false`
|
||||||
|
at the carried-forward-assurance call site. `authTime` now falls back to the
|
||||||
|
prior session's `IssuedAt` only when `!freshMFA`.
|
||||||
|
|
||||||
|
Added `TestAuthorizeCallback_MFASubmission_FreshEvenWithStalePriorLoginCookie`,
|
||||||
|
which seeds a 4-hour-old `kc_login` cookie for the same user, then submits a
|
||||||
|
valid MFA token and asserts the resulting `AuthTime` is within a minute of now.
|
||||||
|
Confirmed it fails against the pre-fix code (`AuthTime` ~4h old) and passes
|
||||||
|
after. Full suite (`go test ./...`) green, no other test needed adjustment —
|
||||||
|
the carried-forward-assurance case (`TestPolicy_...`, `TestLogout_...`,
|
||||||
|
`assurance_time_test.go`) is unaffected since none of it exercises a fresh MFA
|
||||||
|
submission with a pre-existing cookie for the same user.
|
||||||
|
|
||||||
|
Does not touch flex-auth (its 900s window is correct; it was consuming a bad
|
||||||
|
timestamp) or informed-decision (correctly refuses to interpret the PDP's
|
||||||
|
denial). `INFD-IN-0005` can close once informed-decision confirms platform-root
|
||||||
|
can approve after a fresh sign-in against this build.
|
||||||
Loading…
Add table
Add a link
Reference in a new issue