README.md

@@ -686,6 +686,44 @@ databases written by `database` targets (`archive-{uuid}.db`). Mount
 this as a persistent volume to preserve data across container
 restarts.
 
+**The bind-mounted directory must be owned by UID 1000, or the
+container does not start.** Docker creates a `-v` source path that
+does not exist yet as `root:root`, and the process runs as UID 1000,
+so it cannot take its `DATA_DIR` lock:
+
+```
+webhooker: locking data directory /var/lib/webhooker: open
+/var/lib/webhooker/webhooker.lock: permission denied
+```
+
+It exits non-zero at that point, before opening any database. Create
+the directory ahead of the first `docker run`:
+
+```bash
+mkdir -p /path/to/data
+chown 1000:1000 /path/to/data
+chmod 750 /path/to/data
+```
+
+The same `chown` is what a restore needs — see step 4 of
+[Restore](#restore). A **named volume** does not have this problem:
+Docker copies the image's ownership onto a volume it initializes, and
+the image creates `/var/lib/webhooker` owned by `webhooker`.
+
+**The file modes are not yours to set, and do not depend on the
+directory.** `webhooker.db` holds target configuration in plaintext —
+bearer tokens, API keys, Slack webhook URLs — along with the session
+encryption key, so webhooker creates every SQLite file it owns `0600`:
+each database and both of its `-wal` and `-shm` sidecars, across all
+three tiers. Files an earlier build left `0644` are tightened when
+they are opened. A `DATA_DIR` webhooker creates itself is `0750`, but
+a bind mount supplies its own directory and Docker's default for one
+it creates is `0755`; the `0600` files hold there regardless. The
+`chmod 750` above is defence in depth — it stops other local users
+listing the directory and learning your webhook UUIDs from the
+`events-{uuid}.db` filenames — not the barrier protecting the
+credentials.
+
 ## Deployment behind a reverse proxy
 
 webhooker terminates no TLS of its own. It serves plaintext HTTP and
@@ -1642,6 +1680,11 @@ webhooker uses **separate SQLite database files**: a main application
 database for configuration data and per-webhook databases for event
 storage. All database files live in the `DATA_DIR` directory.
 
+Every one of them is created `0600`, and so is each `-wal` and `-shm`
+sidecar. See
+[Running with Docker](#running-with-docker) for what that does and
+does not protect.
+
 **Main Application Database** (`{DATA_DIR}/webhooker.db`) — stores
 configuration and application state:
 

internal/database/sqlite_mode_test.go

@@ -0,0 +1,240 @@
+package database_test
+
+import (
+	"context"
+	"io/fs"
+	"net/http"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx/fxtest"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/globals"
+	"sneak.berlin/go/webhooker/internal/logger"
+)
+
+// ownerOnly is the mode every SQLite file the service owns must have.
+// Spelled out rather than referencing database.SQLiteFilePerm so the
+// test fails if the constant itself is loosened.
+const ownerOnly fs.FileMode = 0o600
+
+// requireOwnerOnly asserts that path exists and is readable and
+// writable by its owner and by nobody else.
+func requireOwnerOnly(t *testing.T, path string) {
+	t.Helper()
+
+	info, err := os.Stat(path)
+	require.NoError(t, err, "%s must exist", path)
+	assert.Equal(
+		t,
+		ownerOnly,
+		info.Mode().Perm(),
+		"%s holds credentials and must not be readable by "+
+			"anyone but its owner",
+		path,
+	)
+}
+
+// requireDatabaseSetOwnerOnly asserts the mode of a database file and
+// of both WAL sidecars. The sidecars carry the same rows as the
+// database, so tightening only the main file fixes nothing.
+func requireDatabaseSetOwnerOnly(t *testing.T, dbPath string) {
+	t.Helper()
+
+	requireOwnerOnly(t, dbPath)
+	requireOwnerOnly(t, dbPath+"-wal")
+	requireOwnerOnly(t, dbPath+"-shm")
+}
+
+// TestMainDatabaseFilesAreOwnerOnly covers the tier the defect was
+// reported against: webhooker.db holds targets.config in plaintext —
+// bearer tokens, API keys, Slack webhook URLs — and the session
+// encryption key.
+func TestMainDatabaseFilesAreOwnerOnly(t *testing.T) {
+	t.Parallel()
+
+	lc := fxtest.NewLifecycle(t)
+
+	l, err := logger.New(lc, logger.LoggerParams{
+		Globals: &globals.Globals{
+			Appname: testAppname,
+			Version: testVersion,
+		},
+	})
+	require.NoError(t, err)
+
+	// A directory the application creates itself, not one t.TempDir
+	// made at 0700, so the mode below is the application's.
+	dataDir := filepath.Join(t.TempDir(), "data")
+
+	db, err := database.New(lc, database.DatabaseParams{
+		Config: &config.Config{DataDir: dataDir},
+		Logger: l,
+	})
+	require.NoError(t, err)
+
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	// Write through the real model so the WAL is populated and both
+	// sidecars are on disk while the handle is open.
+	require.NoError(t, db.DB().Create(&database.Webhook{
+		Name: testWebhookName,
+	}).Error)
+
+	requireDatabaseSetOwnerOnly(
+		t, filepath.Join(dataDir, database.MainDBFileName),
+	)
+
+	// The data directory grants nothing to `other`. Asserted as a
+	// property rather than as an exact 0750, because MkdirAll applies
+	// the ambient umask: the exact mode is the developer's umask as
+	// much as the application's request, and pinning it would make
+	// `make check` pass or fail on where it is run. The group bits are
+	// deliberately left unasserted — deployments may rely on them.
+	info, err := os.Stat(dataDir)
+	require.NoError(t, err)
+	assert.Zero(
+		t,
+		info.Mode().Perm()&0o007,
+		"the data directory must not be world-accessible",
+	)
+}
+
+// TestPerWebhookEventDatabaseFilesAreOwnerOnly covers the events-*.db
+// tier. These carry no credential canaries since
+// https://git.eeqj.de/sneak/webhooker/issues/206, but they hold every
+// received request body and header.
+func TestPerWebhookEventDatabaseFilesAreOwnerOnly(t *testing.T) {
+	t.Parallel()
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
+
+	db, err := mgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	require.NoError(t, db.Create(&database.Event{
+		WebhookID:    webhookID,
+		EntrypointID: uuid.New().String(),
+		Method:       http.MethodPost,
+		Body:         "{}",
+	}).Error)
+
+	requireDatabaseSetOwnerOnly(t, mgr.DBPath(webhookID))
+}
+
+// TestArchiveDatabaseFilesAreOwnerOnly covers the archive-*.db tier.
+// internal/delivery builds that path and opens it through OpenSQLite,
+// the same single open path exercised here, so the mode is settled for
+// all three tiers in one place.
+func TestArchiveDatabaseFilesAreOwnerOnly(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	path := filepath.Join(
+		t.TempDir(), "archive-"+uuid.New().String()+".db",
+	)
+
+	sqlDB, err := database.OpenSQLite(path, database.SQLiteModeCreate)
+	require.NoError(t, err)
+
+	defer func() { require.NoError(t, sqlDB.Close()) }()
+
+	_, err = sqlDB.ExecContext(ctx, "create table t (id integer)")
+	require.NoError(t, err)
+
+	requireDatabaseSetOwnerOnly(t, path)
+}
+
+// TestOpenSQLiteTightensFilesLeftWorldReadable is the upgrade case: a
+// data directory an earlier build left at 0644, including a
+// developer's own scratch directory, is fixed when it is opened rather
+// than staying exposed until it is recreated.
+func TestOpenSQLiteTightensFilesLeftWorldReadable(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, database.MainDBFileName)
+
+	// A database and both sidecars as the pre-fix build left them.
+	for _, p := range []string{path, path + "-wal", path + "-shm"} {
+		require.NoError(t, os.WriteFile(p, nil, 0o644)) //nolint:gosec // the mode under test
+	}
+
+	sqlDB, err := database.OpenSQLite(path, database.SQLiteModeCreate)
+	require.NoError(t, err)
+
+	require.NoError(t, sqlDB.Close())
+
+	requireDatabaseSetOwnerOnly(t, path)
+}
+
+// TestOpenSQLiteExistingModeDoesNotCreateTheFile guards the mechanism
+// the fix uses: OpenSQLite now creates the database file itself, and
+// must not do so for a caller that asked for an existing database. An
+// empty file materialized here would turn a missing-database error
+// into a silently empty one.
+func TestOpenSQLiteExistingModeDoesNotCreateTheFile(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	path := filepath.Join(t.TempDir(), "absent.db")
+
+	sqlDB, err := database.OpenSQLite(path, database.SQLiteModeExisting)
+	if err == nil {
+		// sql.Open is lazy: force the connection that fails.
+		require.Error(t, sqlDB.PingContext(ctx))
+		require.NoError(t, sqlDB.Close())
+	}
+
+	_, statErr := os.Stat(path)
+	assert.ErrorIs(t, statErr, fs.ErrNotExist)
+}
+
+// TestReopenAfterRestartKeepsFilesOwnerOnly is the restart case: a
+// process that closed its files must be able to open them again at
+// 0600, including through a gorm handle, and the sidecars must come
+// back at 0600 too rather than at SQLite's own default.
+func TestReopenAfterRestartKeepsFilesOwnerOnly(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	dir := t.TempDir()
+	path := filepath.Join(dir, database.MainDBFileName)
+
+	first, err := database.OpenSQLite(path, database.SQLiteModeCreate)
+	require.NoError(t, err)
+
+	_, err = first.ExecContext(ctx, "create table t (id integer)")
+	require.NoError(t, err)
+	require.NoError(t, first.Close())
+
+	second, err := database.OpenSQLite(path, database.SQLiteModeCreate)
+	require.NoError(t, err)
+
+	defer func() { require.NoError(t, second.Close()) }()
+
+	_, err = second.ExecContext(ctx, "insert into t (id) values (1)")
+	require.NoError(t, err)
+
+	requireDatabaseSetOwnerOnly(t, path)
+
+	var got int
+
+	require.NoError(t,
+		second.QueryRowContext(ctx, "select id from t").Scan(&got))
+	assert.Equal(t, 1, got)
+}

internal/database/sqlite_open.go

@@ -2,8 +2,11 @@ package database
 
 import (
 	"database/sql"
+	"errors"
 	"fmt"
+	"io/fs"
 	"net/url"
+	"os"
 	"time"
 
 	_ "modernc.org/sqlite" // Pure Go SQLite driver
@@ -72,6 +75,90 @@ const (
 	sqliteConnMaxIdleTime = time.Minute
 )
 
+// SQLiteFilePerm is the mode every SQLite file this service owns is
+// created with and held at: owner read/write, nothing for group or
+// other.
+//
+// These files hold credentials in plaintext. The main database stores
+// `targets.config` — bearer tokens, API keys, Slack webhook URLs — and
+// the session encryption key. SQLite left to itself creates them 0644
+// (see reserveSQLiteFile), which made the 0750 data directory the only
+// barrier; a bind-mounted directory supplied at 0755 removes it and
+// every local user on the host can read every stored credential.
+//
+// This is a file-mode fix and not encryption at rest. An unattended
+// process needs a key it can read without a human, so the key lands
+// beside the data and an attacker who can read the database can read
+// it too. See https://git.eeqj.de/sneak/webhooker/issues/212.
+const SQLiteFilePerm fs.FileMode = 0o600
+
+// reserveSQLiteFile puts path at SQLiteFilePerm before the driver ever
+// touches it, and tightens any sidecar already on disk.
+//
+// The mode has to be settled here rather than by a chmod after opening,
+// because SQLite picks it: robust_open substitutes
+// SQLITE_DEFAULT_FILE_PERMISSIONS (0644) whenever it is handed mode 0,
+// and findCreateFileMode yields 0 for a main database opened by URI
+// with no `modeof` parameter. A chmod afterwards would leave a window
+// in which the credentials are on disk world-readable.
+//
+// Creating the file ourselves also settles the sidecars, which is the
+// half that could quietly not work. SQLite does not create those at a
+// mode we choose — it derives both from the main database file:
+// `-wal` through findCreateFileMode, which stats the path with the
+// suffix stripped, and `-shm` in unixOpenSharedMemory from an fstat of
+// the already-open database descriptor. A main file at 0600 therefore
+// produces sidecars at 0600. A zero-length file is a valid empty
+// database, so reserving it changes nothing else.
+//
+// create says whether the caller is opening in a mode that may create
+// the database. When it is false a missing file is left missing, so
+// SQLite still reports the absence rather than this function
+// materializing an empty database the caller asked not to create.
+//
+// Chmod of a file that already exists is what tightens a data
+// directory an earlier build left at 0644 — including a developer's
+// own scratch directory — without any migration machinery.
+func reserveSQLiteFile(path string, create bool) error {
+	if create {
+		// gosec G304: the path is the database file the caller asked
+		// to open, and the driver is about to open the same path
+		// anyway. Creating it here is what fixes its mode.
+		f, err := os.OpenFile( //nolint:gosec // see above
+			path, os.O_RDWR|os.O_CREATE, SQLiteFilePerm,
+		)
+		if err != nil {
+			return fmt.Errorf("creating %s: %w", path, err)
+		}
+
+		err = f.Close()
+		if err != nil {
+			return fmt.Errorf("closing %s: %w", path, err)
+		}
+	}
+
+	// O_CREATE leaves an existing file's mode alone, and umask can only
+	// have narrowed a new one. Chmod settles both cases at exactly
+	// SQLiteFilePerm.
+	for _, p := range append(
+		[]string{path}, sqliteSidecarPaths(path)...,
+	) {
+		err := os.Chmod(p, SQLiteFilePerm)
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("securing %s: %w", p, err)
+		}
+	}
+
+	return nil
+}
+
+// sqliteSidecarPaths returns the files SQLite maintains beside a
+// database under WAL. They carry the same rows as the database itself,
+// so a fix that tightens only the main file has fixed nothing.
+func sqliteSidecarPaths(path string) []string {
+	return []string{path + "-wal", path + "-shm"}
+}
+
 // SQLiteDSN builds the connection string for one database file.
 //
 // mode is the SQLite URI open mode: "rwc" to create the file when it
@@ -138,9 +225,17 @@ func SQLiteDSN(path, mode string) string {
 // durability settings and pool bounds applied. mode is the SQLite URI
 // open mode ("rwc" or "rw").
 //
+// The file and its WAL sidecars are settled at SQLiteFilePerm before
+// the driver sees the path; see reserveSQLiteFile.
+//
 // The handle is returned rather than a *gorm.DB because the callers
 // wrap it in gorm themselves with their own logger.
 func OpenSQLite(path, mode string) (*sql.DB, error) {
+	err := reserveSQLiteFile(path, mode == SQLiteModeCreate)
+	if err != nil {
+		return nil, err
+	}
+
 	sqlDB, err := sql.Open("sqlite", SQLiteDSN(path, mode))
 	if err != nil {
 		return nil, fmt.Errorf(