diff --git a/modules/zfs/README.md b/modules/zfs/README.md index d7975f4a..6eac079f 100644 --- a/modules/zfs/README.md +++ b/modules/zfs/README.md @@ -105,7 +105,10 @@ the last scheduled time not after now. If the slot is not yet served, the run ac It never pauses or stops a scrub. The unit has no ExecStop, so stopping the unit ends only the wait. A resilver that is running defers the scrub to the next run after it. The served slot is kept in -`/var/lib/zfs-tools/scrub-served.json`. Without that file, a scrub that status shows started in the slot +`/var/lib/zfs-scrub/scrub-served.json`, in a folder of root's alone (0700), apart from the account's state. +The run writes it only into a real folder of its own and never through a link: a new file opened with +`O_NOFOLLOW` and `O_EXCL`, then renamed over the old one. The tools run as the operator account and cannot +read it, so they judge a missed scrub from status. Without that file, a scrub that status shows started in the slot serves it. So a scrub starts within 15 minutes of its time, and after the machine was off, within 15 minutes of its coming back. diff --git a/modules/zfs/cmd/zfs-tools/manifest_test.go b/modules/zfs/cmd/zfs-tools/manifest_test.go index a008e039..09a52d6a 100644 --- a/modules/zfs/cmd/zfs-tools/manifest_test.go +++ b/modules/zfs/cmd/zfs-tools/manifest_test.go @@ -94,8 +94,12 @@ func TestTheHealthChecksAndWhoRunsWhat(t *testing.T) { t.Errorf("the follower runs as root, judged by its unit: %v", e) } if d := byID["state-dir"]; d["path"] != "/var/lib/zfs-tools" || d["owner"] != "${machine:account}" || - filepath.Dir(ErrorsFile) != d["path"] || filepath.Dir(ServedFile) != d["path"] { - t.Errorf("the state the watch and the run keep: %v", d) + filepath.Dir(ErrorsFile) != d["path"] { + t.Errorf("the state the account keeps: %v", d) + } + // The scheduled run's state is root's alone, apart from the account's (third review). + if d := byID["scrub-state-dir"]; d["path"] != filepath.Dir(ServedFile) || d["mode"] != "0700" || d["owner"] != nil { + t.Errorf("the state the run keeps: %v", d) } if strings.Contains(raw, `"package"`) { t.Errorf("the module installs no package: ZFS is the machine's, and its kernel module is built against its kernel") diff --git a/modules/zfs/cmd/zfs-tools/schedule.go b/modules/zfs/cmd/zfs-tools/schedule.go index acd521b2..c9e6e4eb 100644 --- a/modules/zfs/cmd/zfs-tools/schedule.go +++ b/modules/zfs/cmd/zfs-tools/schedule.go @@ -9,6 +9,7 @@ import ( "regexp" "strconv" "strings" + "syscall" "time" ) @@ -31,8 +32,11 @@ const ( ScrubService = "zfs-mesh-scrub.service" ) -// ServedFile is where the scheduled run keeps the last slot it served, per pool. -const ServedFile = "/var/lib/zfs-tools/scrub-served.json" +// ServedFile is where the scheduled run keeps the last slot it served, per pool: in a folder of root's +// alone (0700), apart from the operator account's state, so nothing the account runs can steer the run's +// writes (third review of mesh-catalog #147). The tools, which run as the account, cannot read it, and +// judge a missed scrub from status instead. +const ServedFile = "/var/lib/zfs-scrub/scrub-served.json" // MissedAfter is how long after its slot a scrub that has not started is said to be missed: the run // fires every quarter of an hour. @@ -164,6 +168,49 @@ func (m *Machine) readServed(path string) map[string]int64 { return out } +// writeServed keeps the served slots, never through a link: the folder must be a real folder of this +// process's account alone (0700, made so when missing), and the file is written new beside it with +// O_NOFOLLOW and O_EXCL, then renamed over the old one, which replaces a link rather than following it. +func writeServed(path string, served map[string]int64) error { + dir := filepath.Dir(path) + if _, err := os.Lstat(dir); os.IsNotExist(err) { + if err := os.Mkdir(dir, 0o700); err != nil { + return err + } + } + fi, err := os.Lstat(dir) + if err != nil { + return err + } + st, _ := fi.Sys().(*syscall.Stat_t) + switch { + case fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir(): + return fmt.Errorf("%s is not a folder of its own (a link or a file): the served slot is not written", dir) + case fi.Mode().Perm() != 0o700: + return fmt.Errorf("%s is %v, not 0700: the served slot is not written", dir, fi.Mode().Perm()) + case st != nil && int(st.Uid) != os.Geteuid(): + return fmt.Errorf("%s belongs to uid %d, not this process's %d: the served slot is not written", dir, st.Uid, os.Geteuid()) + } + if fi, err := os.Lstat(path); err == nil && !fi.Mode().IsRegular() { + return fmt.Errorf("%s is not a plain file (a link or something else): the served slot is not written", path) + } + b, _ := json.Marshal(served) + tmp := path + ".new" + _ = os.Remove(tmp) + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_NOFOLLOW, 0o600) + if err != nil { + return err + } + if _, err := f.Write(b); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + return os.Rename(tmp, path) +} + // startedSince says whether status shows the pool's scrub started at or after a time. func startedSince(p *PoolStatus, slot time.Time) bool { if p == nil { @@ -200,15 +247,7 @@ func (m *Machine) ScrubRun(ctx context.Context, servedPath string) (string, erro } mark := func() error { served[s.Pool] = slot.Unix() - b, _ := json.Marshal(served) - if err := os.MkdirAll(filepath.Dir(servedPath), 0o755); err != nil { - return err - } - tmp := servedPath + ".new" - if err := os.WriteFile(tmp, b, 0o644); err != nil { - return err - } - return os.Rename(tmp, servedPath) + return writeServed(servedPath, served) } if startedSince(p, slot) && p.Scrub().State != "running" { return fmt.Sprintf("the scrub of %s scheduled %s ran already", s.Pool, slot.Format("2006-01-02 15:04")), mark() diff --git a/modules/zfs/cmd/zfs-tools/scrubrun_test.go b/modules/zfs/cmd/zfs-tools/scrubrun_test.go index 7b402362..857256e0 100644 --- a/modules/zfs/cmd/zfs-tools/scrubrun_test.go +++ b/modules/zfs/cmd/zfs-tools/scrubrun_test.go @@ -52,7 +52,7 @@ func rootFake(t *testing.T, conf string) (*fake, string) { f := withZFS(t) f.uid = 0 f.files[ScrubConfig] = conf - dir := t.TempDir() + dir := filepath.Join(t.TempDir(), "zfs-scrub") return f, filepath.Join(dir, "scrub-served.json") } @@ -166,3 +166,41 @@ func TestAScheduledScrubThatDidNotStartIsAWarning(t *testing.T) { t.Errorf("warnings: %v", h.Warnings) } } + +// Third review: the scheduled run's state is root's alone — a folder of its own, 0700, never through a +// link — so the operator account, or anything it runs, cannot make the run write elsewhere. +func TestTheServedSlotIsWrittenOnlyIntoAPrivateFolderAndNeverThroughALink(t *testing.T) { + dir := filepath.Join(t.TempDir(), "zfs-scrub") + path := filepath.Join(dir, "scrub-served.json") + if err := writeServed(path, map[string]int64{"tank": 1}); err != nil { + t.Fatal(err) + } + if fi, err := os.Lstat(dir); err != nil || fi.Mode().Perm() != 0o700 || !fi.IsDir() { + t.Errorf("the folder: %v %v", fi.Mode(), err) + } + // The file replaced by a link to somewhere else: refused, and the other place untouched. + victim := filepath.Join(t.TempDir(), "victim") + os.WriteFile(victim, []byte("keep"), 0o644) + os.Remove(path) + os.Symlink(victim, path) + if err := writeServed(path, map[string]int64{"tank": 2}); err == nil { + t.Errorf("written through a link") + } + if b, _ := os.ReadFile(victim); string(b) != "keep" { + t.Errorf("the link's target was changed: %q", b) + } + // The folder itself a link, or open to others: refused. + linked := filepath.Join(t.TempDir(), "linked") + os.Symlink(t.TempDir(), linked) + if err := writeServed(filepath.Join(linked, "scrub-served.json"), map[string]int64{"tank": 3}); err == nil { + t.Errorf("written into a linked folder") + } + open := filepath.Join(t.TempDir(), "open") + os.Mkdir(open, 0o755) + if err := writeServed(filepath.Join(open, "scrub-served.json"), map[string]int64{"tank": 3}); err == nil { + t.Errorf("written into a folder others can read") + } + if filepath.Dir(ServedFile) == filepath.Dir(ErrorsFile) { + t.Errorf("the run's state shares the account's folder") + } +} diff --git a/modules/zfs/module.json b/modules/zfs/module.json index 7b4de50e..6a2f3301 100644 --- a/modules/zfs/module.json +++ b/modules/zfs/module.json @@ -153,6 +153,12 @@ "mode": "0644", "content": "# Written by the mesh (module zfs) from this machine's assignment. Replaced on every push; change the\n# settings scrub-pool, scrub-cadence, scrub-day and scrub-time, never this file. The module's tools read it.\npool=${setting:scrub-pool}\ncadence=${setting:scrub-cadence}\nday=${setting:scrub-day}\ntime=${setting:scrub-time}\n" }, + { + "id": "scrub-state-dir", + "type": "directory", + "path": "/var/lib/zfs-scrub", + "mode": "0700" + }, { "id": "scrub-run", "type": "process",