diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 4f7c5a62..806dd844 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -143,4 +143,8 @@ var ( ErrAPIKeyExpiresAtRequired = errors.New("expires_at is required") ErrAPIKeyExpiresAtTooFar = errors.New("expires_at exceeds the maximum API key lifetime") ErrAPIKeyExpiresAtInPast = errors.New("expires_at must be in the future") + + // ErrAPIKeyPasswordRotated: the owner's password changed after the key was + // minted (issue #402). + ErrAPIKeyPasswordRotated = errors.New("API key predates the owner's last password change") ) diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index 90bd0052..b34726c7 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -123,6 +123,9 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password strin CreatedAt: now, LastUsedAt: nil, IsActive: true, + // The version of the row whose password was just verified, so a + // rotation that commits before this insert still invalidates the key. + PasswordVersion: user.PasswordVersion, } // Store the API key @@ -396,6 +399,9 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA if err != nil { return nil, nil, err } + if key.PasswordVersion != user.PasswordVersion { + return nil, nil, ErrAPIKeyPasswordRotated + } return key, user, nil } diff --git a/internal/auth/service_apikeys_rotation_db_test.go b/internal/auth/service_apikeys_rotation_db_test.go new file mode 100644 index 00000000..41bf47d1 --- /dev/null +++ b/internal/auth/service_apikeys_rotation_db_test.go @@ -0,0 +1,104 @@ +//go:build integration + +package auth + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + rotationPassword = "SyntheticPassword123!" + rotationNewPassword = "RotatedPassword456#" +) + +// failingKeyListStore makes the best-effort revocation scan after a password +// change fail, as a transient database error would. +type failingKeyListStore struct{ *PostgresStore } + +func (failingKeyListStore) ListAPIKeysByUser(context.Context, string) ([]*UserAPIKey, error) { + return nil, errors.New("synthetic list failure") +} + +// userReadHook runs onRead once, right after the first GetUserByID returns. +type userReadHook struct { + *PostgresStore + onRead func() +} + +func (s *userReadHook) GetUserByID(ctx context.Context, id string) (*User, error) { + user, err := s.PostgresStore.GetUserByID(ctx, id) + if hook := s.onRead; hook != nil { + s.onRead = nil + hook() + } + return user, err +} + +func newRotationService(store StoreInterface) *Service { + svc := NewService(ServiceConfig{Store: store}) + svc.bcryptCostOverride = 4 + return svc +} + +func createRotationUser(t *testing.T, svc *Service, store *PostgresStore, email string) *User { + t.Helper() + hash, err := svc.hashPassword(rotationPassword) + require.NoError(t, err) + user := &User{Email: email, PasswordHash: hash, Active: true, GroupIDs: []string{DefaultPurchaserGroupID}} + require.NoError(t, store.CreateUser(t.Context(), user)) + return user +} + +func TestIntegration_APIKeyRejectedAfterPasswordRotation(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + svc := newRotationService(store) + ctx := t.Context() + expiry := time.Now().Add(time.Hour) + perms := []Permission{{Action: ActionView, Resource: ResourceRecommendations}} + change := ChangePasswordRequest{CurrentPassword: rotationPassword, NewPassword: rotationNewPassword} + + t.Run("revocation-scan-fails", func(t *testing.T) { + user := createRotationUser(t, svc, store, "rotation-scan-fails@example.com") + apiKey, _, err := svc.CreateAPIKey(ctx, user.ID, "ci", rotationPassword, perms, &expiry) + require.NoError(t, err) + require.NoError(t, newRotationService(failingKeyListStore{store}).ChangePassword(ctx, user.ID, change)) + + _, _, err = svc.ValidateUserAPIKey(ctx, apiKey) + require.ErrorIs(t, err, ErrAPIKeyPasswordRotated) + }) + + t.Run("key-minted-across-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "rotation-mint-race@example.com") + racing := &userReadHook{PostgresStore: store, onRead: func() { + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + }} + apiKey, _, err := newRotationService(racing).CreateAPIKey(ctx, user.ID, "ci", rotationPassword, perms, &expiry) + require.NoError(t, err, "the mint verified the password it read before the rotation") + + _, _, err = svc.ValidateUserAPIKey(ctx, apiKey) + require.ErrorIs(t, err, ErrAPIKeyPasswordRotated) + }) + + t.Run("key-minted-after-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "rotation-fresh-key@example.com") + before, _, err := svc.CreateAPIKey(ctx, user.ID, "old", rotationPassword, perms, &expiry) + require.NoError(t, err) + require.NoError(t, svc.UpdateUserProfile(ctx, user.ID, "rotation-fresh-key-renamed@example.com", rotationPassword, "")) + _, _, err = svc.ValidateUserAPIKey(ctx, before) + require.NoError(t, err, "a write that keeps the password must not invalidate keys") + + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + after, _, err := svc.CreateAPIKey(ctx, user.ID, "new", rotationNewPassword, perms, &expiry) + require.NoError(t, err) + key, owner, err := svc.ValidateUserAPIKey(ctx, after) + require.NoError(t, err) + assert.Equal(t, user.ID, owner.ID) + assert.Equal(t, "new", key.Name) + }) +} diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go index 097b875a..9a6c16db 100644 --- a/internal/auth/service_apikeys_test.go +++ b/internal/auth/service_apikeys_test.go @@ -720,6 +720,27 @@ func TestService_ValidateUserAPIKey(t *testing.T) { mockStore.AssertExpectations(t) }) + t.Run("fail when owner's password changed after minting", func(t *testing.T) { + mockStore := new(MockStore) + service := &Service{store: mockStore} + + apiKey := "test-api-key-123456" + hash := sha256.Sum256([]byte(apiKey)) + keyHash := base64.RawURLEncoding.EncodeToString(hash[:]) + + mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(&UserAPIKey{ + ID: "key-1", UserID: "user-123", KeyHash: keyHash, IsActive: true, PasswordVersion: 2, + }, nil) + mockStore.On("GetUserByID", ctx, "user-123").Return(&User{ID: "user-123", Active: true, PasswordVersion: 3}, nil) + + resultKey, resultUser, err := service.ValidateUserAPIKey(ctx, apiKey) + + require.ErrorIs(t, err, ErrAPIKeyPasswordRotated) + assert.Nil(t, resultUser) + assert.Nil(t, resultKey) + mockStore.AssertExpectations(t) + }) + t.Run("fail when API key not found", func(t *testing.T) { mockStore := new(MockStore) service := &Service{store: mockStore} diff --git a/internal/auth/service_credentials_db_test.go b/internal/auth/service_credentials_db_test.go index ae5d495a..86070835 100644 --- a/internal/auth/service_credentials_db_test.go +++ b/internal/auth/service_credentials_db_test.go @@ -168,6 +168,7 @@ func TestIntegration_CredentialWritesPreserveConcurrentMFA(t *testing.T) { assert.Equal(t, []string{f.user.PasswordHash}, stored.PasswordHistory) assertOnlyChanged(t, enrolled(), stored, func(w *User) { w.Email, w.PasswordHash, w.Salt, w.PasswordHistory = "profile-both-new@example.com", stored.PasswordHash, "", stored.PasswordHistory + w.PasswordVersion++ }) _, err := f.svc.ValidateSession(ctx, f.session) require.Error(t, err, "a password change must revoke existing sessions") @@ -185,6 +186,7 @@ func TestIntegration_CredentialWritesPreserveConcurrentMFA(t *testing.T) { assert.Equal(t, []string{f.user.PasswordHash}, stored.PasswordHistory) assertOnlyChanged(t, enrolled(), stored, func(w *User) { w.PasswordHash, w.Salt, w.PasswordHistory = stored.PasswordHash, "", stored.PasswordHistory + w.PasswordVersion++ }) _, err := f.svc.ValidateSession(ctx, f.session) require.Error(t, err, "a password change must revoke existing sessions") diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index b2202b10..052ac813 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -48,7 +48,7 @@ func (s *PostgresStore) GetUserByID(ctx context.Context, userID string) (*User, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, - created_at, updated_at, last_login_at, deactivated_at + created_at, updated_at, last_login_at, deactivated_at, password_version FROM users WHERE id = $1 ` @@ -63,7 +63,7 @@ func (s *PostgresStore) GetUserByEmail(ctx context.Context, email string) (*User mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, - created_at, updated_at, last_login_at, deactivated_at + created_at, updated_at, last_login_at, deactivated_at, password_version FROM users WHERE email = $1 ` @@ -277,7 +277,7 @@ func (s *PostgresStore) ListUsers(ctx context.Context) ([]User, error) { mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, - created_at, updated_at, last_login_at, deactivated_at + created_at, updated_at, last_login_at, deactivated_at, password_version FROM users ORDER BY created_at DESC LIMIT 10000 @@ -316,7 +316,7 @@ func (s *PostgresStore) GetUserByResetToken(ctx context.Context, token string) ( mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, - created_at, updated_at, last_login_at, deactivated_at + created_at, updated_at, last_login_at, deactivated_at, password_version FROM users WHERE password_reset_token = $1 ` @@ -777,6 +777,7 @@ func (s *PostgresStore) scanUser(scanner Scanner) (*User, error) { &user.UpdatedAt, &lastLoginAt, &deactivatedAt, + &user.PasswordVersion, ) if err != nil { diff --git a/internal/auth/store_postgres_apikeys.go b/internal/auth/store_postgres_apikeys.go index 43ec0a9d..0ee96903 100644 --- a/internal/auth/store_postgres_apikeys.go +++ b/internal/auth/store_postgres_apikeys.go @@ -39,8 +39,8 @@ func (s *PostgresStore) CreateAPIKey(ctx context.Context, key *UserAPIKey) error query := ` INSERT INTO api_keys ( id, user_id, name, key_prefix, key_hash, permissions, - is_active, expires_at, created_at, last_used_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) + is_active, expires_at, created_at, last_used_at, password_version + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) ` _, err = s.db.Exec(ctx, query, @@ -54,6 +54,7 @@ func (s *PostgresStore) CreateAPIKey(ctx context.Context, key *UserAPIKey) error key.ExpiresAt, key.CreatedAt, key.LastUsedAt, + key.PasswordVersion, ) if err != nil { @@ -68,7 +69,8 @@ func (s *PostgresStore) GetAPIKeyByID(ctx context.Context, keyID string) (*UserA query := ` SELECT id, user_id, name, key_prefix, key_hash, permissions, is_active, expires_at, created_at, last_used_at, - request_count_total, request_count_window, request_count_window_start + request_count_total, request_count_window, request_count_window_start, + password_version FROM api_keys WHERE id = $1 ` @@ -81,7 +83,8 @@ func (s *PostgresStore) GetAPIKeyByHash(ctx context.Context, keyHash string) (*U query := ` SELECT id, user_id, name, key_prefix, key_hash, permissions, is_active, expires_at, created_at, last_used_at, - request_count_total, request_count_window, request_count_window_start + request_count_total, request_count_window, request_count_window_start, + password_version FROM api_keys WHERE key_hash = $1 AND is_active = true AND (expires_at IS NULL OR expires_at > NOW()) @@ -95,7 +98,8 @@ func (s *PostgresStore) ListAPIKeysByUser(ctx context.Context, userID string) ([ query := ` SELECT id, user_id, name, key_prefix, key_hash, permissions, is_active, expires_at, created_at, last_used_at, - request_count_total, request_count_window, request_count_window_start + request_count_total, request_count_window, request_count_window_start, + password_version FROM api_keys WHERE user_id = $1 ORDER BY created_at DESC @@ -274,6 +278,7 @@ func (s *PostgresStore) scanAPIKey(scanner Scanner) (*UserAPIKey, error) { &key.RequestCountTotal, &key.RequestCountWindow, &windowStart, + &key.PasswordVersion, ) if err != nil { diff --git a/internal/auth/store_postgres_pgxmock_test.go b/internal/auth/store_postgres_pgxmock_test.go index 7db8d8f4..d7b7aff8 100644 --- a/internal/auth/store_postgres_pgxmock_test.go +++ b/internal/auth/store_postgres_pgxmock_test.go @@ -319,7 +319,7 @@ var userColumns = []string{ "mfa_enabled", "mfa_secret", "mfa_pending_secret", "mfa_pending_secret_expires_at", "mfa_recovery_codes", "password_reset_token", "password_reset_expiry", "failed_login_attempts", "locked_until", "password_history", - "created_at", "updated_at", "last_login_at", "deactivated_at", + "created_at", "updated_at", "last_login_at", "deactivated_at", "password_version", } func TestPGXMock_ListUsers_Success(t *testing.T) { @@ -337,7 +337,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { true, "mfa-secret", nil, nil, []string{"code1"}, nil, nil, 0, nil, []string{"old-hash"}, - created, created, lastLogin, nil, + created, created, lastLogin, nil, int64(4), ). AddRow( "user-2", "user@example.com", "hash2", "salt2", @@ -345,7 +345,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { false, nil, nil, nil, []string{}, nil, nil, 2, nil, []string{}, - created, created, nil, deactivated, + created, created, nil, deactivated, int64(0), ) mock.ExpectQuery(`(?s)SELECT id, email, password_hash, salt, group_ids, active,.*FROM users\s+ORDER BY created_at DESC\s+LIMIT 10000`). @@ -364,6 +364,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { assert.Equal(t, lastLogin, *users[0].LastLoginAt) assert.Nil(t, users[0].DeactivatedAt) + assert.Equal(t, int64(4), users[0].PasswordVersion) assert.Equal(t, "user-2", users[1].ID) assert.False(t, users[1].Active) @@ -412,7 +413,7 @@ func TestPGXMock_ListUsers_RowError(t *testing.T) { false, nil, nil, nil, []string{}, nil, nil, 0, nil, []string{}, - created, created, nil, nil, + created, created, nil, nil, int64(0), ). RowError(0, errors.New("connection reset mid-iteration")) @@ -429,6 +430,7 @@ var apiKeyColumns = []string{ "id", "user_id", "name", "key_prefix", "key_hash", "permissions", "is_active", "expires_at", "created_at", "last_used_at", "request_count_total", "request_count_window", "request_count_window_start", + "password_version", } func TestPGXMock_ListAPIKeysByUser_Success(t *testing.T) { @@ -444,12 +446,12 @@ func TestPGXMock_ListAPIKeysByUser_Success(t *testing.T) { AddRow( "key-1", "user-1", "ci key", "cudly_ab", "hash-1", []byte(`[{"action":"view","resource":"recommendations"}]`), true, expires, created, lastUsed, - int64(42), int64(3), windowStart, + int64(42), int64(3), windowStart, int64(4), ). AddRow( "key-2", "user-1", "old key", "cudly_cd", "hash-2", []byte(`[]`), false, nil, created, nil, - int64(0), int64(0), nil, + int64(0), int64(0), nil, int64(0), ) mock.ExpectQuery(`(?s)SELECT id, user_id, name, key_prefix, key_hash, permissions,.*FROM api_keys\s+WHERE user_id = \$1\s+ORDER BY created_at DESC`). @@ -470,6 +472,7 @@ func TestPGXMock_ListAPIKeysByUser_Success(t *testing.T) { assert.Equal(t, lastUsed, *keys[0].LastUsedAt) assert.Equal(t, int64(42), keys[0].RequestCountTotal) assert.Equal(t, int64(3), keys[0].RequestCountWindow) + assert.Equal(t, int64(4), keys[0].PasswordVersion) require.NotNil(t, keys[0].RequestCountWindowStart) assert.Equal(t, windowStart, *keys[0].RequestCountWindowStart) diff --git a/internal/auth/types.go b/internal/auth/types.go index f3cd809d..35956457 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -30,6 +30,9 @@ type User struct { FailedLoginAttempts int `json:"-" dynamodbav:"FailedLoginAttempts,omitempty"` Active bool `json:"active" dynamodbav:"Active"` MFAEnabled bool `json:"mfa_enabled" dynamodbav:"MFAEnabled"` + // PasswordVersion is bumped by a database trigger on every password_hash + // change; API keys minted under an older version are rejected (#402). + PasswordVersion int64 `json:"-" dynamodbav:"PasswordVersion"` } // Group represents a permission group. @@ -85,6 +88,8 @@ type UserAPIKey struct { KeyHash string `json:"-" dynamodbav:"KeyHash"` Permissions []Permission `json:"permissions,omitempty" dynamodbav:"Permissions"` IsActive bool `json:"is_active" dynamodbav:"IsActive"` + // PasswordVersion is the owner's User.PasswordVersion read when the key was minted. + PasswordVersion int64 `json:"-" dynamodbav:"PasswordVersion"` // Usage counters (issue #340/#344 deferred sub-task -- migration 000094). // Both default to 0 for legacy rows. RequestCountWindow is a // FIXED/TUMBLING window count, not a true trailing-24h rolling count: diff --git a/internal/database/postgres/migrations/000103_api_key_password_version.down.sql b/internal/database/postgres/migrations/000103_api_key_password_version.down.sql new file mode 100644 index 00000000..da09ddd3 --- /dev/null +++ b/internal/database/postgres/migrations/000103_api_key_password_version.down.sql @@ -0,0 +1,5 @@ +-- 000103 down: drop the password-version binding of API keys. +DROP TRIGGER IF EXISTS users_bump_password_version ON users; +DROP FUNCTION IF EXISTS bump_user_password_version(); +ALTER TABLE api_keys DROP COLUMN IF EXISTS password_version; +ALTER TABLE users DROP COLUMN IF EXISTS password_version; diff --git a/internal/database/postgres/migrations/000103_api_key_password_version.up.sql b/internal/database/postgres/migrations/000103_api_key_password_version.up.sql new file mode 100644 index 00000000..8132e365 --- /dev/null +++ b/internal/database/postgres/migrations/000103_api_key_password_version.up.sql @@ -0,0 +1,24 @@ +-- Migration 000103: bind API keys to the owner's password (issue #402). +-- +-- users.password_version is bumped by trigger whenever password_hash changes, +-- whichever code path writes it. A key records the version of the user row +-- its minting read, and ValidateUserAPIKey rejects it once the two differ, so +-- a rotation invalidates every earlier key even if the revocation scan fails +-- or a key is minted concurrently with the rotation. Existing keys start at +-- version 0, like their owners, and stop working at the owner's next rotation. +ALTER TABLE users ADD COLUMN password_version BIGINT NOT NULL DEFAULT 0; +ALTER TABLE api_keys ADD COLUMN password_version BIGINT NOT NULL DEFAULT 0; + +CREATE OR REPLACE FUNCTION bump_user_password_version() +RETURNS TRIGGER AS $$ +BEGIN + NEW.password_version = OLD.password_version + 1; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +CREATE TRIGGER users_bump_password_version + BEFORE UPDATE OF password_hash ON users + FOR EACH ROW + WHEN (OLD.password_hash IS DISTINCT FROM NEW.password_hash) + EXECUTE FUNCTION bump_user_password_version(); diff --git a/internal/database/postgres/migrations/000103_api_key_password_version_test.go b/internal/database/postgres/migrations/000103_api_key_password_version_test.go new file mode 100644 index 00000000..9b1e4b39 --- /dev/null +++ b/internal/database/postgres/migrations/000103_api_key_password_version_test.go @@ -0,0 +1,42 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +func TestMigration_APIKeyPasswordVersion(t *testing.T) { + ctx := context.Background() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, container.Cleanup(ctx)) }) + pool := container.DB.Pool() + path := getMigrationsPath() + require.NoError(t, migrations.RunMigrations(ctx, pool, path, "", "")) + + var id string + var version int64 + require.NoError(t, pool.QueryRow(ctx, `INSERT INTO users (email, password_hash, salt, group_ids) + VALUES ('password-version@example.com', 'hash-1', '', ARRAY[$1::uuid]) RETURNING id, password_version`, + adminGroupIDForMinAdminTest).Scan(&id, &version)) + require.Zero(t, version) + require.NoError(t, pool.QueryRow(ctx, `UPDATE users SET email = 'renamed@example.com', password_hash = 'hash-1' + WHERE id = $1 RETURNING password_version`, id).Scan(&version)) + require.Zero(t, version, "rewriting the same hash must not bump the version") + require.NoError(t, pool.QueryRow(ctx, `UPDATE users SET password_hash = 'hash-2', password_version = 0 + WHERE id = $1 RETURNING password_version`, id).Scan(&version)) + require.Equal(t, int64(1), version, "the trigger owns the version, whatever the writer sets") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 103))) + var columns int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE column_name = 'password_version' AND table_name IN ('users', 'api_keys')`).Scan(&columns)) + require.Zero(t, columns) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 103)) +}