feat: use keying for webhook secrets (#10059)

- Follow up of forgejo/forgejo!5041, forgejo/forgejo!6074, forgejo/forgejo!8692, forgejo/forgejo!9923
- The `webhook` table contains a encrypted header authorization.
- Use `keying` to safely store this secret and bound them to the table, column and row id
- The migration isn't spectacular but does closely follow what we learned in the previous three migrations: use a transaction and delete records when you can't decrypt them.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10059
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Reviewed-by: oliverpool <oliverpool@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

models/forgejo_migrations/v14a_migrate_webhook_authorization.go

@@ -0,0 +1,91 @@
+// 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"
+	webhook_model "forgejo.org/models/webhook"
+	"forgejo.org/modules/keying"
+	"forgejo.org/modules/log"
+	"forgejo.org/modules/secret"
+	"forgejo.org/modules/setting"
+
+	"xorm.io/xorm"
+	"xorm.io/xorm/schemas"
+)
+
+func init() {
+	registerMigration(&Migration{
+		Description: "migrate `header_authorization_encrypted` of `webhook` table to store keying material",
+		Upgrade:     migrateWebhookSecrets,
+	})
+}
+
+func migrateWebhookSecrets(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 `webhook` MODIFY `header_authorization_encrypted` BLOB"); err != nil {
+				return err
+			}
+		case schemas.SQLITE:
+			if _, err := sess.Exec("ALTER TABLE `webhook` RENAME COLUMN `header_authorization_encrypted` TO `header_authorization_encrypted_backup`"); err != nil {
+				return err
+			}
+			if _, err := sess.Exec("ALTER TABLE `webhook` ADD COLUMN `header_authorization_encrypted` BLOB"); err != nil {
+				return err
+			}
+			if _, err := sess.Exec("UPDATE `webhook` SET `header_authorization_encrypted` = `header_authorization_encrypted_backup`"); err != nil {
+				return err
+			}
+			if _, err := sess.Exec("ALTER TABLE `webhook` DROP COLUMN `header_authorization_encrypted_backup`"); err != nil {
+				return err
+			}
+		case schemas.POSTGRES:
+			if _, err := sess.Exec("ALTER TABLE `webhook` ALTER COLUMN `header_authorization_encrypted` SET DATA TYPE bytea USING header_authorization_encrypted::text::bytea"); err != nil {
+				return err
+			}
+		}
+
+		key := keying.Webhook
+
+		oldEncryptionKey := setting.SecretKey
+		messages := make([]string, 0, 100)
+		ids := make([]int64, 0, 100)
+
+		err := db.Iterate(ctx, nil, func(ctx context.Context, bean *webhook_model.Webhook) error {
+			if len(bean.HeaderAuthorizationEncrypted) == 0 {
+				return nil
+			}
+
+			secretBytes, err := secret.DecryptSecret(oldEncryptionKey, string(bean.HeaderAuthorizationEncrypted))
+			if err != nil {
+				messages = append(messages, fmt.Sprintf("webhook.id=%d, webhook.repo_id=%d, webhook.owner_id=%d: secret.DecryptSecret(): %v", bean.ID, bean.RepoID, bean.OwnerID, err))
+				ids = append(ids, bean.ID)
+				return nil
+			}
+
+			bean.HeaderAuthorizationEncrypted = key.Encrypt([]byte(secretBytes), keying.ColumnAndID("header_authorization_encrypted", bean.ID))
+			_, err = sess.Cols("header_authorization_encrypted").ID(bean.ID).Update(bean)
+			return err
+		})
+
+		if err == nil {
+			if len(ids) > 0 {
+				log.Error("migration[v14a_migrate_webhook_authorization]: The following webhook were found to be corrupted and removed from the database.")
+				for _, message := range messages {
+					log.Error("migration[v14a_migrate_webhook_authorization]: %s", message)
+				}
+
+				_, err = sess.In("id", ids).NoAutoCondition().NoAutoTime().Delete(&webhook_model.Webhook{})
+			}
+		}
+		return err
+	})
+}

models/forgejo_migrations/v14a_migrate_webhook_authorization_test.go

@@ -0,0 +1,97 @@
+// 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/gitea_migrations/test"
+	webhook_model "forgejo.org/models/webhook"
+	"forgejo.org/modules/keying"
+	"forgejo.org/modules/timeutil"
+	webhook_module "forgejo.org/modules/webhook"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_MigrateWebhookSecrets(t *testing.T) {
+	type Webhook struct {
+		ID              int64 `xorm:"pk autoincr"`
+		RepoID          int64 `xorm:"INDEX"`
+		OwnerID         int64 `xorm:"INDEX"`
+		IsSystemWebhook bool
+		URL             string `xorm:"url TEXT"`
+		HTTPMethod      string `xorm:"http_method"`
+		ContentType     webhook_model.HookContentType
+		Secret          string                  `xorm:"TEXT"`
+		Events          string                  `xorm:"TEXT"`
+		IsActive        bool                    `xorm:"INDEX"`
+		Type            webhook_module.HookType `xorm:"VARCHAR(16) 'type'"`
+		Meta            string                  `xorm:"TEXT"`
+		LastStatus      webhook_module.HookStatus
+
+		HeaderAuthorizationEncrypted string `xorm:"TEXT"`
+
+		CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
+		UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
+	}
+
+	type NewWebhook struct {
+		ID              int64 `xorm:"pk autoincr"`
+		RepoID          int64 `xorm:"INDEX"`
+		OwnerID         int64 `xorm:"INDEX"`
+		IsSystemWebhook bool
+		URL             string `xorm:"url TEXT"`
+		HTTPMethod      string `xorm:"http_method"`
+		ContentType     webhook_model.HookContentType
+		Secret          string                  `xorm:"TEXT"`
+		Events          string                  `xorm:"TEXT"`
+		IsActive        bool                    `xorm:"INDEX"`
+		Type            webhook_module.HookType `xorm:"VARCHAR(16) 'type'"`
+		Meta            string                  `xorm:"TEXT"`
+		LastStatus      webhook_module.HookStatus
+
+		HeaderAuthorizationEncrypted []byte `xorm:"BLOB"`
+
+		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(Webhook))
+	defer deferable()
+	if x == nil || t.Failed() {
+		return
+	}
+
+	cnt, err := x.Table("webhook").Count()
+	require.NoError(t, err)
+	assert.EqualValues(t, 3, cnt)
+
+	require.NoError(t, migrateWebhookSecrets(x))
+
+	cnt, err = x.Table("webhook").Count()
+	require.NoError(t, err)
+	assert.EqualValues(t, 2, cnt)
+
+	key := keying.Webhook
+
+	t.Run("webhook 1", func(t *testing.T) {
+		var webhook NewWebhook
+		_, err = x.Table("webhook").ID(1).Get(&webhook)
+		require.NoError(t, err)
+
+		secret, err := key.Decrypt(webhook.HeaderAuthorizationEncrypted, keying.ColumnAndID("header_authorization_encrypted", webhook.ID))
+		require.NoError(t, err)
+		assert.EqualValues(t, "Bearer s3cr3t", secret)
+	})
+
+	t.Run("webhook 3", func(t *testing.T) {
+		var webhook NewWebhook
+		_, err = x.Table("webhook").ID(3).Get(&webhook)
+		require.NoError(t, err)
+		assert.Empty(t, webhook.HeaderAuthorizationEncrypted)
+	})
+}

models/gitea_migrations/fixtures/Test_MigrateWebhookSecrets/webhook.yml

@@ -0,0 +1,17 @@
+-
+  id: 1
+  owner_id: 3
+  repo_id: 3
+  header_authorization_encrypted: '54586e944822336738b940c2560b7ef38bea3a91dcfe43c9c32e55b2a57050f75c63456b'
+
+-
+  id: 2
+  owner_id: 1
+  repo_id: 2
+  header_authorization_encrypted: 'badbadbad'
+
+-
+  id: 3
+  owner_id: 2
+  repo_id: 1
+  header_authorization_encrypted: ''

models/webhook/webhook.go

@@ -11,10 +11,9 @@ import (
 
 	"forgejo.org/models/db"
 	"forgejo.org/modules/json"
+	"forgejo.org/modules/keying"
 	"forgejo.org/modules/log"
 	"forgejo.org/modules/optional"
-	"forgejo.org/modules/secret"
-	"forgejo.org/modules/setting"
 	"forgejo.org/modules/timeutil"
 	"forgejo.org/modules/util"
 	webhook_module "forgejo.org/modules/webhook"
@@ -137,7 +136,7 @@ type Webhook struct {
 	LastStatus                webhook_module.HookStatus // Last delivery status
 
 	// HeaderAuthorizationEncrypted should be accessed using HeaderAuthorization() and SetHeaderAuthorization()
-	HeaderAuthorizationEncrypted string `xorm:"TEXT"`
+	HeaderAuthorizationEncrypted []byte `xorm:"BLOB"`
 
 	CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@@ -376,10 +375,15 @@ func (w *Webhook) EventsArray() []string {
 // HeaderAuthorization returns the decrypted Authorization header.
 // Not on the reference (*w), to be accessible on WebhooksNew.
 func (w Webhook) HeaderAuthorization() (string, error) {
-	if w.HeaderAuthorizationEncrypted == "" {
+	if len(w.HeaderAuthorizationEncrypted) == 0 {
 		return "", nil
 	}
-	return secret.DecryptSecret(setting.SecretKey, w.HeaderAuthorizationEncrypted)
+
+	headerAuthorization, err := keying.Webhook.Decrypt(w.HeaderAuthorizationEncrypted, keying.ColumnAndID("header_authorization_encrypted", w.ID))
+	if err != nil {
+		return "", err
+	}
+	return string(headerAuthorization), nil
 }
 
 // HeaderAuthorizationTrimPrefix returns the decrypted Authorization with a specified prefix trimmed.
@@ -392,23 +396,31 @@ func (w Webhook) HeaderAuthorizationTrimPrefix(prefix string) (string, error) {
 }
 
 // SetHeaderAuthorization encrypts and sets the Authorization header.
-func (w *Webhook) SetHeaderAuthorization(cleartext string) error {
+func (w *Webhook) SetHeaderAuthorization(cleartext string) {
 	if cleartext == "" {
-		w.HeaderAuthorizationEncrypted = ""
-		return nil
+		w.HeaderAuthorizationEncrypted = nil
+		return
 	}
-	ciphertext, err := secret.EncryptSecret(setting.SecretKey, cleartext)
-	if err != nil {
-		return err
-	}
-	w.HeaderAuthorizationEncrypted = ciphertext
-	return nil
+
+	w.HeaderAuthorizationEncrypted = keying.Webhook.Encrypt([]byte(cleartext), keying.ColumnAndID("header_authorization_encrypted", w.ID))
 }
 
 // CreateWebhook creates a new web hook.
-func CreateWebhook(ctx context.Context, w *Webhook) error {
+func CreateWebhook(ctx context.Context, w *Webhook, authorizationHeader string) error {
 	w.Type = strings.TrimSpace(w.Type)
-	return db.Insert(ctx, w)
+
+	if len(authorizationHeader) == 0 {
+		return db.Insert(ctx, w)
+	}
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		if err := db.Insert(ctx, w); err != nil {
+			return err
+		}
+
+		w.SetHeaderAuthorization(authorizationHeader)
+		_, err := db.GetEngine(ctx).Cols("header_authorization_encrypted").ID(w.ID).Update(w)
+		return err
+	})
 }
 
 // CreateWebhooks creates multiple web hooks

models/webhook/webhook_system.go

@@ -77,7 +77,7 @@ func CopyDefaultWebhooksToRepo(ctx context.Context, repoID int64) error {
 	for _, w := range ws {
 		w.ID = 0
 		w.RepoID = repoID
-		if err := CreateWebhook(ctx, w); err != nil {
+		if err := CreateWebhook(ctx, w, ""); err != nil {
 			return fmt.Errorf("CreateWebhook: %v", err)
 		}
 	}

models/webhook/webhook_test.go

@@ -98,7 +98,7 @@ func TestCreateWebhook(t *testing.T) {
 			Events:      `{"push_only":false,"send_everything":false,"choose_events":true,"events":{"create":false,"push":true,"pull_request":true}}`,
 		}
 		unittest.AssertNotExistsBean(t, hook)
-		require.NoError(t, CreateWebhook(db.DefaultContext, hook))
+		require.NoError(t, CreateWebhook(db.DefaultContext, hook, ""))
 		hookFromDb := unittest.AssertExistsAndLoadBean(t, hook)
 		assert.Equal(t, []string{
 			string(webhook_module.HookEventPush),
@@ -114,7 +114,7 @@ func TestCreateWebhook(t *testing.T) {
 			Events:      `{"push_only":false,"send_everything":false,"choose_events":true,"events":{"action_run_recover":false,"action_run_success":true}}`,
 		}
 		unittest.AssertNotExistsBean(t, hook)
-		require.NoError(t, CreateWebhook(db.DefaultContext, hook))
+		require.NoError(t, CreateWebhook(db.DefaultContext, hook, ""))
 		hookFromDb := unittest.AssertExistsAndLoadBean(t, hook)
 		assert.Equal(t, []string{string(webhook_module.HookEventActionRunSuccess)}, hookFromDb.EventsArray())
 	})
@@ -127,7 +127,7 @@ func TestCreateWebhook(t *testing.T) {
 			Events:      `{"push_only":false,"send_everything":false,"choose_events":true,"events":{"create":true,"delete":true,"fork":true,"issues":true,"issue_assign":true,"issue_label":true,"issue_milestone":true,"issue_comment":true,"push":true,"pull_request":true,"pull_request_assign":true,"pull_request_label":true,"pull_request_milestone":true,"pull_request_comment":true,"pull_request_review":true,"pull_request_sync":true,"pull_request_review_request":true,"wiki":true,"repository":true,"release":true,"package":true,"action_run_failure":true,"action_run_recover":true,"action_run_success":true}}`,
 		}
 		unittest.AssertNotExistsBean(t, hook)
-		require.NoError(t, CreateWebhook(db.DefaultContext, hook))
+		require.NoError(t, CreateWebhook(db.DefaultContext, hook, ""))
 		hookFromDb := unittest.AssertExistsAndLoadBean(t, hook)
 		assert.Equal(t, []string{
 			string(webhook_module.HookEventCreate),

modules/keying/keying.go

@@ -39,6 +39,8 @@ var (
 	ActionSecret = deriveKey("action_secret")
 	// Used for the `task` table where type == TaskTypeMigrateRepo.
 	MigrateTask = deriveKey("migrate_repo_task")
+	// Used for the `webhook` table.
+	Webhook = deriveKey("webhook")
 )
 
 var (

routers/api/v1/utils/hook.go

@@ -220,11 +220,6 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
 		IsActive: form.Active,
 		Type:     form.Type,
 	}
-	err := w.SetHeaderAuthorization(form.AuthorizationHeader)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "SetHeaderAuthorization", err)
-		return nil, false
-	}
 	if w.Type == webhook_module.SLACK {
 		channel, ok := form.Config["channel"]
 		if !ok {
@@ -254,7 +249,9 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
 	if err := w.UpdateEvent(); err != nil {
 		ctx.Error(http.StatusInternalServerError, "UpdateEvent", err)
 		return nil, false
-	} else if err := webhook.CreateWebhook(ctx, w); err != nil {
+	}
+
+	if err := webhook.CreateWebhook(ctx, w, form.AuthorizationHeader); err != nil {
 		ctx.Error(http.StatusInternalServerError, "CreateWebhook", err)
 		return nil, false
 	}
@@ -379,11 +376,7 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh
 	w.Release = util.SliceContainsString(form.Events, string(webhook_module.HookEventRelease), true)
 	w.BranchFilter = form.BranchFilter
 
-	err := w.SetHeaderAuthorization(form.AuthorizationHeader)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "SetHeaderAuthorization", err)
-		return false
-	}
+	w.SetHeaderAuthorization(form.AuthorizationHeader)
 
 	// Issues
 	w.Issues = issuesHook(form.Events, "issues_only")

routers/web/repo/setting/webhook.go

@@ -215,17 +215,14 @@ func WebhookCreate(ctx *context.Context) {
 	if ctx.HasError() {
 		// pre-fill the form with the submitted data
 		var w webhook.Webhook
+		w.ID = -1 // We are going to encrypt it using this ID, bind the encrypted value to a ID that will never exist in the database. Safety precaution.
 		w.URL = fields.URL
 		w.ContentType = fields.ContentType
 		w.Secret = fields.Secret
 		w.HookEvent = ParseHookEvent(fields.WebhookCoreForm)
 		w.IsActive = fields.Active
 		w.HTTPMethod = fields.HTTPMethod
-		err := w.SetHeaderAuthorization(fields.AuthorizationHeader)
-		if err != nil {
-			ctx.ServerError("SetHeaderAuthorization", err)
-			return
-		}
+		w.SetHeaderAuthorization(fields.AuthorizationHeader)
 		ctx.Data["Webhook"] = w
 		ctx.Data["HookMetadata"] = fields.Metadata
 
@@ -255,15 +252,12 @@ func WebhookCreate(ctx *context.Context) {
 		OwnerID:         orCtx.OwnerID,
 		IsSystemWebhook: orCtx.IsSystemWebhook,
 	}
-	err = w.SetHeaderAuthorization(fields.AuthorizationHeader)
-	if err != nil {
-		ctx.ServerError("SetHeaderAuthorization", err)
-		return
-	}
 	if err := w.UpdateEvent(); err != nil {
 		ctx.ServerError("UpdateEvent", err)
 		return
-	} else if err := webhook.CreateWebhook(ctx, w); err != nil {
+	}
+
+	if err := webhook.CreateWebhook(ctx, w, fields.AuthorizationHeader); err != nil {
 		ctx.ServerError("CreateWebhook", err)
 		return
 	}
@@ -302,11 +296,7 @@ func WebhookUpdate(ctx *context.Context) {
 	w.IsActive = fields.Active
 	w.HTTPMethod = fields.HTTPMethod
 
-	err := w.SetHeaderAuthorization(fields.AuthorizationHeader)
-	if err != nil {
-		ctx.ServerError("SetHeaderAuthorization", err)
-		return
-	}
+	w.SetHeaderAuthorization(fields.AuthorizationHeader)
 
 	if ctx.HasError() {
 		ctx.Data["HookMetadata"] = fields.Metadata
@@ -314,6 +304,7 @@ func WebhookUpdate(ctx *context.Context) {
 		return
 	}
 
+	var err error
 	var meta []byte
 	if fields.Metadata != nil {
 		meta, err = json.Marshal(fields.Metadata)

services/issue/comments_test.go

@@ -40,7 +40,7 @@ func TestDeleteComment(t *testing.T) {
 			RepoID:   issue.RepoID,
 			IsActive: true,
 			Events:   `{"choose_events":true,"events":{"issue_comment": true}}`,
-		}))
+		}, ""))
 		hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{})
 
 		require.NoError(t, issue_service.DeleteComment(db.DefaultContext, nil, comment))
@@ -68,7 +68,7 @@ func TestDeleteComment(t *testing.T) {
 			RepoID:   issue.RepoID,
 			IsActive: true,
 			Events:   `{"choose_events":true,"events":{"issue_comment": true}}`,
-		}))
+		}, ""))
 		hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{})
 
 		require.NoError(t, comment.LoadReview(t.Context()))
@@ -101,7 +101,7 @@ func TestUpdateComment(t *testing.T) {
 			RepoID:   issue.RepoID,
 			IsActive: true,
 			Events:   `{"choose_events":true,"events":{"issue_comment": true}}`,
-		}))
+		}, ""))
 		hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{})
 		oldContent := comment.Content
 		comment.Content = "Hello!"
@@ -132,7 +132,7 @@ func TestUpdateComment(t *testing.T) {
 			RepoID:   issue.RepoID,
 			IsActive: true,
 			Events:   `{"choose_events":true,"events":{"issue_comment": true}}`,
-		}))
+		}, ""))
 		hookTaskCount := unittest.GetCount(t, &webhook_model.HookTask{})
 		oldContent := comment.Content
 		comment.Content = "Hello!"

services/webhook/deliver_test.go

@@ -102,9 +102,7 @@ func TestWebhookDeliverAuthorizationHeader(t *testing.T) {
 		IsActive:    true,
 		Type:        webhook_module.GITEA,
 	}
-	err := hook.SetHeaderAuthorization("Bearer s3cr3t-t0ken")
-	require.NoError(t, err)
-	require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook))
+	require.NoError(t, webhook_model.CreateWebhook(t.Context(), hook, "Bearer s3cr3t-t0ken"))
 
 	hookTask := &webhook_model.HookTask{
 		HookID:         hook.ID,
@@ -112,7 +110,7 @@ func TestWebhookDeliverAuthorizationHeader(t *testing.T) {
 		PayloadVersion: 2,
 	}
 
-	hookTask, err = webhook_model.CreateHookTask(db.DefaultContext, hookTask)
+	hookTask, err := webhook_model.CreateHookTask(db.DefaultContext, hookTask)
 	require.NoError(t, err)
 	assert.NotNil(t, hookTask)
 
@@ -168,7 +166,7 @@ func TestWebhookDeliverHookTask(t *testing.T) {
 		ContentType: webhook_model.ContentTypeJSON,
 		Meta:        `{"message_type":0}`, // text
 	}
-	require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook))
+	require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook, ""))
 
 	t.Run("Version 1", func(t *testing.T) {
 		hookTask := &webhook_model.HookTask{
@@ -296,7 +294,7 @@ func TestWebhookDeliverSpecificTypes(t *testing.T) {
 				ContentType: 0,  // set to 0 so that falling back to default request fails with "invalid content type"
 				Meta:        "{}",
 			}
-			require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook))
+			require.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook, ""))
 
 			hookTask := &webhook_model.HookTask{
 				HookID:         hook.ID,