diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index ddcdd0f09..4f1810685 100644 --- a/ssh/tailssh/incubator.go +++ b/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 { - incubatorArgs = append(incubatorArgs, "--sftp") - } else { - if isShell { - incubatorArgs = append(incubatorArgs, "--shell") + // 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") + return incubatorArgs } - incubatorArgs = append(incubatorArgs, "--cmd="+name) - if len(args) > 0 { - incubatorArgs = append(incubatorArgs, "--") - incubatorArgs = append(incubatorArgs, args...) - } + // We can use su, tell incubator to spawn itself + cmdName = ss.conn.srv.tailscaledPath + cmdArgs = ss.buildIncubatorArgs(lu, gids, remoteUser, remoteIP, false, false, "", nil) } - return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...) + if isShell { + incubatorArgs = append(incubatorArgs, "--shell") + } + + incubatorArgs = append(incubatorArgs, "--cmd="+cmdName) + if len(cmdArgs) > 0 { + incubatorArgs = append(incubatorArgs, "--") + incubatorArgs = append(incubatorArgs, cmdArgs...) + } + + 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 - // potentially could work on BSDs as well, but requires testing. - 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) + su, supportsPTY := findSUCommand(logf) + if 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 diff --git a/ssh/tailssh/tailssh_integration_test.go b/ssh/tailssh/tailssh_integration_test.go index bb78ba528..314986a8a 100644 --- a/ssh/tailssh/tailssh_integration_test.go +++ b/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 @@ "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 diff --git a/ssh/tailssh/testcontainers/Dockerfile b/ssh/tailssh/testcontainers/Dockerfile index dd5a740b9..044ab4655 100644 --- a/ssh/tailssh/testcontainers/Dockerfile +++ b/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