diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index a22d35dd21..c5553388ea 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -115,6 +115,8 @@ var migrations = []*Migration{ NewMigration("Add `branch_filter` to `push_mirror` table", AddPushMirrorBranchFilter), // v37 -> v38 NewMigration("Add `resolved_unix` column to `abuse_report` table", AddResolvedUnixToAbuseReport), + // v38 -> v39 + NewMigration("Migrate `data` column of `secret` table to store keying material", MigrateActionSecretsToKeying), } // GetCurrentDBVersion returns the current Forgejo database version. diff --git a/models/forgejo_migrations/v39.go b/models/forgejo_migrations/v39.go new file mode 100644 index 0000000000..9af1c250b3 --- /dev/null +++ b/models/forgejo_migrations/v39.go @@ -0,0 +1,78 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "context" + "fmt" + + "forgejo.org/models/db" + secret_model "forgejo.org/models/secret" + "forgejo.org/modules/log" + "forgejo.org/modules/secret" + "forgejo.org/modules/setting" + + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +func MigrateActionSecretsToKeying(x *xorm.Engine) error { + return db.WithTx(db.DefaultContext, func(ctx context.Context) error { + sess := db.GetEngine(ctx) + + switch x.Dialect().URI().DBType { + case schemas.MYSQL: + if _, err := sess.Exec("ALTER TABLE `secret` MODIFY `data` BLOB"); err != nil { + return err + } + case schemas.SQLITE: + if _, err := sess.Exec("ALTER TABLE `secret` RENAME COLUMN `data` TO `data_backup`"); err != nil { + return err + } + if _, err := sess.Exec("ALTER TABLE `secret` ADD COLUMN `data` BLOB"); err != nil { + return err + } + if _, err := sess.Exec("UPDATE `secret` SET `data` = `data_backup`"); err != nil { + return err + } + if _, err := sess.Exec("ALTER TABLE `secret` DROP COLUMN `data_backup`"); err != nil { + return err + } + case schemas.POSTGRES: + if _, err := sess.Exec("ALTER TABLE `secret` ALTER COLUMN `data` SET DATA TYPE bytea USING data::text::bytea"); err != nil { + return err + } + } + + oldEncryptionKey := setting.SecretKey + + messages := make([]string, 0, 100) + ids := make([]int64, 0, 100) + + err := db.Iterate(ctx, nil, func(ctx context.Context, bean *secret_model.Secret) error { + secretBytes, err := secret.DecryptSecret(oldEncryptionKey, string(bean.Data)) + if err != nil { + messages = append(messages, fmt.Sprintf("secret.id=%d, secret.name=%q, secret.repo_id=%d, secret.owner_id=%d: secret.DecryptSecret(): %v", bean.ID, bean.Name, bean.RepoID, bean.OwnerID, err)) + ids = append(ids, bean.ID) + return nil + } + + bean.SetSecret(secretBytes) + _, err = sess.Cols("data").ID(bean.ID).Update(bean) + return err + }) + + if err == nil { + if len(ids) > 0 { + log.Error("Forgejo migration[37]: The following action secrets were found to be corrupted and removed from the database.") + for _, message := range messages { + log.Error("Forgejo migration[37]: %s", message) + } + + _, err = sess.In("id", ids).NoAutoCondition().NoAutoTime().Delete(&secret_model.Secret{}) + } + } + return err + }) +} diff --git a/models/forgejo_migrations/v39_test.go b/models/forgejo_migrations/v39_test.go new file mode 100644 index 0000000000..42934d912f --- /dev/null +++ b/models/forgejo_migrations/v39_test.go @@ -0,0 +1,52 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "testing" + + migration_tests "forgejo.org/models/migrations/test" + "forgejo.org/models/secret" + "forgejo.org/modules/keying" + "forgejo.org/modules/timeutil" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_MigrateActionSecretToKeying(t *testing.T) { + type Secret struct { + ID int64 + OwnerID int64 `xorm:"INDEX UNIQUE(owner_repo_name) NOT NULL"` + RepoID int64 `xorm:"INDEX UNIQUE(owner_repo_name) NOT NULL DEFAULT 0"` + Name string `xorm:"UNIQUE(owner_repo_name) NOT NULL"` + Data string `xorm:"LONGTEXT"` // encrypted data + CreatedUnix timeutil.TimeStamp `xorm:"created NOT NULL"` + } + + // Prepare and load the testing database + x, deferable := migration_tests.PrepareTestEnv(t, 0, new(Secret)) + defer deferable() + if x == nil || t.Failed() { + return + } + + cnt, err := x.Table("secret").Count() + require.NoError(t, err) + assert.EqualValues(t, 2, cnt) + + require.NoError(t, MigrateActionSecretsToKeying(x)) + + cnt, err = x.Table("secret").Count() + require.NoError(t, err) + assert.EqualValues(t, 1, cnt) + + var secret secret.Secret + _, err = x.Table("secret").ID(1).Get(&secret) + require.NoError(t, err) + + secretBytes, err := keying.DeriveKey(keying.ContextActionSecret).Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.Equal(t, []byte("A deep dark secret"), secretBytes) +} diff --git a/models/migrations/fixtures/Test_MigrateActionSecretToKeying/secret.yml b/models/migrations/fixtures/Test_MigrateActionSecretToKeying/secret.yml new file mode 100644 index 0000000000..908b428321 --- /dev/null +++ b/models/migrations/fixtures/Test_MigrateActionSecretToKeying/secret.yml @@ -0,0 +1,14 @@ +- + id: 1 + owner_id: 2 + repo_id: 1 + name: SECRET_1 + data: 02458e5f341b2d5081a31283559843b6b7543ab98ed213d2b15b5cef94385fa348afa7e0875122e9 + created_unix: 1753556968 +- + id: 2 + owner_id: 2 + repo_id: 1 + name: BADBAD + data: badbad + created_unix: 1753556968 diff --git a/models/secret/main_test.go b/models/secret/main_test.go new file mode 100644 index 0000000000..85bfec0c4f --- /dev/null +++ b/models/secret/main_test.go @@ -0,0 +1,17 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package secret_test + +import ( + "testing" + + "forgejo.org/models/unittest" + + _ "forgejo.org/models" + _ "forgejo.org/models/activities" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} diff --git a/models/secret/secret.go b/models/secret/secret.go index 7be7f454a1..6f6867db52 100644 --- a/models/secret/secret.go +++ b/models/secret/secret.go @@ -11,9 +11,8 @@ import ( actions_model "forgejo.org/models/actions" "forgejo.org/models/db" actions_module "forgejo.org/modules/actions" + "forgejo.org/modules/keying" "forgejo.org/modules/log" - secret_module "forgejo.org/modules/secret" - "forgejo.org/modules/setting" "forgejo.org/modules/timeutil" "forgejo.org/modules/util" @@ -39,7 +38,7 @@ type Secret struct { OwnerID int64 `xorm:"INDEX UNIQUE(owner_repo_name) NOT NULL"` RepoID int64 `xorm:"INDEX UNIQUE(owner_repo_name) NOT NULL DEFAULT 0"` Name string `xorm:"UNIQUE(owner_repo_name) NOT NULL"` - Data string `xorm:"LONGTEXT"` // encrypted data + Data []byte `xorm:"BLOB"` // encrypted data CreatedUnix timeutil.TimeStamp `xorm:"created NOT NULL"` } @@ -67,17 +66,21 @@ func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, dat return nil, fmt.Errorf("%w: ownerID and repoID cannot be both zero, global secrets are not supported", util.ErrInvalidArgument) } - encrypted, err := secret_module.EncryptSecret(setting.SecretKey, data) - if err != nil { - return nil, err - } secret := &Secret{ OwnerID: ownerID, RepoID: repoID, Name: strings.ToUpper(name), - Data: encrypted, } - return secret, db.Insert(ctx, secret) + + return secret, db.WithTx(ctx, func(ctx context.Context) error { + if err := db.Insert(ctx, secret); err != nil { + return err + } + + secret.SetSecret(data) + _, err := db.GetEngine(ctx).ID(secret.ID).Cols("data").Update(secret) + return err + }) } func init() { @@ -113,21 +116,9 @@ func (opts FindSecretsOptions) ToConds() builder.Cond { return cond } -// UpdateSecret changes org or user reop secret. -func UpdateSecret(ctx context.Context, secretID int64, data string) error { - encrypted, err := secret_module.EncryptSecret(setting.SecretKey, data) - if err != nil { - return err - } - - s := &Secret{ - Data: encrypted, - } - affected, err := db.GetEngine(ctx).ID(secretID).Cols("data").Update(s) - if affected != 1 { - return ErrSecretNotFound{} - } - return err +func (s *Secret) SetSecret(data string) { + key := keying.DeriveKey(keying.ContextActionSecret) + s.Data = key.Encrypt([]byte(data), keying.ColumnAndID("data", s.ID)) } func GetSecretsOfTask(ctx context.Context, task *actions_model.ActionTask) (map[string]string, error) { @@ -155,13 +146,14 @@ func GetSecretsOfTask(ctx context.Context, task *actions_model.ActionTask) (map[ return nil, err } + key := keying.DeriveKey(keying.ContextActionSecret) for _, secret := range append(ownerSecrets, repoSecrets...) { - v, err := secret_module.DecryptSecret(setting.SecretKey, secret.Data) + v, err := key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) if err != nil { - log.Error("decrypt secret %v %q: %v", secret.ID, secret.Name, err) + log.Error("unable to decrypt secret[id=%d,name=%q]: %v", secret.ID, secret.Name, err) return nil, err } - secrets[secret.Name] = v + secrets[secret.Name] = string(v) } return secrets, nil diff --git a/models/secret/secret_test.go b/models/secret/secret_test.go new file mode 100644 index 0000000000..15142d207b --- /dev/null +++ b/models/secret/secret_test.go @@ -0,0 +1,103 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package secret + +import ( + "testing" + + "forgejo.org/models/actions" + "forgejo.org/models/repo" + "forgejo.org/models/unittest" + "forgejo.org/modules/keying" + "forgejo.org/modules/util" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInsertEncryptedSecret(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("Global secret", func(t *testing.T) { + secret, err := InsertEncryptedSecret(t.Context(), 0, 0, "GLOBAL_SECRET", "some common secret") + require.ErrorIs(t, err, util.ErrInvalidArgument) + assert.Nil(t, secret) + }) + + key := keying.DeriveKey(keying.ContextActionSecret) + + t.Run("Insert repository secret", func(t *testing.T) { + secret, err := InsertEncryptedSecret(t.Context(), 0, 1, "REPO_SECRET", "some repository secret") + require.NoError(t, err) + assert.NotNil(t, secret) + assert.Equal(t, "REPO_SECRET", secret.Name) + assert.EqualValues(t, 1, secret.RepoID) + assert.NotEmpty(t, secret.Data) + + // Assert the secret is stored in the database. + unittest.AssertExistsAndLoadBean(t, &Secret{RepoID: 1, Name: "REPO_SECRET", Data: secret.Data}) + + t.Run("Keying", func(t *testing.T) { + // Cannot decrypt with different ID. + plainText, err := key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID+1)) + require.Error(t, err) + assert.Nil(t, plainText) + + // Cannot decrypt with different column. + plainText, err = key.Decrypt(secret.Data, keying.ColumnAndID("metadata", secret.ID)) + require.Error(t, err) + assert.Nil(t, plainText) + + // Can decrypt with correct column and ID. + plainText, err = key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.EqualValues(t, "some repository secret", plainText) + }) + }) + + t.Run("Insert owner secret", func(t *testing.T) { + secret, err := InsertEncryptedSecret(t.Context(), 2, 0, "OWNER_SECRET", "some owner secret") + require.NoError(t, err) + assert.NotNil(t, secret) + assert.Equal(t, "OWNER_SECRET", secret.Name) + assert.EqualValues(t, 2, secret.OwnerID) + assert.NotEmpty(t, secret.Data) + + // Assert the secret is stored in the database. + unittest.AssertExistsAndLoadBean(t, &Secret{OwnerID: 2, Name: "OWNER_SECRET", Data: secret.Data}) + + t.Run("Keying", func(t *testing.T) { + // Cannot decrypt with different ID. + plainText, err := key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID+1)) + require.Error(t, err) + assert.Nil(t, plainText) + + // Cannot decrypt with different column. + plainText, err = key.Decrypt(secret.Data, keying.ColumnAndID("metadata", secret.ID)) + require.Error(t, err) + assert.Nil(t, plainText) + + // Can decrypt with correct column and ID. + plainText, err = key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + require.NoError(t, err) + assert.EqualValues(t, "some owner secret", plainText) + }) + }) + + t.Run("Get secrets", func(t *testing.T) { + secrets, err := GetSecretsOfTask(t.Context(), &actions.ActionTask{ + Job: &actions.ActionRunJob{ + Run: &actions.ActionRun{ + RepoID: 1, + Repo: &repo.Repository{ + OwnerID: 2, + }, + }, + }, + }) + require.NoError(t, err) + assert.Equal(t, "some owner secret", secrets["OWNER_SECRET"]) + assert.Equal(t, "some repository secret", secrets["REPO_SECRET"]) + }) +} diff --git a/modules/keying/keying.go b/modules/keying/keying.go index c859e30e9f..f39e16aeed 100644 --- a/modules/keying/keying.go +++ b/modules/keying/keying.go @@ -58,6 +58,8 @@ var ( ContextPushMirror Context = "pushmirror" // Used for the `two_factor` table. ContextTOTP Context = "totp" + // Used for the `secret` table. + ContextActionSecret Context = "action_secret" ) // Derive *the* key for a given context, this is a deterministic function. diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index 2d5aebdbc1..2de83ef5a2 100644 --- a/services/secrets/secrets.go +++ b/services/secrets/secrets.go @@ -15,16 +15,16 @@ func CreateOrUpdateSecret(ctx context.Context, ownerID, repoID int64, name, data return nil, false, err } - s, err := db.Find[secret_model.Secret](ctx, secret_model.FindSecretsOptions{ + s, exists, err := db.Get[secret_model.Secret](ctx, secret_model.FindSecretsOptions{ OwnerID: ownerID, RepoID: repoID, Name: name, - }) + }.ToConds()) if err != nil { return nil, false, err } - if len(s) == 0 { + if !exists { s, err := secret_model.InsertEncryptedSecret(ctx, ownerID, repoID, name, data) if err != nil { return nil, false, err @@ -32,11 +32,11 @@ func CreateOrUpdateSecret(ctx context.Context, ownerID, repoID int64, name, data return s, true, nil } - if err := secret_model.UpdateSecret(ctx, s[0].ID, data); err != nil { + s.SetSecret(data) + if _, err := db.GetEngine(ctx).Cols("data").ID(s.ID).Update(s); err != nil { return nil, false, err } - - return s[0], false, nil + return s, false, nil } func DeleteSecretByID(ctx context.Context, ownerID, repoID, secretID int64) error {