mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-10-15 03:39:53 +00:00
fix(config): write the geo filter through a symlinked config.json (#2137)
Found while reworking #2125, unrelated to that PR's change. In the Docker image the entrypoint symlinks `/app/config.json` to the bind-mounted `/app/data/config.json`. `SaveGeoFilter` takes the first path `os.Stat` resolves, so `configPath` stays the symlink, and the tmp+rename then **replaces the link with a regular file inside the container**: - the mounted `config.json` keeps the old `geo_filter`, so the filter the operator just saved appears nowhere on the host; - server and ingestor read the detached file from then on, so the UI keeps showing the new filter and nothing looks wrong; - the next container start reverts it: the entrypoint's `ln -sf` puts the link back, and re-creating the container drops the file anyway. **Fix:** resolve a symlink at `configPath` before the write, so the save lands in the file it points at and the link stays a link. Only a regular file is followed — a link to a device, fifo or directory is left alone and handled as before. This is deliberately the opposite of `writeStatsAtomic` (refs #1170), where replacing a destination symlink is the point because the default stats path can be world-writable `/tmp`. `config.json` is different: the link is one the image itself creates, and the directory is not world-writable. Happy to narrow it further (e.g. only follow a link that stays inside `configDir`) if you'd rather keep the two paths consistent. **Verification:** `TestSaveGeoFilterSymlinkedConfig` fails on master — ``` --- FAIL: TestSaveGeoFilterSymlinkedConfig config_symlink_test.go:47: .../config.json is no longer a symlink (mode -rw-------) — the rename replaced it config_symlink_test.go:60: geo_filter missing from .../data/config.json: {"port":3000,"_comment":"keep me"} ``` — and passes here. `TestSaveGeoFilterKeepsOwner` covers keeping the file's owner, and `TestSaveGeoFilterFifoSymlinkTarget` checks that only regular files are followed. `go test ./...` in `cmd/server` passes. **Single-file mounts:** when `config.json` is mounted as a single file, the save now fails with 500 (`EBUSY`) instead of writing a copy that the next start discards. Our own instance has no geo filter configured, which is why this went unnoticed there; its `/app/config.json` is still a symlink. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
e53c055d6e
commit
478f74a452
+31
-2
@@ -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)
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
@@ -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())
|
||||
}
|
||||
}
|
||||
@@ -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) {}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user