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

internal/cli/ssh_test.go

@@ -1,8 +1,10 @@
 package cli_test
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 	"testing"
@@ -14,7 +16,7 @@ import (
 )
 
 // The modes the host is supposed to end up with, and the mode the
-// stand-in ssh needs so that it can be run at all.
+// stand-ins need so that they can be run at all.
 const (
 	directoryMode = 0o700
 	fileMode      = 0o600
@@ -22,8 +24,16 @@ const (
 )
 
 // failingStatus is the status the stand-in ssh ends with when a test
-// wants to see a status handed on.
-const failingStatus = 7
+// wants to see a status handed on, and failedStatus is the status the
+// tool itself ends with when something went wrong.
+const (
+	failingStatus = 7
+	failedStatus  = 1
+)
+
+// notADirectory is what a test puts where the .ssh directory belongs
+// to make a step of the write session fail.
+const notADirectory = "a file where the directory belongs\n"
 
 // The host, and where on it the key ends up.
 const (
@@ -32,20 +42,59 @@ const (
 	keptIn    = "authorized_keys"
 )
 
-// installer is a stand-in for the system ssh for the install command.
-// It writes down what it was given and then runs the command meant for
-// the host right here, with the home directory pointed at a directory
-// standing in for the host's, so that what keyfunc sends can be
-// watched doing its work.
+// 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.
+const keyLine = vectorZero + " keyfunc/ssh/0\n"
+
+// installer is a stand-in for the system sftp for the install
+// command. It writes down the arguments and every command of the
+// batch it is given, echoes each command as sftp does, and carries
+// 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.
 const installer = `
-while [ $# -gt 1 ]; do
-	printf '%s\n' "$1" >> "$KEYFUNC_TEST_ARGUMENTS"
-	shift
+for argument in "$@"; do
+	printf '%s\n' "$argument" >> "$KEYFUNC_TEST_ARGUMENTS"
+done
+home="$KEYFUNC_TEST_HOME"
+while IFS= read -r line; do
+	printf 'sftp> %s\n' "$line"
+	printf '%s\n' "$line" >> "$KEYFUNC_TEST_BATCH"
+	allowed=no
+	case "$line" in
+	-*)
+		line=${line#-}
+		allowed=yes
+		;;
+	esac
+	eval "set -- $line"
+	worked=yes
+	case "$1" in
+	get)
+		if [ -e "$home/$2" ]; then
+			cp "$home/$2" "$3" 2>/dev/null || worked=no
+		else
+			worked=no
+			printf 'File "%s" not found.\n' "$2" >&2
+		fi
+		;;
+	put) cp "$2" "$home/$3" 2>/dev/null || worked=no ;;
+	mkdir) mkdir "$home/$2" 2>/dev/null || worked=no ;;
+	chmod) chmod "$2" "$home/$3" 2>/dev/null || worked=no ;;
+	rename) mv "$home/$2" "$home/$3" 2>/dev/null || worked=no ;;
+	esac
+	if [ "$worked" = no ] && [ "$allowed" = no ]; then
+		printf 'sftp: %s failed\n' "$1" >&2
+		exit 1
+	fi
 done
-printf '%s' "$1" > "$KEYFUNC_TEST_COMMAND"
-HOME="$KEYFUNC_TEST_HOME"
-export HOME
-eval "$1"
 `
 
 // caller is a stand-in for the system ssh for the to command. It
@@ -63,24 +112,22 @@ fi
 exit "$KEYFUNC_TEST_STATUS"
 `
 
-// pretended is where a stand-in ssh writes down what it was asked to
-// do.
+// pretended is where a stand-in writes down what it was asked to do.
 type pretended struct {
 	// home stands in for the home directory on the host.
 	home string
-	// arguments holds what ssh was given before the command, one per
-	// line.
+	// arguments holds the arguments of every session, one per line.
 	arguments string
-	// command holds what ssh was told to run on the host.
-	command string
+	// batch holds the commands of every session, one per line.
+	batch string
 }
 
-func TestTheKeyIsAddedToTheHostAndThenLeftAlone(t *testing.T) {
+func TestTheKeyIsAddedToAHostThatHasNoFileYet(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	pretend := pretendHost(t)
 
-	require.Equal(t, "added\n", run(t, "ssh", "install", host))
+	require.Equal(t, "added\n", install(t, host))
 
 	directory, err := os.Stat(filepath.Join(pretend.home, keptUnder))
 	require.NoError(t, err)
@@ -94,11 +141,30 @@ func TestTheKeyIsAddedToTheHostAndThenLeftAlone(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, os.FileMode(fileMode), file.Mode().Perm())
 
-	added := read(t, path)
-	require.Equal(t, vectorZero+" keyfunc/ssh/0\n", added)
+	require.Equal(t, keyLine, read(t, path))
+}
 
-	require.Equal(t, "already present\n", run(t, "ssh", "install", host))
-	require.Equal(t, added, read(t, path))
+func TestAKeyThatIsAlreadyThereIsLeftAlone(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+	path := seed(t, pretend, "somebody else\n"+keyLine)
+
+	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)
+}
+
+func TestAnEmptyFileGetsTheKeyAndNoBlankLineBeforeIt(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+	path := seed(t, pretend, "")
+
+	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, keyLine, read(t, path))
 }
 
 func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) {
@@ -106,41 +172,124 @@ func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) {
 
 	pretend := pretendHost(t)
 	already := "ssh-ed25519 AAAAsomebodyelse somebody@else"
+	path := seed(t, pretend, already)
 
+	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, already+"\n"+keyLine, read(t, path))
+}
+
+func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+
+	require.Equal(t, "added\n", install(t, host))
+
+	sent := recorded(t, pretend.batch)
+	require.Len(t, sent, 6)
+
+	// The name of the uploaded file is random, so it is read off the
+	// put and then looked for in the two commands that follow.
+	beside := strings.Fields(sent[3])[2]
+	require.True(t,
+		strings.HasPrefix(beside, ".ssh/authorized_keys.keyfunc-"),
+	)
+
+	require.True(t, strings.HasPrefix(sent[0], "get .ssh/authorized_keys "))
+	require.Equal(t, "-mkdir .ssh", sent[1])
+	require.Equal(t, "chmod 700 .ssh", sent[2])
+	require.Equal(t, "put", strings.Fields(sent[3])[0])
+	require.Equal(t, "chmod 600 "+beside, sent[4])
+	require.Equal(t, "rename "+beside+" .ssh/authorized_keys", sent[5])
+}
+
+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),
 	)
 
-	path := filepath.Join(pretend.home, keptUnder, keptIn)
-	require.NoError(t, os.WriteFile(path, []byte(already), fileMode))
+	unreadable := filepath.Join(pretend.home, keptUnder, keptIn)
+	require.NoError(t, os.Mkdir(unreadable, directoryMode))
 
-	require.Equal(t, "added\n", run(t, "ssh", "install", host))
-	require.Equal(t,
-		already+"\n"+vectorZero+" keyfunc/ssh/0\n",
-		read(t, path),
-	)
+	printed, said, err := attempt(t, host)
+	require.Error(t, err)
+	require.Empty(t, printed)
+	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.
+	require.Len(t, recorded(t, pretend.batch), 1)
+	require.DirExists(t, unreadable)
 }
 
-func TestTheKeyLineIsNotOnTheCommandLine(t *testing.T) {
+func TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	pretend := pretendHost(t)
 
-	run(t, "ssh", "install", host)
+	// 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.
+	inTheWay := filepath.Join(pretend.home, keptUnder)
+	require.NoError(t,
+		os.WriteFile(inTheWay, []byte(notADirectory), fileMode),
+	)
+
+	printed, said, err := attempt(t, host)
+	require.Error(t, err)
+	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.
+	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.Equal(t, notADirectory, read(t, inTheWay))
+
+	// The same run again, this way for the status it ends with.
+	given := os.Args
+
+	t.Cleanup(func() { os.Args = given })
+
+	os.Args = []string{"keyfunc", subcommand, "install", host}
+
+	require.Equal(t, failedStatus, cli.Main())
+}
+
+func TestTheKeyLineIsNotSentAsACommand(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+
+	install(t, host)
 
 	require.NotContains(t, read(t, pretend.arguments), "ssh-ed25519")
-	require.NotContains(t, read(t, pretend.command), "ssh-ed25519")
+	require.NotContains(t, read(t, pretend.batch), "ssh-ed25519")
 }
 
-func TestWhatComesAfterTheDashesIsGivenToSSH(t *testing.T) {
+func TestWhatComesAfterTheDashesIsGivenToSFTP(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	pretend := pretendHost(t)
 
-	run(t, "ssh", "install", host, "--", "-p", "2222")
+	install(t, host, "--", "-P", "2222")
 
+	// The same arguments twice over: adding a line takes two
+	// connections, one to fetch the file and one to write it back.
+	session := []string{"-b", "-", "-P", "2222", host}
 	require.Equal(t,
-		[]string{"-p", "2222", host},
+		slices.Concat(session, session),
 		recorded(t, pretend.arguments),
 	)
 }
@@ -150,7 +299,7 @@ func TestSSHIsPointedAtTheAgentAndItsStatusIsHandedOn(t *testing.T) {
 
 	arguments, noted := pretendCall(t)
 
-	_, err := execute(t, "ssh", "to", host, "uptime")
+	_, err := execute(t, subcommand, "to", host, "uptime")
 
 	var passed ssh.StatusError
 
@@ -177,7 +326,7 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{"keyfunc", "ssh", "to", host, "uptime"}
+	os.Args = []string{"keyfunc", subcommand, "to", host, "uptime"}
 
 	require.Equal(t, failingStatus, cli.Main())
 }
@@ -190,17 +339,61 @@ func pretendHost(t *testing.T) pretended {
 	pretend := pretended{
 		home:      t.TempDir(),
 		arguments: filepath.Join(t.TempDir(), "arguments"),
-		command:   filepath.Join(t.TempDir(), "command"),
+		batch:     filepath.Join(t.TempDir(), "batch"),
 	}
 
 	t.Setenv("KEYFUNC_TEST_HOME", pretend.home)
 	t.Setenv("KEYFUNC_TEST_ARGUMENTS", pretend.arguments)
-	t.Setenv("KEYFUNC_TEST_COMMAND", pretend.command)
-	standIn(t, installer)
+	t.Setenv("KEYFUNC_TEST_BATCH", pretend.batch)
+	standIn(t, "sftp", installer)
 
 	return pretend
 }
 
+// install runs the install command, requires it to have worked, and
+// gives back what the tool itself printed.
+func install(t *testing.T, args ...string) string {
+	t.Helper()
+
+	printed, _, err := attempt(t, args...)
+	require.NoError(t, err)
+
+	return printed
+}
+
+// attempt runs the install command with the tool's own output kept
+// apart from what the stand-in said, since the stand-in echoes its
+// batch as sftp does. It gives back what the tool printed, what the
+// stand-in said, and how the run ended.
+func attempt(t *testing.T, args ...string) (string, string, error) {
+	t.Helper()
+
+	var printed, said bytes.Buffer
+
+	root := cli.Root()
+	root.SetOut(&printed)
+	root.SetErr(&said)
+	root.SetArgs(slices.Concat([]string{subcommand, "install"}, args))
+
+	err := root.ExecuteContext(t.Context())
+
+	return printed.String(), said.String(), err
+}
+
+// seed puts an authorized_keys file on the stand-in host before the
+// tool runs and gives back its path.
+func seed(t *testing.T, pretend pretended, content string) string {
+	t.Helper()
+
+	directory := filepath.Join(pretend.home, keptUnder)
+	require.NoError(t, os.Mkdir(directory, directoryMode))
+
+	path := filepath.Join(directory, keptIn)
+	require.NoError(t, os.WriteFile(path, []byte(content), fileMode))
+
+	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.
@@ -213,20 +406,21 @@ func pretendCall(t *testing.T) (string, string) {
 	t.Setenv("KEYFUNC_TEST_ARGUMENTS", arguments)
 	t.Setenv("KEYFUNC_TEST_SOCKET", noted)
 	t.Setenv("KEYFUNC_TEST_STATUS", strconv.Itoa(failingStatus))
-	standIn(t, caller)
+	standIn(t, "ssh", caller)
 
 	return arguments, noted
 }
 
-// standIn writes a stand-in for the system ssh and puts it first on
-// the path, so that the tool finds it instead of the real one.
-func standIn(t *testing.T, body string) {
+// standIn writes a stand-in for one of the system programs and puts it
+// first on the path, so that the tool finds it instead of the real
+// one.
+func standIn(t *testing.T, name, body string) {
 	t.Helper()
 
 	directory := t.TempDir()
 
 	err := os.WriteFile(
-		filepath.Join(directory, "ssh"),
+		filepath.Join(directory, name),
 		[]byte("#!/bin/sh\n"+body), standInMode,
 	)
 	require.NoError(t, err)
@@ -247,7 +441,7 @@ func read(t *testing.T, path string) string {
 	return string(content)
 }
 
-// recorded returns the arguments a stand-in wrote down, one per line.
+// recorded returns the lines a stand-in wrote down.
 func recorded(t *testing.T, path string) []string {
 	t.Helper()