Ineffective attempt making sftp work with pam

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

ssh/tailssh/incubator.go

@@ -104,6 +104,12 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) {
 		remoteUser = strings.Join(ci.node.Tags().AsSlice(), ",")
 	}
 
+	incubatorArgs := ss.buildIncubatorArgs(lu, gids, remoteUser, ci.src.Addr().String(), isSFTP, isShell, name, args)
+	log.Printf("ZZZZ incubator args are: %+s", incubatorArgs)
+	return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...)
+}
+
+func (ss *sshSession) buildIncubatorArgs(lu *userMeta, gids, remoteUser, remoteIP string, isSFTP, isShell bool, cmdName string, cmdArgs []string) []string {
 	incubatorArgs := []string{
 		"be-child",
 		"ssh",
@@ -112,7 +118,7 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) {
 		"--groups=" + gids,
 		"--local-user=" + lu.Username,
 		"--remote-user=" + remoteUser,
-		"--remote-ip=" + ci.src.Addr().String(),
+		"--remote-ip=" + remoteIP,
 		"--has-tty=false", // updated in-place by startWithPTY
 		"--tty-name=",     // updated in-place by startWithPTY
 	}
@@ -122,19 +128,34 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) {
 	}
 
 	if isSFTP {
+		// We have two ways of running an SFTP server.
+		// 1. Immediately launch incubator as SFTP server
+		// 2. Launch incubator to spawn itself as SFTP server inside of a login
+		//    shell.
+		// The second method is preferred because it allows PAM authentication
+		// to run.
+		su, _ := findSUCommand(ss.logf)
+		if su == "" {
+			// We can't use su, just serve sftp directly
 			incubatorArgs = append(incubatorArgs, "--sftp")
-	} else {
+			return incubatorArgs
+		}
+
+		// We can use su, tell incubator to spawn itself
+		cmdName = ss.conn.srv.tailscaledPath
+		cmdArgs = ss.buildIncubatorArgs(lu, gids, remoteUser, remoteIP, false, false, "", nil)
+	}
 	if isShell {
 		incubatorArgs = append(incubatorArgs, "--shell")
 	}
 
-		incubatorArgs = append(incubatorArgs, "--cmd="+name)
+	incubatorArgs = append(incubatorArgs, "--cmd="+cmdName)
-		if len(args) > 0 {
+	if len(cmdArgs) > 0 {
 		incubatorArgs = append(incubatorArgs, "--")
-			incubatorArgs = append(incubatorArgs, args...)
+		incubatorArgs = append(incubatorArgs, cmdArgs...)
 	}
-	}
+
-	return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...)
+	return incubatorArgs
 }
 
 var debugIncubator bool
@@ -229,6 +250,7 @@ func beIncubator(args []string) error {
 			defer logFile.Close()
 		}
 	}
+	logf("ZZZZ Being incubator with args %+s", args)
 
 	if ia.isSFTP {
 		return dropPrivilegesAndHandleSFTP(logf, ia)
@@ -364,43 +386,15 @@ func tryLoginCmd(logf logger.Logf, ia incubatorArgs) (bool, error) {
 // an su command which accepts the right flags, we'll use su instead of login
 // when no TTY is available.
 func tryLoginWithSU(logf logger.Logf, ia incubatorArgs) (bool, error) {
-	// Currently, we only support falling back to su on Linux. This
+	su, supportsPTY := findSUCommand(logf)
-	// potentially could work on BSDs as well, but requires testing.
+	if su == "" {
-	if runtime.GOOS != "linux" {
-		return false, nil
-	}
-
-	su, err := exec.LookPath("su")
-	if err != nil {
-		logf("can't find su command")
-		return false, nil
-	}
-
-	// Get help text to inspect supported flags.
-	out, err := exec.Command(su, "-h").CombinedOutput()
-	if err != nil {
-		logf("%s doesn't support -h, don't use", su)
-		return false, nil
-	}
-
-	supportsFlag := func(flag string) bool {
-		return bytes.Contains(out, []byte(flag))
-	}
-
-	// Make sure su supports the necessary flags.
-	if !supportsFlag("--login") {
-		logf("%s doesn't support --login, don't use", su)
-		return false, nil
-	}
-	if !supportsFlag("--command") {
-		logf("%s doesn't support --command, don't use", su)
 		return false, nil
 	}
 
 	loginArgs := []string{
 		"--login",
 	}
-	if ia.hasTTY && supportsFlag("--pty") {
+	if ia.hasTTY && supportsPTY {
 		// Allocate a pseudo terminal for improved security. In particular,
 		// this can help avoid TIOCSTI ioctl terminal injection.
 		loginArgs = append(loginArgs, "--pty")
@@ -421,6 +415,52 @@ func tryLoginWithSU(logf logger.Logf, ia incubatorArgs) (bool, error) {
 	return true, unix.Exec(su, loginArgs, os.Environ())
 }
 
+// findSUCommand finds the su command and returns the path to it, if and only
+// if.
+//
+// 1. We're running on Linux
+// 2. The su command is on the path
+// 3. The su command supports the -h, --login, and --command flags
+//
+// This also returns a boolean indicating whether or not su supports the --pty
+// flag.
+func findSUCommand(logf logger.Logf) (string, bool) {
+	// Currently, we only support using 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")
+		return "", false
+	}
+
+	// Get help text to inspect supported flags.
+	out, err := exec.Command(su, "-h").CombinedOutput()
+	if err != nil {
+		logf("%s doesn't support -h, don't use", su)
+		return "", false
+	}
+
+	supportsFlag := func(flag string) bool {
+		return bytes.Contains(out, []byte(flag))
+	}
+
+	// Make sure su supports the necessary flags.
+	if !supportsFlag("--login") {
+		logf("%s doesn't support --login, don't use", su)
+		return "", false
+	}
+	if !supportsFlag("--command") {
+		logf("%s doesn't support --command, don't use", su)
+		return "", false
+	}
+
+	return su, supportsFlag("--pty")
+}
+
 // dropPrivilegesAndRun is a last resort if we couldn't use login or su. It
 // drops privileges for the current process, registers a new session with the
 // OS, sets its UID, GID and groups to the specified values, and then launches

ssh/tailssh/tailssh_integration_test.go

@@ -1,9 +1,6 @@
 // Copyright (c) Tailscale Inc & AUTHORS
 // SPDX-License-Identifier: BSD-3-Clause
 
-//go:build integrationtest
-// +build integrationtest
-
 package tailssh
 
 import (
@@ -23,6 +20,7 @@ import (
 	"net/netip"
 	"os"
 	"os/exec"
+	"path"
 	"runtime"
 	"strings"
 	"testing"
@@ -81,25 +79,20 @@ func TestMain(m *testing.M) {
 	m.Run()
 }
 
+var homeDir = "/home/testuser"
+
+func init() {
+	if runtime.GOOS == "darwin" {
+		homeDir = "/Users/testuser"
+	}
+}
+
 func TestIntegrationSSH(t *testing.T) {
 	debugTest.Store(true)
 	t.Cleanup(func() {
 		debugTest.Store(false)
 	})
 
-	homeDir := "/home/testuser"
-	if runtime.GOOS == "darwin" {
-		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,15 +105,11 @@ func TestIntegrationSSH(t *testing.T) {
 		{
 			cmd:  "pwd",
 			want: []string{homeDir},
-			skip: runtime.GOOS != "linux" || !suPresent || !loginPresent,
+			skip: !expectPAMToCreateHomeDir(),
 		},
 	}
 
 	for _, test := range tests {
-		if test.skip {
-			continue
-		}
-
 		// run every test both without and with a shell
 		for _, shell := range []bool{false, true} {
 			shellQualifier := "no_shell"
@@ -129,6 +118,10 @@ func TestIntegrationSSH(t *testing.T) {
 			}
 
 			t.Run(fmt.Sprintf("%s_%s", test.cmd, shellQualifier), func(t *testing.T) {
+				if test.skip {
+					t.SkipNow()
+				}
+
 				s := testSession(t)
 
 				if shell {
@@ -159,7 +152,12 @@ func TestIntegrationSFTP(t *testing.T) {
 		debugTest.Store(false)
 	})
 
-	filePath := "/tmp/sftptest.dat"
+	baseDir := homeDir
+	if !expectPAMToCreateHomeDir() {
+		baseDir = "/tmp"
+	}
+
+	filePath := path.Join(baseDir, "sftptest.dat")
 	wantText := "hello world"
 
 	cl := testClient(t)
@@ -168,6 +166,11 @@ func TestIntegrationSFTP(t *testing.T) {
 		t.Fatalf("can't get sftp client: %s", err)
 	}
 
+	_, err = scl.Stat(baseDir)
+	if err != nil {
+		t.Fatalf("can't stat %s: %s", homeDir, err)
+	}
+
 	file, err := scl.Create(filePath)
 	if err != nil {
 		t.Fatalf("can't create file: %s", err)
@@ -203,6 +206,21 @@ func TestIntegrationSFTP(t *testing.T) {
 	}
 }
 
+// expectPAMToCreateHomeDir tells us whether the test can expect pam_mkhomedir
+// to actually make the home directory. This returns true if and only if both
+// su and login commands are available on the path.
+func expectPAMToCreateHomeDir() bool {
+	_, 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
+
+	return suPresent && loginPresent
+}
+
 type session struct {
 	*ssh.Session
 

ssh/tailssh/testcontainers/Dockerfile

@@ -15,8 +15,14 @@ RUN authconfig --enablemkhomedir --update || echo "might not be fedora"
 
 COPY . .
 
+# RUN echo "Test doDropPrivileges once"
+# RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestDoDropPrivileges
+
 RUN echo "First run tests normally."
-RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegration TestDoDropPrivileges
+# RUN rm -Rf /home/testuser
+# RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSSH
+RUN rm -Rf /home/testuser
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSFTP
 
 RUN echo "Then run tests as non-root user testuser and make sure tests still pass."
 RUN chown testuser:groupone /tmp/tailscalessh.log
@@ -26,11 +32,14 @@ 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.run TestIntegration TestDoDropPrivileges
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSSH
+RUN rm -Rf /home/testuser
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSFTP
 
 RUN echo "Then remove the su command and make sure tests still pass."
-RUN ls -l /tmp/sftptest.dat
 RUN chown root:root /tmp/tailscalessh.log
 RUN rm `which su`
 RUN rm -Rf /home/testuser
-RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegration TestDoDropPrivileges
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSSH
+RUN rm -Rf /home/testuser
+RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegrationSFTP