diff --git a/TODO.md b/TODO.md index 0cfec79..6bf0cb8 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,17 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-18: `netwatch-server prepare-data-dir` no longer changes the owner of + a file outside `/data` through a hard link in the volume + ([#143](https://git.eeqj.de/sneak/netwatch/issues/143)). A regular file or + symbolic link with a second name (hard link) keeps its owner, and the start + logs its path. Owners are now set on the opened directory, file or link, never + by name, so a hard link swapped in after the check is caught too; a symbolic + link is opened without following it and given to the netwatch user itself, so + the server can still follow one in a sticky directory that anyone can write + to. Other files that are neither directories, regular files nor symbolic links + keep their owner. The test that runs as root hard-links a file from outside + the volume into it and finds its owner unchanged - 2026-10-18: after an interval change, the summary's Reachable slot reads `--/--` in white, as at page load, until the second round since the change ends ([#140](https://git.eeqj.de/sneak/netwatch/issues/140)). It used to read diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index 8bf4ec4..86a7c77 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -66,10 +66,12 @@ done > /etc/nginx/trusted-proxies.conf # netwatch-server keeps its report files in DATA_DIR, on the /data # volume, which may be a host directory owned by root or by another # uid. Here, as root, netwatch-server prepare-data-dir creates DATA_DIR -# and gives /data and everything in it to the netwatch user, so the -# host directory needs no preparing. It stops the start, naming +# and gives /data and the directories, regular files and symbolic links +# in it to the netwatch user, so the host directory needs no preparing, +# except a file with a second name (hard link), which keeps its owner, +# as that name could be outside /data. It stops the start, naming # DATA_DIR, unless DATA_DIR is /data or a path below it, and it acts on -# nothing outside /data, whatever symbolic links it meets there. +# nothing outside /data, whatever symbolic or hard links it meets there. export DATA_DIR="${DATA_DIR:-/data/reports}" netwatch-server prepare-data-dir "$DATA_DIR" || exit 1 diff --git a/go.mod b/go.mod index b158b83..20cca2a 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/slok/go-http-metrics v0.13.0 github.com/spf13/viper v1.21.0 go.uber.org/fx v1.24.0 + golang.org/x/sys v0.47.0 ) require ( @@ -41,7 +42,6 @@ require ( go.uber.org/multierr v1.10.0 // indirect go.uber.org/zap v1.26.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect google.golang.org/protobuf v1.36.11 // indirect ) diff --git a/internal/reportbuf/datadir.go b/internal/reportbuf/datadir.go index c2fb9f6..5950c88 100644 --- a/internal/reportbuf/datadir.go +++ b/internal/reportbuf/datadir.go @@ -3,11 +3,14 @@ package reportbuf import ( "errors" "io/fs" + "log/slog" "os" "os/user" "path/filepath" "strconv" "syscall" + + "golang.org/x/sys/unix" ) // ErrDataDirOutsideVolume is returned by PrepareDataDir for a DATA_DIR @@ -18,9 +21,13 @@ var ErrDataDirOutsideVolume = errors.New( // PrepareDataDir gets dir, the server's DATA_DIR, ready for owner, the // user the server runs as, so that a host directory mounted at volume, // /data in the image, needs no preparing: it creates dir, gives volume -// and everything in it to owner, and gives volume and dir the mode the -// server gives a directory it creates. dir must be volume or a path -// below it, with no '.', '..', empty part or '/' at the end. +// and the directories, regular files and symbolic links in it to +// owner, and gives volume and dir the mode the server gives a directory +// it creates. It leaves as they are a regular file or symbolic link +// with a second name (hard link), as that name could be outside volume, +// and any other file, such as a device (see chownAll). dir must be +// volume or a path below it, with no '.', '..', empty part or '/' at +// the end. // // bin/entrypoint.sh runs this as root, which would follow a symbolic // link anywhere, so every step goes through an os.Root opened on @@ -32,10 +39,9 @@ var ErrDataDirOutsideVolume = errors.New( // open, never following a link at the name it creates, and follows a // link on the path only as the os.Root allows, so a relative link // inside volume can lead it to create directories elsewhere inside -// volume. Lchown never changes what a link points to, and the modes -// are set on directories already opened (see chmodDir), so the most -// that process can do is make a step fail or wait, or act on -// something else inside volume. +// volume. The owners and modes are set on files already opened (see +// chownAll and chmodDir), so the most that process can do is make a +// step fail or wait, or act on something else inside volume. func PrepareDataDir(volume, dir string, owner *user.User) error { // rel is dir as a path from volume; IsLocal is false for one that // leads out of it. @@ -66,7 +72,7 @@ func PrepareDataDir(volume, dir string, owner *user.User) error { return err } - err = lchownAll(root, ".", uid, gid) + err = chownAll(root, ".", uid, gid) if err != nil { return err } @@ -79,38 +85,36 @@ func PrepareDataDir(volume, dir string, owner *user.User) error { return chmodDir(root, rel) } -// lchownAll gives name, a directory inside root, and everything in it -// to uid and gid. It reads each directory opened through root, not -// through root.FS(), which refuses a name that is not valid UTF-8, and -// calls Lchown on every entry, which gives a symbolic link itself to -// them, not what it points to. It goes into an entry only when the -// read found a directory there, so it follows no link it finds; one -// swapped in for that directory afterwards is followed only as the -// os.Root allows. -func lchownAll(root *os.Root, name string, uid, gid int) error { - err := root.Lchown(name, uid, gid) - if err != nil { - return err - } - - dir, err := root.Open(name) - if err != nil { - return err - } - - entries, err := dir.ReadDir(-1) - _ = dir.Close() - +// chownAll gives name, a directory inside root, and the directories, +// regular files and symbolic links in it to uid and gid. It opens each +// through root and gives the open file to them, as chmodDir sets a +// mode on the open directory. Root.Lchown would not hold: it acts on +// whatever is at the name by then, and a hard link swapped in there +// shares its owner with a file that could be outside root. A symbolic +// link is given to them itself, never what it points to, because in a +// directory that anyone can write to and that has the sticky bit set, +// as /tmp has, the kernel follows a link only for the link's owner or +// when the link and the directory have the same owner. chownAll goes +// into an entry only when the read found a directory there, so it +// follows no link it finds; one swapped in afterwards for a directory +// is followed only as the os.Root allows, and one swapped in for a +// regular file or symbolic link is not followed at all (see +// chownFile). It leaves any other file, such as a device: the server +// needs to own none of them. +func chownAll(root *os.Root, name string, uid, gid int) error { + entries, err := chownDir(root, name, uid, gid) if err != nil { return err } for _, entry := range entries { entryName := filepath.Join(name, entry.Name()) - if entry.IsDir() { - err = lchownAll(root, entryName, uid, gid) - } else { - err = root.Lchown(entryName, uid, gid) + + switch { + case entry.IsDir(): + err = chownAll(root, entryName, uid, gid) + case entry.Type().IsRegular(), entry.Type() == fs.ModeSymlink: + err = chownFile(root, entryName, uid, gid) } if err != nil { @@ -121,6 +125,71 @@ func lchownAll(root *os.Root, name string, uid, gid int) error { return nil } +// chownDir gives name, a directory inside root, to uid and gid, and +// returns what it holds. Like chmodDir, it opens name through root and +// refuses anything but a directory, which has no second name (hard +// link). It reads the open directory, not root.FS(), which refuses a +// name that is not valid UTF-8. +func chownDir(root *os.Root, name string, uid, gid int) ([]fs.DirEntry, error) { + dir, err := root.Open(name) + if err != nil { + return nil, err + } + + defer func() { _ = dir.Close() }() + + info, err := dir.Stat() + if err != nil { + return nil, err + } + + if !info.IsDir() { + return nil, &fs.PathError{Op: "chown", Path: name, Err: syscall.ENOTDIR} + } + + err = dir.Chown(uid, gid) + if err != nil { + return nil, err + } + + return dir.ReadDir(-1) +} + +// chownFile gives name, a regular file or symbolic link inside root, to +// uid and gid, unless it has a second name (hard link): all the names +// of a file share its owner, and the other could be outside root. It +// opens name through root with O_PATH, which reads nothing from the +// file; os.Root adds O_NOFOLLOW, and the two together open a symbolic +// link itself, never what it points to. It reads the number of names +// from the open file and gives that file to them, so a hard link +// swapped in at name is caught too. It logs a file it leaves. +func chownFile(root *os.Root, name string, uid, gid int) error { + file, err := root.OpenFile(name, unix.O_PATH, 0) + if err != nil { + return err + } + + defer func() { _ = file.Close() }() + + info, err := file.Stat() + if err != nil { + return err + } + + // File.Chown would fail: fchown refuses a file opened with O_PATH. + // Fchownat with an empty name and AT_EMPTY_PATH acts on the open + // file instead. + stat, ok := info.Sys().(*syscall.Stat_t) + if ok && stat.Nlink == 1 { + return unix.Fchownat(int(file.Fd()), "", uid, gid, unix.AT_EMPTY_PATH) + } + + slog.Warn("owner not changed: file has a second name (hard link)", + "path", file.Name()) + + return nil +} + // chmodDir gives name, a directory inside root, the mode the server // gives a directory it creates. Root.Chmod would not hold: on Linux it // checks that name is not a symbolic link, then sets the mode by name, diff --git a/internal/reportbuf/datadir_test.go b/internal/reportbuf/datadir_test.go index 86b2048..e57ea98 100644 --- a/internal/reportbuf/datadir_test.go +++ b/internal/reportbuf/datadir_test.go @@ -254,10 +254,12 @@ func TestPrepareDataDirTakesDirectoryNamedInLatin1(t *testing.T) { requireOwner(t, filepath.Join(latin1, "f"), uint32(uid), uint32(gid)) } -// TestPrepareDataDirGivesVolumeToOwner gives everything in the volume -// to a uid and gid that own nothing, which only root can do. A -// symbolic link in the volume to a directory outside it is given to -// them itself; what it points to is left as it was. +// TestPrepareDataDirGivesVolumeToOwner gives the directories, regular +// files and symbolic links in the volume to a uid and gid that own +// nothing, which only root can do. A symbolic link in the volume to a +// directory outside it is given to them itself; what it points to is +// left as it was. A hard link to a file outside the volume, and one to +// a symbolic link outside it, are left as they were. func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Parallel() @@ -289,6 +291,24 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Fatal(err) } + err = os.Link(filepath.Join(outside, "f"), filepath.Join(old, "hardlink")) + if err != nil { + t.Fatal(err) + } + + // os.Link does not follow a symbolic link: it gives the link itself + // a second name. + err = os.Symlink("f", filepath.Join(outside, "link")) + if err != nil { + t.Fatal(err) + } + + err = os.Link(filepath.Join(outside, "link"), + filepath.Join(old, "hardlink-to-link")) + if err != nil { + t.Fatal(err) + } + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), &user.User{Uid: "4242", Gid: "4343"}) if err != nil { @@ -296,12 +316,15 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { } for _, path := range []string{ - volume, filepath.Join(volume, reports), old, - filepath.Join(old, "f"), filepath.Join(old, "link"), + volume, filepath.Join(volume, reports), old, filepath.Join(old, "f"), + filepath.Join(old, "link"), } { requireOwner(t, path, 4242, 4343) } - requireOwner(t, outside, 0, 0) - requireOwner(t, filepath.Join(outside, "f"), 0, 0) + for _, path := range []string{ + outside, filepath.Join(outside, "f"), filepath.Join(outside, "link"), + } { + requireOwner(t, path, 0, 0) + } } diff --git a/internal/reportbuf/reportbuf.go b/internal/reportbuf/reportbuf.go index 924a7a2..f4e925b 100644 --- a/internal/reportbuf/reportbuf.go +++ b/internal/reportbuf/reportbuf.go @@ -1,6 +1,9 @@ // Package reportbuf accumulates telemetry reports in memory // and periodically flushes them to zstd-compressed JSONL files, -// deleting the oldest files to keep them under a size cap. +// deleting the oldest files to keep them under a size cap. Before the +// server starts, PrepareDataDir gets the directory those files are +// kept in, DATA_DIR, ready for the user the server runs as: it creates +// it and gives it and the volume it is on to that user. package reportbuf import (