diff --git a/internal/cli/cli.go b/internal/cli/cli.go index e91e58d7..5619d4be 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -851,6 +851,13 @@ func initTemplateFile(cmd *cli.Command, content string, defaultPath string, labe return fmt.Errorf("failed to write %s to %s: %w", label, outputPath, err) } + // WriteFile's mode is masked by the umask and ignored outright when the + // file already exists (--force), so enforce it. This matters most for + // pg_service.conf, which holds database credentials at 0600. + if err := os.Chmod(outputPath, perm); err != nil { + return fmt.Errorf("failed to set permissions on %s: %w", outputPath, err) + } + fmt.Printf("Wrote %s to %s\n", label, outputPath) return nil } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 9a4d2411..841ccec7 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -13,6 +13,8 @@ package cli import ( "context" + "os" + "path/filepath" "strings" "testing" @@ -171,3 +173,37 @@ func TestInterspersedFlags(t *testing.T) { }) } } + +// `ace cluster init` writes database credentials, so pg_service.conf must end +// up owner-only even when --force overwrites a world-readable file: +// os.WriteFile's mode is umask-masked, and ignored for an existing file. +func TestClusterInitWritesOwnerOnlyServiceFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "pg_service.conf") + if err := os.WriteFile(path, []byte("stale"), 0o644); err != nil { + t.Fatalf("seed: %v", err) + } + if err := os.Chmod(path, 0o644); err != nil { + t.Fatalf("seed mode: %v", err) + } + + cmd := &cli.Command{ + Name: "init", + Flags: []cli.Flag{ + &cli.StringFlag{Name: "path", Value: path}, + &cli.BoolFlag{Name: "force", Value: true}, + &cli.BoolFlag{Name: "stdout"}, + }, + Action: ClusterInitCLI, + } + if err := cmd.Run(context.Background(), []string{"init"}); err != nil { + t.Fatalf("cluster init: %v", err) + } + + st, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if st.Mode().Perm()&0o077 != 0 { + t.Errorf("%s has mode %v, want owner-only for a credentials file", path, st.Mode().Perm()) + } +} diff --git a/internal/consistency/diff/spock_diff.go b/internal/consistency/diff/spock_diff.go index 1fd933b9..2febf767 100644 --- a/internal/consistency/diff/spock_diff.go +++ b/internal/consistency/diff/spock_diff.go @@ -16,7 +16,6 @@ import ( "encoding/json" "fmt" "maps" - "os" "reflect" "sort" "strings" @@ -473,7 +472,7 @@ func (t *SpockDiffTask) ExecuteTask() (err error) { return fmt.Errorf("failed to marshal diffs: %w", err) } - if err = os.WriteFile(outputFileName, jsonData, 0644); err != nil { + if err = utils.WriteFileSecure(outputFileName, jsonData); err != nil { logger.Info("ERROR writing diff output to file %s: %v", outputFileName, err) return fmt.Errorf("failed to write diffs file: %w", err) } diff --git a/internal/consistency/diff/table_rerun.go b/internal/consistency/diff/table_rerun.go index aa948111..0dfc7f7a 100644 --- a/internal/consistency/diff/table_rerun.go +++ b/internal/consistency/diff/table_rerun.go @@ -154,7 +154,7 @@ func (t *TableDiffTask) ExecuteRerunTask() error { if mErr != nil { return fmt.Errorf("failed to marshal new diff report: %w", mErr) } - if wErr := os.WriteFile(outputFileName, jsonData, 0644); wErr != nil { + if wErr := utils.WriteFileSecure(outputFileName, jsonData); wErr != nil { return fmt.Errorf("failed to write new diff report: %w", wErr) } } else { diff --git a/internal/consistency/mtree/merkle.go b/internal/consistency/mtree/merkle.go index ac05e3d6..9faad798 100644 --- a/internal/consistency/mtree/merkle.go +++ b/internal/consistency/mtree/merkle.go @@ -1802,7 +1802,7 @@ func (m *MerkleTreeTask) BuildMtree() (err error) { if err != nil { return fmt.Errorf("failed to marshal block ranges: %w", err) } - if err := os.WriteFile(filename, data, 0644); err != nil { + if err := utils.WriteFileSecure(filename, data); err != nil { return fmt.Errorf("failed to write block ranges to file: %w", err) } logger.Info("Block ranges written to %s", filename) diff --git a/internal/consistency/repair/stale_repair.go b/internal/consistency/repair/stale_repair.go index 3e7a6616..274072b8 100644 --- a/internal/consistency/repair/stale_repair.go +++ b/internal/consistency/repair/stale_repair.go @@ -414,12 +414,12 @@ func (l *staleSkipLogger) ensureOpen() error { } now := time.Now() reportDir := filepath.Join("reports", now.Format("2006-01-02")) - if err := os.MkdirAll(reportDir, 0755); err != nil { + if err := utils.MkdirAllSecure(reportDir); err != nil { return fmt.Errorf("create stale skip log directory %s: %w", reportDir, err) } fileName := fmt.Sprintf("stale_repair_skips_%s.json", now.Format("150405")+fmt.Sprintf(".%03d", now.Nanosecond()/1e6)) path := filepath.Join(reportDir, fileName) - file, err := os.Create(path) + file, err := utils.CreateFileSecure(path) if err != nil { return fmt.Errorf("create stale skip log file %s: %w", path, err) } diff --git a/internal/consistency/repair/table_repair.go b/internal/consistency/repair/table_repair.go index 32768a49..7688f75c 100644 --- a/internal/consistency/repair/table_repair.go +++ b/internal/consistency/repair/table_repair.go @@ -633,7 +633,7 @@ func writeReportToFile(report *RepairReport) error { dateFolderName := now.Format("2006-01-02") reportDir := filepath.Join(reportFolder, dateFolderName) - if err := os.MkdirAll(reportDir, 0755); err != nil { + if err := utils.MkdirAllSecure(reportDir); err != nil { return fmt.Errorf("failed to create report directory %s: %w", reportDir, err) } @@ -652,7 +652,7 @@ func writeReportToFile(report *RepairReport) error { return fmt.Errorf("failed to marshal report to JSON: %w", err) } - if err := os.WriteFile(filePath, reportData, 0644); err != nil { + if err := utils.WriteFileSecure(filePath, reportData); err != nil { return fmt.Errorf("failed to write report to file %s: %w", filePath, err) } @@ -3158,4 +3158,3 @@ func (t *TableRepairTask) setupReplicationOriginXact(tx pgx.Tx, originLSN *uint6 return nil } - diff --git a/pkg/common/html_reporter.go b/pkg/common/html_reporter.go index e4b187d9..131b280e 100644 --- a/pkg/common/html_reporter.go +++ b/pkg/common/html_reporter.go @@ -17,7 +17,6 @@ import ( "encoding/json" "fmt" "html/template" - "os" "path/filepath" "sort" "strconv" @@ -364,7 +363,7 @@ func writeHTMLDiffReport(diffResult types.DiffOutput, jsonFilePath string) (stri return "", fmt.Errorf("failed to render HTML diff report: %w", err) } - if err := os.WriteFile(htmlPath, buf.Bytes(), 0644); err != nil { + if err := WriteFileSecure(htmlPath, buf.Bytes()); err != nil { return "", fmt.Errorf("failed to write HTML diff report: %w", err) } diff --git a/pkg/common/secure_file.go b/pkg/common/secure_file.go new file mode 100644 index 00000000..22fb78db --- /dev/null +++ b/pkg/common/secure_file.go @@ -0,0 +1,68 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package common + +import ( + "fmt" + "os" +) + +// ACE writes row data copied straight out of the compared tables: diff JSON, +// HTML reports, repair reports, stale-skip logs. os.Create and os.WriteFile +// only *request* a mode, which the umask then masks off — under the usual 0022 +// that lands at 0644, readable by every local user. These helpers set the mode +// explicitly so it does not depend on the operator's umask. +const ( + SecureFileMode os.FileMode = 0o600 + SecureDirMode os.FileMode = 0o700 +) + +// CreateFileSecure creates or truncates path for writing, owner-only. The +// Chmod is not redundant: O_CREATE's mode is umask-masked, and ignored +// altogether when the file already exists. +func CreateFileSecure(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, SecureFileMode) + if err != nil { + return nil, err + } + if err := f.Chmod(SecureFileMode); err != nil { + f.Close() + return nil, fmt.Errorf("restrict permissions on %s: %w", path, err) + } + return f, nil +} + +// WriteFileSecure is os.WriteFile for files that may contain table data. +func WriteFileSecure(path string, data []byte) error { + f, err := CreateFileSecure(path) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + return f.Close() +} + +// MkdirAllSecure creates path and any missing parents, restricting path itself +// to the owner. Only the leaf is tightened, so an existing reports/ stays as +// the operator set it; the files written underneath are owner-only anyway. +func MkdirAllSecure(path string) error { + if err := os.MkdirAll(path, SecureDirMode); err != nil { + return err + } + if err := os.Chmod(path, SecureDirMode); err != nil { + return fmt.Errorf("restrict permissions on %s: %w", path, err) + } + return nil +} diff --git a/pkg/common/secure_file_test.go b/pkg/common/secure_file_test.go new file mode 100644 index 00000000..f6303315 --- /dev/null +++ b/pkg/common/secure_file_test.go @@ -0,0 +1,109 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package common + +import ( + "os" + "path/filepath" + "testing" + + "github.com/pgedge/ace/pkg/types" +) + +func assertOwnerOnly(t *testing.T, path string) { + t.Helper() + st, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + if st.Mode().Perm()&0o077 != 0 { + t.Errorf("%s has mode %v, want no group/other bits", path, st.Mode().Perm()) + } +} + +func TestWriteFileSecure(t *testing.T) { + path := filepath.Join(t.TempDir(), "diff.json") + if err := WriteFileSecure(path, []byte("rows")); err != nil { + t.Fatalf("WriteFileSecure: %v", err) + } + assertOwnerOnly(t, path) + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read back: %v", err) + } + if string(got) != "rows" { + t.Errorf("content = %q, want %q", got, "rows") + } +} + +// A file an older ACE build left world-readable must be tightened on rewrite: +// O_CREATE's mode is ignored for an existing file, so a stale 0644 would +// otherwise survive. +func TestWriteFileSecureTightensExistingFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "stale.json") + if err := os.WriteFile(path, []byte("old"), 0o644); err != nil { + t.Fatalf("seed: %v", err) + } + if err := os.Chmod(path, 0o644); err != nil { + t.Fatalf("seed mode: %v", err) + } + if err := WriteFileSecure(path, []byte("new")); err != nil { + t.Fatalf("WriteFileSecure: %v", err) + } + assertOwnerOnly(t, path) +} + +func TestMkdirAllSecure(t *testing.T) { + dir := filepath.Join(t.TempDir(), "reports", "2026-01-01") + if err := MkdirAllSecure(dir); err != nil { + t.Fatalf("MkdirAllSecure: %v", err) + } + assertOwnerOnly(t, dir) +} + +// The real writer: WriteDiffReport emits row data, so neither the JSON nor the +// HTML report may be readable by other local users. +func TestWriteDiffReportIsNotWorldReadable(t *testing.T) { + for _, format := range []string{"json", "html"} { + t.Run(format, func(t *testing.T) { + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(t.TempDir()); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { _ = os.Chdir(cwd) }) + + diff := types.DiffOutput{ + NodeDiffs: map[string]types.DiffByNodePair{ + "n1/n2": {Rows: map[string][]types.OrderedMap{"n1": {}, "n2": {}}}, + }, + Summary: types.DiffSummary{ + Schema: "public", Table: "customers", + Nodes: []string{"n1", "n2"}, PrimaryKey: []string{"id"}, + DiffRowsCount: map[string]int{"n1/n2": 0}, + }, + } + + jsonPath, htmlPath, err := WriteDiffReport(diff, "public", "customers", format) + if err != nil { + t.Fatalf("WriteDiffReport: %v", err) + } + assertOwnerOnly(t, jsonPath) + if format == "html" { + assertOwnerOnly(t, htmlPath) + } + }) + } +} diff --git a/pkg/common/secure_file_umask_test.go b/pkg/common/secure_file_umask_test.go new file mode 100644 index 00000000..dc853de4 --- /dev/null +++ b/pkg/common/secure_file_umask_test.go @@ -0,0 +1,42 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +//go:build unix + +package common + +import ( + "path/filepath" + "syscall" + "testing" +) + +// The defect was that the mode came from the ambient umask rather than from +// ACE. Pinning the umask wide open isolates that. Not parallel: umask is +// process-global. +func TestSecureWritesIgnoreUmask(t *testing.T) { + old := syscall.Umask(0) + t.Cleanup(func() { syscall.Umask(old) }) + + dir := t.TempDir() + + file := filepath.Join(dir, "diff.json") + if err := WriteFileSecure(file, []byte("rows")); err != nil { + t.Fatalf("WriteFileSecure: %v", err) + } + assertOwnerOnly(t, file) + + sub := filepath.Join(dir, "reports", "2026-01-01") + if err := MkdirAllSecure(sub); err != nil { + t.Fatalf("MkdirAllSecure: %v", err) + } + assertOwnerOnly(t, sub) +} diff --git a/pkg/common/utils.go b/pkg/common/utils.go index 527a9462..a6be8c0a 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -1534,7 +1534,7 @@ func WriteDiffReport(diffResult types.DiffOutput, schema, table, format string) jsonFileName := outputPrefix + ".json" // Stream JSON directly to file — avoids holding a second full copy in memory - f, err := os.Create(jsonFileName) + f, err := CreateFileSecure(jsonFileName) if err != nil { logger.Error("ERROR creating diff output file %s: %v", jsonFileName, err) return "", "", fmt.Errorf("failed to create diffs file: %w", err) diff --git a/pkg/taskstore/taskstore.go b/pkg/taskstore/taskstore.go index ad6b4ecd..6c3b1097 100644 --- a/pkg/taskstore/taskstore.go +++ b/pkg/taskstore/taskstore.go @@ -157,6 +157,13 @@ func New(path string) (*Store, error) { return nil, fmt.Errorf("create sqlite directory: %w", err) } + // The driver would create the database 0666&^umask, i.e. 0644 in practice. + // ace_tasks.db holds task context and diff paths, so claim it owner-only + // first, and tighten one an older build left readable. + if err := secureDBFile(sqlitePath); err != nil { + return nil, err + } + db, err := sql.Open("sqlite", sqlitePath) if err != nil { return nil, fmt.Errorf("open sqlite database: %w", err) @@ -169,9 +176,34 @@ func New(path string) (*Store, error) { db.Close() return nil, err } + + // WAL mode leaves -wal/-shm sidecars with the same permissive default. + // Best effort: they are absent outside WAL mode, and failing to tighten + // one must not stop ACE recording tasks. + for _, suffix := range []string{"-wal", "-shm"} { + _ = os.Chmod(sqlitePath+suffix, secureDBMode) + } + return s, nil } +const secureDBMode os.FileMode = 0o600 + +// secureDBFile creates path if missing and restricts it to the owner. The +// Chmod is not redundant: O_CREATE's mode is umask-masked, and ignored +// altogether when the file already exists. +func secureDBFile(path string) error { + f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, secureDBMode) + if err != nil { + return fmt.Errorf("create sqlite database %s: %w", path, err) + } + if err := f.Chmod(secureDBMode); err != nil { + f.Close() + return fmt.Errorf("restrict permissions on %s: %w", path, err) + } + return f.Close() +} + func (s *Store) Close() error { if s == nil || s.db == nil { return nil diff --git a/pkg/taskstore/taskstore_perm_test.go b/pkg/taskstore/taskstore_perm_test.go new file mode 100644 index 00000000..aa393b70 --- /dev/null +++ b/pkg/taskstore/taskstore_perm_test.go @@ -0,0 +1,55 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package taskstore + +import ( + "os" + "path/filepath" + "testing" +) + +func assertOwnerOnly(t *testing.T, path string) { + t.Helper() + st, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + if st.Mode().Perm()&0o077 != 0 { + t.Errorf("%s has mode %v, want no group/other bits", path, st.Mode().Perm()) + } +} + +// ace_tasks.db records task context and diff file paths, so the driver's +// umask-masked 0666 must not survive — on a new database or an existing one an +// older build left readable. +func TestNewCreatesOwnerOnlyDatabase(t *testing.T) { + path := filepath.Join(t.TempDir(), "ace_tasks.db") + + store, err := New(path) + if err != nil { + t.Fatalf("New: %v", err) + } + if err := store.Close(); err != nil { + t.Fatalf("close: %v", err) + } + assertOwnerOnly(t, path) + + if err := os.Chmod(path, 0o644); err != nil { + t.Fatalf("loosen mode: %v", err) + } + reopened, err := New(path) + if err != nil { + t.Fatalf("reopen: %v", err) + } + t.Cleanup(func() { _ = reopened.Close() }) + assertOwnerOnly(t, path) +}