diff --git a/README.md b/README.md index a5b03d6..7dcb19f 100644 --- a/README.md +++ b/README.md @@ -88,17 +88,49 @@ Prints the unencrypted private key in OpenSSH format (the and nothing else, so it can be redirected into a file. The key's comment is the same as for `pub`. -### `keyfunc ssh install <[user@]host> [-- ssh options...]` +### `keyfunc ssh install <[user@]host> [-- sftp options...]` -Runs the system `ssh` to the host and, on the host: +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. -- creates `~/.ssh` with mode `0700` if it is missing; -- creates `~/.ssh/authorized_keys` with mode `0600` if it is missing; -- appends the `pub` line only if an identical line is not already there. +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 +`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: -It then prints `added` or `already present`. How this `ssh` connection -authenticates is up to the user's normal `ssh` setup (existing keys, agent, -password). Anything after `--` is passed to `ssh` unchanged. +- creates `~/.ssh` and sets it to mode `0700`; +- uploads the new file as `~/.ssh/authorized_keys.keyfunc-` and sets it + to mode `0600`; +- renames that file over `~/.ssh/authorized_keys`. + +The tool then prints `added`. So a run that adds a line connects twice. The +rename is the step that either happens or does not: the file on the host is +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` +writes goes to standard error, so the tool's own standard output is only +`added` or `already present`. + +Anything after `--` is passed to `sftp` unchanged, which is where the port goes +(`-P 2222`, not `-p`). How the connection authenticates is up to the user's +normal `ssh` setup, except that batch mode does not prompt: a key or an agent +has to do it, not a typed password. ### `keyfunc ssh to [ssh arguments...]` diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 8133261..5886a69 100644 --- a/internal/cli/ssh/install.go +++ b/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 + `"` } diff --git a/internal/cli/ssh/install_test.go b/internal/cli/ssh/install_test.go new file mode 100644 index 0000000..051014c --- /dev/null +++ b/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, + ) + } + }) + } +} diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 6b787fc..9aafe21 100644 --- a/internal/cli/ssh_test.go +++ b/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,21 @@ 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" + +// 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 ( @@ -32,20 +47,77 @@ 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. +// 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" +) + +// 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. +// +// 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. const installer = ` -while [ $# -gt 1 ]; do - printf '%s\n' "$1" >> "$KEYFUNC_TEST_ARGUMENTS" - shift +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 + 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 + 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 + 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 +135,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 +164,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 +195,145 @@ func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) { pretend := pretendHost(t) already := "ssh-ed25519 AAAAsomebodyelse somebody@else" + path := seed(t, pretend, already) - 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)) - - require.Equal(t, "added\n", run(t, "ssh", "install", host)) - require.Equal(t, - already+"\n"+vectorZero+" keyfunc/ssh/0\n", - read(t, path), - ) + require.Equal(t, "added\n", install(t, host)) + require.Equal(t, already+"\n"+keyLine, read(t, path)) } -func TestTheKeyLineIsNotOnTheCommandLine(t *testing.T) { +func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) { t.Setenv(mnemonic.Variable, example()) pretend := pretendHost(t) - run(t, "ssh", "install", host) + 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) + unreadable := unfetchable(t, pretend) + + printed, said, err := attempt(t, host) + require.Error(t, err) + require.Empty(t, printed) + require.Contains(t, said, "Permission denied") + + // 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 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()) + + 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{tool, subcommand, installing, 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 +343,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 +370,7 @@ func TestTheToolEndsWithTheStatusSSHEndedWith(t *testing.T) { t.Cleanup(func() { os.Args = given }) - os.Args = []string{"keyfunc", "ssh", "to", host, "uptime"} + os.Args = []string{tool, subcommand, "to", host, "uptime"} require.Equal(t, failingStatus, cli.Main()) } @@ -190,17 +383,78 @@ 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, installing}, 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 +} + +// 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. @@ -213,20 +467,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 +502,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()