Refactored to make flow in beIncubator clearer

Signed-off-by: Percy Wegmann <percy@tailscale.com>

ssh/tailssh/incubator.go

@@ -49,9 +49,12 @@ var ptyName = func(f *os.File) (string, error) {
 	return "", fmt.Errorf("unimplemented")
 }
 
-// maybeStartLoginSession starts a new login session for the specified UID.
-// On success, it may return a non-nil close func which must be closed to
+// maybeStartLoginSession informs the system that we are about to log someone
+// in. On success, it may return a non-nil close func which must be closed to
 // release the session.
+// We can only do this if we are running as root.
+// This is best effort to still allow running on machines where
+// we don't support starting sessions, e.g. darwin.
 // See maybeStartLoginSessionLinux.
 var maybeStartLoginSession = func(logf logger.Logf, ia incubatorArgs) (close func() error, err error) {
 	return nil, nil
@@ -117,14 +120,13 @@ func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd) {
 		incubatorArgs = append(incubatorArgs, "--debug-test")
 	}
 
-	if isSFTP {
+	switch {
+	case isSFTP:
 		incubatorArgs = append(incubatorArgs, "--sftp")
-	} else {
-		if isShell {
-			incubatorArgs = append(incubatorArgs, "--shell")
-		} else {
-			incubatorArgs = append(incubatorArgs, "--cmd="+ss.RawCommand())
-		}
+	case isShell:
+		incubatorArgs = append(incubatorArgs, "--shell")
+	default:
+		incubatorArgs = append(incubatorArgs, "--cmd="+ss.RawCommand())
 	}
 
 	return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...)
@@ -227,7 +229,7 @@ func beIncubator(args []string) error {
 			logf = log.New(sl, "", 0).Printf
 		}
 	} else if ia.debugTest {
-		// In testing, we don't always have syslog, log to a temp file
+		// In testing, we don't always have syslog, so log to a temp file.
 		if logFile, err := os.OpenFile("/tmp/tailscalessh.log", os.O_APPEND|os.O_WRONLY, 0666); err == nil {
 			lf := log.New(logFile, "", 0)
 			logf = func(msg string, args ...any) {
@@ -238,52 +240,51 @@ func beIncubator(args []string) error {
 		}
 	}
 
-	attemptLoginShell := shouldAttemptLoginShell(ia)
-	if !attemptLoginShell {
+	switch {
+	case ia.isSFTP:
+		return handleSFTPInProcess(logf, ia)
+	case !shouldAttemptLoginShell(ia):
 		logf("not attempting login shell")
-	} else if err := tryExecLogin(logf, ia); err != nil {
-		return err
-	}
+		return handleSSHInProcess(logf, ia)
+	default:
+		// First try the login command
+		if err := tryExecLogin(logf, ia); err != nil {
+			return err
+		}
+
+		// If we got here, we weren't able to use login (because tryExecLogin
+		// returned without replacing the running process), maybe we can use
+		// su.
+		if handled, err := trySU(logf, ia); handled {
+			return err
+		} else {
+			logf("not attempting su")
+			return handleSSHInProcess(logf, ia)
+		}
+	}
+}
+
+// handleSFTPInProcess serves SFTP connections.
+func handleSFTPInProcess(logf logger.Logf, ia incubatorArgs) error {
+	logf("handling sftp")
+
+	// In order to trigger PAM modules like pam_mkhomedir to run, call
+	// findSU, which as a side-effect will actually invoke the su command,
+	// which will trigger the creation of a homedir if so configured.
+	// Note - we won't actually be handling SFTP within a PAM session, so
+	// modules like pam_tty_audit won't work, only side-effecting modules
+	// like pam_mkhomedir will have an effect.
+	_, _ = findSU(logf, ia)
 
-	// Inform the system that we are about to log someone in.
-	// We can only do this if we are running as root.
-	// This is best effort to still allow running on machines where
-	// we don't support starting sessions, e.g. darwin.
 	sessionCloser, err := maybeStartLoginSession(logf, ia)
 	if err == nil && sessionCloser != nil {
 		defer sessionCloser()
 	}
 
-	if attemptLoginShell {
-		// If we got here, we weren't able to use login (because tryExecLogin returned without replacing the running process), maybe we can use su.
-		if handled, err := trySU(logf, ia); handled {
-			return err
-		} else {
-			logf("not attempting su")
-		}
-	}
-
-	// login and su didn't work, drop privileges and handle in-process.
 	if err := dropPrivileges(logf, ia); err != nil {
 		return err
 	}
 
-	if ia.isSFTP {
-		// Note - this does not trigger PAM authentication, so things like
-		// pam_mkhomedir won't work with FTP.
-		// TODO: it would be nice to make SFTP work with PAM, but that might
-		// require directly talking to the PAM API, which would require us to
-		// introduce CGO.
-		return handleSFTP(logf)
-	}
-
-	return handleSSHInProcess(logf, ia)
-}
-
-// handleSFTP serves SFTP connections.
-func handleSFTP(logf logger.Logf) error {
-	logf("handling sftp")
-
 	server, err := sftp.NewServer(stdRWC{})
 	if err != nil {
 		return err
@@ -300,7 +301,6 @@ func handleSFTP(logf logger.Logf) error {
 // login shell with the login or su commands. We will attempt a login shell
 // if all of the following conditions are met.
 //
-// - This is not an sftp session
 // - We are running as root
 // - This is not an SELinuxEnforcing host
 //
@@ -310,7 +310,7 @@ func handleSFTP(logf logger.Logf) error {
 // the incubator to launch the shell.
 // See http://github.com/tailscale/tailscale/issues/4908.
 func shouldAttemptLoginShell(ia incubatorArgs) bool {
-	return !ia.isSFTP && runningAsRoot() && !hostinfo.IsSELinuxEnforcing()
+	return runningAsRoot() && !hostinfo.IsSELinuxEnforcing()
 }
 
 func runningAsRoot() bool {
@@ -378,32 +378,19 @@ func tryExecLogin(logf logger.Logf, ia incubatorArgs) error {
 // an su command which accepts the right flags, we'll use su instead of login
 // when no TTY is available.
 func trySU(logf logger.Logf, ia incubatorArgs) (bool, error) {
-	// Currently, we only support falling back to su on Linux. This
-	// potentially could work on BSDs as well, but requires testing.
-	if runtime.GOOS != "linux" {
+	su, found := findSU(logf, ia)
+	if !found {
 		return false, nil
 	}
 
-	su, err := exec.LookPath("su")
-	if err != nil {
-		logf("can't find su command: %v", err)
-		return false, nil
-	}
-
-	// First try to execute su -l <user> -c id to make sure su supports the
-	// necessary arguments.
-	err = exec.Command("su", "-l", ia.localUser, "-c", "id").Run()
-	if err != nil {
-		logf("su check failed: %s", err)
-		return false, nil
+	sessionCloser, err := maybeStartLoginSession(logf, ia)
+	if err == nil && sessionCloser != nil {
+		defer sessionCloser()
 	}
 
 	loginArgs := []string{"-l", ia.localUser}
-	if !ia.isShell && ia.cmd != "" {
-		// We only execute the requested command if we're not requesting a
-		// shell. When requesting a shell, the command is the requested shell,
-		// which is redundant because `su -l` will give the user their default
-		// shell.
+	if ia.cmd != "" {
+		// Note - unlike the login command, su allows using both -l and -c.
 		loginArgs = append(loginArgs, "-c", ia.cmd)
 	}
 
@@ -412,15 +399,51 @@ func trySU(logf logger.Logf, ia incubatorArgs) (bool, error) {
 	return true, cmd.Run()
 }
 
+// findSU attempts to find an su command which supports the -l and -c flags.
+// This actually calls the su command, which can cause side effects like
+// triggering pam_mkhomedir.
+func findSU(logf logger.Logf, ia incubatorArgs) (string, bool) {
+	// Currently, we only support falling back to su on Linux. This
+	// potentially could work on BSDs as well, but requires testing.
+	if runtime.GOOS != "linux" {
+		return "", false
+	}
+
+	su, err := exec.LookPath("su")
+	if err != nil {
+		logf("can't find su command: %v", err)
+		return "", false
+	}
+
+	// First try to execute su -l <user> -c id to make sure su supports the
+	// necessary arguments.
+	err = exec.Command(su, "-l", ia.localUser, "-c", "pwd").Run()
+	if err != nil {
+		logf("su check failed: %s", err)
+		return "", false
+	}
+
+	return su, true
+}
+
 // handleSSHInProcess is a last resort if we couldn't use login or su. It
 // registers a new session with the OS, sets its UID, GID and groups to the
 // specified values, and then launches the requested `--cmd` in the user's
 // login shell.
 func handleSSHInProcess(logf logger.Logf, ia incubatorArgs) error {
+	sessionCloser, err := maybeStartLoginSession(logf, ia)
+	if err == nil && sessionCloser != nil {
+		defer sessionCloser()
+	}
+
+	if err := dropPrivileges(logf, ia); err != nil {
+		return err
+	}
+
 	args := shellArgs(ia.isShell, ia.cmd)
 	logf("running %s %q", ia.loginShell, args)
 	cmd := newCommand(ia.hasTTY, ia.loginShell, args)
-	err := cmd.Run()
+	err = cmd.Run()
 	if ee, ok := err.(*exec.ExitError); ok {
 		ps := ee.ProcessState
 		code := ps.ExitCode()

ssh/tailssh/tailssh_integration_test.go

@@ -92,14 +92,6 @@ func TestIntegrationSSH(t *testing.T) {
 		homeDir = "/Users/testuser"
 	}
 
-	_, err := exec.LookPath("su")
-	suPresent := err == nil
-
-	// Some operating systems like Fedora seem to require login to be present
-	// in order for su to work.
-	_, err = exec.LookPath("login")
-	loginPresent := err == nil
-
 	tests := []struct {
 		cmd  string
 		want []string
@@ -112,12 +104,12 @@ func TestIntegrationSSH(t *testing.T) {
 		{
 			cmd:  "pwd",
 			want: []string{homeDir},
-			skip: runtime.GOOS != "linux" || !suPresent || !loginPresent,
+			skip: !fallbackToSUAvailable(),
 		},
 		{
 			cmd:  "echo 'hello'",
 			want: []string{"hello"},
-			skip: runtime.GOOS != "linux" || !suPresent || !loginPresent,
+			skip: !fallbackToSUAvailable(),
 		},
 	}
 
@@ -172,7 +164,10 @@ func TestIntegrationSFTP(t *testing.T) {
 		debugTest.Store(false)
 	})
 
-	filePath := "/tmp/sftptest.dat"
+	filePath := "/home/testuser/sftptest.dat"
+	if !fallbackToSUAvailable() {
+		filePath = "/tmp/sftptest.dat"
+	}
 	wantText := "hello world"
 
 	cl := testClient(t)
@@ -216,6 +211,22 @@ func TestIntegrationSFTP(t *testing.T) {
 	}
 }
 
+func fallbackToSUAvailable() bool {
+	if runtime.GOOS != "linux" {
+		return false
+	}
+
+	_, err := exec.LookPath("su")
+	if err != nil {
+		return false
+	}
+
+	// Some operating systems like Fedora seem to require login to be present
+	// in order for su to work.
+	_, err = exec.LookPath("login")
+	return err == nil
+}
+
 type session struct {
 	*ssh.Session
 

ssh/tailssh/testcontainers/Dockerfile

@@ -16,23 +16,23 @@ RUN authconfig --enablemkhomedir --update || echo "might not be fedora"
 COPY . .
 
 RUN echo "First run tests normally."
-RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP TestIntegrationSSH
 
 RUN echo "Then run tests as non-root user testuser and make sure tests still pass."
 RUN chown testuser:groupone /tmp/tailscalessh.log
-RUN TAILSCALED_PATH=`pwd`tailscaled su -m testuser -c "./tailssh.test -test.v -test.run TestIntegration TestDoDropPrivileges"
+RUN TAILSCALED_PATH=`pwd`tailscaled su -m testuser -c "./tailssh.test -test.v -test.run TestIntegrationSFTP TestIntegrationSSH TestDoDropPrivileges"
 
 RUN echo "Then remove the login command and make sure tests still pass."
 RUN chown root:root /tmp/tailscalessh.log
 RUN rm `which login`
 RUN rm -Rf /home/testuser
-RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP TestIntegrationSSH
 
 RUN echo "Then remove the su command and make sure tests still pass."
 RUN chown root:root /tmp/tailscalessh.log
 RUN rm `which su`
 RUN rm -Rf /home/testuser
-RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP TestIntegrationSSH
 
 RUN echo "Test doDropPrivileges"
 RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestDoDropPrivileges