From 9193fa4736feffa8ddc199b36e80a98c840f9127 Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 20 Aug 2026 04:17:50 +0000 Subject: [PATCH] Stop target credentials leaking into event databases (closes #206) A Delivery carries its Event and Target structs in memory for the delivery engine, so GORM's automatic association save upserted the whole target row -- config included, which holds destination URLs and bearer credentials -- into the per-webhook event database with an empty webhook_id. Event databases are the files most likely to be backed up or handed to someone else, so they shipped the credentials with them. Register a create and update callback on every per-webhook connection that omits associations, rather than fixing the one call site: it covers writes inside a transaction and write paths added later. Sweep any rows already written, before the migration on each open, so it is idempotent and a no-op on a database with no targets table. Encryption of target config at rest in webhooker.db is deliberately not part of this: it is tracked separately. --- README.md | 23 +- internal/database/event_db_isolation.go | 116 ++++++++ internal/database/event_db_isolation_test.go | 290 +++++++++++++++++++ internal/database/webhook_db_manager.go | 19 ++ internal/delivery/event_db_isolation_test.go | 157 ++++++++++ 5 files changed, 597 insertions(+), 8 deletions(-) create mode 100644 internal/database/event_db_isolation.go create mode 100644 internal/database/event_db_isolation_test.go create mode 100644 internal/delivery/event_db_isolation_test.go diff --git a/README.md b/README.md index 06acc8b..54ded5d 100644 --- a/README.md +++ b/README.md @@ -425,14 +425,21 @@ backups at rest and restrict who can read them. - `events-{uuid}.db` and `archive-{uuid}.db` hold the **full payload body and headers** of every event as received, including whatever the sending service put in them — tokens, signatures, personal data. -- Until - [issue #206](https://git.eeqj.de/sneak/webhooker/issues/206) is fixed, - the event databases **also contain target credentials**: a GORM - association upsert on the delivery and retry write path copies - `targets` rows, `config` included, into the per-webhook database. For - a Slack target the `webhookUrl` *is* the bearer credential, and an - `http` target's URL can embed userinfo. Handing someone an - `events-*.db` today hands them live delivery destinations. +- Event databases written before + [issue #206](https://git.eeqj.de/sneak/webhooker/issues/206) was fixed + **also contain target credentials**: a GORM association upsert on the + delivery and retry write path copied `targets` rows, `config` + included, into the per-webhook database. For a Slack target the + `webhookUrl` *is* the bearer credential, and an `http` target's URL + can embed userinfo. This version never writes those rows; the first + time it opens such a file it deletes them and vacuums the file, which + removes the credential bytes rather than only unlinking the rows. Two + cases still hand over live delivery destinations: a backup taken from + an older build, and a backup of a file this version has not opened + yet. Copies already made stay affected — the sweep only rewrites the + file it opens, and freed blocks may persist in filesystem snapshots + and on the underlying storage. Rotate any target credential that was + in a backup you cannot account for. - `webhooker.db` stores target config **unencrypted**, tracked at [issue #212](https://git.eeqj.de/sneak/webhooker/issues/212), next to the session encryption key and the Argon2id password hashes. diff --git a/internal/database/event_db_isolation.go b/internal/database/event_db_isolation.go new file mode 100644 index 0000000..0a247b2 --- /dev/null +++ b/internal/database/event_db_isolation.go @@ -0,0 +1,116 @@ +package database + +import ( + "fmt" + "log/slog" + + "gorm.io/gorm" + "gorm.io/gorm/clause" +) + +// omitAssociationsCallback is the name the association guard is +// registered under on a per-webhook database's create and update +// callback chains. +const omitAssociationsCallback = "webhooker:omit_associations" + +// omitAssociations makes every create and update issued against a +// per-webhook database skip GORM's automatic association save. +// +// A per-webhook database holds the event tier only, but Delivery +// declares belongs-to Event and Target and the delivery engine fills +// both in memory before writing. Without this guard GORM upserts +// those parent rows here on the delivery and retry write paths, +// copying targets.config, which holds destination URLs and bearer +// credentials, into the file most likely to be backed up or handed +// to someone else. Registering the guard on the connection covers +// every write path, including writes inside a transaction and write +// paths added later. Every event-tier row this file holds is written +// explicitly, so nothing depends on the automatic save. +func omitAssociations(db *gorm.DB) error { + omit := func(tx *gorm.DB) { + tx.Statement.Omits = append( + tx.Statement.Omits, clause.Associations, + ) + } + + err := db.Callback().Create(). + Before("gorm:save_before_associations"). + Register(omitAssociationsCallback, omit) + if err != nil { + return fmt.Errorf( + "registering create association guard: %w", err, + ) + } + + err = db.Callback().Update(). + Before("gorm:save_before_associations"). + Register(omitAssociationsCallback, omit) + if err != nil { + return fmt.Errorf( + "registering update association guard: %w", err, + ) + } + + return nil +} + +// purgeTargetRows deletes target rows that an earlier build's +// association upsert wrote into a per-webhook database. AutoMigrate +// creates a targets table in every one of these files because +// Delivery declares a belongs-to Target, but nothing in the event +// tier may put rows in it. The rows it did put there are junk, not +// history: they carry an empty webhook_id, and delivery rows resolve +// their target against the main database, so nothing here refers to +// them. +// +// It runs before every migration, so it is idempotent, and it is a +// no-op on a database that has no targets table at all. +// +// The DELETE only unlinks the rows: modernc.org/sqlite leaves +// secure_delete at SQLite's default of off, so the credential bytes +// stay readable in the file's free pages and a backup of a swept file +// would still hand them over. VACUUM rewrites the file without them. +// It is gated on having actually deleted something, so a file that +// was never leaked into, or that an earlier run already swept, does +// not pay for a rewrite on every open. +func purgeTargetRows( + db *gorm.DB, log *slog.Logger, webhookID string, +) error { + if !db.Migrator().HasTable("targets") { + return nil + } + + res := db.Exec("DELETE FROM targets") + if res.Error != nil { + return fmt.Errorf( + "purging target rows from webhook database %s: %w", + webhookID, res.Error, + ) + } + + if res.RowsAffected == 0 { + return nil + } + + err := db.Exec("VACUUM").Error + if err != nil { + // The rows are already gone, so a later open will not retry + // this: the operator has to vacuum the file by hand or the + // credentials stay recoverable in it. + return fmt.Errorf( + "purged %d leaked target rows from webhook database %s "+ + "but vacuuming it failed, so the deleted target "+ + "credentials are still recoverable from the file and "+ + "it must be vacuumed by hand: %w", + res.RowsAffected, webhookID, err, + ) + } + + log.Warn( + "purged leaked target rows from per-webhook database", + "webhook_id", webhookID, + "rows", res.RowsAffected, + ) + + return nil +} diff --git a/internal/database/event_db_isolation_test.go b/internal/database/event_db_isolation_test.go new file mode 100644 index 0000000..c87097e --- /dev/null +++ b/internal/database/event_db_isolation_test.go @@ -0,0 +1,290 @@ +package database_test + +import ( + "bytes" + "database/sql" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" + "sneak.berlin/go/webhooker/internal/database" +) + +// testDataDirPerm is the mode the test data directory is created +// with. +const testDataDirPerm = 0o750 + +// eventDBDataDir returns a data directory that a WebhookDBManager +// can be pointed at. +func eventDBDataDir(t *testing.T) string { + t.Helper() + + dir := filepath.Join(t.TempDir(), "events") + require.NoError(t, os.MkdirAll(dir, testDataDirPerm)) + + return dir +} + +// openRawEventDB opens the per-webhook database file directly, +// without the manager, so a test can put a file on disk in a state +// the manager has to cope with, or inspect one afterwards. +func openRawEventDB( + t *testing.T, dataDir, webhookID string, +) *sql.DB { + t.Helper() + + path := filepath.Join( + dataDir, fmt.Sprintf("events-%s.db", webhookID), + ) + + sqlDB, err := sql.Open( + "sqlite", + fmt.Sprintf("file:%s?mode=rwc", path), + ) + require.NoError(t, err) + + t.Cleanup(func() { _ = sqlDB.Close() }) + + return sqlDB +} + +// eventDBFileBytes reads a per-webhook database file off disk, so a +// test can assert on what the file itself still holds rather than on +// what a query returns. +func eventDBFileBytes(t *testing.T, dataDir, webhookID string) []byte { + t.Helper() + + //nolint:gosec // reads a file the test just created under t.TempDir() + raw, err := os.ReadFile(filepath.Join( + dataDir, fmt.Sprintf("events-%s.db", webhookID), + )) + require.NoError(t, err) + + return raw +} + +// countTargetRows returns the number of rows in the targets table of +// a per-webhook database file, or -1 if the table does not exist. +func countTargetRows(t *testing.T, sqlDB *sql.DB) int { + t.Helper() + + var tables int + + require.NoError(t, sqlDB.QueryRowContext( + t.Context(), + "SELECT count(*) FROM sqlite_master "+ + "WHERE type = 'table' AND name = 'targets'", + ).Scan(&tables)) + + if tables == 0 { + return -1 + } + + var rows int + + require.NoError(t, sqlDB.QueryRowContext( + t.Context(), "SELECT count(*) FROM targets", + ).Scan(&rows)) + + return rows +} + +// TestOpenPurgesLeakedTargetRows covers the sweep for event +// databases written by a build that let GORM upsert target rows +// into them: opening the database clears them, and opening it again +// is a no-op. +func TestOpenPurgesLeakedTargetRows(t *testing.T) { + t.Parallel() + + dataDir := eventDBDataDir(t) + webhookID := uuid.New().String() + + // Create the file the way the application does, so the targets + // table has exactly the shape AutoMigrate gives it, then write + // a leaked row into it the way the association upsert did. + initial := database.NewTestWebhookDBManager(dataDir) + + _, err := initial.GetDB(webhookID) + require.NoError(t, err) + require.NoError(t, initial.CloseAll()) + + seed := openRawEventDB(t, dataDir, webhookID) + + _, err = seed.ExecContext( + t.Context(), + "INSERT INTO targets "+ + "(id, webhook_id, name, type, config) "+ + "VALUES (?, '', ?, ?, ?)", + uuid.New().String(), + "leaked-target", + "slack", + `{"webhookUrl":"https://hooks.example/T000/B000/secret"}`, + ) + require.NoError(t, err) + require.Equal(t, 1, countTargetRows(t, seed)) + require.NoError(t, seed.Close()) + + mgr := database.NewTestWebhookDBManager(dataDir) + + _, err = mgr.GetDB(webhookID) + require.NoError(t, err) + require.NoError(t, mgr.CloseAll()) + + check := openRawEventDB(t, dataDir, webhookID) + assert.Zero(t, countTargetRows(t, check)) + require.NoError(t, check.Close()) + + // Idempotent: a second open leaves it at zero and does not + // error. + again := database.NewTestWebhookDBManager(dataDir) + + _, err = again.GetDB(webhookID) + require.NoError(t, err) + require.NoError(t, again.CloseAll()) + + recheck := openRawEventDB(t, dataDir, webhookID) + assert.Zero(t, countTargetRows(t, recheck)) +} + +// TestOpenPurgeRemovesCredentialBytes covers the sweep at the level +// that matters for a backup handed to someone else: the leaked +// credential must be gone from the raw bytes of the file, not merely +// unreachable by query. A bare DELETE unlinks the row and leaves the +// bytes readable in the free pages, so this fails without the VACUUM +// in purgeTargetRows. +func TestOpenPurgeRemovesCredentialBytes(t *testing.T) { + t.Parallel() + + dataDir := eventDBDataDir(t) + webhookID := uuid.New().String() + credential := "T00000000/B00000000/" + uuid.New().String() + + initial := database.NewTestWebhookDBManager(dataDir) + + _, err := initial.GetDB(webhookID) + require.NoError(t, err) + require.NoError(t, initial.CloseAll()) + + seed := openRawEventDB(t, dataDir, webhookID) + + _, err = seed.ExecContext( + t.Context(), + "INSERT INTO targets "+ + "(id, webhook_id, name, type, config) "+ + "VALUES (?, '', ?, ?, ?)", + uuid.New().String(), + "leaked-target", + "slack", + fmt.Sprintf( + `{"webhookUrl":"https://hooks.example/%s"}`, credential, + ), + ) + require.NoError(t, err) + require.NoError(t, seed.Close()) + + // The seed has to be in the file for its absence later to mean + // anything. + require.True( + t, + bytes.Contains( + eventDBFileBytes(t, dataDir, webhookID), + []byte(credential), + ), + "seeded credential is not in the file, so this test proves nothing", + ) + + mgr := database.NewTestWebhookDBManager(dataDir) + + _, err = mgr.GetDB(webhookID) + require.NoError(t, err) + require.NoError(t, mgr.CloseAll()) + + assert.NotContains( + t, + string(eventDBFileBytes(t, dataDir, webhookID)), + credential, + "leaked credential is still recoverable from the raw file", + ) +} + +// TestOpenSucceedsWithoutTargetsTable covers an existing event +// database that never grew a targets table. The sweep must not fail +// startup on it. +func TestOpenSucceedsWithoutTargetsTable(t *testing.T) { + t.Parallel() + + dataDir := eventDBDataDir(t) + webhookID := uuid.New().String() + + seed := openRawEventDB(t, dataDir, webhookID) + + _, err := seed.ExecContext( + t.Context(), + "CREATE TABLE events (id text PRIMARY KEY)", + ) + require.NoError(t, err) + require.NoError(t, seed.Close()) + + mgr := database.NewTestWebhookDBManager(dataDir) + + db, err := mgr.GetDB(webhookID) + require.NoError(t, err) + assert.NotNil(t, db) + require.NoError(t, mgr.CloseAll()) +} + +// TestEventDBCreateOmitsAssociations covers the connection-level +// guard directly: a Delivery carrying its Event and Target in +// memory, written through the manager's handle, must store only the +// delivery row. +func TestEventDBCreateOmitsAssociations(t *testing.T) { + t.Parallel() + + dataDir := eventDBDataDir(t) + webhookID := uuid.New().String() + + mgr := database.NewTestWebhookDBManager(dataDir) + + db, err := mgr.GetDB(webhookID) + require.NoError(t, err) + + target := database.Target{ + WebhookID: webhookID, + Name: "leaky-target", + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"https://hooks.example/secret"}`, + } + target.ID = uuid.New().String() + + event := database.Event{ + WebhookID: webhookID, + EntrypointID: uuid.New().String(), + Method: "POST", + Headers: `{}`, + Body: `{}`, + } + event.ID = uuid.New().String() + + d := &database.Delivery{ + EventID: event.ID, + TargetID: target.ID, + Status: database.DeliveryStatusPending, + Event: event, + Target: target, + } + d.ID = uuid.New().String() + + require.NoError(t, db.Create(d).Error) + require.NoError(t, db.Model(d). + Update("status", database.DeliveryStatusDelivered). + Error) + require.NoError(t, mgr.CloseAll()) + + check := openRawEventDB(t, dataDir, webhookID) + assert.Zero(t, countTargetRows(t, check)) +} diff --git a/internal/database/webhook_db_manager.go b/internal/database/webhook_db_manager.go index 0e89be1..a888ad5 100644 --- a/internal/database/webhook_db_manager.go +++ b/internal/database/webhook_db_manager.go @@ -262,6 +262,25 @@ func (m *WebhookDBManager) openDB( ) } + // Keep main-database rows out of this file. See + // event_db_isolation.go. + err = omitAssociations(db) + if err != nil { + _ = sqlDB.Close() + + return nil, fmt.Errorf( + "guarding webhook database %s: %w", + webhookID, err, + ) + } + + err = purgeTargetRows(db, m.log, webhookID) + if err != nil { + _ = sqlDB.Close() + + return nil, err + } + // Run migrations for event-tier models only err = db.AutoMigrate( &Event{}, &Delivery{}, &DeliveryResult{}, diff --git a/internal/delivery/event_db_isolation_test.go b/internal/delivery/event_db_isolation_test.go new file mode 100644 index 0000000..b865d5e --- /dev/null +++ b/internal/delivery/event_db_isolation_test.go @@ -0,0 +1,157 @@ +package delivery_test + +import ( + "context" + "database/sql" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + _ "modernc.org/sqlite" + "sneak.berlin/go/webhooker/internal/database" +) + +// assertNoTargetRows opens the per-webhook database file directly, +// outside GORM, and fails if its targets table holds any rows. +// Target config is the credential for slack and http targets, and +// event databases are the files that get backed up and handed +// around. +func assertNoTargetRows(t *testing.T, dbPath string) { + t.Helper() + + sqlDB, err := sql.Open( + "sqlite", fmt.Sprintf("file:%s?mode=ro", dbPath), + ) + require.NoError(t, err) + + defer func() { _ = sqlDB.Close() }() + + var tables int + + require.NoError(t, sqlDB.QueryRowContext( + t.Context(), + "SELECT count(*) FROM sqlite_master "+ + "WHERE type = 'table' AND name = 'targets'", + ).Scan(&tables)) + + if tables == 0 { + return + } + + var rows int + + require.NoError(t, sqlDB.QueryRowContext( + t.Context(), "SELECT count(*) FROM targets", + ).Scan(&rows)) + + assert.Zero( + t, rows, + "per-webhook event database must hold no target rows", + ) +} + +// TestEventDBHoldsNoTargetRows drives a delivery and then a retry +// through the real engine write paths and asserts neither leaves a +// target row behind in events-*.db. +func TestEventDBHoldsNoTargetRows(t *testing.T) { + t.Parallel() + + s := newISetup(t) + + ts := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + )) + defer ts.Close() + + cfg := iHTTPConfig(ts.URL) + targetID := uuid.New().String() + dbPath := s.DBMgr.DBPath(s.WebhookID) + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, `{"leak":"none"}`, + ) + body := event.Body + + // A new delivery. + d := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusPending, + ) + task := iTask( + d, event, s.WebhookID, targetID, + "leaky-target", cfg, 5, 1, &body, + ) + + s.Engine.ExportProcessNewTask(context.TODO(), &task) + + iAssertStatus( + t, s.WebhookDB, d.ID, + database.DeliveryStatusDelivered, + ) + assertNoTargetRows(t, dbPath) + + // A retry. + rd := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusRetrying, + ) + rTask := iTask( + rd, event, s.WebhookID, targetID, + "leaky-target", cfg, 5, 2, &body, + ) + + s.Engine.ExportProcessRetryTask(context.TODO(), &rTask) + + iAssertStatus( + t, s.WebhookDB, rd.ID, + database.DeliveryStatusDelivered, + ) + assertNoTargetRows(t, dbPath) +} + +// TestEventDBHoldsNoTargetRowsOnFailedDelivery covers the failure +// write path, which updates the delivery to failed and records a +// result, rather than the success path above. +func TestEventDBHoldsNoTargetRowsOnFailedDelivery(t *testing.T) { + t.Parallel() + + s := newISetup(t) + + ts := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + )) + defer ts.Close() + + cfg := iHTTPConfig(ts.URL) + targetID := uuid.New().String() + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, `{"leak":"none"}`, + ) + body := event.Body + + d := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusPending, + ) + task := iTask( + d, event, s.WebhookID, targetID, + "leaky-target", cfg, 0, 1, &body, + ) + + s.Engine.ExportProcessNewTask(context.TODO(), &task) + + iAssertStatus( + t, s.WebhookDB, d.ID, + database.DeliveryStatusFailed, + ) + assertNoTargetRows(t, s.DBMgr.DBPath(s.WebhookID)) +}