diff --git a/AUDIT.md b/AUDIT.md index 53e6a7e..b68347c 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -222,7 +222,7 @@ The REST `handleTokenIssue` and gRPC `IssueServiceToken` both revoke the existin |--------|----|----------|-------|--------| | Yes | F-01 | MEDIUM | TOTP enrollment sets required=1 before confirmation | Small | | No | F-02 | MEDIUM | Password in HTML hidden fields during TOTP step | Medium | -| No | F-03 | MEDIUM | Token renewal not atomic (race window) | Small | +| Yes | F-03 | MEDIUM | Token renewal not atomic (race window) | Small | | Yes | F-04 | MEDIUM | Rate limiter not applied to REST login endpoint | Small | | Yes | F-11 | MEDIUM | Missing security headers on UI responses | Small | | No | F-05 | LOW | No `nbf` claim in issued JWTs | Trivial | diff --git a/internal/db/accounts.go b/internal/db/accounts.go index 6beb36d..6bb880f 100644 --- a/internal/db/accounts.go +++ b/internal/db/accounts.go @@ -559,6 +559,56 @@ func (db *DB) RevokeToken(jti, reason string) error { return nil } +// RenewToken atomically revokes the old token and tracks the new one in a +// single SQLite transaction. +// +// Security: the two operations must be atomic so that a failure between them +// cannot leave the user with neither a valid old token nor a new one. With +// MaxOpenConns(1) and SQLite's serialised write path, BEGIN IMMEDIATE acquires +// the write lock immediately and prevents any other writer from interleaving. +func (db *DB) RenewToken(oldJTI, reason, newJTI string, accountID int64, issuedAt, expiresAt time.Time) error { + tx, err := db.sql.Begin() + if err != nil { + return fmt.Errorf("db: renew token begin tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + n := now() + + // Revoke the old token. + result, err := tx.Exec(` + UPDATE token_revocation + SET revoked_at = ?, revoke_reason = ? + WHERE jti = ? AND revoked_at IS NULL + `, n, nullString(reason), oldJTI) + if err != nil { + return fmt.Errorf("db: renew token revoke old %q: %w", oldJTI, err) + } + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("db: renew token revoke rows affected: %w", err) + } + if rows == 0 { + return fmt.Errorf("db: renew token: old token %q not found or already revoked", oldJTI) + } + + // Track the new token. + _, err = tx.Exec(` + INSERT INTO token_revocation (jti, account_id, issued_at, expires_at) + VALUES (?, ?, ?, ?) + `, newJTI, accountID, + issuedAt.UTC().Format(time.RFC3339), + expiresAt.UTC().Format(time.RFC3339)) + if err != nil { + return fmt.Errorf("db: renew token track new %q: %w", newJTI, err) + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("db: renew token commit: %w", err) + } + return nil +} + // RevokeAllUserTokens revokes all non-expired, non-revoked tokens for an account. func (db *DB) RevokeAllUserTokens(accountID int64, reason string) error { n := now() diff --git a/internal/db/db_test.go b/internal/db/db_test.go index ca00d4b..95e19fa 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -326,6 +326,55 @@ func TestPGCredentials(t *testing.T) { } } +func TestRenewTokenAtomic(t *testing.T) { + db := openTestDB(t) + acct, err := db.CreateAccount("judy", model.AccountTypeHuman, "hash") + if err != nil { + t.Fatalf("CreateAccount: %v", err) + } + + now := time.Now().UTC() + exp := now.Add(time.Hour) + + // Set up the old token. + oldJTI := "renew-old-jti" + newJTI := "renew-new-jti" + if err := db.TrackToken(oldJTI, acct.ID, now, exp); err != nil { + t.Fatalf("TrackToken old: %v", err) + } + + // RenewToken should atomically revoke old and track new. + if err := db.RenewToken(oldJTI, "renewed", newJTI, acct.ID, now, exp); err != nil { + t.Fatalf("RenewToken: %v", err) + } + + // Old token must be revoked. + oldRec, err := db.GetTokenRecord(oldJTI) + if err != nil { + t.Fatalf("GetTokenRecord old: %v", err) + } + if !oldRec.IsRevoked() { + t.Error("old token should be revoked after RenewToken") + } + if oldRec.RevokeReason != "renewed" { + t.Errorf("old token revoke reason = %q, want %q", oldRec.RevokeReason, "renewed") + } + + // New token must be tracked and not revoked. + newRec, err := db.GetTokenRecord(newJTI) + if err != nil { + t.Fatalf("GetTokenRecord new: %v", err) + } + if newRec.IsRevoked() { + t.Error("new token should not be revoked") + } + + // RenewToken on an already-revoked old token must fail (atomicity guard). + if err := db.RenewToken(oldJTI, "renewed", "other-jti", acct.ID, now, exp); err == nil { + t.Error("expected error when renewing an already-revoked token") + } +} + func TestRevokeAllUserTokens(t *testing.T) { db := openTestDB(t) acct, err := db.CreateAccount("ivan", model.AccountTypeHuman, "hash") diff --git a/internal/grpcserver/auth.go b/internal/grpcserver/auth.go index 3b08554..ed23042 100644 --- a/internal/grpcserver/auth.go +++ b/internal/grpcserver/auth.go @@ -149,8 +149,9 @@ func (a *authServiceServer) RenewToken(ctx context.Context, _ *mciasv1.RenewToke return nil, status.Error(codes.Internal, "internal error") } - _ = a.s.db.RevokeToken(claims.JTI, "renewed") - if err := a.s.db.TrackToken(newClaims.JTI, acct.ID, newClaims.IssuedAt, newClaims.ExpiresAt); err != nil { + // Security: revoke old + track new in a single transaction (F-03) so that a + // failure between the two steps cannot leave the user with no valid token. + if err := a.s.db.RenewToken(claims.JTI, "renewed", newClaims.JTI, acct.ID, newClaims.IssuedAt, newClaims.ExpiresAt); err != nil { return nil, status.Error(codes.Internal, "internal error") } diff --git a/internal/server/server.go b/internal/server/server.go index e3e8bf3..08d2a90 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -284,12 +284,10 @@ func (s *Server) handleRenew(w http.ResponseWriter, r *http.Request) { return } - // Revoke the old token and track the new one atomically is not possible - // in SQLite without a transaction. We do best-effort: revoke old, track new. - if err := s.db.RevokeToken(claims.JTI, "renewed"); err != nil { - s.logger.Error("revoke old token on renew", "error", err) - } - if err := s.db.TrackToken(newClaims.JTI, acct.ID, newClaims.IssuedAt, newClaims.ExpiresAt); err != nil { + // Security: revoke old + track new in a single transaction (F-03) so that a + // failure between the two steps cannot leave the user with no valid token. + if err := s.db.RenewToken(claims.JTI, "renewed", newClaims.JTI, acct.ID, newClaims.IssuedAt, newClaims.ExpiresAt); err != nil { + s.logger.Error("renew token atomic", "error", err) middleware.WriteError(w, http.StatusInternalServerError, "internal error", "internal_error") return }