From 5d5dccdd5515bf68125a5e9a3f5522afb7e2cf23 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 03:57:34 +0200 Subject: [PATCH 1/3] A login the mesh set is given back, and undeclaring one no longer stops the apply (hq issue 225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user had no removal, so an undeclared one failed as an orphan and aborted every apply after. Removal now keeps the account, gives back the shell recorded when the mesh first changed it if it is still the mesh's and still usable, and says why otherwise (hq ADR 0176 §2). A shell is refused before it is set unless it is executable and listed in /etc/shells, since usermod succeeds on a missing one. --- internal/apply/apply.go | 10 +- internal/apply/user.go | 124 ++++++++++++--- internal/apply/user_test.go | 294 ++++++++++++++++++++++++++++++++++++ internal/store/store.go | 21 +++ internal/system/system.go | 56 +++++++ 5 files changed, 486 insertions(+), 19 deletions(-) create mode 100644 internal/apply/user_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index f4a858e..22f3244 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -60,6 +60,9 @@ type Outcome struct { stateless bool // found is, for a service, its unit as the host first found it (novox/hq ADR 0118). found *store.FoundUnit + // shell is, for a user, the login shell it was found with and the one the mesh set (novox/hq + // ADR 0176 §2, issue 225). + shell *store.LoginShell // reads is, for a container, the digest of each file it was created reading, by path — so // the next apply can say which one changed (novox/hq 04-ISSUES/103). reads map[string]string @@ -587,6 +590,7 @@ func ApplyKeeping( Reads: outcome.reads, Stateless: outcome.stateless, Found: outcome.found, + Shell: outcome.shell, Holds: holds(resource), }) if outcome.found != nil { @@ -716,7 +720,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) + return applyUser(ctx, sys, res, run, previous) case *declaration.Archive: return applyArchive(ctx, res, previous) case *declaration.Process: @@ -1430,6 +1434,10 @@ func remove(ctx context.Context, sys system.System, a store.Applied, run Runner, // would be the data loss ADR 0030 exists to prevent, on a directory the mesh never made. return "forgotten", "an operator-owned path is never the host's to remove", nil + case declaration.TypeUser: + // Kept, with its shell given back when that is safe (novox/hq ADR 0176 §2, issue 225). + return removeUser(ctx, sys, a, run) + case declaration.TypeNetwork: // **The reason this is a shape at all** (novox/hq ADR 0029). Orphans are removed in // reverse declaration order, so a network written before the containers that join it is diff --git a/internal/apply/user.go b/internal/apply/user.go index 2cf2026..d882586 100644 --- a/internal/apply/user.go +++ b/internal/apply/user.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" "github.com/novox/mesh-host/internal/system" ) @@ -23,14 +24,35 @@ import ( // // Reconciling, like everything else here: it is not told whether the user is new. Creating, // setting a shell and adding groups are each done only when the machine does not already agree. -func applyUser(ctx context.Context, sys system.System, r *declaration.User, run Runner) (Outcome, error) { +// +// previous is this resource's record, which carries the shell the account had before the mesh +// first changed it, so removal can give it back (novox/hq ADR 0176 §2, issue 225). +func applyUser(ctx context.Context, sys system.System, r *declaration.User, run Runner, + previous store.Applied) (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 + // account only: a declaration that renamed its user says nothing about the new one's shell. + if previous.Shell != nil && previous.Target == r.Name { + kept := *previous.Shell + out.shell = &kept + } login, exists, err := system.LookUpUser(ctx, system.Runner(run), r.Name) if err != nil { return out, err } + + // **A shell is refused before anything is touched** (novox/hq issue 225). Refused after the + // account was created or its groups changed, the account would be half the declaration's; a + // refusal fails this resource and leaves the account exactly as it was. + if r.Shell != "" && (!exists || login.Shell != r.Shell) { + if err := system.UsableShell(r.Shell); err != nil { + return out, fmt.Errorf("%q's shell was not set, and the account was left as it is: %w", + r.Name, err) + } + } + if !exists { if err := sys.CreateUser(ctx, system.Runner(run), r.Name, r.Home, r.Shell); err != nil { return out, err @@ -46,26 +68,15 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run r.Name) } out.Action = "created" - } - - // The shell, only when it differs. Absent means the host asserts nothing — a field that - // always asserts cannot express "leave it alone", which is the difference between managing a - // machine and taking it over. - if r.Shell != "" && login.Shell != r.Shell { - if err := sys.SetUserShell(ctx, system.Runner(run), r.Name, r.Shell); err != nil { - return out, err - } - if back, _, err := system.LookUpUser(ctx, system.Runner(run), r.Name); err != nil { - return out, err - } else if back.Shell != r.Shell { - return out, fmt.Errorf("set %q's shell to %q and the user database says %q", - r.Name, r.Shell, back.Shell) - } - if out.Action == "unchanged" { - out.Action = "updated" + if r.Shell != "" { + // No shell from before to give back: the account had none until the mesh made it. + out.shell = &store.LoginShell{Set: login.Shell, Created: true} } } + // 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 len(r.Groups) > 0 { in, err := system.GroupsOf(ctx, system.Runner(run), r.Name) if err != nil { @@ -87,9 +98,86 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run } } } + + // The shell, only when it differs. Absent means the host asserts nothing — a field that + // always asserts cannot express "leave it alone", which is the difference between managing a + // machine and taking it over. + if r.Shell != "" && login.Shell != r.Shell { + if err := sys.SetUserShell(ctx, system.Runner(run), r.Name, r.Shell); err != nil { + return out, err + } + if back, _, err := system.LookUpUser(ctx, system.Runner(run), r.Name); err != nil { + return out, err + } else if back.Shell != r.Shell { + return out, fmt.Errorf("set %q's shell to %q and the user database says %q", + r.Name, r.Shell, back.Shell) + } + // **What was found is recorded once** (novox/hq ADR 0176 §2). A later change keeps it: what + // is given back is the shell from before the mesh, never the mesh's own earlier choice. + if out.shell == nil { + out.shell = &store.LoginShell{Found: login.Shell} + } + out.shell.Set = r.Shell + if out.Action == "unchanged" { + out.Action = "updated" + } + } return out, nil } +// removeUser is what undeclaring a login does: never deleting the account, and giving back the +// shell the mesh replaced when that is still safe (novox/hq ADR 0176 §2, issue 225). +// +// **The account is never deleted, whether or not the mesh created it.** An account owns a home, +// files, a crontab, a mailbox — what a person did with it is not the mesh's to know, and deleting +// it is the data loss ADR 0030 exists to prevent. It is the package's rule, on a login: the +// mesh no longer requires it, which is not the same as "remove it". +// +// The shell goes back only while the account still has the one the mesh set — one a person chose +// since is theirs — and only to a shell that is still usable: giving back a shell that has been +// uninstalled since would break the very logins the giving back is for. Otherwise it is left, and +// the outcome says why. Never errNoRemoval: an orphaned login that failed removal stopped the +// whole apply, on every apply after. +func removeUser(ctx context.Context, sys system.System, a store.Applied, run Runner) (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 { + return "", "", err + } + if !exists { + return "forgotten", "no longer there", nil + } + found := a.Shell + switch { + case found == nil: + return "forgotten", kept + ", and its shell was never changed by the mesh", nil + case found.Created: + return "forgotten", kept + "; the mesh created it, so there is no shell from before to give back", nil + case login.Shell != found.Set: + return "forgotten", fmt.Sprintf("%s, and its shell %s left as it is: changed since the mesh set %s", + kept, login.Shell, found.Set), nil + case found.Found == "": + return "forgotten", kept + ", and its shell left as it is: it had none before the mesh set one", nil + } + if err := system.UsableShell(found.Found); err != nil { + return "forgotten", fmt.Sprintf("%s, and its shell %s left as it is: the one it had before "+ + "cannot be given back: %v", kept, login.Shell, err), nil + } + // A give-back that fails is said and not fatal: fatal, the record would stay and fail the same + // way on every apply after — the very wedge this removal exists to end. + if err := sys.SetUserShell(ctx, system.Runner(run), a.Target, found.Found); err != nil { + return "forgotten", fmt.Sprintf("%s, and the shell it had before the mesh, %s, could not be "+ + "given back: %v", kept, found.Found, err), nil + } + if back, _, err := system.LookUpUser(ctx, system.Runner(run), a.Target); err != nil { + return "", "", err + } else if back.Shell != found.Found { + return "forgotten", fmt.Sprintf("%s; gave back the shell %s and the user database says %s", + kept, found.Found, back.Shell), nil + } + return "restored", fmt.Sprintf("%s; the shell it had before the mesh, %s, given back", kept, found.Found), nil +} + // own sets a path's owner, when one was declared. // // Looked up by name every time rather than cached: a user's numeric id is not stable across diff --git a/internal/apply/user_test.go b/internal/apply/user_test.go new file mode 100644 index 0000000..401461f --- /dev/null +++ b/internal/apply/user_test.go @@ -0,0 +1,294 @@ +package apply + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/store" + "github.com/novox/mesh-host/internal/system" +) + +// Defends novox/hq ADR 0176 §2 and issue 225: a login the mesh set is given back when its holding +// moves, undeclaring one never stops the node applying, and a shell is checked before it is set. + +// logins is a fake user database: each account's shell by name, and every command it was asked. +type logins struct { + shells map[string]string + asked []string +} + +func (l *logins) run(_ context.Context, name string, args ...string) (string, error) { + l.asked = append(l.asked, name+" "+strings.Join(args, " ")) + who := args[len(args)-1] + switch name { + case "getent": + if shell, ok := l.shells[who]; ok { + return who + ":x:1500:1500::/home/" + who + ":" + shell + "\n", nil + } + return "", errors.New("getent exited 2: ") // the host's runner's words for "no such key" + case "useradd": + shell := "" + for i, a := range args { + if a == "--shell" { + shell = args[i+1] + } + } + l.shells[who] = shell + case "usermod": + if args[0] == "--shell" { + l.shells[who] = args[1] + } + case "userdel": + delete(l.shells, who) + case "id": + return "\n", nil + } + return "", nil +} + +func (l *logins) did(prefix string) bool { + for _, a := range l.asked { + if strings.HasPrefix(a, prefix) { + return true + } + } + return false +} + +// shellsOn makes a machine's shells in a directory a test owns: each name an executable file, +// listed or not in the machine's list of shells as said, which this test's apply then reads. +func shellsOn(t *testing.T, listed []string, unlisted ...string) string { + t.Helper() + dir := t.TempDir() + var list strings.Builder + list.WriteString("# Pathnames of valid login shells.\n") + for _, name := range append(append([]string{}, listed...), unlisted...) { + if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + } + for _, name := range listed { + list.WriteString(filepath.Join(dir, name) + "\n") + } + if err := os.WriteFile(filepath.Join(dir, "shells"), []byte(list.String()), 0o644); err != nil { + t.Fatal(err) + } + t.Cleanup(system.ShellsIn(filepath.Join(dir, "shells"))) + return dir +} + +func applyUsers(t *testing.T, l *logins, known store.State, resources string) (Report, store.State, error) { + t.Helper() + if resources == "" { + // Undeclared: something else stays, since a declaration with nothing in it is refused. + resources = `{"id":"other.dir","type":"directory","path":"` + t.TempDir() + `/other"}` + } + return Apply(context.Background(), archHost(t), parse(t, `{"declaration":1,"resources":[`+resources+`]}`), + known, store.OriginDeclared, l.run, nil, nil) +} + +func userWith(shell string) string { + return `{"id":"shell.login","type":"user","name":"operator","shell":"` + shell + `"}` +} + +func TestAnUndeclaredUserNoLongerStopsTheApply(t *testing.T) { + // Before issue 225 the host had no removal for a user, the orphan failed with "no way to + // remove", and an orphan's failure aborts the apply before its first resource — on every + // apply after, since the record stayed. + dir := shellsOn(t, []string{"bash", "zsh"}) + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + _, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + page := filepath.Join(t.TempDir(), "page") + report, state, err := applyUsers(t, l, state, + `{"id":"web.page","type":"file","path":"`+page+`","content":"hello\n"}`) + if err != nil { + t.Fatalf("an undeclared user stopped the apply: %v", err) + } + if _, err := os.Stat(page); err != nil { + t.Errorf("a file in the same declaration was not written: %v", err) + } + if o := outcomeOf(report, "shell.login"); o.Action == "" { + t.Errorf("the user's removal was not reported: %+v", report.Outcomes) + } + if _, still := state.Find("shell.login"); still { + t.Error("the user is still recorded, so the next apply would meet it again") + } +} + +func TestTheShellFoundIsGivenBackWhenTheUserIsUndeclared(t *testing.T) { + dir := shellsOn(t, []string{"bash", "zsh"}) + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + _, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + if l.shells["operator"] != dir+"/zsh" { + t.Fatalf("the declared shell was not set: %q", l.shells["operator"]) + } + if r, _ := state.Find("shell.login"); r.Shell == nil || r.Shell.Found != dir+"/bash" { + t.Fatalf("the shell the account had was not recorded: %+v", r.Shell) + } + + report, _, err := applyUsers(t, l, state, "") + if err != nil { + t.Fatal(err) + } + if l.shells["operator"] != dir+"/bash" { + t.Errorf("the shell the account had was not given back: %q", l.shells["operator"]) + } + if o := outcomeOf(report, "shell.login"); o.Action != "restored" { + t.Errorf("the give-back was not said: %+v", o) + } + if l.did("userdel") { + t.Error("the account was deleted") + } +} + +func TestAShellAPersonChangedSinceIsLeftAlone(t *testing.T) { + dir := shellsOn(t, []string{"bash", "zsh", "fish"}) + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + _, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + l.shells["operator"] = dir + "/fish" // chsh, by the person whose login it is + l.asked = nil + + report, _, err := applyUsers(t, l, state, "") + if err != nil { + t.Fatal(err) + } + if l.did("usermod") || l.shells["operator"] != dir+"/fish" { + t.Errorf("a shell a person chose was taken from them: %q, %v", l.shells["operator"], l.asked) + } + if o := outcomeOf(report, "shell.login"); o.Action != "forgotten" || !strings.Contains(o.Detail, "changed since") { + t.Errorf("the outcome does not say why the shell was left: %+v", o) + } +} + +func TestAFoundShellThatIsGoneIsNotGivenBack(t *testing.T) { + // Giving back a shell uninstalled since would break the logins the giving back is for. + dir := shellsOn(t, []string{"bash", "zsh"}) + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + _, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(dir + "/bash"); err != nil { + t.Fatal(err) + } + l.asked = nil + report, _, err := applyUsers(t, l, state, "") + if err != nil { + t.Fatal(err) + } + if l.did("usermod") || l.shells["operator"] != dir+"/zsh" { + t.Errorf("a shell no longer on the machine was given back: %q", l.shells["operator"]) + } + if o := outcomeOf(report, "shell.login"); !strings.Contains(o.Detail, "cannot be given back") { + t.Errorf("the outcome does not say why the shell was left: %+v", o) + } +} + +func TestAShellThatIsMissingOrUnlistedIsRefusedBeforeItIsSet(t *testing.T) { + dir := shellsOn(t, []string{"bash"}, "unlisted") + for name, shell := range map[string]string{ + "missing": dir + "/zsh", + "unlisted": dir + "/unlisted", + } { + t.Run(name, func(t *testing.T) { + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + page := filepath.Join(t.TempDir(), "page") + report, state, err := applyUsers(t, l, store.State{}, userWith(shell)+`, + {"id":"web.page","type":"file","path":"`+page+`","content":"hello\n"}`) + if err == nil { + t.Fatal("the refused shell did not fail its resource") + } + if l.did("usermod") || l.shells["operator"] != dir+"/bash" { + t.Errorf("the account was changed: %q, %v", l.shells["operator"], l.asked) + } + if _, recorded := state.Find("shell.login"); recorded { + t.Error("a refused user was recorded") + } + if o := outcomeOf(report, "web.page"); o.Action != "created" { + t.Errorf("the refusal stopped the rest of the declaration: %+v", report.Outcomes) + } + }) + } + t.Run("an account not yet made", func(t *testing.T) { + l := &logins{shells: map[string]string{}} + if _, _, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")); err == nil { + t.Fatal("the missing shell was not refused") + } + if l.did("useradd") { + t.Errorf("the account was made with a shell that is not there: %v", l.asked) + } + }) +} + +func TestAnAccountThatRefusesLoginsNeedNotBeListed(t *testing.T) { + // A service's account has nologin, which no distribution lists among its shells; refusing it + // would refuse the controller's own account. + dir := shellsOn(t, []string{"bash"}, "nologin") + l := &logins{shells: map[string]string{}} + if _, _, err := applyUsers(t, l, store.State{}, userWith(dir+"/nologin")); err != nil { + t.Fatalf("a service account was refused: %v", err) + } + if l.shells["operator"] != dir+"/nologin" { + t.Errorf("the account was not made: %v", l.asked) + } +} + +func TestACreatedAccountSurvivesItsRemoval(t *testing.T) { + dir := shellsOn(t, []string{"zsh"}) + l := &logins{shells: map[string]string{}} + report, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + if o := outcomeOf(report, "shell.login"); o.Action != "created" { + t.Fatalf("the account was not created: %+v", o) + } + l.asked = nil + report, _, err = applyUsers(t, l, state, "") + if err != nil { + t.Fatal(err) + } + if _, still := l.shells["operator"]; !still || l.did("userdel") || l.did("usermod") { + t.Errorf("a created account was not left as it is: %v", l.asked) + } + if o := outcomeOf(report, "shell.login"); !strings.Contains(o.Detail, "account is kept") { + t.Errorf("the outcome does not say the account was kept: %+v", o) + } +} + +func TestTheFoundShellIsNotOverwrittenByASecondChange(t *testing.T) { + // The holding moves from one shell module to another: what is given back in the end is the + // shell from before the mesh, not the first module's. + dir := shellsOn(t, []string{"bash", "zsh", "fish"}) + l := &logins{shells: map[string]string{"operator": dir + "/bash"}} + _, state, err := applyUsers(t, l, store.State{}, userWith(dir+"/zsh")) + if err != nil { + t.Fatal(err) + } + _, state, err = applyUsers(t, l, state, userWith(dir+"/fish")) + if err != nil { + t.Fatal(err) + } + if r, _ := state.Find("shell.login"); r.Shell == nil || r.Shell.Found != dir+"/bash" || r.Shell.Set != dir+"/fish" { + t.Fatalf("the record is not the shell found and the one set last: %+v", r.Shell) + } + if _, _, err := applyUsers(t, l, state, ""); err != nil { + t.Fatal(err) + } + if l.shells["operator"] != dir+"/bash" { + t.Errorf("given back %q, not the shell from before the mesh", l.shells["operator"]) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 55efedf..8b01bf7 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -95,6 +95,13 @@ type Applied struct { // file itself was — so undeclaring it gives the machine back exactly what it had. Into *Into `json:"into,omitempty"` + // Shell is, for a user, the login shell the account had before the mesh first set one, and + // the shell the mesh set last (novox/hq ADR 0176 §2, issue 225). Removal gives the found shell + // back, and only while the account still has the one the mesh set: a shell a person chose since + // is theirs. Absent when the mesh never changed the shell, and on a record written before the + // host kept it — then the shell is left exactly as it is. + Shell *LoginShell `json:"shell,omitempty"` + // Reads is, for a container, the digest of each file it was created reading — its env-files // and the files mounted into it — by path (novox/hq 04-ISSUES/103). // @@ -597,6 +604,20 @@ type FoundUnit struct { Boot string `json:"boot,omitempty"` } +// LoginShell is what the host knows about an account's login shell, to give it back. +type LoginShell struct { + // Found is the shell the account had when the mesh first changed it. Never overwritten by a + // later change: what is given back is what was there before the mesh, not the mesh's own + // previous choice. Empty for an account the mesh created, which had no shell before it. + Found string `json:"found,omitempty"` + // Set is the shell the mesh set last — what removal compares the account against, since the + // declaration that said so is gone by then. + Set string `json:"set"` + // Created is an account the mesh made. Kept only so removal can say why there is nothing to + // give back; the account itself is never deleted. + Created bool `json:"created,omitempty"` +} + // PendingFound is a unit as found by an apply of its service that has not yet been recorded, and // who asked for that apply — so only a declaration from the same origin can say it is gone. type PendingFound struct { diff --git a/internal/system/system.go b/internal/system/system.go index 3712197..aac4b14 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -19,7 +19,9 @@ import ( "context" "errors" "fmt" + "os" "os/exec" + "path/filepath" "regexp" "strconv" "strings" @@ -125,6 +127,60 @@ func GroupsOf(ctx context.Context, run Runner, name string) ([]string, error) { return strings.Fields(out), nil } +// shells is where the machine lists the shells a login may have (shells(5)). A variable so a test +// can point it at a list of its own; ShellsIn is how. +var shells = "/etc/shells" + +// ShellsIn points where the machine's shells are listed at a file a test owns, until the returned +// function puts it back. Nothing outside a test calls it. +func ShellsIn(list string) (restore func()) { + was := shells + shells = list + return func() { shells = was } +} + +// UsableShell says why a path cannot be an account's login shell, or nil when it can. +// +// **Asked before a shell is set, because nothing after it would say.** `usermod --shell` only +// warns about a shell that is missing or not executable, and succeeds; the host's read-back +// compares the user database's string, which then matches. So an account could be pointed at a +// shell that is not there, and console, ssh and display-manager logins all fail — after a +// failed package install, say, which does not stop the resources after it (novox/hq issue 225). +// +// Listed among the machine's shells as well as executable, because that list is what login +// services check: an unlisted shell is one ssh and the display manager may refuse. +// +// **Except a shell that refuses a login.** nologin and false are how a service's account says it +// is not a login at all, and no distribution lists them — the controller's own account has one. +// Requiring them listed would refuse every service account; they are still required to exist. +func UsableShell(path string) error { + if !filepath.IsAbs(path) { + return fmt.Errorf("the shell %q is not an absolute path", path) + } + info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("the shell %s is not on this machine: %w", path, err) + } + if !info.Mode().IsRegular() || info.Mode().Perm()&0o111 == 0 { + return fmt.Errorf("the shell %s is not an executable file", path) + } + if base := filepath.Base(path); base == "nologin" || base == "false" { + return nil + } + raw, err := os.ReadFile(shells) + if err != nil { + // Unreadable is not "not listed": the two are told apart, as the user database's are. + return fmt.Errorf("the machine's shells (%s) could not be read, so %s cannot be checked: %w", + shells, path, err) + } + for _, line := range strings.Split(string(raw), "\n") { + if line = strings.TrimSpace(line); line == path { + return nil + } + } + return fmt.Errorf("the shell %s is not listed in %s, so logins may refuse it", path, shells) +} + // Supports reports whether this host can apply a shape. func Supports(s System, t declaration.Type) bool { for _, shape := range s.Shapes() { From 2a5f4c82701e4435ab350ca0989cb22c6ec370ff Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 04:07:40 +0200 Subject: [PATCH 2/3] Parents the host makes inside an owner's home are the owner's (hq ADR 0182, to-be 41) A file or archive placed under a fresh account's home with an owner left the parents it created, such as ~/.config or ~/.local/share, owned by root, so the person's own programs could not write there. Parents that already existed, and any outside the owner's home, are left as before. --- internal/apply/apply.go | 4 +- internal/apply/archive.go | 2 +- internal/apply/block.go | 2 +- internal/apply/home_parents_test.go | 126 ++++++++++++++++++++++++++++ internal/apply/into.go | 2 +- internal/apply/user.go | 63 ++++++++++++++ 6 files changed, 194 insertions(+), 5 deletions(-) create mode 100644 internal/apply/home_parents_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 22f3244..1d489d0 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -772,7 +772,7 @@ func applyDirectory(r *declaration.Directory) (Outcome, error) { } if !existed { - if err := os.MkdirAll(r.Path, mode); err != nil { + if err := makeDirs(r.Path, mode, r.Owner); err != nil { return out, err } } @@ -968,7 +968,7 @@ func applyFile(r *declaration.File, previous store.Applied, unseal Unseal, keepF return out, fmt.Errorf("keeping the original of %s before writing over it: %w", r.Path, err) } } - if err := os.MkdirAll(filepath.Dir(r.Path), 0o755); err != nil { + if err := makeDirs(filepath.Dir(r.Path), 0o755, r.Owner); err != nil { return out, err } if err := writeAtomically(r.Path, []byte(content), mode); err != nil { diff --git a/internal/apply/archive.go b/internal/apply/archive.go index 526e0bb..460682f 100644 --- a/internal/apply/archive.go +++ b/internal/apply/archive.go @@ -89,7 +89,7 @@ func applyArchive(ctx context.Context, r *declaration.Archive, previous store.Ap // removed only once the new one is in place. A failed unpack leaves the old tree untouched. func replaceWith(body []byte, path, owner string) (int, error) { parent := filepath.Dir(path) - if err := os.MkdirAll(parent, 0o755); err != nil { + if err := makeDirs(parent, 0o755, owner); err != nil { return 0, err } fresh := path + ".unpacking" diff --git a/internal/apply/block.go b/internal/apply/block.go index 0ebca87..ca025cc 100644 --- a/internal/apply/block.go +++ b/internal/apply/block.go @@ -183,7 +183,7 @@ func applyBlock(r *declaration.File, previous store.Applied) (Outcome, error) { } else if mode, err = modeOf(r.Mode, mode); err != nil { return out, err } - if err := os.MkdirAll(filepath.Dir(real), 0o755); err != nil { + if err := makeDirs(filepath.Dir(real), 0o755, r.Owner); err != nil { return out, err } if err := writeAtomically(real, []byte(next), mode); err != nil { diff --git a/internal/apply/home_parents_test.go b/internal/apply/home_parents_test.go new file mode 100644 index 0000000..8970633 --- /dev/null +++ b/internal/apply/home_parents_test.go @@ -0,0 +1,126 @@ +package apply + +import ( + "context" + "os" + osuser "os/user" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/store" +) + +// Defends novox/hq ADR 0182 and to-be 41: a parent the host makes inside an owner's home is the +// owner's, one that was there is held as found, and one outside the home is made as before. + +// aHome gives the account running the test a home in a directory the test owns, and writes down +// every directory the host gives to whom. The account's own name, so what the host chowns resolves +// without being root; the record, so what was given is told apart from what was merely made. +func aHome(t *testing.T) (home, owner string, given map[string]string) { + t.Helper() + me, err := osuser.Current() + if err != nil { + t.Skip("no current user to own anything") + } + home = t.TempDir() + given = map[string]string{} + wasHome, wasOwn := homeOf, ownMade + homeOf = func(name string) (string, error) { + if name == me.Username { + return home, nil + } + return wasHome(name) + } + ownMade = func(path, owner string) error { + given[path] = owner + return wasOwn(path, owner) + } + t.Cleanup(func() { homeOf, ownMade = wasHome, wasOwn }) + return home, me.Username, given +} + +func givenPaths(given map[string]string) []string { + var paths []string + for p := range given { + paths = append(paths, p) + } + sort.Strings(paths) + return paths +} + +func TestAFileUnderAHomeGivesTheParentsItMadeToItsOwner(t *testing.T) { + home, owner, given := aHome(t) + target := filepath.Join(home, ".config", "mesh", "environment.sh") + d := parse(t, `{"declaration":1,"resources":[{"id":"shell.env","type":"file","path":"`+target+ + `","content":"export A=1\n","owner":"`+owner+`"}]}`) + if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + noServices, nil, nil); err != nil { + t.Fatal(err) + } + want := []string{filepath.Join(home, ".config"), filepath.Join(home, ".config", "mesh")} + if got := givenPaths(given); strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("given to the owner: %v, want %v", got, want) + } + for _, p := range want { + if given[p] != owner { + t.Errorf("%s given to %q", p, given[p]) + } + } +} + +func TestAnArchiveUnderAHomeGivesTheParentsItMadeToItsOwner(t *testing.T) { + home, owner, given := aHome(t) + body, digest := anArchive(t, map[string]string{"p10k.zsh": "theme"}) + target := filepath.Join(home, ".local", "share", "powerlevel10k") + d := declare(t, `{"id":"shell.theme","type":"archive","source":"`+serving(t, body)+ + `","digest":"`+digest+`","path":"`+target+`","owner":"`+owner+`"}`) + if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + noServices, nil, nil); err != nil { + t.Fatal(err) + } + for _, p := range []string{filepath.Join(home, ".local"), filepath.Join(home, ".local", "share")} { + if given[p] != owner { + t.Errorf("%s, made by the host, was not given to the owner: %v", p, givenPaths(given)) + } + } +} + +func TestAParentThatWasThereIsHeldAsFound(t *testing.T) { + home, owner, given := aHome(t) + config := filepath.Join(home, ".config") + if err := os.Mkdir(config, 0o700); err != nil { + t.Fatal(err) + } + target := filepath.Join(config, "mesh", "environment.sh") + d := parse(t, `{"declaration":1,"resources":[{"id":"shell.env","type":"file","path":"`+target+ + `","content":"export A=1\n","owner":"`+owner+`"}]}`) + if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + noServices, nil, nil); err != nil { + t.Fatal(err) + } + if _, touched := given[config]; touched { + t.Error("a parent that was already there was given to the owner") + } + if info, _ := os.Stat(config); info.Mode().Perm() != 0o700 { + t.Errorf("a parent that was already there changed mode: %o", info.Mode().Perm()) + } + if given[filepath.Join(config, "mesh")] != owner { + t.Errorf("the parent the host made was not given to the owner: %v", givenPaths(given)) + } +} + +func TestAParentOutsideTheHomeIsMadeAsBefore(t *testing.T) { + _, owner, given := aHome(t) + target := filepath.Join(t.TempDir(), "var", "lib", "module", "settings.conf") + d := parse(t, `{"declaration":1,"resources":[{"id":"module.conf","type":"file","path":"`+target+ + `","content":"a=1\n","owner":"`+owner+`"}]}`) + if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginDeclared, + noServices, nil, nil); err != nil { + t.Fatal(err) + } + if len(given) != 0 { + t.Errorf("parents outside the owner's home were given to it: %v", givenPaths(given)) + } +} diff --git a/internal/apply/into.go b/internal/apply/into.go index eefcbeb..a02e7da 100644 --- a/internal/apply/into.go +++ b/internal/apply/into.go @@ -132,7 +132,7 @@ func applyInto(r *declaration.File, previous store.Applied) (Outcome, error) { mode = m } } - if err := os.MkdirAll(filepath.Dir(r.Path), 0o755); err != nil { + if err := makeDirs(filepath.Dir(r.Path), 0o755, r.Owner); err != nil { return out, err } if err := writeAtomically(r.Path, want, mode); err != nil { diff --git a/internal/apply/user.go b/internal/apply/user.go index d882586..81e80ff 100644 --- a/internal/apply/user.go +++ b/internal/apply/user.go @@ -2,6 +2,7 @@ package apply import ( "context" + "errors" "fmt" "os" osuser "os/user" @@ -258,6 +259,68 @@ func ownedBy(path, owner string) (bool, error) { return uid == wantUID && gid == wantGID, nil } +// makeDirs makes a directory and any parent of it that is missing, as MkdirAll does — and gives +// each one it made inside the owner's home to the owner (novox/hq ADR 0182, to-be 41). +// +// **A parent made as root inside a home is a home the person cannot use.** A module writing +// ~/.config/mesh/environment.sh, or unpacking into ~/.local/share/powerlevel10k, on a fresh account +// made ~/.config and ~/.local/share owned by root: the file was the person's, the directory every +// program of theirs writes into was not. So what the host creates between the home and the target +// is the owner's, as the target is. +// +// **Only what the host created.** A parent that was already there is never chowned or chmodded: +// what a person or another program made is held as found (ADR 0182). And only inside the owner's +// home, read from the user database, not guessed from a prefix on /home: a module's directory under +// /var/lib is made exactly as before, whoever its files belong to. +func makeDirs(dir string, mode os.FileMode, owner string) error { + var made []string + for d := filepath.Clean(dir); ; d = filepath.Dir(d) { + if _, err := os.Lstat(d); !errors.Is(err, os.ErrNotExist) { + break + } + made = append(made, d) + if filepath.Dir(d) == d { + break + } + } + if err := os.MkdirAll(dir, mode); err != nil { + return err + } + if owner == "" || len(made) == 0 { + return nil + } + home, err := homeOf(owner) + if err != nil || home == "" { + // A numeric owner — a container's user — has no home, and a name the machine does not + // know fails where the target is given to it. Either way nothing here is a home's. + return nil + } + home = filepath.Clean(home) + for _, d := range made { + if d != home && !strings.HasPrefix(d, home+string(os.PathSeparator)) { + continue + } + if err := ownMade(d, owner); err != nil { + return err + } + } + return nil +} + +// homeOf is an owner's home from the user database, and ownMade gives a directory the host made to +// its owner. Variables so a test can give an owner a home it owns, and see what was given to whom +// without being root. +var ( + homeOf = func(owner string) (string, error) { + found, err := osuser.Lookup(owner) + if err != nil { + return "", err + } + return found.HomeDir, nil + } + ownMade = own +) + // ownAll gives a whole tree to a user, for an archive that was unpacked into it. func ownAll(root, owner string) error { if owner == "" { From f2eda240ecd51ca81a677463d025ab185dff8c16 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 4 Oct 2026 10:30:57 +0200 Subject: [PATCH 3/3] Cite hq issue 228: 225 was taken on main while this branch was open --- internal/apply/apply.go | 4 ++-- internal/apply/user.go | 6 +++--- internal/apply/user_test.go | 4 ++-- internal/store/store.go | 2 +- internal/system/system.go | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 1d489d0..79ef74f 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -61,7 +61,7 @@ type Outcome struct { // found is, for a service, its unit as the host first found it (novox/hq ADR 0118). found *store.FoundUnit // shell is, for a user, the login shell it was found with and the one the mesh set (novox/hq - // ADR 0176 §2, issue 225). + // ADR 0176 §2, issue 228). shell *store.LoginShell // reads is, for a container, the digest of each file it was created reading, by path — so // the next apply can say which one changed (novox/hq 04-ISSUES/103). @@ -1435,7 +1435,7 @@ func remove(ctx context.Context, sys system.System, a store.Applied, run Runner, return "forgotten", "an operator-owned path is never the host's to remove", nil case declaration.TypeUser: - // Kept, with its shell given back when that is safe (novox/hq ADR 0176 §2, issue 225). + // Kept, with its shell given back when that is safe (novox/hq ADR 0176 §2, issue 228). return removeUser(ctx, sys, a, run) case declaration.TypeNetwork: diff --git a/internal/apply/user.go b/internal/apply/user.go index 81e80ff..740eea2 100644 --- a/internal/apply/user.go +++ b/internal/apply/user.go @@ -27,7 +27,7 @@ import ( // setting a shell and adding groups are each done only when the machine does not already agree. // // previous is this resource's record, which carries the shell the account had before the mesh -// first changed it, so removal can give it back (novox/hq ADR 0176 §2, issue 225). +// first changed it, so removal can give it back (novox/hq ADR 0176 §2, issue 228). func applyUser(ctx context.Context, sys system.System, r *declaration.User, run Runner, previous store.Applied) (Outcome, error) { out := begin(r) @@ -44,7 +44,7 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run return out, err } - // **A shell is refused before anything is touched** (novox/hq issue 225). Refused after the + // **A shell is refused before anything is touched** (novox/hq issue 228). Refused after the // account was created or its groups changed, the account would be half the declaration's; a // refusal fails this resource and leaves the account exactly as it was. if r.Shell != "" && (!exists || login.Shell != r.Shell) { @@ -127,7 +127,7 @@ func applyUser(ctx context.Context, sys system.System, r *declaration.User, run } // removeUser is what undeclaring a login does: never deleting the account, and giving back the -// shell the mesh replaced when that is still safe (novox/hq ADR 0176 §2, issue 225). +// shell the mesh replaced when that is still safe (novox/hq ADR 0176 §2, issue 228). // // **The account is never deleted, whether or not the mesh created it.** An account owns a home, // files, a crontab, a mailbox — what a person did with it is not the mesh's to know, and deleting diff --git a/internal/apply/user_test.go b/internal/apply/user_test.go index 401461f..e6e81a9 100644 --- a/internal/apply/user_test.go +++ b/internal/apply/user_test.go @@ -12,7 +12,7 @@ import ( "github.com/novox/mesh-host/internal/system" ) -// Defends novox/hq ADR 0176 §2 and issue 225: a login the mesh set is given back when its holding +// Defends novox/hq ADR 0176 §2 and issue 228: a login the mesh set is given back when its holding // moves, undeclaring one never stops the node applying, and a shell is checked before it is set. // logins is a fake user database: each account's shell by name, and every command it was asked. @@ -96,7 +96,7 @@ func userWith(shell string) string { } func TestAnUndeclaredUserNoLongerStopsTheApply(t *testing.T) { - // Before issue 225 the host had no removal for a user, the orphan failed with "no way to + // Before issue 228 the host had no removal for a user, the orphan failed with "no way to // remove", and an orphan's failure aborts the apply before its first resource — on every // apply after, since the record stayed. dir := shellsOn(t, []string{"bash", "zsh"}) diff --git a/internal/store/store.go b/internal/store/store.go index 8b01bf7..05d9c92 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -96,7 +96,7 @@ type Applied struct { Into *Into `json:"into,omitempty"` // Shell is, for a user, the login shell the account had before the mesh first set one, and - // the shell the mesh set last (novox/hq ADR 0176 §2, issue 225). Removal gives the found shell + // the shell the mesh set last (novox/hq ADR 0176 §2, issue 228). Removal gives the found shell // back, and only while the account still has the one the mesh set: a shell a person chose since // is theirs. Absent when the mesh never changed the shell, and on a record written before the // host kept it — then the shell is left exactly as it is. diff --git a/internal/system/system.go b/internal/system/system.go index aac4b14..7669917 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -145,7 +145,7 @@ func ShellsIn(list string) (restore func()) { // warns about a shell that is missing or not executable, and succeeds; the host's read-back // compares the user database's string, which then matches. So an account could be pointed at a // shell that is not there, and console, ssh and display-manager logins all fail — after a -// failed package install, say, which does not stop the resources after it (novox/hq issue 225). +// failed package install, say, which does not stop the resources after it (novox/hq issue 228). // // Listed among the machine's shells as well as executable, because that list is what login // services check: an unlisted shell is one ssh and the display manager may refuse.