ssh install works over sftp and runs nothing on the host (closes #10)

ssh install no longer runs a command on the host. It reads .ssh/authorized_keys over sftp, takes the empty reading only from sftp's own message about that path, appends the derived key locally when it is not already present, uploads the result beside the file with mode 0600 and renames it over the original. Any other failure prints what sftp said, writes nothing and exits 1. sftp batch mode disables password prompts, so a key or agent is required; a directory the owner cannot enter reads as a host with no file, which README.md states.

Model: opus-5 (implementation); fable-5-1 (landing)

README.md

@@ -95,11 +95,17 @@ 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` said there is no such file; when `sftp` failed for any other
-reason — the file is there but cannot be read, `~/.ssh` cannot be entered, 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. If an identical line is already in the file, the tool prints
+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:
 

internal/cli/ssh/install.go

@@ -212,15 +212,42 @@ func fetch(
 	return string(content), nil
 }
 
-// absent says whether what sftp said about the file it was asked for
-// is that there is no such file, which is the one failure of the
-// fetch that is read as an empty authorized_keys. sftp has spelled
-// that both ways; anything else it says is a failure.
+// absent says whether sftp reported the file that was asked for as
+// not being there, which is the one failure of the fetch that is read
+// as an empty authorized_keys. The reading is taken only from the
+// line in which sftp reports on that file, because ssh writes "no
+// such file" into the same output for reasons of its own — a missing
+// -i identity file draws that warning on a session that then
+// authenticates through the agent — and a real read failure on such a
+// session must not pass for an empty file.
 func absent(said string) bool {
-	lower := strings.ToLower(said)
+	for line := range strings.Lines(said) {
+		named, is := reportedNotFound(strings.TrimSpace(line))
+		if is && (named == authorized ||
+			strings.HasSuffix(named, "/"+authorized)) {
+			return true
+		}
+	}
 
-	return strings.Contains(lower, "no such file") ||
-		strings.Contains(lower, "not found")
+	return false
+}
+
+// reportedNotFound returns the path an sftp line reports as not being
+// there, and whether the line is such a report. The client writes one
+// wording for a remote file it cannot find, naming the path the
+// server expanded, which is the absolute one.
+func reportedNotFound(line string) (string, bool) {
+	const (
+		before = `File "`
+		after  = `" not found.`
+	)
+
+	if !strings.HasPrefix(line, before) ||
+		!strings.HasSuffix(line, after) {
+		return "", false
+	}
+
+	return strings.TrimSuffix(strings.TrimPrefix(line, before), after), true
 }
 
 // sidecarName returns the name the new file is uploaded under.

internal/cli/ssh/install_test.go

@@ -0,0 +1,78 @@
+//nolint:testpackage // absent is what these wordings are read by
+package ssh
+
+import "testing"
+
+// What a session says besides its report on the file that was asked
+// for: sftp echoes the command it is running, and ssh warns about an
+// identity file it cannot find in the words of a missing file even
+// though the session goes on to authenticate.
+const (
+	echoed = `sftp> get .ssh/authorized_keys "/tmp/keyfunc/authorized_keys"
+`
+	warning = `Warning: Identity file /gone not accessible: ` +
+		"No such file or directory.\n"
+)
+
+// TestAbsenceIsReadOnlyFromWhatSFTPSaidAboutAuthorizedKeys holds the
+// wordings the OpenSSH client was seen to use against a real server:
+// a file it cannot find is reported one way, naming the path the
+// server expanded, and everything else it says is a failure.
+func TestAbsenceIsReadOnlyFromWhatSFTPSaidAboutAuthorizedKeys(t *testing.T) {
+	t.Parallel()
+
+	sessions := map[string]struct {
+		said string
+		want bool
+	}{
+		"the file is not there": {
+			said: echoed +
+				`File "/home/someone/.ssh/authorized_keys" not found.` + "\n",
+			want: true,
+		},
+		"the file is not there, named as it was asked for": {
+			said: echoed + `File ".ssh/authorized_keys" not found.` + "\n",
+			want: true,
+		},
+		"the file is not there and an identity file is not either": {
+			said: warning + echoed +
+				`File "/home/someone/.ssh/authorized_keys" not found.` + "\n",
+			want: true,
+		},
+		"the file is there and cannot be read": {
+			said: echoed +
+				`remote open "/home/someone/.ssh/authorized_keys": ` +
+				"Permission denied\n",
+			want: false,
+		},
+		"only an identity file is not there": {
+			said: warning + echoed +
+				`remote open "/home/someone/.ssh/authorized_keys": ` +
+				"Permission denied\n",
+			want: false,
+		},
+		"some other file is not there": {
+			said: echoed + `File "/home/someone/.ssh/known_hosts" 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, session := range sessions {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			if absent(session.said) != session.want {
+				t.Errorf(
+					"read as absent: %t, wanted %t, from:\n%s",
+					!session.want, session.want, session.said,
+				)
+			}
+		})
+	}
+}

internal/cli/ssh_test.go

@@ -35,6 +35,11 @@ const (
 // to make a step of the write session fail.
 const notADirectory = "a file where the directory belongs\n"
 
+// missingIdentity is a path with no file at it, handed to sftp after
+// the dashes so that ssh warns about it in the words of a missing
+// file.
+const missingIdentity = "/nonexistent/keyfunc-test-identity"
+
 // The host, and where on it the key ends up.
 const (
 	host      = "someone@example.com"
@@ -42,9 +47,14 @@ const (
 	keptIn    = "authorized_keys"
 )
 
-// subcommand is the tool's ssh subcommand, which both commands the
-// tests here drive live under.
-const subcommand = "ssh"
+// The tool's own name, as it stands in the arguments a test hands to
+// Main, the ssh subcommand both commands the tests here drive live
+// under, and the one of those two these tests name most.
+const (
+	tool       = "keyfunc"
+	subcommand = "ssh"
+	installing = "install"
+)
 
 // The key line the example mnemonic gives at index 0, as it stands in
 // an authorized_keys file.
@@ -56,12 +66,24 @@ const keyLine = vectorZero + " keyfunc/ssh/0\n"
 // the commands out against a directory standing in for the host's
 // home directory, so that what keyfunc sends can be watched doing its
 // 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. A get of a
-// file that is not there says so in the words sftp uses for it, since
-// that is the one failure the tool reads as an empty file.
+// 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.
 const installer = `
+previous=
 for argument in "$@"; do
 	printf '%s\n' "$argument" >> "$KEYFUNC_TEST_ARGUMENTS"
+	if [ "$previous" = -i ] && [ ! -e "$argument" ]; then
+		printf 'Warning: Identity file %s not accessible: %s.\n' \
+			"$argument" "No such file or directory" >&2
+	fi
+	previous=$argument
 done
 home="$KEYFUNC_TEST_HOME"
 while IFS= read -r line; do
@@ -78,11 +100,12 @@ while IFS= read -r line; do
 	worked=yes
 	case "$1" in
 	get)
-		if [ -e "$home/$2" ]; then
-			cp "$home/$2" "$3" 2>/dev/null || worked=no
-		else
+		if [ ! -e "$home/$2" ]; then
 			worked=no
-			printf 'File "%s" not found.\n' "$2" >&2
+			printf 'File "%s" not found.\n' "$home/$2" >&2
+		elif ! cp "$home/$2" "$3" 2>/dev/null; then
+			worked=no
+			printf 'remote open "%s": Permission denied\n' "$home/$2" >&2
 		fi
 		;;
 	put) cp "$2" "$home/$3" 2>/dev/null || worked=no ;;
@@ -207,22 +230,12 @@ func TestAFileThatCannotBeReadIsNotWrittenOver(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	pretend := pretendHost(t)
-
-	// A directory where authorized_keys belongs: the stand-in can see
-	// it but cannot fetch it, which is how a file that is there and
-	// cannot be read looks from here. sftp fails without saying that
-	// there is no such file.
-	require.NoError(t,
-		os.Mkdir(filepath.Join(pretend.home, keptUnder), directoryMode),
-	)
-
-	unreadable := filepath.Join(pretend.home, keptUnder, keptIn)
-	require.NoError(t, os.Mkdir(unreadable, directoryMode))
+	unreadable := unfetchable(t, pretend)
 
 	printed, said, err := attempt(t, host)
 	require.Error(t, err)
 	require.Empty(t, printed)
-	require.Contains(t, said, "get failed")
+	require.Contains(t, said, "Permission denied")
 
 	// The fetch and nothing after it, and what was on the host is
 	// still what is on the host.
@@ -230,6 +243,37 @@ func TestAFileThatCannotBeReadIsNotWrittenOver(t *testing.T) {
 	require.DirExists(t, unreadable)
 }
 
+func TestAWarningAboutAnotherFileIsNotTakenForTheOneAskedFor(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+	unreadable := unfetchable(t, pretend)
+
+	// ssh warns about an -i it cannot find in the words of a missing
+	// file, on a session that then authenticates perfectly well. That
+	// warning is not sftp reporting on authorized_keys, so the fetch
+	// failure is still a failure.
+	printed, said, err := attempt(t, host, "--", "-i", missingIdentity)
+	require.Error(t, err)
+	require.Empty(t, printed)
+	require.Contains(t, said, "No such file or directory")
+	require.Contains(t, said, "Permission denied")
+
+	require.Len(t, recorded(t, pretend.batch), 1)
+	require.DirExists(t, unreadable)
+
+	// The same run again, this way for the status it ends with.
+	given := os.Args
+
+	t.Cleanup(func() { os.Args = given })
+
+	os.Args = []string{
+		tool, subcommand, installing, host, "--", "-i", missingIdentity,
+	}
+
+	require.Equal(t, failedStatus, cli.Main())
+}
+
 func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
@@ -262,7 +306,7 @@ func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{"keyfunc", subcommand, "install", host}
+	os.Args = []string{tool, subcommand, installing, host}
 
 	require.Equal(t, failedStatus, cli.Main())
 }
@@ -326,7 +370,7 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{"keyfunc", subcommand, "to", host, "uptime"}
+	os.Args = []string{tool, subcommand, "to", host, "uptime"}
 
 	require.Equal(t, failingStatus, cli.Main())
 }
@@ -373,7 +417,7 @@ func attempt(t *testing.T, args ...string) (string, string, error) {
 	root := cli.Root()
 	root.SetOut(&printed)
 	root.SetErr(&said)
-	root.SetArgs(slices.Concat([]string{subcommand, "install"}, args))
+	root.SetArgs(slices.Concat([]string{subcommand, installing}, args))
 
 	err := root.ExecuteContext(t.Context())
 
@@ -394,6 +438,23 @@ func seed(t *testing.T, pretend pretended, content string) string {
 	return path
 }
 
+// unfetchable puts a directory where authorized_keys belongs on the
+// stand-in host, which the stand-in can see but cannot fetch: that is
+// how a file that is there and cannot be read looks from here. It
+// gives back the path.
+func unfetchable(t *testing.T, pretend pretended) string {
+	t.Helper()
+
+	require.NoError(t,
+		os.Mkdir(filepath.Join(pretend.home, keptUnder), directoryMode),
+	)
+
+	path := filepath.Join(pretend.home, keptUnder, keptIn)
+	require.NoError(t, os.Mkdir(path, directoryMode))
+
+	return path
+}
+
 // 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.