diff --git a/TODO.md b/TODO.md index 83deb42..d2a9f95 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,21 @@ 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 file 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. Devices, named + pipes and sockets keep their owner. Setting the owner on an open file that is + not a directory is built only for Linux: `prepare-data-dir` runs only in the + Linux image, and macOS, where the backend is also built for development, has + neither `O_PATH` nor `AT_EMPTY_PATH`. On any other system the package still + builds, and `prepare-data-dir` fails at the first file that is not a + directory. The test that runs as root hard-links a file, a symbolic link and a + named pipe from outside the volume into it and finds their owners unchanged - 2026-10-18: from a trusted proxy, the backend takes the client address from the right-most `X-Forwarded-For` entry that is not in `TRUSTED_PROXIES` ([#144](https://git.eeqj.de/sneak/netwatch/issues/144)). It took the left-most diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index 8bf4ec4..db7c3ff 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -66,10 +66,13 @@ 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 -# 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. +# and gives /data and the directories, regular files and symbolic links +# in it to the netwatch user, so the host directory needs no preparing. +# A file with a second name (hard link) keeps its owner, as that name +# could be outside /data, and its path is logged. 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 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..1516e9f 100644 --- a/internal/reportbuf/datadir.go +++ b/internal/reportbuf/datadir.go @@ -18,9 +18,15 @@ 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 a file with a second name (hard link) as it +// is, as that name could be outside volume, and logs its path; it +// leaves a device, named pipe or socket as it is too (see chownFile). +// dir must be volume or a path below it, with no '.', '..', empty part +// or '/' at the end. On any system but Linux, the system of the image, it +// fails at the first file in volume that is not a directory (see +// chownFile). // // 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 +38,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 +71,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,28 +84,23 @@ 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, but not the +// files chownFile leaves. 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 any other file is not +// followed at all (see chownFile). +func chownAll(root *os.Root, name string, uid, gid int) error { + entries, err := chownDir(root, name, uid, gid) if err != nil { return err } @@ -108,9 +108,9 @@ func lchownAll(root *os.Root, name string, uid, gid int) error { for _, entry := range entries { entryName := filepath.Join(name, entry.Name()) if entry.IsDir() { - err = lchownAll(root, entryName, uid, gid) + err = chownAll(root, entryName, uid, gid) } else { - err = root.Lchown(entryName, uid, gid) + err = chownFile(root, entryName, uid, gid) } if err != nil { @@ -121,6 +121,36 @@ 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) +} + // 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_linux.go b/internal/reportbuf/datadir_linux.go new file mode 100644 index 0000000..7d9f926 --- /dev/null +++ b/internal/reportbuf/datadir_linux.go @@ -0,0 +1,65 @@ +//go:build linux + +package reportbuf + +import ( + "io/fs" + "log/slog" + "os" + "syscall" + + "golang.org/x/sys/unix" +) + +// chownFile gives name, a file inside root that is not a directory, to +// uid and gid if it is a regular file or symbolic link with one name. +// It leaves, and logs, a file of any kind with a second name (hard +// link): all the names of a file share its owner, and the other could +// be outside root. It leaves any other file with one name, such as a +// device, named pipe or socket: the server needs to own none of them. +// It opens name through root with O_PATH, which reads nothing from the +// file, so opening a device or named pipe neither waits nor acts on +// it; os.Root adds O_NOFOLLOW, and the two together open a symbolic +// link itself, never what it points to. It reads the kind of file and +// its number of names from the open file and gives that file to them, +// so a hard link swapped in at name is caught too. +// +// This file is built only on Linux: prepare-data-dir runs only in the +// Linux image, and macOS, where the backend is also built for +// development, has neither O_PATH nor AT_EMPTY_PATH. On any other +// system chownFile returns an error (see datadir_other.go). +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 + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok || stat.Nlink > 1 { + slog.Warn("owner not changed: file has a second name (hard link)", + "path", file.Name()) + + return nil + } + + if !info.Mode().IsRegular() && info.Mode().Type() != fs.ModeSymlink { + return nil + } + + // 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. + err = unix.Fchownat(int(file.Fd()), "", uid, gid, unix.AT_EMPTY_PATH) + if err != nil { + return &fs.PathError{Op: "chown", Path: file.Name(), Err: err} + } + + return nil +} diff --git a/internal/reportbuf/datadir_other.go b/internal/reportbuf/datadir_other.go new file mode 100644 index 0000000..d563839 --- /dev/null +++ b/internal/reportbuf/datadir_other.go @@ -0,0 +1,21 @@ +//go:build !linux + +package reportbuf + +import ( + "errors" + "io/fs" + "os" +) + +// chownFile returns an error on every system but Linux. Opening a file +// that is not a directory without reading it or following it, and +// giving it to a user on the open file, never by name, is built only +// for Linux (see datadir_linux.go): prepare-data-dir runs only in the +// Linux image, and macOS, where the backend is also built for +// development, has neither O_PATH nor AT_EMPTY_PATH. This file lets +// the package build elsewhere, such as on macOS, where a developer +// runs make build and make run. +func chownFile(_ *os.Root, name string, _, _ int) error { + return &fs.PathError{Op: "chown", Path: name, Err: errors.ErrUnsupported} +} diff --git a/internal/reportbuf/datadir_test.go b/internal/reportbuf/datadir_test.go index 86b2048..3618dda 100644 --- a/internal/reportbuf/datadir_test.go +++ b/internal/reportbuf/datadir_test.go @@ -254,10 +254,13 @@ 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, one to a +// symbolic link outside it and one to a named pipe outside it are left +// as they were, and so is a named pipe in the volume. func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Parallel() @@ -274,6 +277,16 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Fatal(err) } + err = os.Symlink("f", filepath.Join(outside, "link")) + if err != nil { + t.Fatal(err) + } + + err = syscall.Mkfifo(filepath.Join(outside, "fifo"), 0o600) + if err != nil { + t.Fatal(err) + } + err = os.Mkdir(old, 0o700) if err != nil { t.Fatal(err) @@ -289,6 +302,21 @@ func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Fatal(err) } + err = syscall.Mkfifo(filepath.Join(old, "fifo"), 0o600) + if err != nil { + t.Fatal(err) + } + + // os.Link does not follow a symbolic link: it gives the link itself + // a second name. + for _, name := range []string{"f", "link", "fifo"} { + err = os.Link(filepath.Join(outside, name), + filepath.Join(old, "hardlink-to-"+name)) + 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 +324,16 @@ 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"), + filepath.Join(outside, "fifo"), filepath.Join(old, "fifo"), + } { + 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 (