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 a line connects twice.

The file reads as empty only when sftp said there is no such file; any
other failure of the fetch stops the run, 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.

sftp's output goes to standard error, so the tool prints one word.

Model: opus-5

README.md

@@ -95,17 +95,11 @@ 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
+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
 `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,42 +212,15 @@ func fetch(
 	return string(content), nil
 }
 
-// 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.
+// 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.
 func absent(said string) bool {
-	for line := range strings.Lines(said) {
-		named, is := reportedNotFound(strings.TrimSpace(line))
-		if is && (named == authorized ||
-			strings.HasSuffix(named, "/"+authorized)) {
-			return true
-		}
-	}
+	lower := strings.ToLower(said)
 
-	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
+	return strings.Contains(lower, "no such file") ||
+		strings.Contains(lower, "not found")
 }
 
 // sidecarName returns the name the new file is uploaded under.

internal/cli/ssh/install_test.go

@@ -1,78 +0,0 @@
-//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,11 +35,6 @@ 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"
@@ -47,14 +42,9 @@ const (
 	keptIn    = "authorized_keys"
 )
 
-// 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"
-)
+// subcommand is the tool's ssh subcommand, which both commands the
+// tests here drive live under.
+const subcommand = "ssh"
 
 // The key line the example mnemonic gives at index 0, as it stands in
 // an authorized_keys file.
@@ -66,24 +56,12 @@ 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.
-//
-// 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.
+// 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.
 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
@@ -100,12 +78,11 @@ while IFS= read -r line; do
 	worked=yes
 	case "$1" in
 	get)
-		if [ ! -e "$home/$2" ]; then
+		if [ -e "$home/$2" ]; then
+			cp "$home/$2" "$3" 2>/dev/null || worked=no
+		else
 			worked=no
-			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
+			printf 'File "%s" not found.\n' "$2" >&2
 		fi
 		;;
 	put) cp "$2" "$home/$3" 2>/dev/null || worked=no ;;
@@ -230,12 +207,22 @@ func TestAFileThatCannotBeReadIsNotWrittenOver(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	pretend := pretendHost(t)
-	unreadable := unfetchable(t, pretend)
+
+	// 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))
 
 	printed, said, err := attempt(t, host)
 	require.Error(t, err)
 	require.Empty(t, printed)
-	require.Contains(t, said, "Permission denied")
+	require.Contains(t, said, "get failed")
 
 	// The fetch and nothing after it, and what was on the host is
 	// still what is on the host.
@@ -243,37 +230,6 @@ 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())
 
@@ -306,7 +262,7 @@ func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{tool, subcommand, installing, host}
+	os.Args = []string{"keyfunc", subcommand, "install", host}
 
 	require.Equal(t, failedStatus, cli.Main())
 }
@@ -370,7 +326,7 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{tool, subcommand, "to", host, "uptime"}
+	os.Args = []string{"keyfunc", subcommand, "to", host, "uptime"}
 
 	require.Equal(t, failingStatus, cli.Main())
 }
@@ -417,7 +373,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, installing}, args))
+	root.SetArgs(slices.Concat([]string{subcommand, "install"}, args))
 
 	err := root.ExecuteContext(t.Context())
 
@@ -438,23 +394,6 @@ 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.