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() {