From 64b57aa2cc51b7faeee730e5b8b7ca1c7315e9fe Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 23 Mar 2026 13:52:07 -0700 Subject: [PATCH 1/3] Use pbkdf2 instead of md5, add salt in totp --- models/auth/twofactor.go | 58 ++++++++++++++++++++---- models/auth/twofactor_test.go | 68 ++++++++++++++++++++++++++++ models/migrations/migrations.go | 1 + models/migrations/v1_26/v330.go | 21 +++++++++ models/migrations/v1_26/v330_test.go | 31 +++++++++++++ routers/web/auth/2fa.go | 2 +- routers/web/auth/password.go | 2 +- services/auth/basic.go | 11 ++++- 8 files changed, 181 insertions(+), 13 deletions(-) create mode 100644 models/auth/twofactor_test.go create mode 100644 models/migrations/v1_26/v330.go create mode 100644 models/migrations/v1_26/v330_test.go diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index 4263495650f23..d6cdc4f60151b 100644 --- a/models/auth/twofactor.go +++ b/models/auth/twofactor.go @@ -52,6 +52,8 @@ type TwoFactor struct { ID int64 `xorm:"pk autoincr"` UID int64 `xorm:"UNIQUE"` Secret string + SecretSalt string + SecretAlgo string ScratchSalt string ScratchHash string LastUsedPasscode string `xorm:"VARCHAR(10)"` @@ -92,14 +94,38 @@ func (t *TwoFactor) VerifyScratchToken(token string) bool { return subtle.ConstantTimeCompare([]byte(t.ScratchHash), []byte(tempHash)) == 1 } -func (t *TwoFactor) getEncryptionKey() []byte { - k := md5.Sum([]byte(setting.SecretKey)) - return k[:] +const ( + totpSecretKeyIterations = 10000 + totpSecretKeyLength = 32 + totpSecretSaltSize = 16 +) + +func (t *TwoFactor) getEncryptionKey() ([]byte, bool) { + if t.SecretAlgo == "" || t.SecretAlgo == "md5" || t.SecretSalt == "" { + k := md5.Sum([]byte(setting.SecretKey)) + return k[:], true + } + key := pbkdf2.Key([]byte(setting.SecretKey), []byte(t.SecretSalt), totpSecretKeyIterations, totpSecretKeyLength, sha256.New) + return key, false +} + +func (t *TwoFactor) rotateSecretSalt() error { + saltBytes, err := util.CryptoRandomBytes(totpSecretSaltSize) + if err != nil { + return err + } + t.SecretSalt = hex.EncodeToString(saltBytes) + t.SecretAlgo = "pbkdf2" + return nil } // SetSecret sets the 2FA secret. func (t *TwoFactor) SetSecret(secretString string) error { - secretBytes, err := secret.AesEncrypt(t.getEncryptionKey(), []byte(secretString)) + if err := t.rotateSecretSalt(); err != nil { + return err + } + key, _ := t.getEncryptionKey() + secretBytes, err := secret.AesEncrypt(key, []byte(secretString)) if err != nil { return err } @@ -108,17 +134,31 @@ func (t *TwoFactor) SetSecret(secretString string) error { } // ValidateTOTP validates the provided passcode. -func (t *TwoFactor) ValidateTOTP(passcode string) (bool, error) { +func (t *TwoFactor) ValidateTOTP(passcode string) (bool, bool, error) { decodedStoredSecret, err := base64.StdEncoding.DecodeString(t.Secret) if err != nil { - return false, fmt.Errorf("ValidateTOTP invalid base64: %w", err) + return false, false, fmt.Errorf("ValidateTOTP invalid base64: %w", err) } - secretBytes, err := secret.AesDecrypt(t.getEncryptionKey(), decodedStoredSecret) + key, legacyKey := t.getEncryptionKey() + secretBytes, err := secret.AesDecrypt(key, decodedStoredSecret) if err != nil { - return false, fmt.Errorf("ValidateTOTP unable to decrypt (maybe SECRET_KEY is wrong): %w", err) + return false, false, fmt.Errorf("ValidateTOTP unable to decrypt (maybe SECRET_KEY is wrong): %w", err) } secretStr := string(secretBytes) - return totp.Validate(passcode, secretStr), nil + ok := totp.Validate(passcode, secretStr) + if ok && legacyKey { + if err := t.rotateSecretSalt(); err != nil { + return ok, false, err + } + key, _ = t.getEncryptionKey() + secretBytes, err = secret.AesEncrypt(key, []byte(secretStr)) + if err != nil { + return ok, false, err + } + t.Secret = base64.StdEncoding.EncodeToString(secretBytes) + return ok, true, nil + } + return ok, false, nil } // NewTwoFactor creates a new two-factor authentication token. diff --git a/models/auth/twofactor_test.go b/models/auth/twofactor_test.go new file mode 100644 index 0000000000000..912de9502c5be --- /dev/null +++ b/models/auth/twofactor_test.go @@ -0,0 +1,68 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth_test + +import ( + "crypto/md5" + "encoding/base64" + "testing" + "time" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/modules/secret" + "code.gitea.io/gitea/modules/setting" + + "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTwoFactorSetSecretUsesPBKDF2(t *testing.T) { + oldSecretKey := setting.SecretKey + setting.SecretKey = "twofactor-test-secret" + defer func() { + setting.SecretKey = oldSecretKey + }() + + secretStr := "JBSWY3DPEHPK3PXP" + twofa := &auth_model.TwoFactor{} + require.NoError(t, twofa.SetSecret(secretStr)) + + assert.NotEmpty(t, twofa.SecretSalt) + assert.Equal(t, "pbkdf2", twofa.SecretAlgo) + + passcode, err := totp.GenerateCode(secretStr, time.Now()) + require.NoError(t, err) + + ok, upgraded, err := twofa.ValidateTOTP(passcode) + require.NoError(t, err) + assert.True(t, ok) + assert.False(t, upgraded) +} + +func TestTwoFactorLegacySecretUpgrade(t *testing.T) { + oldSecretKey := setting.SecretKey + setting.SecretKey = "twofactor-test-secret" + defer func() { + setting.SecretKey = oldSecretKey + }() + + secretStr := "JBSWY3DPEHPK3PXP" + legacyKey := md5.Sum([]byte(setting.SecretKey)) + ciphertext, err := secret.AesEncrypt(legacyKey[:], []byte(secretStr)) + require.NoError(t, err) + legacySecret := base64.StdEncoding.EncodeToString(ciphertext) + + twofa := &auth_model.TwoFactor{Secret: legacySecret} + passcode, err := totp.GenerateCode(secretStr, time.Now()) + require.NoError(t, err) + + ok, upgraded, err := twofa.ValidateTOTP(passcode) + require.NoError(t, err) + assert.True(t, ok) + assert.True(t, upgraded) + assert.NotEmpty(t, twofa.SecretSalt) + assert.Equal(t, "pbkdf2", twofa.SecretAlgo) + assert.NotEqual(t, legacySecret, twofa.Secret) +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index bb8dad5ec6d8a..0d7622c125d10 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -404,6 +404,7 @@ func prepareMigrationTasks() []*migration { newMigration(327, "Add disabled state to action runners", v1_26.AddDisabledToActionRunner), newMigration(328, "Add TokenPermissions column to ActionRunJob", v1_26.AddTokenPermissionsToActionRunJob), newMigration(329, "Add unique constraint for user badge", v1_26.AddUniqueIndexForUserBadge), + newMigration(330, "Add secret_salt and secret_algo to two_factor", v1_26.AddSecretSaltToTwoFactor), } return preparedMigrations } diff --git a/models/migrations/v1_26/v330.go b/models/migrations/v1_26/v330.go new file mode 100644 index 0000000000000..facafb3582001 --- /dev/null +++ b/models/migrations/v1_26/v330.go @@ -0,0 +1,21 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_26 + +import "xorm.io/xorm" + +// AddSecretSaltToTwoFactor adds a per-user salt column for TOTP secrets. +func AddSecretSaltToTwoFactor(x *xorm.Engine) error { + type TwoFactor struct { + SecretSalt string + SecretAlgo string + } + + if err := x.Sync(new(TwoFactor)); err != nil { + return err + } + + _, err := x.Exec("UPDATE two_factor SET secret_algo = 'md5' WHERE secret_algo = '' OR secret_algo IS NULL") + return err +} diff --git a/models/migrations/v1_26/v330_test.go b/models/migrations/v1_26/v330_test.go new file mode 100644 index 0000000000000..bf877ed898012 --- /dev/null +++ b/models/migrations/v1_26/v330_test.go @@ -0,0 +1,31 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_26 + +import ( + "testing" + + "code.gitea.io/gitea/models/migrations/base" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_AddSecretSaltToTwoFactor(t *testing.T) { + type TwoFactor struct { + ID int64 `xorm:"pk autoincr"` + UID int64 `xorm:"UNIQUE"` + Secret string + } + + x, deferrable := base.PrepareTestEnv(t, 0, new(TwoFactor)) + defer deferrable() + + require.NoError(t, AddSecretSaltToTwoFactor(x)) + + table := base.LoadTableSchemasMap(t, x)["two_factor"] + require.NotNil(t, table) + assert.NotNil(t, table.GetColumn("secret_salt")) + assert.NotNil(t, table.GetColumn("secret_algo")) +} diff --git a/routers/web/auth/2fa.go b/routers/web/auth/2fa.go index 73b218d92dfa6..0c7b38e99d8ab 100644 --- a/routers/web/auth/2fa.go +++ b/routers/web/auth/2fa.go @@ -59,7 +59,7 @@ func TwoFactorPost(ctx *context.Context) { } // Validate the passcode with the stored TOTP secret. - ok, err := twofa.ValidateTOTP(form.Passcode) + ok, _, err := twofa.ValidateTOTP(form.Passcode) if err != nil { ctx.ServerError("UserSignIn", err) return diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 11e085f5b1e9c..5b35c8aba4a89 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -177,7 +177,7 @@ func ResetPasswdPost(ctx *context.Context) { regenerateScratchToken = true } else { passcode := ctx.FormString("passcode") - ok, err := twofa.ValidateTOTP(passcode) + ok, _, err := twofa.ValidateTOTP(passcode) if err != nil { ctx.HTTPError(http.StatusInternalServerError, "ValidateTOTP", err.Error()) return diff --git a/services/auth/basic.go b/services/auth/basic.go index dda6451c3652e..cadefd3e6badc 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -175,11 +175,18 @@ func validateTOTP(req *http.Request, u *user_model.User) error { } return err } - if ok, err := twofa.ValidateTOTP(req.Header.Get("X-Gitea-OTP")); err != nil { + ok, upgraded, err := twofa.ValidateTOTP(req.Header.Get("X-Gitea-OTP")) + if err != nil { return err - } else if !ok { + } + if !ok { return util.NewInvalidArgumentErrorf("invalid provided OTP") } + if upgraded { + if err := auth_model.UpdateTwoFactor(req.Context(), twofa); err != nil { + return err + } + } return nil } From 96369d250fa98351684104939e7029e6a2d0da92 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 23 Mar 2026 14:02:10 -0700 Subject: [PATCH 2/3] Fix checks --- models/auth/twofactor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index d6cdc4f60151b..5eea884c51124 100644 --- a/models/auth/twofactor.go +++ b/models/auth/twofactor.go @@ -97,7 +97,7 @@ func (t *TwoFactor) VerifyScratchToken(token string) bool { const ( totpSecretKeyIterations = 10000 totpSecretKeyLength = 32 - totpSecretSaltSize = 16 + totpSecretSaltSize = 16 ) func (t *TwoFactor) getEncryptionKey() ([]byte, bool) { From 93b026b43353dbd6323cd1c964fdc7463a4df338 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 23 Mar 2026 16:39:38 -0700 Subject: [PATCH 3/3] Update models/auth/twofactor.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lunny Xiao --- models/auth/twofactor.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index 5eea884c51124..3ee2427245cc0 100644 --- a/models/auth/twofactor.go +++ b/models/auth/twofactor.go @@ -134,6 +134,15 @@ func (t *TwoFactor) SetSecret(secretString string) error { } // ValidateTOTP validates the provided passcode. +// +// It returns three values: ok indicates whether the passcode is valid, upgraded +// indicates whether a legacy-stored secret was successfully validated and then +// re-encrypted using the current secret storage scheme, and err reports any +// error encountered during validation. +// +// When upgraded is true, this method may mutate t.SecretSalt, t.SecretAlgo, and +// t.Secret to store the secret using the current algorithm. Callers must persist +// the updated TwoFactor model to ensure the upgraded secret is saved. func (t *TwoFactor) ValidateTOTP(passcode string) (bool, bool, error) { decodedStoredSecret, err := base64.StdEncoding.DecodeString(t.Secret) if err != nil {