From 2fb3653783d062f62cb61a14626c457b01e30fbc Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 09:31:22 +0100 Subject: [PATCH 1/2] feat(otp): report which time step a TOTP code belongs to (#849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerifyTOTP answers yes or no, so a caller cannot tell a fresh code from one it already accepted. MatchTOTPStep returns the matching step within the same ±1 tolerance, comparing in constant time, so the caller can refuse a replay. Signed-off-by: alex-dembele --- backend/pkg/otp/match_step_test.go | 63 ++++++++++++++++++++++++++++++ backend/pkg/otp/totp.go | 34 ++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 backend/pkg/otp/match_step_test.go diff --git a/backend/pkg/otp/match_step_test.go b/backend/pkg/otp/match_step_test.go new file mode 100644 index 00000000..189a7d52 --- /dev/null +++ b/backend/pkg/otp/match_step_test.go @@ -0,0 +1,63 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package otp + +import ( + "testing" + "time" + + "github.com/pquerna/otp/totp" +) + +// #849 — MatchTOTPStep names the step a code belongs to, within ±1 step. +func TestMatchTOTPStep(t *testing.T) { + secret, err := GenerateTOTPSecret() + if err != nil { + t.Fatal(err) + } + now := time.Unix(1_900_000_015, 0) // mid-step, so ±1 is unambiguous + current := now.Unix() / 30 + codeAt := func(step int64) string { + c, err := totp.GenerateCode(secret, time.Unix(step*30, 0)) + if err != nil { + t.Fatal(err) + } + return c + } + + for name, tc := range map[string]struct { + step int64 + ok bool + }{ + "previous step": {current - 1, true}, + "current step": {current, true}, + "next step": {current + 1, true}, + "two back": {current - 2, false}, + "two ahead": {current + 2, false}, + } { + t.Run(name, func(t *testing.T) { + code := codeAt(tc.step) + got, ok := MatchTOTPStep(secret, code, now) + if ok != tc.ok { + // A code two steps away can collide with a neighbour's by chance + // (1 in a million); only flag it when it did not. + if !tc.ok && (code == codeAt(current-1) || code == codeAt(current) || code == codeAt(current+1)) { + t.Skip("code collides with an in-window step") + } + t.Fatalf("ok = %v, want %v", ok, tc.ok) + } + if ok && got != tc.step { + t.Fatalf("step = %d, want %d", got, tc.step) + } + }) + } + + for _, bad := range []string{"", "12345", "1234567", "abcdef"} { + if _, ok := MatchTOTPStep(secret, bad, now); ok { + t.Fatalf("%q must not match", bad) + } + } +} diff --git a/backend/pkg/otp/totp.go b/backend/pkg/otp/totp.go index 351e9cb2..55a04d1c 100644 --- a/backend/pkg/otp/totp.go +++ b/backend/pkg/otp/totp.go @@ -8,6 +8,7 @@ package otp import ( "bytes" "crypto/rand" + "crypto/subtle" "encoding/base64" "fmt" "image/jpeg" @@ -102,6 +103,39 @@ func VerifyTOTPWithCustomWindow(secret, code string, window uint) bool { return valid } +// totpPeriod is the step length every enrolled authenticator uses. +const totpPeriod = 30 + +// MatchTOTPStep reports which time step a code belongs to, checking the current +// step and one on each side (the same ±30 s tolerance as VerifyTOTP). +// +// VerifyTOTP only answers yes or no, so a caller cannot tell a fresh code from +// one it already accepted (#849). The step is what lets it refuse a replay: a +// code is accepted only if its step is later than the last one accepted for +// that secret. Codes are compared in constant time. +func MatchTOTPStep(secret, code string, now time.Time) (int64, bool) { + if len(code) != 6 { + return 0, false + } + current := now.Unix() / totpPeriod + matched, found := int64(0), false + for _, step := range []int64{current - 1, current, current + 1} { + want, err := totp.GenerateCodeCustom(secret, time.Unix(step*totpPeriod, 0).UTC(), totp.ValidateOpts{ + Period: totpPeriod, + Digits: otp.DigitsSix, + Algorithm: otp.AlgorithmSHA1, + }) + if err != nil { + return 0, false + } + // No early return: every step costs the same whether or not it matches. + if subtle.ConstantTimeCompare([]byte(want), []byte(code)) == 1 { + matched, found = step, true + } + } + return matched, found +} + // backupCodeCount is how many single-use MFA backup codes are minted per user. const backupCodeCount = 8 From 7e6c668da9b8e175c6c6d85aa94273de06123657 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 09:31:22 +0100 Subject: [PATCH 2/2] fix(auth): accept a TOTP code only once (#849) A code could be used again for about 90 s after it was typed: to open a second session, confirm a second sensitive action, or turn MFA off. Each secret now records the step of the last accepted code (mfa_secrets.last_totp_step, added by AutoMigrate and migration 0066), and a code is accepted only for a later step, in one conditional UPDATE so concurrent requests cannot both win. Login, enrolment, the MFA disable and the step-up gate all go through it; a replay is refused exactly like a wrong code. Saves of the secret no longer write the column, so a stale struct cannot lower the mark. Signed-off-by: alex-dembele --- .../application/auth/mfa_replay_test.go | 117 ++++++++++++++++++ .../internal/application/auth/mfa_usecase.go | 48 +++++-- .../application/auth/mfa_usecase_test.go | 9 ++ backend/internal/domain/mfa.go | 5 + .../internal/infrastructure/authmfa/gate.go | 13 +- .../infrastructure/authmfa/gate_test.go | 70 +++++++++++ .../repository/gorm_mfa_replay_pg_test.go | 89 +++++++++++++ .../repository/gorm_mfa_repository.go | 24 +++- .../repository/mfa_repository_interface.go | 3 + .../0066_mfa_secrets_last_totp_step.down.sql | 5 + .../0066_mfa_secrets_last_totp_step.up.sql | 14 +++ 11 files changed, 384 insertions(+), 13 deletions(-) create mode 100644 backend/internal/application/auth/mfa_replay_test.go create mode 100644 backend/internal/infrastructure/authmfa/gate_test.go create mode 100644 backend/internal/infrastructure/repository/gorm_mfa_replay_pg_test.go create mode 100644 migrations/0066_mfa_secrets_last_totp_step.down.sql create mode 100644 migrations/0066_mfa_secrets_last_totp_step.up.sql diff --git a/backend/internal/application/auth/mfa_replay_test.go b/backend/internal/application/auth/mfa_replay_test.go new file mode 100644 index 00000000..210760bc --- /dev/null +++ b/backend/internal/application/auth/mfa_replay_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/infrastructure/repository" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" +) + +// #849 — a TOTP code is accepted once, wherever it is presented. + +var replayKey = []byte("0123456789abcdef0123456789abcdef") + +type replayFixture struct { + repo *MockMFARepository + user, tenant uuid.UUID + plain string +} + +func newReplayFixture(t *testing.T, verified bool) *replayFixture { + t.Helper() + plain, err := otp.GenerateTOTPSecret() + require.NoError(t, err) + enc, err := crypto.EncryptAES256GCM(plain, replayKey) + require.NoError(t, err) + f := &replayFixture{repo: NewMockMFARepository(), user: uuid.New(), tenant: uuid.New(), plain: plain} + require.NoError(t, f.repo.CreateMFASecret(context.Background(), &domain.MFASecret{ + UserID: f.user, TenantID: f.tenant, SecretEncrypted: enc, IsVerified: verified, + })) + return f +} + +// codeAt returns the code for the step `offset` steps from now. +func (f *replayFixture) codeAt(t *testing.T, offset int) string { + t.Helper() + c, err := totp.GenerateCode(f.plain, time.Now().Add(time.Duration(offset)*30*time.Second)) + require.NoError(t, err) + return c +} + +func (f *replayFixture) challenge(code string) error { + _, err := NewChallengeMFAUseCase(f.repo, replayKey).Execute(context.Background(), + ChallengeMFAInput{UserID: f.user, TenantID: f.tenant, Code: code}) + return err +} + +func TestChallengeMFA_ReplayedCodeIsRefused(t *testing.T) { + f := newReplayFixture(t, true) + code := f.codeAt(t, 0) + + require.NoError(t, f.challenge(code), "the first use opens the session") + err := f.challenge(code) + + require.Error(t, err, "the same code must not open a second session") + // Refused exactly like a wrong code: no hint that it was once valid. + wrong := f.challenge("000000") + if wrong != nil { + assert.Equal(t, wrong.Error(), err.Error()) + } +} + +func TestChallengeMFA_CodeFromAnEarlierStepIsRefused(t *testing.T) { + f := newReplayFixture(t, true) + require.NoError(t, f.challenge(f.codeAt(t, 0))) + + // The previous step's code is still inside the ±30 s window, but older than + // the one just accepted. + assert.Error(t, f.challenge(f.codeAt(t, -1))) +} + +func TestChallengeMFA_ANewerCodeIsStillAccepted(t *testing.T) { + f := newReplayFixture(t, true) + require.NoError(t, f.challenge(f.codeAt(t, 0))) + + assert.NoError(t, f.challenge(f.codeAt(t, 1)), "the next step's code is fresh") +} + +func TestVerifyMFA_EnrolmentCodeCannotThenOpenALogin(t *testing.T) { + f := newReplayFixture(t, false) + code := f.codeAt(t, 0) + + _, err := NewVerifyMFAUseCase(f.repo, repository.GormUserRepository{}, replayKey).Execute(context.Background(), + VerifyMFAInput{UserID: f.user, TenantID: f.tenant, Code: code}) + require.NoError(t, err) + + assert.Error(t, f.challenge(code)) +} + +func TestDisableMFA_ReplayedCodeIsRefused(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + plain := f.ssoAccount(t) + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + + // The code was just used to sign in... + _, err = NewChallengeMFAUseCase(f.repo, disableTOTPKey).Execute(context.Background(), + ChallengeMFAInput{UserID: f.user.ID, TenantID: f.tenant, Code: code}) + require.NoError(t, err) + + // ...so someone who watched it typed cannot turn MFA off with it. + assert.ErrorIs(t, f.runWithCode("", code), ErrMFADisableCodeIncorrect) + f.assertStillEnrolled(t) +} diff --git a/backend/internal/application/auth/mfa_usecase.go b/backend/internal/application/auth/mfa_usecase.go index c863f2d0..edae301d 100644 --- a/backend/internal/application/auth/mfa_usecase.go +++ b/backend/internal/application/auth/mfa_usecase.go @@ -186,8 +186,13 @@ func (uc *VerifyMFAUseCase) Execute(ctx context.Context, input VerifyMFAInput) ( return nil, fmt.Errorf("failed to decrypt secret: %w", err) } - // Verify TOTP code (±1 window) - if !otp.VerifyTOTP(decryptedSecret, input.Code) { + // Verify TOTP code (±1 window), once: the enrolment code cannot then open + // a login (#849). + accepted, err := acceptTOTP(ctx, uc.mfaRepo, input.UserID, input.TenantID, decryptedSecret, input.Code) + if err != nil { + return nil, err + } + if !accepted { return nil, domain.NewValidationError("invalid TOTP code") } @@ -350,7 +355,14 @@ func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) if err != nil { return nil, fmt.Errorf("auth.DisableMFA: decrypt secret: %w", err) } - if input.Code == "" || !otp.VerifyTOTP(plain, input.Code) { + if input.Code == "" { + return nil, ErrMFADisableCodeIncorrect + } + accepted, err := acceptTOTP(ctx, uc.mfaRepo, input.UserID, input.TenantID, plain, input.Code) + if err != nil { + return nil, err + } + if !accepted { return nil, ErrMFADisableCodeIncorrect } } @@ -447,13 +459,13 @@ func (uc *ChallengeMFAUseCase) Execute(ctx context.Context, input ChallengeMFAIn return nil, fmt.Errorf("failed to decrypt secret: %w", err) } - // Try TOTP first - if otp.VerifyTOTP(decryptedSecret, input.Code) { - // Mark as last used - now := time.Now() - mfaSecret.LastUsedAt = &now - _ = uc.mfaRepo.UpdateMFASecret(ctx, mfaSecret) - + // Try TOTP first. Accepting it also records its step and last_used_at, so + // the same code cannot open a second session (#849). + accepted, err := acceptTOTP(ctx, uc.mfaRepo, input.UserID, input.TenantID, decryptedSecret, input.Code) + if err != nil { + return nil, err + } + if accepted { return &ChallengeMFAOutput{ Verified: true, Message: "MFA verified successfully", @@ -482,3 +494,19 @@ func (uc *ChallengeMFAUseCase) Execute(ctx context.Context, input ChallengeMFAIn return nil, domain.NewValidationError("invalid MFA code") } + +// acceptTOTP accepts a TOTP code at most once (#849). A code that matches but +// whose step was already used, or is older than the last one used, is refused +// exactly like a wrong code: telling them apart would tell an observer the +// code they replayed was once valid. +func acceptTOTP(ctx context.Context, repo repository.MFARepository, userID, tenantID uuid.UUID, plainSecret, code string) (bool, error) { + step, ok := otp.MatchTOTPStep(plainSecret, code, time.Now()) + if !ok { + return false, nil + } + consumed, err := repo.ConsumeTOTPStep(ctx, userID, tenantID, step) + if err != nil { + return false, fmt.Errorf("auth: record TOTP step: %w", err) + } + return consumed, nil +} diff --git a/backend/internal/application/auth/mfa_usecase_test.go b/backend/internal/application/auth/mfa_usecase_test.go index 9356cf72..cbe151c8 100644 --- a/backend/internal/application/auth/mfa_usecase_test.go +++ b/backend/internal/application/auth/mfa_usecase_test.go @@ -47,6 +47,15 @@ func (m *MockMFARepository) UpdateMFASecret(ctx context.Context, secret *domain. return nil } +func (m *MockMFARepository) ConsumeTOTPStep(ctx context.Context, userID, tenantID uuid.UUID, step int64) (bool, error) { + secret := m.secrets[userID.String()+":"+tenantID.String()] + if secret == nil || (secret.LastTOTPStep != nil && *secret.LastTOTPStep >= step) { + return false, nil + } + secret.LastTOTPStep = &step + return true, nil +} + func (m *MockMFARepository) DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error { key := userID.String() + ":" + tenantID.String() delete(m.secrets, key) diff --git a/backend/internal/domain/mfa.go b/backend/internal/domain/mfa.go index d1e6baa5..ad078ff0 100644 --- a/backend/internal/domain/mfa.go +++ b/backend/internal/domain/mfa.go @@ -24,6 +24,11 @@ type MFASecret struct { CreatedAt time.Time `gorm:"autoCreateTime" json:"created_at"` UpdatedAt time.Time `gorm:"autoUpdateTime" json:"updated_at"` DeletedAt gorm.DeletedAt `gorm:"index" json:"-"` + + // LastTOTPStep is the time step of the last code accepted (#849). A code is + // only accepted for a later step, so a code seen on screen cannot be used + // twice. Written only by MFARepository.ConsumeTOTPStep, never by a Save. + LastTOTPStep *int64 `gorm:"column:last_totp_step" json:"-"` } // TableName specifies the table name for MFASecret diff --git a/backend/internal/infrastructure/authmfa/gate.go b/backend/internal/infrastructure/authmfa/gate.go index bef9e982..98288b65 100644 --- a/backend/internal/infrastructure/authmfa/gate.go +++ b/backend/internal/infrastructure/authmfa/gate.go @@ -10,6 +10,7 @@ package authmfa import ( "context" "errors" + "time" "github.com/google/uuid" "github.com/opendefender/openrisk/internal/infrastructure/repository" @@ -43,7 +44,17 @@ func (g *Gate) VerifyRequired(ctx context.Context, user, tenant uuid.UUID, code if err != nil { return ErrInvalidCode } - if !otp.VerifyTOTP(plain, code) { + // Once only (#849): a code already accepted — here, at login, anywhere — + // cannot confirm a second sensitive action. + step, ok := otp.MatchTOTPStep(plain, code, time.Now()) + if !ok { + return ErrInvalidCode + } + consumed, err := g.mfaRepo.ConsumeTOTPStep(ctx, user, tenant, step) + if err != nil { + return err + } + if !consumed { return ErrInvalidCode } return nil diff --git a/backend/internal/infrastructure/authmfa/gate_test.go b/backend/internal/infrastructure/authmfa/gate_test.go new file mode 100644 index 00000000..3ac549c2 --- /dev/null +++ b/backend/internal/infrastructure/authmfa/gate_test.go @@ -0,0 +1,70 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package authmfa + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/infrastructure/repository" + "github.com/opendefender/openrisk/internal/testsupport/sqliteschema" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" +) + +// #849 — the step-up gate accepts a code once, like login does. + +var gateKey = []byte("0123456789abcdef0123456789abcdef") + +func newGate(t *testing.T) (*Gate, uuid.UUID, uuid.UUID, string) { + t.Helper() + db, err := gorm.Open(sqlite.Open("file:gate_"+uuid.NewString()+"?mode=memory&cache=private"), + &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + require.NoError(t, db.Exec(`CREATE TABLE mfa_secrets (id TEXT PRIMARY KEY)`).Error) + require.NoError(t, sqliteschema.Reconcile(db, "mfa_secrets", &domain.MFASecret{})) + + plain, err := otp.GenerateTOTPSecret() + require.NoError(t, err) + enc, err := crypto.EncryptAES256GCM(plain, gateKey) + require.NoError(t, err) + user, tenant := uuid.New(), uuid.New() + require.NoError(t, db.Create(&domain.MFASecret{ + ID: uuid.New(), UserID: user, TenantID: tenant, SecretEncrypted: enc, IsVerified: true, + }).Error) + return NewGate(repository.NewGormMFARepository(db), gateKey), user, tenant, plain +} + +func TestGate_ReplayedCodeIsRefused(t *testing.T) { + gate, user, tenant, plain := newGate(t) + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + + require.NoError(t, gate.VerifyRequired(context.Background(), user, tenant, code)) + assert.ErrorIs(t, gate.VerifyRequired(context.Background(), user, tenant, code), ErrInvalidCode, + "one code confirms one sensitive action") +} + +func TestGate_CodeIsScopedToItsTenant(t *testing.T) { + gate, user, tenant, plain := newGate(t) + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + + // Another tenant id finds no secret, so it is exempt and consumes nothing... + require.NoError(t, gate.VerifyRequired(context.Background(), user, uuid.New(), code)) + // ...and the code is still good for its own tenant. + assert.NoError(t, gate.VerifyRequired(context.Background(), user, tenant, code)) +} diff --git a/backend/internal/infrastructure/repository/gorm_mfa_replay_pg_test.go b/backend/internal/infrastructure/repository/gorm_mfa_replay_pg_test.go new file mode 100644 index 00000000..8d89fdd4 --- /dev/null +++ b/backend/internal/infrastructure/repository/gorm_mfa_replay_pg_test.go @@ -0,0 +1,89 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package repository + +import ( + "context" + "os" + "sync" + "sync/atomic" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + "github.com/opendefender/openrisk/internal/domain" +) + +// #849 — two requests racing with the same TOTP code cannot both win. This needs +// separate connections, so unlike the other _pg tests it writes one row to the +// real mfa_secrets table (migrated schema required) and deletes it afterwards. +func TestGormMFARepository_ConsumeTOTPStep_ConcurrentReplay_Postgres(t *testing.T) { + dsn := os.Getenv("DATABASE_URL") + if dsn == "" { + t.Skip("DATABASE_URL not set") + } + db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + sqlDB, err := db.DB() + require.NoError(t, err) + sqlDB.SetMaxOpenConns(20) + + repo := NewGormMFARepository(db) + ctx := context.Background() + id, user, tenant := uuid.New(), uuid.New(), uuid.New() + require.NoError(t, db.Create(&domain.MFASecret{ID: id, UserID: user, TenantID: tenant, SecretEncrypted: "x", IsVerified: true}).Error) + t.Cleanup(func() { + // Scoped to the row this test created, by primary key and tenant. + db.Unscoped().Where("id = ? AND tenant_id = ?", id, tenant).Delete(&domain.MFASecret{}) + }) + + const racers = 20 + const step = int64(63_333_333) + var wins atomic.Int32 + var wg sync.WaitGroup + start := make(chan struct{}) + for range racers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + ok, err := repo.ConsumeTOTPStep(ctx, user, tenant, step) + assert.NoError(t, err) + if ok { + wins.Add(1) + } + }() + } + close(start) + wg.Wait() + + assert.EqualValues(t, 1, wins.Load(), "exactly one of %d concurrent uses of one code may succeed", racers) + + // An earlier step is refused, a later one accepted. + ok, err := repo.ConsumeTOTPStep(ctx, user, tenant, step-1) + require.NoError(t, err) + assert.False(t, ok) + ok, err = repo.ConsumeTOTPStep(ctx, user, tenant, step+1) + require.NoError(t, err) + assert.True(t, ok) + + // Another tenant's id touches nothing. + ok, err = repo.ConsumeTOTPStep(ctx, user, uuid.New(), step+5) + require.NoError(t, err) + assert.False(t, ok) + + // A Save of a stale struct must not lower the mark. + stale := &domain.MFASecret{ID: id, UserID: user, TenantID: tenant, SecretEncrypted: "x", IsVerified: true} + require.NoError(t, repo.UpdateMFASecret(ctx, stale)) + ok, err = repo.ConsumeTOTPStep(ctx, user, tenant, step+1) + require.NoError(t, err) + assert.False(t, ok, "UpdateMFASecret must leave last_totp_step alone") +} diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository.go b/backend/internal/infrastructure/repository/gorm_mfa_repository.go index 9fd29ec4..a6646463 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_repository.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository.go @@ -46,9 +46,29 @@ func (r *GormMFARepository) GetMFASecret(ctx context.Context, userID, tenantID u return &secret, nil } -// UpdateMFASecret updates an existing MFA secret +// UpdateMFASecret updates an existing MFA secret. +// +// last_totp_step is left out: the struct in hand may predate the last accepted +// code, and writing it back would lower the mark and re-open the replay #849 +// closes. ConsumeTOTPStep is its only writer. func (r *GormMFARepository) UpdateMFASecret(ctx context.Context, secret *domain.MFASecret) error { - return r.db.WithContext(ctx).Save(secret).Error + return r.db.WithContext(ctx).Omit("last_totp_step").Save(secret).Error +} + +// ConsumeTOTPStep records that a code for step was accepted, and reports false +// when a code for that step or a later one already was (#849). +// +// One conditional UPDATE does the check and the write, so two requests racing +// with the same code cannot both win: Postgres serialises them on the row, and +// the second re-evaluates the WHERE after the first commits. +func (r *GormMFARepository) ConsumeTOTPStep(ctx context.Context, userID, tenantID uuid.UUID, step int64) (bool, error) { + res := r.db.WithContext(ctx).Model(&domain.MFASecret{}). + Where("user_id = ? AND tenant_id = ? AND (last_totp_step IS NULL OR last_totp_step < ?)", userID, tenantID, step). + Updates(map[string]any{"last_totp_step": step, "last_used_at": time.Now()}) + if res.Error != nil { + return false, res.Error + } + return res.RowsAffected == 1, nil } // DisableMFA removes the TOTP secret and every backup code of one user, in one diff --git a/backend/internal/infrastructure/repository/mfa_repository_interface.go b/backend/internal/infrastructure/repository/mfa_repository_interface.go index c69e10e5..b9243308 100644 --- a/backend/internal/infrastructure/repository/mfa_repository_interface.go +++ b/backend/internal/infrastructure/repository/mfa_repository_interface.go @@ -18,6 +18,9 @@ type MFARepository interface { CreateMFASecret(ctx context.Context, secret *domain.MFASecret) error GetMFASecret(ctx context.Context, userID, tenantID uuid.UUID) (*domain.MFASecret, error) UpdateMFASecret(ctx context.Context, secret *domain.MFASecret) error + // ConsumeTOTPStep marks a TOTP step as used; false means it (or a later one) + // already was, i.e. a replay (#849). + ConsumeTOTPStep(ctx context.Context, userID, tenantID uuid.UUID, step int64) (bool, error) // DisableMFA deletes the secret AND the backup codes, atomically. DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error diff --git a/migrations/0066_mfa_secrets_last_totp_step.down.sql b/migrations/0066_mfa_secrets_last_totp_step.down.sql new file mode 100644 index 00000000..493b2ef1 --- /dev/null +++ b/migrations/0066_mfa_secrets_last_totp_step.down.sql @@ -0,0 +1,5 @@ +-- Reverses nothing on purpose. Dropping last_totp_step would re-open the TOTP +-- replay #849 closes on a running server, and the column is harmless to an older +-- binary, which never reads it (GORM selects by struct fields). An operator who +-- wants it gone can drop it by hand once no binary that writes it is deployed. +SELECT 1; diff --git a/migrations/0066_mfa_secrets_last_totp_step.up.sql b/migrations/0066_mfa_secrets_last_totp_step.up.sql new file mode 100644 index 00000000..9ce85e70 --- /dev/null +++ b/migrations/0066_mfa_secrets_last_totp_step.up.sql @@ -0,0 +1,14 @@ +-- #849 — a TOTP code is accepted once. +-- +-- The server checked a code against its ±30 s window and never recorded that it +-- had been used, so the same code could open a second session, confirm a second +-- sensitive action, or turn MFA off within about 90 s of being typed. +-- last_totp_step holds the time step (unix seconds / 30) of the last code +-- accepted for this secret; a code is accepted only for a later step, in one +-- conditional UPDATE (GormMFARepository.ConsumeTOTPStep). +-- +-- Nullable, no default: NULL means "no code accepted yet", which is the true +-- state of every existing row. AutoMigrate adds the same column from +-- domain.MFASecret on boot; this file carries it for the migration CLI path. + +ALTER TABLE mfa_secrets ADD COLUMN IF NOT EXISTS last_totp_step bigint;