ssh install works over sftp and runs nothing on the host (closes #10)

ssh install no longer runs a command on the host. It reads .ssh/authorized_keys over sftp, takes the empty reading only from sftp's own message about that path, appends the derived key locally when it is not already present, uploads the result beside the file with mode 0600 and renames it over the original. Any other failure prints what sftp said, writes nothing and exits 1. sftp batch mode disables password prompts, so a key or agent is required; a directory the owner cannot enter reads as a host with no file, which README.md states.

Model: opus-5 (implementation); fable-5-1 (landing)

internal/cli/ssh/install.go

@@ -1,57 +1,48 @@
 package ssh
 
 import (
+	"bytes"
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
+	"os"
 	"os/exec"
+	"path/filepath"
 	"slices"
 	"strings"
 
 	"github.com/spf13/cobra"
 )
 
-// script is what runs on the host. It reads the key line from its own
-// standard input, so the line never appears on a command line, where
-// anyone else on the host could read it out of the process list. It
-// contains no single quote, so the whole of it travels through ssh
-// inside one pair of them. The umask keeps anything it makes to the
-// owner from the start; the modes are then set outright, whatever the
-// umask on the host turns out to be. A file whose last line has no
-// newline at its end gets one before the key line goes on, so that the
-// two do not run into each other.
-const script = `
-set -e
-umask 077
-directory="$HOME/.ssh"
-file="$directory/authorized_keys"
-if [ ! -d "$directory" ]; then
-	mkdir -p "$directory"
-	chmod 700 "$directory"
-fi
-if [ ! -f "$file" ]; then
-	: > "$file"
-	chmod 600 "$file"
-fi
-IFS= read -r line
-if grep -q -x -F -e "$line" "$file"; then
-	echo "already present"
-else
-	if [ -s "$file" ] && [ -n "$(tail -c 1 "$file")" ]; then
-		printf "\n" >> "$file"
-	fi
-	printf "%s\n" "$line" >> "$file"
-	echo "added"
-fi
-`
+// Where the key goes on the host and what the file it arrives in is
+// called before it is renamed into place. The random end of that name
+// keeps two runs at once from writing to the same file.
+const (
+	directory     = ".ssh"
+	authorized    = ".ssh/authorized_keys"
+	sidecarPrefix = ".ssh/authorized_keys.keyfunc-"
+	sidecarBytes  = 8
+)
+
+// The modes the host is left with, as sftp's chmod spells them, and
+// the mode of the copy made here on the way.
+const (
+	directoryMode = "700"
+	fileMode      = "600"
+	localMode     = 0o600
+)
 
 // install returns the command that adds the public key to a host.
 func install() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "install <[user@]host> [-- ssh options...]",
+		Use:   "install <[user@]host> [-- sftp options...]",
 		Short: "add the public key to a host's authorized_keys",
-		Long: "Runs the system ssh to the host, which makes ~/.ssh and " +
-			"~/.ssh/authorized_keys there if they are missing and adds " +
-			"the public key unless the same line is already in the " +
-			"file. Anything after -- is given to ssh unchanged.",
+		Long: "Downloads the host's authorized_keys with the system " +
+			"sftp, adds the public key to it here unless the same " +
+			"line is already there, and uploads the result as a file " +
+			"beside it which is then renamed over it. Nothing is run " +
+			"on the host. Anything after -- is given to sftp " +
+			"unchanged, which is where the port goes (-P).",
 		Args: cobra.MinimumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			key, comment, err := derived(cmd)
@@ -64,7 +55,7 @@ func install() *cobra.Command {
 				return err
 			}
 
-			return send(cmd, args[0], args[1:], line)
+			return add(cmd, args[0], args[1:], line)
 		},
 	}
 
@@ -73,24 +64,207 @@ func install() *cobra.Command {
 	return cmd
 }
 
-// send runs ssh to the host with the user's options, gives it the
-// script to run there, and writes the key line to its standard input.
-// What the host says, added or already present, is passed straight on.
-func send(cmd *cobra.Command, host string, options []string, line string) error {
-	argv := slices.Concat(options, []string{
-		host, "/bin/sh -c '" + script + "'",
-	})
-
-	//nolint:gosec // the options are the user's own, meant for ssh
-	command := exec.CommandContext(cmd.Context(), "ssh", argv...)
-	command.Stdin = strings.NewReader(line + "\n")
-	command.Stdout = cmd.OutOrStdout()
-	command.Stderr = cmd.ErrOrStderr()
-
-	err := command.Run()
+// add puts the key line in the host's authorized_keys. The file is
+// fetched in one sftp session and written back in another, so a run
+// that adds a line connects twice; a run that finds the line already
+// there connects once and stops.
+func add(cmd *cobra.Command, host string, options []string, line string) error {
+	work, err := os.MkdirTemp("", "keyfunc-install-")
 	if err != nil {
-		return fmt.Errorf("running ssh: %w", err)
+		return fmt.Errorf("making a temporary directory: %w", err)
 	}
 
-	return nil
+	defer func() { _ = os.RemoveAll(work) }()
+
+	content, err := fetch(cmd, host, options,
+		filepath.Join(work, "authorized_keys"),
+	)
+	if err != nil {
+		return err
+	}
+
+	merged, added := merge(content, line)
+	if !added {
+		return write(cmd, "already present\n")
+	}
+
+	return upload(cmd, host, options, work, merged)
+}
+
+// upload writes the new file to the host and renames it over
+// authorized_keys, which is the step that either happens or does not.
+// Nothing is removed when a step fails: the file left behind is named
+// so that it can be looked at and cleared away by hand.
+func upload(
+	cmd *cobra.Command, host string, options []string,
+	work, merged string,
+) error {
+	local := filepath.Join(work, "authorized_keys.merged")
+
+	err := os.WriteFile(local, []byte(merged), localMode)
+	if err != nil {
+		return fmt.Errorf("writing the new file: %w", err)
+	}
+
+	sidecar, err := sidecarName()
+	if err != nil {
+		return err
+	}
+
+	// The mkdir may fail: the directory is usually there already.
+	said, err := session(cmd, host, options, []string{
+		"-mkdir " + directory,
+		"chmod " + directoryMode + " " + directory,
+		"put " + quoted(local) + " " + sidecar,
+		"chmod " + fileMode + " " + sidecar,
+		"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 write(cmd, "added\n")
+}
+
+// session runs one sftp session with the user's own options and the
+// batch of commands, which sftp reads from its standard input and
+// 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.
+func session(
+	cmd *cobra.Command, host string, options []string, batch []string,
+) (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 = &said
+	command.Stderr = &said
+
+	err := command.Run()
+
+	_, _ = cmd.ErrOrStderr().Write(said.Bytes())
+
+	if err != nil {
+		return said.String(), fmt.Errorf("running sftp: %w", err)
+	}
+
+	return said.String(), nil
+}
+
+// merge returns the file with the key line on the end, and whether it
+// had to be added. A file whose last line has no newline at its end
+// gets one first, so that the two lines do not run into each other.
+func merge(content, line string) (string, bool) {
+	if slices.Contains(strings.Split(content, "\n"), line) {
+		return content, false
+	}
+
+	if content != "" && !strings.HasSuffix(content, "\n") {
+		content += "\n"
+	}
+
+	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
+	}
+
+	//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)
+	}
+
+	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.
+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
+		}
+	}
+
+	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.
+func sidecarName() (string, error) {
+	random := make([]byte, sidecarBytes)
+
+	_, err := rand.Read(random)
+	if err != nil {
+		return "", fmt.Errorf("making a name for the new file: %w", err)
+	}
+
+	return sidecarPrefix + hex.EncodeToString(random), nil
+}
+
+// quoted puts the double quotes around a path that sftp needs when the
+// path has a space in it. Only paths of the tool's own making are
+// given to it, and they hold no quote of their own.
+func quoted(path string) string {
+	return `"` + path + `"`
 }

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,
+				)
+			}
+		})
+	}
+}