feat: migrate action secrets to keying to store them more securely (#8692)

- Use the keying module, that was introduced in forgejo/forgejo#5041, to store action secrets safely and securely in the database.
- Introduce a central function that sets the secret, `SetSecret` and let the caller do the update call. This is similar to how the twofactor (TOTP) models does it. Ref. https://codeberg.org/forgejo/forgejo/pulls/6074
- Add a relaxed migration, that is run inside a transaction. If it cannot decrypt a action secret, then it's deleted.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8692
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

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.

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
+	})
+}

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)
+}

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

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)
+}

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

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"])
+	})
+}

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.

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 {