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, then
rename over authorized_keys. A run that adds a line connects twice; one
that finds the line there connects once and stops. A failed step leaves
everything as it is and names the uploaded file.

sftp echoes the commands it runs, so all of its output goes to standard
error and the tool prints only "added" or "already present". Batch mode
cannot prompt for a password; the README says so.

Model: opus-5

README.md

@@ -94,14 +94,10 @@ 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`. 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:
+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:
 
 - creates `~/.ssh` and sets it to mode `0700`;
 - uploads the new file as `~/.ssh/authorized_keys.keyfunc-<random>` and sets it
@@ -114,10 +110,8 @@ 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, 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`
+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`
 writes goes to standard error, so the tool's own standard output is only
 `added` or `already present`.
 

internal/cli/ssh/install.go

@@ -1,10 +1,11 @@
 package ssh
 
 import (
-	"bytes"
 	"crypto/rand"
 	"encoding/hex"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -76,9 +77,18 @@ func add(cmd *cobra.Command, host string, options []string, line string) error {
 
 	defer func() { _ = os.RemoveAll(work) }()
 
-	content, err := fetch(cmd, host, options,
-		filepath.Join(work, "authorized_keys"),
-	)
+	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)
 	if err != nil {
 		return err
 	}
@@ -112,7 +122,7 @@ func upload(
 	}
 
 	// The mkdir may fail: the directory is usually there already.
-	said, err := session(cmd, host, options, []string{
+	err = session(cmd, host, options, []string{
 		"-mkdir " + directory,
 		"chmod " + directoryMode + " " + directory,
 		"put " + quoted(local) + " " + sidecar,
@@ -120,17 +130,7 @@ func upload(
 		"rename " + sidecar + " " + authorized,
 	})
 	if err != nil {
-		// 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 fmt.Errorf("%w; %s may be left on the host", err, sidecar)
 	}
 
 	return write(cmd, "added\n")
@@ -141,32 +141,26 @@ 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. What it said is also given back: a session that
-// failed says there what went wrong, and the status alone does not.
+// one word it prints.
 func session(
 	cmd *cobra.Command, host string, options []string, batch []string,
-) (string, error) {
+) 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 = &said
-	command.Stderr = &said
+	command.Stdout = cmd.ErrOrStderr()
+	command.Stderr = cmd.ErrOrStderr()
 
 	err := command.Run()
-
-	_, _ = cmd.ErrOrStderr().Write(said.Bytes())
-
 	if err != nil {
-		return said.String(), fmt.Errorf("running sftp: %w", err)
+		return fmt.Errorf("running sftp: %w", err)
 	}
 
-	return said.String(), nil
+	return nil
 }
 
 // merge returns the file with the key line on the end, and whether it
@@ -184,27 +178,15 @@ func merge(content, line string) (string, bool) {
 	return content + line + "\n", true
 }
 
-// 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
+// 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
 	}
 
-	//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)
 	}
@@ -212,17 +194,6 @@ 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.
-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,7 +1,6 @@
 package cli_test
 
 import (
-	"bytes"
 	"os"
 	"path/filepath"
 	"slices"
@@ -24,16 +23,8 @@ const (
 )
 
 // failingStatus is the status the stand-in ssh ends with when a test
-// 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"
+// wants to see a status handed on.
+const failingStatus = 7
 
 // The host, and where on it the key ends up.
 const (
@@ -42,30 +33,23 @@ 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, 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.
+// 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.
 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
@@ -77,14 +61,7 @@ while IFS= read -r line; do
 	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
-		;;
+	get) cp "$home/$2" "$3" 2>/dev/null || worked=no ;;
 	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 ;;
@@ -127,7 +104,7 @@ func TestTheKeyIsAddedToAHostThatHasNoFileYet(t *testing.T) {
 
 	pretend := pretendHost(t)
 
-	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, "added\n", run(t, "ssh", "install", host))
 
 	directory, err := os.Stat(filepath.Join(pretend.home, keptUnder))
 	require.NoError(t, err)
@@ -150,7 +127,9 @@ func TestAKeyThatIsAlreadyThereIsLeftAlone(t *testing.T) {
 	pretend := pretendHost(t)
 	path := seed(t, pretend, "somebody else\n"+keyLine)
 
-	require.Equal(t, "already present\n", install(t, host))
+	require.Equal(t,
+		"already present\n", run(t, "ssh", "install", host),
+	)
 	require.Equal(t, "somebody else\n"+keyLine, read(t, path))
 
 	// The fetch and nothing after it: the tool did not connect again.
@@ -163,7 +142,7 @@ func TestAnEmptyFileGetsTheKeyAndNoBlankLineBeforeIt(t *testing.T) {
 	pretend := pretendHost(t)
 	path := seed(t, pretend, "")
 
-	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, "added\n", run(t, "ssh", "install", host))
 	require.Equal(t, keyLine, read(t, path))
 }
 
@@ -174,7 +153,7 @@ func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) {
 	already := "ssh-ed25519 AAAAsomebodyelse somebody@else"
 	path := seed(t, pretend, already)
 
-	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, "added\n", run(t, "ssh", "install", host))
 	require.Equal(t, already+"\n"+keyLine, read(t, path))
 }
 
@@ -183,7 +162,7 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
 
 	pretend := pretendHost(t)
 
-	require.Equal(t, "added\n", install(t, host))
+	require.Equal(t, "added\n", run(t, "ssh", "install", host))
 
 	sent := recorded(t, pretend.batch)
 	require.Len(t, sent, 6)
@@ -195,7 +174,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])
@@ -203,76 +182,12 @@ 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)
 
-	install(t, host)
+	run(t, "ssh", "install", host)
 
 	require.NotContains(t, read(t, pretend.arguments), "ssh-ed25519")
 	require.NotContains(t, read(t, pretend.batch), "ssh-ed25519")
@@ -283,7 +198,7 @@ func TestWhatComesAfterTheDashesIsGivenToSFTP(t *testing.T) {
 
 	pretend := pretendHost(t)
 
-	install(t, host, "--", "-P", "2222")
+	run(t, "ssh", "install", 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.
@@ -299,7 +214,7 @@ func TestSSHIsPointedAtTheAgentAndItsStatusIsHandedOn(t *testing.T) {
 
 	arguments, noted := pretendCall(t)
 
-	_, err := execute(t, subcommand, "to", host, "uptime")
+	_, err := execute(t, "ssh", "to", host, "uptime")
 
 	var passed ssh.StatusError
 
@@ -326,7 +241,7 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) {
 
 	t.Cleanup(func() { os.Args = given })
 
-	os.Args = []string{"keyfunc", subcommand, "to", host, "uptime"}
+	os.Args = []string{"keyfunc", "ssh", "to", host, "uptime"}
 
 	require.Equal(t, failingStatus, cli.Main())
 }
@@ -350,36 +265,6 @@ 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 {