Merge pull request 'feat: migrate TOTP secrets to keying' (#6074) from gusted/forgejo-totp-keying into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6074
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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"
 
@@ -49,9 +46,9 @@ func (err ErrTwoFactorNotEnrolled) Unwrap() error {
 
 // TwoFactor represents a two-factor authentication token.
 type TwoFactor struct {
-	ID               int64 `xorm:"pk autoincr"`
-	UID              int64 `xorm:"UNIQUE"`
-	Secret           string
+	ID               int64  `xorm:"pk autoincr"`
+	UID              int64  `xorm:"UNIQUE"`
+	Secret           []byte `xorm:"BLOB"`
 	ScratchSalt      string
 	ScratchHash      string
 	LastUsedPasscode string             `xorm:"VARCHAR(10)"`
@@ -92,39 +89,35 @@ 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)
-	return err
+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.

models/fixtures/two_factor.yml

@@ -1,9 +1,9 @@
 -
-  id: 1
-  uid: 24
-  secret: KlDporn6Ile4vFcKI8z7Z6sqK1Scj2Qp0ovtUzCZO6jVbRW2lAoT7UDxDPtrab8d2B9zKOocBRdBJnS8orsrUNrsyETY+jJHb79M82uZRioKbRUz15sfOpmJmEzkFeSg6S4LicUBQos=
-  scratch_salt: Qb5bq2DyR2
-  scratch_hash: 068eb9b8746e0bcfe332fac4457693df1bda55800eb0f6894d14ebb736ae6a24e0fc8fc5333c19f57f81599788f0b8e51ec1
-  last_used_passcode:
-  created_unix: 1564253724
-  updated_unix: 1564253724
+   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

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
 
-// Used for the `push_mirror` table.
-var ContextPushMirror Context = "pushmirror"
+var (
+	// Used for the `push_mirror` table.
+	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)