From 3d90ac87f1a8fcae285541f437f28d33b2195a04 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 09:49:59 +0200 Subject: [PATCH] ssh install tells a missing .ssh from one it cannot enter (closes #10) The first sftp session now lists .ssh before fetching authorized_keys. The file reads as empty only when sftp reports .ssh itself as missing, or the listing succeeded and the file is reported missing. A directory or file that is there but cannot be read fails the run and nothing is written, so no existing authorized_keys is replaced by content that was not built from what was read. An .ssh that already exists keeps its mode; the directory is made and set to 0700 only when none was found. The README describes the rule and states batch mode's limit: a key or an agent must authenticate. Model: opus-4-8 (implementation); fable-5-1 (summary) --- README.md | 32 +++---- internal/cli/ssh/install.go | 107 ++++++++++++++++++----- internal/cli/ssh/install_test.go | 55 ++++++++++++ internal/cli/ssh_test.go | 143 ++++++++++++++++++++++++++----- 4 files changed, 279 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index 6312e75..aa78dc6 100644 --- a/README.md +++ b/README.md @@ -95,22 +95,24 @@ Adds the `pub` line to `~/.ssh/authorized_keys` on the host. No command is run on the host: the file is fetched, changed here, and written back with the system `sftp` client in batch mode. -The first connection fetches `~/.ssh/authorized_keys`. The file reads as empty -only when `sftp` reported that file as not being there — the one line naming -that path. The same wording anywhere else in the session does not count: `ssh` -writes `No such file or directory` about an `-i` it cannot find, on a session -that then authenticates through the agent. When `sftp` failed for any other -reason — the file is there and cannot be read, the connection did not come up — -the tool prints what `sftp` said and exits with status 1 without writing -anything, rather than put a file back holding the new key alone. What `sftp` -cannot tell apart is a missing file and one in a directory it cannot enter, so a -`~/.ssh` whose mode shuts the user out reads as a host with no file; the second -connection sets that mode to `0700` and writes, as on a host that has none. If -an identical line is already in the file, the tool prints -`already present` and connects no further. Otherwise the line is added (after a -newline, if the file did not end with one) and a second connection: +The first connection lists `~/.ssh` and then fetches +`~/.ssh/authorized_keys` from it. The file reads as empty in two cases only: +`sftp` reported `~/.ssh` itself as not being there, or the listing came up and +the file was not in it. Any other outcome of that connection fails the run — a +`~/.ssh` that is there but cannot be entered, an `authorized_keys` that is there +but cannot be read, or a connection that did not come up — and the tool prints +what `sftp` said and exits with status 1 without writing anything, rather than +put a file back holding the new key alone. The listing is what tells a missing +directory from one shut to the user, which `sftp` reports on a fetch the same +way; the wording of a missing file elsewhere does not count either, since `ssh` +writes `No such file or directory` about an `-i` it cannot find on a session +that then authenticates through the agent. If an identical line is already in +the file, the tool prints `already present` and connects no further. Otherwise +the line is added (after a newline, if the file did not end with one) and a +second connection: -- creates `~/.ssh` and sets it to mode `0700`; +- makes `~/.ssh` and sets it to mode `0700`, but only when the first connection + found none; a `~/.ssh` that was already there keeps the mode it had; - uploads the new file as `~/.ssh/authorized_keys.keyfunc-` and sets it to mode `0600`; - renames that file over `~/.ssh/authorized_keys`. diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 5886a69..0a6cb1a 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -76,7 +76,7 @@ func add(cmd *cobra.Command, host string, options []string, line string) error { defer func() { _ = os.RemoveAll(work) }() - content, err := fetch(cmd, host, options, + content, present, err := fetch(cmd, host, options, filepath.Join(work, "authorized_keys"), ) if err != nil { @@ -88,16 +88,18 @@ func add(cmd *cobra.Command, host string, options []string, line string) error { return write(cmd, "already present\n") } - return upload(cmd, host, options, work, merged) + return upload(cmd, host, options, work, merged, present) } // upload writes the new file to the host and renames it over // authorized_keys, which is the step that either happens or does not. // Nothing is removed when a step fails: the file left behind is named -// so that it can be looked at and cleared away by hand. +// so that it can be looked at and cleared away by hand. The directory +// is made and set to its mode only when the read found none: an .ssh +// that was already there is left with the mode it had. func upload( cmd *cobra.Command, host string, options []string, - work, merged string, + work, merged string, present bool, ) error { local := filepath.Join(work, "authorized_keys.merged") @@ -111,14 +113,24 @@ func upload( return err } - // The mkdir may fail: the directory is usually there already. - said, err := session(cmd, host, options, []string{ - "-mkdir " + directory, - "chmod " + directoryMode + " " + directory, - "put " + quoted(local) + " " + sidecar, - "chmod " + fileMode + " " + sidecar, - "rename " + sidecar + " " + authorized, - }) + var batch []string + + if !present { + // The mkdir is allowed to fail in case the directory appeared + // between the read and now; the chmod then sets its mode. + batch = append(batch, + "-mkdir "+directory, + "chmod "+directoryMode+" "+directory, + ) + } + + batch = append(batch, + "put "+quoted(local)+" "+sidecar, + "chmod "+fileMode+" "+sidecar, + "rename "+sidecar+" "+authorized, + ) + + said, err := session(cmd, host, options, batch) if err != nil { // sftp echoes each command as it runs it and stops at the // first that fails, so the name is in what it said only once @@ -185,31 +197,82 @@ func merge(content, line string) (string, bool) { } // fetch brings the host's authorized_keys into the given path and -// returns what is in it. A host that has no such file reads as empty, -// but only when that is what sftp said about it: a file that is there -// and cannot be read fails the run, because writing back over it -// would leave the host with the new key and nothing else. +// returns what is in it, and whether the .ssh directory was already +// there. The one session lists .ssh and then gets the file, so the +// listing settles the state of the directory before the get is read. +// +// The file reads as empty in just two cases: sftp reported .ssh itself +// as not there, or the listing succeeded and the get then reported the +// file as not there. Anything else — the listing refused, the file +// there but unreadable, the connection down — fails the run and writes +// nothing, because writing back over what was not read would leave the +// host with the new key and nothing else. sftp cannot tell a missing +// file from one in a directory it cannot enter, so the listing does: +// a directory that is there but cannot be read is a failure, not an +// empty file. func fetch( cmd *cobra.Command, host string, options []string, into string, -) (string, error) { +) (string, bool, error) { said, err := session(cmd, host, options, []string{ + "ls -1 " + directory, "get " + authorized + " " + quoted(into), }) if err != nil { - if absent(said) { - return "", nil + if directoryAbsent(said) { + return "", false, nil } - return "", err + if absent(said) { + return "", true, nil + } + + return "", false, err } //nolint:gosec // the path is a temporary file of the tool's own content, err := os.ReadFile(into) if err != nil { - return "", fmt.Errorf("reading the fetched file: %w", err) + return "", false, fmt.Errorf("reading the fetched file: %w", err) } - return string(content), nil + return string(content), true, nil +} + +// directoryAbsent says whether sftp reported .ssh itself as not being +// there, which is the one listing failure read as a host that has no +// authorized_keys yet. The reading is taken only from the line in which +// sftp reports on that directory: any other failure of the listing, in +// particular a directory that is there but cannot be entered, is left +// as a failure, so that no key is written to a host whose keys were +// never read. +func directoryAbsent(said string) bool { + for line := range strings.Lines(said) { + named, is := reportedCannotList(strings.TrimSpace(line)) + if is && (named == directory || + strings.HasSuffix(named, "/"+directory)) { + return true + } + } + + return false +} + +// reportedCannotList returns the path an sftp line reports it cannot +// list for want of the directory, and whether the line is such a +// report. The client writes this one wording when the directory a +// listing names is not there, giving the path the server expanded. +func reportedCannotList(line string) (string, bool) { + const ( + before = `Can't ls: "` + after = `" not found` + ) + + if !strings.HasPrefix(line, before) || + !strings.HasSuffix(line, after) { + return "", false + } + + return strings.TrimSuffix(strings.TrimPrefix(line, before), after), true } // absent says whether sftp reported the file that was asked for as diff --git a/internal/cli/ssh/install_test.go b/internal/cli/ssh/install_test.go index 051014c..09d53de 100644 --- a/internal/cli/ssh/install_test.go +++ b/internal/cli/ssh/install_test.go @@ -10,6 +10,7 @@ import "testing" const ( echoed = `sftp> get .ssh/authorized_keys "/tmp/keyfunc/authorized_keys" ` + listed = "sftp> ls -1 .ssh\n" warning = `Warning: Identity file /gone not accessible: ` + "No such file or directory.\n" ) @@ -76,3 +77,57 @@ func TestAbsenceIsReadOnlyFromWhatSFTPSaidAboutAuthorizedKeys(t *testing.T) { }) } } + +// TestTheDirectoryIsReadAsAbsentOnlyFromTheListingSayingSo holds the +// wordings the OpenSSH client was seen to use when a listing fails: a +// directory it cannot find is reported one way, and one it cannot enter +// another, and only the first is read as a host with no .ssh yet. +func TestTheDirectoryIsReadAsAbsentOnlyFromTheListingSayingSo(t *testing.T) { + t.Parallel() + + listings := map[string]struct { + said string + want bool + }{ + "the directory is not there": { + said: listed + `Can't ls: "/home/someone/.ssh" not found` + "\n", + want: true, + }, + "the directory is not there, named as it was asked for": { + said: listed + `Can't ls: ".ssh" not found` + "\n", + want: true, + }, + "the directory is not there and an identity file is not either": { + said: warning + listed + + `Can't ls: "/home/someone/.ssh" not found` + "\n", + want: true, + }, + "the directory is there and cannot be entered": { + said: listed + + `remote readdir("/home/someone/.ssh/"): Permission denied` + "\n", + want: false, + }, + "some other directory is not there": { + said: listed + `Can't ls: "/home/someone/.config" not found` + "\n", + want: false, + }, + "the connection did not come up": { + said: "ssh: connect to host example.com port 22: " + + "Connection refused\nConnection closed\n", + want: false, + }, + } + + for name, listing := range listings { + t.Run(name, func(t *testing.T) { + t.Parallel() + + if directoryAbsent(listing.said) != listing.want { + t.Errorf( + "read as absent: %t, wanted %t, from:\n%s", + !listing.want, listing.want, listing.said, + ) + } + }) + } +} diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 9aafe21..9fd4ae9 100644 --- a/internal/cli/ssh_test.go +++ b/internal/cli/ssh_test.go @@ -68,13 +68,18 @@ const keyLine = vectorZero + " keyfunc/ssh/0\n" // work. A command that begins with a dash may fail; any other failure // ends the session, as it does in sftp's own batch mode. // -// The two ways a get can fail are worded as the OpenSSH client words -// them, both naming the path the server expanded: a file that is not -// there, which is the one failure the tool reads as an empty file, and -// a file that is there and cannot be read, which is not. An -i naming -// a file that is not here draws the warning ssh writes for it, which -// carries the wording of a missing file into a session that goes on to -// authenticate. +// The listing and the two ways a get can fail are worded as the +// OpenSSH client words them, each naming the path the server expanded. +// A listing fails one way when .ssh is not there and another when it is +// there but shut to the user; the first is the only failure read as a +// host with no file. A get fails one way for a file that is not there, +// which after a listing that came up empty is also read as no file, and +// another for a file that is there and cannot be read, which is a +// failure. A directory shut to the user is stood in for by mode 000, +// which the listing reads off the mode itself so that the test does not +// turn on the user it runs as. An -i naming a file that is not here +// draws the warning ssh writes for it, which carries the wording of a +// missing file into a session that goes on to authenticate. const installer = ` previous= for argument in "$@"; do @@ -99,6 +104,23 @@ while IFS= read -r line; do eval "set -- $line" worked=yes case "$1" in + ls) + dir=$2 + [ "$dir" = -1 ] && dir=$3 + if [ ! -e "$home/$dir" ]; then + worked=no + printf 'Can'\''t ls: "%s" not found\n' "$home/$dir" >&2 + elif [ -d "$home/$dir" ] && [ "$(stat -c '%a' "$home/$dir")" = 0 ]; then + worked=no + printf 'remote readdir("%s/"): Permission denied\n' \ + "$home/$dir" >&2 + else + for entry in "$home/$dir"/*; do + [ -e "$entry" ] || continue + printf '%s/%s\n' "$dir" "$(basename "$entry")" + done + fi + ;; get) if [ ! -e "$home/$2" ]; then worked=no @@ -176,8 +198,8 @@ func TestAKeyThatIsAlreadyThereIsLeftAlone(t *testing.T) { require.Equal(t, "already present\n", install(t, host)) require.Equal(t, "somebody else\n"+keyLine, read(t, path)) - // The fetch and nothing after it: the tool did not connect again. - require.Len(t, recorded(t, pretend.batch), 1) + // The read and nothing after it: the tool did not connect again. + require.Equal(t, 1, connections(t, pretend)) } func TestAnEmptyFileGetsTheKeyAndNoBlankLineBeforeIt(t *testing.T) { @@ -218,7 +240,9 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) { strings.HasPrefix(beside, ".ssh/authorized_keys.keyfunc-"), ) - require.True(t, strings.HasPrefix(sent[0], "get .ssh/authorized_keys ")) + // The listing fails on a host with no .ssh, so the get never runs; + // the write session then makes the directory and puts the file. + require.Equal(t, "ls -1 .ssh", sent[0]) require.Equal(t, "-mkdir .ssh", sent[1]) require.Equal(t, "chmod 700 .ssh", sent[2]) require.Equal(t, "put", strings.Fields(sent[3])[0]) @@ -237,12 +261,57 @@ func TestAFileThatCannotBeReadIsNotWrittenOver(t *testing.T) { require.Empty(t, printed) require.Contains(t, said, "Permission denied") - // The fetch and nothing after it, and what was on the host is + // The read and nothing after it, and what was on the host is // still what is on the host. - require.Len(t, recorded(t, pretend.batch), 1) + require.Equal(t, 1, connections(t, pretend)) require.DirExists(t, unreadable) } +func TestAnUnreadableDirectoryIsNotWrittenInto(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + pretend := pretendHost(t) + unlistable(t, pretend) + + // The listing is refused, which is not the same as no directory, so + // the tool writes nothing rather than treat a directory it cannot + // enter as a host with no file. + printed, said, err := attempt(t, host) + require.Error(t, err) + require.Empty(t, printed) + require.Contains(t, said, "Permission denied") + + // The read and nothing after it: no second connection wrote a key. + require.Equal(t, 1, connections(t, pretend)) +} + +func TestAnExistingDirectoryKeepsItsModeAndIsNotRemade(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + pretend := pretendHost(t) + + // A directory that is there but holds no file yet, made with a mode + // of its own so that a stray chmod would show. + const ownMode = 0o755 + + directory := filepath.Join(pretend.home, keptUnder) + require.NoError(t, os.Mkdir(directory, ownMode)) + + require.Equal(t, "added\n", install(t, host)) + + // The key is added and the directory keeps the mode it had: the + // write session neither made it nor set its mode. + require.Equal(t, keyLine, read(t, filepath.Join(directory, keptIn))) + + kept, err := os.Stat(directory) + require.NoError(t, err) + require.Equal(t, os.FileMode(ownMode), kept.Mode().Perm()) + + sent := recorded(t, pretend.batch) + require.NotContains(t, sent, "-mkdir .ssh") + require.NotContains(t, sent, "chmod 700 .ssh") +} + func TestAWarningAboutAnotherFileIsNotTakenForTheOneAskedFor(t *testing.T) { t.Setenv(mnemonic.Variable, example()) @@ -259,7 +328,7 @@ func TestAWarningAboutAnotherFileIsNotTakenForTheOneAskedFor(t *testing.T) { require.Contains(t, said, "No such file or directory") require.Contains(t, said, "Permission denied") - require.Len(t, recorded(t, pretend.batch), 1) + require.Equal(t, 1, connections(t, pretend)) require.DirExists(t, unreadable) // The same run again, this way for the status it ends with. @@ -279,9 +348,9 @@ func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) { pretend := pretendHost(t) - // A file where the .ssh directory belongs: nothing is there to - // fetch, and then the put has nowhere to put anything, so the - // write session ends at the put. + // A file where the .ssh directory belongs: the listing shows it and + // so the directory reads as already there, but then the put has + // nowhere to put anything, so the write session ends at the put. inTheWay := filepath.Join(pretend.home, keptUnder) require.NoError(t, os.WriteFile(inTheWay, []byte(notADirectory), fileMode), @@ -292,12 +361,12 @@ func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) { require.Empty(t, printed) require.Contains(t, said, "put failed") - // The put is the last command the session got to, and the file it - // was uploading is the one the message names. + // The put is the first and last command the write session got to, + // and the file it was uploading is the one the message names. sent := recorded(t, pretend.batch) - require.Len(t, sent, 4) - require.Equal(t, "put", strings.Fields(sent[3])[0]) - require.Contains(t, err.Error(), strings.Fields(sent[3])[2]) + require.Len(t, sent, 3) + require.Equal(t, "put", strings.Fields(sent[2])[0]) + require.Contains(t, err.Error(), strings.Fields(sent[2])[2]) require.Equal(t, notADirectory, read(t, inTheWay)) @@ -455,6 +524,38 @@ func unfetchable(t *testing.T, pretend pretended) string { return path } +// unlistable puts a .ssh on the stand-in host that is there but shut to +// the user, a directory of mode 000, and gives back its path. Its mode +// is put back before the temporary directory is cleared so that it can +// be. +func unlistable(t *testing.T, pretend pretended) string { + t.Helper() + + directory := filepath.Join(pretend.home, keptUnder) + require.NoError(t, os.Mkdir(directory, directoryMode)) + require.NoError(t, os.Chmod(directory, 0)) + + t.Cleanup(func() { _ = os.Chmod(directory, directoryMode) }) + + return directory +} + +// connections returns how many times the tool ran sftp, counted from +// the -b that opens each session's arguments. +func connections(t *testing.T, pretend pretended) int { + t.Helper() + + count := 0 + + for _, argument := range recorded(t, pretend.arguments) { + if argument == "-b" { + count++ + } + } + + return count +} + // pretendCall puts the to stand-in on the path and gives back the file // the arguments are written down in and the file the agent socket is // noted in.