feat: migrate TOTP secrets to keying

- Currently the TOTP secrets are stored using the `secrets` module with
as key the MD5 hash of the Secretkey, the `secrets` module uses general
bad practices. This patch migrates the secrets to use the `keying`
module (#5041) which is easier to use and use better practices to store
secrets in databases.
- Migration test added.
- Remove the Forgejo migration databases, and let the gitea migration
databases also run forgejo migration databases. This is required as the
Forgejo migration is now also touching tables that the forgejo migration
didn't create itself.

models/auth/twofactor.go

@@ -5,17 +5,14 @@ package auth
 
 import (
 	"context"
-	"crypto/md5"
 	"crypto/sha256"
 	"crypto/subtle"
 	"encoding/base32"
-	"encoding/base64"
 	"encoding/hex"
 	"fmt"
 
 	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/modules/secret"
-	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/keying"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 
@@ -51,7 +48,7 @@ func (err ErrTwoFactorNotEnrolled) Unwrap() error {
 type TwoFactor struct {
 	ID               int64  `xorm:"pk autoincr"`
 	UID              int64  `xorm:"UNIQUE"`
-	Secret           string
+	Secret           []byte `xorm:"BLOB"`
 	ScratchSalt      string
 	ScratchHash      string
 	LastUsedPasscode string             `xorm:"VARCHAR(10)"`
@@ -92,41 +89,37 @@ 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[:]
-}
-
 // SetSecret sets the 2FA secret.
-func (t *TwoFactor) SetSecret(secretString string) error {
-	secretBytes, err := secret.AesEncrypt(t.getEncryptionKey(), []byte(secretString))
-	if err != nil {
-		return err
-	}
-	t.Secret = base64.StdEncoding.EncodeToString(secretBytes)
-	return nil
+func (t *TwoFactor) SetSecret(secretString string) {
+	key := keying.DeriveKey(keying.ContextTOTP)
+	t.Secret = key.Encrypt([]byte(secretString), keying.ColumnAndID("secret", t.ID))
 }
 
 // ValidateTOTP validates the provided passcode.
 func (t *TwoFactor) ValidateTOTP(passcode string) (bool, error) {
-	decodedStoredSecret, err := base64.StdEncoding.DecodeString(t.Secret)
+	key := keying.DeriveKey(keying.ContextTOTP)
+	secret, err := key.Decrypt(t.Secret, keying.ColumnAndID("secret", t.ID))
 	if err != nil {
 		return false, err
 	}
-	secretBytes, err := secret.AesDecrypt(t.getEncryptionKey(), decodedStoredSecret)
-	if err != nil {
-		return false, err
-	}
-	secretStr := string(secretBytes)
-	return totp.Validate(passcode, secretStr), nil
+	return totp.Validate(passcode, string(secret)), nil
 }
 
 // NewTwoFactor creates a new two-factor authentication token.
-func NewTwoFactor(ctx context.Context, t *TwoFactor) error {
-	_, err := db.GetEngine(ctx).Insert(t)
+func NewTwoFactor(ctx context.Context, t *TwoFactor, secret string) error {
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		sess := db.GetEngine(ctx)
+		_, err := sess.Insert(t)
+		if err != nil {
 			return err
 		}
 
+		t.SetSecret(secret)
+		_, err = sess.Cols("secret").ID(t.ID).Update(t)
+		return err
+	})
+}
+
 // UpdateTwoFactor updates a two-factor authentication token.
 func UpdateTwoFactor(ctx context.Context, t *TwoFactor) error {
 	_, err := db.GetEngine(ctx).ID(t.ID).AllCols().Update(t)

models/fixtures/two_factor.yml

@@ -1,7 +1,7 @@
 -
    id: 1
    uid: 24
-  secret: KlDporn6Ile4vFcKI8z7Z6sqK1Scj2Qp0ovtUzCZO6jVbRW2lAoT7UDxDPtrab8d2B9zKOocBRdBJnS8orsrUNrsyETY+jJHb79M82uZRioKbRUz15sfOpmJmEzkFeSg6S4LicUBQos=
+   secret: MrAed+7K+fKQKu1l3aU45oTDSWK/i5Ugtgk8CmORrKWTMwa2w97rniLU+h+2xq8ZF+16uuXGLzjWa0bOV5xg4NY6w5Ec/tkwQ5rEecOTvc/JZV5lrrlDi48B7Y5/lNcjAWBmH2nEUlM=
    scratch_salt: Qb5bq2DyR2
    scratch_hash: 068eb9b8746e0bcfe332fac4457693df1bda55800eb0f6894d14ebb736ae6a24e0fc8fc5333c19f57f81599788f0b8e51ec1
    last_used_passcode:

models/forgejo_migrations/migrate.go

@@ -86,6 +86,8 @@ var migrations = []*Migration{
 	NewMigration("Add `delete_branch_after_merge` to `auto_merge` table", AddDeleteBranchAfterMergeToAutoMerge),
 	// v24 -> v25
 	NewMigration("Add `purpose` column to `forgejo_auth_token` table", AddPurposeToForgejoAuthToken),
+	// v25 -> v26
+	NewMigration("Migrate `secret` column to store keying material", MigrateTwoFactorToKeying),
 }
 
 // GetCurrentDBVersion returns the current Forgejo database version.

models/forgejo_migrations/v25.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package forgejo_migrations //nolint:revive
+
+import (
+	"context"
+	"crypto/md5"
+	"encoding/base64"
+
+	"code.gitea.io/gitea/models/auth"
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/secret"
+	"code.gitea.io/gitea/modules/setting"
+
+	"xorm.io/xorm"
+	"xorm.io/xorm/schemas"
+)
+
+func MigrateTwoFactorToKeying(x *xorm.Engine) error {
+	var err error
+
+	switch x.Dialect().URI().DBType {
+	case schemas.MYSQL:
+		_, err = x.Exec("ALTER TABLE `two_factor` MODIFY `secret` BLOB")
+	case schemas.POSTGRES:
+		_, err = x.Exec("ALTER TABLE `two_factor` ALTER COLUMN `secret` SET DATA TYPE bytea USING secret::text::bytea")
+	}
+	if err != nil {
+		return err
+	}
+
+	oldEncryptionKey := md5.Sum([]byte(setting.SecretKey))
+
+	return db.Iterate(context.Background(), nil, func(ctx context.Context, bean *auth.TwoFactor) error {
+		decodedStoredSecret, err := base64.StdEncoding.DecodeString(string(bean.Secret))
+		if err != nil {
+			return err
+		}
+
+		secretBytes, err := secret.AesDecrypt(oldEncryptionKey[:], decodedStoredSecret)
+		if err != nil {
+			return err
+		}
+
+		bean.SetSecret(string(secretBytes))
+		_, err = db.GetEngine(ctx).Cols("secret").ID(bean.ID).Update(bean)
+		return err
+	})
+}

models/forgejo_migrations/v25_test.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package forgejo_migrations //nolint:revive
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/auth"
+	migration_tests "code.gitea.io/gitea/models/migrations/test"
+	"code.gitea.io/gitea/modules/keying"
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_MigrateTwoFactorToKeying(t *testing.T) {
+	type TwoFactor struct { //revive:disable-line:exported
+		ID               int64 `xorm:"pk autoincr"`
+		UID              int64 `xorm:"UNIQUE"`
+		Secret           string
+		ScratchSalt      string
+		ScratchHash      string
+		LastUsedPasscode string             `xorm:"VARCHAR(10)"`
+		CreatedUnix      timeutil.TimeStamp `xorm:"INDEX created"`
+		UpdatedUnix      timeutil.TimeStamp `xorm:"INDEX updated"`
+	}
+
+	// Prepare and load the testing database
+	x, deferable := migration_tests.PrepareTestEnv(t, 0, new(TwoFactor))
+	defer deferable()
+	if x == nil || t.Failed() {
+		return
+	}
+
+	cnt, err := x.Table("two_factor").Count()
+	require.NoError(t, err)
+	assert.EqualValues(t, 1, cnt)
+
+	require.NoError(t, MigrateTwoFactorToKeying(x))
+
+	var twofactor auth.TwoFactor
+	_, err = x.Table("two_factor").ID(1).Get(&twofactor)
+	require.NoError(t, err)
+
+	secretBytes, err := keying.DeriveKey(keying.ContextTOTP).Decrypt(twofactor.Secret, keying.ColumnAndID("secret", twofactor.ID))
+	require.NoError(t, err)
+	assert.Equal(t, []byte("AVDYS32OPIAYSNBG2NKYV4AHBVEMKKKIGBQ46OXTLMJO664G4TIECOGEANMSNBLS"), secretBytes)
+}

models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml

@@ -0,0 +1,9 @@
+-
+  id: 1
+  uid: 24
+  secret: MrAed+7K+fKQKu1l3aU45oTDSWK/i5Ugtgk8CmORrKWTMwa2w97rniLU+h+2xq8ZF+16uuXGLzjWa0bOV5xg4NY6w5Ec/tkwQ5rEecOTvc/JZV5lrrlDi48B7Y5/lNcjAWBmH2nEUlM=
+  scratch_salt: Qb5bq2DyR2
+  scratch_hash: 068eb9b8746e0bcfe332fac4457693df1bda55800eb0f6894d14ebb736ae6a24e0fc8fc5333c19f57f81599788f0b8e51ec1
+  last_used_passcode:
+  created_unix: 1564253724
+  updated_unix: 1564253724

modules/keying/keying.go

@@ -45,8 +45,12 @@ func Init(ikm []byte) {
 // This must be a hardcoded string and must not be arbitrarily constructed.
 type Context string
 
+var (
 	// Used for the `push_mirror` table.
-var ContextPushMirror Context = "pushmirror"
+	ContextPushMirror Context = "pushmirror"
+	// Used for the `two_factor` table.
+	ContextTOTP Context = "totp"
+)
 
 // Derive *the* key for a given context, this is a determistic function. The
 // same key will be provided for the same context.

routers/web/user/setting/security/2fa.go

@@ -220,11 +220,6 @@ func EnrollTwoFactorPost(ctx *context.Context) {
 	t = &auth.TwoFactor{
 		UID: ctx.Doer.ID,
 	}
-	err = t.SetSecret(secret)
-	if err != nil {
-		ctx.ServerError("SettingsTwoFactor: Failed to set secret", err)
-		return
-	}
 	token, err := t.GenerateScratchToken()
 	if err != nil {
 		ctx.ServerError("SettingsTwoFactor: Failed to generate scratch token", err)
@@ -251,7 +246,7 @@ func EnrollTwoFactorPost(ctx *context.Context) {
 		return
 	}
 
-	if err = auth.NewTwoFactor(ctx, t); err != nil {
+	if err = auth.NewTwoFactor(ctx, t, secret); err != nil {
 		// FIXME: We need to handle a unique constraint fail here it's entirely possible that another request has beaten us.
 		// If there is a unique constraint fail we should just tolerate the error
 		ctx.ServerError("SettingsTwoFactor: Failed to save two factor", err)

tests/integration/api_twofa_test.go

@@ -38,9 +38,8 @@ func TestAPITwoFactor(t *testing.T) {
 	tfa := &auth_model.TwoFactor{
 		UID: user.ID,
 	}
-	require.NoError(t, tfa.SetSecret(otpKey.Secret()))
 
-	require.NoError(t, auth_model.NewTwoFactor(db.DefaultContext, tfa))
+	require.NoError(t, auth_model.NewTwoFactor(db.DefaultContext, tfa, otpKey.Secret()))
 
 	req = NewRequest(t, "GET", "/api/v1/user").
 		AddBasicAuth(user.Name)