From 11a6050ac7aea6a59e6d34498ea0140c5ff78885 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 4 Oct 2026 15:58:13 +0000 Subject: [PATCH] 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 --- README.md | 11 ++++-- internal/cli/ssh/install.go | 58 ++++++++++++++++++++++++++- internal/cli/ssh/install_test.go | 2 +- internal/cli/ssh_test.go | 68 ++++++++++++++++++++++++++++++-- 4 files changed, 128 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 511f00c..4128af2 100644 --- a/README.md +++ b/README.md @@ -177,10 +177,13 @@ same way: one that cannot be read fails the first listing, and one that can be read but not entered fails the second, after which the tool says that `~/.ssh` cannot be entered. The wording of a missing file elsewhere does not count either, since `ssh` writes `No such file or directory` about an `-i` it cannot -find on a session that then authenticates through the agent. 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: +find on a session that then authenticates through the agent. An +`authorized_keys` that the first listing shows to be a symlink is refused and +left as it is, since the rename below would replace the link itself and the file +it points at would never get the key. 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: - makes `~/.ssh` and sets it to mode `0700`, but only when the first connection found none; a `~/.ssh` that was already there keeps the mode it had; diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 1e89e2a..1837e68 100644 --- a/internal/cli/ssh/install.go +++ b/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 diff --git a/internal/cli/ssh/install_test.go b/internal/cli/ssh/install_test.go index 510404a..cc25219 100644 --- a/internal/cli/ssh/install_test.go +++ b/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" ) diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 5342323..3aa8ef8 100644 --- a/internal/cli/ssh_test.go +++ b/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()) -- 2.54.0