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

@@ -94,10 +94,14 @@ 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`; a host that has no such
-file yet reads as empty. 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 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
+`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`;
 - uploads the new file as `~/.ssh/authorized_keys.keyfunc-<random>` and sets it
@@ -110,8 +114,10 @@ never half-written. `sftp` does it in one step against servers that offer
 OpenSSH's POSIX rename extension, as OpenSSH's own server does; a server
 without it may refuse to rename onto a file that is already there.
 
-If a step fails, the tool prints what `sftp` said, names the uploaded file if
-there was one, removes nothing, and exits with status 1. Everything `sftp`
+If a step fails, the tool prints what `sftp` said, removes nothing, and exits
+with status 1. It names the uploaded file only when the step that failed was
+the upload or one after it, which is where a file of that name can be on the
+host; a failure before the upload names none. Everything `sftp`
 writes goes to standard error, so the tool's own standard output is only
 `added` or `already present`.
 

internal/cli/ssh/install.go

@@ -1,11 +1,10 @@
 package ssh
 
 import (
+	"bytes"
 	"crypto/rand"
 	"encoding/hex"
-	"errors"
 	"fmt"
-	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -77,18 +76,9 @@ func add(cmd *cobra.Command, host string, options []string, line string) error {
 
 	defer func() { _ = os.RemoveAll(work) }()
 
-	fetched := filepath.Join(work, "authorized_keys")
-
-	// The get may fail: a host with no authorized_keys yet is not an
-	// error, and nothing arrives.
-	err = session(cmd, host, options, []string{
-		"-get " + authorized + " " + quoted(fetched),
-	})
-	if err != nil {
-		return err
-	}
-
-	content, err := arrived(fetched)
+	content, err := fetch(cmd, host, options,
+		filepath.Join(work, "authorized_keys"),
+	)
 	if err != nil {
 		return err
 	}
@@ -122,7 +112,7 @@ func upload(
 	}
 
 	// The mkdir may fail: the directory is usually there already.
-	err = session(cmd, host, options, []string{
+	said, err := session(cmd, host, options, []string{
 		"-mkdir " + directory,
 		"chmod " + directoryMode + " " + directory,
 		"put " + quoted(local) + " " + sidecar,
@@ -130,7 +120,17 @@ func upload(
 		"rename " + sidecar + " " + authorized,
 	})
 	if err != nil {
-		return fmt.Errorf("%w; %s may be left on the host", err, sidecar)
+		// 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
+		// the put was reached, which is where a file of that name
+		// can be on the host. Before that there is none to name.
+		if strings.Contains(said, sidecar) {
+			return fmt.Errorf(
+				"%w; %s may be left on the host", err, sidecar,
+			)
+		}
+
+		return err
 	}
 
 	return write(cmd, "added\n")
@@ -141,26 +141,32 @@ func upload(
 // stops at the first of which that fails, unless it begins with a
 // dash. sftp echoes the commands as it runs them, so everything it
 // says goes to the error output and the tool's own output stays the
-// one word it prints.
+// one word it prints. What it said is also given back: a session that
+// failed says there what went wrong, and the status alone does not.
 func session(
 	cmd *cobra.Command, host string, options []string, batch []string,
-) error {
+) (string, error) {
 	argv := slices.Concat(
 		[]string{"-b", "-"}, options, []string{host},
 	)
 
+	var said bytes.Buffer
+
 	//nolint:gosec // the options are the user's own, meant for sftp
 	command := exec.CommandContext(cmd.Context(), "sftp", argv...)
 	command.Stdin = strings.NewReader(strings.Join(batch, "\n") + "\n")
-	command.Stdout = cmd.ErrOrStderr()
-	command.Stderr = cmd.ErrOrStderr()
+	command.Stdout = &said
+	command.Stderr = &said
 
 	err := command.Run()
+
+	_, _ = cmd.ErrOrStderr().Write(said.Bytes())
+
 	if err != nil {
-		return fmt.Errorf("running sftp: %w", err)
+		return said.String(), fmt.Errorf("running sftp: %w", err)
 	}
 
-	return nil
+	return said.String(), nil
 }
 
 // merge returns the file with the key line on the end, and whether it
@@ -178,15 +184,27 @@ func merge(content, line string) (string, bool) {
 	return content + line + "\n", true
 }
 
-// arrived returns what is in the fetched file, and nothing at all when
-// no file arrived because the host has none.
-func arrived(path string) (string, error) {
-	//nolint:gosec // the path is a temporary file of the tool's own
-	content, err := os.ReadFile(path)
-	if errors.Is(err, fs.ErrNotExist) {
-		return "", nil
+// 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.
+func fetch(
+	cmd *cobra.Command, host string, options []string, into string,
+) (string, error) {
+	said, err := session(cmd, host, options, []string{
+		"get " + authorized + " " + quoted(into),
+	})
+	if err != nil {
+		if absent(said) {
+			return "", nil
+		}
+
+		return "", 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)
 	}
@@ -194,6 +212,17 @@ func arrived(path string) (string, error) {
 	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.
+func absent(said string) bool {
+	lower := strings.ToLower(said)
+
+	return strings.Contains(lower, "no such file") ||
+		strings.Contains(lower, "not found")
+}
+
 // sidecarName returns the name the new file is uploaded under.
 func sidecarName() (string, error) {
 	random := make([]byte, sidecarBytes)

internal/cli/ssh_test.go

@@ -1,6 +1,7 @@
 package cli_test
 
 import (
+	"bytes"
 	"os"
 	"path/filepath"
 	"slices"
@@ -23,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 (
@@ -33,23 +42,30 @@ const (
 	keptIn    = "authorized_keys"
 )
 
+// 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, 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.
+// 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 = `
 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
@@ -61,7 +77,14 @@ while IFS= read -r line; do
 	eval "set -- $line"
 	worked=yes
 	case "$1" in
-	get) cp "$home/$2" "$3" 2>/dev/null || worked=no ;;
+	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 ;;
@@ -104,7 +127,7 @@ func TestTheKeyIsAddedToAHostThatHasNoFileYet(t *testing.T) {
 
 	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)
@@ -127,9 +150,7 @@ func TestAKeyThatIsAlreadyThereIsLeftAlone(t *testing.T) {
 	pretend := pretendHost(t)
 	path := seed(t, pretend, "somebody else\n"+keyLine)
 
-	require.Equal(t,
-		"already present\n", run(t, "ssh", "install", host),
-	)
+	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.
@@ -142,7 +163,7 @@ func TestAnEmptyFileGetsTheKeyAndNoBlankLineBeforeIt(t *testing.T) {
 	pretend := pretendHost(t)
 	path := seed(t, pretend, "")
 
-	require.Equal(t, "added\n", run(t, "ssh", "install", host))
+	require.Equal(t, "added\n", install(t, host))
 	require.Equal(t, keyLine, read(t, path))
 }
 
@@ -153,7 +174,7 @@ func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) {
 	already := "ssh-ed25519 AAAAsomebodyelse somebody@else"
 	path := seed(t, pretend, already)
 
-	require.Equal(t, "added\n", run(t, "ssh", "install", host))
+	require.Equal(t, "added\n", install(t, host))
 	require.Equal(t, already+"\n"+keyLine, read(t, path))
 }
 
@@ -162,7 +183,7 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
 
 	pretend := pretendHost(t)
 
-	require.Equal(t, "added\n", run(t, "ssh", "install", host))
+	require.Equal(t, "added\n", install(t, host))
 
 	sent := recorded(t, pretend.batch)
 	require.Len(t, sent, 6)
@@ -174,7 +195,7 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
 		strings.HasPrefix(beside, ".ssh/authorized_keys.keyfunc-"),
 	)
 
-	require.True(t, strings.HasPrefix(sent[0], "-get .ssh/authorized_keys "))
+	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])
@@ -182,12 +203,76 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
 	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),
+	)
+
+	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, "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 TestAFailedStepNamesTheUploadedFileAndChangesNothing(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	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.
+	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)
 
-	run(t, "ssh", "install", host)
+	install(t, host)
 
 	require.NotContains(t, read(t, pretend.arguments), "ssh-ed25519")
 	require.NotContains(t, read(t, pretend.batch), "ssh-ed25519")
@@ -198,7 +283,7 @@ func TestWhatComesAfterTheDashesIsGivenToSFTP(t *testing.T) {
 
 	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.
@@ -214,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
 
@@ -241,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())
 }
@@ -265,6 +350,36 @@ func pretendHost(t *testing.T) pretended {
 	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 {