From 05e6b583e2e3a98d4afc111e67cafca23321c1e9 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Sat, 17 Oct 2026 13:36:27 +0000 Subject: [PATCH] Keep what ssh reads itself and linked key folders in ~/.ssh, and fail the sweep loudly (issue 1020, ADR 0417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep from issue 1017 moved known_hosts2, rc and environment, which ssh and sshd read by default, and moved a link to a folder of keys, breaking every login through it. It could move a file a tool was still writing. When ~/.local was on another file system, every sweep failed at its first move and left an empty folder each round, seen only in the journal. Its list was written after the moves with errors ignored. Now those files and linked key folders stay; a link whose target, or the folder it names, cannot be checked is left and named; a copy of authorized_keys is moved at any depth; a path changed in the last minute, at any depth, waits for the next sweep; a retired folder on another file system fails the sweep before anything is made; the list names each move before it is made; and each sweep's outcome is kept for ssh_client_check, which shows a failed sweep as a problem. A key's first line is read only so far. The catalogue's new checks/sshdir refuses any manifest but ssh-client naming a path under ~/.ssh (ADR 0415 §6). The README cites §7 for config.d. --- checks/sshdir/go.mod | 3 + checks/sshdir/main.go | 112 ++++++++ checks/sshdir/main_test.go | 48 ++++ merge-check.sh | 4 + modules/ssh-client/README.md | 23 +- .../ssh-client/cmd/ssh-client-tools/client.go | 23 +- .../cmd/ssh-client-tools/ssh_test.go | 32 +++ .../ssh-client/cmd/ssh-client-tools/sweep.go | 251 +++++++++++++++--- .../cmd/ssh-client-tools/sweep_test.go | 239 +++++++++++++++++ 9 files changed, 702 insertions(+), 33 deletions(-) create mode 100644 checks/sshdir/go.mod create mode 100644 checks/sshdir/main.go create mode 100644 checks/sshdir/main_test.go diff --git a/checks/sshdir/go.mod b/checks/sshdir/go.mod new file mode 100644 index 00000000..fb15b012 --- /dev/null +++ b/checks/sshdir/go.mod @@ -0,0 +1,3 @@ +module git.novox.be/novox/mesh-catalog/checks/sshdir + +go 1.22 diff --git a/checks/sshdir/main.go b/checks/sshdir/main.go new file mode 100644 index 00000000..0b7eb4ab --- /dev/null +++ b/checks/sshdir/main.go @@ -0,0 +1,112 @@ +// Command sshdir refuses any manifest but modules/ssh-client naming a path under ~/.ssh (novox/hq ADR 0415 §6, +// issue 1020): ssh-client owns ~/.ssh and sweeps what it does not know, so a path another module placed there +// would be moved out at the next sweep. A module that needs something there provides it to ssh-client instead. +// +// go run . +// +// Every string of a manifest is read, wherever it stands, except prose and a file's text (Prose): a path, a +// place, a volume. Exits 1 on a finding. +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "regexp" + "sort" +) + +// SSHModule is the one module allowed to name a path under ~/.ssh. +const SSHModule = "ssh-client" + +// underSSH is a path under ~/.ssh, or ~/.ssh itself, however the home is spelled: ${machine:account-home}/.ssh, +// ~/.ssh, .ssh/config, or a volume's host side (…/.ssh:/root/.ssh). +var underSSH = regexp.MustCompile(`(^|[/~])\.ssh($|[/:])`) + +// Prose are the keys whose strings are words or a file's text, not a path: a template may say ~/.ssh in a comment. +var Prose = map[string]bool{"description": true, "why": true, "does": true, "template": true, "content": true, "title": true} + +// Finding is one path under ~/.ssh in a manifest other than ssh-client's. +type Finding struct { + Module string + Key string + Value string +} + +// Check reads every manifest under root/modules and answers which paths it refuses. +func Check(root string) ([]Finding, error) { + manifests, err := filepath.Glob(filepath.Join(root, "modules", "*", "module.json")) + if err != nil { + return nil, err + } + sort.Strings(manifests) + var out []Finding + for _, path := range manifests { + raw, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var m any + if err := json.Unmarshal(raw, &m); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + module := filepath.Base(filepath.Dir(path)) + if obj, ok := m.(map[string]any); ok { + if name, ok := obj["module"].(string); ok && name != "" { + module = name + } + } + if module == SSHModule { + continue + } + walk(m, "", func(key, value string) { + out = append(out, Finding{Module: module, Key: key, Value: value}) + }) + } + return out, nil +} + +// walk calls found for every string under v, outside Prose, that names a path under ~/.ssh. +func walk(v any, key string, found func(key, value string)) { + switch x := v.(type) { + case map[string]any: + keys := make([]string, 0, len(x)) + for k := range x { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + if !Prose[k] { + walk(x[k], k, found) + } + } + case []any: + for _, e := range x { + walk(e, key, found) + } + case string: + if underSSH.MatchString(x) { + found(key, x) + } + } +} + +func main() { + root := "." + if len(os.Args) > 1 { + root = os.Args[1] + } + found, err := Check(root) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + for _, f := range found { + fmt.Printf("%s names %q (%s) under ~/.ssh: only %s may, and the sweep moves anything else out (novox/hq ADR 0415 §6)\n", f.Module, f.Value, f.Key, SSHModule) + } + if len(found) > 0 { + os.Exit(1) + } + fmt.Println("~/.ssh: named only by ssh-client") +} diff --git a/checks/sshdir/main_test.go b/checks/sshdir/main_test.go new file mode 100644 index 00000000..ca78b3e7 --- /dev/null +++ b/checks/sshdir/main_test.go @@ -0,0 +1,48 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func manifest(t *testing.T, root, module, body string) { + t.Helper() + dir := filepath.Join(root, "modules", module) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "module.json"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestAnotherModuleNamingSSHIsRefused(t *testing.T) { + root := t.TempDir() + manifest(t, root, "ssh-client", `{"module":"ssh-client","resources":[{"path":"${machine:account-home}/.ssh/config"}]}`) + manifest(t, root, "forge-account", `{"module":"forge-account","resources":[{"path":"${machine:account-home}/.ssh/config.d/20-forge"}]}`) + manifest(t, root, "roster", `{"module":"roster","facts":{"hosts":{"path":".ssh/known_hosts"}}}`) + manifest(t, root, "box", `{"module":"box","containers":[{"volumes":["~/.ssh:/root/.ssh:ro"]}]}`) + manifest(t, root, "quiet", `{"module":"quiet","description":"reads ~/.ssh/config","resources":[{"path":"${machine:account-home}/.sshrc-not","template":"# see ~/.ssh/config"},{"path":"/etc/ssh/sshd_config"}]}`) + found, err := Check(root) + if err != nil { + t.Fatal(err) + } + got := map[string]bool{} + for _, f := range found { + got[f.Module] = true + } + if len(found) != 3 || !got["forge-account"] || !got["roster"] || !got["box"] { + t.Fatalf("refused %+v; want forge-account, roster and box", found) + } +} + +func TestTheCatalogueHasNone(t *testing.T) { + found, err := Check("../..") + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf("the catalogue names paths under ~/.ssh outside ssh-client: %+v", found) + } +} diff --git a/merge-check.sh b/merge-check.sh index 0e4e4cdc..8afe1cc1 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -45,6 +45,8 @@ # unless checks/templates/inline lists it as not yet moved; each names its template in files/. That list # only shrinks, judged against the trunk's copy at the merge base. # 10. the mail-store seat (novox/hq ADR 0378 §3, issue 800): no manifest but modules/mesh-mail claims it. +# 11. ~/.ssh (novox/hq ADR 0415 §6, issue 1020): no manifest but modules/ssh-client names a path under it — +# ssh-client owns it and sweeps out what it does not know. set -eu (cd checks/words && go test -count=1 . && go run . ../..) @@ -52,6 +54,8 @@ set -eu (cd checks/agenthome && go test -count=1 . && go run . ../..) # The mail-store seat (novox/hq ADR 0378 §3): only mesh-mail may claim it. (cd checks/mailstore && go test -count=1 . && go run . ../..) +# ~/.ssh (novox/hq ADR 0415 §6): only ssh-client may name a path under it. +(cd checks/sshdir && go test -count=1 . && go run . ../..) # The secrets (novox/hq issue 342): a password is not a setting, so no setting can replace the vault's. The # known list only shrinks: judged against the trunk's copy at the merge base, a line it does not hold is refused. secrets_base="" diff --git a/modules/ssh-client/README.md b/modules/ssh-client/README.md index 8b92fd9c..301cd579 100644 --- a/modules/ssh-client/README.md +++ b/modules/ssh-client/README.md @@ -8,13 +8,14 @@ item 9). `sshd` is the machine's side. | path | class | how | |---|---|---| | `~/.ssh/` | owned | directory, the account's, mode 0700 | -| `~/.ssh/config.d/` | owned | directory, the account's, mode 0700. It holds only the files this module declares (ADR 0415 §6). As every module's owned path, ssh-client is the only module that writes under `~/.ssh`: another module that needs a host there provides it through a provision, and this module writes it (ADR 0349 §1; the forge's host comes from `forge-account`). A host of the operator's own goes in `~/.ssh/config` below the mesh's region | +| `~/.ssh/config.d/` | owned | directory, the account's, mode 0700. Apart from keys, it holds only the files this module declares (ADR 0415 §7). As every module's owned path, ssh-client is the only module that writes under `~/.ssh`: another module that needs a host there provides it through a provision, and this module writes it (ADR 0349 §1; the forge's host comes from `forge-account`). A host of the operator's own goes in `~/.ssh/config` below the mesh's region | | `~/.ssh/config.d/10-forge` | owned | the forge's host, with the machine's own key (ADR 0399) | | `~/.ssh/mesh_ed25519`, `.pub` | written by the module's own process | the machine's account's key for the forge, made once (keykeeper.go) | | `~/.ssh/config.d/00-mesh` | owned | a Host block per other machine of the mesh (a roster fact), regenerated whenever a machine joins, leaves or is renamed. It begins with the mesh's header | | `~/.ssh/config` | written into, **at the start** | the mesh's region, `# BEGIN mesh ssh-client.config` … `# END …`. It holds one `Include ~/.ssh/config.d/*`. Every line below the region is the operator's, kept byte for byte | | `~/.ssh/authorized_keys` | found | see *The gap* | | `~/.ssh/known_hosts` | found | see *The gap* | +| `~/.ssh/authorized_keys2`, `known_hosts2`, `rc`, `environment` | kept | the files ssh and sshd read by themselves; this module writes none of them and never moves them (ADR 0415 §1, ADR 0417 §1) | | private keys, public keys, certificates | kept | never read, never written, never moved, at any depth and whatever their name. `ssh_client_keys` and `ssh_client_check` read a file's first line to recognise a key, and ask `ssh-keygen` about it | | anything else | swept | moved out of `~/.ssh` by the sweep (below) | @@ -68,6 +69,23 @@ of what moved is written beside it as `.json`. Nothing is deleted. A f is gone into rather than moved whole, so the key stays where it is. A file or folder that cannot be read to tell whether it holds a key is left and named in the log. +ADR 0417 (issue 1020) adds: + +- the files ssh and sshd read by themselves (`authorized_keys2`, `known_hosts2`, `rc`, `environment`) stay; +- a symbolic link to a key, or to a folder holding one, stays; a link whose target cannot be checked is left + and named; any other link is moved as the link; +- a path changed less than a minute ago is being written (ssh-keygen's or `ssh_client_revoke`'s temporary + file) and waits for the next sweep; +- the retired folder must be on `~/.ssh`'s file system: otherwise the sweep moves nothing, makes no + folder and fails. A sweep that moved nothing leaves no folder; +- the list names each move before it is made, and a list that cannot be written stops the sweep; +- each sweep writes what it did, or why it failed, to `~/.local/state/ssh-client/last-sweep.json`. + `ssh_client_check` reports a failed last sweep, and a listed move with nothing at its destination, as a + problem. + +No other module declares a path under `~/.ssh` (ADR 0415 §6): the catalogue's `checks/sshdir` fails one +that does. + So the predecessor's `config.d/mesh`, `config.bak-*`, `known_hosts.old`, `removed-*` and `authorized_keys.revoked-*` leave `~/.ssh` at the first sweep. Each move is logged by the process, and `ssh_client_check` lists every sweep that moved something, with where each path lies now. @@ -110,4 +128,7 @@ The tests use a temporary home and a fake runner, with public keys made for the - `known_host` matching, changed and refreshed, and an unreachable host never refreshed; - `test`: success, and failure with the agent named; - `check`'s findings; +- the sweep: what it keeps (keys at any depth, links to keys and to key folders, sockets, what ssh reads + itself), what it moves, what it leaves and names, a file being written, another file system, and the + list written before each move; - that the tools served are the manifest's `tools`. diff --git a/modules/ssh-client/cmd/ssh-client-tools/client.go b/modules/ssh-client/cmd/ssh-client-tools/client.go index 7e1398ad..454de9dd 100644 --- a/modules/ssh-client/cmd/ssh-client-tools/client.go +++ b/modules/ssh-client/cmd/ssh-client-tools/client.go @@ -8,6 +8,7 @@ import ( "bufio" "context" "fmt" + "io" "io/fs" "os" "path/filepath" @@ -24,6 +25,8 @@ type Client struct { UID int Run Runner Now func() time.Time + // Settle is how long a path in ~/.ssh is left by the sweep after it changed (sweep.go): zero in tests. + Settle time.Duration } // NewClient is the client the bundle serves with: the operator's home as the runtime names it. @@ -32,7 +35,7 @@ func NewClient() *Client { if home == "" { home, _ = os.UserHomeDir() } - return &Client{Home: home, UID: os.Getuid(), Run: ExecRunner, Now: time.Now} + return &Client{Home: home, UID: os.Getuid(), Run: ExecRunner, Now: time.Now, Settle: time.Minute} } func (c *Client) ssh(name string) string { return filepath.Join(c.Home, ".ssh", name) } @@ -173,7 +176,7 @@ func header(path string) bool { return false } defer f.Close() - line, _ := bufio.NewReader(f).ReadString('\n') + line, _ := bufio.NewReader(io.LimitReader(f, 256)).ReadString('\n') // a key's first line is short; a large file's may not be return strings.HasPrefix(line, "-----BEGIN") && strings.Contains(line, "PRIVATE KEY-----") } @@ -813,6 +816,22 @@ func (c *Client) Check(ctx context.Context, scan bool) (map[string]any, error) { if err != nil { add("problem", filepath.Join(c.Home, RetiredDir), "cannot be read: %v", err) } + for _, r := range retired { + for _, m := range r.Moved { + if m.Missing { + add("problem", m.To, "the sweep of %s lists %s as moved here, but nothing is here: it may still be in ~/.ssh", r.At, m.From) + } + } + } + last, err := c.LastSwept() + switch { + case err != nil: + add("problem", filepath.Join(c.Home, LastSweepFile), "cannot be read: %v", err) + case last == nil: + notChecked = append(notChecked, "the sweep: it has not run on this account yet") + case last.Error != "": + add("problem", dir, "the last sweep (%s) failed: %s", last.At, last.Error) + } problems := 0 for _, x := range f { if x.Severity == "problem" { diff --git a/modules/ssh-client/cmd/ssh-client-tools/ssh_test.go b/modules/ssh-client/cmd/ssh-client-tools/ssh_test.go index 3ceccc65..763540cb 100644 --- a/modules/ssh-client/cmd/ssh-client-tools/ssh_test.go +++ b/modules/ssh-client/cmd/ssh-client-tools/ssh_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "sort" "strings" "sync" @@ -376,6 +377,37 @@ func TestCheckFindsWhatIsWrongAndSaysWhatItDidNotCheck(t *testing.T) { } } +// Issue 1020, ADR 0417: the check shows a failed last sweep and a listed move with nothing at its destination +// as problems, and says when no sweep has run. +func TestCheckShowsAFailedSweep(t *testing.T) { + h := home(t, map[string]string{"config": layout["config"], "known_hosts": ""}) + f := &fake{answer: func(call) Ran { return Ran{Status: 1} }} + c := client(h, f) + got, _ := c.Check(context.Background(), false) + if !slices.ContainsFunc(got["not_checked"].([]string), func(s string) bool { return strings.Contains(s, "the sweep") }) { + t.Errorf("not_checked = %v, want the sweep named as not run yet", got["not_checked"]) + } + if err := c.writeLastSweep(LastSweep{At: "20261017T120000Z", Error: "it broke"}); err != nil { + t.Fatal(err) + } + list := filepath.Join(h, RetiredDir, "20261001T000000Z.json") + os.MkdirAll(filepath.Dir(list), 0o700) + os.WriteFile(list, []byte(`[{"from":"`+filepath.Join(h, ".ssh", "x")+`","to":"`+filepath.Join(h, RetiredDir, "20261001T000000Z", "x")+`"}]`), 0o600) + got, _ = c.Check(context.Background(), false) + var whats []string + for _, x := range got["findings"].([]Finding) { + if x.Severity == "problem" { + whats = append(whats, x.What) + } + } + all := strings.Join(whats, "\n") + for _, want := range []string{"the last sweep (20261017T120000Z) failed: it broke", "nothing is here"} { + if !strings.Contains(all, want) { + t.Errorf("missing problem %q in:\n%s", want, all) + } + } +} + func TestTheToolsServedAreTheToolsTheManifestNames(t *testing.T) { raw, err := os.ReadFile("../../module.json") if err != nil { diff --git a/modules/ssh-client/cmd/ssh-client-tools/sweep.go b/modules/ssh-client/cmd/ssh-client-tools/sweep.go index 539bcddb..c297bbf5 100644 --- a/modules/ssh-client/cmd/ssh-client-tools/sweep.go +++ b/modules/ssh-client/cmd/ssh-client-tools/sweep.go @@ -5,7 +5,10 @@ package main // keeping its path relative to ~/.ssh. Nothing is deleted. A key (private, public or a certificate), // at any depth and whatever its name, and authorized_keys are never moved (design 29 §3: moving one // can lock the operator out); a folder holding a key is gone into rather than moved whole. A socket -// is ssh's own runtime and stays. The process sweeps when it starts and every KeyRound (main.go). +// is ssh's own runtime and stays, and so do the files ssh and sshd read by themselves (SSHReads). A +// path changed in the last Settle is being written (a temporary file of atomicWrite or ssh-keygen) and +// waits for the next sweep. The process sweeps when it starts and every KeyRound (main.go); what the +// last sweep did, or why it failed, is kept beside the retired folder for ssh_client_check (issue 1020). import ( "encoding/json" @@ -16,6 +19,7 @@ import ( "regexp" "sort" "strings" + "syscall" "time" ) @@ -30,20 +34,38 @@ var Declared = map[string]bool{ KeyFile: true, KeyFile + ".pub": true, "authorized_keys": true, - "authorized_keys2": true, // sshd's second AuthorizedKeysFile by default: moving it can lock the operator out } +// SSHReads are the files ssh and sshd read in ~/.ssh by themselves, though this module writes none of +// them (ssh(1), sshd(8) FILES): moving one loses a trust or a login's setup, or locks the operator out. +var SSHReads = map[string]bool{ + "authorized_keys2": true, // sshd's second AuthorizedKeysFile by default + "known_hosts2": true, // ssh's second UserKnownHostsFile by default + "rc": true, // run by sshd at every login + "environment": true, // read by sshd at login when PermitUserEnvironment allows +} + +// known is whether rel, relative to ~/.ssh, is a path this module declares or one ssh reads itself. +func known(rel string) bool { return Declared[rel] || SSHReads[rel] } + // copyOf is a copy of authorized_keys or known_hosts (authorized_keys.revoked-*, known_hosts.old): it -// carries key lines but is no key, and sshd and ssh never read it. +// carries key lines but is no key, and sshd and ssh never read it. It is matched on a name, at any depth. var copyOf = regexp.MustCompile(`^(authorized_keys|known_hosts)[^/]+$`) // RetiredDir is where the sweep moves what it does not know, under the account's home. const RetiredDir = ".local/state/ssh-client/retired" +// LastSweepFile is where each sweep writes what it did, or why it failed, under the account's home: +// the sweep runs in the background, so a failure that is only logged is quiet (issue 1020). +const LastSweepFile = ".local/state/ssh-client/last-sweep.json" + // Moved is one path a sweep moved out of ~/.ssh. type Moved struct { From string `json:"from"` To string `json:"to"` + // Missing is set by Retired when nothing lies at To: the sweep listed the move before making it and + // stopped between the two. + Missing bool `json:"missing,omitempty"` } // Sweep is what one sweep did: what it moved, and what it left because it could not tell what it is. @@ -53,6 +75,14 @@ type Sweep struct { Left []string `json:"left,omitempty"` } +// LastSweep is what the last sweep did, as LastSweepFile holds it. +type LastSweep struct { + At string `json:"at"` + Moved int `json:"moved"` + Left []string `json:"left,omitempty"` + Error string `json:"error,omitempty"` +} + // Sweep moves every path under ~/.ssh this module does not know into a retired folder named by now, // and writes beside that folder the list of what it moved (