From c433e76de2295d52863289228e030f1829c0a991 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 12:53:42 +0200 Subject: [PATCH] Judge a granted group by number, grant nothing a group cannot write, and keep the look bounded (hq ADR 0312, issue 537 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine is built without cgo, so a check by name missed a root group a name service holds or one number carrying another name: the root groups are resolved to numbers (now with kmem and shadow) and a number named nothing is refused for containers too. A group without write is granted nothing (§2). The look reads each path off the lock within five seconds, so a dead network mount cannot stop every health statement. A grant's account may not start with a dash. A group a record leaves for another asker is handed to it, so it is still taken back when the last asker goes (§5). The scheduler skips paths issue 339's guard refuses. --- cmd/mesh-host/grants.go | 72 ++++++++++-- cmd/mesh-host/grants_test.go | 62 ++++++++++- internal/accounts/accounts.go | 8 +- internal/apply/apply.go | 14 ++- internal/apply/grants.go | 167 +++++++++++++++++++++++----- internal/apply/grants_test.go | 114 ++++++++++++++++++- internal/apply/groups.go | 13 ++- internal/apply/user.go | 8 +- internal/declaration/declaration.go | 12 +- 9 files changed, 409 insertions(+), 61 deletions(-) diff --git a/cmd/mesh-host/grants.go b/cmd/mesh-host/grants.go index 0a275bc..892ca57 100644 --- a/cmd/mesh-host/grants.go +++ b/cmd/mesh-host/grants.go @@ -34,10 +34,18 @@ type accessJudge struct { accesses []grantedAccess // was is each access's last verdict, with since when and how many looks in a row found it unhealthy. was map[string]link.ResourceHealth + + // reading guards inFlight: every path whose read has not returned yet. + reading sync.Mutex + inFlight map[string]bool } // accesses is the one access judge on this machine. -var accesses = &accessJudge{was: map[string]link.ResourceHealth{}} +var accesses = newAccessJudge() + +func newAccessJudge() *accessJudge { + return &accessJudge{was: map[string]link.ResourceHealth{}, inFlight: map[string]bool{}} +} // set is what the declaration just applied asks: every read-write access of a module that names a grant, not // held as found. An access no longer declared is forgotten. @@ -71,17 +79,58 @@ func (j *accessJudge) set(d *declaration.Declaration, held map[string]bool) { } } +// lookBound is how long a look waits for one accessed path to answer. A hard network mount whose server is down +// blocks a stat for ever, and one access must never stop every health statement this machine makes. +var lookBound = 5 * time.Second + +// pathGroupBounded reads a path's group within lookBound, outside any lock. A read that has not answered is said +// so; while it still hangs, later looks do not start another on the same path, so a dead mount costs one blocked +// read, not one per look. +func (j *accessJudge) pathGroupBounded(path string) (apply.PathGroup, bool, error) { + j.reading.Lock() + if j.inFlight[path] { + j.reading.Unlock() + return apply.PathGroup{}, false, nil + } + j.inFlight[path] = true + j.reading.Unlock() + type read struct { + g apply.PathGroup + err error + } + done := make(chan read, 1) + go func() { + g, err := apply.GroupOfPath(path) + j.reading.Lock() + delete(j.inFlight, path) + j.reading.Unlock() + done <- read{g, err} + }() + select { + case r := <-done: + return r.g, true, r.err + case <-time.After(lookBound): + return apply.PathGroup{}, false, nil + } +} + // look reads every access now: healthy when its group may write and may be granted, unhealthy saying why -// otherwise. Whether any verdict changed since the last look, for the loop to say it. +// otherwise. Whether any verdict changed since the last look, for the loop to say it. The paths are read with no +// lock held, each within lookBound. func (j *accessJudge) look(now time.Time) ([]link.ResourceHealth, bool) { j.mu.Lock() - defer j.mu.Unlock() - changed := false - out := make([]link.ResourceHealth, 0, len(j.accesses)) - for _, a := range j.accesses { + as := append([]grantedAccess(nil), j.accesses...) + j.mu.Unlock() + + type verdict struct{ state, reason string } + read := make([]verdict, len(as)) + for i, a := range as { state, reason := link.StateHealthy, "" - g, err := apply.GroupOfPath(a.path) + g, answered, err := j.pathGroupBounded(a.path) switch { + case !answered: + state, reason = link.StateUnhealthy, "its path does not answer: reading which group owns it did not "+ + "return within "+lookBound.String() case err != nil: state, reason = link.StateUnhealthy, "its group cannot be read: "+strings.ReplaceAll(err.Error(), a.path, "the path") case apply.GrantRefusal(g) != nil: @@ -89,6 +138,15 @@ func (j *accessJudge) look(now time.Time) ([]link.ResourceHealth, bool) { case !g.Writable: state, reason = link.StateUnhealthy, apply.CannotWriteWords(g) } + read[i] = verdict{state, reason} + } + + j.mu.Lock() + defer j.mu.Unlock() + changed := false + out := make([]link.ResourceHealth, 0, len(as)) + for i, a := range as { + state, reason := read[i].state, read[i].reason v := link.ResourceHealth{Module: a.module, Resource: a.id, Kind: link.KindAccess, Target: a.path, State: state, Reason: reason, Since: now.UTC()} if was, had := j.was[a.id]; had && was.State == state && was.Reason == reason { diff --git a/cmd/mesh-host/grants_test.go b/cmd/mesh-host/grants_test.go index 51510de..27f1227 100644 --- a/cmd/mesh-host/grants_test.go +++ b/cmd/mesh-host/grants_test.go @@ -2,6 +2,7 @@ package main import ( "strings" + "sync/atomic" "testing" "time" @@ -23,6 +24,9 @@ func TestAnAccessWhoseGroupCannotWriteIsStatedUnhealthyWithTheOperatorsRemedy(t was := apply.GroupOfPath apply.GroupOfPath = func(path string) (apply.PathGroup, error) { return groups[path], nil } t.Cleanup(func() { apply.GroupOfPath = was }) + wasRoot := apply.RootGroupIDs + apply.RootGroupIDs = func() map[int]string { return map[int]string{0: "root"} } + t.Cleanup(func() { apply.RootGroupIDs = wasRoot }) d, err := declaration.Parse([]byte(`{"declaration":1,"resources":[ {"id":"nzbget.downloads","type":"access","path":"` + spool + `","mode":"read-write","grant":{"account":"operator"}}, @@ -32,7 +36,7 @@ func TestAnAccessWhoseGroupCannotWriteIsStatedUnhealthyWithTheOperatorsRemedy(t if err != nil { t.Fatal(err) } - j := &accessJudge{was: map[string]link.ResourceHealth{}} + j := newAccessJudge() j.set(d, nil) at := time.Date(2026, 10, 12, 12, 0, 0, 0, time.UTC) vs, changed := j.look(at) @@ -76,3 +80,59 @@ func TestAnAccessWhoseGroupCannotWriteIsStatedUnhealthyWithTheOperatorsRemedy(t } } } + +// A path that does not answer — a hard network mount whose server is down — is stated unhealthy within the bound, +// and stops neither this look nor the next: one blocked read per path, never one per look. +func TestAnAccessWhosePathDoesNotAnswerIsStatedWithinTheBound(t *testing.T) { + hung, library := t.TempDir(), t.TempDir() + release, finished := make(chan struct{}), make(chan struct{}) + var started atomic.Int32 + was, wasBound := apply.GroupOfPath, lookBound + apply.GroupOfPath = func(path string) (apply.PathGroup, error) { + if path == hung { + started.Add(1) + <-release + defer close(finished) + } + return apply.PathGroup{GID: 1001, Name: "media", Writable: true}, nil + } + lookBound = 50 * time.Millisecond + wasRoot := apply.RootGroupIDs + apply.RootGroupIDs = func() map[int]string { return map[int]string{0: "root"} } + t.Cleanup(func() { apply.GroupOfPath, lookBound, apply.RootGroupIDs = was, wasBound, wasRoot }) + // Run first: the read that hangs is let go and has returned before anything it read is put back. + t.Cleanup(func() { close(release); <-finished }) + + d, err := declaration.Parse([]byte(`{"declaration":1,"resources":[ + {"id":"nzbget.downloads","type":"access","path":"` + hung + `","mode":"read-write","grant":{}}, + {"id":"importer.music","type":"access","path":"` + library + `","mode":"read-write","grant":{}}]}`)) + if err != nil { + t.Fatal(err) + } + j := newAccessJudge() + j.set(d, nil) + for look := 0; look < 3; look++ { + done := make(chan []link.ResourceHealth, 1) + go func() { vs, _ := j.look(time.Now()); done <- vs }() + select { + case vs := <-done: + for _, v := range vs { + switch v.Resource { + case "nzbget.downloads": + if v.State != link.StateUnhealthy || !strings.Contains(v.Reason, "does not answer") { + t.Errorf("look %d: a path that does not answer is stated %+v", look, v) + } + case "importer.music": + if v.State != link.StateHealthy { + t.Errorf("look %d: the path that answers is stated %+v", look, v) + } + } + } + case <-time.After(5 * time.Second): + t.Fatalf("look %d did not return while one path hangs", look) + } + } + if n := started.Load(); n != 1 { + t.Errorf("%d reads were started on the path that hangs; want one", n) + } +} diff --git a/internal/accounts/accounts.go b/internal/accounts/accounts.go index f72169e..1316527 100644 --- a/internal/accounts/accounts.go +++ b/internal/accounts/accounts.go @@ -65,9 +65,11 @@ const ReasonRoot = "can become root without a person" // RootGroups are the groups whose members become root by membership alone, without a sudo rule naming // them: the administrators' groups a distribution's own rules or polkit treat as root, the container // runtime's socket (a container with the host's root mounted), the raw disks, and the virtualisation -// daemons that start a guest with the host's devices. A closed list, so what the judge calls "grants root" -// is written down and reviewable rather than guessed per machine. -var RootGroups = []string{"root", "wheel", "sudo", "admin", "docker", "disk", "lxd", "incus-admin", "libvirt"} +// daemons that start a guest with the host's devices — and the kernel's memory (kmem) and the password hashes +// (shadow), either of which is root a step away (novox/hq ADR 0312 §4 review). A closed list, so what the judge +// calls "grants root" is written down and reviewable rather than guessed per machine. +var RootGroups = []string{"root", "wheel", "sudo", "admin", "docker", "disk", "lxd", "incus-admin", "libvirt", + "kmem", "shadow"} // Account is one user resource of a module that declares groups. type Account struct { diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 14121ec..867293b 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -315,6 +315,8 @@ func ApplyMindingWindows( // (novox/hq ADR 0252), on the unit's rule above: two modules may each want the account in one group, // and the one going takes nothing the other still needs. wanted := groupsWanted(d.Resources) + // And what a record going hands to a resource that still asks for its group (novox/hq ADR 0312 §5). + handed := Handed{} removeOrphan := func(orphan store.Applied) error { var action, detail string @@ -339,10 +341,10 @@ func ApplyMindingWindows( case declaration.TypeOpening: action, detail, err = removeOpening(ctx, orphan, run, known.Firewall) case declaration.TypeUser: - action, detail, err = removeUser(ctx, sys, orphan, run, wanted) + action, detail, err = removeUser(ctx, sys, orphan, run, wanted, handed) case declaration.TypeAccess: // The path is left as it is; what it granted an account is taken back (novox/hq ADR 0312 §5). - action, detail = removeAccess(ctx, sys, orphan, run, wanted) + action, detail = removeAccess(ctx, sys, orphan, run, wanted, handed) default: action, detail, err = remove(ctx, sys, orphan, run, made) } @@ -509,7 +511,7 @@ func ApplyMindingWindows( // And one patience with the artifact store for the whole apply (novox/hq issue 291): a fetch it // does not answer during a maintenance window waits for the window instead of failing. in := inputs{declares: map[string]string{}, known: &known, windows: windows, away: newStoreAway(windows, log), - groups: wanted} + groups: wanted, handed: handed} // Where nothing is placed or mounted from, judged before anything is applied (novox/hq issue 339), and // which declared directories were already here: one this apply makes is the mesh's, one found may not be. refreshRuntimeRoots(ctx) @@ -1080,7 +1082,7 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru case *declaration.Container: return applyContainer(ctx, res, run, changed, in, previous) case *declaration.User: - return applyUser(ctx, sys, res, run, previous, in.groups) + return applyUser(ctx, sys, res, run, previous, in.groups, in.handed) case *declaration.Archive: return applyArchive(ctx, res, previous, in.away) case *declaration.Process: @@ -1968,7 +1970,7 @@ func remove(ctx context.Context, sys system.System, a store.Applied, run Runner, case declaration.TypeUser: // Kept, with its shell given back when that is safe (novox/hq ADR 0176 §2, issue 228). - return removeUser(ctx, sys, a, run, nil) + return removeUser(ctx, sys, a, run, nil, nil) case declaration.TypeNetwork: // **The reason this is a shape at all** (novox/hq ADR 0029). Orphans are removed in @@ -2131,6 +2133,8 @@ type inputs struct { // containerGroups the number of every group granted to a container, by container id. grants map[string]grant containerGroups map[string][]int + // handed is every group a record that went this apply handed to a resource still asking for it. + handed Handed } // fileDigest is what a file the container reads holds, by digest. diff --git a/internal/apply/grants.go b/internal/apply/grants.go index af3e04f..da702cf 100644 --- a/internal/apply/grants.go +++ b/internal/apply/grants.go @@ -4,12 +4,15 @@ import ( "context" "fmt" "os" + "os/exec" "os/user" "path/filepath" "slices" "sort" "strconv" "strings" + "sync" + "time" "github.com/novox/mesh-host/internal/accounts" "github.com/novox/mesh-host/internal/declaration" @@ -32,10 +35,11 @@ import ( // the path as its group may, and as nothing else. // // **The path is never changed** (§2). A group that cannot write the path is the operator's choice, as the data -// is: the access is stated unhealthy on every look, saying so and that the operator decides, and nothing is -// done to make it writable. **A read access grants nothing** (§3). **The root group is never granted** (§4): a +// is: the access grants nothing, is stated unhealthy on every look, saying so and that the operator decides, and +// nothing is done to make it writable. **A read access grants nothing** (§3). **The root group is never granted** (§4): a // path owned by group 0, or by a group whose members act as root (accounts.RootGroups, the account judge's own -// closed list), is refused by name. +// closed list, judged by the numbers those groups have here), or by a number the machine names nothing for, is +// refused by name. // // **What was granted is taken back as ADR 0252 takes back** (§5): the access records each group it put the // account in that the account was not in before; when no declared resource asks for it any more, the account @@ -86,18 +90,88 @@ var GroupOfPath = func(path string) (PathGroup, error) { return g, nil } -// GrantRefusal is why a path's group is never granted (novox/hq ADR 0312 §4), or nil when it may be: group 0, -// or a group whose members act as root by membership alone. +// RootGroupIDs is the number of every group whose members act as root (accounts.RootGroups), as this machine +// numbers them: read from /etc/group itself and from the machine's name service (`getent group`), since the engine +// is built without cgo and Go's own lookup reads /etc/group alone — a group a name service holds, or one number +// carrying several names, would otherwise pass a check made by name. Read at most once a minute, every look and +// apply judging against the same answer, and the name service given five seconds in all. A variable so a test can +// stand in. +var RootGroupIDs = func() map[int]string { + rootIDs.Lock() + defer rootIDs.Unlock() + if rootIDs.ids == nil || time.Since(rootIDs.at) > time.Minute { + rootIDs.ids, rootIDs.at = readRootGroupIDs(), time.Now() + } + return rootIDs.ids +} + +var rootIDs struct { + sync.Mutex + ids map[int]string + at time.Time +} + +func readRootGroupIDs() map[int]string { + out := map[int]string{} + if raw, err := os.ReadFile("/etc/group"); err == nil { + for name, gid := range groupEntries(string(raw)) { + if slices.Contains(accounts.RootGroups, name) { + out[gid] = name + } + } + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for _, name := range accounts.RootGroups { + raw, err := exec.CommandContext(ctx, "getent", "group", name).Output() + if err != nil { + continue + } + for found, gid := range groupEntries(string(raw)) { + if found == name { + out[gid] = name + } + } + } + return out +} + +// groupEntries reads group database lines, name:password:gid:members, as name to number. +func groupEntries(raw string) map[string]int { + out := map[string]int{} + for _, line := range strings.Split(raw, "\n") { + fields := strings.Split(strings.TrimSpace(line), ":") + if len(fields) < 3 || strings.HasPrefix(fields[0], "#") { + continue + } + if gid, err := strconv.Atoi(fields[2]); err == nil { + out[fields[0]] = gid + } + } + return out +} + +// GrantRefusal is why a path's group is never granted (novox/hq ADR 0312 §4), or nil when it may be: group 0, a +// group whose members act as root by membership alone — judged by its NUMBER against every number those groups +// have here, and by its name — or a group this machine names nothing for, which cannot be judged at all. func GrantRefusal(g PathGroup) error { - switch { - case g.GID == 0: - return fmt.Errorf("it is owned by group 0 (%s), and a read-write access never grants the root group: a "+ - "shared path is never a way to root (novox/hq ADR 0312 §4); nothing was granted, and nothing was "+ - "changed on it", orRoot(g.Name)) - case slices.Contains(accounts.RootGroups, g.Name): + const never = "a shared path is never a way to root (novox/hq ADR 0312 §4); nothing was granted, and nothing " + + "was changed on it" + if g.GID == 0 { + return fmt.Errorf("it is owned by group 0 (%s), and a read-write access never grants the root group: %s", + orRoot(g.Name), never) + } + if root, is := RootGroupIDs()[g.GID]; is { + return fmt.Errorf("it is owned by gid %d, which is %s here, whose members act as root, and a read-write "+ + "access never grants such a group: %s", g.GID, root, never) + } + if slices.Contains(accounts.RootGroups, g.Name) { return fmt.Errorf("it is owned by the group %s, whose members act as root, and a read-write access never "+ - "grants such a group: a shared path is never a way to root (novox/hq ADR 0312 §4); nothing was "+ - "granted, and nothing was changed on it", g.Words()) + "grants such a group: %s", g.Words(), never) + } + if g.Name == "" { + return fmt.Errorf("it is owned by gid %d, which this machine's group database names nothing, so whether "+ + "its members act as root cannot be judged, and an unjudged group is never granted: %s", g.GID, never) } return nil } @@ -126,6 +200,10 @@ type grant struct { refused error } +// grants says the group is given: it may be granted, and it may write the path. A group that cannot write is +// granted nothing (novox/hq ADR 0312 §2) — the access is stated unhealthy for the operator to decide. +func (g grant) grants() bool { return g.refused == nil && g.group.Writable } + // grantsOf reads the group of every read-write access that names a grant, by access id. One that is refused // grants nothing — the access itself fails, saying why — and one whose path the engine refuses to place // anything at (issue 339) is not read at all. @@ -159,7 +237,7 @@ func containerGroups(resources []declaration.Resource, grants map[string]grant) continue } g, granted := grants[a.ID] - if !granted || g.refused != nil { + if !granted || !g.grants() { continue } for _, id := range a.Grant.Containers { @@ -175,12 +253,13 @@ func containerGroups(resources []declaration.Resource, grants map[string]grant) } // ContainerGroupsOf is containerGroups for a whole declaration, read now: what the scheduler arms a scheduled -// step with, so a fire runs with the groups an apply would have created it with. +// step with, so a fire runs with the groups an apply would have created it with. A path issue 339's guard refuses +// is not read, as the apply reads none. func ContainerGroupsOf(d *declaration.Declaration) map[string][]int { if d == nil { return nil } - return containerGroups(d.Resources, grantsOf(d.Resources, nil)) + return containerGroups(d.Resources, grantsOf(d.Resources, refusedPlaces(d.Resources))) } // withGrants adds to what the declaration asks of each account the groups its read-write accesses grant (novox/hq @@ -192,7 +271,7 @@ func (w Wanted) withGrants(resources []declaration.Resource, grants map[string]g continue } g, granted := grants[a.ID] - if !granted || g.refused != nil || g.group.Name == "" { + if !granted || !g.grants() { continue } if w[a.Grant.Account] == nil { @@ -211,7 +290,7 @@ func GrantedAccounts(d *declaration.Declaration, held map[string]bool) []account if d == nil { return nil } - grants := grantsOf(d.Resources, nil) + grants := grantsOf(d.Resources, refusedPlaces(d.Resources)) var out []accounts.Account for _, r := range d.Resources { a, ok := r.(*declaration.Access) @@ -219,7 +298,7 @@ func GrantedAccounts(d *declaration.Declaration, held map[string]bool) []account continue } g, granted := grants[a.ID] - if !granted || g.refused != nil || g.group.Name == "" { + if !granted || !g.grants() { continue } at := strings.LastIndex(a.ID, ".") @@ -251,8 +330,9 @@ func grantAccess(ctx context.Context, sys system.System, r *declaration.Access, return fmt.Errorf("%s grants nothing: %w", r.Path, g.refused) } } + // A group that cannot write is granted nothing (§2): what an earlier apply granted is taken back below. account := "" - if granting { + if granting && g.group.Writable { account = r.Grant.Account } if account != "" && g.group.Name == "" { @@ -286,8 +366,9 @@ func grantAccess(ctx context.Context, sys system.System, r *declaration.Access, said = append(said, fmt.Sprintf("put %s in %s, the group that owns it; the tool runner holds it once "+ "restarted, a session that began before only after a new login", account, g.group.Name)) } - // Recorded only when the mesh put it there, now or before: a group held before is never taken back. - if !inGroup || slices.Contains(recorded, g.group.Name) { + // Recorded only when the mesh put it there, now or before, or a record that went handed it over: a group + // held before is never taken back. + if !inGroup || slices.Contains(recorded, g.group.Name) || in.handed.has(account, g.group.Name) { kept = append(kept, g.group.Name) } var others []string @@ -312,13 +393,13 @@ func grantAccess(ctx context.Context, sys system.System, r *declaration.Access, if account != "" { to = append(to, account) } - if len(to) > 0 { + switch { + case !g.group.Writable: + out.Detail = joinDetail(out.Detail, fmt.Sprintf("its group %s cannot write it, so nothing is granted: "+ + "stated unhealthy, and nothing was changed on it — the operator decides", g.group.Words())) + case len(to) > 0: out.Detail = joinDetail(out.Detail, fmt.Sprintf("grants %s to %s", g.group.Words(), strings.Join(to, " and "))) } - if !g.group.Writable { - out.Detail = joinDetail(out.Detail, "its group cannot write it: stated unhealthy, and nothing was changed "+ - "on it — the operator decides") - } } if len(said) > 0 { out.Detail = joinDetail(out.Detail, strings.Join(said, "; ")) @@ -373,13 +454,21 @@ func takeBack(ctx context.Context, sys system.System, account, id string, groups } // removeAccess is an access no longer declared: the path left exactly as it is (novox/hq ADR 0051), and every -// group it put an account in taken back unless another declared resource still asks for it (ADR 0312 §5). -func removeAccess(ctx context.Context, sys system.System, a store.Applied, run Runner, wanted Wanted) (string, string) { +// group it put an account in taken back unless another declared resource still asks for it (ADR 0312 §5) — then +// handed to that resource, which records it as the mesh's and takes it back when it stops asking. +func removeAccess(ctx context.Context, sys system.System, a store.Applied, run Runner, wanted Wanted, + handed Handed) (string, string) { const left = "an operator-owned path is never the host's to remove" if a.User == "" || len(a.Groups) == 0 { return "forgotten", left } - gave, said := takeBack(ctx, sys, a.User, a.ID, a.Groups, nil, run, wanted) + var stays []string + gave, said := takeBack(ctx, sys, a.User, a.ID, a.Groups, &stays, run, wanted) + for _, g := range stays { + if len(wanted.othersAsk(a.User, g, a.ID)) > 0 { + handed.hand(a.User, g) + } + } action := "forgotten" if gave { action = "restored" @@ -405,3 +494,21 @@ func gidWords(groups []int) string { } return strings.Join(words, ", ") } + +// Handed is every group a record that went in this apply put an account in, and another declared resource still +// asks for: account → group. The resource that still asks records it as the mesh's when it is applied, so the group +// is taken back once nobody asks for it any more, rather than staying for ever because the record that put it +// there went first (novox/hq ADR 0252 §3, ADR 0312 §5). +type Handed map[string]map[string]bool + +func (h Handed) hand(account, group string) { + if h == nil { + return + } + if h[account] == nil { + h[account] = map[string]bool{} + } + h[account][group] = true +} + +func (h Handed) has(account, group string) bool { return h[account][group] } diff --git a/internal/apply/grants_test.go b/internal/apply/grants_test.go index 8f3d176..e540a4d 100644 --- a/internal/apply/grants_test.go +++ b/internal/apply/grants_test.go @@ -93,6 +93,15 @@ func groupOfPaths(t *testing.T, groups map[string]PathGroup) { return g, nil } t.Cleanup(func() { GroupOfPath = was }) + rootGroupsAre(t, map[int]string{0: "root", 10: "wheel", 968: "docker"}) +} + +// rootGroupsAre stands in for the numbers this machine gives the groups whose members act as root. +func rootGroupsAre(t *testing.T, ids map[int]string) { + t.Helper() + was := RootGroupIDs + RootGroupIDs = func() map[int]string { return ids } + t.Cleanup(func() { RootGroupIDs = was }) } func accessGranting(id, path, account string, containers ...string) string { @@ -213,7 +222,7 @@ func TestAReadAccessGrantsNothing(t *testing.T) { } } -func TestAnAccessWhoseGroupCannotWriteIsGrantedAndSaysTheOperatorDecides(t *testing.T) { +func TestAnAccessWhoseGroupCannotWriteGrantsNothingAndSaysTheOperatorDecides(t *testing.T) { spool := t.TempDir() groupOfPaths(t, map[string]PathGroup{spool: {GID: 1002, Name: "downloads", Writable: false}}) before := statOf(t, spool) @@ -225,10 +234,17 @@ func TestAnAccessWhoseGroupCannotWriteIsGrantedAndSaysTheOperatorDecides(t *test t.Fatal(err) } o := outcomeOf(report, "nzbget.downloads") - if !strings.Contains(o.Detail, "its group cannot write it") || !strings.Contains(o.Detail, "the operator decides") || + if !strings.Contains(o.Detail, "cannot write it, so nothing is granted") || !strings.Contains(o.Detail, "the operator decides") || !strings.Contains(o.Detail, "nothing was changed") { t.Errorf("the access did not say its group cannot write and that the operator decides: %+v", o) } + // Nothing is given (ADR 0312 §2): no account group, no container group. + if strings.Contains(m.groups(), "downloads") { + t.Errorf("a group that cannot write was granted to the account: %q", m.groups()) + } + if run := strings.Join(m.runs["nzbget-app"], " "); strings.Contains(run, "--group-add") { + t.Errorf("a group that cannot write was granted to the container: %s", run) + } if after := statOf(t, spool); after != before { t.Errorf("the engine changed a path whose group cannot write: %s, was %s", after, before) } @@ -241,8 +257,16 @@ func TestAnAccessWhoseGroupCannotWriteIsGrantedAndSaysTheOperatorDecides(t *test func TestTheRootGroupIsNeverGranted(t *testing.T) { for _, g := range []PathGroup{{GID: 0, Name: "root", Writable: true}, {GID: 968, Name: "docker", Writable: true}, - {GID: 10, Name: "wheel", Writable: true}} { - t.Run(g.Name, func(t *testing.T) { + {GID: 10, Name: "wheel", Writable: true}, {GID: 42, Name: "shadow", Writable: true}, + // A number the root groups have here under another name — judged by the number, never the name. + {GID: 968, Name: "containers", Writable: true}, + // A number the machine names nothing for: unjudged, so never granted, to a container either. + {GID: 4242, Name: "", Writable: true}} { + want := g.Name + if want == "" || want == "containers" { + want = fmt.Sprintf("gid %d", g.GID) + } + t.Run(fmt.Sprintf("%d %s", g.GID, g.Name), func(t *testing.T) { shared := t.TempDir() groupOfPaths(t, map[string]PathGroup{shared: g}) m := newGrantMachine("operator", nil, g.Name) @@ -254,10 +278,10 @@ func TestTheRootGroupIsNeverGranted(t *testing.T) { } var failed *Error if !errors.As(err, &failed) || failed.Resource != "importer.shared" || - !strings.Contains(err.Error(), g.Name) || !strings.Contains(err.Error(), "never") { + !strings.Contains(err.Error(), want) || !strings.Contains(err.Error(), "never") { t.Errorf("the refusal does not name the access and the group: %v", err) } - if strings.Contains(m.groups(), g.Name) { + if g.Name != "" && strings.Contains(m.groups(), g.Name) { t.Errorf("the account was put in %s", g.Name) } if run := strings.Join(m.runs["importer-app"], " "); strings.Contains(run, "--group-add") { @@ -455,3 +479,81 @@ func TestAnAccountsOwnSessionIsSaidReloginNeededForAGrantedGroup(t *testing.T) { t.Errorf("a refused grant is judged: %+v", as) } } + +// An access going while a user resource still asks for its group hands the group over: the user resource records it +// as the mesh's, and takes it back when it stops asking too (novox/hq ADR 0312 §5, ADR 0252 §3). +func TestAGroupStillAskedForIsHandedOverAndTakenBackWhenTheLastAskerGoes(t *testing.T) { + music := t.TempDir() + groupOfPaths(t, map[string]PathGroup{music: {GID: 1001, Name: "media", Writable: true}}) + player := `{"id":"player.account","type":"user","name":"operator","groups":["media"]}` + m := newGrantMachine("operator", []string{"operator"}, "media") + + _, state, err := applyGrants(t, m, store.State{}, accessGranting("importer.music", music, "operator"), player) + if err != nil { + t.Fatal(err) + } + if rec, _ := state.Find("player.account"); len(rec.Groups) != 0 { + t.Fatalf("the user resource recorded a group the access put the account in: %+v", rec) + } + _, state, err = applyGrants(t, m, state, player) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(m.groups(), "media") { + t.Fatalf("a group the user resource still asks for was taken back: %q", m.groups()) + } + if rec, _ := state.Find("player.account"); strings.Join(rec.Groups, " ") != "media" { + t.Fatalf("the group was not handed to the user resource that still asks for it: %+v", rec) + } + if _, _, err := applyGrants(t, m, state, mounting("other.app", t.TempDir())); err != nil { + t.Fatal(err) + } + if strings.Contains(m.groups(), "media") { + t.Errorf("the group stayed after the last resource asking for it went: %q", m.groups()) + } +} + +// A grant's account is an argument to id, usermod and gpasswd, and a name that starts with a dash would be read as +// an option by each. +func TestAGrantsAccountMayNotReadAsAnOption(t *testing.T) { + for _, name := range []string{"-ogroups", "--help", "a b", "a:b"} { + _, err := declaration.Parse([]byte(`{"declaration":1,"resources":[{"id":"m.lib","type":"access","path":"/srv/lib",` + + `"mode":"read-write","grant":{"account":"` + name + `"}}]}`)) + if err == nil || !strings.Contains(err.Error(), "not starting with a dash") { + t.Errorf("the account %q was accepted: %v", name, err) + } + } + if _, err := declaration.Parse([]byte(`{"declaration":1,"resources":[{"id":"m.lib","type":"access","path":"/srv/lib",` + + `"mode":"read-write","grant":{"account":"jochen"}}]}`)); err != nil { + t.Errorf("an ordinary account was refused: %v", err) + } +} + +// The scheduler reads no path issue 339's guard refuses, as the apply reads none. +func TestAScheduledStepIsArmedWithoutReadingARefusedPath(t *testing.T) { + var read []string + was := GroupOfPath + GroupOfPath = func(path string) (PathGroup, error) { + read = append(read, path) + return PathGroup{GID: 1001, Name: "media", Writable: true}, nil + } + t.Cleanup(func() { GroupOfPath = was }) + rootGroupsAre(t, map[int]string{0: "root"}) + d := parse(t, `{"declaration":1,"resources":[`+accessGranting("m.etc", "/etc", "", "m.step")+`,`+ + `{"id":"m.step","type":"container","name":"step","image":"`+pinned+`","schedule":"0 3 * * *","volumes":["/etc:/x"]}]}`) + if got := ContainerGroupsOf(d); len(got["m.step"]) != 0 { + t.Errorf("a step was armed with the group of a refused path: %v", got) + } + if len(read) != 0 { + t.Errorf("the scheduler read a path the guard refuses: %v", read) + } +} + +// The group database is read as name to number, so a number carrying a root group's name under any other name +// is still that number. +func TestGroupEntriesAreReadByNumber(t *testing.T) { + got := groupEntries("root:x:0:\n# a comment\ndocker:x:968:operator\ncontainers:x:968:\nbroken\n") + if got["root"] != 0 || got["docker"] != 968 || got["containers"] != 968 || len(got) != 3 { + t.Errorf("read %v", got) + } +} diff --git a/internal/apply/groups.go b/internal/apply/groups.go index e19672e..308fa31 100644 --- a/internal/apply/groups.go +++ b/internal/apply/groups.go @@ -63,7 +63,7 @@ func (w Wanted) othersAsk(account, group, except string) []string { // applyGroups makes the account be in every group the resource declares, and takes it out of every group // this resource put it in and no longer asks for. What the mesh put the account in is recorded on out. func applyGroups(ctx context.Context, sys system.System, r *declaration.User, run Runner, - previous store.Applied, wanted Wanted, out *Outcome) error { + previous store.Applied, wanted Wanted, handed Handed, out *Outcome) error { // What this resource put the account in before, for this account only: a declaration that renamed its // user says nothing about the new one's groups. var added []string @@ -83,6 +83,10 @@ func applyGroups(ctx context.Context, sys system.System, r *declaration.User, ru var put []string for _, want := range r.Groups { if already[want] { + // A record that went handed it here: the mesh's, and this resource's to take back (novox/hq ADR 0312 §5). + if handed.has(r.Name, want) && !contains(added, want) { + added = append(added, want) + } continue } exists, err := system.GroupExists(ctx, system.Runner(run), want) @@ -139,7 +143,11 @@ func applyGroups(ctx context.Context, sys system.System, r *declaration.User, ru // giveGroupsBack is removeUser's groups: every group the mesh put the account in, taken back unless // another declared resource still asks for it. Never fatal, for the shell's reason: a removal that failed // would stay recorded and fail the same way on every apply after. Empty when there was nothing to say. -func giveGroupsBack(ctx context.Context, sys system.System, a store.Applied, run Runner, wanted Wanted) (bool, string) { +// +// A group left because another declared resource still asks for it is handed to that resource (handed), which +// records it as the mesh's, so it is taken back once nobody asks (novox/hq ADR 0312 §5). +func giveGroupsBack(ctx context.Context, sys system.System, a store.Applied, run Runner, wanted Wanted, + handed Handed) (bool, string) { if len(a.Groups) == 0 { return false, "" } @@ -157,6 +165,7 @@ func giveGroupsBack(ctx context.Context, sys system.System, a store.Applied, run } if others := wanted.othersAsk(a.Target, g, a.ID); len(others) > 0 { said = append(said, fmt.Sprintf("left in %s, which %s still asks for", g, strings.Join(others, ", "))) + handed.hand(a.Target, g) continue } gone, why := leaveGroup(ctx, sys, a.Target, g, run) diff --git a/internal/apply/user.go b/internal/apply/user.go index 44e227b..4891e74 100644 --- a/internal/apply/user.go +++ b/internal/apply/user.go @@ -35,7 +35,7 @@ const FirstLoginUID = 1000 // wanted is every group the whole declaration asks an account to be in (novox/hq ADR 0252), so a group // this resource stops asking for is kept while another resource asks for it. func applyUser(ctx context.Context, sys system.System, r *declaration.User, run Runner, - previous store.Applied, wanted Wanted) (Outcome, error) { + previous store.Applied, wanted Wanted, handed Handed) (Outcome, error) { out := begin(r) out.Action = "unchanged" // What was found is carried from the record for as long as the resource is recorded — for this @@ -100,7 +100,7 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run // Groups before the shell, so that a failure here comes before the shell is changed: a record // is written only for an apply that worked, and a shell changed by a failed one would be read // next time as the account's own, and the one it replaced lost. - if err := applyGroups(ctx, sys, r, run, previous, wanted, &out); err != nil { + if err := applyGroups(ctx, sys, r, run, previous, wanted, handed, &out); err != nil { return out, err } @@ -207,7 +207,7 @@ func onOff(on bool) string { // And every group the mesh put the account in, while no other declared resource asks for it (novox/hq // ADR 0252), on the same rule: never fatal, and never a group the account was in before. func removeUser(ctx context.Context, sys system.System, a store.Applied, run Runner, - wanted Wanted) (string, string, error) { + wanted Wanted, handed Handed) (string, string, error) { const kept = "the account is kept; the host never deletes a login" login, exists, err := system.LookUpUser(ctx, system.Runner(run), a.Target) if err != nil { @@ -226,7 +226,7 @@ func removeUser(ctx context.Context, sys system.System, a store.Applied, run Run action = "restored" } } - if gave, said := giveGroupsBack(ctx, sys, a, run, wanted); said != "" { + if gave, said := giveGroupsBack(ctx, sys, a, run, wanted, handed); said != "" { detail += "; " + said if gave { action = "restored" diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index e4318f9..e2f6d29 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -522,9 +522,11 @@ func (a *Access) validate(where string, _ bool) []string { problems = append(problems, where+": a grant names a container with no id") } } - if a.Grant.Account != "" && strings.ContainsAny(a.Grant.Account, " :/\t\n") { - problems = append(problems, where+": a grant's account is an account's name, and "+ - a.Grant.Account+" is not one") + if a.Grant.Account != "" && !accountName.MatchString(a.Grant.Account) { + // Handed to id, usermod and gpasswd as an argument: a name starting with a dash would be read as an + // option by each of them. + problems = append(problems, fmt.Sprintf("%s: a grant's account is an account's name — letters, digits, "+ + "dots, underscores and dashes, not starting with a dash — and %q is not one", where, a.Grant.Account)) } } return problems @@ -1778,6 +1780,10 @@ func checkWhileStopped(resources []Resource) []string { return problems } +// accountName is what a grant's account may be: the portable account name the user tools take, optionally ending +// in `$`, never starting with a dash. +var accountName = regexp.MustCompile(`^[A-Za-z0-9_.][A-Za-z0-9_.-]*\$?$`) + // checkGrants judges each access's grant against the whole declaration (novox/hq ADR 0312): a container it // names is a container of this declaration, as a maintenance window's are. A grant to a container the machine // is not told about would be a group given to nothing, said as granted.