diff --git a/README.md b/README.md index a5b03d6..d3dda4c 100644 --- a/README.md +++ b/README.md @@ -88,17 +88,37 @@ 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`; 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: -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, 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`. + +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..15599d6 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -1,57 +1,49 @@ package ssh import ( + "crypto/rand" + "encoding/hex" + "errors" "fmt" + "io/fs" + "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 +56,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 +65,150 @@ 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 + "'", - }) +// 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("making a temporary directory: %w", err) + } - //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() + 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) + 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. + err = session(cmd, host, options, []string{ + "-mkdir " + directory, + "chmod " + directoryMode + " " + directory, + "put " + quoted(local) + " " + sidecar, + "chmod " + fileMode + " " + sidecar, + "rename " + sidecar + " " + authorized, + }) + if err != nil { + return fmt.Errorf("%w; %s may be left on the host", err, sidecar) + } + + 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. +func session( + cmd *cobra.Command, host string, options []string, batch []string, +) error { + argv := slices.Concat( + []string{"-b", "-"}, options, []string{host}, + ) + + //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() err := command.Run() if err != nil { - return fmt.Errorf("running ssh: %w", err) + return fmt.Errorf("running sftp: %w", err) } return 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 +} + +// 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 + } + + if err != nil { + return "", fmt.Errorf("reading the fetched file: %w", err) + } + + return string(content), nil +} + +// 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_test.go b/internal/cli/ssh_test.go index 6b787fc..082a50a 100644 --- a/internal/cli/ssh_test.go +++ b/internal/cli/ssh_test.go @@ -3,6 +3,7 @@ package cli_test import ( "os" "path/filepath" + "slices" "strconv" "strings" "testing" @@ -14,7 +15,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 @@ -32,20 +33,45 @@ 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 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. 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 '%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) 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 ;; + 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,19 +89,17 @@ 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) @@ -94,11 +118,32 @@ 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", 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. + 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", run(t, "ssh", "install", host)) + require.Equal(t, keyLine, read(t, path)) } func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) { @@ -106,22 +151,38 @@ func TestTheKeyDoesNotRunIntoALineWithNoNewlineAtItsEnd(t *testing.T) { pretend := pretendHost(t) already := "ssh-ed25519 AAAAsomebodyelse somebody@else" - - 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)) + path := seed(t, pretend, already) 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, already+"\n"+keyLine, read(t, path)) } -func TestTheKeyLineIsNotOnTheCommandLine(t *testing.T) { +func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + pretend := pretendHost(t) + + require.Equal(t, "added\n", run(t, "ssh", "install", 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 TestTheKeyLineIsNotSentAsACommand(t *testing.T) { t.Setenv(mnemonic.Variable, example()) pretend := pretendHost(t) @@ -129,18 +190,21 @@ func TestTheKeyLineIsNotOnTheCommandLine(t *testing.T) { run(t, "ssh", "install", 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") + 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. + session := []string{"-b", "-", "-P", "2222", host} require.Equal(t, - []string{"-p", "2222", host}, + slices.Concat(session, session), recorded(t, pretend.arguments), ) } @@ -190,17 +254,31 @@ 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 } +// 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 +291,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 +326,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()