Create every SQLite file 0600 (closes #255)
All checks were successful
check / check (push) Successful in 3m24s
All checks were successful
check / check (push) Successful in 3m24s
webhooker.db holds target configuration in plaintext — bearer tokens, API keys, Slack webhook URLs — and the session encryption key, and it was created 0644. The 0750 data directory was therefore the only barrier, and a Docker bind mount supplies that directory at 0755, which removes it: every local user on the host could read every stored credential. The mode is settled in OpenSQLite, the single open path all three tiers share, so the main database, the per-webhook event databases and the archive databases are covered in one place. 0644 comes from SQLite itself: robust_open substitutes SQLITE_DEFAULT_FILE_PERMISSIONS whenever it is handed mode 0, and findCreateFileMode yields 0 for a main database opened by URI with no `modeof` parameter. A chmod after opening would leave a window in which the credentials are on disk world-readable, so OpenSQLite creates the file itself at 0600 before the driver sees the path. That also settles the WAL sidecars, which carry the same rows and would otherwise leave the fix worthless. SQLite 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 open database descriptor — so a main file at 0600 produces sidecars at 0600. Verified by stat rather than by reading the driver: a pre-change build leaves webhooker.db, -wal and -shm all 644; this one leaves all three 600, in a 0755 bind mount, for all three tiers, with delivery and restart working. Existing files are chmodded on open, so a directory an earlier build left 0644 — including a developer's scratch directory — is fixed without any migration machinery. This is 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. The data directory stays 0750: the group bit may matter to a deployment, and with the files at 0600 the directory is no longer the barrier. README: the Docker section now states the DATA_DIR ownership requirement where the bind mount is documented. A `-v` source path Docker creates is root:root, and the container runs as UID 1000, so it fails to start on the lock file; the chown that fixes it appeared only under Restore.
This commit is contained in:
232
internal/database/sqlite_mode_test.go
Normal file
232
internal/database/sqlite_mode_test.go
Normal file
@@ -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)
|
||||
}
|
||||
Reference in New Issue
Block a user