diff --git a/internal/authenticators/totp/totp.go b/internal/authenticators/totp/totp.go index 45bb212c5..5d9e3baff 100644 --- a/internal/authenticators/totp/totp.go +++ b/internal/authenticators/totp/totp.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "encoding/json" "errors" + "fmt" "image/png" "time" @@ -37,6 +38,10 @@ const ( // will accept — Period 30 with Skew 1 spans three steps — so a redeemed // code stays claimed for as long as it would otherwise still validate. totpPasscodeReuseWindowSeconds = 90 + // totalRecoveryCodes is how many single-use recovery codes Generate issues + // per enrollment. It also bounds recoveryCodeConsumeAttempts: it is the most + // redemptions that can legitimately contend for the row. + totalRecoveryCodes = 10 ) // pendingTOTPSecret is the memory-store payload for a re-enrollment awaiting @@ -161,7 +166,7 @@ func (p *provider) Generate(ctx context.Context, id string) (*config.Authenticat encodedText := crypto.EncodeB64(buf.String()) secret := key.Secret() recoveryCodes := []string{} - for i := 0; i < 10; i++ { + for i := 0; i < totalRecoveryCodes; i++ { recoveryCodes = append(recoveryCodes, uuid.NewString()) } // recoverCodesMap is the plaintext map returned to the caller once (the @@ -380,55 +385,94 @@ func (p *provider) Validate(ctx context.Context, passcode string, userID string) return true, nil } +// recoveryCodeConsumeAttempts bounds the compare-and-swap retry loop in +// ValidateRecoveryCode. +// +// The value is not a generic "retry a few times" constant. A caller can only +// lose the swap because another redemption committed to the same row, and each +// of those spends one of the user's recovery codes — of which Generate issues +// exactly totalRecoveryCodes. So that count IS the worst case for legitimate +// contention: even if every code a user has is redeemed simultaneously, each +// request gets through within this many attempts. Anything beyond it is a +// database that is not making progress, and the loop stops rather than +// hammering a login path. +// +// Erring high is close to free — the realistic case resolves in one or two +// rounds, and the cost of the bound is only paid on the failure path — whereas +// erring low turns ordinary contention into a spurious fault for a user who is +// already locked out of their authenticator app. +const recoveryCodeConsumeAttempts = totalRecoveryCodes + // ValidateRecoveryCode validates a Time-Based One-Time Password (TOTP) recovery code against the stored TOTP recovery code for a user. +// +// Recovery codes are single-use, and the write that spends one has to be what +// enforces that. Reading the blob, deciding in Go, and writing back +// unconditionally let two concurrent redemptions of the SAME code both read it +// unconsumed and both return true — measured at 3 of 8 concurrent attempts +// succeeding before this loop existed — which hands an attacker holding one +// leaked code an unlimited number of logins. So the write goes through +// ConsumeAuthenticatorRecoveryCode, which only lands while the row still holds +// the blob this call read; on a lost race we re-read and look again, and the +// code is then marked consumed, so the loser returns false. func (p *provider) ValidateRecoveryCode(ctx context.Context, recoveryCode, userID string) (bool, error) { - // get totp details - totpModel, err := p.deps.StorageProvider.GetAuthenticatorDetailsByUserId(ctx, userID, constants.EnvKeyTOTPAuthenticator) - if err != nil { - return false, err - } - // See Validate: the DynamoDB provider signals "not enrolled" as (nil, nil) - // rather than an error, so guard before dereferencing. - if totpModel == nil { - return false, nil - } - // convert recoveryCodes to map - recoveryCodesMap := map[string]bool{} - err = json.Unmarshal([]byte(refs.StringValue(totpModel.RecoveryCodes)), &recoveryCodesMap) - if err != nil { - return false, err - } - // Recovery codes are stored as SHA-256 hashes. Look up the hash of the - // supplied code; if it isn't present, fall back to a direct plaintext - // lookup for rows written by a pre-hashing release (lazy backward - // compatibility, mirroring the TOTP secret migration in Validate). The - // matched key is marked consumed either way, preserving one-time use. - matchKey := crypto.HashRecoveryCode(recoveryCode) - val, ok := recoveryCodesMap[matchKey] - if !ok { - // Legacy plaintext row: the code itself is the stored key. - matchKey = recoveryCode - val, ok = recoveryCodesMap[matchKey] - } - if !ok || val { - // Not a known code, or one that has already been consumed: this is - // a verification failure, not a server fault. Return (false, nil) so - // the caller counts it as a failed attempt rather than an error. - return false, nil - } - // mark the matched recovery code consumed - recoveryCodesMap[matchKey] = true - // convert recoveryCodesMap to string - jsonData, err := json.Marshal(recoveryCodesMap) - if err != nil { - return false, err - } - recoveryCodesString := string(jsonData) - totpModel.RecoveryCodes = refs.NewStringRef(recoveryCodesString) - // update recovery code map in db - _, err = p.deps.StorageProvider.UpdateAuthenticator(ctx, totpModel) - if err != nil { - return false, err + log := p.deps.Log.With().Str("func", "ValidateRecoveryCode").Str("user_id", userID).Logger() + for attempt := 0; attempt < recoveryCodeConsumeAttempts; attempt++ { + // get totp details + totpModel, err := p.deps.StorageProvider.GetAuthenticatorDetailsByUserId(ctx, userID, constants.EnvKeyTOTPAuthenticator) + if err != nil { + return false, err + } + // See Validate: the DynamoDB provider signals "not enrolled" as (nil, nil) + // rather than an error, so guard before dereferencing. + if totpModel == nil { + return false, nil + } + // The blob is compared byte for byte by the storage layer, so keep the + // exact string that was read — re-marshalling the map would reorder + // keys and match nothing. + storedCodes := refs.StringValue(totpModel.RecoveryCodes) + // convert recoveryCodes to map + recoveryCodesMap := map[string]bool{} + if err := json.Unmarshal([]byte(storedCodes), &recoveryCodesMap); err != nil { + return false, err + } + // Recovery codes are stored as SHA-256 hashes. Look up the hash of the + // supplied code; if it isn't present, fall back to a direct plaintext + // lookup for rows written by a pre-hashing release (lazy backward + // compatibility, mirroring the TOTP secret migration in Validate). The + // matched key is marked consumed either way, preserving one-time use. + matchKey := crypto.HashRecoveryCode(recoveryCode) + val, ok := recoveryCodesMap[matchKey] + if !ok { + // Legacy plaintext row: the code itself is the stored key. + matchKey = recoveryCode + val, ok = recoveryCodesMap[matchKey] + } + if !ok || val { + // Not a known code, or one that has already been consumed: this is + // a verification failure, not a server fault. Return (false, nil) so + // the caller counts it as a failed attempt rather than an error. + return false, nil + } + // mark the matched recovery code consumed + recoveryCodesMap[matchKey] = true + // convert recoveryCodesMap to string + jsonData, err := json.Marshal(recoveryCodesMap) + if err != nil { + return false, err + } + consumed, err := p.deps.StorageProvider.ConsumeAuthenticatorRecoveryCode(ctx, totpModel.ID, storedCodes, string(jsonData)) + if err != nil { + return false, err + } + if consumed { + return true, nil + } + log.Debug().Int("attempt", attempt+1).Msg("recovery code blob changed under us, re-reading") } - return true, nil + // Never report exhaustion as a failed code. The code may well still be + // valid; what failed is the write, and answering "invalid" would spend the + // user's credential on a database problem. See the fault-tolerance note on + // ConsumeAuthenticatorRecoveryCode. + return false, fmt.Errorf("could not consume recovery code after %d attempts", recoveryCodeConsumeAttempts) } diff --git a/internal/authenticators/totp/totp_recovery_single_use_test.go b/internal/authenticators/totp/totp_recovery_single_use_test.go new file mode 100644 index 000000000..7e9040893 --- /dev/null +++ b/internal/authenticators/totp/totp_recovery_single_use_test.go @@ -0,0 +1,305 @@ +package totp + +import ( + "context" + "encoding/json" + "errors" + "sync" + "testing" + + "github.com/google/uuid" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/crypto" + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// recoveryStore is a correct backend in miniature: the compare of oldCodes and +// the write happen under one lock, which is what every real implementation gets +// from its database (row lock, LWT, ConditionExpression, CAS). It exists so the +// retry loop in ValidateRecoveryCode can be tested for the behaviours a real +// database only produces under luck — a lost race, a run of lost races, a +// storage fault — without needing six databases running. +type recoveryStore struct { + storage.Provider + + mu sync.Mutex + row *schemas.Authenticator + reads int + + // beforeConsume runs outside the lock, so a test can commit an interfering + // write in the window between the caller's read and its swap. + beforeConsume func() + // consumeErr, when set, makes every swap fail as a storage fault. + consumeErr error + // loseFirstN refuses that many swaps before behaving normally, standing in + // for other requests committing to the row. + loseFirstN int +} + +func (s *recoveryStore) GetAuthenticatorDetailsByUserId(_ context.Context, _, _ string) (*schemas.Authenticator, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.reads++ + // Hand back a copy: a caller must never be able to mutate the stored row + // by writing through the struct it was given. + cp := *s.row + codes := refs.StringValue(s.row.RecoveryCodes) + cp.RecoveryCodes = &codes + return &cp, nil +} + +func (s *recoveryStore) ConsumeAuthenticatorRecoveryCode(_ context.Context, id, oldCodes, newCodes string) (bool, error) { + if s.beforeConsume != nil { + s.beforeConsume() + } + s.mu.Lock() + defer s.mu.Unlock() + if s.consumeErr != nil { + return false, s.consumeErr + } + if s.loseFirstN > 0 { + s.loseFirstN-- + return false, nil + } + if id != s.row.ID || refs.StringValue(s.row.RecoveryCodes) != oldCodes { + return false, nil + } + s.row.RecoveryCodes = &newCodes + return true, nil +} + +func (s *recoveryStore) codes(t *testing.T) map[string]bool { + t.Helper() + s.mu.Lock() + defer s.mu.Unlock() + out := map[string]bool{} + require.NoError(t, json.Unmarshal([]byte(refs.StringValue(s.row.RecoveryCodes)), &out)) + return out +} + +// newRecoveryProvider builds a provider over a store holding n fresh hashed +// recovery codes, and returns the plaintext codes. +func newRecoveryProvider(t *testing.T, n int) (*provider, *recoveryStore, []string) { + t.Helper() + plain := make([]string, n) + stored := map[string]bool{} + for i := range plain { + plain[i] = uuid.NewString() + stored[crypto.HashRecoveryCode(plain[i])] = false + } + blob, err := json.Marshal(stored) + require.NoError(t, err) + + store := &recoveryStore{row: &schemas.Authenticator{ + ID: uuid.NewString(), + UserID: "user-1", + Method: "totp", + RecoveryCodes: refs.NewStringRef(string(blob)), + }} + + l := zerolog.Nop() + p, err := NewProvider(&Dependencies{ + Log: &l, + StorageProvider: store, + EncryptionKey: "test-key", + }) + require.NoError(t, err) + return p, store, plain +} + +// TestValidateRecoveryCodeIsSingleUseUnderConcurrency is the regression guard +// for the double-spend. Read-decide-write let every racer see the code +// unconsumed and return true; the swap has to be what enforces "once". +func TestValidateRecoveryCodeIsSingleUseUnderConcurrency(t *testing.T) { + const racers = 16 + for round := 0; round < 50; round++ { + p, store, plain := newRecoveryProvider(t, 10) + + var start, done sync.WaitGroup + var mu sync.Mutex + accepted := 0 + start.Add(1) + for i := 0; i < racers; i++ { + done.Add(1) + go func() { + defer done.Done() + start.Wait() + ok, err := p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + assert.NoError(t, err) + if ok { + mu.Lock() + accepted++ + mu.Unlock() + } + }() + } + start.Done() + done.Wait() + + require.Equal(t, 1, accepted, "round %d: one code, one redemption", round) + assert.True(t, store.codes(t)[crypto.HashRecoveryCode(plain[0])], "the redeemed code must be marked consumed") + } +} + +// TestValidateRecoveryCodeRetriesAfterALostRace pins the recovery half of the +// contract: losing the swap is not a rejection. Another request writing to the +// row does not make THIS caller's code invalid, so the loop re-reads and +// re-applies rather than answering false. +func TestValidateRecoveryCodeRetriesAfterALostRace(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + store.loseFirstN = recoveryCodeConsumeAttempts - 1 + + ok, err := p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.NoError(t, err) + assert.True(t, ok, "a code that is still unconsumed must be accepted despite lost races") + assert.Equal(t, recoveryCodeConsumeAttempts, store.reads, "each retry must re-read rather than reuse a stale blob") + assert.True(t, store.codes(t)[crypto.HashRecoveryCode(plain[0])]) +} + +// TestValidateRecoveryCodeLosesToAConcurrentRedemptionOfTheSameCode drives the +// exact interleaving the fix targets: the code is spent by someone else in the +// window between this caller's read and its swap. The retry must re-read, find +// it consumed, and reject — cleanly, with no error. +func TestValidateRecoveryCodeLosesToAConcurrentRedemptionOfTheSameCode(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + + var once sync.Once + store.beforeConsume = func() { + once.Do(func() { + // Someone else redeems the same code first. + store.mu.Lock() + codes := map[string]bool{} + _ = json.Unmarshal([]byte(refs.StringValue(store.row.RecoveryCodes)), &codes) + codes[crypto.HashRecoveryCode(plain[0])] = true + blob, _ := json.Marshal(codes) + store.row.RecoveryCodes = refs.NewStringRef(string(blob)) + store.mu.Unlock() + }) + } + + ok, err := p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.NoError(t, err, "losing the race is a rejection, not a fault") + assert.False(t, ok, "a code spent by another request must not validate here too") +} + +// TestValidateRecoveryCodeReportsSustainedContentionAsAFault pins the direction +// the loop fails in. Exhausting the retries means the WRITE never landed — the +// code may well still be valid — so answering false would spend a user's +// recovery credential on a database problem and tell them their input was +// wrong. It must be an error, and the bool must stay false. +func TestValidateRecoveryCodeReportsSustainedContentionAsAFault(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + store.loseFirstN = recoveryCodeConsumeAttempts + + ok, err := p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.Error(t, err, "exhausted retries must not be reported as an invalid code") + assert.False(t, ok) + assert.False(t, store.codes(t)[crypto.HashRecoveryCode(plain[0])], "an unconsumed code must remain spendable") +} + +// TestValidateRecoveryCodePropagatesStorageErrors keeps a database outage +// distinguishable from a wrong code, per the not-found contract in AGENTS.md. +func TestValidateRecoveryCodePropagatesStorageErrors(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + boom := errors.New("database is on fire") + store.consumeErr = boom + + ok, err := p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.ErrorIs(t, err, boom) + assert.False(t, ok) +} + +// TestValidateRecoveryCodeRejectionsDoNotWrite covers the two rejections that +// must never reach the database at all: an unknown code and an already-spent +// one. Both are failed attempts, not faults. +func TestValidateRecoveryCodeRejectionsDoNotWrite(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + // Make any swap that does happen loudly wrong. + store.consumeErr = errors.New("no write should have been attempted") + + ok, err := p.ValidateRecoveryCode(context.Background(), uuid.NewString(), "user-1") + require.NoError(t, err, "an unissued code is a failed attempt, not an error") + assert.False(t, ok) + + store.consumeErr = nil + ok, err = p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.NoError(t, err) + require.True(t, ok) + + store.consumeErr = errors.New("no write should have been attempted") + ok, err = p.ValidateRecoveryCode(context.Background(), plain[0], "user-1") + require.NoError(t, err, "a spent code is a failed attempt, not an error") + assert.False(t, ok) +} + +// TestValidateRecoveryCodeConsumesOnlyTheMatchedCode pins that the swap offered +// to storage differs from the stored blob in exactly one key. The whole blob is +// rewritten on every redemption, so a bug here silently burns or revives the +// other nine codes. +func TestValidateRecoveryCodeConsumesOnlyTheMatchedCode(t *testing.T) { + p, store, plain := newRecoveryProvider(t, 10) + + ok, err := p.ValidateRecoveryCode(context.Background(), plain[3], "user-1") + require.NoError(t, err) + require.True(t, ok) + + after := store.codes(t) + require.Len(t, after, 10, "no code may be added or dropped by a redemption") + for i, code := range plain { + assert.Equal(t, i == 3, after[crypto.HashRecoveryCode(code)], + "only the redeemed code may change state (index %d)", i) + } +} + +// TestValidateRecoveryCodeConsumesLegacyPlaintextCodes keeps the rolling-upgrade +// fallback working through the new swap: a row written by a pre-hashing release +// stores the code itself as the key, and the compare-and-swap must be built from +// that same blob or the write never lands. +func TestValidateRecoveryCodeConsumesLegacyPlaintextCodes(t *testing.T) { + legacyCode := uuid.NewString() + blob, err := json.Marshal(map[string]bool{legacyCode: false}) + require.NoError(t, err) + + store := &recoveryStore{row: &schemas.Authenticator{ + ID: uuid.NewString(), + UserID: "user-1", + Method: "totp", + RecoveryCodes: refs.NewStringRef(string(blob)), + }} + l := zerolog.Nop() + p, err := NewProvider(&Dependencies{Log: &l, StorageProvider: store, EncryptionKey: "test-key"}) + require.NoError(t, err) + + ok, err := p.ValidateRecoveryCode(context.Background(), legacyCode, "user-1") + require.NoError(t, err) + assert.True(t, ok, "a legacy plaintext code must still validate during a rolling upgrade") + assert.True(t, store.codes(t)[legacyCode], "and be consumed under its plaintext key") + + ok, err = p.ValidateRecoveryCode(context.Background(), legacyCode, "user-1") + require.NoError(t, err) + assert.False(t, ok, "a legacy code is single-use too") +} + +// TestValidateRecoveryCodeRejectsACorruptBlob pins that an unparseable +// recovery-code column is a fault, not a silent "wrong code" — the latter would +// hide the corruption behind a plausible-looking login failure. +func TestValidateRecoveryCodeRejectsACorruptBlob(t *testing.T) { + store := &recoveryStore{row: &schemas.Authenticator{ + ID: uuid.NewString(), + UserID: "user-1", + Method: "totp", + RecoveryCodes: refs.NewStringRef("not json"), + }} + l := zerolog.Nop() + p, err := NewProvider(&Dependencies{Log: &l, StorageProvider: store, EncryptionKey: "test-key"}) + require.NoError(t, err) + + ok, err := p.ValidateRecoveryCode(context.Background(), uuid.NewString(), "user-1") + assert.Error(t, err) + assert.False(t, ok) +} diff --git a/internal/integration_tests/totp_recovery_codes_at_rest_test.go b/internal/integration_tests/totp_recovery_codes_at_rest_test.go index daba8c263..eefbed351 100644 --- a/internal/integration_tests/totp_recovery_codes_at_rest_test.go +++ b/internal/integration_tests/totp_recovery_codes_at_rest_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "strings" + "sync" + "sync/atomic" "testing" "github.com/google/uuid" @@ -129,4 +131,97 @@ func TestTOTPRecoveryCodesAtRest(t *testing.T) { require.NoError(t, err) assert.False(t, ok, "consumed legacy recovery code must not validate again") }) + + // "Exactly once" has to hold under concurrency, not just in sequence. The + // original implementation read the blob, decided in Go, and wrote it back + // unconditionally, so racing redemptions of one code all saw it unconsumed + // and all returned true — measured here at 3 of 8 before the fix. That + // turns a single leaked recovery code into an unlimited supply of logins, + // which is the one thing a recovery code must not be. + t.Run("one recovery code redeemed concurrently succeeds exactly once", func(t *testing.T) { + const racers = 8 + for round := 0; round < 25; round++ { + user := mkUser(t) + authConfig, err := ts.AuthenticatorProvider.Generate(ctx, user.ID) + require.NoError(t, err) + code := authConfig.RecoveryCodes[0] + + var start sync.WaitGroup + var done sync.WaitGroup + var accepted atomic.Int32 + start.Add(1) + for i := 0; i < racers; i++ { + done.Add(1) + go func() { + defer done.Done() + start.Wait() + ok, err := ts.AuthenticatorProvider.ValidateRecoveryCode(ctx, code, user.ID) + assert.NoError(t, err, "a lost race is a rejection, never a fault") + if ok { + accepted.Add(1) + } + }() + } + start.Done() + done.Wait() + + require.Equal(t, int32(1), accepted.Load(), + "round %d: exactly one concurrent redemption of a single recovery code may succeed", round) + } + }) + + // Racing redemptions of DIFFERENT codes must both land. The blob is one + // column, so a compare-and-swap that simply gave up would silently drop the + // loser's code back to unconsumed — trading a double-spend for a lost + // write. The retry re-reads and re-applies instead. + t.Run("two different recovery codes redeemed concurrently both succeed", func(t *testing.T) { + for round := 0; round < 25; round++ { + user := mkUser(t) + authConfig, err := ts.AuthenticatorProvider.Generate(ctx, user.ID) + require.NoError(t, err) + + var start sync.WaitGroup + var done sync.WaitGroup + var accepted atomic.Int32 + start.Add(1) + for i := 0; i < 2; i++ { + code := authConfig.RecoveryCodes[i] + done.Add(1) + go func() { + defer done.Done() + start.Wait() + ok, err := ts.AuthenticatorProvider.ValidateRecoveryCode(ctx, code, user.ID) + assert.NoError(t, err) + if ok { + accepted.Add(1) + } + }() + } + start.Done() + done.Wait() + + require.Equal(t, int32(2), accepted.Load(), + "round %d: distinct recovery codes must not invalidate each other", round) + + // Both are spent, and the other eight are untouched. + row, err := ts.StorageProvider.GetAuthenticatorDetailsByUserId(ctx, user.ID, constants.EnvKeyTOTPAuthenticator) + require.NoError(t, err) + storedMap := map[string]bool{} + require.NoError(t, json.Unmarshal([]byte(refs.StringValue(row.RecoveryCodes)), &storedMap)) + require.Len(t, storedMap, 10, "no code may be lost by a concurrent write") + spent := 0 + for _, consumed := range storedMap { + if consumed { + spent++ + } + } + assert.Equal(t, 2, spent, "round %d: exactly the two redeemed codes are marked consumed", round) + + // And neither of the remaining codes was collaterally burned. + for _, code := range authConfig.RecoveryCodes[2:] { + assert.False(t, storedMap[crypto.HashRecoveryCode(code)], + "an unredeemed code must still be spendable") + } + } + }) } diff --git a/internal/integration_tests/verify_otp_totp_test.go b/internal/integration_tests/verify_otp_totp_test.go index 20f2ade84..7d11650ce 100644 --- a/internal/integration_tests/verify_otp_totp_test.go +++ b/internal/integration_tests/verify_otp_totp_test.go @@ -83,6 +83,44 @@ func TestVerifyOTPTOTPThroughService(t *testing.T) { assert.NotEmpty(t, res.AccessToken, "a valid recovery code must mint an access token") }) + // The screen at /app posts a recovery code into the same field as a TOTP + // passcode, so this is the path a real user drives. Proving single-use at + // the provider is not enough: a fresh MFA session is armed below, so the + // only thing that can reject the second attempt is the code already having + // been spent by the subtest above. + t.Run("a spent recovery code is rejected on replay", func(t *testing.T) { + armMfaSession() + res, err := ts.GraphQLProvider.VerifyOTP(ctx, &model.VerifyOTPRequest{ + Email: &email, + Otp: authConfig.RecoveryCodes[0], + IsTotp: refs.NewBoolRef(true), + }) + require.Error(t, err, "a recovery code already redeemed must not authenticate again") + assert.Nil(t, res, "no token may be minted from a spent recovery code") + + // The other nine are untouched — spending one must not burn the rest, + // and must not leave them unusable either. + armMfaSession() + res, err = ts.GraphQLProvider.VerifyOTP(ctx, &model.VerifyOTPRequest{ + Email: &email, + Otp: authConfig.RecoveryCodes[1], + IsTotp: refs.NewBoolRef(true), + }) + require.NoError(t, err) + require.NotNil(t, res) + assert.NotEmpty(t, res.AccessToken, "an unspent recovery code must still work") + + // And that one is now spent too. + armMfaSession() + res, err = ts.GraphQLProvider.VerifyOTP(ctx, &model.VerifyOTPRequest{ + Email: &email, + Otp: authConfig.RecoveryCodes[1], + IsTotp: refs.NewBoolRef(true), + }) + require.Error(t, err) + assert.Nil(t, res) + }) + t.Run("invalid TOTP passcode is rejected", func(t *testing.T) { armMfaSession() res, err := ts.GraphQLProvider.VerifyOTP(ctx, &model.VerifyOTPRequest{ diff --git a/internal/storage/db/arangodb/authenticator.go b/internal/storage/db/arangodb/authenticator.go index 56d6da7e0..8a53bc04b 100644 --- a/internal/storage/db/arangodb/authenticator.go +++ b/internal/storage/db/arangodb/authenticator.go @@ -49,6 +49,35 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// document still holds oldCodes. FILTER and UPDATE are one AQL statement over a +// single document, which ArangoDB applies atomically, so RETURN NEW yields a +// document for the caller that wrote and nothing for the rest. The document is +// matched on `_id` because that is the identifier the struct carries back out +// of GetAuthenticatorDetailsByUserId — ArangoDB overwrites the field with its +// own "collection/key" value, so the ID a caller holds is never the bare UUID. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + query := fmt.Sprintf("FOR d IN %s FILTER d._id == @id AND d.recovery_codes == @old_codes UPDATE d WITH {recovery_codes: @new_codes, updated_at: @updated_at} IN %s RETURN NEW", schemas.Collections.Authenticators, schemas.Collections.Authenticators) + bindVars := map[string]interface{}{ + "id": id, + "old_codes": oldCodes, + "new_codes": newCodes, + "updated_at": time.Now().Unix(), + } + cursor, err := p.db.Query(ctx, query, bindVars) + if err != nil { + // A concurrent writer that got there first surfaces as a write-write + // conflict, which is a lost race and not a fault: the caller re-reads + // and finds the code already consumed. + if arangoDriver.IsConflict(err) { + return false, nil + } + return false, err + } + defer func() { _ = cursor.Close() }() + return cursor.HasMore(), nil +} + // DeleteAuthenticatorsByUserID removes every authenticator row for a user. // Used by admin MFA reset. func (p *provider) DeleteAuthenticatorsByUserID(ctx context.Context, userID string) error { diff --git a/internal/storage/db/cassandradb/authenticator.go b/internal/storage/db/cassandradb/authenticator.go index bd68a8b62..63ff1784d 100644 --- a/internal/storage/db/cassandradb/authenticator.go +++ b/internal/storage/db/cassandradb/authenticator.go @@ -97,6 +97,22 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// row still holds oldCodes. `id` is the partition key, so the IF clause makes +// this a lightweight transaction — Cassandra serialises it through Paxos on +// that partition and reports whether it applied, which is the one construct in +// CQL that can decide this race. MapScanCAS is used rather than ScanCAS because +// a rejected LWT returns the compared columns and the map absorbs them without +// the destination arguments having to match. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + query := fmt.Sprintf("UPDATE %s SET recovery_codes = ?, updated_at = ? WHERE id = ? IF recovery_codes = ?", KeySpace+"."+schemas.Collections.Authenticators) + applied, err := p.db.Query(query, newCodes, time.Now().Unix(), id, oldCodes).MapScanCAS(map[string]interface{}{}) + if err != nil { + return false, err + } + return applied, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { var authenticators schemas.Authenticator query := fmt.Sprintf("SELECT id, user_id, method, secret, recovery_codes, verified_at, created_at, updated_at FROM %s WHERE user_id = ? AND method = ? LIMIT 1 ALLOW FILTERING", KeySpace+"."+schemas.Collections.Authenticators) diff --git a/internal/storage/db/couchbase/authenticator.go b/internal/storage/db/couchbase/authenticator.go index fe472cf64..4a6f1b393 100644 --- a/internal/storage/db/couchbase/authenticator.go +++ b/internal/storage/db/couchbase/authenticator.go @@ -3,6 +3,7 @@ package couchbase import ( "context" "encoding/json" + "errors" "fmt" "strings" "time" @@ -72,6 +73,50 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// document still holds oldCodes. +// +// This goes through the KV API rather than N1QL on purpose. A N1QL `UPDATE ... +// WHERE recovery_codes = $old` gives no dependable way to tell "matched nothing" +// from "the statement was retried", whereas Get→Replace carries the document's +// CAS: the Replace is rejected outright if anything mutated the document between +// the two calls, which is precisely the race being closed. Both a stale blob and +// a CAS mismatch mean the same thing — another caller got there first — and both +// return (false, nil). +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + collection := p.db.Collection(schemas.Collections.Authenticators) + res, err := collection.Get(id, &gocb.GetOptions{Context: ctx}) + if err != nil { + if errors.Is(err, gocb.ErrDocumentNotFound) { + return false, nil + } + return false, err + } + var authenticator schemas.Authenticator + if err := res.Content(&authenticator); err != nil { + return false, err + } + // Decode into the schema rather than a bare map so the int64 timestamps keep + // their type on the way back out — a map round trip turns them into float64. + if authenticator.RecoveryCodes == nil || *authenticator.RecoveryCodes != oldCodes { + return false, nil + } + authenticator.RecoveryCodes = &newCodes + authenticator.UpdatedAt = time.Now().Unix() + doc, err := structToDocument(&authenticator) + if err != nil { + return false, err + } + _, err = collection.Replace(id, doc, &gocb.ReplaceOptions{Context: ctx, Cas: res.Cas()}) + if err != nil { + if errors.Is(err, gocb.ErrCasMismatch) || errors.Is(err, gocb.ErrDocumentNotFound) { + return false, nil + } + return false, err + } + return true, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { var authenticators *schemas.Authenticator query := fmt.Sprintf("SELECT _id, user_id, method, secret, recovery_codes, verified_at, created_at, updated_at FROM %s.%s WHERE user_id = $1 AND method = $2 LIMIT 1", p.scopeName, schemas.Collections.Authenticators) diff --git a/internal/storage/db/dynamodb/authenticator.go b/internal/storage/db/dynamodb/authenticator.go index 14eae3b14..2518f3ada 100644 --- a/internal/storage/db/dynamodb/authenticator.go +++ b/internal/storage/db/dynamodb/authenticator.go @@ -2,10 +2,15 @@ package dynamodb import ( "context" + "errors" "fmt" + "strconv" "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/feature/dynamodb/expression" + "github.com/aws/aws-sdk-go-v2/service/dynamodb" + "github.com/aws/aws-sdk-go-v2/service/dynamodb/types" "github.com/google/uuid" "github.com/authorizerdev/authorizer/internal/storage/schemas" @@ -37,6 +42,40 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// item still holds oldCodes. DynamoDB evaluates the ConditionExpression as part +// of the same UpdateItem, so exactly one caller writes under concurrent +// redemption and the rest get ConditionalCheckFailed — a lost race, returned as +// (false, nil) rather than an error. An item whose recovery_codes attribute is +// absent or different fails the same way, which is the intended answer. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + _, err := p.client.UpdateItem(ctx, &dynamodb.UpdateItemInput{ + TableName: aws.String(schemas.Collections.Authenticators), + Key: map[string]types.AttributeValue{ + "id": &types.AttributeValueMemberS{Value: id}, + }, + UpdateExpression: aws.String("SET #rc = :new_codes, #ua = :updated_at"), + ConditionExpression: aws.String("#rc = :old_codes"), + ExpressionAttributeNames: map[string]string{ + "#rc": "recovery_codes", + "#ua": "updated_at", + }, + ExpressionAttributeValues: map[string]types.AttributeValue{ + ":new_codes": &types.AttributeValueMemberS{Value: newCodes}, + ":old_codes": &types.AttributeValueMemberS{Value: oldCodes}, + ":updated_at": &types.AttributeValueMemberN{Value: strconv.FormatInt(time.Now().Unix(), 10)}, + }, + }) + if err != nil { + var ccf *types.ConditionalCheckFailedException + if errors.As(err, &ccf) { + return false, nil + } + return false, err + } + return true, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { f := expression.Name("user_id").Equal(expression.Value(userId)).And(expression.Name("method").Equal(expression.Value(authenticatorType))) items, err := p.scanFilteredAll(ctx, schemas.Collections.Authenticators, nil, &f) diff --git a/internal/storage/db/mongodb/authenticator.go b/internal/storage/db/mongodb/authenticator.go index 383948062..7a9d7cd58 100644 --- a/internal/storage/db/mongodb/authenticator.go +++ b/internal/storage/db/mongodb/authenticator.go @@ -49,6 +49,22 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// row still holds oldCodes. The expected blob is part of the UpdateOne filter, +// and a single-document update is atomic in MongoDB, so MatchedCount is 1 for +// exactly one caller under concurrent redemption and 0 for the rest. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + authenticatorsCollection := p.db.Collection(schemas.Collections.Authenticators, options.Collection()) + res, err := authenticatorsCollection.UpdateOne(ctx, + bson.M{"_id": id, "recovery_codes": oldCodes}, + bson.M{"$set": bson.M{"recovery_codes": newCodes, "updated_at": time.Now().Unix()}}, + ) + if err != nil { + return false, err + } + return res.MatchedCount == 1, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { var authenticators *schemas.Authenticator authenticatorsCollection := p.db.Collection(schemas.Collections.Authenticators, options.Collection()) diff --git a/internal/storage/db/provider_template/authenticator.go b/internal/storage/db/provider_template/authenticator.go index d0f3ef61d..53b8fd84e 100644 --- a/internal/storage/db/provider_template/authenticator.go +++ b/internal/storage/db/provider_template/authenticator.go @@ -28,6 +28,19 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// row still holds oldCodes, and reports whether THIS call performed the write. +// +// Implement it with ONE conditional statement — a WHERE/filter on the expected +// blob combined with the write, or the backend's compare-and-swap. Never read +// the row, compare in Go, and then write unconditionally: that is the bug this +// method exists to remove, and it lets a single TOTP recovery code be redeemed +// by any number of concurrent requests. A refused write is (false, nil), not an +// error. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + return false, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { var authenticators *schemas.Authenticator return authenticators, nil diff --git a/internal/storage/db/sql/authenticator.go b/internal/storage/db/sql/authenticator.go index e6e854f12..2681e86fe 100644 --- a/internal/storage/db/sql/authenticator.go +++ b/internal/storage/db/sql/authenticator.go @@ -46,6 +46,23 @@ func (p *provider) UpdateAuthenticator(ctx context.Context, authenticators *sche return authenticators, nil } +// ConsumeAuthenticatorRecoveryCode swaps the recovery-code blob only while the +// row still holds oldCodes. The WHERE clause and the SET are one statement, so +// the row lock the UPDATE takes decides the race: RowsAffected is 1 for the +// caller that wrote and 0 for everyone whose expected blob was already stale. +func (p *provider) ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) { + res := p.db.WithContext(ctx).Model(&schemas.Authenticator{}). + Where("id = ?", id).Where("recovery_codes = ?", oldCodes). + Updates(map[string]any{ + "recovery_codes": newCodes, + "updated_at": time.Now().Unix(), + }) + if res.Error != nil { + return false, res.Error + } + return res.RowsAffected == 1, nil +} + func (p *provider) GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) { var authenticators schemas.Authenticator result := p.db.WithContext(ctx).Where("user_id = ?", userId).Where("method = ?", authenticatorType).First(&authenticators) diff --git a/internal/storage/provider.go b/internal/storage/provider.go index 1da4e12c9..6ad31d2f5 100644 --- a/internal/storage/provider.go +++ b/internal/storage/provider.go @@ -161,6 +161,32 @@ type Provider interface { // UpdateAuthenticator updates an existing authenticator document in the database. // The updated document is returned, or an error if the operation fails. UpdateAuthenticator(ctx context.Context, totp *schemas.Authenticator) (*schemas.Authenticator, error) + // ConsumeAuthenticatorRecoveryCode replaces the recovery-code blob of the + // authenticator row with the given ID, but only while the row still holds + // oldCodes. The bool reports whether THIS call performed the write, and it + // MUST be decided by a single atomic database operation — never by a + // separate read followed by an unconditional update. + // + // This is the single-use primitive behind TOTP recovery codes. The caller + // reads the blob, marks one code consumed, and offers the before/after pair + // here; if anything changed the blob in between, the write is refused and + // the caller re-reads. A read-then-write implementation lets two concurrent + // redemptions of the SAME recovery code both observe it unconsumed and both + // succeed, so one code authenticates any number of times — which is the + // whole property a recovery code is supposed to have. + // + // oldCodes MUST be the exact string read from the row, byte for byte, never + // a re-marshalled map: re-encoding can reorder keys or change spacing and + // then the comparison matches nothing and every redemption fails. + // + // A refused write is not an error — it returns (false, nil), and that + // includes the case where the row no longer exists. + // + // FAULT TOLERANCE: on error the bool is always false. Callers MUST check the + // error first and treat it as "claim outcome unknown", never as "not + // consumed" — reporting a database outage as an invalid recovery code + // burns the user's credential for nothing. + ConsumeAuthenticatorRecoveryCode(ctx context.Context, id, oldCodes, newCodes string) (bool, error) // GetAuthenticatorDetailsByUserId retrieves details of an authenticator document based on user ID and authenticator type. // If found, the authenticator document is returned, or an error if not found or an error occurs during the retrieval. GetAuthenticatorDetailsByUserId(ctx context.Context, userId string, authenticatorType string) (*schemas.Authenticator, error) diff --git a/internal/storage/provider_test.go b/internal/storage/provider_test.go index 3c0774f43..92fe1ed18 100644 --- a/internal/storage/provider_test.go +++ b/internal/storage/provider_test.go @@ -932,6 +932,48 @@ func testAuthenticatorOperations(t *testing.T, ctx context.Context, provider Pro require.NoError(t, err) assert.Equal(t, "updated_secret", afterDup.Secret, "second enrollment must not create a divergent duplicate") } + + // ConsumeAuthenticatorRecoveryCode is the single-use primitive behind TOTP + // recovery codes, so every backend has to decide the same race the same + // way: the swap lands only while the row still holds the blob the caller + // read, a stale expectation is refused WITHOUT writing, and a refusal is + // (false, nil) rather than an error. Each backend implements this with a + // different construct — WHERE clause, LWT, ConditionExpression, document + // CAS — so a divergence here is a silent double-spend on one database + // only. Runs last: it rewrites recovery_codes. + t.Run("ConsumeAuthenticatorRecoveryCode", func(t *testing.T) { + base, err := provider.GetAuthenticatorDetailsByUserId(ctx, auth.UserID, constants.EnvKeyTOTPAuthenticator) + require.NoError(t, err) + require.NotNil(t, base.RecoveryCodes) + oldCodes := *base.RecoveryCodes + + read := func(t *testing.T) string { + t.Helper() + row, err := provider.GetAuthenticatorDetailsByUserId(ctx, auth.UserID, constants.EnvKeyTOTPAuthenticator) + require.NoError(t, err) + require.NotNil(t, row.RecoveryCodes) + return *row.RecoveryCodes + } + + claimed, err := provider.ConsumeAuthenticatorRecoveryCode(ctx, base.ID, "not-what-the-row-holds", `{"stale":true}`) + require.NoError(t, err, "a refused swap is a lost race, not a fault") + assert.False(t, claimed) + assert.Equal(t, oldCodes, read(t), "a refused swap must not write") + + claimed, err = provider.ConsumeAuthenticatorRecoveryCode(ctx, base.ID, oldCodes, `{"consumed":true}`) + require.NoError(t, err) + assert.True(t, claimed, "a swap matching the current blob must land") + assert.Equal(t, `{"consumed":true}`, read(t), "the winner's blob must be stored verbatim") + + claimed, err = provider.ConsumeAuthenticatorRecoveryCode(ctx, base.ID, oldCodes, `{"replayed":true}`) + require.NoError(t, err) + assert.False(t, claimed, "the same before-blob must not win twice") + assert.Equal(t, `{"consumed":true}`, read(t)) + + claimed, err = provider.ConsumeAuthenticatorRecoveryCode(ctx, uuid.New().String(), oldCodes, `{"orphan":true}`) + require.NoError(t, err, "a row that does not exist must not surface as a fault") + assert.False(t, claimed) + }) } func testSessionTokenOperations(t *testing.T, ctx context.Context, provider Provider) {