diff --git a/Makefile b/Makefile index 20ee97e9b..4ae343c3b 100644 --- a/Makefile +++ b/Makefile @@ -115,10 +115,7 @@ sshintegrationtest: ## Run the SSH integration tests in various Docker container echo "Testing on ubuntu:focal" && docker build --build-arg="BASE=ubuntu:focal" -t ssh-ubuntu-focal ssh/tailssh/testcontainers && \ echo "Testing on ubuntu:jammy" && docker build --build-arg="BASE=ubuntu:jammy" -t ssh-ubuntu-jammy ssh/tailssh/testcontainers && \ echo "Testing on ubuntu:mantic" && docker build --build-arg="BASE=ubuntu:mantic" -t ssh-ubuntu-mantic ssh/tailssh/testcontainers && \ - echo "Testing on ubuntu:noble" && docker build --build-arg="BASE=ubuntu:noble" -t ssh-ubuntu-noble ssh/tailssh/testcontainers && \ - echo "Testing on fedora:38" && docker build --build-arg="BASE=dokken/fedora-38" -t ssh-fedora-38 ssh/tailssh/testcontainers && \ - echo "Testing on fedora:39" && docker build --build-arg="BASE=dokken/fedora-39" -t ssh-fedora-39 ssh/tailssh/testcontainers && \ - echo "Testing on fedora:40" && docker build --build-arg="BASE=dokken/fedora-40" -t ssh-fedora-40 ssh/tailssh/testcontainers + echo "Testing on ubuntu:noble" && docker build --build-arg="BASE=ubuntu:noble" -t ssh-ubuntu-noble ssh/tailssh/testcontainers help: ## Show this help @echo "\nSpecify a command. The choices are:\n" diff --git a/go.mod b/go.mod index 4ba70dee6..922667e5e 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.64 github.com/aws/aws-sdk-go-v2/service/s3 v1.33.0 github.com/aws/aws-sdk-go-v2/service/ssm v1.44.7 + github.com/bramvdbogaerde/go-scp v1.4.0 github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6 github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf github.com/creack/pty v1.1.21 diff --git a/go.sum b/go.sum index 3e7085a41..59d1a8034 100644 --- a/go.sum +++ b/go.sum @@ -177,6 +177,8 @@ github.com/blizzy78/varnamelen v0.8.0 h1:oqSblyuQvFsW1hbBHh1zfwrKe3kcSj0rnXkKzsQ github.com/blizzy78/varnamelen v0.8.0/go.mod h1:V9TzQZ4fLJ1DSrjVDfl89H7aMnTvKkApdHeyESmyR7k= github.com/bombsimon/wsl/v3 v3.4.0 h1:RkSxjT3tmlptwfgEgTgU+KYKLI35p/tviNXNXiL2aNU= github.com/bombsimon/wsl/v3 v3.4.0/go.mod h1:KkIB+TXkqy6MvK9BDZVbZxKNYsE1/oLRJbIFtf14qqo= +github.com/bramvdbogaerde/go-scp v1.4.0 h1:jKMwpwCbcX1KyvDbm/PDJuXcMuNVlLGi0Q0reuzjyKY= +github.com/bramvdbogaerde/go-scp v1.4.0/go.mod h1:on2aH5AxaFb2G0N5Vsdy6B0Ml7k9HuHSwfo1y0QzAbQ= github.com/breml/bidichk v0.2.4 h1:i3yedFWWQ7YzjdZJHnPo9d/xURinSq3OM+gyM43K4/8= github.com/breml/bidichk v0.2.4/go.mod h1:7Zk0kRFt1LIZxtQdl9W9JwGAcLTTkOs+tN7wuEYGJ3s= github.com/breml/errchkjson v0.3.1 h1:hlIeXuspTyt8Y/UmP5qy1JocGNR00KQHgfaNtRAjoxQ= diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index 547995e56..e43486072 100644 --- a/ssh/tailssh/incubator.go +++ b/ssh/tailssh/incubator.go @@ -36,6 +36,7 @@ import ( "golang.org/x/sys/unix" "tailscale.com/cmd/tailscaled/childproc" "tailscale.com/hostinfo" + "tailscale.com/tailcfg" "tailscale.com/tempfork/gliderlabs/ssh" "tailscale.com/types/logger" "tailscale.com/version/distro" @@ -43,18 +44,22 @@ import ( func init() { childproc.Add("ssh", beIncubator) + childproc.Add("sftp", beSFTP) } 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 +var maybeStartLoginSession = func(dlogf logger.Logf, ia incubatorArgs) (close func() error) { + return nil } // newIncubatorCommand returns a new exec.Cmd configured with @@ -64,40 +69,39 @@ var maybeStartLoginSession = func(logf logger.Logf, ia incubatorArgs) (close fun // exec.CommandContext. // // The returned Cmd.Env is guaranteed to be nil; the caller populates it. -func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) { +func (ss *sshSession) newIncubatorCommand(logf logger.Logf) (cmd *exec.Cmd, err error) { defer func() { if cmd.Env != nil { panic("internal error") } }() - var ( - name string - args []string - isSFTP bool - isShell bool - ) + + var isSFTP, isShell bool switch ss.Subsystem() { case "sftp": isSFTP = true case "": - name = ss.conn.localUser.LoginShell() - if rawCmd := ss.RawCommand(); rawCmd != "" { - args = append(args, "-c", rawCmd) - } else { - isShell = true - args = append(args, "-l") // login shell - } + isShell = ss.RawCommand() == "" default: panic(fmt.Sprintf("unexpected subsystem: %v", ss.Subsystem())) } if ss.conn.srv.tailscaledPath == "" { - // TODO(maisem): this doesn't work with sftp - return exec.CommandContext(ss.ctx, name, args...) + if isSFTP { + // SFTP relies on the embedded Go-based SFTP server in tailscaled, + // so without tailscaled, we can't serve SFTP. + return nil, errors.New("no tailscaled found on path, can't serve SFTP") + } + + loginShell := ss.conn.localUser.LoginShell() + args := shellArgs(isShell, ss.RawCommand()) + logf("directly running %s %q", loginShell, args) + return exec.CommandContext(ss.ctx, loginShell, args...), nil } + lu := ss.conn.localUser ci := ss.conn.info - gids := strings.Join(ss.conn.userGroupIDs, ",") + groups := strings.Join(ss.conn.userGroupIDs, ",") remoteUser := ci.uprof.LoginName if ci.node.IsTagged() { remoteUser = strings.Join(ci.node.Tags().AsSlice(), ",") @@ -106,9 +110,10 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) { incubatorArgs := []string{ "be-child", "ssh", + "--login-shell=" + lu.LoginShell(), "--uid=" + lu.Uid, "--gid=" + lu.Gid, - "--groups=" + gids, + "--groups=" + groups, "--local-user=" + lu.Username, "--remote-user=" + remoteUser, "--remote-ip=" + ci.src.Addr().String(), @@ -116,39 +121,31 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) { "--tty-name=", // updated in-place by startWithPTY } + forceV1Behavior := ss.conn.srv.lb.NetMap().HasCap(tailcfg.NodeAttrSSHBehaviorV1) + if forceV1Behavior { + incubatorArgs = append(incubatorArgs, "--force-v1-behavior") + } + if debugTest.Load() { incubatorArgs = append(incubatorArgs, "--debug-test") } - if isSFTP { - incubatorArgs = append(incubatorArgs, "--sftp") - } else { - if isShell { - incubatorArgs = append(incubatorArgs, "--shell") - } - // Only the macOS version of the login command supports executing a - // command, all other versions only support launching a shell - // without taking any arguments. - shouldUseLoginCmd := isShell || runtime.GOOS == "darwin" - if hostinfo.IsSELinuxEnforcing() { - // If we're running on a SELinux-enabled system, the login - // command will be unable to set the correct context for the - // shell. Fall back to using the incubator to launch the shell. - // See http://github.com/tailscale/tailscale/issues/4908. - shouldUseLoginCmd = false - } - if shouldUseLoginCmd { - if lp, err := exec.LookPath("login"); err == nil { - incubatorArgs = append(incubatorArgs, "--login-cmd="+lp) - } - } - incubatorArgs = append(incubatorArgs, "--cmd="+name) - if len(args) > 0 { - incubatorArgs = append(incubatorArgs, "--") - incubatorArgs = append(incubatorArgs, args...) - } + switch { + case isSFTP: + // Note that we include both the `--sftp` flag and a command to launch + // tailscaled as `be-child sftp`. If login or su is available, and + // we're not running with tailcfg.NodeAttrSSHBehaviorV1, this will + // result in serving SFTP within a login shell, with full PAM + // integration. Otherwise, we'll serve SFTP in the incubator process + // with no PAM integration. + incubatorArgs = append(incubatorArgs, "--sftp", fmt.Sprintf("--cmd=%s be-child sftp", ss.conn.srv.tailscaledPath)) + case isShell: + incubatorArgs = append(incubatorArgs, "--shell") + default: + incubatorArgs = append(incubatorArgs, "--cmd="+ss.RawCommand()) } - return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...) + + return exec.CommandContext(ss.ctx, ss.conn.srv.tailscaledPath, incubatorArgs...), nil } var debugIncubator bool @@ -170,51 +167,60 @@ func (stdRWC) Close() error { } type incubatorArgs struct { - uid int - gid int - groups string - localUser string - remoteUser string - remoteIP string - ttyName string - hasTTY bool - cmdName string - isSFTP bool - isShell bool - loginCmdPath string - cmdArgs []string - debugTest bool -} - -func parseIncubatorArgs(args []string) (a incubatorArgs) { + loginShell string + uid int + gid int + gids []int + localUser string + remoteUser string + remoteIP string + ttyName string + hasTTY bool + cmd string + isSFTP bool + isShell bool + forceV1Behavior bool + debugTest bool +} + +func parseIncubatorArgs(args []string) (incubatorArgs, error) { + var ia incubatorArgs + var groups string + flags := flag.NewFlagSet("", flag.ExitOnError) - flags.IntVar(&a.uid, "uid", 0, "the uid of local-user") - flags.IntVar(&a.gid, "gid", 0, "the gid of local-user") - flags.StringVar(&a.groups, "groups", "", "comma-separated list of gids of local-user") - flags.StringVar(&a.localUser, "local-user", "", "the user to run as") - flags.StringVar(&a.remoteUser, "remote-user", "", "the remote user/tags") - flags.StringVar(&a.remoteIP, "remote-ip", "", "the remote Tailscale IP") - flags.StringVar(&a.ttyName, "tty-name", "", "the tty name (pts/3)") - flags.BoolVar(&a.hasTTY, "has-tty", false, "is the output attached to a tty") - flags.StringVar(&a.cmdName, "cmd", "", "the cmd to launch (ignored in sftp mode)") - flags.BoolVar(&a.isShell, "shell", false, "is launching a shell (with no cmds)") - flags.BoolVar(&a.isSFTP, "sftp", false, "run sftp server (cmd is ignored)") - flags.StringVar(&a.loginCmdPath, "login-cmd", "", "the path to `login` cmd") - flags.BoolVar(&a.debugTest, "debug-test", false, "should debug in test mode") + flags.StringVar(&ia.loginShell, "login-shell", "", "path to the user's preferred login shell") + flags.IntVar(&ia.uid, "uid", 0, "the uid of local-user") + flags.IntVar(&ia.gid, "gid", 0, "the gid of local-user") + flags.StringVar(&groups, "groups", "", "comma-separated list of gids of local-user") + flags.StringVar(&ia.localUser, "local-user", "", "the user to run as") + flags.StringVar(&ia.remoteUser, "remote-user", "", "the remote user/tags") + flags.StringVar(&ia.remoteIP, "remote-ip", "", "the remote Tailscale IP") + flags.StringVar(&ia.ttyName, "tty-name", "", "the tty name (pts/3)") + flags.BoolVar(&ia.hasTTY, "has-tty", false, "is the output attached to a tty") + flags.StringVar(&ia.cmd, "cmd", "", "the cmd to launch, including all arguments (ignored in sftp mode)") + flags.BoolVar(&ia.isShell, "shell", false, "is launching a shell (with no cmds)") + flags.BoolVar(&ia.isSFTP, "sftp", false, "run sftp server (cmd is ignored)") + flags.BoolVar(&ia.forceV1Behavior, "force-v1-behavior", false, "allow falling back to the su command if login is unavailable") + flags.BoolVar(&ia.debugTest, "debug-test", false, "should debug in test mode") flags.Parse(args) - a.cmdArgs = flags.Args() - return a + + for _, g := range strings.Split(groups, ",") { + gid, err := strconv.Atoi(g) + if err != nil { + return ia, fmt.Errorf("unable to parse group id %q: %w", g, err) + } + ia.gids = append(ia.gids, gid) + } + + return ia, nil } // beIncubator is the entrypoint to the `tailscaled be-child ssh` subcommand. -// It is responsible for informing the system of a new login session for the user. -// This is sometimes necessary for mounting home directories and decrypting file -// systems. +// It is responsible for informing the system of a new login session for the +// user. This is sometimes necessary for mounting home directories and +// decrypting file systems. // -// Tailscaled launches the incubator as the same user as it was -// launched as. The incubator then registers a new session with the -// OS, sets its UID and groups to the specified `--uid`, `--gid` and -// `--groups` and then launches the requested `--cmd`. +// Tailscaled launches the incubator as the same user as it was launched as. func beIncubator(args []string) error { // To defend against issues like https://golang.org/issue/1435, // defensively lock our current goroutine's thread to the current @@ -226,22 +232,25 @@ func beIncubator(args []string) error { runtime.LockOSThread() defer runtime.UnlockOSThread() - ia := parseIncubatorArgs(args) + ia, err := parseIncubatorArgs(args) + if err != nil { + return err + } if ia.isSFTP && ia.isShell { return fmt.Errorf("--sftp and --shell are mutually exclusive") } - logf := logger.Discard + dlogf := logger.Discard if debugIncubator { // We don't own stdout or stderr, so the only place we can log is syslog. if sl, err := syslog.New(syslog.LOG_INFO|syslog.LOG_DAEMON, "tailscaled-ssh"); err == nil { - logf = log.New(sl, "", 0).Printf + dlogf = 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) { + dlogf = func(msg string, args ...any) { lf.Printf(msg, args...) logFile.Sync() } @@ -249,72 +258,233 @@ func beIncubator(args []string) error { } } - euid := os.Geteuid() - runningAsRoot := euid == 0 - if runningAsRoot && ia.loginCmdPath != "" { - // Check if we can exec into the login command instead of trying to - // incubate ourselves. - if la := ia.loginArgs(); la != nil { - return unix.Exec(ia.loginCmdPath, la, os.Environ()) - } + if !shouldAttemptLoginShell(dlogf, ia) { + dlogf("not attempting login shell") + return handleInProcess(dlogf, ia) + } + + // First try the login command + if err := tryExecLogin(dlogf, 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(dlogf, ia); handled { + return err + } else { + dlogf("not attempting su") + return handleInProcess(dlogf, ia) } +} + +func handleInProcess(dlogf logger.Logf, ia incubatorArgs) error { + if ia.isSFTP { + return handleSFTPInProcess(dlogf, ia) + } + return handleSSHInProcess(dlogf, 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 { +func handleSFTPInProcess(dlogf logger.Logf, ia incubatorArgs) error { + dlogf("handling sftp") + + sessionCloser := maybeStartLoginSession(dlogf, ia) + if sessionCloser != nil { defer sessionCloser() } - var groupIDs []int - for _, g := range strings.Split(ia.groups, ",") { - gid, err := strconv.ParseInt(g, 10, 32) - if err != nil { - return err - } - groupIDs = append(groupIDs, int(gid)) + if err := dropPrivileges(dlogf, ia); err != nil { + return err } - if err := dropPrivileges(logf, ia.uid, ia.gid, groupIDs); err != nil { + return serveSFTP() +} + +// beSFTP serves SFTP in-process. +func beSFTP(args []string) error { + return serveSFTP() +} + +func serveSFTP() error { + server, err := sftp.NewServer(stdRWC{}) + if err != nil { return err } + // TODO(https://github.com/pkg/sftp/pull/554): Revert the check for io.EOF, + // when sftp is patched to report clean termination. + if err := server.Serve(); err != nil && err != io.EOF { + return err + } + return nil +} - if ia.isSFTP { - logf("handling sftp") +// shouldAttemptLoginShell decides whether we should attempt to get a full +// login shell with the login or su commands. We will attempt a login shell +// if all of the following conditions are met. +// +// - We are running as root +// - This is not an SELinuxEnforcing host +// +// The last condition exists because if we're running on a SELinux-enabled +// system, neiher login nor su will be able to set the correct context for the +// shell. So, we don't bother trying to run them and instead fall back to using +// the incubator to launch the shell. +// See http://github.com/tailscale/tailscale/issues/4908. +func shouldAttemptLoginShell(dlogf logger.Logf, ia incubatorArgs) bool { + if ia.forceV1Behavior && ia.isSFTP { + // v1 behavior did not run SFTP within a login shell. + dlogf("Forcing v1 behavior, won't use login shell for SFTP") + return false + } - server, err := sftp.NewServer(stdRWC{}) - if err != nil { - return err - } - // TODO(https://github.com/pkg/sftp/pull/554): Revert the check for io.EOF, - // when sftp is patched to report clean termination. - if err := server.Serve(); err != nil && err != io.EOF { - return err + return runningAsRoot() && !hostinfo.IsSELinuxEnforcing() +} + +func runningAsRoot() bool { + euid := os.Geteuid() + return euid == 0 +} + +// tryExecLogin attempts to handle the ssh session by creating a full login +// shell using the login command. If it never tried, it returns nil. If it +// failed to do so, it returns an error. +// +// Creating a login shell in this way allows us to register the remote IP of +// the login session, trigger PAM authentication, and get the "remote" PAM +// profile. +// +// However, login is subject to some limitations. +// +// 1. login cannot be used to execute commands except on macOS. +// 2. On Linux and BSD, login requires a TTY to keep running. +// +// In these cases, tryExecLogin returns (false, nil) to indicate that processing +// should fall through to other methods, such as using the su command. +// +// Note that this uses unix.Exec to replace the current process, so in cases +// where we actually do run login, no subsequent Go code will execute. +func tryExecLogin(dlogf logger.Logf, ia incubatorArgs) error { + // Only the macOS version of the login command supports executing a + // command, all other versions only support launching a shell without + // taking any arguments. + if !ia.isShell && runtime.GOOS != "darwin" { + dlogf("won't use login because we're not in a shell or on macOS") + return nil + } + + switch runtime.GOOS { + case "linux", "freebsd", "openbsd": + if !ia.hasTTY { + dlogf("can't use login because of missing TTY") + // We can only use the login command if a shell was requested with + // a TTY. If there is no TTY, login exits immediately, which + // breaks things like mosh and VSCode. + return nil } + } + + loginCmdPath, err := exec.LookPath("login") + if err != nil { + dlogf("failed to get login args: %s", err) return nil } + loginArgs := ia.loginArgs(loginCmdPath) + dlogf("logging in with %s %+v", loginCmdPath, loginArgs) + // replace the running process + return unix.Exec(loginCmdPath, loginArgs, os.Environ()) +} - cmd := exec.Command(ia.cmdName, ia.cmdArgs...) - cmd.Stdin = os.Stdin - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - cmd.Env = os.Environ() +// trySU attempts to start a login shell using su. If su is available and +// supports the necessary arguments, this returns true, plus the result of +// executing su. Otherwise, it returns (false, nil). +// +// Creating a login shell in this way allows us to trigger PAM authentication +// and get the "login" PAM profile. +// +// Unlike login, su often does not require a TTY, so on Linux hosts that have +// an su command which accepts the right flags, we'll use su instead of login +// when no TTY is available. +func trySU(dlogf logger.Logf, ia incubatorArgs) (handled bool, err error) { + if ia.forceV1Behavior { + // v1 behavior did not use su. + dlogf("Forcing v1 behavior, won't use su") + return false, nil + } - if ia.hasTTY { - // If we were launched with a tty then we should - // mark that as the ctty of the child. However, - // as the ctty is being passed from the parent - // we set the child to foreground instead which - // also passes the ctty. - // However, we can not do this if never had a tty to - // begin with. - cmd.SysProcAttr = &syscall.SysProcAttr{ - Foreground: true, - } + su := findSU(dlogf, ia) + if su == "" { + return false, nil } - err = cmd.Run() + + sessionCloser := maybeStartLoginSession(dlogf, ia) + if sessionCloser != nil { + defer sessionCloser() + } + + loginArgs := []string{"-l", ia.localUser} + if ia.cmd != "" { + // Note - unlike the login command, su allows using both -l and -c. + loginArgs = append(loginArgs, "-c", ia.cmd) + } + + dlogf("logging in with %s %q", su, loginArgs) + cmd := newCommand(ia.hasTTY, su, loginArgs) + 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. If a suitable su is not available, this returns +// "". +func findSU(dlogf logger.Logf, ia incubatorArgs) string { + // 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 "" + } + + // gokrazy doesn't include su. And, if someone installs a breakglass/ + // debugging package on gokrazy, we don't want to use its su. + if distro.Get() == distro.Gokrazy { + return "" + } + + su, err := exec.LookPath("su") + if err != nil { + dlogf("can't find su command: %v", err) + return "" + } + + // First try to execute su -l -c true to make sure su supports the + // necessary arguments. + err = exec.Command(su, "-l", ia.localUser, "-c", "true").Run() + if err != nil { + dlogf("su check failed: %s", err) + return "" + } + + return su +} + +// 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(dlogf logger.Logf, ia incubatorArgs) error { + sessionCloser := maybeStartLoginSession(dlogf, ia) + if sessionCloser != nil { + defer sessionCloser() + } + + if err := dropPrivileges(dlogf, ia); err != nil { + return err + } + + args := shellArgs(ia.isShell, ia.cmd) + dlogf("running %s %q", ia.loginShell, args) + cmd := newCommand(ia.hasTTY, ia.loginShell, args) + err := cmd.Run() if ee, ok := err.(*exec.ExitError); ok { ps := ee.ProcessState code := ps.ExitCode() @@ -330,6 +500,26 @@ func beIncubator(args []string) error { return err } +func newCommand(hasTTY bool, cmdPath string, cmdArgs []string) *exec.Cmd { + cmd := exec.Command(cmdPath, cmdArgs...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Env = os.Environ() + + if hasTTY { + // If we were launched with a tty then we should mark that as the ctty + // of the child. However, as the ctty is being passed from the parent + // we set the child to foreground instead which also passes the ctty. + // However, we can not do this if never had a tty to begin with. + cmd.SysProcAttr = &syscall.SysProcAttr{ + Foreground: true, + } + } + + return cmd +} + const ( // This controls whether we assert that our privileges were dropped // using geteuid/getegid; it's a const and not an envknob because the @@ -344,19 +534,26 @@ const ( assertPrivilegesWereDroppedByAttemptingToUnDrop = false ) -// dropPrivileges contains all the logic for dropping privileges to a different +// dropPrivileges calls doDropPrivileges with uid, gid, and gids from the given +// incubatorArgs. +func dropPrivileges(dlogf logger.Logf, ia incubatorArgs) error { + return doDropPrivileges(dlogf, ia.uid, ia.gid, ia.gids) +} + +// doDropPrivileges contains all the logic for dropping privileges to a different // UID, GID, and set of supplementary groups. This function is // security-sensitive and ordering-dependent; please be very cautious if/when // refactoring. // -// WARNING: if you change this function, you *MUST* run the TestDropPrivileges +// WARNING: if you change this function, you *MUST* run the TestDoDropPrivileges // test in this package as root on at least Linux, FreeBSD and Darwin. This can // be done by running: // -// go test -c ./ssh/tailssh/ && sudo ./tailssh.test -test.v -test.run TestDropPrivileges -func dropPrivileges(logf logger.Logf, wantUid, wantGid int, supplementaryGroups []int) error { +// go test -c ./ssh/tailssh/ && sudo ./tailssh.test -test.v -test.run TestDoDropPrivileges +func doDropPrivileges(dlogf logger.Logf, wantUid, wantGid int, supplementaryGroups []int) error { + dlogf("dropping privileges") fatalf := func(format string, args ...any) { - logf("[unexpected] error dropping privileges: "+format, args...) + dlogf("[unexpected] error dropping privileges: "+format, args...) os.Exit(1) } @@ -448,7 +645,11 @@ func dropPrivileges(logf logger.Logf, wantUid, wantGid int, supplementaryGroups // // It sets ss.cmd, stdin, stdout, and stderr. func (ss *sshSession) launchProcess() error { - ss.cmd = ss.newIncubatorCommand() + var err error + ss.cmd, err = ss.newIncubatorCommand(ss.logf) + if err != nil { + return err + } cmd := ss.cmd homeDir := ss.conn.localUser.HomeDir @@ -749,18 +950,11 @@ func fileExists(path string) bool { } // loginArgs returns the arguments to use to exec the login binary. -// It returns nil if the login binary should not be used. -// The login binary is only used: -// - on darwin, if the client is requesting a shell or a command. -// - on linux and BSD, if the client is requesting a shell with a TTY. -func (ia *incubatorArgs) loginArgs() []string { - if ia.isSFTP { - return nil - } +func (ia *incubatorArgs) loginArgs(loginCmdPath string) []string { switch runtime.GOOS { case "darwin": args := []string{ - ia.loginCmdPath, + loginCmdPath, "-f", // already authenticated // login typically discards the previous environment, but we want to @@ -773,39 +967,35 @@ func (ia *incubatorArgs) loginArgs() []string { if !ia.hasTTY { args[2] = "-pq" // -q is "quiet" which suppresses the login banner } - if ia.cmdName != "" { - args = append(args, ia.cmdName) - args = append(args, ia.cmdArgs...) + if ia.cmd != "" { + args = append(args, ia.loginShell, "-c", ia.cmd) } + return args case "linux": - if !ia.isShell || !ia.hasTTY { - // We can only use login command if a shell was requested with a TTY. If - // there is no TTY, login exits immediately, which breaks things likes - // mosh and VSCode. - return nil - } if distro.Get() == distro.Arch && !fileExists("/etc/pam.d/remote") { // See https://github.com/tailscale/tailscale/issues/4924 // // Arch uses a different login binary that makes the -h flag set the PAM // service to "remote". So if they don't have that configured, don't // pass -h. - return []string{ia.loginCmdPath, "-f", ia.localUser, "-p"} + return []string{loginCmdPath, "-f", ia.localUser, "-p"} } - return []string{ia.loginCmdPath, "-f", ia.localUser, "-h", ia.remoteIP, "-p"} + return []string{loginCmdPath, "-f", ia.localUser, "-h", ia.remoteIP, "-p"} case "freebsd", "openbsd": - if !ia.isShell || !ia.hasTTY { - // We can only use login command if a shell was requested with a TTY. If - // there is no TTY, login exits immediately, which breaks things likes - // mosh and VSCode. - return nil - } - return []string{ia.loginCmdPath, "-fp", "-h", ia.remoteIP, ia.localUser} + return []string{loginCmdPath, "-fp", "-h", ia.remoteIP, ia.localUser} } panic("unimplemented") } +func shellArgs(isShell bool, cmd string) []string { + if isShell { + return []string{"-l"} + } else { + return []string{"-c", cmd} + } +} + func setGroups(groupIDs []int) error { if runtime.GOOS == "darwin" && len(groupIDs) > 16 { // darwin returns "invalid argument" if more than 16 groups are passed to syscall.Setgroups diff --git a/ssh/tailssh/incubator_linux.go b/ssh/tailssh/incubator_linux.go index f9abdb272..bcbe0e240 100644 --- a/ssh/tailssh/incubator_linux.go +++ b/ssh/tailssh/incubator_linux.go @@ -146,11 +146,11 @@ func releaseSession(sessionID string) error { } // maybeStartLoginSessionLinux is the linux implementation of maybeStartLoginSession. -func maybeStartLoginSessionLinux(logf logger.Logf, ia incubatorArgs) (func() error, error) { +func maybeStartLoginSessionLinux(dlogf logger.Logf, ia incubatorArgs) func() error { if os.Geteuid() != 0 { - return nil, nil + return nil } - logf("starting session for user %d", ia.uid) + dlogf("starting session for user %d", ia.uid) // The only way we can actually start a new session is if we are // running outside one and are root, which is typically the case // for systemd managed tailscaled. @@ -160,14 +160,14 @@ func maybeStartLoginSessionLinux(logf logger.Logf, ia incubatorArgs) (func() err // We can look at the DBus GetSessionByPID API. // https://www.freedesktop.org/software/systemd/man/org.freedesktop.login1.html // For now best effort is fine. - logf("ssh: failed to CreateSession for user %q (%d) %v", ia.localUser, ia.uid, err) - return nil, nil + dlogf("ssh: failed to CreateSession for user %q (%d) %v", ia.localUser, ia.uid, err) + return nil } os.Setenv("DBUS_SESSION_BUS_ADDRESS", fmt.Sprintf("unix:path=%v/bus", resp.runtimePath)) if !resp.existing { return func() error { return releaseSession(resp.sessionID) - }, nil + } } - return nil, nil + return nil } diff --git a/ssh/tailssh/privs_test.go b/ssh/tailssh/privs_test.go index 4ef597933..5ebf4e25c 100644 --- a/ssh/tailssh/privs_test.go +++ b/ssh/tailssh/privs_test.go @@ -23,7 +23,7 @@ import ( "tailscale.com/types/logger" ) -func TestDropPrivileges(t *testing.T) { +func TestDoDropPrivileges(t *testing.T) { type SubprocInput struct { UID int GID int @@ -49,7 +49,7 @@ func TestDropPrivileges(t *testing.T) { f := os.NewFile(3, "out.json") // We're in our subprocess; actually drop privileges now. - dropPrivileges(t.Logf, input.UID, input.GID, input.AdditionalGroups) + doDropPrivileges(t.Logf, input.UID, input.GID, input.AdditionalGroups) additional, _ := syscall.Getgroups() diff --git a/ssh/tailssh/tailssh_integration_test.go b/ssh/tailssh/tailssh_integration_test.go index 888158d68..1a2ee91cf 100644 --- a/ssh/tailssh/tailssh_integration_test.go +++ b/ssh/tailssh/tailssh_integration_test.go @@ -8,6 +8,7 @@ package tailssh import ( "bufio" + "context" "crypto/ecdsa" "crypto/ed25519" "crypto/elliptic" @@ -28,6 +29,7 @@ import ( "testing" "time" + "github.com/bramvdbogaerde/go-scp" "github.com/google/go-cmp/cmp" "github.com/pkg/sftp" gossh "github.com/tailscale/golang-x-crypto/ssh" @@ -36,6 +38,7 @@ import ( "tailscale.com/tailcfg" "tailscale.com/types/key" "tailscale.com/types/netmap" + "tailscale.com/util/set" ) // This file contains integration tests of the SSH functionality. These tests @@ -58,7 +61,7 @@ func TestMain(m *testing.M) { file.Close() // Tail our log file. - cmd := exec.Command("tail", "-f", "/tmp/tailscalessh.log") + cmd := exec.Command("tail", "-F", "/tmp/tailscalessh.log") r, err := cmd.StdoutPipe() if err != nil { @@ -77,6 +80,12 @@ func TestMain(m *testing.M) { if err != nil { return } + defer func() { + // tail -f has a default sleep interval of 1 second, so it takes a + // moment for it to finish reading our log file after we've terminated. + // So, wait a bit to let it catch up. + time.Sleep(2 * time.Second) + }() m.Run() } @@ -93,20 +102,40 @@ func TestIntegrationSSH(t *testing.T) { } tests := []struct { - cmd string - want []string + cmd string + want []string + forceV1Behavior bool + skip bool }{ { - cmd: "id", - want: []string{"testuser", "groupone", "grouptwo"}, + cmd: "id", + want: []string{"testuser", "groupone", "grouptwo"}, + forceV1Behavior: false, + }, + { + cmd: "id", + want: []string{"testuser", "groupone", "grouptwo"}, + forceV1Behavior: true, + }, + { + cmd: "pwd", + want: []string{homeDir}, + skip: !fallbackToSUAvailable(), + forceV1Behavior: false, }, { - cmd: "pwd", - want: []string{homeDir}, + cmd: "echo 'hello'", + want: []string{"hello"}, + skip: !fallbackToSUAvailable(), + forceV1Behavior: false, }, } 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" @@ -114,8 +143,13 @@ func TestIntegrationSSH(t *testing.T) { shellQualifier = "shell" } - t.Run(fmt.Sprintf("%s_%s", test.cmd, shellQualifier), func(t *testing.T) { - s := testSession(t) + versionQualifier := "v2" + if test.forceV1Behavior { + versionQualifier = "v1" + } + + t.Run(fmt.Sprintf("%s_%s_%s", test.cmd, shellQualifier, versionQualifier), func(t *testing.T) { + s := testSession(t, test.forceV1Behavior) if shell { err := s.RequestPty("xterm", 40, 80, ssh.TerminalModes{ @@ -123,12 +157,20 @@ func TestIntegrationSSH(t *testing.T) { ssh.TTY_OP_ISPEED: 14400, ssh.TTY_OP_OSPEED: 14400, }) + if err != nil { + t.Fatalf("unable to request PTY: %s", err) + } + + err = s.Shell() if err != nil { t.Fatalf("unable to request shell: %s", err) } + + // Read the shell prompt + s.read() } - got := s.run(t, test.cmd) + got := s.run(t, test.cmd, shell) for _, want := range test.want { if !strings.Contains(got, want) { t.Errorf("%q does not contain %q", got, want) @@ -145,48 +187,133 @@ func TestIntegrationSFTP(t *testing.T) { debugTest.Store(false) }) - filePath := "/tmp/sftptest.dat" - wantText := "hello world" + for _, forceV1Behavior := range []bool{false, true} { + name := "v2" + if forceV1Behavior { + name = "v1" + } + t.Run(name, func(t *testing.T) { + filePath := "/home/testuser/sftptest.dat" + if forceV1Behavior || !fallbackToSUAvailable() { + filePath = "/tmp/sftptest.dat" + } + wantText := "hello world" - cl := testClient(t) - scl, err := sftp.NewClient(cl) - if err != nil { - t.Fatalf("can't get sftp client: %s", err) - } + cl := testClient(t, forceV1Behavior) + scl, err := sftp.NewClient(cl) + if err != nil { + t.Fatalf("can't get sftp client: %s", err) + } - file, err := scl.Create(filePath) - if err != nil { - t.Fatalf("can't create file: %s", err) - } - _, err = file.Write([]byte(wantText)) - if err != nil { - t.Fatalf("can't write to file: %s", err) + file, err := scl.Create(filePath) + if err != nil { + t.Fatalf("can't create file: %s", err) + } + _, err = file.Write([]byte(wantText)) + if err != nil { + t.Fatalf("can't write to file: %s", err) + } + err = file.Close() + if err != nil { + t.Fatalf("can't close file: %s", err) + } + + file, err = scl.OpenFile(filePath, os.O_RDONLY) + if err != nil { + t.Fatalf("can't open file: %s", err) + } + defer file.Close() + gotText, err := io.ReadAll(file) + if err != nil { + t.Fatalf("can't read file: %s", err) + } + if diff := cmp.Diff(string(gotText), wantText); diff != "" { + t.Fatalf("unexpected file contents (-got +want):\n%s", diff) + } + + s := testSessionFor(t, cl) + got := s.run(t, "ls -l "+filePath, false) + if !strings.Contains(got, "testuser") { + t.Fatalf("unexpected file owner user: %s", got) + } else if !strings.Contains(got, "testuser") { + t.Fatalf("unexpected file owner group: %s", got) + } + }) } - err = file.Close() - if err != nil { - t.Fatalf("can't close file: %s", err) +} + +func TestIntegrationSCP(t *testing.T) { + debugTest.Store(true) + t.Cleanup(func() { + debugTest.Store(false) + }) + + for _, forceV1Behavior := range []bool{false, true} { + name := "v2" + if forceV1Behavior { + name = "v1" + } + t.Run(name, func(t *testing.T) { + filePath := "/home/testuser/scptest.dat" + if !fallbackToSUAvailable() { + filePath = "/tmp/scptest.dat" + } + wantText := "hello world" + + cl := testClient(t, forceV1Behavior) + scl, err := scp.NewClientBySSH(cl) + if err != nil { + t.Fatalf("can't get sftp client: %s", err) + } + + err = scl.Copy(context.Background(), strings.NewReader(wantText), filePath, "0644", int64(len(wantText))) + if err != nil { + t.Fatalf("can't create file: %s", err) + } + + outfile, err := os.CreateTemp("", "") + if err != nil { + t.Fatalf("can't create temp file: %s", err) + } + err = scl.CopyFromRemote(context.Background(), outfile, filePath) + if err != nil { + t.Fatalf("can't copy file from remote: %s", err) + } + outfile.Close() + + gotText, err := os.ReadFile(outfile.Name()) + if err != nil { + t.Fatalf("can't read file: %s", err) + } + if diff := cmp.Diff(string(gotText), wantText); diff != "" { + t.Fatalf("unexpected file contents (-got +want):\n%s", diff) + } + + s := testSessionFor(t, cl) + got := s.run(t, "ls -l "+filePath, false) + if !strings.Contains(got, "testuser") { + t.Fatalf("unexpected file owner user: %s", got) + } else if !strings.Contains(got, "testuser") { + t.Fatalf("unexpected file owner group: %s", got) + } + }) } +} - file, err = scl.OpenFile(filePath, os.O_RDONLY) - if err != nil { - t.Fatalf("can't open file: %s", err) +func fallbackToSUAvailable() bool { + if runtime.GOOS != "linux" { + return false } - defer file.Close() - gotText, err := io.ReadAll(file) + + _, err := exec.LookPath("su") if err != nil { - t.Fatalf("can't read file: %s", err) - } - if diff := cmp.Diff(string(gotText), wantText); diff != "" { - t.Fatalf("unexpected file contents (-got +want):\n%s", diff) + return false } - s := testSessionFor(t, cl) - got := s.run(t, "ls -l "+filePath) - if !strings.Contains(got, "testuser") { - t.Fatalf("unexpected file owner user: %s", got) - } else if !strings.Contains(got, "testuser") { - t.Fatalf("unexpected file owner group: %s", got) - } + // 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 { @@ -197,14 +324,25 @@ type session struct { stderr io.ReadCloser } -func (s *session) run(t *testing.T, cmdString string) string { +func (s *session) run(t *testing.T, cmdString string, shell bool) string { t.Helper() - err := s.Start(cmdString) - if err != nil { - t.Fatalf("unable to start command: %s", err) + if shell { + _, err := s.stdin.Write([]byte(fmt.Sprintf("%s\n", cmdString))) + if err != nil { + t.Fatalf("unable to send command to shell: %s", err) + } + } else { + err := s.Start(cmdString) + if err != nil { + t.Fatalf("unable to start command: %s", err) + } } + return s.read() +} + +func (s *session) read() string { ch := make(chan []byte) go func() { for { @@ -228,7 +366,7 @@ readLoop: select { case b := <-ch: _got = append(_got, b...) - case <-time.After(25 * time.Millisecond): + case <-time.After(1 * time.Second): break readLoop } } @@ -236,12 +374,12 @@ readLoop: return string(_got) } -func testClient(t *testing.T) *ssh.Client { +func testClient(t *testing.T, forceV1Behavior bool) *ssh.Client { t.Helper() username := "testuser" srv := &server{ - lb: &testBackend{localUser: username}, + lb: &testBackend{localUser: username, forceV1Behavior: forceV1Behavior}, logf: log.Printf, tailscaledPath: os.Getenv("TAILSCALED_PATH"), timeNow: time.Now, @@ -271,8 +409,8 @@ func testClient(t *testing.T) *ssh.Client { return cl } -func testSession(t *testing.T) *session { - cl := testClient(t) +func testSession(t *testing.T, forceV1Behavior bool) *session { + cl := testClient(t, forceV1Behavior) return testSessionFor(t, cl) } @@ -299,7 +437,8 @@ func testSessionFor(t *testing.T, cl *ssh.Client) *session { // testBackend implements ipnLocalBackend type testBackend struct { - localUser string + localUser string + forceV1Behavior bool } func (tb *testBackend) GetSSH_HostKeys() ([]gossh.Signer, error) { @@ -339,16 +478,21 @@ func (tb *testBackend) ShouldRunSSH() bool { } func (tb *testBackend) NetMap() *netmap.NetworkMap { + capMap := make(set.Set[tailcfg.NodeCapability]) + if tb.forceV1Behavior { + capMap[tailcfg.NodeAttrSSHBehaviorV1] = struct{}{} + } return &netmap.NetworkMap{ SSHPolicy: &tailcfg.SSHPolicy{ Rules: []*tailcfg.SSHRule{ - &tailcfg.SSHRule{ + { Principals: []*tailcfg.SSHPrincipal{{Any: true}}, Action: &tailcfg.SSHAction{Accept: true}, SSHUsers: map[string]string{"*": tb.localUser}, }, }, }, + AllCaps: capMap, } } diff --git a/ssh/tailssh/testcontainers/Dockerfile b/ssh/tailssh/testcontainers/Dockerfile index 7832a5853..cb24aca12 100644 --- a/ssh/tailssh/testcontainers/Dockerfile +++ b/ssh/tailssh/testcontainers/Dockerfile @@ -1,18 +1,51 @@ ARG BASE FROM ${BASE} +RUN echo "Install openssh, needed for scp." +RUN apt-get update -y && apt-get install -y openssh-client + RUN groupadd -g 10000 groupone RUN groupadd -g 10001 grouptwo -RUN useradd -g 10000 -G 10001 -u 10002 -m testuser -COPY . . +# Note - we do not create the user's home directory, pam_mkhomedir will do that +# for us, and we want to test that PAM gets triggered by Tailscale SSH. +RUN useradd -g 10000 -G 10001 -u 10002 testuser -# First run tests normally. -RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegration +RUN echo "Set up pam_mkhomedir." +RUN sed -i -e 's/Default: no/Default: yes/g' /usr/share/pam-configs/mkhomedir || echo "might not be ubuntu" +RUN cat /usr/share/pam-configs/mkhomedir +RUN pam-auth-update --enable mkhomedir -# Then remove the login command and make sure tests still pass. -RUN rm `which login` -RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.run TestIntegration +COPY tailscaled . +COPY tailssh.test . + +RUN chmod 755 tailscaled + +RUN echo "First run tests normally." +RUN rm -Rf /home/testuser +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP +RUN rm -Rf /home/testuser +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSCP +RUN rm -Rf /home/testuser +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSSH -# Then run tests as non-root user testuser. +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.run TestIntegration" +RUN TAILSCALED_PATH=`pwd`tailscaled su -m testuser -c "./tailssh.test -test.v -test.run TestIntegration 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 TestIntegrationSFTP +RUN rm -Rf /home/testuser +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSCP +RUN rm -Rf /home/testuser +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run 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 TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration + +RUN echo "Test doDropPrivileges" +RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestDoDropPrivileges diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 98ea855ec..7ed0e7d73 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -136,7 +136,8 @@ type CapabilityVersion int // - 93: 2024-05-06: added support for stateful firewalling. // - 94: 2024-05-06: Client understands Node.IsJailed. // - 95: 2024-05-06: Client uses NodeAttrUserDialUseRoutes to change DNS dialing behavior. -const CurrentCapabilityVersion CapabilityVersion = 95 +// - 96: 2024-05-29: Client understands NodeAttrSSHBehaviorV1 +const CurrentCapabilityVersion CapabilityVersion = 96 type StableID string @@ -2274,6 +2275,10 @@ const ( // depending on the destination address and the configured routes. When present, it also makes // the DNS forwarder use UserDial instead of SystemDial when dialing resolvers. NodeAttrUserDialUseRoutes NodeCapability = "user-dial-routes" + + // NodeAttrSSHBehaviorV1 forces SSH to use the V1 behavior (no su, run SFTP in-process) + // Added 2024-05-29 in Tailscale version 1.68. + NodeAttrSSHBehaviorV1 NodeCapability = "ssh-behavior-v1" ) // SetDNSRequest is a request to add a DNS record.