diff --git a/TODO.md b/TODO.md index 302faf8..161092b 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,16 @@ 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 with a + second name (hard link) keeps its owner, and the start logs its path. Owners + are now set on the opened directory or file, never by name, so a hard link + swapped in after the check is caught too; symbolic links and other files that + are neither directories nor regular files keep their owner, as setting it by + name could reach a hard link swapped in at that name. The test that runs as + root hard-links a file from outside the volume into it and finds its owner + unchanged - 2026-10-14: stopping the server waits for a report file write already under way, the periodic flush's or the one `Append` starts when 10 MiB are waiting, before the final flush diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index 8bf4ec4..fcc9595 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -66,10 +66,11 @@ 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 and regular files 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 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/internal/reportbuf/datadir.go b/internal/reportbuf/datadir.go index c2fb9f6..6609154 100644 --- a/internal/reportbuf/datadir.go +++ b/internal/reportbuf/datadir.go @@ -3,6 +3,7 @@ package reportbuf import ( "errors" "io/fs" + "log/slog" "os" "os/user" "path/filepath" @@ -18,9 +19,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 and regular files 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 with a second name (hard link), as +// that name could be outside volume, and any file that is neither a +// directory nor a regular file, such as a symbolic link (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 +37,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 +70,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 +83,32 @@ 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 +// and regular files 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. 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 or a regular file +// is followed only as the os.Root allows. It leaves any file that is +// neither a directory nor a regular file, such as a symbolic link: +// opening one would follow the link or open the device, Root.Lchown +// would not hold, and 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(): + err = chownFile(root, entryName, uid, gid) } if err != nil { @@ -121,6 +119,66 @@ 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 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, 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.Open(name) + 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 { + return file.Chown(uid, gid) + } + + 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..cb454eb 100644 --- a/internal/reportbuf/datadir_test.go +++ b/internal/reportbuf/datadir_test.go @@ -254,10 +254,11 @@ 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 and +// regular files 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, and a hard link to a file outside it, are left as they +// were, and so is what they lead to. func TestPrepareDataDirGivesVolumeToOwner(t *testing.T) { t.Parallel() @@ -289,6 +290,11 @@ 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) + } + err = reportbuf.PrepareDataDir(volume, filepath.Join(volume, reports), &user.User{Uid: "4242", Gid: "4343"}) if err != nil { @@ -296,12 +302,12 @@ 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"), } { requireOwner(t, path, 4242, 4343) } + requireOwner(t, filepath.Join(old, "link"), 0, 0) requireOwner(t, outside, 0, 0) requireOwner(t, filepath.Join(outside, "f"), 0, 0) }