ssh install refuses stray arguments before -- and a symlinked authorized_keys (closes #61)

Anything but the host before --, or a second argument when there is no --, used to reach sftp in front of the host; it is now refused before the mnemonic is read or any connection is made. That includes "install -- host", which names no host before --.

The first listing of ~/.ssh is now a long one, ls -n, which the client formats itself whatever the server, so an authorized_keys that is a symlink shows as one. It is refused before any upload, since the rename would have replaced the link and left the file it points at without the key. The README says so.

Model: opus-5-5

internal/cli/ssh/install.go

@@ -49,6 +49,20 @@ var ErrCannotEnter = errors.New(
 	"~/.ssh is there on the host but cannot be entered",
 )
 
+// ErrSymlink is the refusal of a host whose authorized_keys is a
+// symlink: the rename that puts the new file in place would replace the
+// link itself, and the file it points at would never get the key.
+var ErrSymlink = errors.New(
+	"~/.ssh/authorized_keys on the host is a symlink, which the tool " +
+		"leaves alone",
+)
+
+// ErrStrayArgument is the refusal of anything but the host before --,
+// which would otherwise be handed to sftp in front of the host.
+var ErrStrayArgument = errors.New(
+	"only the host goes before --; options for sftp go after --",
+)
+
 // install returns the command that adds the public key to a host.
 func install() *cobra.Command {
 	cmd := &cobra.Command{
@@ -60,7 +74,22 @@ func install() *cobra.Command {
 			"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),
+		Args: cobra.MatchAll(
+			cobra.MinimumNArgs(1),
+			func(cmd *cobra.Command, args []string) error {
+				// ArgsLenAtDash is -1 when there is no --.
+				before := cmd.ArgsLenAtDash()
+				if before == -1 {
+					before = len(args)
+				}
+
+				if before != 1 {
+					return ErrStrayArgument
+				}
+
+				return nil
+			},
+		),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			key, comment, err := derived(cmd)
 			if err != nil {
@@ -256,14 +285,23 @@ func merge(content, line string) (string, bool) {
 // it can be looked up, not even ".". The first listing of such a
 // directory comes up empty, as the server leaves out every name it
 // cannot look up.
+//
+// The first listing is a long one, which shows an authorized_keys that
+// is a symlink as one. That is refused before anything else sftp said
+// is read, so a link the get could not follow is refused in the same
+// words.
 func fetch(
 	cmd *cobra.Command, host string, options []string, into string,
 ) (string, bool, error) {
 	said, err := session(cmd, host, options, []string{
-		"ls -1 " + directory,
+		"ls -n " + directory,
 		"ls -1 " + directory + "/.",
 		"get " + authorized + " " + quoted(into),
 	})
+	if symlinked(said) {
+		return "", false, ErrSymlink
+	}
+
 	if err != nil {
 		if listingNotFound(said, directory) {
 			return "", false, nil
@@ -326,6 +364,22 @@ func reportedCannotList(line string) (string, bool) {
 	return strings.TrimSuffix(strings.TrimPrefix(line, before), after), true
 }
 
+// symlinked says whether the long listing of .ssh shows authorized_keys
+// as a symlink. With -n the client writes each line itself, as ls -l
+// does, whatever the server: the type comes first, "l" for a symlink,
+// and the path as the listing named it comes last.
+func symlinked(said string) bool {
+	for line := range strings.Lines(said) {
+		fields := strings.Fields(line)
+		if len(fields) > 0 && strings.HasPrefix(fields[0], "l") &&
+			fields[len(fields)-1] == authorized {
+			return true
+		}
+	}
+
+	return false
+}
+
 // 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

internal/cli/ssh/install_test.go

@@ -10,7 +10,7 @@ import "testing"
 const (
 	echoed = `sftp> get .ssh/authorized_keys "/tmp/keyfunc/authorized_keys"
 `
-	listed  = "sftp> ls -1 .ssh\n"
+	listed  = "sftp> ls -n .ssh\n"
 	warning = `Warning: Identity file /gone not accessible: ` +
 		"No such file or directory.\n"
 )

internal/cli/ssh_test.go

@@ -99,6 +99,8 @@ const marker = "KEYFUNC_TEST_MARKER"
 //
 // The listing and the two ways a get can fail are worded as the
 // OpenSSH client words them, each naming the path the server expanded.
+// A long listing (-n) writes each entry as the client does, its type
+// first, so that a symlink shows as one.
 // A listing fails one way when .ssh is not there and another when it is
 // there but shut to the user; the first is the only failure read as a
 // host with no file. A get fails one way for a file that is not there,
@@ -137,8 +139,7 @@ while IFS= read -r line; do
 	worked=yes
 	case "$1" in
 	ls)
-		dir=$2
-		[ "$dir" = -1 ] && dir=$3
+		dir=$3
 		if [ ! -e "$home/$dir" ]; then
 			worked=no
 			printf 'Can'\''t ls: "%s" not found\n' "$home/$dir" >&2
@@ -149,7 +150,13 @@ while IFS= read -r line; do
 		else
 			for entry in "$home/$dir"/*; do
 				[ -e "$entry" ] || continue
-				printf '%s/%s\n' "$dir" "$(basename "$entry")"
+				name="$dir/$(basename "$entry")"
+				if [ "$2" = -n ]; then
+					printf '%s    ? someone  users  0 Oct  4 15:44 %s\n' \
+						"$(stat -c '%A' "$entry")" "$name"
+				else
+					printf '%s\n' "$name"
+				fi
 			done
 		fi
 		;;
@@ -306,7 +313,7 @@ func TestTheFileIsUploadedBesideTheOldOneAndThenRenamedOverIt(t *testing.T) {
 
 	// The listing fails on a host with no .ssh, so the get never runs;
 	// the write session then makes the directory and puts the file.
-	require.Equal(t, "ls -1 .ssh", sent[0])
+	require.Equal(t, "ls -n .ssh", sent[0])
 	require.Equal(t, "-mkdir .ssh", sent[1])
 	require.Equal(t, "chmod 700 .ssh", sent[2])
 	require.Equal(t, "put", strings.Fields(sent[3])[0])
@@ -372,6 +379,59 @@ func TestADirectoryThatCannotBeEnteredIsRefusedBeforeAnyUpload(t *testing.T) {
 	require.Equal(t, notADirectory, read(t, inTheWay))
 }
 
+func TestASymlinkedFileIsRefusedBeforeAnyUpload(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+
+	// The file the link points at, which the key would never reach.
+	target := filepath.Join(pretend.home, "keys")
+	require.NoError(t,
+		os.WriteFile(target, []byte("somebody else\n"), fileMode),
+	)
+
+	directory := filepath.Join(pretend.home, keptUnder)
+	require.NoError(t, os.Mkdir(directory, directoryMode))
+
+	link := filepath.Join(directory, keptIn)
+	require.NoError(t, os.Symlink(target, link))
+
+	printed, _, err := attempt(t, host)
+	require.ErrorIs(t, err, ssh.ErrSymlink)
+	require.Empty(t, printed)
+
+	// The read and nothing after it: no upload was tried, the link
+	// still points where it did, and what it points at is unchanged.
+	require.Equal(t, 1, connections(t, pretend))
+
+	pointsAt, err := os.Readlink(link)
+	require.NoError(t, err)
+	require.Equal(t, target, pointsAt)
+	require.Equal(t, "somebody else\n", read(t, target))
+}
+
+func TestAnArgumentBesideTheHostIsRefusedBeforeAnyConnection(t *testing.T) {
+	t.Setenv(mnemonic.Variable, example())
+
+	pretend := pretendHost(t)
+
+	runs := [][]string{
+		{host, "frank@example.com"},
+		{host, "2222"},
+		{host, "frank@example.com", "--", "-P", "2222"},
+		{"--", host},
+	}
+
+	for _, args := range runs {
+		printed, _, err := attempt(t, args...)
+		require.ErrorIs(t, err, ssh.ErrStrayArgument)
+		require.Empty(t, printed)
+	}
+
+	// sftp was never started, so nothing was uploaded.
+	require.NoFileExists(t, pretend.arguments)
+}
+
 func TestAnExistingDirectoryKeepsItsModeAndIsNotRemade(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())