diff --git a/cmd/mesh-host/libraries.go b/cmd/mesh-host/libraries.go index f4b1cec..8588901 100644 --- a/cmd/mesh-host/libraries.go +++ b/cmd/mesh-host/libraries.go @@ -118,7 +118,7 @@ func judgeLibrary(l declaration.Library) verdict { if a := v.Act; a != nil { out.act = &link.AccessActRecord{Act: a.Act, Access: access, Warrant: a.Warrant, By: a.By, State: a.State, Started: a.Started, Ended: a.Ended, Folders: a.Folders, Files: a.Files, Kept: a.Kept, Links: a.Links, - Other: a.Other, Mounts: a.Mounts, Shared: a.Shared, Why: a.Why} + Other: a.Other, Mounts: a.Mounts, Shared: a.Shared, Linked: a.Linked, Stripped: a.Stripped, Why: a.Why} } switch v.Kind { case apply.VerdictWaitKeep: diff --git a/internal/apply/identities_test.go b/internal/apply/identities_test.go index e324319..798bf25 100644 --- a/internal/apply/identities_test.go +++ b/internal/apply/identities_test.go @@ -19,9 +19,10 @@ import ( // accountsMachine is a user and group database the runner answers from, and every command that changed it. type accountsMachine struct { users, groups map[string][2]int // name → id, primary gid (a group's second is unused) - // in is each account's added groups, by name. - in map[string][]string - ran []string + // in is each account's added groups, by name; homes each account's home, "/" when not set. + in map[string][]string + homes map[string]string + ran []string } // groupNamed is the name of gid in m, or the number. @@ -48,7 +49,11 @@ func (m *accountsMachine) run(_ context.Context, name string, args ...string) (s if db == "group" { return n + ":x:" + strconv.Itoa(ids[0]) + ":\n", nil } - return n + ":x:" + strconv.Itoa(ids[0]) + ":" + strconv.Itoa(ids[1]) + "::/:/usr/bin/nologin\n", nil + home := m.homes[n] + if home == "" { + home = "/" + } + return n + ":x:" + strconv.Itoa(ids[0]) + ":" + strconv.Itoa(ids[1]) + "::" + home + ":/usr/bin/nologin\n", nil } } return "", errors.New("getent exited 2") @@ -67,10 +72,19 @@ func (m *accountsMachine) run(_ context.Context, name string, args ...string) (s if args[5] == "--groups" { m.setIn(name, strings.Split(args[6], ",")) } + if m.homes == nil { + m.homes = map[string]string{} + } + m.homes[name] = "/nonexistent" case "usermod": name := args[len(args)-1] if args[0] == "--append" { m.setIn(name, append(m.in[name], args[2])) + } else if args[0] == "--home" { + if m.homes == nil { + m.homes = map[string]string{} + } + m.homes[name] = args[1] } else { u := m.users[name] m.users[name] = [2]int{u[0], mustAtoi(args[1])} @@ -151,7 +165,7 @@ func TestAModulesIdentityIsMadeWithItsNumbersAndRemovedWhenUndeclared(t *testing } want := []string{ "groupadd --system --gid 61001 mesh-radarr", - "useradd --system --uid 61001 --gid 61001 --no-create-home --home-dir / --shell /usr/bin/nologin mesh-radarr", + "useradd --system --uid 61001 --gid 61001 --no-create-home --home-dir /nonexistent --shell /usr/bin/nologin mesh-radarr", } if strings.Join(m.ran, "\n") != strings.Join(want, "\n") { t.Fatalf("made with:\n%s\nwant:\n%s", strings.Join(m.ran, "\n"), strings.Join(want, "\n")) diff --git a/internal/apply/library.go b/internal/apply/library.go index 9e44753..fa7278b 100644 --- a/internal/apply/library.go +++ b/internal/apply/library.go @@ -179,12 +179,18 @@ func assessLibrary(ctx context.Context, l declaration.Library, rec store.Library // The controller's word that the library was taken holds for the filesystem the warrant was carried out on. takenHere := l.Taken && (rec.KeptOn == here || (rec.KeptOn == "" && !moved)) switch { - case rec.Kept && rec.KeptOn == here && !moved: - v.kind, v.how = libraryKeep, rec.How - return v - case takenHere: + case rec.Kept && rec.KeptOn == here && !moved && rec.How == store.KeptByWarrant, takenHere: + // Taken whole on the operator's word: every sub-path is the library's. v.kind, v.how = libraryKeep, store.KeptByWarrant return v + case rec.Kept && rec.KeptOn == here && !moved: + // Kept without a warrant (empty, or in shape): a sub-path asked for since that holds data not in the group + // waits for the operator's word as the root would (§7). + v.kind, v.how = libraryKeep, rec.How + if !pathsInShape(l, rec.Paths) { + v.kind, v.how = libraryWaitKeep, "" + } + return v } facts, err := readFolder(l.Root) if err != nil { @@ -192,8 +198,10 @@ func assessLibrary(ctx context.Context, l declaration.Library, rec store.Library } switch { case facts.Empty: + // Nothing there, so no sub-path either: every one is made. v.kind, v.how = libraryKeep, store.KeptEmpty - case int(facts.GID) == l.GID && facts.Mode&0o2070 == 0o2070 && sampleInShape(l.Root, l.GID): + case int(facts.GID) == l.GID && facts.Mode&0o2070 == 0o2070 && sampleInShape(l.Root, l.GID) && + pathsInShape(l, nil): // The root and a bounded sample beneath it already in the group with group write and setgid: data that // is the library's already, kept without asking. One entry of the sample out of shape asks (§7). v.kind, v.how = libraryKeep, store.KeptInShape @@ -226,7 +234,14 @@ func recordedLibraryMounts(except string) []string { func applyLibrary(ctx context.Context, l declaration.Library) (Outcome, libraryVerdict, error) { out := Outcome{ID: LibraryAccess(l.Library), Type: "library", Target: l.Root} lock := libraryLock(l.Library) - lock.Lock() + if !lock.TryLock() { + // **A take walk is running (up to its bound, half an hour): the apply does not wait for it.** The library is + // the walk's until it is through; it keeps it, and its containers are not held for it. + rec, _ := libraryRecord(l.Library) + out.Action = "unchanged" + out.Detail = "the take act is setting its group under its root now; this apply leaves the library to it" + return out, libraryVerdict{kind: libraryKeep, how: store.KeptByWarrant, placed: rec.KeptOn}, nil + } defer lock.Unlock() rec, _ := libraryRecord(l.Library) v := assessLibrary(ctx, l, rec) @@ -272,6 +287,7 @@ func applyLibrary(ctx context.Context, l declaration.Library) (Outcome, libraryV } r.Root, r.Mount, r.Placed, r.Fs, r.GID = l.Root, v.mount, v.placed, v.fs, l.GID r.Kept, r.KeptOn = true, v.placed + r.Paths = unionPaths(r.Paths, l.Paths) return true }) out.Action = "unchanged" @@ -434,3 +450,49 @@ func leaveRunners(ctx context.Context, sys system.System, libs []declaration.Lib sort.Slice(out, func(i, j int) bool { return out[i].ID < out[j].ID }) return out } + +// pathsInShape is whether every requested sub-path not kept before is absent (it is made), empty, or already in the +// group with group write and setgid, it and a bounded sample beneath it: what may be kept without the operator's +// word (§7). One that cannot be read is not. +func pathsInShape(l declaration.Library, kept []string) bool { + done := map[string]bool{} + for _, p := range kept { + done[p] = true + } + for _, p := range l.Paths { + if done[p] { + continue + } + path := filepath.Join(l.Root, p) + if _, err := lstatWithin(path); errors.Is(err, os.ErrNotExist) { + continue + } + facts, err := readFolder(path) + if err != nil { + return false + } + if facts.Empty { + continue + } + if int(facts.GID) != l.GID || facts.Mode&0o2070 != 0o2070 || !sampleInShape(path, l.GID) { + return false + } + } + return true +} + +// unionPaths is a with every path of b not in it. +func unionPaths(a, b []string) []string { + have := map[string]bool{} + for _, p := range a { + have[p] = true + } + for _, p := range b { + if !have[p] { + a = append(a, p) + have[p] = true + } + } + sort.Strings(a) + return a +} diff --git a/internal/apply/library_linux.go b/internal/apply/library_linux.go index d8bc00f..a9010c1 100644 --- a/internal/apply/library_linux.go +++ b/internal/apply/library_linux.go @@ -19,7 +19,11 @@ func actRecord(warrant, by, on string, started time.Time, w *treeWalk, err error c := w.counts rec := &store.KeptAct{Act: "take", Warrant: warrant, By: by, On: on, State: "done", Started: started, Ended: time.Now().UTC(), Folders: c.Folders, Files: c.Files, Kept: c.Kept, Links: c.Links, Other: c.Other, - Mounts: c.Mounts, Shared: c.Shared} + Mounts: c.Mounts, Shared: c.Shared, Linked: c.Linked, Stripped: c.Stripped} + if c.Shared+c.Linked+c.Stripped > 0 { + rec.Why = fmt.Sprintf("%d file(s) with another name given the group, %d root's left as they are, %d whose "+ + "setuid or setgid bit the group change took", c.Linked, c.Shared, c.Stripped) + } switch { case errors.Is(err, ErrWalkBound): rec.State, rec.Why = "stopped", "it met its bound of entries or time; it goes on from where it stopped" diff --git a/internal/apply/library_test.go b/internal/apply/library_test.go index 15bc383..8cca204 100644 --- a/internal/apply/library_test.go +++ b/internal/apply/library_test.go @@ -249,6 +249,11 @@ func TestTheTakeActSetsTheGroupUnderTheRoot(t *testing.T) { if err := os.Link(secret, filepath.Join(root, "movies", "hardlink")); err != nil { t.Fatal(err) } + if os.Geteuid() == 0 { // an account's file, as an *arr app's hardlinked download is + if err := os.Chown(secret, 65534, -1); err != nil { + t.Fatal(err) + } + } if err := os.Symlink(outside, filepath.Join(root, "link")); err != nil { t.Fatal(err) } @@ -281,7 +286,7 @@ func TestTheTakeActSetsTheGroupUnderTheRoot(t *testing.T) { break } } - if rec.Act.State != "done" || rec.Act.Shared != 1 || !rec.Kept || rec.How != store.KeptByWarrant || + if rec.Act.State != "done" || rec.Act.Linked != 1 || rec.Act.Stripped != 1 || !rec.Kept || rec.How != store.KeptByWarrant || rec.KeptOn == "" { t.Fatalf("the act is not recorded done under its warrant on its filesystem: %+v %+v", rec, rec.Act) } @@ -291,12 +296,12 @@ func TestTheTakeActSetsTheGroupUnderTheRoot(t *testing.T) { st.Gid, st.Mode&0o7777) } } - if st := statOf(t, file); int(st.Gid) != gid || st.Mode&0o7777 != 0o4664 { - t.Errorf("a file: gid %d mode %o, and the group with 4664 (its own bits kept, group rw) was wanted", st.Gid, - st.Mode&0o7777) + if st := statOf(t, file); int(st.Gid) != gid || st.Mode&0o7777 != 0o0664 { + t.Errorf("a file: gid %d mode %o, and the group with 0664 (its setuid bit taken by the group change and "+ + "never put back) was wanted", st.Gid, st.Mode&0o7777) } - if st := statOf(t, secret); int(st.Gid) == gid || st.Mode&0o7777 != 0o600 { - t.Error("a file with another name outside the library was given the group") + if st := statOf(t, secret); int(st.Gid) != gid || st.Mode&0o7777 != 0o660 { + t.Error("an account's file with another name (a hardlinked download) was not given the group") } if st := statOf(t, outside); int(st.Gid) == gid { t.Error("the act followed a link out of the library") @@ -430,7 +435,7 @@ func TestAModulesUserIsInItsLibrariesGroups(t *testing.T) { "groupadd --system --gid 62000 mesh-media", "groupadd --system --gid 62001 mesh-books", "groupadd --system --gid 61001 mesh-radarr", - "useradd --system --uid 61001 --gid 62000 --groups mesh-books --no-create-home --home-dir / --shell " + + "useradd --system --uid 61001 --gid 62000 --groups mesh-books --no-create-home --home-dir /nonexistent --shell " + "/usr/bin/nologin mesh-radarr", } if strings.Join(m.ran, "\n") != strings.Join(want, "\n") { @@ -689,3 +694,97 @@ func TestARootInShapeWithDataBelowNotInTheGroupWaits(t *testing.T) { t.Fatal("a kept library asked again") } } + +// Review: a file with another name that root owns is left as it is (its other name may be anywhere), and a setgid +// file never keeps its setgid bit once regrouped. +func TestRootsLinkedFilesAreLeftAndSetgidIsNeverKept(t *testing.T) { + gid := testGID(t) + f, err := os.CreateTemp(t.TempDir(), "file") + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := unix.Chmod(f.Name(), 0o2755); err != nil { + t.Fatal(err) + } + st := statOf(t, f.Name()) + rootLinked := st + rootLinked.Uid, rootLinked.Nlink = 0, 2 + if did, err := groupVisit(gid)(int(f.Fd()), rootLinked, false); err != nil || did != visitShared { + t.Fatalf("a root-owned file with another name was not left: %d %v", did, err) + } + if after := statOf(t, f.Name()); after.Gid != st.Gid || after.Mode != st.Mode { + t.Fatal("a root-owned file with another name was changed") + } + did, err := groupVisit(gid)(int(f.Fd()), st, false) + if err != nil || did&visitStripped == 0 { + t.Fatalf("a setgid file's bit is not said taken: %d %v", did, err) + } + if after := statOf(t, f.Name()); after.Mode&(unix.S_ISGID|unix.S_ISUID) != 0 || int(after.Gid) != gid { + t.Fatalf("a setgid file kept its bit in the library's group: %o", after.Mode&0o7777) + } +} + +// Review: a sub-path asked for after the library was kept without a warrant, which holds data not in the group, +// waits for the operator's word as the root would, and nothing of it is changed; an empty one is kept. +func TestASubPathHoldingOldDataWaitsForTheOperatorsWord(t *testing.T) { + gid := testGID(t) + libraries(t, nil, nil) + root := filepath.Join(t.TempDir(), "media") + if err := os.Mkdir(root, 0o755); err != nil { + t.Fatal(err) + } + if _, _, err := applyLib(t, libraryDecl(t, libJSON("media", root, gid, true, false, "movies")), store.State{}); err != nil { + t.Fatal(err) + } + // Someone puts old data at a sub-path the library is asked for only now. + music := filepath.Join(root, "music") + if err := os.MkdirAll(filepath.Join(music, "album"), 0o755); err != nil { + t.Fatal(err) + } + before := statOf(t, music) + report, _, err := applyLib(t, libraryDecl(t, libJSON("media", root, gid, true, false, "movies", "music")), store.State{}) + if err != nil { + t.Fatal(err) + } + if o := outcomeOf(report, "library:media"); o.Action != "waiting" || !strings.Contains(o.Detail, KeepWords("media")) { + t.Fatalf("a sub-path holding old data was kept without a warrant: %+v", o) + } + if after := statOf(t, music); after.Gid != before.Gid || after.Mode != before.Mode { + t.Fatal("a sub-path holding old data was changed") + } + // An empty one asked for is kept. + if err := os.RemoveAll(filepath.Join(music, "album")); err != nil { + t.Fatal(err) + } + report, _, _ = applyLib(t, libraryDecl(t, libJSON("media", root, gid, true, false, "movies", "music")), store.State{}) + if o := outcomeOf(report, "library:media"); o.Action == "waiting" { + t.Fatalf("an empty sub-path was not kept: %+v", o) + } +} + +// Review: an apply does not wait for a take walk that is running: it leaves the library to it. +func TestAnApplyDoesNotWaitForATakeWalk(t *testing.T) { + gid := testGID(t) + libraries(t, nil, nil) + root := filepath.Join(t.TempDir(), "media") + if err := os.Mkdir(root, 0o755); err != nil { + t.Fatal(err) + } + lock := libraryLock("media") + lock.Lock() + defer lock.Unlock() + done := make(chan Report, 1) + go func() { + r, _, _ := applyLib(t, libraryDecl(t, libJSON("media", root, gid, true, false)), store.State{}) + done <- r + }() + select { + case r := <-done: + if o := outcomeOf(r, "library:media"); !strings.Contains(o.Detail, "take act") { + t.Fatalf("the apply does not say it left the library to the walk: %+v", o) + } + case <-time.After(2 * time.Second): + t.Fatal("the apply waited for the take walk") + } +} diff --git a/internal/apply/walk.go b/internal/apply/walk.go index 2473410..61ee282 100644 --- a/internal/apply/walk.go +++ b/internal/apply/walk.go @@ -33,8 +33,9 @@ type WalkCounts struct { // Folders and Files are the entries changed; Kept the ones that already had what they need. Folders, Files, Kept int // Links are links passed by, Other entries neither a file nor a folder, Mounts folders on another filesystem - // not entered, Shared files with another name elsewhere left as they are. - Links, Other, Mounts, Shared int + // not entered; Linked files with another name an account owns, given the group; Shared files with another + // name root owns, left as they are; Stripped files whose setuid or setgid bit the group change took. + Links, Other, Mounts, Linked, Shared, Stripped int // Read is every entry looked at; Failed the entries that could not be changed (an immutable file, say). Read, Failed int } diff --git a/internal/apply/walk_linux.go b/internal/apply/walk_linux.go index b96d534..8cd864a 100644 --- a/internal/apply/walk_linux.go +++ b/internal/apply/walk_linux.go @@ -30,11 +30,14 @@ var machineStats = walkStats{ fd: unix.Fstat, } -// What a visit did to one entry. +// What a visit did to one entry, as flags: nothing (it had what it needs), or changed, and beside it whether it was a +// file with another name (linked, changed; shared, left), and whether its setuid or setgid bit went. const ( - visitKept = iota // it had what it needs - visitChanged // it was changed - visitShared // a file with another name elsewhere: left as it is + visitKept = 0 + visitChanged = 1 << iota + visitLinked // a file with another name, owned by an account: given the group + visitShared // a file with another name, owned by root: left as it is + visitStripped // a file whose setuid or setgid bit the group change took, and which stays taken ) // visitFunc is what a walk does to one folder or file, open on fd, whose stat is st: what it did, or the error @@ -224,37 +227,57 @@ func (w *treeWalk) visited(fd int, st unix.Stat_t, folder bool) error { return nil } switch { - case did == visitShared: + case did&visitShared != 0: w.counts.Shared++ - case did == visitChanged && folder: + case did&visitChanged != 0 && folder: w.counts.Folders++ - case did == visitChanged: + case did&visitChanged != 0: w.counts.Files++ default: w.counts.Kept++ } + if did&visitLinked != 0 { + w.counts.Linked++ + } + if did&visitStripped != 0 { + w.counts.Stripped++ + } return nil } // groupVisit gives each entry the library's group gid, group read and write, and on a folder group enter and the // setgid bit, so what is made beneath keeps the group (ADR 0328 §6, §7). The owner, the owner's bits and others' -// bits are never changed: a file's own setuid or setgid bit, which the kernel drops when the group changes, is put -// back as it was. A file with more than one name is left as it is — its other name may be outside the library, and -// giving it the group would give the group that other file. +// bits are never changed — but for what the group change itself takes: +// +// - **A file's setuid and setgid bits stay taken.** The kernel drops them when a file's group changes; a setgid +// file in the library's group would hand anyone who runs it the group, and a setuid one is no library data. They +// are never put back, setgid is taken off a file the group change left it on, and each such file is counted. +// - **A file with another name** (the *arr apps hardlink downloads into the library) is given the group when an +// account owns it: the kernel's protected_hardlinks already keeps an account from linking a file it does not +// own. One root owns is left as it is: its other name may be anywhere, and the group would reach it. Both are +// counted. func groupVisit(gid int) visitFunc { return func(fd int, st unix.Stat_t, folder bool) (int, error) { + did := visitKept if !folder && st.Nlink > 1 { - return visitShared, nil + if st.Uid == 0 { + return visitShared, nil + } + did |= visitLinked } mode := uint32(st.Mode) & 0o7777 want := mode | 0o060 if folder { want = mode | 0o070 | unix.S_ISGID } - if int(st.Gid) == gid && want == mode { - return visitKept, nil + regroup := int(st.Gid) != gid + if !folder && regroup { + want &^= unix.S_ISUID | unix.S_ISGID } - if int(st.Gid) != gid { + if !regroup && want == mode { + return did &^ visitLinked, nil + } + if regroup { if err := unix.Fchown(fd, -1, gid); err != nil { return 0, fmt.Errorf("an entry's group could not be set: %v", err) } @@ -262,7 +285,10 @@ func groupVisit(gid int) visitFunc { if err := unix.Fchmod(fd, want); err != nil { return 0, fmt.Errorf("an entry's mode could not be set: %v", err) } - return visitChanged, nil + if !folder && mode&(unix.S_ISUID|unix.S_ISGID) != 0 && want&(unix.S_ISUID|unix.S_ISGID) == 0 { + did |= visitStripped + } + return did | visitChanged, nil } } diff --git a/internal/link/messages.go b/internal/link/messages.go index 56e1b94..9609f27 100644 --- a/internal/link/messages.go +++ b/internal/link/messages.go @@ -498,8 +498,11 @@ type AccessActRecord struct { Links int `json:"links,omitempty"` Other int `json:"other,omitempty"` Mounts int `json:"mounts,omitempty"` - // Shared is the files with another name elsewhere (more than one link), left as they are. - Shared int `json:"shared,omitempty"` + // Shared is the files with another name root owns, left as they are; Linked those an account owns, given the + // group; Stripped the files whose setuid or setgid bit the group change took. + Shared int `json:"shared,omitempty"` + Linked int `json:"linked,omitempty"` + Stripped int `json:"stripped,omitempty"` // Why is what stopped or failed it, in words with no path. Why string `json:"why,omitempty"` } diff --git a/internal/store/libraries.go b/internal/store/libraries.go index 5c09620..f39ac41 100644 --- a/internal/store/libraries.go +++ b/internal/store/libraries.go @@ -54,7 +54,10 @@ type LibraryRecord struct { At time.Time `json:"at,omitzero"` // Act is the last walk under the root; Cursor where a walk that met its bound stopped (the names of each level // below the root, never said), for the next to go on from. - Act *KeptAct `json:"act,omitempty"` + Act *KeptAct `json:"act,omitempty"` + // Paths are the sub-paths kept so far on the filesystem KeptOn names: one asked for later that holds data not + // in the group waits for the operator's word as a root would, unless the library was taken whole. + Paths []string `json:"paths,omitempty"` Cursor []string `json:"cursor,omitempty"` } @@ -70,7 +73,7 @@ type KeptAct struct { On string `json:"on,omitempty"` // Folders and Files are the entries changed; Kept the ones that already had what they need; Links the links // passed by unfollowed; Other the entries neither a file nor a folder; Mounts the folders on another filesystem - // not entered; Shared the files with another name elsewhere (more than one link), left as they are. + // not entered; Shared the files with another name root owns, left as they are. Folders int `json:"folders,omitempty"` Files int `json:"files,omitempty"` Kept int `json:"kept,omitempty"` @@ -78,6 +81,10 @@ type KeptAct struct { Other int `json:"other,omitempty"` Mounts int `json:"mounts,omitempty"` Shared int `json:"shared,omitempty"` + // Linked is the files with another name an account owns, given the group; Stripped the files whose setuid or + // setgid bit the group change took, and which stays taken. + Linked int `json:"linked,omitempty"` + Stripped int `json:"stripped,omitempty"` // Why is what stopped or failed it, in words with no path. Why string `json:"why,omitempty"` } diff --git a/internal/system/identities.go b/internal/system/identities.go index 7c13b67..386e72a 100644 --- a/internal/system/identities.go +++ b/internal/system/identities.go @@ -19,14 +19,19 @@ func (arch) CreateSystemGroup(ctx context.Context, run Runner, name string, gid return nil } -// CreateSystemAccount makes a system account with uid and the group gid as its own: no home made, `/` as the +// NoHome is the home field of every module account: a path that is not there, so nothing that follows an account's +// home — usermod regrouping every file under it, for one — ever walks a real tree. Never `/`: `usermod --gid` on +// an account whose home is `/` regroups every file it owns on the machine. +const NoHome = "/nonexistent" + +// CreateSystemAccount makes a system account with uid and the group gid as its own: no home made, NoHome as the // home field, nologin as its shell, and the password useradd leaves locked. func (arch) CreateSystemAccount(ctx context.Context, run Runner, name string, uid, gid int, groups []string) error { args := []string{"--system", "--uid", strconv.Itoa(uid), "--gid", strconv.Itoa(gid)} if len(groups) > 0 { args = append(args, "--groups", strings.Join(groups, ",")) } - args = append(args, "--no-create-home", "--home-dir", "/", "--shell", "/usr/bin/nologin", name) + args = append(args, "--no-create-home", "--home-dir", NoHome, "--shell", "/usr/bin/nologin", name) if _, err := run(ctx, "useradd", args...); err != nil { return fmt.Errorf("cannot create the account %q: %w", name, err) } @@ -34,7 +39,25 @@ func (arch) CreateSystemAccount(ctx context.Context, run Runner, name string, ui } // ModifyAccount gives an account the mesh made its primary group gid and exactly groups as its added groups. +// +// **Its home is moved to NoHome first, never its files.** `usermod --gid` regroups every file under the account's +// home that the account owns; an account made with `/` as its home (an earlier engine's) would have the whole +// machine walked as root. So an account whose home is not NoHome is given NoHome first, without -m: no file moves. func (arch) ModifyAccount(ctx context.Context, run Runner, name string, gid int, groups []string) error { + passwd, err := run(ctx, "getent", "passwd", name) + if err != nil { + return fmt.Errorf("cannot read the account %q: %w", name, err) + } + fields := strings.Split(strings.TrimSpace(passwd), ":") + if len(fields) < 6 { + return fmt.Errorf("the user database gave %q for %q, which is not a passwd entry", strings.TrimSpace(passwd), + name) + } + if fields[5] != NoHome { + if _, err := run(ctx, "usermod", "--home", NoHome, name); err != nil { + return fmt.Errorf("cannot give the account %q no home before its groups: %w", name, err) + } + } if _, err := run(ctx, "usermod", "--gid", strconv.Itoa(gid), "--groups", strings.Join(groups, ","), name); err != nil { return fmt.Errorf("cannot give the account %q its groups: %w", name, err) @@ -76,7 +99,7 @@ func (alpine) CreateSystemAccount(ctx context.Context, run Runner, name string, if n, _, ok := strings.Cut(group, ":"); ok { groupName = n } - if _, err := run(ctx, "adduser", "-S", "-D", "-H", "-h", "/", "-s", "/sbin/nologin", "-u", strconv.Itoa(uid), + if _, err := run(ctx, "adduser", "-S", "-D", "-H", "-h", NoHome, "-s", "/sbin/nologin", "-u", strconv.Itoa(uid), "-G", groupName, name); err != nil { return fmt.Errorf("cannot create the account %q: %w", name, err) } diff --git a/internal/system/identities_test.go b/internal/system/identities_test.go index 4fe84b1..7eabea1 100644 --- a/internal/system/identities_test.go +++ b/internal/system/identities_test.go @@ -36,3 +36,38 @@ func TestBusyboxGivesAnAccountItsAddedGroupsAndRefusesAnotherPrimary(t *testing. t.Fatalf("a change of primary group was not refused saying why: %v", err) } } + +// An account is made with no home, and a change of its groups moves an old home of `/` away first: `usermod --gid` +// regroups every file under the home the account owns, and under `/` that is the whole machine. +func TestAnAccountsGroupsChangeOnlyWithNoHome(t *testing.T) { + var ran []string + home := "/" + run := func(_ context.Context, name string, args ...string) (string, error) { + line := name + " " + strings.Join(args, " ") + ran = append(ran, line) + if name == "getent" { + return "mesh-radarr:x:61001:61001::" + home + ":/usr/bin/nologin\n", nil + } + if name == "usermod" && args[0] == "--home" { + home = args[1] + } + if name == "usermod" && args[0] == "--gid" && home != NoHome { + t.Fatalf("usermod --gid ran with the home %s: every file there would be regrouped", home) + } + return "", nil + } + if err := (arch{}).CreateSystemAccount(context.Background(), run, "mesh-radarr", 61001, 61001, nil); err != nil { + t.Fatal(err) + } + if !strings.Contains(ran[0], "--no-create-home --home-dir "+NoHome+" ") { + t.Fatalf("an account is made with a home: %s", ran[0]) + } + if err := (arch{}).ModifyAccount(context.Background(), run, "mesh-radarr", 62000, nil); err != nil { + t.Fatal(err) + } + got := strings.Join(ran, "\n") + if !strings.Contains(got, "usermod --home "+NoHome+" mesh-radarr") || strings.Contains(got, " -m") || + strings.Contains(got, "--move-home") { + t.Fatalf("the home was not moved away first, without moving files:\n%s", got) + } +}