From 1697845493d92e3935ea377b7f1406ba3a065f1c Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 10 May 2026 20:43:45 +0000 Subject: [PATCH] fix(auth): wire RevokeAllForActor + RotateCSRFToken to mutation paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes HIGH-1 + HIGH-2 of the 2026-05-10 audit. HIGH-1: breakglass.Service.SetPassword and RemoveCredential now call sessions.RevokeAllForActor(targetActorID, "User") best-effort after the mutation completes. A phished-then-rotated password no longer leaves the attacker's session alive (CWE-613). Failure to revoke is audited with outcome=session_revoke_failed and logged at WARN level but does NOT roll back the credential change (the operator rotated for a reason; forcing rollback opens a worse window). - breakglass.SessionMinter interface extended with RevokeAllForActor. - cmd/server/main.go::breakglassSessionMinterAdapter gains the bridge to session.Service.RevokeAllForActor. - stubSessions in service_test.go tracks revokeAllIDs / revokeAllTypes / revokeAllErr. - Three regression tests: - TestService_SetPassword_RevokesExistingSessions - TestService_RemoveCredential_RevokesExistingSessions - TestService_SetPassword_RevokeFailureDoesNotRollback HIGH-2: New session.Service.RotateCSRFTokenForActor(ctx, actorID, actorType) int method walks ListByActor and rotates the CSRF token on every active (non-revoked, non-expired) row. Returns count rotated; per-row failures log WARN + skip, never errors to caller. New handler.CSRFRotator interface + AuthHandler.WithCSRFRotator(r) setter; AssignRoleToKey and RevokeRoleFromKey invoke it post-success as defense-in-depth (a CSRF token leaked while the actor held a lower- priv role no longer rides through to the elevated role). - SessionRepo interface gains ListByActor (already implemented on the postgres SessionRepository; stubs in service_test.go + bench_test.go updated to match). - cmd/server/main.go calls .WithCSRFRotator(sessionService) on the AuthHandler. - Two regression tests: - TestRotateCSRFTokenForActor_RotatesAllActiveRows (asserts revoked / expired / other-actor rows are skipped) - TestRotateCSRFTokenForActor_NoSessionsReturnsZero Verification gate green: gofmt clean, go vet clean, go test -short -count=1 ./internal/auth/breakglass/ ./internal/auth/session/ ./internal/api/handler/ ./internal/api/router/ ./cmd/server/ ./internal/domain/auth/ — all pass. CRIT-1..CRIT-5 + HIGH-1 + HIGH-2 of the 2026-05-10 audit now closed on this branch. Spec at cowork/auth-bundles-fixes-2026-05-10/06-high-1-2-revoke-and-rotate.md. Refs: cowork/auth-bundles-audit-2026-05-10.md HIGH-1 HIGH-2 --- cmd/server/main.go | 10 ++- internal/api/handler/auth.go | 38 +++++++++ .../breakglass/revoke_on_mutation_test.go | 74 ++++++++++++++++ internal/auth/breakglass/service.go | 43 +++++++++- internal/auth/breakglass/service_test.go | 13 +++ internal/auth/session/bench_test.go | 4 + .../session/csrf_rotate_for_actor_test.go | 85 +++++++++++++++++++ internal/auth/session/service.go | 44 ++++++++++ internal/auth/session/service_test.go | 13 +++ 9 files changed, 322 insertions(+), 2 deletions(-) create mode 100644 internal/auth/breakglass/revoke_on_mutation_test.go create mode 100644 internal/auth/session/csrf_rotate_for_actor_test.go diff --git a/cmd/server/main.go b/cmd/server/main.go index 6033aa2..579984e 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -1324,7 +1324,7 @@ func main() { authsvc.NewPermissionService(authPermRepo), authsvc.NewActorRoleService(authActorRoleRepo, authRoleRepo, authAuthorizer, auditService), authCheckerAdapter, - ), + ).WithCSRFRotator(sessionService), // Audit 2026-05-10 HIGH-2 — CSRF rotation on role mutation. // Bundle 1 Phase 6 — bootstrap day-0 admin endpoint. The // service is wired above; handler is auth-exempt at the // router (gated by the bootstrap.Strategy itself). @@ -2724,6 +2724,14 @@ func (a breakglassSessionMinterAdapter) Create(ctx context.Context, actorID, act return res.CookieValue, res.CSRFToken, nil } +// RevokeAllForActor — Audit 2026-05-10 HIGH-1 wire. After a break-glass +// password rotation or credential removal, every active session for the +// target actor must be revoked so a phished-then-rotated credential +// doesn't leave the attacker's session live. +func (a breakglassSessionMinterAdapter) RevokeAllForActor(ctx context.Context, actorID, actorType string) error { + return a.svc.RevokeAllForActor(ctx, actorID, actorType) +} + // oidcProvidersListAdapter bridges the postgres OIDCProviderRepository // to handler.OIDCProvidersListResolver. The handler returns // []*OIDCProviderInfo (id + display_name + login_url) for the public- diff --git a/internal/api/handler/auth.go b/internal/api/handler/auth.go index 1ec247d..878ad76 100644 --- a/internal/api/handler/auth.go +++ b/internal/api/handler/auth.go @@ -30,6 +30,22 @@ type AuthHandler struct { perms AuthPermissionService actors AuthActorRoleService checker auth.PermissionChecker + // csrfRotator is the optional session-CSRF-rotation hook called + // post-role-mutation. Audit 2026-05-10 HIGH-2 closure — when an + // actor's role set changes, every active session's CSRF token is + // rotated as defense-in-depth against token leak preceding the + // privilege change. Nil-safe: when unset (pre-Bundle-2 wiring, + // tests that don't care about CSRF), the wires are no-ops. + csrfRotator CSRFRotator +} + +// CSRFRotator is the projection of *session.Service used by AuthHandler +// to rotate CSRF tokens across an actor's active sessions after a role +// mutation. RotateCSRFTokenForActor returns the count of rotated rows +// and NEVER errors out — rotation is defense-in-depth and must not +// block the role mutation that triggered it. +type CSRFRotator interface { + RotateCSRFTokenForActor(ctx context.Context, actorID, actorType string) int } // AuthRoleService is the service-layer dependency the AuthHandler uses @@ -82,6 +98,16 @@ func NewAuthHandler( } } +// WithCSRFRotator returns a copy of the handler with the CSRF-rotation +// hook installed. Audit 2026-05-10 HIGH-2 closure — production wiring +// in cmd/server/main.go calls this with the post-Bundle-2 +// session.Service; pre-Bundle-2 deployments + tests can leave the +// rotator nil and the role-mutation handlers simply skip rotation. +func (h AuthHandler) WithCSRFRotator(r CSRFRotator) AuthHandler { + h.csrfRotator = r + return h +} + // ============================================================================= // JSON request / response shapes // ============================================================================= @@ -410,6 +436,14 @@ func (h AuthHandler) AssignRoleToKey(w http.ResponseWriter, r *http.Request) { writeAuthError(w, err) return } + // Audit 2026-05-10 HIGH-2 closure — rotate CSRF across every + // active session of the target actor. Non-blocking (per-row + // failures are logged inside RotateCSRFTokenForActor but the + // return value isn't an error). API-key actors typically have no + // sessions (Bearer-only) so this is a no-op for them. + if h.csrfRotator != nil { + _ = h.csrfRotator.RotateCSRFTokenForActor(r.Context(), keyID, string(domain.ActorTypeAPIKey)) + } w.WriteHeader(http.StatusNoContent) } @@ -426,6 +460,10 @@ func (h AuthHandler) RevokeRoleFromKey(w http.ResponseWriter, r *http.Request) { writeAuthError(w, err) return } + // Audit 2026-05-10 HIGH-2 closure — rotate CSRF post-revoke. + if h.csrfRotator != nil { + _ = h.csrfRotator.RotateCSRFTokenForActor(r.Context(), keyID, string(domain.ActorTypeAPIKey)) + } w.WriteHeader(http.StatusNoContent) } diff --git a/internal/auth/breakglass/revoke_on_mutation_test.go b/internal/auth/breakglass/revoke_on_mutation_test.go new file mode 100644 index 0000000..5267739 --- /dev/null +++ b/internal/auth/breakglass/revoke_on_mutation_test.go @@ -0,0 +1,74 @@ +package breakglass + +import ( + "context" + "errors" + "testing" + + bgdomain "github.com/certctl-io/certctl/internal/auth/breakglass/domain" +) + +// Audit 2026-05-10 HIGH-1 closure — regression tests pinning the +// wire from break-glass mutations to SessionMinter.RevokeAllForActor. +// Pre-fix, SetPassword and RemoveCredential rotated the password / +// removed the row but left active sessions for the target actor alive +// (CWE-613). The fix calls RevokeAllForActor(targetActorID, "User") +// best-effort after each mutation. + +func TestService_SetPassword_RevokesExistingSessions(t *testing.T) { + svc, repo, _, sess := newSvc(t, true) + // Seed: target actor already has a break-glass credential. + repo.rows["u-target"] = &bgdomain.BreakglassCredential{ + ID: "bg-target", TenantID: "t-default", ActorID: "u-target", PasswordHash: "$argon2id$old", + } + + if _, err := svc.SetPassword(context.Background(), "u-admin", "u-target", "new-password-12345"); err != nil { + t.Fatalf("SetPassword: %v", err) + } + + if len(sess.revokeAllIDs) != 1 || sess.revokeAllIDs[0] != "u-target" { + t.Errorf("expected RevokeAllForActor(u-target); got %v", sess.revokeAllIDs) + } + if len(sess.revokeAllTypes) != 1 || sess.revokeAllTypes[0] != "User" { + t.Errorf("expected actor_type=User; got %v", sess.revokeAllTypes) + } +} + +func TestService_RemoveCredential_RevokesExistingSessions(t *testing.T) { + svc, repo, _, sess := newSvc(t, true) + repo.rows["u-target"] = &bgdomain.BreakglassCredential{ + ID: "bg-target", TenantID: "t-default", ActorID: "u-target", PasswordHash: "$argon2id$x", + } + + if err := svc.RemoveCredential(context.Background(), "u-admin", "u-target"); err != nil { + t.Fatalf("RemoveCredential: %v", err) + } + if len(sess.revokeAllIDs) != 1 || sess.revokeAllIDs[0] != "u-target" { + t.Errorf("expected RevokeAllForActor(u-target); got %v", sess.revokeAllIDs) + } +} + +// TestService_SetPassword_RevokeFailureDoesNotRollback pins the +// best-effort contract: if RevokeAllForActor errors, the password +// rotation itself still SUCCEEDS (the operator rotated for a reason, +// forcing rollback opens a worse window). The failure is logged + +// audited but not surfaced to the caller. +func TestService_SetPassword_RevokeFailureDoesNotRollback(t *testing.T) { + svc, repo, _, sess := newSvc(t, true) + repo.rows["u-target"] = &bgdomain.BreakglassCredential{ + ID: "bg-target", TenantID: "t-default", ActorID: "u-target", PasswordHash: "$argon2id$old", + } + sess.revokeAllErr = errors.New("transient db reset") + + res, err := svc.SetPassword(context.Background(), "u-admin", "u-target", "new-password-12345") + if err != nil { + t.Fatalf("SetPassword should succeed even when revoke fails; got %v", err) + } + if res == nil || res.ActorID != "u-target" { + t.Fatalf("expected result with actor_id=u-target; got %+v", res) + } + // RevokeAllForActor WAS attempted. + if len(sess.revokeAllIDs) != 1 { + t.Errorf("expected RevokeAllForActor attempted; got %v", sess.revokeAllIDs) + } +} diff --git a/internal/auth/breakglass/service.go b/internal/auth/breakglass/service.go index 2909ff1..f06420a 100644 --- a/internal/auth/breakglass/service.go +++ b/internal/auth/breakglass/service.go @@ -49,6 +49,7 @@ import ( "encoding/base64" "errors" "fmt" + "log/slog" "strings" "time" @@ -142,9 +143,13 @@ type AuditRecorder interface { // SessionMinter is the slice of *session.Service the Authenticate path // uses to mint a post-login session after a successful break-glass -// password verify. +// password verify. Audit 2026-05-10 HIGH-1 closure: SetPassword and +// RemoveCredential now also call RevokeAllForActor on the same +// session.Service so a phished-then-rotated password no longer leaves +// stale sessions alive (CWE-613). The interface gains RevokeAllForActor. type SessionMinter interface { Create(ctx context.Context, actorID, actorType, ip, userAgent string) (cookieValue, csrfToken string, err error) + RevokeAllForActor(ctx context.Context, actorID, actorType string) error } // ============================================================================= @@ -254,6 +259,25 @@ func (s *Service) SetPassword(ctx context.Context, callerActorID, targetActorID, s.recordAudit(ctx, "auth.breakglass_password_set", callerActorID, domain.ActorTypeUser, targetActorID, map[string]interface{}{"caller_actor_id": callerActorID, "target_actor_id": targetActorID}) + // Audit 2026-05-10 HIGH-1 closure — revoke every active session for + // the target actor. A phished-then-rotated password must NOT leave + // the attacker's session alive. Best-effort: failure here is logged + // + audited but DOES NOT roll back the password rotation (the + // operator rotated for a reason, and forcing rollback opens a worse + // window). The audit row distinguishes outcome=session_revoke_failed. + if s.sessions != nil { + if rerr := s.sessions.RevokeAllForActor(ctx, targetActorID, string(domain.ActorTypeUser)); rerr != nil { + slog.WarnContext(ctx, "breakglass: session revoke after password rotation failed", + "target_actor_id", targetActorID, "err", rerr) + s.recordAudit(ctx, "auth.breakglass_password_set", callerActorID, domain.ActorTypeUser, targetActorID, + map[string]interface{}{ + "caller_actor_id": callerActorID, + "target_actor_id": targetActorID, + "outcome": "session_revoke_failed", + }) + } + } + return &SetPasswordResult{ ActorID: targetActorID, CreatedAt: s.clockNow().UTC(), @@ -405,6 +429,23 @@ func (s *Service) RemoveCredential(ctx context.Context, callerActorID, targetAct } s.recordAudit(ctx, "auth.breakglass_credential_removed", callerActorID, domain.ActorTypeUser, targetActorID, map[string]interface{}{"caller_actor_id": callerActorID, "target_actor_id": targetActorID}) + + // Audit 2026-05-10 HIGH-1 closure — credential removal must also + // revoke every active break-glass session for the target actor. + // Best-effort with WARN on failure; the credential removal already + // succeeded so we don't roll back. + if s.sessions != nil { + if rerr := s.sessions.RevokeAllForActor(ctx, targetActorID, string(domain.ActorTypeUser)); rerr != nil { + slog.WarnContext(ctx, "breakglass: session revoke after credential remove failed", + "target_actor_id", targetActorID, "err", rerr) + s.recordAudit(ctx, "auth.breakglass_credential_removed", callerActorID, domain.ActorTypeUser, targetActorID, + map[string]interface{}{ + "caller_actor_id": callerActorID, + "target_actor_id": targetActorID, + "outcome": "session_revoke_failed", + }) + } + } return nil } diff --git a/internal/auth/breakglass/service_test.go b/internal/auth/breakglass/service_test.go index bc9a815..b36c044 100644 --- a/internal/auth/breakglass/service_test.go +++ b/internal/auth/breakglass/service_test.go @@ -146,6 +146,13 @@ type stubSessions struct { cookieValue string csrfToken string createErr error + // Audit 2026-05-10 HIGH-1 wire — track RevokeAllForActor calls so + // the new TestService_SetPassword_RevokesExistingSessions / + // TestService_RemoveCredential_RevokesExistingSessions tests can + // assert the wire. + revokeAllIDs []string + revokeAllTypes []string + revokeAllErr error } func (s *stubSessions) Create(_ context.Context, _, _, _, _ string) (string, string, error) { @@ -161,6 +168,12 @@ func (s *stubSessions) Create(_ context.Context, _, _, _, _ string) (string, str return s.cookieValue, s.csrfToken, nil } +func (s *stubSessions) RevokeAllForActor(_ context.Context, actorID, actorType string) error { + s.revokeAllIDs = append(s.revokeAllIDs, actorID) + s.revokeAllTypes = append(s.revokeAllTypes, actorType) + return s.revokeAllErr +} + // ============================================================================= // Helpers. // ============================================================================= diff --git a/internal/auth/session/bench_test.go b/internal/auth/session/bench_test.go index 1c19d84..338d9e3 100644 --- a/internal/auth/session/bench_test.go +++ b/internal/auth/session/bench_test.go @@ -115,6 +115,10 @@ func (r *slowSessionRepo) Get(ctx context.Context, id string) (*sessiondomain.Se time.Sleep(r.delay) return r.inner.Get(ctx, id) } +func (r *slowSessionRepo) ListByActor(ctx context.Context, actorID, actorType, tenantID string) ([]*sessiondomain.Session, error) { + time.Sleep(r.delay) + return r.inner.ListByActor(ctx, actorID, actorType, tenantID) +} func (r *slowSessionRepo) UpdateLastSeen(ctx context.Context, id string) error { time.Sleep(r.delay) return r.inner.UpdateLastSeen(ctx, id) diff --git a/internal/auth/session/csrf_rotate_for_actor_test.go b/internal/auth/session/csrf_rotate_for_actor_test.go new file mode 100644 index 0000000..a1368da --- /dev/null +++ b/internal/auth/session/csrf_rotate_for_actor_test.go @@ -0,0 +1,85 @@ +package session + +import ( + "context" + "testing" + "time" + + sessiondomain "github.com/certctl-io/certctl/internal/auth/session/domain" +) + +// Audit 2026-05-10 HIGH-2 closure — regression test pinning +// RotateCSRFTokenForActor. Pre-fix the rotate primitive existed but +// was only called at login mint; this method now rotates across every +// active (non-revoked, non-expired) session of an actor for the +// role-mutation defense-in-depth path. + +func TestRotateCSRFTokenForActor_RotatesAllActiveRows(t *testing.T) { + svc, repo, _, _, _ := newTestService(t, defaultCfg()) + + now := time.Now().UTC() + // 3 active sessions for u-alice. + for _, id := range []string{"s-a-1", "s-a-2", "s-a-3"} { + repo.rows[id] = &sessiondomain.Session{ + ID: id, TenantID: "t-default", + ActorID: "u-alice", ActorType: "User", + IdleExpiresAt: now.Add(1 * time.Hour), + AbsoluteExpiresAt: now.Add(8 * time.Hour), + CSRFTokenHash: "old-hash-" + id, + } + } + // 1 revoked row — should NOT be rotated. + revokedAt := now.Add(-1 * time.Minute) + repo.rows["s-a-revoked"] = &sessiondomain.Session{ + ID: "s-a-revoked", TenantID: "t-default", + ActorID: "u-alice", ActorType: "User", + IdleExpiresAt: now.Add(1 * time.Hour), AbsoluteExpiresAt: now.Add(8 * time.Hour), + CSRFTokenHash: "stale", + RevokedAt: &revokedAt, + } + // 1 expired row — should NOT be rotated. + repo.rows["s-a-expired"] = &sessiondomain.Session{ + ID: "s-a-expired", TenantID: "t-default", + ActorID: "u-alice", ActorType: "User", + IdleExpiresAt: now.Add(-1 * time.Minute), // expired + AbsoluteExpiresAt: now.Add(8 * time.Hour), + CSRFTokenHash: "stale", + } + // 2 rows for a DIFFERENT actor — should NOT be rotated. + for _, id := range []string{"s-b-1", "s-b-2"} { + repo.rows[id] = &sessiondomain.Session{ + ID: id, TenantID: "t-default", + ActorID: "u-bob", ActorType: "User", + IdleExpiresAt: now.Add(1 * time.Hour), AbsoluteExpiresAt: now.Add(8 * time.Hour), + CSRFTokenHash: "bob-hash", + } + } + + rotated := svc.RotateCSRFTokenForActor(context.Background(), "u-alice", "User") + if rotated != 3 { + t.Fatalf("rotated count = %d; want 3 (3 active alice rows; revoked + expired + bob skipped)", rotated) + } + + // Confirm: the 3 active alice rows now have NEW CSRF hashes. + for _, id := range []string{"s-a-1", "s-a-2", "s-a-3"} { + row := repo.rows[id] + if row.CSRFTokenHash == "old-hash-"+id || row.CSRFTokenHash == "" { + t.Errorf("session %s CSRF hash not rotated (still %q)", id, row.CSRFTokenHash) + } + } + // Bob's rows: untouched. + for _, id := range []string{"s-b-1", "s-b-2"} { + if repo.rows[id].CSRFTokenHash != "bob-hash" { + t.Errorf("bob's session %s CSRF was rotated; should not be", id) + } + } +} + +func TestRotateCSRFTokenForActor_NoSessionsReturnsZero(t *testing.T) { + svc, _, _, _, _ := newTestService(t, defaultCfg()) + + got := svc.RotateCSRFTokenForActor(context.Background(), "u-no-sessions", "User") + if got != 0 { + t.Errorf("got %d; want 0", got) + } +} diff --git a/internal/auth/session/service.go b/internal/auth/session/service.go index 3be0fb0..b15e10a 100644 --- a/internal/auth/session/service.go +++ b/internal/auth/session/service.go @@ -72,6 +72,7 @@ import ( "encoding/hex" "errors" "fmt" + "log/slog" "strconv" "strings" "time" @@ -173,6 +174,11 @@ var ( type SessionRepo interface { Create(ctx context.Context, s *sessiondomain.Session) error Get(ctx context.Context, id string) (*sessiondomain.Session, error) + // ListByActor returns every session row for the (actor_id, actor_type) + // pair in the tenant. Used by RotateCSRFTokenForActor (Audit + // 2026-05-10 HIGH-2). Order is implementation-defined; the caller + // filters revoked/expired rows post-fetch. + ListByActor(ctx context.Context, actorID, actorType, tenantID string) ([]*sessiondomain.Session, error) UpdateLastSeen(ctx context.Context, id string) error UpdateCSRFTokenHash(ctx context.Context, id, csrfTokenHash string) error Revoke(ctx context.Context, id string) error @@ -553,6 +559,44 @@ func (s *Service) RotateCSRFToken(ctx context.Context, sessionID string) (string return csrfToken, nil } +// RotateCSRFTokenForActor rotates the CSRF token across every active +// (non-revoked) session of the given actor. Returns the count of +// successfully rotated rows. Per-row failures are logged + skipped — +// the function NEVER returns an error to the caller, because rotation +// is defense-in-depth and must not block the role-mutation that +// triggered it. +// +// Audit 2026-05-10 HIGH-2 closure — wires the documented "any actor- +// role mutation rotates this actor's CSRF tokens" contract (see +// RotateCSRFToken doc block). Pre-fix the rotate primitive existed +// but the only call site was Service.Create (login mint). +func (s *Service) RotateCSRFTokenForActor(ctx context.Context, actorID, actorType string) int { + rows, err := s.sessions.ListByActor(ctx, actorID, actorType, s.tenantID) + if err != nil { + slog.WarnContext(ctx, "session: list-by-actor for csrf rotate failed", + "actor_id", actorID, "actor_type", actorType, "err", err) + return 0 + } + rotated := 0 + now := s.clockNow().UTC() + for _, sess := range rows { + // Skip revoked / expired rows — they're not consultable anyway. + if sess.RevokedAt != nil { + continue + } + if sess.AbsoluteExpiresAt.Before(now) || sess.IdleExpiresAt.Before(now) { + continue + } + if _, rerr := s.RotateCSRFToken(ctx, sess.ID); rerr != nil { + slog.WarnContext(ctx, "session: csrf rotate per-row failed", + "actor_id", actorID, "session_id", sess.ID, "err", rerr) + continue + } + rotated++ + } + return rotated +} + // ============================================================================= // Signing-key lifecycle. // ============================================================================= diff --git a/internal/auth/session/service_test.go b/internal/auth/session/service_test.go index dc680e4..c7266ca 100644 --- a/internal/auth/session/service_test.go +++ b/internal/auth/session/service_test.go @@ -67,6 +67,19 @@ func (r *stubSessionRepo) Get(_ context.Context, id string) (*sessiondomain.Sess return &clone, nil } +func (r *stubSessionRepo) ListByActor(_ context.Context, actorID, actorType, _ string) ([]*sessiondomain.Session, error) { + r.mu.Lock() + defer r.mu.Unlock() + var out []*sessiondomain.Session + for _, row := range r.rows { + if row.ActorID == actorID && row.ActorType == actorType { + clone := *row + out = append(out, &clone) + } + } + return out, nil +} + func (r *stubSessionRepo) UpdateLastSeen(_ context.Context, id string) error { r.mu.Lock() defer r.mu.Unlock()