From 241079686980a12716048608785745e64743b16c Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 00:53:05 +0200 Subject: [PATCH 1/3] Let the setuid search run to completion and judge from the last complete one (hq issue 361) A machine with millions of files never finished the walk within fifteen minutes, so its agent account was never judged and the router refused to link. Only root can make a setuid-root file, so a complete search stays sound until root acts: it now runs at idle priority with no bound, is kept beside the engine's state, judges for rootsearch.FreshFor from its start, runs again daily and after an apply that moves accounts, packages, actions or sudo rules, and an incomplete or stale search is never free. --- cmd/mesh-host/main.go | 22 +- internal/accounts/exec.go | 3 + internal/accounts/idle_linux.go | 68 +++++ internal/accounts/idle_other.go | 10 + internal/accounts/setuid_cache.go | 338 +++++++++++++++++++++++++ internal/accounts/setuid_cache_test.go | 292 +++++++++++++++++++++ internal/accounts/ways.go | 130 +--------- internal/accounts/ways_test.go | 77 ------ internal/apply/root_moved.go | 46 ++++ internal/apply/root_moved_test.go | 29 +++ rootsearch/rootsearch.go | 40 ++- 11 files changed, 853 insertions(+), 202 deletions(-) create mode 100644 internal/accounts/idle_linux.go create mode 100644 internal/accounts/idle_other.go create mode 100644 internal/accounts/setuid_cache.go create mode 100644 internal/accounts/setuid_cache_test.go create mode 100644 internal/apply/root_moved.go create mode 100644 internal/apply/root_moved_test.go diff --git a/cmd/mesh-host/main.go b/cmd/mesh-host/main.go index 8b237c2..9267b9b 100644 --- a/cmd/mesh-host/main.go +++ b/cmd/mesh-host/main.go @@ -1120,7 +1120,11 @@ func runLink(ctx context.Context, opts options) error { // And which units its service managers say failed, and whose each is (novox/hq issue 315). unitJudge = units.New(units.Exec{Run: apply.ExecRunner, System: builtFor}) // And whether an account a module put in a group has it where it runs (novox/hq ADR 0252). - accountJudge = accounts.New(accounts.Exec{Run: accounts.CLocale, Cache: &accounts.SetuidCache{Every: time.Hour}}) + // Its search for setuid programs runs to its end at idle priority, and the last complete one is kept + // beside this state, so a restart judges from it (novox/hq issue 361). + setuidSearch = &accounts.SetuidCache{Path: filepath.Join(filepath.Dir(opts.state), accounts.SetuidSearchFile), + Say: say} + accountJudge = accounts.New(accounts.Exec{Run: accounts.CLocale, SearchRun: accounts.Idle, Cache: setuidSearch}) } // **Standing aside for a successor happens between reconciles and nowhere else** (novox/hq ADR @@ -1431,6 +1435,10 @@ var unitJudge *units.Judge // ADR 0252); nil where judging is. var accountJudge *accounts.Judge +// setuidSearch is accountJudge's search for setuid programs, asked to search again after an apply that moved +// what root controls (novox/hq issue 361); nil where accountJudge is. +var setuidSearch *accounts.SetuidCache + // netJudge is the serving host's judge of its machine's networking (novox/hq ADR 0241); empty in a // one-shot command and in a test, which say nothing of the network. var netJudge networkJudge @@ -1891,6 +1899,18 @@ func applyAndKeepHeld(ctx context.Context, opts options, raw []byte, signed *sto // either way, and the node is held to it from the next reconcile (novox/hq ADR 0100). updated.Mode = apply.ModeOf(declared) + // Root acted on what an agent account's judgement reads — an account, a package, an action, sudo's + // rules: the search for setuid programs runs again (novox/hq issue 361). A failed apply counts what it + // did before it stopped. + moved := outcome.Outcomes + var partial *apply.Error + if errors.As(applyErr, &partial) { + moved = append(append([]apply.Outcome(nil), moved...), partial.Done.Outcomes...) + } + if apply.MovesRoot(moved) { + setuidSearch.SearchAgain() + } + // Saved whichever way it went. Recording only on success would lose the footprint of a // failed apply, and that footprint is on the machine either way. if saveErr := store.Save(opts.state, updated); saveErr != nil { diff --git a/internal/accounts/exec.go b/internal/accounts/exec.go index 67ededc..8260a30 100644 --- a/internal/accounts/exec.go +++ b/internal/accounts/exec.go @@ -34,6 +34,9 @@ type Exec struct { ACL func(path string) ([]ACLEntry, error) // Cache keeps the search for setuid programs between looks; nil searches on every look. Cache *SetuidCache + // SearchRun runs the setuid search's commands (find, and the package manager it asks); nil is Run. A + // serving engine gives Idle, so the walk of every filesystem yields to everything else on the machine. + SearchRun Runner // Now is the clock the cache is kept by; nil is the machine's. Now func() time.Time } diff --git a/internal/accounts/idle_linux.go b/internal/accounts/idle_linux.go new file mode 100644 index 0000000..7ccfa38 --- /dev/null +++ b/internal/accounts/idle_linux.go @@ -0,0 +1,68 @@ +//go:build linux + +package accounts + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "runtime" + "strings" + "syscall" + + "golang.org/x/sys/unix" +) + +// ioprioIdle is the idle I/O class (IOPRIO_CLASS_IDLE, 3, shifted by IOPRIO_CLASS_SHIFT, 13): the process gets +// the disk only when nothing else asks for it, where the disk's scheduler honours classes (BFQ does). +const ioprioIdle = 3 << 13 + +// Idle runs a command as CLocale does, at the lowest priority the machine has (novox/hq issue 361): CPU nice +// 19 and the idle I/O class, set on the process as soon as it has started — so the search for setuid programs, +// which walks every file and runs as long as that takes, yields to everything else. It dies with the engine +// (the parent-death signal), so a restarted engine never leaves a walk running beside its own. A priority it +// could not lower is a failure: the command is killed rather than walk the disks at full priority. +func Idle(ctx context.Context, name string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, name, args...) + cmd.Env = append(os.Environ(), "LC_ALL=C", "LANG=C", "LANGUAGE=C") + cmd.SysProcAttr = &syscall.SysProcAttr{Pdeathsig: syscall.SIGKILL} + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + // The parent-death signal follows the thread that started the child, not the process: held on this + // thread until the child has ended, so the runtime retiring a thread never kills a walk midway. + runtime.LockOSThread() + defer runtime.UnlockOSThread() + if err := cmd.Start(); err != nil { + return "", fmt.Errorf("%s: %w", name, err) + } + pid := cmd.Process.Pid + if err := lower(pid); err != nil { + _ = cmd.Process.Kill() + _ = cmd.Wait() + return "", fmt.Errorf("%s: its priority could not be lowered, so it was stopped: %w", name, err) + } + err := cmd.Wait() + out := stdout.String() + if err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + return out, fmt.Errorf("%s exited %d: %s", name, exit.ExitCode(), strings.TrimSpace(stderr.String())) + } + return out, fmt.Errorf("%s: %w", name, err) + } + return out, nil +} + +// lower sets a process's CPU nice to 19 and its I/O class to idle. +func lower(pid int) error { + if err := unix.Setpriority(unix.PRIO_PROCESS, pid, 19); err != nil { + return fmt.Errorf("nice: %w", err) + } + if _, _, errno := unix.Syscall(unix.SYS_IOPRIO_SET, 1 /* IOPRIO_WHO_PROCESS */, uintptr(pid), ioprioIdle); errno != 0 { + return fmt.Errorf("I/O class: %w", errno) + } + return nil +} diff --git a/internal/accounts/idle_other.go b/internal/accounts/idle_other.go new file mode 100644 index 0000000..2dff47f --- /dev/null +++ b/internal/accounts/idle_other.go @@ -0,0 +1,10 @@ +//go:build !linux + +package accounts + +import "context" + +// Idle is CLocale where the machine offers no idle class this engine sets (novox/hq issue 361). +func Idle(ctx context.Context, name string, args ...string) (string, error) { + return CLocale(ctx, name, args...) +} diff --git a/internal/accounts/setuid_cache.go b/internal/accounts/setuid_cache.go new file mode 100644 index 0000000..b85b648 --- /dev/null +++ b/internal/accounts/setuid_cache.go @@ -0,0 +1,338 @@ +package accounts + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "slices" + "sync" + "syscall" + "time" + + "github.com/novox/mesh-host/rootsearch" +) + +// SetuidCache keeps the search for setuid programs, which walks every local filesystem, and runs it in the +// background (novox/hq ADR 0266, issue 361): a look never waits for it, and the search itself is never cut +// short. +// +// - **A verdict comes only from a complete search, while it is fresh**: one that walked everything and had the +// package manager answer for every path found, started less than FreshFor ago. Before the first completes, +// or once the last is older, the answer is "not judged yet" (ReasonPending) while a search runs, and "not +// judged" (ReasonIncomplete) after one failed. Never "none found" from a search that did not finish. +// - **It runs to completion**, once at a time, with no bound: a fresh one when the last complete one is Every +// old, when SearchAgain asks (an apply changed what root controls), or when a filesystem appeared that the +// last one did not walk. One that fails is tried again after a back-off that doubles from FailedBackoff up +// to MaxBackoff. +// - **Kept** in Path, beside the engine's other state, with when it started and finished: an engine restart +// serves the last complete search rather than judging nothing until a new walk ends. +// +// Why a complete search stays sound for a day and more is said in the rootsearch package: only root makes a +// setuid-root file, never the agent account. +type SetuidCache struct { + // Every and FreshFor are rootsearch's when zero. + Every, FreshFor time.Duration + // Path is the file the last complete search is kept in; empty keeps it in memory only. The engine's own + // state directory, which only root writes: a file there that is not a regular file root owns and alone + // may write is not read (keptSearch). + Path string + // Say is where a search's end and a kept search refused are said; nil says nothing. + Say func(string) + + mu sync.Mutex + wg sync.WaitGroup + loaded bool + last *setuidResult // the last complete search + running bool + startedAt time.Time // when the search running, or the last one, was started + again bool // asked to search again since the running or last one started + failedAt time.Time + failed error + backoff time.Duration + starts int // how many searches were started, for a test +} + +// setuidResult is one complete search, as kept. +type setuidResult struct { + // Started and Finished are on the caller's clock: Finished is Started plus how long the search took. + Started time.Time `json:"started"` + Finished time.Time `json:"finished"` + // Mounts are the filesystems it walked; Found the setuid-root programs no package owns. + Mounts []string `json:"mounts"` + Found []string `json:"found"` +} + +const ( + // FailedBackoff is the first wait after a search failed, doubling to MaxBackoff. + FailedBackoff = time.Hour + MaxBackoff = 6 * time.Hour + // AgainAfter is the least time between the starts of two searches that SearchAgain or a new filesystem + // asked for: a run of applies does not walk the disks back to back. + AgainAfter = time.Hour + // clockSkew is how far in the future a kept search may say it started and still judge: a clock set back + // further makes it no verdict, never a fresh one. + clockSkew = 5 * time.Minute + // SetuidSearchFile is the file's name beside the engine's state. + SetuidSearchFile = "setuid-search.json" +) + +// SetuidNotJudged is the search's answer when no complete search judges: none has finished yet, the last one is +// older than FreshFor, or every one since failed. The judge says the way is not judged, never that there is none. +type SetuidNotJudged struct { + // Pending is true while a search runs; false after one failed. + Pending bool + Since time.Time + Err error + // Last is when the last complete search started, when there is one too old to judge. + Last time.Time +} + +func (e *SetuidNotJudged) Error() string { + stale := "" + if !e.Last.IsZero() { + stale = fmt.Sprintf("; the last complete one, started %s, is too old to judge", e.Last.Local().Format("2006-01-02 15:04")) + } + if e.Pending { + return fmt.Sprintf("%s: the search for setuid programs, started %s, has not finished yet%s", ReasonPending, + e.Since.Local().Format("2006-01-02 15:04"), stale) + } + return fmt.Sprintf("%s: the search for setuid programs did not complete (last tried %s: %v); it is tried "+ + "again later%s", ReasonIncomplete, e.Since.Local().Format("2006-01-02 15:04"), e.Err, stale) +} + +// The reasons of a root verdict the setuid search could not answer. +const ( + ReasonPending = rootsearch.ReasonPending + ReasonIncomplete = rootsearch.ReasonIncomplete +) + +func (c *SetuidCache) every() time.Duration { + if c.Every > 0 { + return c.Every + } + return rootsearch.Every +} + +func (c *SetuidCache) freshFor() time.Duration { + if c.FreshFor > 0 { + return c.FreshFor + } + return rootsearch.FreshFor +} + +// SearchAgain asks for a new search at the next look: an apply changed what root controls (an account, a group, +// a sudo rule, a package, an action). The last complete search keeps judging until it is too old: only root acts +// through an apply, and the agent account cannot make a setuid-root file. Nil c is nothing to ask. +func (c *SetuidCache) SearchAgain() { + if c == nil { + return + } + c.mu.Lock() + defer c.mu.Unlock() + c.again = true +} + +// Look answers the setuid-root programs no package owns as the last complete search found them, and when it +// started — while it is fresh — and starts a search in the background when one is due; it never waits for one. +// mounts are the filesystems a search walks now. Nil c searches in place, for a caller that wants to. +// +// **One clock: the caller's.** A search's time is the `now` it was started at, never the wall clock read when it +// finished: due-ness compares a later `now` with it, and mixing the two clocks made a search never due again +// whenever the caller's clock ran behind the wall clock (found while checking issue 356). Measured from the +// start, a result reads at most one search older than it is, which errs towards not judging. +func (c *SetuidCache) Look(now time.Time, mounts []string, + search func(ctx context.Context, mounts []string) ([]string, error)) ([]string, time.Time, error) { + if c == nil { + found, err := search(context.Background(), mounts) + return found, now, err + } + c.mu.Lock() + defer c.mu.Unlock() + if !c.loaded { + c.loaded = true + c.last = c.load() + } + var age time.Duration + if c.last != nil { + age = now.Sub(c.last.Started) + } + fresh := c.last != nil && age > -clockSkew && age < c.freshFor() + asked := c.again || (c.last != nil && !covers(c.last.Mounts, mounts)) + due := !c.running && (c.failed == nil || now.Sub(c.failedAt) >= c.backoff) && + (!fresh || age >= c.every() || (asked && (c.startedAt.IsZero() || now.Sub(c.startedAt) >= AgainAfter))) + if due { + c.start(now, mounts, search) + } + switch { + case fresh: + return slices.Clone(c.last.Found), c.last.Started, nil + case c.running: + return nil, time.Time{}, &SetuidNotJudged{Pending: true, Since: c.startedAt, Last: c.lastStarted()} + case c.failed != nil: + return nil, time.Time{}, &SetuidNotJudged{Since: c.failedAt, Err: c.failed, Last: c.lastStarted()} + default: + // Not fresh, not running, never failed: cannot be, as not fresh is due. Never a verdict. + return nil, time.Time{}, &SetuidNotJudged{Pending: true, Since: now, Last: c.lastStarted()} + } +} + +func (c *SetuidCache) lastStarted() time.Time { + if c.last == nil { + return time.Time{} + } + return c.last.Started +} + +// start runs one search in the background, with no bound: c.mu held. +func (c *SetuidCache) start(now time.Time, mounts []string, search func(context.Context, []string) ([]string, error)) { + c.running, c.startedAt, c.again = true, now, false + c.starts++ + mounts = slices.Clone(mounts) + began := time.Now() + c.wg.Add(1) + go func() { + defer c.wg.Done() + found, err := search(context.Background(), mounts) + took := time.Since(began) + c.mu.Lock() + defer c.mu.Unlock() + c.running = false + if err != nil { + c.failed, c.failedAt = err, now + c.backoff = min(max(2*c.backoff, FailedBackoff), MaxBackoff) + c.say(fmt.Sprintf("the search for setuid programs started %s did not complete after %s: %v", + now.Local().Format("15:04"), took.Round(time.Second), err)) + return + } + r := &setuidResult{Started: now, Finished: now.Add(took), Mounts: mounts, Found: found} + c.last, c.failed, c.backoff = r, nil, 0 + if err := c.keep(r); err != nil { + c.say("the search for setuid programs could not be kept, and judges from memory until the engine " + + "restarts: " + err.Error()) + } + c.say(fmt.Sprintf("the search for setuid programs started %s completed in %s: %d no package owns", + now.Local().Format("15:04"), took.Round(time.Second), len(found))) + }() +} + +func (c *SetuidCache) say(s string) { + if c.Say != nil { + c.Say(s) + } +} + +// covers says every filesystem a search would walk now was walked. +func covers(walked, now []string) bool { + for _, m := range now { + if !slices.Contains(walked, m) { + return false + } + } + return true +} + +// Wait is for a test: until the search running, if any, has finished. +func (c *SetuidCache) Wait() { c.wg.Wait() } + +// keep writes a complete search to Path: a new file beside it, readable by root alone, renamed over it. +func (c *SetuidCache) keep(r *setuidResult) error { + if c.Path == "" { + return nil + } + raw, err := json.Marshal(r) + if err != nil { + return err + } + f, err := os.CreateTemp(filepath.Dir(c.Path), "."+filepath.Base(c.Path)+".*") + if err != nil { + return err + } + defer os.Remove(f.Name()) + if _, err := f.Write(raw); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + return os.Rename(f.Name(), c.Path) +} + +// load reads the kept search, or nil: none kept, or one that cannot be believed, said why. +func (c *SetuidCache) load() *setuidResult { + if c.Path == "" { + return nil + } + r, err := keptSearch(c.Path) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + c.say("the kept search for setuid programs is not read, and a new one runs: " + err.Error()) + } + return r +} + +// keptSearch reads a kept search, believing it only as a regular file, not followed through a link, owned by +// the engine's own account and writable by it alone, whose directory is the same: anything else could have been +// written by somebody other than root, and a search it says is no search. +func keptSearch(path string) (*setuidResult, error) { + if err := ownedAlone(filepath.Dir(path), true); err != nil { + return nil, err + } + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW, 0) + if err != nil { + return nil, err + } + defer f.Close() + info, err := f.Stat() + if err != nil { + return nil, err + } + if err := ownedAloneInfo(path, info, false); err != nil { + return nil, err + } + raw, err := io.ReadAll(io.LimitReader(f, 1<<20)) + if err != nil { + return nil, err + } + var r setuidResult + if err := json.Unmarshal(raw, &r); err != nil { + return nil, fmt.Errorf("%s is not a kept search: %w", path, err) + } + if r.Started.IsZero() || r.Finished.Before(r.Started) || len(r.Mounts) == 0 { + return nil, fmt.Errorf("%s says no complete search", path) + } + return &r, nil +} + +func ownedAlone(path string, dir bool) error { + info, err := os.Lstat(path) + if err != nil { + return err + } + return ownedAloneInfo(path, info, dir) +} + +func ownedAloneInfo(path string, info fs.FileInfo, dir bool) error { + if dir && !info.IsDir() || !dir && !info.Mode().IsRegular() { + return fmt.Errorf("%s is not a plain %s", path, map[bool]string{true: "directory", false: "file"}[dir]) + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("%s's owner could not be read", path) + } + if int(st.Uid) != os.Geteuid() { + return fmt.Errorf("%s is owned by uid %d, not the engine's %d", path, st.Uid, os.Geteuid()) + } + if info.Mode().Perm()&0o022 != 0 { + return fmt.Errorf("%s may be written by others than its owner (mode %o)", path, info.Mode().Perm()) + } + return nil +} diff --git a/internal/accounts/setuid_cache_test.go b/internal/accounts/setuid_cache_test.go new file mode 100644 index 0000000..c072b93 --- /dev/null +++ b/internal/accounts/setuid_cache_test.go @@ -0,0 +1,292 @@ +package accounts + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/novox/mesh-host/rootsearch" +) + +// The search for setuid programs runs to completion and judges from its last complete result while it is fresh +// (novox/hq issue 361): never a bound that ends it, never a verdict from a search that did not finish or is too +// old, a new search after root acted, and the result kept beside the engine's state, believed only as root's. + +var at = time.Date(2026, 10, 10, 1, 0, 0, 0, time.UTC) + +var root = []string{"/"} + +// gate is a search that waits until released; each search started hands its context on started. found and +// err are read when it is released. +type gate struct { + release chan struct{} + started chan context.Context + found []string + err error +} + +func newGate() *gate { + return &gate{release: make(chan struct{}, 16), started: make(chan context.Context, 16)} +} + +func (g *gate) search(ctx context.Context, _ []string) ([]string, error) { + g.started <- ctx + <-g.release + return g.found, g.err +} + +func (g *gate) finish(c *SetuidCache) { g.release <- struct{}{}; c.Wait() } + +// starts is how many searches c started. +func starts(c *SetuidCache) int { + c.mu.Lock() + defer c.mu.Unlock() + return c.starts +} + +func notJudged(t *testing.T, err error, pending bool) *SetuidNotJudged { + t.Helper() + var nj *SetuidNotJudged + if !errors.As(err, &nj) || nj.Pending != pending { + t.Fatalf("want not judged (pending %v), got %v", pending, err) + } + want := ReasonIncomplete + if pending { + want = ReasonPending + } + if !strings.HasPrefix(err.Error(), want) { + t.Fatalf("the words start %q: %v", want, err) + } + return nj +} + +func TestTheSearchRunsToCompletionWithNoBound(t *testing.T) { + c := &SetuidCache{} + g := newGate() + if _, _, err := c.Look(at, root, g.search); err == nil { + t.Fatal("a look before the first search finished judged") + } + ctx := <-g.started + if _, ok := ctx.Deadline(); ok { + t.Fatal("the search was given a deadline: it must run until it has walked everything") + } + // Hours later, still running: still pending, never killed, never a second walk beside it. + _, _, err := c.Look(at.Add(5*time.Hour), root, g.search) + notJudged(t, err, true) + if starts(c) != 1 || ctx.Err() != nil { + t.Fatalf("a running search was started again or stopped: %d searches, %v", starts(c), ctx.Err()) + } + g.found = []string{"/opt/x"} + g.finish(c) + found, when, err := c.Look(at.Add(5*time.Hour), root, g.search) + if err != nil || len(found) != 1 || !when.Equal(at) { + t.Fatalf("the complete search judges, from its start: %v %v %v", found, when, err) + } +} + +func TestAFreshCompleteSearchJudgesUntilItIsDue(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + if _, _, err := c.Look(at.Add(rootsearch.Every-time.Minute), root, g.search); err != nil { + t.Fatalf("a complete search within a day: %v", err) + } + if starts(c) != 1 { + t.Fatalf("searched again before it was due: %d", starts(c)) + } + // Due: a new search starts, and the last complete one judges meanwhile. + if _, _, err := c.Look(at.Add(rootsearch.Every), root, g.search); err != nil { + t.Fatalf("while the daily search runs, the last complete one still judges: %v", err) + } + if starts(c) != 2 { + t.Fatalf("the daily search did not start: %d", starts(c)) + } + g.finish(c) +} + +func TestAStaleSearchIsNotJudgedNeverFree(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + c.Look(at.Add(rootsearch.Every), root, g.search) // the daily one, which does not finish + _, _, err := c.Look(at.Add(rootsearch.FreshFor), root, g.search) + nj := notJudged(t, err, true) + if !nj.Last.Equal(at) || !strings.Contains(err.Error(), "too old to judge") { + t.Fatalf("a stale search says when the last complete one started: %v", err) + } + g.finish(c) + if _, _, err := c.Look(at.Add(rootsearch.FreshFor), root, g.search); err != nil { + t.Fatalf("once the new search completed it judges again: %v", err) + } +} + +func TestAnIncompleteSearchIsNeverFree(t *testing.T) { + c := &SetuidCache{} + g := newGate() + g.err = errors.New("the search for setuid programs did not complete its walk") + c.Look(at, root, g.search) + g.finish(c) + _, _, err := c.Look(at.Add(time.Minute), root, g.search) + notJudged(t, err, false) + if starts(c) != 1 { + t.Fatalf("tried again within its back-off: %d", starts(c)) + } + c.Look(at.Add(FailedBackoff), root, g.search) + if starts(c) != 2 { + t.Fatalf("not tried again after its back-off: %d", starts(c)) + } + g.finish(c) + // A stale complete search and a failed new one: still not judged, never the stale one. + c = &SetuidCache{} + g = newGate() + c.Look(at, root, g.search) + g.finish(c) + g.err = errors.New("pacman could not say who owns the setuid programs found") + c.Look(at.Add(rootsearch.Every), root, g.search) + g.finish(c) + if _, _, err := c.Look(at.Add(rootsearch.Every+time.Hour), root, g.search); err != nil { + t.Fatalf("a failed search leaves the fresh complete one judging: %v", err) + } + _, _, err = c.Look(at.Add(rootsearch.FreshFor), root, g.search) + if err == nil { + t.Fatal("a stale search judged after the newer one failed") + } + g.finish(c) +} + +func TestAnApplyThatMovedRootSearchesAgain(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + c.SearchAgain() + if _, _, err := c.Look(at.Add(AgainAfter), root, g.search); err != nil { + t.Fatalf("the last complete search judges while the new one runs: %v", err) + } + if starts(c) != 2 { + t.Fatalf("an apply that moved root did not search again: %d", starts(c)) + } + // Asked again while one runs: one more after it, never two at once. + c.SearchAgain() + c.Look(at.Add(AgainAfter+time.Minute), root, g.search) + if starts(c) != 2 { + t.Fatalf("a second search started beside the running one: %d", starts(c)) + } + g.finish(c) + c.Look(at.Add(2*AgainAfter+time.Minute), root, g.search) + if starts(c) != 3 { + t.Fatalf("the ask made while a search ran was dropped: %d", starts(c)) + } + g.finish(c) + // A filesystem the last search did not walk asks for one too. + c.Look(at.Add(4*AgainAfter), []string{"/", "/srv"}, g.search) + if starts(c) != 4 { + t.Fatalf("a new filesystem was not searched: %d", starts(c)) + } + g.finish(c) + var nilCache *SetuidCache + nilCache.SearchAgain() +} + +func TestTheCompleteSearchIsKeptAndReadAfterARestart(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, SetuidSearchFile) + c := &SetuidCache{Path: path} + g := newGate() + g.found = []string{"/opt/x"} + c.Look(at, root, g.search) + g.finish(c) + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != 0o600 { + t.Fatalf("kept readable by its owner alone: %v %v", info, err) + } + restarted := &SetuidCache{Path: path} + found, when, err := restarted.Look(at.Add(time.Hour), root, newGate().search) + if err != nil || len(found) != 1 || !when.Equal(at) { + t.Fatalf("a restarted engine judges from the kept search: %v %v %v", found, when, err) + } + restarted.Wait() + // And not once it is too old. + again := &SetuidCache{Path: path} + g2 := newGate() + if _, _, err := again.Look(at.Add(rootsearch.FreshFor), root, g2.search); err == nil { + t.Fatal("a kept search too old to judge judged") + } + g2.finish(again) +} + +func TestAKeptSearchNotRootsAloneIsNotRead(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, SetuidSearchFile) + c := &SetuidCache{Path: path} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + for name, spoil := range map[string]func() error{ + "writable by others": func() error { return os.Chmod(path, 0o666) }, + "a link": func() error { + other := filepath.Join(dir, "elsewhere.json") + if err := os.Rename(path, other); err != nil { + return err + } + return os.Symlink(other, path) + }, + "its directory writable by others": func() error { return os.Chmod(dir, 0o777) }, + } { + c = &SetuidCache{Path: path} + g = newGate() + c.Look(at, root, g.search) + g.finish(c) // a fresh, good one kept again + os.Chmod(dir, 0o700) + if err := spoil(); err != nil { + t.Fatal(err) + } + var said []string + r := &SetuidCache{Path: path, Say: func(s string) { said = append(said, s) }} + gr := newGate() + if _, _, err := r.Look(at.Add(time.Minute), root, gr.search); err == nil { + t.Fatalf("%s: a kept search that is not root's alone judged", name) + } + if len(said) == 0 { + t.Fatalf("%s: refusing the kept search said nothing", name) + } + gr.finish(r) + os.Chmod(dir, 0o700) + os.Remove(path) + } +} + +func TestAKeptSearchFromTheFutureIsNotFresh(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + if _, _, err := c.Look(at.Add(-time.Hour), root, g.search); err == nil { + t.Fatal("a clock set back an hour read a search as fresh") + } + g.finish(c) +} + +func TestTheIdleRunnerLowersItsCommand(t *testing.T) { + if _, err := exec.LookPath("nice"); err != nil { + t.Skip("no nice here") + } + out, err := Idle(t.Context(), "sh", "-c", "sleep 0.3; nice") + if err != nil { + t.Fatal(err) + } + if strings.TrimSpace(out) != "19" { + t.Fatalf("the search's commands run at nice %q, want 19", strings.TrimSpace(out)) + } + if _, err := Idle(t.Context(), "sh", "-c", "echo gone >&2; exit 1"); err == nil || + !strings.HasPrefix(err.Error(), "sh exited 1: gone") { + t.Fatalf("its failure is worded as CLocale's: %v", err) + } +} diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index 122b0cf..8d6c129 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -10,14 +10,12 @@ import ( "encoding/binary" "errors" "fmt" - "github.com/novox/mesh-host/rootsearch" "io/fs" "os" "os/exec" "path/filepath" "sort" "strings" - "sync" "time" "golang.org/x/sys/unix" @@ -380,114 +378,16 @@ func contains(xs []string, s string) bool { return false } -// SetuidCache keeps the search for setuid programs, which walks every local filesystem, and runs it in the -// background (novox/hq ADR 0266, the third review): a disk too large to search in time must never stall a -// look, the health statement or the apply report. A look reads the last result the search left, and starts a -// new search when the last one is older than Every; a search that failed or did not finish is tried again -// after a back-off that doubles, from Every up to MaxBackoff. -type SetuidCache struct { - Every time.Duration - - mu sync.Mutex - wg sync.WaitGroup - at time.Time // when the last search that finished was started, on the caller's clock - found []string - running bool - startedAt time.Time - failedAt time.Time - failed error - backoff time.Duration -} - -// SearchBound is how long one search may run in the background — the whole search, the walk and the package -// manager's answers together (rootsearch.Bound, which the controller reads too); MaxBackoff the longest wait -// after failures. -const ( - SearchBound = rootsearch.Bound - MaxBackoff = 6 * time.Hour -) - -// SetuidNotJudged is the search's answer when it has no result to give: never run to its end yet, or failed -// every time so far. The judge says the way is not judged, never that there is none. -type SetuidNotJudged struct { - // Pending is true while the first search runs; false after one failed or did not finish. - Pending bool - Since time.Time - Err error -} - -func (e *SetuidNotJudged) Error() string { - if e.Pending { - return fmt.Sprintf("%s: the search for setuid programs, started at %s, has not finished yet", ReasonPending, - e.Since.Local().Format("15:04")) - } - return fmt.Sprintf("%s: the search for setuid programs did not finish (last tried at %s: %v); it is tried "+ - "again later", ReasonIncomplete, e.Since.Local().Format("15:04"), e.Err) -} - -// The reasons of a root verdict the setuid search could not answer. -const ( - ReasonPending = rootsearch.ReasonPending - ReasonIncomplete = rootsearch.ReasonIncomplete -) - -// Look answers the last result and when its search was started, starting a search in the background when one is -// due; it never waits for one. Nil c searches in place, for a caller that wants to. -// -// **One clock: the caller's.** A search's time is the `now` it was started at, never the wall clock read when it -// finished: due-ness compares a later `now` with it, and mixing the two clocks made a search never due again -// whenever the caller's clock ran behind the wall clock — the test with a fixed date failed on every branch from -// 2026-10-09 18:00 UTC (found while checking issue 356). Measured from the start, a result reads at most one -// search older than it is, which errs towards searching again. -func (c *SetuidCache) Look(now time.Time, search func(ctx context.Context) ([]string, error)) ([]string, time.Time, error) { - if c == nil { - ctx, cancel := context.WithTimeout(context.Background(), SearchBound) - defer cancel() - found, err := search(ctx) - return found, now, err - } - c.mu.Lock() - defer c.mu.Unlock() - due := !c.running && (c.at.IsZero() || now.Sub(c.at) >= c.Every) && - (c.failed == nil || now.Sub(c.failedAt) >= c.backoff) - if due { - c.running, c.startedAt = true, now - started := now - c.wg.Add(1) - go func() { - defer c.wg.Done() - ctx, cancel := context.WithTimeout(context.Background(), SearchBound) - found, err := search(ctx) - cancel() - c.mu.Lock() - defer c.mu.Unlock() - c.running = false - if err != nil { - c.failed, c.failedAt = err, started - c.backoff = min(max(2*c.backoff, c.Every), MaxBackoff) - return - } - c.found, c.at, c.failed, c.backoff = found, started, nil, 0 - }() - } - switch { - case !c.at.IsZero(): - return c.found, c.at, nil - case c.failed != nil: - return nil, time.Time{}, &SetuidNotJudged{Since: c.failedAt, Err: c.failed} - default: - return nil, time.Time{}, &SetuidNotJudged{Pending: true, Since: c.startedAt} - } -} - -// Wait is for a test: until the search running, if any, has finished. -func (c *SetuidCache) Wait() { c.wg.Wait() } - // setuidSearch is every setuid- or setgid-root regular file on the root filesystem that no installed package -// owns, found with find and asked of the package manager. Bounded by its caller (SearchBound, the whole search): -// a search that does not finish is an unanswered question, never "none". A walk find could not complete — it -// exited with an error — is incomplete too, unless every error was a file that vanished while it walked. +// owns, found with find and asked of the package manager, through the search's runner (Exec.SearchRun: idle +// priority on a serving engine). **No bound of its own** (novox/hq issue 361): it runs until it has walked +// everything, and only a context its caller ends stops it — a search that is stopped, or whose walk find could +// not complete (it exited with an error), is an unanswered question, never "none", unless every error was a +// file that vanished while it walked. func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, error) { + if e.SearchRun != nil { + e.Run = e.SearchRun + } // Each mount point a starting point of its own, -xdev keeping each to its own filesystem: every local // filesystem mounted without nosuid is searched once (the re-review of 2026-10-08), and a program setgid to // root's group counts whoever owns it. @@ -497,7 +397,7 @@ func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, erro "-2000", ")", ")", "-print") out, err := e.Run(ctx, "find", args...) if ctx.Err() != nil { - return nil, fmt.Errorf("the search for setuid programs did not finish within %s", SearchBound) + return nil, fmt.Errorf("the search for setuid programs was stopped before it finished: %w", ctx.Err()) } if err != nil && (strings.TrimSpace(out) == "" || !onlyVanished(err)) { return nil, fmt.Errorf("the search for setuid programs did not complete its walk: %w", err) @@ -715,18 +615,16 @@ func (e Exec) moreWays(ctx context.Context, account string, uid int, groups []st if len(mounts) == 0 { return nil, errors.New("no filesystem a setuid program could run from was found in /proc/mounts") } - // In the background, its last result served (SetuidCache): never a stall. No result yet is said beside - // the other ways, which are judged all the same. - unowned, searchedAt, searchErr := e.Cache.Look(now(), func(sctx context.Context) ([]string, error) { - return e.setuidSearch(sctx, mounts) - }) + // In the background, the last complete result served while it is fresh (SetuidCache): never a stall. No + // fresh complete result is said beside the other ways, which are judged all the same. + unowned, searchedAt, searchErr := e.Cache.Look(now(), mounts, e.setuidSearch) var notJudged *SetuidNotJudged if searchErr != nil && !errors.As(searchErr, ¬Judged) { return nil, searchErr } for _, p := range unowned { - ways = append(ways, "a setuid-root program no package owns: "+p+" (searched at "+ - searchedAt.Local().Format("15:04")+")") + ways = append(ways, "a setuid-root program no package owns: "+p+" (search started "+ + searchedAt.Local().Format("2006-01-02 15:04")+")") } // The kernel's protection of links: off, any account may link another's file where root then acts on it. diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index b62de60..69d8d19 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -5,7 +5,6 @@ import ( "encoding/binary" "errors" "fmt" - "github.com/novox/mesh-host/rootsearch" "os/exec" "strings" "testing" @@ -132,56 +131,6 @@ func TestAFurtherQuestionUnansweredIsUnknown(t *testing.T) { } } -func TestTheSetuidSearchRunsInTheBackgroundAndIsKeptForItsInterval(t *testing.T) { - c := &SetuidCache{Every: time.Hour} - searched := 0 - release := make(chan struct{}) - search := func(context.Context) ([]string, error) { <-release; searched++; return []string{"/opt/x"}, nil } - at := time.Date(2026, 10, 8, 19, 0, 0, 0, time.UTC) - _, _, err := c.Look(at, search) - var nj *SetuidNotJudged - if !errors.As(err, &nj) || !nj.Pending || !strings.HasPrefix(err.Error(), ReasonPending) { - t.Fatalf("a look while the first search runs answers at once, not judged yet: %v", err) - } - if _, _, err := c.Look(at.Add(time.Minute), search); err == nil { - t.Fatal("still running: still not judged") - } - close(release) - c.Wait() - found, when, err := c.Look(at.Add(30*time.Minute), search) - if err != nil || len(found) != 1 || when.IsZero() { - t.Fatalf("the result once there: %v %v %v", found, when, err) - } - c.Look(at.Add(24*time.Hour), search) - c.Wait() - if searched != 2 { - t.Fatalf("searched %d times, want 2: once at first, once when the result was older than its interval", searched) - } -} - -func TestASearchThatFailsIsNotJudgedAndBacksOff(t *testing.T) { - c := &SetuidCache{Every: time.Hour} - tries := 0 - search := func(context.Context) ([]string, error) { tries++; return nil, errors.New("did not finish") } - at := time.Now() - c.Look(at, search) - c.Wait() - _, _, err := c.Look(at.Add(time.Minute), search) - var nj *SetuidNotJudged - if !errors.As(err, &nj) || nj.Pending || !strings.HasPrefix(err.Error(), ReasonIncomplete) { - t.Fatalf("a failed search is not judged (search incomplete): %v", err) - } - c.Wait() - if tries != 1 { - t.Fatalf("tried again within its back-off: %d", tries) - } - c.Look(at.Add(2*time.Hour), search) - c.Wait() - if tries != 2 { - t.Fatalf("tried again after its back-off: %d", tries) - } -} - func TestANotJudgedSearchStillSaysAWayFound(t *testing.T) { j := New(fakeReader{ways: []string{"in the group docker, which grants root"}, err: &SetuidNotJudged{Pending: true, Since: time.Now()}}) j.Set([]Account{{Module: "m", ID: "m.a", Name: "agent", Root: true}}) @@ -320,32 +269,6 @@ polkit.addRule(function(a, s) { /* isInGroup("wheel") */ return polkit.Result.YE } } -// The search's bound is the one the controller reads (rootsearch.Bound): a search may run its whole fifteen -// minutes, with no shorter limit inside it that would end every long walk incomplete (the review of 2026-10-09). -func TestTheSearchRunsToItsBoundWithNoShorterLimitInside(t *testing.T) { - if rootsearch.Bound != 15*time.Minute { - t.Fatalf("the shared bound is %s", rootsearch.Bound) - } - m := clean() - var left time.Duration - run := func(ctx context.Context, name string, args ...string) (string, error) { - if name == "find" { - if d, ok := ctx.Deadline(); ok { - left = time.Until(d) - } - } - return m.run(ctx, name, args...) - } - ctx, cancel := context.WithTimeout(t.Context(), SearchBound) - defer cancel() - if _, err := (Exec{Run: run}).setuidSearch(ctx, []string{"/"}); err != nil { - t.Fatal(err) - } - if left < SearchBound-time.Minute { - t.Fatalf("find was given %s, less than the search's bound of %s", left, SearchBound) - } -} - // The package manager is asked about every path found in one call, and answers each: owned, or owned by nothing. func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { m := clean() diff --git a/internal/apply/root_moved.go b/internal/apply/root_moved.go new file mode 100644 index 0000000..af9bca2 --- /dev/null +++ b/internal/apply/root_moved.go @@ -0,0 +1,46 @@ +package apply + +import ( + "path/filepath" + "strings" + + "github.com/novox/mesh-host/internal/declaration" +) + +// rootPaths are the files and directories through which an account comes to root, or by which it is told who it +// is: the user and group databases, sudo's, doas's and polkit's rules. A file written there changes what the +// judge of an agent account reads (novox/hq ADR 0266). +var rootPaths = []string{"/etc/passwd", "/etc/shadow", "/etc/group", "/etc/gshadow", "/etc/sudoers", + "/etc/sudoers.d", "/etc/doas.conf", "/etc/doas.d", "/etc/polkit-1", "/usr/share/polkit-1"} + +// MovesRoot says an apply changed something through which root's reach could have changed — an account, a +// package, an action run as root, or a file in rootPaths (novox/hq issue 361). The engine then asks for a new +// search for setuid programs: root acted, and a complete search is sound only until root acts. Outcomes that +// moved nothing (unchanged, held, waiting, held still) do not count. +func MovesRoot(outcomes []Outcome) bool { + for _, o := range outcomes { + switch o.Action { + case "unchanged", "held", "waiting", heldStill: + continue + } + switch declaration.Type(o.Type) { + case declaration.TypeUser, declaration.TypePackage, declaration.TypeAction: + return true + case declaration.TypeFile, declaration.TypeDirectory, declaration.TypeArchive: + if underRootPath(o.Target) { + return true + } + } + } + return false +} + +func underRootPath(target string) bool { + t := filepath.Clean(target) + for _, p := range rootPaths { + if t == p || strings.HasPrefix(t, p+"/") { + return true + } + } + return false +} diff --git a/internal/apply/root_moved_test.go b/internal/apply/root_moved_test.go new file mode 100644 index 0000000..683f088 --- /dev/null +++ b/internal/apply/root_moved_test.go @@ -0,0 +1,29 @@ +package apply + +import "testing" + +// An apply that moved what root controls asks for a new search for setuid programs (novox/hq issue 361); one +// that moved nothing of it does not. +func TestAnApplyMovesRootThroughAnAccountAPackageAnActionOrSudosRules(t *testing.T) { + for _, c := range []struct { + o Outcome + want bool + }{ + {Outcome{Type: "user", Target: "nox", Action: "created"}, true}, + {Outcome{Type: "user", Target: "nox", Action: "unchanged"}, false}, + {Outcome{Type: "package", Target: "telegram", Action: "created"}, true}, + {Outcome{Type: "package", Target: "telegram", Action: "removed"}, true}, + {Outcome{Type: "action", Target: "setup", Action: "updated"}, true}, + {Outcome{Type: "file", Target: "/etc/sudoers.d/mesh", Action: "updated"}, true}, + {Outcome{Type: "file", Target: "/etc/group", Action: "corrected"}, true}, + {Outcome{Type: "directory", Target: "/etc/polkit-1/rules.d", Action: "created"}, true}, + {Outcome{Type: "file", Target: "/etc/sudoers.dx", Action: "updated"}, false}, + {Outcome{Type: "file", Target: "/etc/nginx/nginx.conf", Action: "updated"}, false}, + {Outcome{Type: "container", Target: "plex", Action: "updated"}, false}, + {Outcome{Type: "package", Target: "x", Action: heldStill}, false}, + } { + if got := MovesRoot([]Outcome{c.o}); got != c.want { + t.Errorf("%+v: moves root %v, want %v", c.o, got, c.want) + } + } +} diff --git a/rootsearch/rootsearch.go b/rootsearch/rootsearch.go index a1c70c0..7692529 100644 --- a/rootsearch/rootsearch.go +++ b/rootsearch/rootsearch.go @@ -1,19 +1,43 @@ // Package rootsearch holds what the node-engine's search for setuid programs and the controller that reads its -// verdicts must agree on (novox/hq ADR 0266): how long one search may run, and the words a verdict starts with -// while it has no answer. Public, so the controller imports these rather than keeps copies that could drift. +// verdicts must agree on (novox/hq ADR 0266, issue 361): how often a full search runs, how long a complete one +// judges, how long the controller stays quiet while one runs, and the words a verdict starts with while it has +// no answer. Public, so the controller imports these rather than keeps copies that could drift. +// +// **Why a complete search may judge for a day and more.** The search looks for a setuid- or setgid-root program +// no package owns, a way for the agent account to become root. Only root can make such a file: the agent account +// can neither set the setuid bit on a file root owns nor give a file it owns to root. So a search that walked +// everything stays sound until root acts — and root's acts through the mesh (an apply that changes an account, a +// group, a sudo rule, a package or runs an action) start a new search at once. What is left is root acting by +// hand, which is the operator; FreshFor bounds how long that goes unseen. +// +// **And why no bound on one search.** On a machine with millions of files the walk takes longer than any bound +// worth stating (the control-node's did not finish in fifteen minutes, issue 361), and a search ended early +// judges nothing, so a bound made such a machine never judged. The search runs to its end at idle priority, +// once at a time; an incomplete search is never "free". package rootsearch import "time" -// Bound is how long one search for setuid programs may run. It governs the whole search, the walk and the -// package manager's answers together; the controller does not raise an agent account as not judged while the -// first search after a start is within it. -const Bound = 15 * time.Minute +const ( + // Every is how often a full search runs at most, counted from the start of the last complete one; an + // apply that changes what root controls starts one sooner. + Every = 24 * time.Hour + // FreshFor is how long a complete search judges, counted from its start: after it, the verdict is "not + // judged yet" until a newer search completes. Every plus Quiet, so a daily search that finishes within + // the controller's quiet leaves no hour unjudged. + FreshFor = Every + Quiet + // Quiet is how long the controller raises nothing while an agent account waits for a search, counted + // from when it first saw it waiting: the longest a full search is expected to take at idle priority on + // the largest machine. Past it, waiting is itself the urgent condition. It never makes a machine free. + Quiet = 12 * time.Hour +) // The reasons of a root verdict the search could not answer yet. const ( - // ReasonPending starts the reason while the first search since the engine started runs. + // ReasonPending starts the reason while no complete search judges: none has finished since the engine's + // state was kept, or the last one is older than FreshFor, and one runs. ReasonPending = "not judged yet (search running)" - // ReasonIncomplete starts the reason when no search has finished: one failed or ran out its bound. + // ReasonIncomplete starts the reason when no complete search judges and the last one failed: it is tried + // again after a back-off. ReasonIncomplete = "not judged (search incomplete)" ) From 2dbbda51c6588fef1175ab2ebeef96ac0d0c7fb8 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 01:00:47 +0200 Subject: [PATCH 2/3] Walk for setuid programs in the engine and carry an interrupted walk on (hq issue 361) The control-node's walk takes far longer than the engine runs between updates, so a search that starts over at every restart never completes. The walk is the engine's own, in lexical order, so its progress is one cursor per filesystem, kept every minute beside the engine's state and carried on after a restart while younger than rootsearch.FreshFor. It runs on a thread of its own at nice 19 and the idle I/O class. --- cmd/mesh-host/main.go | 8 +- internal/accounts/exec.go | 6 +- internal/accounts/idle_linux.go | 10 + internal/accounts/idle_other.go | 3 + internal/accounts/root_test.go | 27 ++- internal/accounts/setuid_cache.go | 241 ++++++++++++++++++------- internal/accounts/setuid_cache_test.go | 103 ++++++++++- internal/accounts/setuid_walk.go | 194 ++++++++++++++++++++ internal/accounts/setuid_walk_test.go | 184 +++++++++++++++++++ internal/accounts/ways.go | 59 ++---- internal/accounts/ways_test.go | 30 ++- rootsearch/rootsearch.go | 3 +- 12 files changed, 722 insertions(+), 146 deletions(-) create mode 100644 internal/accounts/setuid_walk.go create mode 100644 internal/accounts/setuid_walk_test.go diff --git a/cmd/mesh-host/main.go b/cmd/mesh-host/main.go index 9267b9b..d675fb0 100644 --- a/cmd/mesh-host/main.go +++ b/cmd/mesh-host/main.go @@ -1120,10 +1120,12 @@ func runLink(ctx context.Context, opts options) error { // And which units its service managers say failed, and whose each is (novox/hq issue 315). unitJudge = units.New(units.Exec{Run: apply.ExecRunner, System: builtFor}) // And whether an account a module put in a group has it where it runs (novox/hq ADR 0252). - // Its search for setuid programs runs to its end at idle priority, and the last complete one is kept - // beside this state, so a restart judges from it (novox/hq issue 361). + // Its search for setuid programs runs to its end on a thread at idle priority, keeping how far its walk + // got so a restart carries it on, and the last complete one beside this state, so a restart judges from + // it (novox/hq issue 361). setuidSearch = &accounts.SetuidCache{Path: filepath.Join(filepath.Dir(opts.state), accounts.SetuidSearchFile), - Say: say} + ProgressPath: filepath.Join(filepath.Dir(opts.state), accounts.SetuidProgressFile), + Lower: accounts.LowerThisThread, Say: say} accountJudge = accounts.New(accounts.Exec{Run: accounts.CLocale, SearchRun: accounts.Idle, Cache: setuidSearch}) } diff --git a/internal/accounts/exec.go b/internal/accounts/exec.go index 8260a30..b7d8889 100644 --- a/internal/accounts/exec.go +++ b/internal/accounts/exec.go @@ -34,9 +34,11 @@ type Exec struct { ACL func(path string) ([]ACLEntry, error) // Cache keeps the search for setuid programs between looks; nil searches on every look. Cache *SetuidCache - // SearchRun runs the setuid search's commands (find, and the package manager it asks); nil is Run. A - // serving engine gives Idle, so the walk of every filesystem yields to everything else on the machine. + // SearchRun runs the setuid search's commands (the package manager it asks); nil is Run. A + // serving engine gives Idle, so they yield to everything else on the machine, as the walk does. SearchRun Runner + // Walk carries a walk for setuid programs on; nil is the engine's own (walker). A test gives its own. + Walk func(ctx context.Context, p *SetuidProgress, checkpoint func()) error // Now is the clock the cache is kept by; nil is the machine's. Now func() time.Time } diff --git a/internal/accounts/idle_linux.go b/internal/accounts/idle_linux.go index 7ccfa38..785807b 100644 --- a/internal/accounts/idle_linux.go +++ b/internal/accounts/idle_linux.go @@ -66,3 +66,13 @@ func lower(pid int) error { } return nil } + +// LowerThisThread puts the calling goroutine on a thread of its own at nice 19 and the idle I/O class, for the +// walk for setuid programs, which runs in the engine (novox/hq issue 361): on Linux both are a thread's, so the +// engine's other work keeps its priority. The goroutine is locked to the thread and never unlocked, so the +// runtime ends the thread with it rather than hand a lowered thread to other work; a command it starts inherits +// the priority too. +func LowerThisThread() error { + runtime.LockOSThread() + return lower(unix.Gettid()) +} diff --git a/internal/accounts/idle_other.go b/internal/accounts/idle_other.go index 2dff47f..814bbfb 100644 --- a/internal/accounts/idle_other.go +++ b/internal/accounts/idle_other.go @@ -8,3 +8,6 @@ import "context" func Idle(ctx context.Context, name string, args ...string) (string, error) { return CLocale(ctx, name, args...) } + +// LowerThisThread is nothing where the machine offers no per-thread priority this engine sets. +func LowerThisThread() error { return nil } diff --git a/internal/accounts/root_test.go b/internal/accounts/root_test.go index a738238..72ae4ad 100644 --- a/internal/accounts/root_test.go +++ b/internal/accounts/root_test.go @@ -28,14 +28,14 @@ type agentMachine struct { failsID bool asked []string // texts are files' contents (doas's, polkit's); dirs a directory's files by name; acls a file's ACL; - // setuid what find prints, and packaged the paths a package owns. + // setuid what the walk finds, one per line, and packaged the paths a package owns. texts map[string]string dirs map[string][]string acls map[string][]ACLEntry setuid string findErr error packaged map[string]bool - // proc is /proc's files, aSafeProc unless a test changes them; findArgs sees find's arguments. + // proc is /proc's files, aSafeProc unless a test changes them; findArgs sees the filesystems walked. proc map[string]string findArgs func([]string) // pacmanCalls counts the package manager's calls: one for every path found. @@ -90,6 +90,20 @@ func (m *agentMachine) acl(path string) ([]ACLEntry, error) { return m.acls[path], nil } +// walk is the walk for setuid programs on this machine: m.setuid found, then m.findErr. +func (m *agentMachine) walk(_ context.Context, p *SetuidProgress, _ func()) error { + m.asked = append(m.asked, "walk "+strings.Join(p.Mounts, " ")) + if m.findArgs != nil { + m.findArgs(p.Mounts) + } + for _, l := range strings.Split(m.setuid, "\n") { + if l = strings.TrimSpace(l); l != "" { + p.Found = append(p.Found, l) + } + } + return m.findErr +} + func (m *agentMachine) run(_ context.Context, name string, args ...string) (string, error) { line := name + " " + strings.Join(args, " ") m.asked = append(m.asked, line) @@ -104,11 +118,6 @@ func (m *agentMachine) run(_ context.Context, name string, args ...string) (stri return m.gids + "\n", nil case line == "sudo -l -U agent": return m.sudo, m.sudoErr - case name == "find": - if m.findArgs != nil { - m.findArgs(args) - } - return m.setuid, m.findErr case name == "pacman" && len(args) >= 2 && args[0] == "-Qo": m.pacmanCalls++ var out, errs []string @@ -148,14 +157,14 @@ func clean() *agentMachine { func lookAgent(t *testing.T, m *agentMachine) Verdict { t.Helper() - j := New(Exec{Run: m.run, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) + j := New(Exec{Run: m.run, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) j.Set([]Account{agent}) st, _ := j.Look(t.Context()) if len(st.Accounts) != 1 { t.Fatalf("the statement: %+v", st) } for _, a := range m.asked { - if !strings.HasPrefix(a, "id ") && a != "sudo -l -U agent" && !strings.HasPrefix(a, "find / ") && + if !strings.HasPrefix(a, "id ") && a != "sudo -l -U agent" && !strings.HasPrefix(a, "walk /") && !strings.HasPrefix(a, "pacman -Qo ") { t.Errorf("the judge asked something that is not a read: %q", a) } diff --git a/internal/accounts/setuid_cache.go b/internal/accounts/setuid_cache.go index b85b648..9c29877 100644 --- a/internal/accounts/setuid_cache.go +++ b/internal/accounts/setuid_cache.go @@ -25,36 +25,44 @@ import ( // package manager answer for every path found, started less than FreshFor ago. Before the first completes, // or once the last is older, the answer is "not judged yet" (ReasonPending) while a search runs, and "not // judged" (ReasonIncomplete) after one failed. Never "none found" from a search that did not finish. -// - **It runs to completion**, once at a time, with no bound: a fresh one when the last complete one is Every -// old, when SearchAgain asks (an apply changed what root controls), or when a filesystem appeared that the -// last one did not walk. One that fails is tried again after a back-off that doubles from FailedBackoff up -// to MaxBackoff. +// - **It runs to completion**, once at a time, with no bound, on a thread of its own at idle priority (Lower): +// a fresh one when the last complete one is Every old, when SearchAgain asks (an apply changed what root +// controls), or when a filesystem appeared that the last one did not walk. One that fails is tried again +// after a back-off that doubles from FailedBackoff up to MaxBackoff. +// - **Resumable**: the walk's progress is kept in ProgressPath every CheckpointEvery, and a search started +// again — after an engine restart, an update, a reboot or a failure — carries on from it while it is +// younger than FreshFor. A search SearchAgain asked for never resumes an older walk: it starts from nothing. // - **Kept** in Path, beside the engine's other state, with when it started and finished: an engine restart // serves the last complete search rather than judging nothing until a new walk ends. // -// Why a complete search stays sound for a day and more is said in the rootsearch package: only root makes a -// setuid-root file, never the agent account. +// Why a complete search stays sound for a day and more, and a resumed walk is sound, is said in the rootsearch +// package and setuid_walk.go: only root makes a setuid-root file, never the agent account. type SetuidCache struct { // Every and FreshFor are rootsearch's when zero. Every, FreshFor time.Duration - // Path is the file the last complete search is kept in; empty keeps it in memory only. The engine's own - // state directory, which only root writes: a file there that is not a regular file root owns and alone - // may write is not read (keptSearch). - Path string - // Say is where a search's end and a kept search refused are said; nil says nothing. + // Path is the file the last complete search is kept in, and ProgressPath the walk under way; empty keeps + // either in memory only. The engine's own state directory, which only root writes: a file there that is + // not a regular file the engine's account owns and alone may write is not read (keptFile). + Path, ProgressPath string + // Lower is called on the search's own goroutine before it walks (LowerThisThread on a serving engine); a + // priority it cannot lower fails the search rather than walk the disks at full priority. Nil lowers nothing. + Lower func() error + // Say is where a search's end and a kept file refused are said; nil says nothing. Say func(string) - mu sync.Mutex - wg sync.WaitGroup - loaded bool - last *setuidResult // the last complete search - running bool - startedAt time.Time // when the search running, or the last one, was started - again bool // asked to search again since the running or last one started - failedAt time.Time - failed error - backoff time.Duration - starts int // how many searches were started, for a test + mu sync.Mutex + wg sync.WaitGroup + loaded bool + last *setuidResult // the last complete search + unfinished *SetuidProgress // a walk that did not finish, to resume + running bool + startedAt time.Time // when the search running, or the last one, was started (a resumed one: its start) + attemptAt time.Time // when the running or last search was started or resumed + again bool // asked to search again since the running or last one started + failedAt time.Time + failed error + backoff time.Duration + starts int // how many searches were started or resumed, for a test } // setuidResult is one complete search, as kept. @@ -77,8 +85,10 @@ const ( // clockSkew is how far in the future a kept search may say it started and still judge: a clock set back // further makes it no verdict, never a fresh one. clockSkew = 5 * time.Minute - // SetuidSearchFile is the file's name beside the engine's state. - SetuidSearchFile = "setuid-search.json" + // SetuidSearchFile and SetuidProgressFile are the files' names beside the engine's state: the last + // complete search, and the walk under way. + SetuidSearchFile = "setuid-search.json" + SetuidProgressFile = "setuid-search-progress.json" ) // SetuidNotJudged is the search's answer when no complete search judges: none has finished yet, the last one is @@ -137,25 +147,30 @@ func (c *SetuidCache) SearchAgain() { c.again = true } +// SetuidSearch is one search: it carries p on to its end — walking what p has not walked, calling checkpoint +// as it goes so its progress can be kept — and answers the setuid-root programs no package owns. +type SetuidSearch func(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) + // Look answers the setuid-root programs no package owns as the last complete search found them, and when it // started — while it is fresh — and starts a search in the background when one is due; it never waits for one. -// mounts are the filesystems a search walks now. Nil c searches in place, for a caller that wants to. +// mounts are the filesystems a new search walks. Nil c searches in place, for a caller that wants to. // // **One clock: the caller's.** A search's time is the `now` it was started at, never the wall clock read when it // finished: due-ness compares a later `now` with it, and mixing the two clocks made a search never due again // whenever the caller's clock ran behind the wall clock (found while checking issue 356). Measured from the // start, a result reads at most one search older than it is, which errs towards not judging. -func (c *SetuidCache) Look(now time.Time, mounts []string, - search func(ctx context.Context, mounts []string) ([]string, error)) ([]string, time.Time, error) { +func (c *SetuidCache) Look(now time.Time, mounts []string, search SetuidSearch) ([]string, time.Time, error) { if c == nil { - found, err := search(context.Background(), mounts) + p := &SetuidProgress{Started: now, Mounts: slices.Clone(mounts)} + found, err := search(context.Background(), p, func() {}) return found, now, err } c.mu.Lock() defer c.mu.Unlock() if !c.loaded { c.loaded = true - c.last = c.load() + c.last = c.loadResult() + c.unfinished = c.loadProgress() } var age time.Duration if c.last != nil { @@ -164,7 +179,7 @@ func (c *SetuidCache) Look(now time.Time, mounts []string, fresh := c.last != nil && age > -clockSkew && age < c.freshFor() asked := c.again || (c.last != nil && !covers(c.last.Mounts, mounts)) due := !c.running && (c.failed == nil || now.Sub(c.failedAt) >= c.backoff) && - (!fresh || age >= c.every() || (asked && (c.startedAt.IsZero() || now.Sub(c.startedAt) >= AgainAfter))) + (!fresh || age >= c.every() || (asked && (c.attemptAt.IsZero() || now.Sub(c.attemptAt) >= AgainAfter))) if due { c.start(now, mounts, search) } @@ -188,16 +203,60 @@ func (c *SetuidCache) lastStarted() time.Time { return c.last.Started } -// start runs one search in the background, with no bound: c.mu held. -func (c *SetuidCache) start(now time.Time, mounts []string, search func(context.Context, []string) ([]string, error)) { - c.running, c.startedAt, c.again = true, now, false +// resumable is the walk that did not finish, when a search may carry it on: younger than FreshFor, and no +// SearchAgain since — an apply that moved root's things wants a walk that starts after it. Anything else is +// thrown away. c.mu held. +func (c *SetuidCache) resumable(now time.Time) *SetuidProgress { + p := c.unfinished + c.unfinished = nil + if p == nil { + return nil + } + age := now.Sub(p.Started) + if c.again || age <= -clockSkew || age >= c.freshFor() { + c.forgetProgress() + return nil + } + return p +} + +// start runs one search in the background, with no bound: a walk that did not finish carried on, or a new one. +// c.mu held. +func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) { + p := c.resumable(now) + resumed := p != nil + if p == nil { + p = &SetuidProgress{ID: fmt.Sprintf("%x", now.UnixNano()), Started: now, Mounts: slices.Clone(mounts), + Cursor: map[string]string{}} + } + c.running, c.startedAt, c.attemptAt, c.again = true, p.Started, now, false c.starts++ - mounts = slices.Clone(mounts) + if resumed { + c.say(fmt.Sprintf("the search for setuid programs started %s carries on where it was (%d of %d "+ + "filesystems walked)", p.Started.Local().Format("2006-01-02 15:04"), len(p.Done), len(p.Mounts))) + } began := time.Now() c.wg.Add(1) go func() { defer c.wg.Done() - found, err := search(context.Background(), mounts) + var found []string + var err error + if c.Lower != nil { + // The goroutine stays on its lowered thread until it ends, and the thread ends with it. + if lerr := c.Lower(); lerr != nil { + err = fmt.Errorf("its priority could not be lowered, so it did not walk: %w", lerr) + } + } + if err == nil { + unkept := false + found, err = search(context.Background(), p, func() { + if kerr := c.keepProgress(p); kerr != nil && !unkept { + unkept = true + c.say("the search for setuid programs could not keep how far it got, and starts over if " + + "the engine stops: " + kerr.Error()) + } + }) + } took := time.Since(began) c.mu.Lock() defer c.mu.Unlock() @@ -205,18 +264,20 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search func(context. if err != nil { c.failed, c.failedAt = err, now c.backoff = min(max(2*c.backoff, FailedBackoff), MaxBackoff) - c.say(fmt.Sprintf("the search for setuid programs started %s did not complete after %s: %v", - now.Local().Format("15:04"), took.Round(time.Second), err)) + c.unfinished = p + c.say(fmt.Sprintf("the search for setuid programs started %s did not complete (%s this time): %v", + p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), err)) return } - r := &setuidResult{Started: now, Finished: now.Add(took), Mounts: mounts, Found: found} + r := &setuidResult{Started: p.Started, Finished: now.Add(took), Mounts: p.Mounts, Found: found} c.last, c.failed, c.backoff = r, nil, 0 - if err := c.keep(r); err != nil { + if err := c.keepResult(r); err != nil { c.say("the search for setuid programs could not be kept, and judges from memory until the engine " + "restarts: " + err.Error()) } - c.say(fmt.Sprintf("the search for setuid programs started %s completed in %s: %d no package owns", - now.Local().Format("15:04"), took.Round(time.Second), len(found))) + c.forgetProgress() + c.say(fmt.Sprintf("the search for setuid programs started %s completed (%s this time): %d no package "+ + "owns", p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), len(found))) }() } @@ -239,16 +300,31 @@ func covers(walked, now []string) bool { // Wait is for a test: until the search running, if any, has finished. func (c *SetuidCache) Wait() { c.wg.Wait() } -// keep writes a complete search to Path: a new file beside it, readable by root alone, renamed over it. -func (c *SetuidCache) keep(r *setuidResult) error { - if c.Path == "" { +func (c *SetuidCache) keepResult(r *setuidResult) error { return keepFile(c.Path, r) } + +func (c *SetuidCache) keepProgress(p *SetuidProgress) error { return keepFile(c.ProgressPath, p) } + +// forgetProgress removes the kept walk: finished, or thrown away. +func (c *SetuidCache) forgetProgress() { + if c.ProgressPath == "" { + return + } + if err := os.Remove(c.ProgressPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + c.say("the search for setuid programs could not forget the walk it kept: " + err.Error()) + } +} + +// keepFile writes v to path: a new file beside it, readable by its owner alone, renamed over it — a link at +// path is replaced, never followed. Empty path keeps nothing. +func keepFile(path string, v any) error { + if path == "" { return nil } - raw, err := json.Marshal(r) + raw, err := json.Marshal(v) if err != nil { return err } - f, err := os.CreateTemp(filepath.Dir(c.Path), "."+filepath.Base(c.Path)+".*") + f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".*") if err != nil { return err } @@ -264,25 +340,63 @@ func (c *SetuidCache) keep(r *setuidResult) error { if err := f.Close(); err != nil { return err } - return os.Rename(f.Name(), c.Path) + return os.Rename(f.Name(), path) } -// load reads the kept search, or nil: none kept, or one that cannot be believed, said why. -func (c *SetuidCache) load() *setuidResult { - if c.Path == "" { +// loadResult reads the kept complete search, or nil: none kept, or one that cannot be believed, said why. +func (c *SetuidCache) loadResult() *setuidResult { + var r setuidResult + if !c.loadFile(c.Path, "complete search", &r) { return nil } - r, err := keptSearch(c.Path) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - c.say("the kept search for setuid programs is not read, and a new one runs: " + err.Error()) + if r.Started.IsZero() || r.Finished.Before(r.Started) || len(r.Mounts) == 0 { + c.say("the kept search for setuid programs says no complete search, and a new one runs") + return nil } - return r + return &r } -// keptSearch reads a kept search, believing it only as a regular file, not followed through a link, owned by -// the engine's own account and writable by it alone, whose directory is the same: anything else could have been +// loadProgress reads the kept walk, or nil. +func (c *SetuidCache) loadProgress() *SetuidProgress { + var p SetuidProgress + if !c.loadFile(c.ProgressPath, "walk under way", &p) { + return nil + } + if p.Started.IsZero() || len(p.Mounts) == 0 { + c.say("the kept walk for setuid programs says no walk, and it starts over") + c.forgetProgress() + return nil + } + if p.Cursor == nil { + p.Cursor = map[string]string{} + } + return &p +} + +func (c *SetuidCache) loadFile(path, what string, v any) bool { + if path == "" { + return false + } + raw, err := keptFile(path) + if errors.Is(err, fs.ErrNotExist) { + return false + } + if err == nil { + if err = json.Unmarshal(raw, v); err != nil { + err = fmt.Errorf("%s is not JSON: %w", path, err) + } + } + if err != nil { + c.say(fmt.Sprintf("the kept %s for setuid programs is not read, and a new one runs: %v", what, err)) + return false + } + return true +} + +// keptFile reads a kept file, believing it only as a regular file, not followed through a link, owned by the +// engine's own account and writable by it alone, in a directory that is the same: anything else could have been // written by somebody other than root, and a search it says is no search. -func keptSearch(path string) (*setuidResult, error) { +func keptFile(path string) ([]byte, error) { if err := ownedAlone(filepath.Dir(path), true); err != nil { return nil, err } @@ -298,18 +412,7 @@ func keptSearch(path string) (*setuidResult, error) { if err := ownedAloneInfo(path, info, false); err != nil { return nil, err } - raw, err := io.ReadAll(io.LimitReader(f, 1<<20)) - if err != nil { - return nil, err - } - var r setuidResult - if err := json.Unmarshal(raw, &r); err != nil { - return nil, fmt.Errorf("%s is not a kept search: %w", path, err) - } - if r.Started.IsZero() || r.Finished.Before(r.Started) || len(r.Mounts) == 0 { - return nil, fmt.Errorf("%s says no complete search", path) - } - return &r, nil + return io.ReadAll(io.LimitReader(f, 64<<20)) } func ownedAlone(path string, dir bool) error { diff --git a/internal/accounts/setuid_cache_test.go b/internal/accounts/setuid_cache_test.go index c072b93..5d0a9be 100644 --- a/internal/accounts/setuid_cache_test.go +++ b/internal/accounts/setuid_cache_test.go @@ -2,6 +2,7 @@ package accounts import ( "context" + "encoding/json" "errors" "os" "os/exec" @@ -34,7 +35,7 @@ func newGate() *gate { return &gate{release: make(chan struct{}, 16), started: make(chan context.Context, 16)} } -func (g *gate) search(ctx context.Context, _ []string) ([]string, error) { +func (g *gate) search(ctx context.Context, p *SetuidProgress, _ func()) ([]string, error) { g.started <- ctx <-g.release return g.found, g.err @@ -290,3 +291,103 @@ func TestTheIdleRunnerLowersItsCommand(t *testing.T) { t.Fatalf("its failure is worded as CLocale's: %v", err) } } + +func mustJSON(t *testing.T, v any) []byte { + t.Helper() + raw, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + return raw +} + +func fromJSON(t *testing.T, raw []byte, v any) { + t.Helper() + if err := json.Unmarshal(raw, v); err != nil { + t.Fatal(err) + } +} + +// interrupted is a search that walks to a cursor, keeps it, and never returns: the engine stopped there. +func interrupted(seen chan<- *SetuidProgress) SetuidSearch { + return func(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { + p.Found = append(p.Found, "/opt/a") + p.Cursor["/"] = "/opt/a" + checkpoint() + seen <- p + select {} // the engine stops here + } +} + +func TestASearchInterruptedByARestartCarriesOnAndIsDatedByItsStart(t *testing.T) { + dir := t.TempDir() + paths := func() *SetuidCache { + return &SetuidCache{Path: filepath.Join(dir, SetuidSearchFile), ProgressPath: filepath.Join(dir, SetuidProgressFile)} + } + seen := make(chan *SetuidProgress, 1) + first := paths() + first.Look(at, root, interrupted(seen)) + <-seen + // The engine restarts two hours later. + restarted := paths() + var resumedWith SetuidProgress + done := make(chan struct{}) + _, _, err := restarted.Look(at.Add(2*time.Hour), root, func(ctx context.Context, p *SetuidProgress, _ func()) ([]string, error) { + resumedWith = *p + close(done) + return p.Found, nil + }) + notJudged(t, err, true) + <-done + restarted.Wait() + if !resumedWith.Started.Equal(at) || resumedWith.Cursor["/"] != "/opt/a" || len(resumedWith.Found) != 1 { + t.Fatalf("the restarted search did not carry on the walk: %+v", resumedWith) + } + found, when, err := restarted.Look(at.Add(2*time.Hour), root, newGate().search) + if err != nil || !when.Equal(at) || len(found) != 1 { + t.Fatalf("the complete search is dated by its start, not its resumption: %v %v %v", found, when, err) + } + if _, err := os.Stat(filepath.Join(dir, SetuidProgressFile)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the finished walk's progress is still kept: %v", err) + } +} + +func TestAWalkTooOldOrAskedAgainStartsOver(t *testing.T) { + for name, c := range map[string]struct { + after time.Duration + again bool + }{"older than it may judge": {rootsearch.FreshFor, false}, "an apply asked for a new one": {time.Hour, true}} { + dir := t.TempDir() + paths := func() *SetuidCache { + return &SetuidCache{Path: filepath.Join(dir, SetuidSearchFile), ProgressPath: filepath.Join(dir, SetuidProgressFile)} + } + seen := make(chan *SetuidProgress, 1) + paths().Look(at, root, interrupted(seen)) + <-seen + restarted := paths() + if c.again { + restarted.SearchAgain() + } + var with SetuidProgress + restarted.Look(at.Add(c.after), root, func(ctx context.Context, p *SetuidProgress, _ func()) ([]string, error) { + with = *p + return nil, nil + }) + restarted.Wait() + if !with.Started.Equal(at.Add(c.after)) || len(with.Cursor) != 0 || len(with.Found) != 0 { + t.Fatalf("%s: the old walk was carried on: %+v", name, with) + } + } +} + +func TestASearchWhosePriorityCannotBeLoweredDoesNotWalk(t *testing.T) { + c := &SetuidCache{Lower: func() error { return errors.New("not permitted") }} + walked := false + c.Look(at, root, func(context.Context, *SetuidProgress, func()) ([]string, error) { walked = true; return nil, nil }) + c.Wait() + _, _, err := c.Look(at.Add(time.Minute), root, newGate().search) + notJudged(t, err, false) + if walked { + t.Fatal("it walked at full priority") + } +} diff --git a/internal/accounts/setuid_walk.go b/internal/accounts/setuid_walk.go new file mode 100644 index 0000000..86dde8e --- /dev/null +++ b/internal/accounts/setuid_walk.go @@ -0,0 +1,194 @@ +package accounts + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "syscall" + "time" +) + +// The walk for setuid programs, resumable (novox/hq issue 361). +// +// The walk is the engine's own, in Go rather than find, so it can say how far it got: filepath.WalkDir visits +// every filesystem's tree depth first, each directory's entries in lexical order, so the walk's progress is one +// cursor per filesystem — the last path it visited — and everything before the cursor in that order is done. +// The progress is kept every CheckpointEvery beside the engine's state (SetuidCache), so an engine restarted, +// updated or rebooted midway carries on from its cursor rather than starting over: on the control-node the +// whole walk takes far longer than an engine runs between updates. +// +// **Why a resumed walk is sound.** Only root can make a setuid- or setgid-root file: the agent account can +// neither set the bit on root's file nor give its own file to root. A file that appears behind the cursor while +// the walk runs, or while it waits to be resumed, was made by root — and root's acts through the mesh start a +// new search (apply.MovesRoot), whose walk starts from nothing. A walk is dated by its start, and one resumed +// more than FreshFor after its start is thrown away and begun again. + +// SetuidProgress is one walk, as far as it got. +type SetuidProgress struct { + // ID names the walk; Started is when it began, on the cache's clock: the search is dated by it. + ID string `json:"id"` + Started time.Time `json:"started"` + // Mounts are the filesystems it walks, root first; Done those walked to their end. + Mounts []string `json:"mounts"` + Done []string `json:"done"` + // Cursor is, for a filesystem being walked, the last path visited. + Cursor map[string]string `json:"cursor"` + // Found is every setuid- or setgid-root regular file found so far, before the package manager is asked. + Found []string `json:"found"` +} + +// CheckpointEvery is how often a walk's progress is kept. +const CheckpointEvery = time.Minute + +// prunedPaths are never walked: the kernel's own trees, and the container runtimes' layers. +var prunedPaths = []string{"/proc", "/sys", "/var/lib/docker", "/var/lib/containers"} + +// walker walks for setuid programs. rootUID and rootGID are the owner and group a match has: root's (0), but +// for a test. +type walker struct { + rootUID, rootGID uint32 + every time.Duration +} + +// walk carries p on to its end, calling checkpoint every w.every and after each filesystem. It ends early only +// with an error — a directory it could not read, or ctx ended — which is an incomplete walk, never "none". +// A file or directory that vanished while it walked is not an error: what still exists was walked. +func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) error { + if p.Cursor == nil { + p.Cursor = map[string]string{} + } + last := time.Now() + for _, m := range p.Mounts { + if slices.Contains(p.Done, m) { + continue + } + if err := w.walkMount(ctx, p, m, checkpoint, &last); err != nil { + return err + } + p.Done = append(p.Done, m) + delete(p.Cursor, m) + checkpoint() + last = time.Now() + } + slices.Sort(p.Found) + p.Found = slices.Compact(p.Found) + return nil +} + +func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, checkpoint func(), last *time.Time) error { + top, err := os.Lstat(mount) + if err != nil { + return fmt.Errorf("the filesystem at %s could not be read: %w", mount, err) + } + dev, ok := deviceOf(top) + if !ok || !top.IsDir() { + return fmt.Errorf("the filesystem at %s is not a directory whose device could be read", mount) + } + var cursor []string + if c := p.Cursor[mount]; c != "" { + cursor = components(c) + } + return filepath.WalkDir(mount, func(path string, d fs.DirEntry, err error) error { + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil // vanished while it walked + } + return fmt.Errorf("the walk for setuid programs could not read %s: %w", path, err) + } + at := components(path) + if cursor != nil && walkOrder(at, cursor) < 0 && !isPrefix(at, cursor) { + // Before the cursor and not on its way: visited by the walk this resumes. + if d.IsDir() { + return filepath.SkipDir + } + return nil + } + switch { + case d.IsDir(): + if path != mount && slices.Contains(prunedPaths, path) { + return filepath.SkipDir + } + info, err := os.Lstat(path) + if errors.Is(err, fs.ErrNotExist) { + return filepath.SkipDir + } + if err != nil { + return fmt.Errorf("the walk for setuid programs could not read %s: %w", path, err) + } + if d, ok := deviceOf(info); !ok || d != dev { + return filepath.SkipDir // another filesystem, walked as its own or not at all (find's -xdev) + } + case d.Type().IsRegular(): + info, err := d.Info() // the entry's own, never through a link + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("the walk for setuid programs could not read %s: %w", path, err) + } + if w.setuidRoot(info) { + p.Found = append(p.Found, path) + } + } + // The cursor only moves on: the directories on the way back to it are visited again, and are before it. + if cursor == nil || walkOrder(at, cursor) > 0 { + p.Cursor[mount], cursor = path, at + } + if time.Since(*last) >= w.every { + checkpoint() + *last = time.Now() + } + return nil + }) +} + +// setuidRoot is a regular file setuid and owned by root, or setgid to root's group. +func (w walker) setuidRoot(info fs.FileInfo) bool { + st, ok := info.Sys().(*syscall.Stat_t) + if !ok || !info.Mode().IsRegular() { + return false + } + return uint32(st.Uid) == w.rootUID && info.Mode()&fs.ModeSetuid != 0 || + uint32(st.Gid) == w.rootGID && info.Mode()&fs.ModeSetgid != 0 +} + +func deviceOf(info fs.FileInfo) (uint64, bool) { + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return uint64(st.Dev), true +} + +// components are a path's names, in order; "/" has none. +func components(path string) []string { + path = strings.Trim(filepath.Clean(path), "/") + if path == "" { + return nil + } + return strings.Split(path, "/") +} + +// walkOrder compares two paths in the order WalkDir visits them: name by name, each in lexical order, a +// directory before what is in it. +func walkOrder(a, b []string) int { + for i := 0; i < len(a) && i < len(b); i++ { + if c := strings.Compare(a[i], b[i]); c != 0 { + return c + } + } + return len(a) - len(b) +} + +// isPrefix says a is b or a directory b is in. +func isPrefix(a, b []string) bool { + return len(a) <= len(b) && slices.Equal(a, b[:len(a)]) +} diff --git a/internal/accounts/setuid_walk_test.go b/internal/accounts/setuid_walk_test.go new file mode 100644 index 0000000..684a8b0 --- /dev/null +++ b/internal/accounts/setuid_walk_test.go @@ -0,0 +1,184 @@ +package accounts + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "sync" + "testing" + "time" +) + +// The engine's own walk for setuid programs (novox/hq issue 361): what it finds, what it never follows, and that +// a walk interrupted carries on from its cursor. + +// tree makes files under a directory of its own: a name ending in "/" a directory, "!" setuid, "%" setgid. +func tree(t *testing.T, names ...string) string { + t.Helper() + top := t.TempDir() + for _, n := range names { + mode := os.FileMode(0o755) + switch { + case strings.HasSuffix(n, "/"): + if err := os.MkdirAll(filepath.Join(top, n), 0o755); err != nil { + t.Fatal(err) + } + continue + case strings.HasSuffix(n, "!"): + n, mode = strings.TrimSuffix(n, "!"), mode|os.ModeSetuid + case strings.HasSuffix(n, "%"): + n, mode = strings.TrimSuffix(n, "%"), mode|os.ModeSetgid + } + p := filepath.Join(top, n) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, mode); err != nil { + t.Fatal(err) + } + } + return top +} + +// mine is a walker for which this test's own account is root. +func mine() walker { + return walker{rootUID: uint32(os.Getuid()), rootGID: uint32(os.Getgid()), every: time.Hour} +} + +func walkAll(t *testing.T, w walker, top string) []string { + t.Helper() + p := &SetuidProgress{Mounts: []string{top}} + if err := w.walk(t.Context(), p, func() {}); err != nil { + t.Fatal(err) + } + return rel(top, p.Found) +} + +func rel(top string, paths []string) []string { + var out []string + for _, p := range paths { + out = append(out, strings.TrimPrefix(p, top+"/")) + } + return out +} + +func TestTheWalkFindsSetuidAndSetgidRootFilesAndFollowsNoLink(t *testing.T) { + top := tree(t, "bin/plain", "bin/rootshell!", "sbin/group%", "deep/a/b/c/x!") + if err := os.Symlink(filepath.Join(top, "bin/rootshell"), filepath.Join(top, "link")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(top, "bin"), filepath.Join(top, "linkdir")); err != nil { + t.Fatal(err) + } + found := walkAll(t, mine(), top) + if strings.Join(found, ",") != "bin/rootshell,deep/a/b/c/x,sbin/group" { + t.Fatalf("found %v", found) + } + if got := walkAll(t, walker{rootUID: 4242424, rootGID: 4242424, every: time.Hour}, top); len(got) != 0 { + t.Fatalf("files nobody's root owns were found: %v", got) + } +} + +func TestAWalkThatCannotReadADirectoryIsIncomplete(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads every directory") + } + top := tree(t, "open/x!", "closed/y!") + if err := os.Chmod(filepath.Join(top, "closed"), 0); err != nil { + t.Fatal(err) + } + defer os.Chmod(filepath.Join(top, "closed"), 0o755) + p := &SetuidProgress{Mounts: []string{top}} + if err := mine().walk(t.Context(), p, func() {}); err == nil { + t.Fatal("a walk that could not read a directory was complete") + } +} + +func TestWalkOrderIsNameByName(t *testing.T) { + // "/a/b" is walked before "/a-c" although '-' sorts before '/': the walk goes name by name. + if walkOrder(components("/a/b"), components("/a-c")) >= 0 { + t.Fatal("/a/b is walked before /a-c") + } + if walkOrder(components("/a"), components("/a/b")) >= 0 || !isPrefix(components("/a"), components("/a/b")) { + t.Fatal("a directory is walked before what is in it") + } + if len(components("/")) != 0 { + t.Fatal("/ has no names") + } +} + +// A walk stopped midway carries on from its cursor: what was before the cursor is not walked again, what is after +// it is, and every file of the first walk is found. +func TestAnInterruptedWalkCarriesOnFromItsCursor(t *testing.T) { + top := tree(t, "a/1!", "a/2", "a-b/3!", "b/c/4!", "b/c/5", "b/d!", "c/6!", "d/7!") + w := mine() + w.every = 0 // a checkpoint at every path + ctx, stop := context.WithCancel(t.Context()) + p := &SetuidProgress{Mounts: []string{top}} + var kept []byte + visits := 0 + err := w.walk(ctx, p, func() { + visits++ + kept = mustJSON(t, p) + if strings.HasSuffix(p.Cursor[top], "/b/c/4") { + stop() // the engine stops here + } + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("the walk was stopped: %v", err) + } + var resumed SetuidProgress + fromJSON(t, kept, &resumed) + if !strings.HasSuffix(resumed.Cursor[top], "/b/c/4") { + t.Fatalf("the kept cursor is %q", resumed.Cursor[top]) + } + // Meanwhile, a file appears before the cursor and one after it. + for _, n := range []string{"a/late!", "e/late!"} { + tree2(t, top, n) + } + w.every = time.Hour + if err := w.walk(t.Context(), &resumed, func() {}); err != nil { + t.Fatal(err) + } + got := rel(top, resumed.Found) + want := []string{"a-b/3", "a/1", "b/c/4", "b/d", "c/6", "d/7", "e/late"} + if !slices.Equal(got, want) { + t.Fatalf("the resumed walk found %v, want %v: the first walk's finds kept, nothing before the cursor "+ + "walked again, everything after it walked", got, want) + } +} + +func tree2(t *testing.T, top, n string) { + t.Helper() + p := filepath.Join(top, strings.TrimSuffix(n, "!")) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(p, 0o755|os.ModeSetuid); err != nil { + t.Fatal(err) + } +} + +// A search's commands and walk run on a lowered thread while the rest of the engine keeps its priority. +func TestTheSearchRunsOnALoweredThreadOfItsOwn(t *testing.T) { + var wg sync.WaitGroup + wg.Add(1) + var err error + go func() { + defer wg.Done() + err = LowerThisThread() + }() + wg.Wait() + if err != nil { + t.Fatalf("the thread could not be lowered: %v", err) + } +} diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index 8d6c129..ef4c0e8 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -14,6 +14,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "sort" "strings" "time" @@ -378,51 +379,25 @@ func contains(xs []string, s string) bool { return false } -// setuidSearch is every setuid- or setgid-root regular file on the root filesystem that no installed package -// owns, found with find and asked of the package manager, through the search's runner (Exec.SearchRun: idle -// priority on a serving engine). **No bound of its own** (novox/hq issue 361): it runs until it has walked -// everything, and only a context its caller ends stops it — a search that is stopped, or whose walk find could -// not complete (it exited with an error), is an unanswered question, never "none", unless every error was a -// file that vanished while it walked. -func (e Exec) setuidSearch(ctx context.Context, mounts []string) ([]string, error) { +// setuidSearch is every setuid- or setgid-root regular file on the filesystems of p that no installed package +// owns: walked by the engine (setuid_walk.go, resumable from p), then asked of the package manager through the +// search's runner (Exec.SearchRun). **No bound of its own** (novox/hq issue 361): it runs until it has walked +// everything. A walk that could not complete — a directory it could not read — is an unanswered question, never +// "none"; a file that vanished while it walked is not. +func (e Exec) setuidSearch(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { + walk := e.Walk + if walk == nil { + walk = walker{every: CheckpointEvery}.walk + } + if err := walk(ctx, p, checkpoint); err != nil { + return nil, fmt.Errorf("the walk for setuid programs did not complete: %w", err) + } if e.SearchRun != nil { e.Run = e.SearchRun } - // Each mount point a starting point of its own, -xdev keeping each to its own filesystem: every local - // filesystem mounted without nosuid is searched once (the re-review of 2026-10-08), and a program setgid to - // root's group counts whoever owns it. - args := append(append([]string{}, mounts...), "-xdev", "(", "-path", "/proc", "-o", "-path", "/sys", "-o", - "-path", "/var/lib/docker", "-o", "-path", "/var/lib/containers", ")", "-prune", "-o", - "-type", "f", "(", "(", "-user", "root", "-perm", "-4000", ")", "-o", "(", "-group", "root", "-perm", - "-2000", ")", ")", "-print") - out, err := e.Run(ctx, "find", args...) - if ctx.Err() != nil { - return nil, fmt.Errorf("the search for setuid programs was stopped before it finished: %w", ctx.Err()) - } - if err != nil && (strings.TrimSpace(out) == "" || !onlyVanished(err)) { - return nil, fmt.Errorf("the search for setuid programs did not complete its walk: %w", err) - } - var paths []string - for _, l := range strings.Split(out, "\n") { - if l = strings.TrimSpace(l); l != "" { - paths = append(paths, l) - } - } - sort.Strings(paths) - return e.unpackaged(ctx, paths) -} - -// onlyVanished says every error find reported was a file that went while it walked — the one error that leaves -// the walk complete for what still exists. find's own words come after its exit, one per line. -func onlyVanished(err error) bool { - said := false - for _, l := range errorLines(err, "find") { - said = true - if !strings.Contains(l, "No such file or directory") { - return false - } - } - return said + found := slices.Clone(p.Found) + slices.Sort(found) + return e.unpackaged(ctx, slices.Compact(found)) } // errorLines are the lines a command said on its error stream, as the runner words a failure: " exited N: diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index 69d8d19..2196db5 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -67,7 +67,7 @@ func TestEachFurtherWayToRootIsSaid(t *testing.T) { m.proc["/proc/mounts"] += "/dev/sdb1 /srv xfs rw 0 0\n/dev/sdc1 /data ext4 rw,nosuid 0 0\n" m.setuid = "/srv/bin/rootshell\n" m.findArgs = func(args []string) { - if args[0] != "/" || args[1] != "/srv" || args[2] != "-xdev" { + if len(args) != 2 || args[0] != "/" || args[1] != "/srv" { panic(fmt.Sprintf("searched %v", args)) } } @@ -124,7 +124,7 @@ func TestAFurtherQuestionUnansweredIsUnknown(t *testing.T) { return "", fmt.Errorf("%s: %w", name, exec.ErrNotFound) } return m.run(ctx, name, args...) - }, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) + }, Walk: m.walk, Stat: m.stat, ReadFile: m.readFile, ReadDir: m.readDir, ACL: m.acl}) j.Set([]Account{agent}) if st, _ := j.Look(t.Context()); st.Accounts[0].State != Unknown { t.Fatalf("no package manager to ask: %+v", st.Accounts[0]) @@ -274,7 +274,10 @@ func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { m := clean() m.setuid = "/usr/bin/sudo\n/usr/bin/passwd\n/usr/local/bin/x\n/opt/y\n" m.packaged = map[string]bool{"/usr/bin/sudo": true, "/usr/bin/passwd": true} - found, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}) + search := func(e Exec) ([]string, error) { + return e.setuidSearch(t.Context(), &SetuidProgress{Mounts: []string{"/"}}, func() {}) + } + found, err := search(Exec{Run: m.run, Walk: m.walk}) if err != nil { t.Fatal(err) } @@ -291,23 +294,12 @@ func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { } return m.run(ctx, name, args...) } - if _, err := (Exec{Run: odd}).setuidSearch(t.Context(), []string{"/"}); err == nil { + if _, err := search(Exec{Run: odd, Walk: m.walk}); err == nil { t.Error("a package manager that could not answer was read as an answer") } -} - -// find that exits with an error has not walked everything: the search is incomplete, unless every error was a -// file that vanished while it walked. -func TestAWalkFindDidNotCompleteIsIncomplete(t *testing.T) { - m := clean() - m.setuid = "/usr/local/bin/x\n" - m.findErr = errors.New("find exited 1: find: '/mnt/data/private': Permission denied") - if _, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}); err == nil { - t.Fatal("a walk that could not enter a directory was read as complete") - } - m.findErr = errors.New("find exited 1: find: '/tmp/a': No such file or directory\nfind: '/tmp/b': No such file or directory") - found, err := (Exec{Run: m.run}).setuidSearch(t.Context(), []string{"/"}) - if err != nil || len(found) != 1 { - t.Fatalf("files that vanished while find walked: %v %v", found, err) + // A walk that did not complete is no answer either. + m.findErr = errors.New("could not read /mnt/data/private: permission denied") + if _, err := search(Exec{Run: m.run, Walk: m.walk}); err == nil { + t.Error("a walk that did not complete was read as an answer") } } diff --git a/rootsearch/rootsearch.go b/rootsearch/rootsearch.go index 7692529..bc56b05 100644 --- a/rootsearch/rootsearch.go +++ b/rootsearch/rootsearch.go @@ -13,7 +13,8 @@ // **And why no bound on one search.** On a machine with millions of files the walk takes longer than any bound // worth stating (the control-node's did not finish in fifteen minutes, issue 361), and a search ended early // judges nothing, so a bound made such a machine never judged. The search runs to its end at idle priority, -// once at a time; an incomplete search is never "free". +// once at a time, keeping how far its walk got every minute so an engine restarted midway carries it on; an +// incomplete search is never "free". A walk resumed more than FreshFor after it started is begun again. package rootsearch import "time" From a7270f00cbe010fb69fe58493334031e38429aab Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 01:10:56 +0200 Subject: [PATCH 3/3] Keep the walk's paths as bytes, forget a walk root superseded, and leave other mounts unread (hq issue 361 review) The review found a cursor of a name that is not UTF-8 came back changed from JSON, so a resumed walk skipped what it stood before; an ask to search again was lost to a restart, which carried the older walk on; a fresh search judged a filesystem it never walked; a mount point root cannot read failed or hung the walk; a found file gone before the package manager was asked failed every resume; and skipped or kept outcomes started a search on every reconcile, back to back with a long walk. --- internal/accounts/setuid_cache.go | 36 ++++++++-- internal/accounts/setuid_cache_test.go | 72 +++++++++++++++++--- internal/accounts/setuid_walk.go | 91 +++++++++++++++++++++++--- internal/accounts/setuid_walk_test.go | 29 ++++++++ internal/accounts/ways.go | 51 +++++++++++++-- internal/accounts/ways_test.go | 2 +- internal/apply/root_moved.go | 7 +- internal/apply/root_moved_test.go | 3 + 8 files changed, 259 insertions(+), 32 deletions(-) diff --git a/internal/accounts/setuid_cache.go b/internal/accounts/setuid_cache.go index 9c29877..3e189a0 100644 --- a/internal/accounts/setuid_cache.go +++ b/internal/accounts/setuid_cache.go @@ -58,6 +58,7 @@ type SetuidCache struct { running bool startedAt time.Time // when the search running, or the last one, was started (a resumed one: its start) attemptAt time.Time // when the running or last search was started or resumed + endedAt time.Time // when the last search ended, on the same clock again bool // asked to search again since the running or last one started failedAt time.Time failed error @@ -71,8 +72,8 @@ type setuidResult struct { Started time.Time `json:"started"` Finished time.Time `json:"finished"` // Mounts are the filesystems it walked; Found the setuid-root programs no package owns. - Mounts []string `json:"mounts"` - Found []string `json:"found"` + Mounts Paths `json:"mounts"` + Found Paths `json:"found"` } const ( @@ -145,6 +146,10 @@ func (c *SetuidCache) SearchAgain() { c.mu.Lock() defer c.mu.Unlock() c.again = true + // The walk under way, or kept, started before root acted: never carried on, even by an engine that + // restarts before the next search starts (the review of 2026-10-10). It keeps no progress from now on. + c.unfinished = nil + c.forgetProgress() } // SetuidSearch is one search: it carries p on to its end — walking what p has not walked, calling checkpoint @@ -176,10 +181,17 @@ func (c *SetuidCache) Look(now time.Time, mounts []string, search SetuidSearch) if c.last != nil { age = now.Sub(c.last.Started) } - fresh := c.last != nil && age > -clockSkew && age < c.freshFor() - asked := c.again || (c.last != nil && !covers(c.last.Mounts, mounts)) + // Fresh only while it walked every filesystem a search would walk now: one mounted since is not judged + // by a search that never saw it. + fresh := c.last != nil && age > -clockSkew && age < c.freshFor() && covers(c.last.Mounts, mounts) + // Asked for (an apply, a new filesystem): at most once an AgainAfter, counted from when the last search + // ended, so a walk of hours is not followed by another at once, apply after apply. + gap := now.Sub(c.attemptAt) + if !c.endedAt.IsZero() { + gap = now.Sub(c.endedAt) + } due := !c.running && (c.failed == nil || now.Sub(c.failedAt) >= c.backoff) && - (!fresh || age >= c.every() || (asked && (c.attemptAt.IsZero() || now.Sub(c.attemptAt) >= AgainAfter))) + (!fresh || age >= c.every() || (c.again && (c.attemptAt.IsZero() || gap >= AgainAfter))) if due { c.start(now, mounts, search) } @@ -250,6 +262,12 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) if err == nil { unkept := false found, err = search(context.Background(), p, func() { + c.mu.Lock() + superseded := c.again + c.mu.Unlock() + if superseded { + return // root acted since this walk started: its progress is not to be carried on + } if kerr := c.keepProgress(p); kerr != nil && !unkept { unkept = true c.say("the search for setuid programs could not keep how far it got, and starts over if " + @@ -261,10 +279,14 @@ func (c *SetuidCache) start(now time.Time, mounts []string, search SetuidSearch) c.mu.Lock() defer c.mu.Unlock() c.running = false + c.endedAt = now.Add(took) if err != nil { - c.failed, c.failedAt = err, now + // The back-off counts from when it failed. + c.failed, c.failedAt = err, c.endedAt c.backoff = min(max(2*c.backoff, FailedBackoff), MaxBackoff) - c.unfinished = p + if !c.again { // superseded by an ask to search again: never carried on + c.unfinished = p + } c.say(fmt.Sprintf("the search for setuid programs started %s did not complete (%s this time): %v", p.Started.Local().Format("2006-01-02 15:04"), took.Round(time.Second), err)) return diff --git a/internal/accounts/setuid_cache_test.go b/internal/accounts/setuid_cache_test.go index 5d0a9be..6cf980d 100644 --- a/internal/accounts/setuid_cache_test.go +++ b/internal/accounts/setuid_cache_test.go @@ -139,7 +139,7 @@ func TestAnIncompleteSearchIsNeverFree(t *testing.T) { if starts(c) != 1 { t.Fatalf("tried again within its back-off: %d", starts(c)) } - c.Look(at.Add(FailedBackoff), root, g.search) + c.Look(at.Add(FailedBackoff+time.Second), root, g.search) if starts(c) != 2 { t.Fatalf("not tried again after its back-off: %d", starts(c)) } @@ -168,7 +168,10 @@ func TestAnApplyThatMovedRootSearchesAgain(t *testing.T) { c.Look(at, root, g.search) g.finish(c) c.SearchAgain() - if _, _, err := c.Look(at.Add(AgainAfter), root, g.search); err != nil { + if _, _, err := c.Look(at.Add(time.Minute), root, g.search); err != nil || starts(c) != 1 { + t.Fatalf("asked again within AgainAfter of the last search's end: %v, %d searches", err, starts(c)) + } + if _, _, err := c.Look(at.Add(AgainAfter+time.Second), root, g.search); err != nil { t.Fatalf("the last complete search judges while the new one runs: %v", err) } if starts(c) != 2 { @@ -180,20 +183,71 @@ func TestAnApplyThatMovedRootSearchesAgain(t *testing.T) { if starts(c) != 2 { t.Fatalf("a second search started beside the running one: %d", starts(c)) } - g.finish(c) - c.Look(at.Add(2*AgainAfter+time.Minute), root, g.search) + g.finish(c) // ends, on the cache's clock, at about AgainAfter+1s + c.Look(at.Add(2*AgainAfter), root, g.search) + if starts(c) != 2 { + t.Fatal("a search started again less than AgainAfter after the last one ended") + } + c.Look(at.Add(2*AgainAfter+2*time.Second), root, g.search) if starts(c) != 3 { t.Fatalf("the ask made while a search ran was dropped: %d", starts(c)) } g.finish(c) - // A filesystem the last search did not walk asks for one too. - c.Look(at.Add(4*AgainAfter), []string{"/", "/srv"}, g.search) - if starts(c) != 4 { + var nilCache *SetuidCache + nilCache.SearchAgain() +} + +// A filesystem the last complete search did not walk is not judged by it: not judged yet, and a search starts. +func TestAFilesystemTheSearchDidNotWalkIsNotJudgedByIt(t *testing.T) { + c := &SetuidCache{} + g := newGate() + c.Look(at, root, g.search) + g.finish(c) + _, _, err := c.Look(at.Add(time.Hour), []string{"/", "/srv"}, g.search) + notJudged(t, err, true) + if starts(c) != 2 { t.Fatalf("a new filesystem was not searched: %d", starts(c)) } g.finish(c) - var nilCache *SetuidCache - nilCache.SearchAgain() + if _, _, err := c.Look(at.Add(time.Hour), []string{"/", "/srv"}, g.search); err != nil { + t.Fatalf("once walked it judges: %v", err) + } +} + +// An ask to search again forgets the walk under way, so an engine restarted before the next search does not +// carry on a walk that started before root acted. +func TestAnAskToSearchAgainIsNotLostToARestart(t *testing.T) { + dir := t.TempDir() + paths := func() *SetuidCache { + return &SetuidCache{Path: filepath.Join(dir, SetuidSearchFile), ProgressPath: filepath.Join(dir, SetuidProgressFile)} + } + seen := make(chan *SetuidProgress, 1) + first := paths() + first.Look(at, root, interrupted(seen)) + <-seen + first.SearchAgain() // an apply moved root's things; then the engine restarts + restarted := paths() + var with SetuidProgress + restarted.Look(at.Add(time.Hour), root, func(ctx context.Context, p *SetuidProgress, _ func()) ([]string, error) { + with = *p + return nil, nil + }) + restarted.Wait() + if !with.Started.Equal(at.Add(time.Hour)) || len(with.Found) != 0 { + t.Fatalf("the walk from before the apply was carried on: %+v", with) + } +} + +// A cursor whose name is not UTF-8 is kept as its bytes: JSON strings would change it, and the resumed walk +// would skip what it stood before. +func TestACursorOfAnyBytesIsKeptExactly(t *testing.T) { + p := &SetuidProgress{Started: at, Mounts: Paths{"/"}, Cursor: Cursors{"/": "/home/agent/\x80/x"}, + Found: Paths{"/opt/\xff"}} + var back SetuidProgress + fromJSON(t, mustJSON(t, p), &back) + if back.Cursor["/"] != "/home/agent/\x80/x" || back.Found[0] != "/opt/\xff" { + t.Fatalf("kept as %q, %q", back.Cursor["/"], back.Found[0]) + } } func TestTheCompleteSearchIsKeptAndReadAfterARestart(t *testing.T) { diff --git a/internal/accounts/setuid_walk.go b/internal/accounts/setuid_walk.go index 86dde8e..1df3031 100644 --- a/internal/accounts/setuid_walk.go +++ b/internal/accounts/setuid_walk.go @@ -2,6 +2,7 @@ package accounts import ( "context" + "encoding/json" "errors" "fmt" "io/fs" @@ -34,12 +35,66 @@ type SetuidProgress struct { ID string `json:"id"` Started time.Time `json:"started"` // Mounts are the filesystems it walks, root first; Done those walked to their end. - Mounts []string `json:"mounts"` - Done []string `json:"done"` + Mounts Paths `json:"mounts"` + Done Paths `json:"done"` // Cursor is, for a filesystem being walked, the last path visited. - Cursor map[string]string `json:"cursor"` + Cursor Cursors `json:"cursor"` // Found is every setuid- or setgid-root regular file found so far, before the package manager is asked. - Found []string `json:"found"` + Found Paths `json:"found"` +} + +// Paths are paths kept as their bytes. **Never as JSON strings**: encoding/json replaces bytes that are not +// UTF-8 with U+FFFD, and a cursor so changed sorts after the names it stood before — a resumed walk would skip +// them unread (the review of 2026-10-10). A name of any bytes is any account's to make. +type Paths []string + +func (ps Paths) MarshalJSON() ([]byte, error) { + raw := make([][]byte, len(ps)) + for i, p := range ps { + raw[i] = []byte(p) + } + return json.Marshal(raw) +} + +func (ps *Paths) UnmarshalJSON(b []byte) error { + var raw [][]byte + if err := json.Unmarshal(b, &raw); err != nil { + return err + } + *ps = make(Paths, len(raw)) + for i, r := range raw { + (*ps)[i] = string(r) + } + return nil +} + +// Cursors are each filesystem's cursor, kept as bytes as Paths are. +type Cursors map[string]string + +type keptCursor struct { + Mount []byte `json:"mount"` + Path []byte `json:"path"` +} + +func (cs Cursors) MarshalJSON() ([]byte, error) { + raw := []keptCursor{} + for m, p := range cs { + raw = append(raw, keptCursor{Mount: []byte(m), Path: []byte(p)}) + } + slices.SortFunc(raw, func(a, b keptCursor) int { return strings.Compare(string(a.Mount), string(b.Mount)) }) + return json.Marshal(raw) +} + +func (cs *Cursors) UnmarshalJSON(b []byte) error { + var raw []keptCursor + if err := json.Unmarshal(b, &raw); err != nil { + return err + } + *cs = Cursors{} + for _, r := range raw { + (*cs)[string(r.Mount)] = string(r.Path) + } + return nil } // CheckpointEvery is how often a walk's progress is kept. @@ -49,10 +104,13 @@ const CheckpointEvery = time.Minute var prunedPaths = []string{"/proc", "/sys", "/var/lib/docker", "/var/lib/containers"} // walker walks for setuid programs. rootUID and rootGID are the owner and group a match has: root's (0), but -// for a test. +// for a test. mountPoints are every filesystem mounted on the machine (MountPoints): a directory that is one is +// another filesystem, left without being read — a FUSE mount root cannot read, or a network mount whose +// server is away, would fail or hang the walk (the review of 2026-10-10). type walker struct { rootUID, rootGID uint32 every time.Duration + mountPoints map[string]bool } // walk carries p on to its end, calling checkpoint every w.every and after each filesystem. It ends early only @@ -60,7 +118,7 @@ type walker struct { // A file or directory that vanished while it walked is not an error: what still exists was walked. func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) error { if p.Cursor == nil { - p.Cursor = map[string]string{} + p.Cursor = Cursors{} } last := time.Now() for _, m := range p.Mounts { @@ -75,8 +133,23 @@ func (w walker) walk(ctx context.Context, p *SetuidProgress, checkpoint func()) checkpoint() last = time.Now() } - slices.Sort(p.Found) - p.Found = slices.Compact(p.Found) + // Looked at again: a file found that is gone, or no longer setuid-root, is no answer the package manager + // can give, and would fail every search that resumes this walk (the review of 2026-10-10). + var still Paths + for _, f := range p.Found { + info, err := os.Lstat(f) + if errors.Is(err, fs.ErrNotExist) { + continue + } + if err != nil { + return fmt.Errorf("the walk for setuid programs could not read %s again: %w", f, err) + } + if w.setuidRoot(info) { + still = append(still, f) + } + } + slices.Sort(still) + p.Found = slices.Compact(still) return nil } @@ -113,7 +186,7 @@ func (w walker) walkMount(ctx context.Context, p *SetuidProgress, mount string, } switch { case d.IsDir(): - if path != mount && slices.Contains(prunedPaths, path) { + if path != mount && (slices.Contains(prunedPaths, path) || w.mountPoints[path]) { return filepath.SkipDir } info, err := os.Lstat(path) diff --git a/internal/accounts/setuid_walk_test.go b/internal/accounts/setuid_walk_test.go index 684a8b0..4d6a636 100644 --- a/internal/accounts/setuid_walk_test.go +++ b/internal/accounts/setuid_walk_test.go @@ -182,3 +182,32 @@ func TestTheSearchRunsOnALoweredThreadOfItsOwn(t *testing.T) { t.Fatalf("the thread could not be lowered: %v", err) } } + +// Another filesystem's mount point is left without being read, and a file found that is gone, or no longer +// setuid-root, when the walk ends is not asked about. +func TestTheWalkLeavesMountPointsAndDropsWhatWentMeanwhile(t *testing.T) { + top := tree(t, "fuse/secret!", "bin/x!", "bin/y!") + w := mine() + w.mountPoints = map[string]bool{filepath.Join(top, "fuse"): true} + w.every = 0 + p := &SetuidProgress{Mounts: []string{top}} + removed := false + if err := w.walk(t.Context(), p, func() { + if !removed && strings.HasSuffix(p.Cursor[top], "/bin/y") { + removed = true + os.Remove(filepath.Join(top, "bin/x")) + } + }); err != nil { + t.Fatal(err) + } + if got := rel(top, p.Found); strings.Join(got, ",") != "bin/y" { + t.Fatalf("found %v", got) + } +} + +func TestAMountPointIsReadAsTheKernelEscapesIt(t *testing.T) { + mp := MountPoints("/dev/a /mnt/a\\040b ext4 rw 0 0\n/dev/b /mnt/back\\134slash ext4 rw 0 0\n") + if !mp["/mnt/a b"] || !mp[`/mnt/back\slash`] { + t.Fatalf("read %v", mp) + } +} diff --git a/internal/accounts/ways.go b/internal/accounts/ways.go index ef4c0e8..100770a 100644 --- a/internal/accounts/ways.go +++ b/internal/accounts/ways.go @@ -355,7 +355,7 @@ func SuidMounts(text string) []string { if len(f) < 4 || pseudoFS[f[2]] || strings.HasPrefix(f[2], "fuse") { continue } - at := strings.ReplaceAll(strings.ReplaceAll(f[1], `\040`, " "), `\011`, "\t") + at := unescapeMount(f[1]) if strings.HasPrefix(at, "/var/lib/docker") || strings.HasPrefix(at, "/var/lib/containers") || strings.HasPrefix(at, "/proc") || strings.HasPrefix(at, "/sys") { continue @@ -370,6 +370,45 @@ func SuidMounts(text string) []string { return out } +// MountPoints is every mount point /proc/mounts lists, of any type: the walk leaves each it meets inside +// another filesystem without reading it. +func MountPoints(text string) map[string]bool { + out := map[string]bool{} + for _, line := range strings.Split(text, "\n") { + if f := strings.Fields(line); len(f) >= 2 { + out[unescapeMount(f[1])] = true + } + } + return out +} + +// unescapeMount is a mount point as /proc/mounts escapes it — a space, a tab, a newline and a backslash as +// three octal digits after a backslash — made a path again. +func unescapeMount(s string) string { + var b strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '\\' && i+3 < len(s) && isOctal(s[i+1:i+4]) { + b.WriteByte((s[i+1]-'0')<<6 | (s[i+2]-'0')<<3 | (s[i+3] - '0')) + i += 3 + continue + } + b.WriteByte(s[i]) + } + return b.String() +} + +func isOctal(s string) bool { + if len(s) != 3 { + return false + } + for i := 0; i < 3; i++ { + if s[i] < '0' || s[i] > '7' { + return false + } + } + return true +} + func contains(xs []string, s string) bool { for _, x := range xs { if x == s { @@ -384,10 +423,10 @@ func contains(xs []string, s string) bool { // search's runner (Exec.SearchRun). **No bound of its own** (novox/hq issue 361): it runs until it has walked // everything. A walk that could not complete — a directory it could not read — is an unanswered question, never // "none"; a file that vanished while it walked is not. -func (e Exec) setuidSearch(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { +func (e Exec) setuidSearch(ctx context.Context, p *SetuidProgress, checkpoint func(), mountPoints map[string]bool) ([]string, error) { walk := e.Walk if walk == nil { - walk = walker{every: CheckpointEvery}.walk + walk = walker{every: CheckpointEvery, mountPoints: mountPoints}.walk } if err := walk(ctx, p, checkpoint); err != nil { return nil, fmt.Errorf("the walk for setuid programs did not complete: %w", err) @@ -592,7 +631,11 @@ func (e Exec) moreWays(ctx context.Context, account string, uid int, groups []st } // In the background, the last complete result served while it is fresh (SetuidCache): never a stall. No // fresh complete result is said beside the other ways, which are judged all the same. - unowned, searchedAt, searchErr := e.Cache.Look(now(), mounts, e.setuidSearch) + mountPoints := MountPoints(string(mountsText)) + unowned, searchedAt, searchErr := e.Cache.Look(now(), mounts, + func(ctx context.Context, p *SetuidProgress, checkpoint func()) ([]string, error) { + return e.setuidSearch(ctx, p, checkpoint, mountPoints) + }) var notJudged *SetuidNotJudged if searchErr != nil && !errors.As(searchErr, ¬Judged) { return nil, searchErr diff --git a/internal/accounts/ways_test.go b/internal/accounts/ways_test.go index 2196db5..c8be804 100644 --- a/internal/accounts/ways_test.go +++ b/internal/accounts/ways_test.go @@ -275,7 +275,7 @@ func TestThePackageManagerIsAskedOnceForEveryPathFound(t *testing.T) { m.setuid = "/usr/bin/sudo\n/usr/bin/passwd\n/usr/local/bin/x\n/opt/y\n" m.packaged = map[string]bool{"/usr/bin/sudo": true, "/usr/bin/passwd": true} search := func(e Exec) ([]string, error) { - return e.setuidSearch(t.Context(), &SetuidProgress{Mounts: []string{"/"}}, func() {}) + return e.setuidSearch(t.Context(), &SetuidProgress{Mounts: []string{"/"}}, func() {}, nil) } found, err := search(Exec{Run: m.run, Walk: m.walk}) if err != nil { diff --git a/internal/apply/root_moved.go b/internal/apply/root_moved.go index af9bca2..b246c0a 100644 --- a/internal/apply/root_moved.go +++ b/internal/apply/root_moved.go @@ -16,11 +16,14 @@ var rootPaths = []string{"/etc/passwd", "/etc/shadow", "/etc/group", "/etc/gshad // MovesRoot says an apply changed something through which root's reach could have changed — an account, a // package, an action run as root, or a file in rootPaths (novox/hq issue 361). The engine then asks for a new // search for setuid programs: root acted, and a complete search is sound only until root acts. Outcomes that -// moved nothing (unchanged, held, waiting, held still) do not count. +// moved nothing (unchanged, held, waiting, held still, skipped, kept, forgotten) do not count. func MovesRoot(outcomes []Outcome) bool { for _, o := range outcomes { + // Only what moved the machine: skipped, kept and forgotten touch nothing, and counting them made one + // broken module start a search on every reconcile (the review of 2026-10-10). switch o.Action { - case "unchanged", "held", "waiting", heldStill: + case "created", "updated", "corrected", "removed": + default: continue } switch declaration.Type(o.Type) { diff --git a/internal/apply/root_moved_test.go b/internal/apply/root_moved_test.go index 683f088..f099087 100644 --- a/internal/apply/root_moved_test.go +++ b/internal/apply/root_moved_test.go @@ -21,6 +21,9 @@ func TestAnApplyMovesRootThroughAnAccountAPackageAnActionOrSudosRules(t *testing {Outcome{Type: "file", Target: "/etc/nginx/nginx.conf", Action: "updated"}, false}, {Outcome{Type: "container", Target: "plex", Action: "updated"}, false}, {Outcome{Type: "package", Target: "x", Action: heldStill}, false}, + {Outcome{Type: "action", Target: "setup", Action: "skipped"}, false}, + {Outcome{Type: "user", Target: "nox", Action: "forgotten"}, false}, + {Outcome{Type: "package", Target: "x", Action: "kept"}, false}, } { if got := MovesRoot([]Outcome{c.o}); got != c.want { t.Errorf("%+v: moves root %v, want %v", c.o, got, c.want)