The ssh install command works over sftp (closes #10)

The command no longer sends a shell script to the host. It fetches
~/.ssh/authorized_keys with the system sftp in batch mode, adds the key
line here, and writes the file back in a second session: mkdir and chmod
on ~/.ssh, put to authorized_keys.keyfunc-<random>, chmod 600, rename
over authorized_keys. Adding the line connects twice.

The file reads as empty only when sftp reported it as not there, in the
line naming that path; the same wording elsewhere -- ssh writes it about
an identity file it cannot find -- does not count, so a file that cannot
be read is never written over. A failed step removes nothing and names
the uploaded file once sftp's echo shows the put was reached.

Model: opus-5

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.