From 911e9de969eeecc66be5bffe1827acac47be45c8 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sun, 27 Sep 2026 19:03:06 +0200 Subject: [PATCH] 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 Assistant: claude-code Assistant-Model: sonnet Assistant-Process: 169987@bnt-lap001 Assistant-Session: 322ef1ef-9048-4021-8570-b6d6f6347999 --- src/internal/server/oidc/authorize.go | 24 ++++++--- src/internal/server/oidc/authorize_test.go | 55 +++++++++++++++++++ workplans/ADHOC-2026-09-27.md | 63 ++++++++++++++++++++++ 3 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 workplans/ADHOC-2026-09-27.md diff --git a/src/internal/server/oidc/authorize.go b/src/internal/server/oidc/authorize.go index 44c70ed..69e5112 100644 --- a/src/internal/server/oidc/authorize.go +++ b/src/internal/server/oidc/authorize.go @@ -384,12 +384,12 @@ func (h *AuthorizeHandler) ServeHTTPCallback(w http.ResponseWriter, r *http.Requ return } h.pending.Delete(state) - h.completeAuthorization(w, r, ps, result.Username, true) + h.completeAuthorization(w, r, ps, result.Username, true, true) return } 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) { @@ -470,22 +470,30 @@ func (h *AuthorizeHandler) serveMFASubmission(w http.ResponseWriter, r *http.Req // Reached only after ValidateMFAToken succeeded above -- MFA was // required and passed, unlike the callback path where mfaRequired may // 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 if mfaVerified { level = domain.AssuranceAAL2 } // When an existing browser session for this same user carried the - // authorization, the authentication happened when that session was - // issued, not now. Creating the new login session below resets IssuedAt, + // authorization -- i.e. no MFA challenge happened in this exact request + // (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. authTime := time.Now() - if prior := h.Logins.fromRequest(r); prior != nil && prior.Username == username && !prior.IssuedAt.IsZero() { - authTime = prior.IssuedAt + if !freshMFA { + 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 { writeLoginCookie(w, login, issuerIsHTTPS(h.Issuer)) diff --git a/src/internal/server/oidc/authorize_test.go b/src/internal/server/oidc/authorize_test.go index 0031da6..ade18a6 100644 --- a/src/internal/server/oidc/authorize_test.go +++ b/src/internal/server/oidc/authorize_test.go @@ -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) { auth := &mockAuthProvider{ callbackResult: &domain.AuthResult{Username: "alice"}, diff --git a/workplans/ADHOC-2026-09-27.md b/workplans/ADHOC-2026-09-27.md new file mode 100644 index 0000000..6b56361 --- /dev/null +++ b/workplans/ADHOC-2026-09-27.md @@ -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.