From e4874a55223edad40118857e0aa4a4a2d4c8ebb4 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 11:00:16 +0000 Subject: [PATCH] Give the local index and its export copy an explicit 0600 mode (closes #168) The local index lists every backed-up path and chunk hash, but its file mode was left to the SQLite driver and the umask, so under a typical 022 umask a fresh index (and its -wal/-shm side files) landed world-readable. The snapshot export copied the index to snapshot.db with a permissive create as well. provideDatabase now calls ensureIndexFileMode before opening the driver: it creates the index 0600 if missing and chmods an existing one to 0600. Doing this before the driver opens the file matters because SQLite gives its -wal and -shm files the mode of the main database file. The export copy is now created 0600 instead of via the umask-dependent default. Tests under umask 022 cover a fresh index, an existing 0644 index in a 0755 directory, and the export copy, asserting each ends up 0600. Model: opus-4-8 --- internal/database/module.go | 44 ++++++++++ internal/database/module_internal_test.go | 100 ++++++++++++++++++++++ internal/snapshot/export_perm_test.go | 49 +++++++++++ internal/snapshot/snapshot.go | 13 ++- 4 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 internal/database/module_internal_test.go create mode 100644 internal/snapshot/export_perm_test.go diff --git a/internal/database/module.go b/internal/database/module.go index ec8fa47..65b04b0 100644 --- a/internal/database/module.go +++ b/internal/database/module.go @@ -2,6 +2,7 @@ package database import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -15,6 +16,44 @@ import ( // the index describes the backed-up file tree and must stay private. const indexDirPerm = 0o700 +// indexFilePerm restricts the index file to the owning user; it lists every +// backed-up path and chunk hash and must stay private. +const indexFilePerm = 0o600 + +// ensureIndexFileMode makes the index file owner-only before the SQLite +// driver opens it: it creates the file 0600 if absent, or chmods an existing +// one to 0600. Doing this first matters because SQLite creates its -wal and +// -shm side files with the mode of the main database file, so a private main +// file yields private side files. The driver treats a zero-byte file as an +// empty database, so pre-creating it here is safe. +func ensureIndexFileMode(path string) error { + info, err := os.Stat(path) + + switch { + case err == nil: + if info.Mode().Perm() == indexFilePerm { + return nil + } + + err = os.Chmod(path, indexFilePerm) + if err != nil { + return fmt.Errorf("restricting index file permissions: %w", err) + } + + return nil + case errors.Is(err, os.ErrNotExist): + //nolint:gosec // G304: the index path is operator-configured by design + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, indexFilePerm) + if err != nil { + return fmt.Errorf("creating index file: %w", err) + } + + return f.Close() + default: + return fmt.Errorf("checking index file: %w", err) + } +} + // Module provides database dependencies // //nolint:gochecknoglobals // fx module definitions are package globals by convention @@ -34,6 +73,11 @@ func provideDatabase(lc fx.Lifecycle, cfg *config.Config) (*DB, error) { return nil, fmt.Errorf("creating index directory: %w", err) } + err = ensureIndexFileMode(cfg.IndexPath) + if err != nil { + return nil, err + } + db, err := New(context.Background(), cfg.IndexPath) if err != nil { return nil, fmt.Errorf("opening database: %w", err) diff --git a/internal/database/module_internal_test.go b/internal/database/module_internal_test.go new file mode 100644 index 0000000..80d89ae --- /dev/null +++ b/internal/database/module_internal_test.go @@ -0,0 +1,100 @@ +package database + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "go.uber.org/fx/fxtest" + "sneak.berlin/go/vaultik/internal/config" +) + +// TestProvideDatabaseFreshIndexMode verifies that provideDatabase creates a +// missing index file owner-only (0600), even under a lenient 022 umask that +// would otherwise leave a freshly created file world-readable. +// +//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash +func TestProvideDatabaseFreshIndexMode(t *testing.T) { + restore := syscall.Umask(0o022) + defer syscall.Umask(restore) + + indexPath := filepath.Join(t.TempDir(), "index.sqlite") + + openIndex(t, indexPath) + assertPerm(t, indexPath, 0o600) +} + +// TestProvideDatabaseExistingIndexMode verifies that provideDatabase tightens +// an existing world-readable index (0644) in a group/other-readable directory +// down to owner-only (0600). +// +//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash +func TestProvideDatabaseExistingIndexMode(t *testing.T) { + restore := syscall.Umask(0o022) + defer syscall.Umask(restore) + + dir := filepath.Join(t.TempDir(), "data") + + //nolint:gosec // G301: the test intentionally uses a 0755 directory + err := os.MkdirAll(dir, 0o755) + if err != nil { + t.Fatalf("creating index directory: %v", err) + } + + //nolint:gosec // G302: the test intentionally uses a 0755 directory + err = os.Chmod(dir, 0o755) + if err != nil { + t.Fatalf("relaxing index directory permissions: %v", err) + } + + indexPath := filepath.Join(dir, "index.sqlite") + + //nolint:gosec // G306: the test intentionally starts from a 0644 index + err = os.WriteFile(indexPath, nil, 0o644) + if err != nil { + t.Fatalf("creating pre-existing index: %v", err) + } + + //nolint:gosec // G302: the test intentionally starts from a 0644 index + err = os.Chmod(indexPath, 0o644) + if err != nil { + t.Fatalf("relaxing pre-existing index permissions: %v", err) + } + + openIndex(t, indexPath) + assertPerm(t, indexPath, 0o600) +} + +// openIndex runs provideDatabase against indexPath and closes the resulting +// database before returning. +func openIndex(t *testing.T, indexPath string) { + t.Helper() + + cfg := &config.Config{IndexPath: indexPath} + + db, err := provideDatabase(fxtest.NewLifecycle(t), cfg) + if err != nil { + t.Fatalf("provideDatabase: %v", err) + } + + err = db.Close() + if err != nil { + t.Fatalf("closing database: %v", err) + } +} + +// assertPerm fails the test unless path has exactly the given permission bits. +func assertPerm(t *testing.T, path string, want os.FileMode) { + t.Helper() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + + got := info.Mode().Perm() + if got != want { + t.Fatalf("permissions of %s = %#o, want %#o", path, got, want) + } +} diff --git a/internal/snapshot/export_perm_test.go b/internal/snapshot/export_perm_test.go new file mode 100644 index 0000000..84e407a --- /dev/null +++ b/internal/snapshot/export_perm_test.go @@ -0,0 +1,49 @@ +//nolint:testpackage // exercises the unexported copyFile helper +package snapshot + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/spf13/afero" +) + +// TestCopyFileExportCopyMode verifies that the exported snapshot database +// copy is created owner-only (0600), even under a lenient 022 umask that +// would otherwise leave a fresh file world-readable. +// +//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash +func TestCopyFileExportCopyMode(t *testing.T) { + restore := syscall.Umask(0o022) + defer syscall.Umask(restore) + + dir := t.TempDir() + + src := filepath.Join(dir, "index.sqlite") + + err := os.WriteFile(src, []byte("index data"), 0o600) + if err != nil { + t.Fatalf("creating source index: %v", err) + } + + dst := filepath.Join(dir, "snapshot.db") + + sm := &SnapshotManager{fs: afero.NewOsFs()} + + err = sm.copyFile(src, dst) + if err != nil { + t.Fatalf("copyFile: %v", err) + } + + info, err := os.Stat(dst) + if err != nil { + t.Fatalf("stat export copy: %v", err) + } + + got := info.Mode().Perm() + if got != 0o600 { + t.Fatalf("export copy permissions = %#o, want %#o", got, 0o600) + } +} diff --git a/internal/snapshot/snapshot.go b/internal/snapshot/snapshot.go index 9e861ce..2e77656 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -44,6 +44,7 @@ import ( "errors" "fmt" "io" + "os" "path/filepath" "strings" "time" @@ -763,7 +764,13 @@ func (sm *SnapshotManager) compressFile(inputPath, outputPath string) error { return nil } -// copyFile copies a file from src to dst +// exportCopyPerm restricts the exported snapshot database copy to the owning +// user; it holds the same private index data as the local index file. +const exportCopyPerm = 0o600 + +// copyFile copies a file from src to dst. The destination is the exported +// snapshot database, so it is created owner-only rather than with the +// umask-dependent default. func (sm *SnapshotManager) copyFile(src, dst string) error { log.Debug("Opening source file for copy", "path", src) @@ -783,7 +790,9 @@ func (sm *SnapshotManager) copyFile(src, dst string) error { log.Debug("Creating destination file", "path", dst) - destFile, err := sm.fs.Create(dst) + destFile, err := sm.fs.OpenFile( + dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, exportCopyPerm, + ) if err != nil { return err } -- 2.54.0