gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-12 18:09:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(auth): wire RevokeAllForActor + RotateCSRFToken to mutation paths

Browse Source 
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
This commit is contained in:
shankar0123
2026-05-10 20:43:45 +00:00
parent 4dcbb3ed87
commit 4d11984645
9 changed files with 322 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/auth/breakglass/revoke_on_mutation_test.go
+74
View File
@@ -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)
}
}
internal/auth/breakglass/service.go
+42 -1
View File
@@ -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
}
internal/auth/breakglass/service_test.go
+13
View File
@@ -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.
// =============================================================================