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"