diff --git a/README.md b/README.md index 4690f62..4ea5878 100644 --- a/README.md +++ b/README.md @@ -666,6 +666,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 @@ -1622,6 +1660,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: diff --git a/internal/database/sqlite_mode_test.go b/internal/database/sqlite_mode_test.go new file mode 100644 index 0000000..b737c7d --- /dev/null +++ b/internal/database/sqlite_mode_test.go @@ -0,0 +1,232 @@ +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 stays group-readable. Deployments may rely on + // the group bit; the file mode is the barrier, not the directory. + info, err := os.Stat(dataDir) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o750), info.Mode().Perm()) +} + +// 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) +} diff --git a/internal/database/sqlite_open.go b/internal/database/sqlite_open.go index 9281185..9003efe 100644 --- a/internal/database/sqlite_open.go +++ b/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(