diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index d0c341a192..67be23320e 100644 --- a/models/auth/twofactor.go +++ b/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. diff --git a/models/fixtures/two_factor.yml b/models/fixtures/two_factor.yml index d8cb85274b..bca1109ea8 100644 --- a/models/fixtures/two_factor.yml +++ b/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 diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 53159e31f2..980f649f41 100644 --- a/models/forgejo_migrations/migrate.go +++ b/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. diff --git a/models/forgejo_migrations/v25.go b/models/forgejo_migrations/v25.go new file mode 100644 index 0000000000..2e9641929c --- /dev/null +++ b/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 + }) +} diff --git a/models/forgejo_migrations/v25_test.go b/models/forgejo_migrations/v25_test.go new file mode 100644 index 0000000000..43c5885c39 --- /dev/null +++ b/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) +} diff --git a/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml b/models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml new file mode 100644 index 0000000000..ef6c158659 --- /dev/null +++ b/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 diff --git a/modules/keying/keying.go b/modules/keying/keying.go index 7c595c7f92..d7dfe0763c 100644 --- a/modules/keying/keying.go +++ b/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. diff --git a/routers/web/user/setting/security/2fa.go b/routers/web/user/setting/security/2fa.go index 7c85c0e4b7..37ccb5e5c4 100644 --- a/routers/web/user/setting/security/2fa.go +++ b/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) diff --git a/tests/integration/api_twofa_test.go b/tests/integration/api_twofa_test.go index fb1d2badfc..9de096ed2f 100644 --- a/tests/integration/api_twofa_test.go +++ b/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) diff --git a/tests/integration/migration-test/forgejo-v1.19.0.mysql.sql.gz b/tests/integration/migration-test/forgejo-v1.19.0.mysql.sql.gz deleted file mode 100644 index 4cea13baf8..0000000000 Binary files a/tests/integration/migration-test/forgejo-v1.19.0.mysql.sql.gz and /dev/null differ diff --git a/tests/integration/migration-test/forgejo-v1.19.0.postgres.sql.gz b/tests/integration/migration-test/forgejo-v1.19.0.postgres.sql.gz deleted file mode 100644 index 7fdc409dbb..0000000000 Binary files a/tests/integration/migration-test/forgejo-v1.19.0.postgres.sql.gz and /dev/null differ diff --git a/tests/integration/migration-test/forgejo-v1.19.0.sqlite3.sql.gz b/tests/integration/migration-test/forgejo-v1.19.0.sqlite3.sql.gz deleted file mode 100644 index dba4bafbdc..0000000000 Binary files a/tests/integration/migration-test/forgejo-v1.19.0.sqlite3.sql.gz and /dev/null differ diff --git a/tests/integration/migration-test/gitea-v1.6.4.mysql.sql.gz b/tests/integration/migration-test/gitea-v1.6.4.mysql.sql.gz index 30cca8b382..fdbd7ff7be 100644 Binary files a/tests/integration/migration-test/gitea-v1.6.4.mysql.sql.gz and b/tests/integration/migration-test/gitea-v1.6.4.mysql.sql.gz differ diff --git a/tests/integration/migration-test/gitea-v1.6.4.postgres.sql.gz b/tests/integration/migration-test/gitea-v1.6.4.postgres.sql.gz index bd66f6ba4f..2cbc109946 100644 Binary files a/tests/integration/migration-test/gitea-v1.6.4.postgres.sql.gz and b/tests/integration/migration-test/gitea-v1.6.4.postgres.sql.gz differ diff --git a/tests/integration/migration-test/gitea-v1.6.4.sqlite3.sql.gz b/tests/integration/migration-test/gitea-v1.6.4.sqlite3.sql.gz index a777c53025..321803bfba 100644 Binary files a/tests/integration/migration-test/gitea-v1.6.4.sqlite3.sql.gz and b/tests/integration/migration-test/gitea-v1.6.4.sqlite3.sql.gz differ diff --git a/tests/integration/migration-test/gitea-v1.7.0.mysql.sql.gz b/tests/integration/migration-test/gitea-v1.7.0.mysql.sql.gz index d0ab10891c..dcaad58784 100644 Binary files a/tests/integration/migration-test/gitea-v1.7.0.mysql.sql.gz and b/tests/integration/migration-test/gitea-v1.7.0.mysql.sql.gz differ diff --git a/tests/integration/migration-test/gitea-v1.7.0.postgres.sql.gz b/tests/integration/migration-test/gitea-v1.7.0.postgres.sql.gz index e4716c6b43..7a3bdc1cd7 100644 Binary files a/tests/integration/migration-test/gitea-v1.7.0.postgres.sql.gz and b/tests/integration/migration-test/gitea-v1.7.0.postgres.sql.gz differ diff --git a/tests/integration/migration-test/gitea-v1.7.0.sqlite3.sql.gz b/tests/integration/migration-test/gitea-v1.7.0.sqlite3.sql.gz index 3155249b07..d753f4d2b8 100644 Binary files a/tests/integration/migration-test/gitea-v1.7.0.sqlite3.sql.gz and b/tests/integration/migration-test/gitea-v1.7.0.sqlite3.sql.gz differ