diff --git a/internal/storage/bbolt.go b/internal/storage/bbolt.go index d23806906..0ee7fa182 100644 --- a/internal/storage/bbolt.go +++ b/internal/storage/bbolt.go @@ -39,8 +39,10 @@ type BoltDB struct { func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { dbPath := filepath.Join(dataDir, "config.db") - // Try to open with timeout, if it fails, immediately return database locked error - db, err := bbolt.Open(dbPath, 0644, &bbolt.Options{ + // Try to open with timeout, if it fails, immediately return database locked error. + // The database holds OAuth tokens and DCR client secrets, so it is created + // owner-only (0600). + db, err := bbolt.Open(dbPath, 0600, &bbolt.Options{ Timeout: 10 * time.Second, }) if err != nil { @@ -59,6 +61,10 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { return nil, fmt.Errorf("failed to open bolt database: %w", err) } + // The mode passed to bbolt.Open only applies when the file is created, so + // databases created by older builds keep their 0644 mode. Tighten them here. + tightenFilePermissions(dbPath, logger) + boltDB := &BoltDB{ db: db, logger: logger, @@ -73,6 +79,34 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { return boltDB, nil } +// tightenFilePermissions clears group and other permission bits from path, +// leaving owner bits untouched so a deliberately stricter mode is never widened. +// It is best-effort and never prevents the proxy from starting: a mode that +// cannot be tightened (read-only mount, exotic filesystem, Windows) is logged +// at warn level (the default log level is info) rather than blocking startup, +// because the file holds OAuth tokens and DCR client secrets and a failure +// here means it silently stays group/world readable - that must be visible +// to an operator without enabling debug logging. +func tightenFilePermissions(path string, logger *zap.SugaredLogger) { + info, err := os.Stat(path) + if err != nil { + logger.Warnf("Could not stat %s to check file permissions: %v", path, err) + return + } + + perm := info.Mode().Perm() + if perm&0o077 == 0 { + return + } + + if err := os.Chmod(path, perm&^0o077); err != nil { + logger.Warnf("Could not tighten permissions on %s: %v", path, err) + return + } + + logger.Infof("Tightened permissions on %s from %#o to %#o", path, perm, perm&^0o077) +} + // Close closes the database func (b *BoltDB) Close() error { return b.db.Close() @@ -663,11 +697,54 @@ func (b *BoltDB) DeleteServerPromptApprovals(serverName string) error { // Generic operations -// Backup creates a backup of the database +// Backup creates a backup of the database. +// The copy carries the same secrets as the live database and the destination is +// caller-chosen (potentially outside the 0700 data directory), so it is written +// owner-only. func (b *BoltDB) Backup(destPath string) error { - return b.db.View(func(tx *bbolt.Tx) error { - return tx.CopyFile(destPath, 0644) - }) + // bbolt's Tx.CopyFile opens the destination with O_CREATE|O_TRUNC, so its + // mode argument is ignored when the file already exists: writing straight to + // destPath would pour the database into whatever permissions a stale backup + // happened to carry, and a chmod afterwards comes too late. Stage the copy in + // an owner-only temporary file (os.CreateTemp creates it 0600) next to the + // destination, then rename it into place - the rename keeps the temp file's + // inode and so carries the 0600 mode with it (atomically on POSIX; Go makes + // no atomicity promise on Windows). + tmpFile, err := os.CreateTemp(filepath.Dir(destPath), ".config.db.backup-*") + if err != nil { + return fmt.Errorf("failed to create temporary backup file: %w", err) + } + + tmpPath := tmpFile.Name() + renamed := false + defer func() { + tmpFile.Close() //nolint:errcheck // best-effort cleanup; Close below is the checked one + if !renamed { + os.Remove(tmpPath) //nolint:errcheck // best-effort cleanup + } + }() + + if err := b.db.View(func(tx *bbolt.Tx) error { + _, writeErr := tx.WriteTo(tmpFile) + return writeErr + }); err != nil { + return fmt.Errorf("failed to write backup: %w", err) + } + + if err := tmpFile.Sync(); err != nil { + return fmt.Errorf("failed to flush backup: %w", err) + } + + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close backup: %w", err) + } + + if err := os.Rename(tmpPath, destPath); err != nil { + return fmt.Errorf("failed to move backup into place: %w", err) + } + renamed = true + + return nil } // Stats returns database statistics diff --git a/internal/storage/bbolt_file_mode_test.go b/internal/storage/bbolt_file_mode_test.go new file mode 100644 index 000000000..f7dd33e03 --- /dev/null +++ b/internal/storage/bbolt_file_mode_test.go @@ -0,0 +1,204 @@ +package storage + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.etcd.io/bbolt" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +// skipOnWindows guards the permission assertions below: os.Chmod on Windows only +// toggles the read-only bit and os.Stat reports 0666, so POSIX mode bits are +// meaningless there. CI runs the unit tests on Windows too. +func skipOnWindows(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("POSIX file mode bits are not meaningful on Windows") + } +} + +// TestNewBoltDBCreatesDatabaseOwnerOnly pins that a freshly created config.db is +// owner-only: it holds OAuth access/refresh tokens and DCR client secrets. +func TestNewBoltDBCreatesDatabaseOwnerOnly(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + info, err := os.Stat(filepath.Join(dir, "config.db")) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "a freshly created config.db must not be group/world readable") +} + +// TestNewBoltDBTightensWorldReadableDatabaseOnOpen covers existing installs: +// bbolt.Open's mode argument applies only at creation, so an already-created +// 0644 database must be chmod-migrated on open. +func TestNewBoltDBTightensWorldReadableDatabaseOnOpen(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + dbPath := filepath.Join(dir, "config.db") + + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db.Close()) + + // Simulate a database created by an older build. + require.NoError(t, os.Chmod(dbPath, 0o644)) + + db2, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db2.Close()) + + info, err := os.Stat(dbPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "an existing world-readable config.db must be tightened on open") +} + +// TestNewBoltDBPreservesOwnerBitsWhenTightening makes sure the migration only +// clears group/other bits instead of assigning 0600 outright, so it can never +// widen a mode the owner chose. +func TestNewBoltDBPreservesOwnerBitsWhenTightening(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + dbPath := filepath.Join(dir, "config.db") + + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db.Close()) + + require.NoError(t, os.Chmod(dbPath, 0o744)) + + db2, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db2.Close()) + + info, err := os.Stat(dbPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o700), info.Mode().Perm(), + "only group/other bits should be cleared; owner bits must be preserved") +} + +// TestBackupWritesOwnerOnlyCopy pins the backup copy: the destination is an +// operator-chosen path that may sit outside the 0700 data directory, where a +// 0644 copy really would be world-readable. +func TestBackupWritesOwnerOnlyCopy(t *testing.T) { + skipOnWindows(t) + + db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + backupPath := filepath.Join(t.TempDir(), "backup.db") + require.NoError(t, db.Backup(backupPath)) + + info, err := os.Stat(backupPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "database backups must not be group/world readable") + + // The copy must still be a usable database, not just a well-permissioned file. + restored, err := bbolt.Open(backupPath, 0600, &bbolt.Options{ + ReadOnly: true, + Timeout: 5 * time.Second, + }) + require.NoError(t, err) + require.NoError(t, restored.Close()) +} + +// TestBackupTightensExistingDestination covers overwriting an earlier backup: +// bbolt's CopyFile opens the destination with O_CREATE|O_TRUNC, so its mode +// argument applies only when the destination does not already exist. The backup +// must not be written into a pre-existing world-readable file at all, so it is +// staged in an owner-only temporary file and renamed into place - which means +// the old destination inode is replaced rather than truncated and rewritten. +func TestBackupTightensExistingDestination(t *testing.T) { + skipOnWindows(t) + + db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + destDir := t.TempDir() + backupPath := filepath.Join(destDir, "backup.db") + // A backup left behind by an older build. + require.NoError(t, os.WriteFile(backupPath, []byte("stale"), 0o644)) + + before, err := os.Stat(backupPath) + require.NoError(t, err) + + require.NoError(t, db.Backup(backupPath)) + + after, err := os.Stat(backupPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), after.Mode().Perm(), + "overwriting an existing backup must also tighten its permissions") + require.False(t, os.SameFile(before, after), + "database bytes must never be written into the pre-existing, possibly world-readable file; "+ + "stage in an owner-only temp file and rename over the destination") + + entries, err := os.ReadDir(destDir) + require.NoError(t, err) + require.Len(t, entries, 1, "backup must not leave temporary files behind: %v", entries) +} + +// TestTightenFilePermissionsLogsStatFailureAtWarn covers SEC-03 follow-up +// review: the database holds OAuth tokens and DCR client secrets, so an +// operator running at the project's default log level (Info) must be able to +// see that the permission-tightening migration did not run, instead of the +// failure disappearing into Debug output nobody has enabled. +func TestTightenFilePermissionsLogsStatFailureAtWarn(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + logger := zap.New(core).Sugar() + + tightenFilePermissions(filepath.Join(t.TempDir(), "does-not-exist.db"), logger) + + entries := logs.FilterMessageSnippet("Could not stat").All() + require.Len(t, entries, 1, "a stat failure must be logged") + require.Equal(t, zapcore.WarnLevel, entries[0].Level, + "a stat failure must be visible at the project's default (Info) log level") +} + +// TestTightenFilePermissionsLogsChmodFailureAtWarn reproduces one of the +// review's named real-world causes (macOS uchg/schg flag) for a chmod that +// cannot succeed even though the process owns the file: darwin's immutable +// flag makes chmod fail with EPERM regardless of ownership. +func TestTightenFilePermissionsLogsChmodFailureAtWarn(t *testing.T) { + skipOnWindows(t) + if runtime.GOOS != "darwin" { + t.Skip("uses chflags uchg to force an owner-proof chmod failure; darwin-only") + } + + dir := t.TempDir() + path := filepath.Join(dir, "config.db") + require.NoError(t, os.WriteFile(path, []byte("x"), 0o644)) + + require.NoError(t, exec.Command("chflags", "uchg", path).Run()) + defer func() { + _ = exec.Command("chflags", "nouchg", path).Run() + }() + + core, logs := observer.New(zapcore.DebugLevel) + logger := zap.New(core).Sugar() + + tightenFilePermissions(path, logger) + + entries := logs.FilterMessageSnippet("Could not tighten permissions").All() + require.Len(t, entries, 1, "a chmod failure must be logged") + require.Equal(t, zapcore.WarnLevel, entries[0].Level, + "a chmod failure must be visible at the project's default (Info) log level, "+ + "since it means the DB is left silently world-readable") +}