From e5a7b55ea249cbc2811410758c3c3b8b9a7bf5df Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Wed, 1 May 2024 19:31:15 -0500 Subject: [PATCH] Ineffective attempt making sftp work with pam Signed-off-by: Percy Wegmann --- ssh/tailssh/incubator.go | 120 ++++++++++++++++-------- ssh/tailssh/tailssh_integration_test.go | 62 +++++++----- ssh/tailssh/testcontainers/Dockerfile | 17 +++- 3 files changed, 133 insertions(+), 66 deletions(-) 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,23 +386,62 @@ 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" { + su, supportsPTY := findSUCommand(logf) + if su == "" { return false, nil } + loginArgs := []string{ + "--login", + } + 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") + } + loginArgs = append(loginArgs, ia.localUser) + + if !ia.isShell && ia.cmdName != "" { + // 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. + loginArgs = append(loginArgs, "--command", ia.cmdName) + loginArgs = append(loginArgs, ia.cmdArgs...) + } + + logf("logging in with %s %+v", su, loginArgs) + // replace the running process + 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, nil + 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, nil + return "", false } supportsFlag := func(flag string) bool { @@ -390,35 +451,14 @@ func tryLoginWithSU(logf logger.Logf, ia incubatorArgs) (bool, error) { // Make sure su supports the necessary flags. if !supportsFlag("--login") { logf("%s doesn't support --login, don't use", su) - return false, nil + return "", false } if !supportsFlag("--command") { logf("%s doesn't support --command, don't use", su) - return false, nil - } - - loginArgs := []string{ - "--login", - } - if ia.hasTTY && supportsFlag("--pty") { - // Allocate a pseudo terminal for improved security. In particular, - // this can help avoid TIOCSTI ioctl terminal injection. - loginArgs = append(loginArgs, "--pty") - } - loginArgs = append(loginArgs, ia.localUser) - - if !ia.isShell && ia.cmdName != "" { - // 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. - loginArgs = append(loginArgs, "--command", ia.cmdName) - loginArgs = append(loginArgs, ia.cmdArgs...) + return "", false } - logf("logging in with %s %+v", su, loginArgs) - // replace the running process - return true, unix.Exec(su, loginArgs, os.Environ()) + return su, supportsFlag("--pty") } // dropPrivilegesAndRun is a last resort if we couldn't use login or su. It 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 @@ import ( "net/netip" "os" "os/exec" + "path" "runtime" "strings" "testing" @@ -81,24 +79,19 @@ func TestMain(m *testing.M) { m.Run() } -func TestIntegrationSSH(t *testing.T) { - debugTest.Store(true) - t.Cleanup(func() { - debugTest.Store(false) - }) +var homeDir = "/home/testuser" - homeDir := "/home/testuser" +func init() { 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 +func TestIntegrationSSH(t *testing.T) { + debugTest.Store(true) + t.Cleanup(func() { + debugTest.Store(false) + }) tests := []struct { cmd 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