From d8abcce89be37f0d0dee6a7e8ce43a11f6a746b4 Mon Sep 17 00:00:00 2001
From: Kristoffer Dalby <kristoffer@tailscale.com>
Date: Wed, 8 Apr 2026 11:59:52 +0000
Subject: [PATCH] ssh/tailssh: add spec citations and source references to
 comments

Add RFC and source references to the comments introduced in the
previous commit:

- SIGHUP: cite OpenSSH session.c:2246 (PTY master close) and POSIX
  General Terminal Interface for terminal disconnect semantics
- Exit codes: cite OpenSSH ssh.c:1693 for the 255 convention, POSIX
  Shell Command Language for the 127 convention
- Exit/Close ordering: cite RFC 4254 Section 6.10 (exit-status before
  CHANNEL_CLOSE) and Section 5.3 (EOF semantics)
- defer ss.Close: explain why Close is deferred and cite RFC 4254

No behavioral changes, only documentation.

Updates #18256

Change-Id: Ice00d19d0462f1d4e6d454df0079c80b3917f5df
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
---
 ssh/tailssh/tailssh.go | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go
index 90bc86ffc..8f64793d7 100644
--- a/ssh/tailssh/tailssh.go
+++ b/ssh/tailssh/tailssh.go
@@ -878,7 +878,12 @@ func (ss *sshSession) killProcessOnContextDone() {
 		// We don't need to Process.Wait here, sshSession.run() does
 		// the waiting regardless of termination reason.
 
-		// Send SIGHUP like a real terminal disconnect would.
+		// Send SIGHUP to match POSIX terminal disconnect semantics.
+		// OpenSSH achieves this implicitly by closing the PTY master fd
+		// (see openssh-portable session.c:2246), which causes the kernel
+		// to send SIGHUP to the child process group. Since tailssh uses
+		// pipes for non-PTY sessions, we send SIGHUP explicitly.
+		// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html#tag_11_01_03
 		ss.cmd.Process.Signal(syscall.SIGHUP)
 	})
 }
@@ -974,10 +979,18 @@ func (ss *sshSession) run() {
 	metricActiveSessions.Add(1)
 	defer metricActiveSessions.Add(-1)
 	defer ss.cancelCtx(errSessionDone)
+	// Close the channel after all I/O and exit-status have been sent.
+	// Per RFC 4254 Section 6.10, SSH_MSG_CHANNEL_CLOSE must come after
+	// exit-status. Since Exit() no longer calls Close() (to allow the
+	// caller to control ordering), we close explicitly here.
+	// https://datatracker.ietf.org/doc/html/rfc4254#section-6.10
 	defer ss.Close()
 
 	if attached := ss.conn.srv.attachSessionToConnIfNotShutdown(ss); !attached {
 		fmt.Fprintf(ss, "Tailscale SSH is shutting down\r\n")
+		// Exit code 255 matches OpenSSH's convention for SSH protocol/infrastructure
+		// errors that prevent the session from starting.
+		// https://github.com/openssh/openssh-portable/blob/master/ssh.c#L1693
 		ss.Exit(255)
 		return
 	}
@@ -1000,8 +1013,9 @@ func (ss *sshSession) run() {
 		if lu.Uid != fmt.Sprint(euid) {
 			ss.logf("can't switch to user %q from process euid %v", lu.Username, euid)
 			fmt.Fprintf(ss, "can't switch user\r\n")
-			// Exit code 255 indicates SSH protocol/permission error,
-			// matching OpenSSH behavior for fatal errors.
+			// Exit code 255 matches OpenSSH's convention for SSH permission errors
+			// that prevent the session from starting.
+			// https://github.com/openssh/openssh-portable/blob/master/ssh.c#L1693
 			ss.Exit(255)
 			return
 		}
@@ -1054,7 +1068,8 @@ func (ss *sshSession) run() {
 				}
 			}
 		} else if isNotFoundOrExecutable(err) {
-			// Use exit code 127 for "command not found" per shell convention.
+			// Exit code 127 for "command not found" per POSIX shell convention.
+			// https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
 			exitCode = 127
 		}
 		ss.Exit(exitCode)
@@ -1083,6 +1098,10 @@ func (ss *sshSession) run() {
 		if _, err := io.Copy(rec.writer("o", ss), ss.rdStdout); err != nil && !errors.Is(err, io.EOF) {
 			logf("stdout copy: %v", err)
 		}
+		// Send EOF to the SSH client when stdout copying completes.
+		// Per RFC 4254 Section 5.3, SSH_MSG_CHANNEL_EOF signals that no
+		// more data will be sent. The channel remains open for exit-status.
+		// https://datatracker.ietf.org/doc/html/rfc4254#section-5.3
 		ss.CloseWrite()
 	}()
 
@@ -1125,8 +1144,12 @@ func (ss *sshSession) run() {
 		exitCode = 1
 	}
 
-	// Send exit-status before EOF/Close. Per RFC 4254 section 6.10,
-	// exit-status should be sent before channel close.
+	// Send exit-status before EOF/Close. Per RFC 4254 Section 6.10,
+	// exit-status must be sent before SSH_MSG_CHANNEL_CLOSE. By sending
+	// it here before closing the child pipes (which triggers the stdout
+	// goroutine to send EOF via CloseWrite), we guarantee the ordering:
+	//   exit-status -> EOF -> CHANNEL_CLOSE
+	// https://datatracker.ietf.org/doc/html/rfc4254#section-6.10
 	ss.Exit(exitCode)
 
 	// Close process-side of pipes to signal io.Copy goroutines