Stop target credentials leaking into event databases (closes #206)
Some checks failed
check / check (push) Failing after 2m36s
Some checks failed
check / check (push) Failing after 2m36s
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.
This commit is contained in:
157
internal/delivery/event_db_isolation_test.go
Normal file
157
internal/delivery/event_db_isolation_test.go
Normal file
@@ -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))
|
||||
}
|
||||
Reference in New Issue
Block a user