-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Use pbkdf2 instead of md5, add salt in totp #36966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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,40 @@ func (t *TwoFactor) SetSecret(secretString string) error { | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // ValidateTOTP validates the provided passcode. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| func (t *TwoFactor) ValidateTOTP(passcode string) (bool, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+160
to
+165
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return ok, false, err | |
| } | |
| key, _ = t.getEncryptionKey() | |
| secretBytes, err = secret.AesEncrypt(key, []byte(secretStr)) | |
| if err != nil { | |
| return ok, false, err | |
| return false, false, err | |
| } | |
| key, _ = t.getEncryptionKey() | |
| secretBytes, err = secret.AesEncrypt(key, []byte(secretStr)) | |
| if err != nil { | |
| return false, false, err |
Copilot
AI
Mar 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, if re-encryption of the legacy secret fails, this returns ok (true) with a non-nil error. Prefer returning false for ok whenever err is non-nil to keep the API contract consistent and reduce the risk of misuse by future callers.
| return ok, false, err | |
| } | |
| key, _ = t.getEncryptionKey() | |
| secretBytes, err = secret.AesEncrypt(key, []byte(secretStr)) | |
| if err != nil { | |
| return ok, false, err | |
| return false, false, err | |
| } | |
| key, _ = t.getEncryptionKey() | |
| secretBytes, err = secret.AesEncrypt(key, []byte(secretStr)) | |
| if err != nil { | |
| return false, false, err |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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")) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.