From 33b2e140e904d4f8360b8a532e481ffb2f9765e9 Mon Sep 17 00:00:00 2001 From: dborup Date: Wed, 7 Oct 2026 19:51:33 +0200 Subject: [PATCH 1/2] security(config): preserve restrictive permissions during atomic geo-filter saves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SaveGeoFilter wrote config.json.tmp with os.WriteFile(..., 0644) and renamed it over config.json. A config deliberately restricted to 0600 therefore came back 0644 after any geo-filter edit in the UI, and config.json can hold broker credentials. The fixed temp path was also a write target an attacker with directory access could pre-create as a symlink or hard link. The replacement now: - reads mode and contents from the same descriptor, and fails rather than guessing a default when that metadata is unavailable; - creates a uniquely named temp file in the destination directory (os.CreateTemp, O_EXCL), so a pre-existing config.json.tmp is never opened, written through or removed; - applies the preserved mode with an explicit chmod on its own descriptor, which makes the result umask-independent — copying upstream's WriteFile mode argument would fix neither case; - carries uid/gid over best-effort, a no-op in the normal case and deliberately non-fatal so an unprivileged server (#339) can still save. Relates to #340. --- cmd/server/config.go | 106 +++- cmd/server/config_geofilter_perms_340_test.go | 490 ++++++++++++++++++ .../config_geofilter_perms_340_unix_test.go | 80 +++ cmd/server/config_owner_unix.go | 19 + cmd/server/config_owner_windows.go | 11 + 5 files changed, 700 insertions(+), 6 deletions(-) create mode 100644 cmd/server/config_geofilter_perms_340_test.go create mode 100644 cmd/server/config_geofilter_perms_340_unix_test.go create mode 100644 cmd/server/config_owner_unix.go create mode 100644 cmd/server/config_owner_windows.go diff --git a/cmd/server/config.go b/cmd/server/config.go index d28acf49f..14189b3bb 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "fmt" + "io" "log" "math" "os" @@ -1137,8 +1138,37 @@ func (c *Config) HiddenNamePrefixesGeneration() uint64 { return c.hiddenPrefixesGen.Load() } +// Test seams for the atomic config replacement in SaveGeoFilter (#340). +// Production always uses the os functions; tests swap them to provoke +// metadata, write and rename failures, which no portable filesystem setup can +// trigger reliably. +var ( + configCreateTemp = os.CreateTemp + configRename = os.Rename + configStatFile = func(f *os.File) (os.FileInfo, error) { return f.Stat() } +) + // SaveGeoFilter writes the geo_filter section back to config.json on disk. // Pass gf=nil to remove the filter. The rest of config.json is preserved as-is. +// +// The replacement is atomic and permission-preserving (#340). config.json can +// hold broker credentials, so a file deliberately restricted to 0600 must not +// come back 0644 just because the geo filter was edited in the UI — and a +// deliberately group-readable 0644 must not silently tighten either. Three +// properties carry that: +// +// - The mode is read from the same descriptor the contents are read from, so +// the bits written back belong to the bytes that were parsed. If that +// metadata is unavailable the save fails instead of guessing a default. +// - The replacement file is created under a unique name in the destination +// directory via os.CreateTemp (O_EXCL), so a pre-existing config.json.tmp +// — stale file, planted symlink or hard link to somewhere else — is never +// opened, never written through and never removed by this function. +// - The preserved mode is applied with an explicit chmod on our own +// descriptor, which makes the result independent of the process umask. +// Copying upstream's WriteFile mode argument would not do either: the mode +// argument is ignored for a file that already exists, and it is masked by +// the umask for one that does not. func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { var configPath string for _, p := range []string{ @@ -1154,7 +1184,20 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { return fmt.Errorf("config.json not found in %s", configDir) } - data, err := os.ReadFile(configPath) + src, err := os.Open(configPath) + if err != nil { + return fmt.Errorf("read config: %w", err) + } + info, err := configStatFile(src) + if err != nil { + src.Close() + return fmt.Errorf("stat config: %w", err) + } + mode := info.Mode().Perm() + data, err := io.ReadAll(src) + if cerr := src.Close(); err == nil { + err = cerr + } if err != nil { return fmt.Errorf("read config: %w", err) } @@ -1181,18 +1224,69 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { } out = append(out, '\n') - // Atomic write: temp file + rename. - tmp := configPath + ".tmp" - if err := os.WriteFile(tmp, out, 0644); err != nil { + // Atomic write: unique temp file in the destination directory + rename. + tmpFile, err := configCreateTemp(filepath.Dir(configPath), filepath.Base(configPath)+".*.tmp") + if err != nil { + return fmt.Errorf("create temp config: %w", err) + } + tmpName := tmpFile.Name() + renamed := false + defer func() { + if renamed { + return + } + // Only ever our own uniquely named file; a pre-existing + // config.json.tmp is left alone. + tmpFile.Close() + os.Remove(tmpName) + }() + + if err := tmpFile.Chmod(mode); err != nil { + return fmt.Errorf("chmod temp config: %w", err) + } + preserveConfigOwner(tmpFile, info) + if _, err := tmpFile.Write(out); err != nil { return fmt.Errorf("write config: %w", err) } - if err := os.Rename(tmp, configPath); err != nil { - os.Remove(tmp) + if err := tmpFile.Sync(); err != nil { + return fmt.Errorf("sync config: %w", err) + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("close temp config: %w", err) + } + if err := configRename(tmpName, configPath); err != nil { return fmt.Errorf("rename config: %w", err) } + renamed = true return nil } +// preserveConfigOwner best-effort carries the replaced file's uid/gid over to +// its replacement. It is a no-op in the normal case, where the saving process +// already owns the file it is rewriting — the only arrangement in which it can +// write at all once the server runs as a non-root container user (#339). +// +// It matters only for a privileged process rewriting a bind-mounted config +// owned by a host user: without the chown the file would silently become +// root-owned and the host user could no longer edit it. An unprivileged +// process cannot chown to a foreign uid, so the failure is ignored rather than +// aborting a save whose security-relevant part — the permission bits — already +// succeeded. +func preserveConfigOwner(f *os.File, want os.FileInfo) { + wantUID, wantGID, ok := configFileOwnerIDs(want) + if !ok { + return + } + cur, err := configStatFile(f) + if err != nil { + return + } + if curUID, curGID, ok := configFileOwnerIDs(cur); ok && curUID == wantUID && curGID == wantGID { + return + } + _ = f.Chown(wantUID, wantGID) +} + // obsBlacklistSet lazily builds and caches the observerBlacklist as a set for O(1) lookups. func (c *Config) obsBlacklistSet() map[string]bool { c.obsBlacklistOnce.Do(func() { diff --git a/cmd/server/config_geofilter_perms_340_test.go b/cmd/server/config_geofilter_perms_340_test.go new file mode 100644 index 000000000..ff900bd77 --- /dev/null +++ b/cmd/server/config_geofilter_perms_340_test.go @@ -0,0 +1,490 @@ +package main + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// Tests for #340: SaveGeoFilter must replace config.json atomically without +// widening its permissions and without ever writing through a pre-existing +// temporary path. config.json can hold credentials, so a deliberate 0600 must +// survive a geo-filter save. + +const geoPerm340Config = `{ + "_comment": "keep me", + "port": 3000, + "mqtt": {"username": "redacted-test-user", "password": "redacted-test-secret"}, + "areas": [{"label": "A", "polygon": [[1, 2], [3, 4], [5, 6]]}] +} +` + +func geoPerm340Filter() *GeoFilterConfig { + return &GeoFilterConfig{ + Polygon: [][2]float64{{51.0, 4.0}, {51.0, 5.0}, {50.5, 4.0}}, + BufferKm: 20, + } +} + +// writeGeoPerm340Config writes the fixture config at path with mode and +// returns path. The explicit chmod defeats the test process umask so the +// recorded starting mode is exact. +func writeGeoPerm340Config(t *testing.T, path string, mode os.FileMode) string { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(geoPerm340Config), mode); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + return path +} + +func geoPerm340Mode(t *testing.T, path string) os.FileMode { + t.Helper() + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + return fi.Mode().Perm() +} + +// assertNoStrayTemp fails if dir holds any entry whose name looks like a +// temporary file left behind by SaveGeoFilter. Entries listed in keep are +// allowed (pre-existing files the save must not touch). +func assertNoStrayTemp(t *testing.T, dir string, keep ...string) { + t.Helper() + allowed := map[string]bool{"config.json": true} + for _, k := range keep { + allowed[k] = true + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read dir: %v", err) + } + for _, e := range entries { + if allowed[e.Name()] { + continue + } + if strings.Contains(e.Name(), ".tmp") || strings.HasPrefix(e.Name(), "config.json.") { + t.Errorf("leftover temp file %q in %s", e.Name(), dir) + } + } +} + +// TestSaveGeoFilterPreservesMode is the core #340 regression: the saved file +// keeps the permission bits it had, both for a locked-down 0600 config and for +// an intentionally group/world-readable one. +func TestSaveGeoFilterPreservesMode(t *testing.T) { + for _, mode := range []os.FileMode{0o600, 0o640, 0o644, 0o664} { + t.Run(mode.String(), func(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), mode) + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != mode { + t.Errorf("mode after save = %04o, want %04o", got, mode) + } + + // A removal save must preserve the mode too. + if err := SaveGeoFilter(dir, nil); err != nil { + t.Fatalf("SaveGeoFilter(nil): %v", err) + } + if got := geoPerm340Mode(t, path); got != mode { + t.Errorf("mode after removal save = %04o, want %04o", got, mode) + } + assertNoStrayTemp(t, dir) + }) + } +} + +// TestSaveGeoFilterFailsSafelyOnUnreadableConfig covers the "handle metadata / +// read failures safely rather than silently choosing permissive defaults" +// criterion: an unreadable config must surface an error and stay untouched. +func TestSaveGeoFilterFailsSafelyOnUnreadableConfig(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses file permission checks") + } + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o000) + + err := SaveGeoFilter(dir, geoPerm340Filter()) + if err == nil { + t.Fatal("expected an error for an unreadable config, got nil") + } + if got := geoPerm340Mode(t, path); got != 0o000 { + t.Errorf("mode after failed save = %04o, want 0000", got) + } + assertNoStrayTemp(t, dir) +} + +// TestSaveGeoFilterDanglingSymlinkConfigFails: when the only config.json is a +// dangling symlink, the lookup must report "not found" instead of creating a +// fresh file through the link with default permissions. +func TestSaveGeoFilterDanglingSymlinkConfigFails(t *testing.T) { + dir := t.TempDir() + link := filepath.Join(dir, "config.json") + if err := os.Symlink(filepath.Join(dir, "missing.json"), link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + if err := SaveGeoFilter(dir, geoPerm340Filter()); err == nil { + t.Fatal("expected an error for a dangling config.json symlink, got nil") + } + if _, err := os.Stat(filepath.Join(dir, "missing.json")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("symlink target was created: stat err = %v", err) + } +} + +// TestSaveGeoFilterIgnoresPreexistingTempPath: a stale config.json.tmp — here a +// directory, which no write can ever succeed through — must not block the save, +// because the temp file is created under a unique name. +func TestSaveGeoFilterIgnoresPreexistingTempPath(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + stale := path + ".tmp" + if err := os.Mkdir(stale, 0o700); err != nil { + t.Fatal(err) + } + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after save = %04o, want 0600", got) + } + fi, err := os.Stat(stale) + if err != nil || !fi.IsDir() { + t.Errorf("pre-existing %s was disturbed: dir=%v err=%v", filepath.Base(stale), err == nil && fi.IsDir(), err) + } + assertNoStrayTemp(t, dir, "config.json.tmp") +} + +// TestSaveGeoFilterDoesNotFollowTempSymlink: writing through a planted +// config.json.tmp symlink would copy config contents — credentials included — +// to an attacker-chosen path. +func TestSaveGeoFilterDoesNotFollowTempSymlink(t *testing.T) { + dir := t.TempDir() + victimDir := t.TempDir() + victim := filepath.Join(victimDir, "victim.txt") + const victimBody = "victim-untouched\n" + if err := os.WriteFile(victim, []byte(victimBody), 0o600); err != nil { + t.Fatal(err) + } + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + link := path + ".tmp" + if err := os.Symlink(victim, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + got, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("read victim: %v", err) + } + if string(got) != victimBody { + t.Errorf("symlink target was written through: %q", string(got)) + } + if fi, err := os.Lstat(link); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("planted symlink was replaced: mode=%v err=%v", fi, err) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after save = %04o, want 0600", got) + } +} + +// TestSaveGeoFilterDoesNotFollowTempHardLink is the same leak via a hard link, +// which no O_NOFOLLOW flag can catch — only a unique temp name can. +func TestSaveGeoFilterDoesNotFollowTempHardLink(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "victim.txt") + const victimBody = "victim-untouched\n" + if err := os.WriteFile(victim, []byte(victimBody), 0o600); err != nil { + t.Fatal(err) + } + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + if err := os.Link(victim, path+".tmp"); err != nil { + t.Skipf("hard links unsupported: %v", err) + } + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + got, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("read victim: %v", err) + } + if string(got) != victimBody { + t.Errorf("hard-linked file was written through: %q", string(got)) + } +} + +// TestSaveGeoFilterCreateTempFailure: when the destination directory is not +// writable the save must fail loudly and leave the config untouched. +func TestSaveGeoFilterCreateTempFailure(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permission checks") + } + parent := t.TempDir() + dir := filepath.Join(parent, "cfg") + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err == nil { + t.Fatal("expected an error when the config directory is read-only, got nil") + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config: %v", err) + } + if string(data) != geoPerm340Config { + t.Error("config was modified despite the failed save") + } +} + +// TestSaveGeoFilterWriteFailureCleansUpOwnTemp exercises a mid-write failure +// through the create-temp seam: the temp file must be removed, a pre-existing +// config.json.tmp must survive, and the config must be unchanged. +func TestSaveGeoFilterWriteFailureCleansUpOwnTemp(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + stale := path + ".tmp" + if err := os.WriteFile(stale, []byte("stale\n"), 0o600); err != nil { + t.Fatal(err) + } + + orig := configCreateTemp + t.Cleanup(func() { configCreateTemp = orig }) + configCreateTemp = func(d, pattern string) (*os.File, error) { + f, err := orig(d, pattern) + if err != nil { + return nil, err + } + // A closed descriptor makes the first Write fail with ErrClosed while + // the temp file itself stays on disk, so cleanup is observable. + _ = f.Close() + return f, nil + } + + err := SaveGeoFilter(dir, geoPerm340Filter()) + if err == nil { + t.Fatal("expected a write error, got nil") + } + data, rerr := os.ReadFile(path) + if rerr != nil { + t.Fatalf("read config: %v", rerr) + } + if string(data) != geoPerm340Config { + t.Error("config was modified despite the failed save") + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after failed save = %04o, want 0600", got) + } + if got, rerr := os.ReadFile(stale); rerr != nil || string(got) != "stale\n" { + t.Errorf("pre-existing temp file was touched: %q err=%v", string(got), rerr) + } + assertNoStrayTemp(t, dir, "config.json.tmp") +} + +// TestSaveGeoFilterRenameFailureCleansUpOwnTemp: a failing rename must not +// leave the half-written replacement behind. +func TestSaveGeoFilterRenameFailureCleansUpOwnTemp(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + stale := path + ".tmp" + if err := os.WriteFile(stale, []byte("stale\n"), 0o600); err != nil { + t.Fatal(err) + } + + orig := configRename + t.Cleanup(func() { configRename = orig }) + sentinel := errors.New("rename refused by test") + configRename = func(string, string) error { return sentinel } + + err := SaveGeoFilter(dir, geoPerm340Filter()) + if !errors.Is(err, sentinel) { + t.Fatalf("SaveGeoFilter error = %v, want it to wrap %v", err, sentinel) + } + data, rerr := os.ReadFile(path) + if rerr != nil { + t.Fatalf("read config: %v", rerr) + } + if string(data) != geoPerm340Config { + t.Error("config was modified despite the failed rename") + } + if got, rerr := os.ReadFile(stale); rerr != nil || string(got) != "stale\n" { + t.Errorf("pre-existing temp file was touched: %q err=%v", string(got), rerr) + } + assertNoStrayTemp(t, dir, "config.json.tmp") +} + +// geoPerm340Decode decodes JSON into a shape comparable with +// reflect.DeepEqual, so assertions are about values and not about indentation. +func geoPerm340Decode(t *testing.T, b []byte) map[string]any { + t.Helper() + var m map[string]any + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("not valid JSON: %v\n%s", err, b) + } + return m +} + +// TestSaveGeoFilterPreservesUnrelatedFields: only geo_filter may change, and +// the permission-preserving rewrite must not cost us any of the other keys — +// including ones the Config struct does not model, such as _comment. +func TestSaveGeoFilterPreservesUnrelatedFields(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after save = %04o, want 0600", got) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + got := geoPerm340Decode(t, raw) + if _, ok := got["geo_filter"]; !ok { + t.Error("geo_filter missing after save") + } + want := geoPerm340Decode(t, []byte(geoPerm340Config)) + for key, wantVal := range want { + gotVal, ok := got[key] + if !ok { + t.Errorf("key %q missing after save", key) + continue + } + if !reflect.DeepEqual(gotVal, wantVal) { + t.Errorf("key %q changed: got %#v, want %#v", key, gotVal, wantVal) + } + } + + // A removal save must keep the same keys minus geo_filter. + if err := SaveGeoFilter(dir, nil); err != nil { + t.Fatalf("SaveGeoFilter(nil): %v", err) + } + raw, err = os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + after := geoPerm340Decode(t, raw) + if _, ok := after["geo_filter"]; ok { + t.Error("geo_filter still present after removal save") + } + delete(got, "geo_filter") + if !reflect.DeepEqual(after, got) { + t.Errorf("removal save changed unrelated fields:\ngot %#v\nwant %#v", after, got) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after removal save = %04o, want 0600", got) + } +} + +// TestSaveGeoFilterStatFailureIsFatal pins the fail-safe half of the mode +// preservation: if the config's metadata cannot be read, the save must report +// the failure rather than fall back to a permissive default mode. +func TestSaveGeoFilterStatFailureIsFatal(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + + orig := configStatFile + t.Cleanup(func() { configStatFile = orig }) + sentinel := errors.New("stat refused by test") + first := true + configStatFile = func(f *os.File) (os.FileInfo, error) { + if first { + first = false + return nil, sentinel + } + return orig(f) + } + + err := SaveGeoFilter(dir, geoPerm340Filter()) + if !errors.Is(err, sentinel) { + t.Fatalf("SaveGeoFilter error = %v, want it to wrap %v", err, sentinel) + } + data, rerr := os.ReadFile(path) + if rerr != nil { + t.Fatalf("read config: %v", rerr) + } + if string(data) != geoPerm340Config { + t.Error("config was modified despite unreadable metadata") + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode after failed save = %04o, want 0600", got) + } + assertNoStrayTemp(t, dir) +} + +// TestSaveGeoFilterConfigLocations covers both supported lookup locations and +// their precedence, asserting mode preservation in each. +func TestSaveGeoFilterConfigLocations(t *testing.T) { + t.Run("top level", func(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode = %04o, want 0600", got) + } + assertNoStrayTemp(t, dir) + }) + + t.Run("data subdirectory", func(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "data", "config.json"), 0o600) + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != 0o600 { + t.Errorf("mode = %04o, want 0600", got) + } + body, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(body), "geo_filter") { + t.Error("geo_filter not written to data/config.json") + } + // The temp file must land next to the config it replaces, not in the + // parent directory, or the rename would cross filesystems on a mount. + assertNoStrayTemp(t, filepath.Join(dir, "data")) + assertNoStrayTemp(t, dir, "data") + }) + + t.Run("top level wins over data subdirectory", func(t *testing.T) { + dir := t.TempDir() + top := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + nested := writeGeoPerm340Config(t, filepath.Join(dir, "data", "config.json"), 0o644) + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, top); got != 0o600 { + t.Errorf("top-level mode = %04o, want 0600", got) + } + body, err := os.ReadFile(nested) + if err != nil { + t.Fatal(err) + } + if string(body) != geoPerm340Config { + t.Error("data/config.json was written when the top-level config exists") + } + }) +} diff --git a/cmd/server/config_geofilter_perms_340_unix_test.go b/cmd/server/config_geofilter_perms_340_unix_test.go new file mode 100644 index 000000000..3f8e61f2d --- /dev/null +++ b/cmd/server/config_geofilter_perms_340_unix_test.go @@ -0,0 +1,80 @@ +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +// TestSaveGeoFilterModeIsUmaskIndependent: the preserved mode must be applied +// explicitly, not left to the creation mode masked by the process umask. With +// umask 0077 a deliberately world-readable 0644 config would otherwise come +// back as 0600 and break anything reading it as another user; with umask 0022 +// a 0600 config would come back as 0644 (#340). +// +// cmd/server tests never call t.Parallel, so flipping the process-wide umask +// for the duration of one save is safe here. +func TestSaveGeoFilterModeIsUmaskIndependent(t *testing.T) { + for _, tc := range []struct { + name string + umask int + mode os.FileMode + }{ + {"restrictive umask keeps 0644", 0o077, 0o644}, + {"permissive umask keeps 0600", 0o000, 0o600}, + {"default umask keeps 0600", 0o022, 0o600}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), tc.mode) + + prev := syscall.Umask(tc.umask) + err := SaveGeoFilter(dir, geoPerm340Filter()) + syscall.Umask(prev) + if err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + if got := geoPerm340Mode(t, path); got != tc.mode { + t.Errorf("mode after save under umask %04o = %04o, want %04o", tc.umask, got, tc.mode) + } + }) + } +} + +// TestSaveGeoFilterPreservesOwner documents and pins the owner/group behavior +// relevant to the non-root container migration (#339): the replacement keeps +// the original uid/gid whenever the saving process already owns the file, which +// is the only arrangement a non-root server can write in anyway. An +// unprivileged process cannot chown to a foreign uid, so the save must not try +// to and must not fail over it. +func TestSaveGeoFilterPreservesOwner(t *testing.T) { + dir := t.TempDir() + path := writeGeoPerm340Config(t, filepath.Join(dir, "config.json"), 0o600) + + before, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + wantUID, wantGID, ok := configFileOwnerIDs(before) + if !ok { + t.Skip("owner IDs unavailable on this platform") + } + + if err := SaveGeoFilter(dir, geoPerm340Filter()); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + after, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + gotUID, gotGID, ok := configFileOwnerIDs(after) + if !ok { + t.Fatal("owner IDs unavailable after save") + } + if gotUID != wantUID || gotGID != wantGID { + t.Errorf("owner after save = %d:%d, want %d:%d", gotUID, gotGID, wantUID, wantGID) + } +} diff --git a/cmd/server/config_owner_unix.go b/cmd/server/config_owner_unix.go new file mode 100644 index 000000000..47ce0a499 --- /dev/null +++ b/cmd/server/config_owner_unix.go @@ -0,0 +1,19 @@ +//go:build !windows + +package main + +import ( + "os" + "syscall" +) + +// configFileOwnerIDs returns the user and group IDs that own fi. Used by the +// atomic config replacement (#340) to decide whether the replacement file +// already has the right ownership. +func configFileOwnerIDs(fi os.FileInfo) (uid, gid int, ok bool) { + st, k := fi.Sys().(*syscall.Stat_t) + if !k { + return 0, 0, false + } + return int(st.Uid), int(st.Gid), true +} diff --git a/cmd/server/config_owner_windows.go b/cmd/server/config_owner_windows.go new file mode 100644 index 000000000..2f02a7cc4 --- /dev/null +++ b/cmd/server/config_owner_windows.go @@ -0,0 +1,11 @@ +//go:build windows + +package main + +import "os" + +// configFileOwnerIDs reports no owner on Windows, which has no Unix user or +// group IDs; the server is only deployed on Linux. The atomic config +// replacement then skips its ownership step and relies on the preserved +// permission bits alone. +func configFileOwnerIDs(os.FileInfo) (uid, gid int, ok bool) { return 0, 0, false } From 9906c04e6c0a4865951a98c0417e6f056cddc4d4 Mon Sep 17 00:00:00 2001 From: dborup Date: Wed, 7 Oct 2026 20:08:50 +0200 Subject: [PATCH 2/2] test(config): assert the injected pre-rename failure, not just any error The create-temp seam hands back a closed descriptor, so the first operation on it fails with os.ErrClosed. Asserting that specific error keeps the test from passing on some unrelated failure, and the comment now says "before the rename" rather than "mid-write": the closed fd trips the chmod first. Relates to #340. --- cmd/server/config_geofilter_perms_340_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/server/config_geofilter_perms_340_test.go b/cmd/server/config_geofilter_perms_340_test.go index ff900bd77..b7f9b6cfd 100644 --- a/cmd/server/config_geofilter_perms_340_test.go +++ b/cmd/server/config_geofilter_perms_340_test.go @@ -255,8 +255,9 @@ func TestSaveGeoFilterCreateTempFailure(t *testing.T) { } } -// TestSaveGeoFilterWriteFailureCleansUpOwnTemp exercises a mid-write failure -// through the create-temp seam: the temp file must be removed, a pre-existing +// TestSaveGeoFilterWriteFailureCleansUpOwnTemp exercises a failure after the +// temp file exists on disk but before the rename, injected through the +// create-temp seam: the temp file must be removed, a pre-existing // config.json.tmp must survive, and the config must be unchanged. func TestSaveGeoFilterWriteFailureCleansUpOwnTemp(t *testing.T) { dir := t.TempDir() @@ -273,15 +274,16 @@ func TestSaveGeoFilterWriteFailureCleansUpOwnTemp(t *testing.T) { if err != nil { return nil, err } - // A closed descriptor makes the first Write fail with ErrClosed while - // the temp file itself stays on disk, so cleanup is observable. + // A closed descriptor makes the first operation on it fail with + // ErrClosed while the temp file itself stays on disk, so cleanup is + // observable. _ = f.Close() return f, nil } err := SaveGeoFilter(dir, geoPerm340Filter()) - if err == nil { - t.Fatal("expected a write error, got nil") + if !errors.Is(err, os.ErrClosed) { + t.Fatalf("SaveGeoFilter error = %v, want it to wrap %v", err, os.ErrClosed) } data, rerr := os.ReadFile(path) if rerr != nil {