From 56e20b66e45374105324f4239edc36b3929e329f Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 4 Oct 2026 18:25:45 +0200 Subject: [PATCH] ssh install refuses stray arguments before -- and a symlinked authorized_keys (closes #61) ssh install now takes the host alone before --: any other word there, or a second argument without --, is refused before the mnemonic is read or sftp runs, so keyfunc ssh install alice@host frank@host no longer installs the key for frank@host. The first listing of ~/.ssh is now ls -n, which shows the file type, so a symlinked authorized_keys is refused before any upload instead of being replaced by a regular file; the README says so. Judgement calls: install -- host is refused; a symlinked authorized_keys is refused even when its target already holds the key. Model: opus-5-5 Co-authored-by: clawbot --- 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())