From ba47477a8fc3f0d5ec5bf2dd370886874be1e017 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 13:49:55 +0200 Subject: [PATCH] fix(config): make a settings save durable, and keep it private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config.yaml is the file the gateway boots from, and every save from the settings UI rewrote it with os.WriteFile + Rename: no fsync of the temp file, no fsync of the directory. A rename is only atomic for bytes that already reached the disk, so a power cut mid-save could publish a truncated or zero-length config and leave an unattended gateway unbootable. Every other durable write in the repo already syncs the file, renames, then syncs the directory (state/parquet.go, driverrepo, nova/identity, state/homelink_credentials); config was the one hole. Both sync failures are now reported. The caller's contract is "the config is now saved", and a save that cannot be made durable has not met it. The temp file was also created 0644 and rename carried that mode onto a file holding MQTT passwords, API keys and OAuth refresh tokens. It is now created 0600 with O_EXCL, after clearing any temp an interrupted save left behind — OpenFile only applies the mode when it creates the file, so a stale 0644 temp would have leaked the mode through the next save. Saves are serialized: the settings handlers do not hold a write lock across a save, and two overlapping requests share one temp path. Co-Authored-By: Claude Opus 5 --- .changeset/durable-private-config-save.md | 5 + go/internal/config/config.go | 90 +++++++++++- go/internal/config/config_test.go | 163 ++++++++++++++++++++++ 3 files changed, 255 insertions(+), 3 deletions(-) create mode 100644 .changeset/durable-private-config-save.md diff --git a/.changeset/durable-private-config-save.md b/.changeset/durable-private-config-save.md new file mode 100644 index 00000000..33bade9d --- /dev/null +++ b/.changeset/durable-private-config-save.md @@ -0,0 +1,5 @@ +--- +"ftw": patch +--- + +A settings save now survives a power cut, and stops leaving the config world-readable. `config.yaml` is the file the gateway boots from, and every save from the UI rewrote it with no fsync of the temp file and no fsync of the containing directory — a rename is only atomic for bytes that already reached the disk, so losing power mid-save could publish a truncated or zero-length config and leave an unattended gateway unbootable. The save now fsyncs the temp file, renames, then fsyncs the directory, and reports a failure instead of claiming a config was saved that the next power cut can still take away. The file is also written 0600 rather than 0644: it holds MQTT passwords, API keys and OAuth refresh tokens. diff --git a/go/internal/config/config.go b/go/internal/config/config.go index 21f5ed0d..a99b357f 100644 --- a/go/internal/config/config.go +++ b/go/internal/config/config.go @@ -13,7 +13,9 @@ import ( "net/url" "os" "path/filepath" + "runtime" "strings" + "sync" "time" "github.com/srcfl/ftw/go/internal/optimizercontract" @@ -1889,8 +1891,51 @@ func (c *Config) SiteMeterDriver() string { return "" } -// SaveAtomic writes config to disk via tmp-file + rename. Safe from partial writes. +// configFileMode is owner-only because config.yaml carries MQTT passwords, +// API keys and OAuth refresh tokens. Rename replaces the destination inode, so +// whatever mode the temp file has is the mode the saved config ends up with. +const configFileMode os.FileMode = 0o600 + +// saveMu serializes config saves. The settings handlers do not hold a write +// lock across a save, so two overlapping requests would otherwise both write +// the shared temp path and rename half of each other's bytes over config.yaml. +var saveMu sync.Mutex + +// durableWriter holds the two sync calls that make a save survive power loss. +// They are fields so a test can prove the ordering and force a sync failure; +// production always uses defaultDurableWriter. +type durableWriter struct { + syncFile func(*os.File) error + syncDir func(string) error +} + +var defaultDurableWriter = durableWriter{ + syncFile: (*os.File).Sync, + syncDir: syncDir, +} + +// syncDir fsyncs a directory so a completed rename survives power loss. +// Best-effort on platforms where directories can't be fsynced (Windows). +func syncDir(dir string) error { + d, err := os.Open(dir) + if err != nil { + return err + } + defer d.Close() + if err := d.Sync(); err != nil && runtime.GOOS != "windows" { + return err + } + return nil +} + +// SaveAtomic writes config to disk via tmp-file + rename. Safe from partial +// writes and from power loss: the temp file is fsynced before the rename and +// the containing directory is fsynced after it. func SaveAtomic(path string, c *Config) error { + return saveAtomic(defaultDurableWriter, path, c) +} + +func saveAtomic(w durableWriter, path string, c *Config) error { // Driver paths are resolved to absolute-ish paths at Load() time. // Convert them back to config-relative before writing so that // repeated save cycles don't accumulate extra "../" prefixes. @@ -1908,11 +1953,50 @@ func SaveAtomic(path string, c *Config) error { if err != nil { return fmt.Errorf("yaml marshal: %w", err) } + saveMu.Lock() + defer saveMu.Unlock() + tmp := path + ".tmp" - if err := os.WriteFile(tmp, data, 0644); err != nil { + // Clear any temp left by an interrupted save, then create with O_EXCL. + // OpenFile only applies the mode when it creates the file, so reusing a + // stale 0644 temp would hand the secrets in config.yaml to every user on + // the box; O_EXCL also refuses to follow a symlink planted at that path. + if err := os.Remove(tmp); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("clear stale tmp: %w", err) + } + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, configFileMode) + if err != nil { + return fmt.Errorf("create tmp: %w", err) + } + if _, err := f.Write(data); err != nil { + f.Close() + os.Remove(tmp) return fmt.Errorf("write tmp: %w", err) } - return os.Rename(tmp, path) + // fsync before rename: a rename is only atomic for bytes that have already + // reached the disk. Without this, a power cut mid-save can publish a + // truncated or zero-length config.yaml — the file the gateway boots from, + // on a device that is expected to come back up unattended. + if err := w.syncFile(f); err != nil { + f.Close() + os.Remove(tmp) + return fmt.Errorf("sync tmp: %w", err) + } + if err := f.Close(); err != nil { + os.Remove(tmp) + return fmt.Errorf("close tmp: %w", err) + } + if err := os.Rename(tmp, path); err != nil { + os.Remove(tmp) + return fmt.Errorf("rename tmp: %w", err) + } + // fsync the directory so the rename itself survives power loss. The + // caller's contract is "the config is now saved", so this failure is + // reported rather than swallowed. + if err := w.syncDir(filepath.Dir(path)); err != nil { + return fmt.Errorf("sync config dir: %w", err) + } + return nil } func relDriverPath(baseDir, p string) string { diff --git a/go/internal/config/config_test.go b/go/internal/config/config_test.go index eea190bd..cbfcd10f 100644 --- a/go/internal/config/config_test.go +++ b/go/internal/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "os" "path/filepath" @@ -675,6 +676,168 @@ func TestSaveAtomicKeepsOutOfTreeDriverPathAbsolute(t *testing.T) { } } +// config.yaml holds MQTT passwords, API keys and OAuth refresh tokens. Rename +// replaces the destination inode, so the temp file's mode is the mode the +// operator ends up with — including when the config on disk was already +// world-readable, or when an interrupted save left a world-readable temp +// behind for the next save to reuse. +func TestSaveAtomicWritesOwnerOnlyMode(t *testing.T) { + tests := []struct { + name string + prep func(t *testing.T, path string) + }{ + { + name: "new config file", + prep: func(*testing.T, string) {}, + }, + { + name: "replacing a world-readable config", + prep: func(t *testing.T, path string) { + if err := os.WriteFile(path, []byte("site:\n name: old\n"), 0o644); err != nil { + t.Fatal(err) + } + }, + }, + { + name: "stale world-readable tmp from an interrupted save", + prep: func(t *testing.T, path string) { + if err := os.WriteFile(path+".tmp", []byte("half a config"), 0o644); err != nil { + t.Fatal(err) + } + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "c.yaml") + c, err := Parse([]byte(minimalYAML), dir) + if err != nil { + t.Fatal(err) + } + tt.prep(t, path) + if err := SaveAtomic(path, c); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Errorf("saved config mode = %04o, want 0600 — the file holds MQTT passwords and OAuth refresh tokens", got) + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Errorf("tmp file survived the save: %v", err) + } + }) + } +} + +// A rename is only atomic for bytes that already reached the disk, and the +// rename itself only survives power loss once the directory entry is synced. +// Both syncs must happen, and they must straddle the rename in that order. +func TestSaveAtomicSyncsFileBeforeRenameAndDirAfter(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "c.yaml") + c, err := Parse([]byte(minimalYAML), dir) + if err != nil { + t.Fatal(err) + } + saved := func() bool { + _, err := os.Stat(path) + return err == nil + } + var order []string + var savedAtFileSync, savedAtDirSync bool + w := durableWriter{ + syncFile: func(f *os.File) error { + order = append(order, "file") + savedAtFileSync = saved() + return f.Sync() + }, + syncDir: func(d string) error { + order = append(order, "dir") + savedAtDirSync = saved() + if d != dir { + t.Errorf("syncDir got %q, want the config's directory %q", d, dir) + } + return syncDir(d) + }, + } + if err := saveAtomic(w, path, c); err != nil { + t.Fatal(err) + } + if got := strings.Join(order, ","); got != "file,dir" { + t.Fatalf("sync order = [%s], want [file,dir]", got) + } + if savedAtFileSync { + t.Error("the temp file was fsynced after the rename; a power cut could publish a truncated config") + } + if !savedAtDirSync { + t.Error("the directory was fsynced before the rename; the rename itself would not be durable") + } +} + +// The caller's contract is "the config is now saved". A sync that fails must +// not report success, or the settings UI tells the operator a change landed +// that the next power cut can still take away. +func TestSaveAtomicReportsSyncFailure(t *testing.T) { + syncFailed := errors.New("no space left on device") + const oldConfig = "site:\n name: previous\n" + tests := []struct { + name string + writer durableWriter + keepsOldCfg bool + }{ + { + name: "temp file sync fails", + writer: durableWriter{ + syncFile: func(*os.File) error { return syncFailed }, + syncDir: syncDir, + }, + // The rename never ran, so the config the gateway boots from is + // still the one that was there before. + keepsOldCfg: true, + }, + { + name: "directory sync fails", + writer: durableWriter{ + syncFile: (*os.File).Sync, + syncDir: func(string) error { return syncFailed }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "c.yaml") + c, err := Parse([]byte(minimalYAML), dir) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(oldConfig), 0o600); err != nil { + t.Fatal(err) + } + err = saveAtomic(tt.writer, path, c) + if !errors.Is(err, syncFailed) { + t.Fatalf("saveAtomic error = %v, want it to report %v", err, syncFailed) + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Errorf("tmp file survived a failed save: %v", err) + } + if tt.keepsOldCfg { + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != oldConfig { + t.Errorf("config on disk = %q, want the previous config left untouched", got) + } + } + }) + } +} + func pretty(f float64) string { return fmt.Sprintf("%g", f) }