diff --git a/cmd/server/config.go b/cmd/server/config.go index 5c4de116..4fa6f543 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -1052,6 +1052,26 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { return fmt.Errorf("config.json not found in %s", configDir) } + // Follow a symlink at configPath to the file it points at before the + // tmp+rename below. In the Docker image /app/config.json is a symlink to + // the bind-mounted /app/data/config.json (docker/entrypoint-go.sh), and + // os.Rename onto the link path would replace the link with a regular + // file inside the container: the mounted file would keep the old + // geo_filter, the saved one would be invisible on the host and would go + // away with the container. Resolving first keeps the write atomic and + // the result where the operator (and the next container) will read it. + // + // Unlike the stats file (writeStatsAtomic, refs #1170), replacing a + // symlink at the destination is not wanted here: this path is not + // world-writable, and the link is one the image itself creates. Only a + // regular file is followed, so a link to a device or a fifo is left + // alone and handled as before. + if resolved, err := filepath.EvalSymlinks(configPath); err == nil && resolved != configPath { + if fi, err := os.Lstat(resolved); err == nil && fi.Mode().IsRegular() { + configPath = resolved + } + } + data, err := os.ReadFile(configPath) if err != nil { return fmt.Errorf("read config: %w", err) @@ -1083,13 +1103,22 @@ func SaveGeoFilter(configDir string, gf *GeoFilterConfig) error { // config.json holds the API key and broker passwords, and an operator // who made it 0600 should not find it 0644 after a geo-filter save. mode := os.FileMode(0644) - if fi, err := os.Stat(configPath); err == nil { - mode = fi.Mode().Perm() + orig, origErr := os.Stat(configPath) + if origErr == nil { + mode = orig.Mode().Perm() } tmp := configPath + ".tmp" if err := os.WriteFile(tmp, out, mode); err != nil { return fmt.Errorf("write config: %w", err) } + // Keep the owner as well. The image runs the server as root by default, + // and without this the rename would hand the operator's bind-mounted + // config.json to root:root, which at 0600 they could no longer edit + // without sudo. Best effort: a server that may not chown leaves the file + // as it was created. + if origErr == nil { + copyOwner(tmp, orig) + } if err := os.Rename(tmp, configPath); err != nil { os.Remove(tmp) return fmt.Errorf("rename config: %w", err) diff --git a/cmd/server/config_owner_unix.go b/cmd/server/config_owner_unix.go new file mode 100644 index 00000000..46eba93f --- /dev/null +++ b/cmd/server/config_owner_unix.go @@ -0,0 +1,17 @@ +//go:build !windows + +package main + +import ( + "os" + "syscall" +) + +// copyOwner gives path the owner and group recorded in fi, best effort: a +// process that may not chown (not root, or not in the group) leaves path as +// it was created. On Windows there is no uid/gid; see config_owner_windows.go. +func copyOwner(path string, fi os.FileInfo) { + if st, ok := fi.Sys().(*syscall.Stat_t); ok { + _ = os.Chown(path, int(st.Uid), int(st.Gid)) + } +} diff --git a/cmd/server/config_owner_unix_test.go b/cmd/server/config_owner_unix_test.go new file mode 100644 index 00000000..4d610a2e --- /dev/null +++ b/cmd/server/config_owner_unix_test.go @@ -0,0 +1,99 @@ +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +// TestSaveGeoFilterKeepsOwner checks that a save keeps the owner and group of +// config.json. The image runs the server as root, and a bind-mounted config +// that came back as root:root 0600 would lock the operator out of their own +// file. +func TestSaveGeoFilterKeepsOwner(t *testing.T) { + // Pick an owner the save would not produce by itself. Root can use any + // uid and gid; anyone else can only switch to a supplementary group. + uid, gid := -1, -1 + if os.Geteuid() == 0 { + uid, gid = 4242, 4242 + } else if groups, err := os.Getgroups(); err == nil { + for _, g := range groups { + if g != os.Getegid() { + gid = g + break + } + } + } + if gid == -1 { + t.Skip("not root and no supplementary group to chown to") + } + + dir := t.TempDir() + cfg := filepath.Join(dir, "config.json") + if err := os.WriteFile(cfg, []byte(`{"port":3000}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chown(cfg, uid, gid); err != nil { + t.Fatalf("chown %s: %v", cfg, err) + } + + gf := &GeoFilterConfig{Polygon: [][2]float64{{51.0, 4.0}, {51.0, 5.0}, {50.5, 4.0}}} + if err := SaveGeoFilter(dir, gf); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + fi, err := os.Stat(cfg) + if err != nil { + t.Fatal(err) + } + st := fi.Sys().(*syscall.Stat_t) + if uid != -1 && int(st.Uid) != uid { + t.Errorf("owner of %s = %d, want %d", cfg, st.Uid, uid) + } + if int(st.Gid) != gid { + t.Errorf("group of %s = %d, want %d", cfg, st.Gid, gid) + } + if perm := fi.Mode().Perm(); perm != 0o600 { + t.Errorf("mode of %s = %#o, want 0600", cfg, perm) + } +} + +// TestSaveGeoFilterFifoSymlinkTarget pins the regular-file check on the +// symlink target: a config.json that links to a fifo is not followed, so the +// save never replaces the fifo with a regular file. A directory target would +// not test this, since reading the config already fails there. +func TestSaveGeoFilterFifoSymlinkTarget(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "feed") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Skipf("mkfifo unavailable: %v", err) + } + link := filepath.Join(dir, "config.json") + if err := os.Symlink(fifo, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + // SaveGeoFilter is the fifo's reader; feed it one config. + go func() { + f, err := os.OpenFile(fifo, os.O_WRONLY, 0) + if err != nil { + return + } + f.WriteString(`{"port":3000}`) + f.Close() + }() + if err := SaveGeoFilter(dir, nil); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + fi, err := os.Lstat(fifo) + if err != nil { + t.Fatalf("lstat %s: %v", fifo, err) + } + if fi.Mode()&os.ModeNamedPipe == 0 { + t.Errorf("%s was replaced (mode %s) — the link to a fifo was followed", fifo, fi.Mode()) + } +} diff --git a/cmd/server/config_owner_windows.go b/cmd/server/config_owner_windows.go new file mode 100644 index 00000000..ffcec731 --- /dev/null +++ b/cmd/server/config_owner_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package main + +import "os" + +// copyOwner is a no-op on Windows: there is no uid/gid to copy, and the new +// file takes the ACL of its directory. +func copyOwner(path string, fi os.FileInfo) {} diff --git a/cmd/server/config_symlink_test.go b/cmd/server/config_symlink_test.go new file mode 100644 index 00000000..240c3429 --- /dev/null +++ b/cmd/server/config_symlink_test.go @@ -0,0 +1,81 @@ +package main + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// TestSaveGeoFilterSymlinkedConfig pins the behavior the Docker image depends +// on: /app/config.json is a symlink to the bind-mounted /app/data/config.json, +// so a geo-filter save has to land in the mounted file and leave the symlink a +// symlink. A tmp+rename on the link path instead replaces the link with a +// regular file in the container layer, and the operator's config silently +// stops being the one in use. +func TestSaveGeoFilterSymlinkedConfig(t *testing.T) { + dir := t.TempDir() + dataDir := filepath.Join(dir, "data") + if err := os.Mkdir(dataDir, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dataDir, "config.json") + if err := os.WriteFile(target, []byte(`{"port":3000,"_comment":"keep me"}`), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "config.json") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + gf := &GeoFilterConfig{ + Polygon: [][2]float64{{51.0, 4.0}, {51.0, 5.0}, {50.5, 4.0}}, + BufferKm: 20, + } + if err := SaveGeoFilter(dir, gf); err != nil { + t.Fatalf("SaveGeoFilter: %v", err) + } + + // The symlink must survive, or the next container start reads a different + // file than the one that was written. + fi, err := os.Lstat(link) + if err != nil { + t.Fatalf("lstat %s: %v", link, err) + } + if fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("%s is no longer a symlink (mode %s) — the rename replaced it", link, fi.Mode()) + } + + // The geo filter must be in the mounted file, next to what was there. + data, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read %s: %v", target, err) + } + var raw map[string]interface{} + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("parse saved config: %v", err) + } + if _, ok := raw["geo_filter"]; !ok { + t.Errorf("geo_filter missing from %s: %s", target, data) + } + if raw["_comment"] != "keep me" { + t.Errorf("unrelated keys not preserved: %s", data) + } + if !bytes.Contains(data, []byte(`"bufferKm"`)) { + t.Errorf("bufferKm missing from saved config: %s", data) + } + + // The 0600 the operator chose is kept (#2126), and no tmp file is left in + // either directory. + if fi, err := os.Stat(target); err != nil { + t.Fatal(err) + } else if perm := fi.Mode().Perm(); perm != 0o600 { + t.Errorf("mode of %s = %#o, want 0600", target, perm) + } + for _, leftover := range []string{link + ".tmp", target + ".tmp"} { + if _, err := os.Lstat(leftover); err == nil { + t.Errorf("leftover temp file %s", leftover) + } + } +}