ssh install tells a missing .ssh from one it cannot enter (closes #10)

The first sftp connection now lists ~/.ssh before it fetches
authorized_keys. The file reads as empty in just two cases: sftp reports
~/.ssh itself as not there, or the listing succeeds and the fetch then
reports the file as not there. A directory that is there but cannot be
entered, or a file that cannot be read, fails the run and writes nothing,
so a ~/.ssh whose mode shuts the user out is no longer read as a host
with no file and replaced by one holding the new key alone. The write
connection makes ~/.ssh and sets 0700 only when the read found none; an
existing directory keeps its mode.

Model: opus-4-8

internal/cli/ssh/install.go

@@ -76,7 +76,7 @@ func add(cmd *cobra.Command, host string, options []string, line string) error {
 
 	defer func() { _ = os.RemoveAll(work) }()
 
-	content, err := fetch(cmd, host, options,
+	content, present, err := fetch(cmd, host, options,
 		filepath.Join(work, "authorized_keys"),
 	)
 	if err != nil {
@@ -88,16 +88,18 @@ func add(cmd *cobra.Command, host string, options []string, line string) error {
 		return write(cmd, "already present\n")
 	}
 
-	return upload(cmd, host, options, work, merged)
+	return upload(cmd, host, options, work, merged, present)
 }
 
 // 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.
+// so that it can be looked at and cleared away by hand. The directory
+// is made and set to its mode only when the read found none: an .ssh
+// that was already there is left with the mode it had.
 func upload(
 	cmd *cobra.Command, host string, options []string,
-	work, merged string,
+	work, merged string, present bool,
 ) error {
 	local := filepath.Join(work, "authorized_keys.merged")
 
@@ -111,14 +113,24 @@ func upload(
 		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,
-	})
+	var batch []string
+
+	if !present {
+		// The mkdir is allowed to fail in case the directory appeared
+		// between the read and now; the chmod then sets its mode.
+		batch = append(batch,
+			"-mkdir "+directory,
+			"chmod "+directoryMode+" "+directory,
+		)
+	}
+
+	batch = append(batch,
+		"put "+quoted(local)+" "+sidecar,
+		"chmod "+fileMode+" "+sidecar,
+		"rename "+sidecar+" "+authorized,
+	)
+
+	said, err := session(cmd, host, options, batch)
 	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
@@ -185,31 +197,82 @@ func merge(content, line string) (string, bool) {
 }
 
 // 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.
+// returns what is in it, and whether the .ssh directory was already
+// there. The one session lists .ssh and then gets the file, so the
+// listing settles the state of the directory before the get is read.
+//
+// The file reads as empty in just two cases: sftp reported .ssh itself
+// as not there, or the listing succeeded and the get then reported the
+// file as not there. Anything else — the listing refused, the file
+// there but unreadable, the connection down — fails the run and writes
+// nothing, because writing back over what was not read would leave the
+// host with the new key and nothing else. sftp cannot tell a missing
+// file from one in a directory it cannot enter, so the listing does:
+// a directory that is there but cannot be read is a failure, not an
+// empty file.
 func fetch(
 	cmd *cobra.Command, host string, options []string, into string,
-) (string, error) {
+) (string, bool, error) {
 	said, err := session(cmd, host, options, []string{
+		"ls -1 " + directory,
 		"get " + authorized + " " + quoted(into),
 	})
 	if err != nil {
-		if absent(said) {
-			return "", nil
+		if directoryAbsent(said) {
+			return "", false, nil
 		}
 
-		return "", err
+		if absent(said) {
+			return "", true, nil
+		}
+
+		return "", false, 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 "", false, fmt.Errorf("reading the fetched file: %w", err)
 	}
 
-	return string(content), nil
+	return string(content), true, nil
+}
+
+// directoryAbsent says whether sftp reported .ssh itself as not being
+// there, which is the one listing failure read as a host that has no
+// authorized_keys yet. The reading is taken only from the line in which
+// sftp reports on that directory: any other failure of the listing, in
+// particular a directory that is there but cannot be entered, is left
+// as a failure, so that no key is written to a host whose keys were
+// never read.
+func directoryAbsent(said string) bool {
+	for line := range strings.Lines(said) {
+		named, is := reportedCannotList(strings.TrimSpace(line))
+		if is && (named == directory ||
+			strings.HasSuffix(named, "/"+directory)) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// reportedCannotList returns the path an sftp line reports it cannot
+// list for want of the directory, and whether the line is such a
+// report. The client writes this one wording when the directory a
+// listing names is not there, giving the path the server expanded.
+func reportedCannotList(line string) (string, bool) {
+	const (
+		before = `Can't ls: "`
+		after  = `" not found`
+	)
+
+	if !strings.HasPrefix(line, before) ||
+		!strings.HasSuffix(line, after) {
+		return "", false
+	}
+
+	return strings.TrimSuffix(strings.TrimPrefix(line, before), after), true
 }
 
 // absent says whether sftp reported the file that was asked for as

internal/cli/ssh/install_test.go

@@ -10,6 +10,7 @@ import "testing"
 const (
 	echoed = `sftp> get .ssh/authorized_keys "/tmp/keyfunc/authorized_keys"
 `
+	listed  = "sftp> ls -1 .ssh\n"
 	warning = `Warning: Identity file /gone not accessible: ` +
 		"No such file or directory.\n"
 )
@@ -76,3 +77,57 @@ func TestAbsenceIsReadOnlyFromWhatSFTPSaidAboutAuthorizedKeys(t *testing.T) {
 		})
 	}
 }
+
+// TestTheDirectoryIsReadAsAbsentOnlyFromTheListingSayingSo holds the
+// wordings the OpenSSH client was seen to use when a listing fails: a
+// directory it cannot find is reported one way, and one it cannot enter
+// another, and only the first is read as a host with no .ssh yet.
+func TestTheDirectoryIsReadAsAbsentOnlyFromTheListingSayingSo(t *testing.T) {
+	t.Parallel()
+
+	listings := map[string]struct {
+		said string
+		want bool
+	}{
+		"the directory is not there": {
+			said: listed + `Can't ls: "/home/someone/.ssh" not found` + "\n",
+			want: true,
+		},
+		"the directory is not there, named as it was asked for": {
+			said: listed + `Can't ls: ".ssh" not found` + "\n",
+			want: true,
+		},
+		"the directory is not there and an identity file is not either": {
+			said: warning + listed +
+				`Can't ls: "/home/someone/.ssh" not found` + "\n",
+			want: true,
+		},
+		"the directory is there and cannot be entered": {
+			said: listed +
+				`remote readdir("/home/someone/.ssh/"): Permission denied` + "\n",
+			want: false,
+		},
+		"some other directory is not there": {
+			said: listed + `Can't ls: "/home/someone/.config" 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, listing := range listings {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			if directoryAbsent(listing.said) != listing.want {
+				t.Errorf(
+					"read as absent: %t, wanted %t, from:\n%s",
+					!listing.want, listing.want, listing.said,
+				)
+			}
+		})
+	}
+}