[v10] Terminate the local shell when a session closes

lib/srv/ctx.go

@@ -349,6 +349,11 @@ type ServerContext struct {
 	contr *os.File
 	contw *os.File
 
+	// killShell{r,w} are used to send kill signal to the child process
+	// to terminate the shell.
+	killShellr *os.File
+	killShellw *os.File
+
 	// ChannelType holds the type of the channel. For example "session" or
 	// "direct-tcpip". Used to create correct subcommand during re-exec.
 	ChannelType string
@@ -497,6 +502,14 @@ func NewServerContext(ctx context.Context, parent *sshutils.ConnectionContext, s
 	child.AddCloser(child.contr)
 	child.AddCloser(child.contw)
 
+	child.killShellr, child.killShellw, err = os.Pipe()
+	if err != nil {
+		childErr := child.Close()
+		return nil, nil, trace.NewAggregate(err, childErr)
+	}
+	child.AddCloser(child.killShellr)
+	child.AddCloser(child.killShellw)
+
 	// Create pipe used to get X11 forwarding ready signal from the child process.
 	child.x11rdyr, child.x11rdyw, err = os.Pipe()
 	if err != nil {

lib/srv/exec_linux_test.go

@@ -22,34 +22,16 @@ package srv
 import (
 	"fmt"
 	"os"
-	os_exec "os/exec"
+	"os/exec"
 	"os/user"
 	"strconv"
 	"syscall"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
-
-	"github.com/gravitational/teleport/lib/utils"
 )
 
-// TestMain will re-execute Teleport to run a command if "exec" is passed to
-// it as an argument. Otherwise it will run tests as normal.
-func TestMain(m *testing.M) {
-	utils.InitLoggerForTests()
-	// If the test is re-executing itself, execute the command that comes over
-	// the pipe.
-	if IsReexec() {
-		RunAndExit(os.Args[1])
-		return
-	}
-
-	// Otherwise run tests as normal.
-	code := m.Run()
-	os.Exit(code)
-}
-
 func TestOSCommandPrep(t *testing.T) {
 	srv := newMockServer(t)
 	scx := newExecServerContext(t, srv)
@@ -139,7 +121,7 @@ func TestContinue(t *testing.T) {
 
 	// Configure Session Context to re-exec "ls".
 	var err error
-	lsPath, err := os_exec.LookPath("ls")
+	lsPath, err := exec.LookPath("ls")
 	require.NoError(t, err)
 	scx.execRequest.SetCommand(lsPath)
 
@@ -164,7 +146,7 @@ func TestContinue(t *testing.T) {
 	case <-time.After(5 * time.Second):
 	}
 
-	// Close the continue pipe to signal to Teleport to now execute the
+	// Close the continue and terminate pipe to signal to Teleport to now execute the
 	// requested program.
 	err = scx.contw.Close()
 	require.NoError(t, err)

lib/srv/exec_test.go

@@ -30,8 +30,26 @@ import (
 	apievents "github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/lib/events"
 	"github.com/gravitational/teleport/lib/sshutils"
+	"github.com/gravitational/teleport/lib/utils"
 )
 
+// TestMain will re-execute Teleport to run a command if "exec" is passed to
+// it as an argument. Otherwise, it will run tests as normal.
+func TestMain(m *testing.M) {
+	utils.InitLoggerForTests()
+
+	// If the test is re-executing itself, execute the command that comes over
+	// the pipe.
+	if IsReexec() {
+		RunAndExit(os.Args[1])
+		return
+	}
+
+	// Otherwise run tests as normal.
+	code := m.Run()
+	os.Exit(code)
+}
+
 // TestEmitExecAuditEvent make sure the full command and exit code for a
 // command is always recorded.
 func TestEmitExecAuditEvent(t *testing.T) {

lib/srv/mock.go

@@ -95,6 +95,9 @@ func newTestServerContext(t *testing.T, srv Server, roleSet services.RoleSet) *S
 	scx.contr, scx.contw, err = os.Pipe()
 	require.NoError(t, err)
 
+	scx.killShellr, scx.killShellw, err = os.Pipe()
+	require.NoError(t, err)
+
 	t.Cleanup(func() { require.NoError(t, scx.Close()) })
 
 	return scx

lib/srv/reexec.go

@@ -58,6 +58,12 @@ const (
 	// it can continue after the parent process assigns a cgroup to the
 	// child process.
 	ContinueFile
+	// TerminateFile is used to communicate to the child process that
+	// the interactive terminal should be killed as the client ended the
+	// SSH session and without termination the terminal process will be assigned
+	// to pid 1 and "live forever". Killing the shell should not prevent processes
+	// preventing SIGHUP to be reassigned (ex. processes running with nohup).
+	TerminateFile
 	// X11File is used to communicate to the parent process that the child
 	// process has set up X11 forwarding.
 	X11File
@@ -68,7 +74,7 @@ const (
 
 	// FirstExtraFile is the first file descriptor that will be valid when
 	// extra files are passed to child processes without a terminal.
-	FirstExtraFile FileFD = X11File + 1
+	FirstExtraFile = X11File + 1
 )
 
 // ExecCommand contains the payload to "teleport exec" which will be used to
@@ -181,14 +187,18 @@ func RunCommand() (errw io.Writer, code int, err error) {
 	errorWriter := os.Stdout
 
 	// Parent sends the command payload in the third file descriptor.
-	cmdfd := os.NewFile(CommandFile, "/proc/self/fd/3")
+	cmdfd := os.NewFile(CommandFile, fmt.Sprintf("/proc/self/fd/%d", CommandFile))
 	if cmdfd == nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("command pipe not found")
 	}
-	contfd := os.NewFile(ContinueFile, "/proc/self/fd/4")
+	contfd := os.NewFile(ContinueFile, fmt.Sprintf("/proc/self/fd/%d", ContinueFile))
 	if contfd == nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("continue pipe not found")
 	}
+	termiantefd := os.NewFile(TerminateFile, fmt.Sprintf("/proc/self/fd/%d", TerminateFile))
+	if termiantefd == nil {
+		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("terminate pipe not found")
+	}
 
 	// Read in the command payload.
 	var b bytes.Buffer
@@ -237,8 +247,8 @@ func RunCommand() (errw io.Writer, code int, err error) {
 	// PTY and TTY. Extract them and set the controlling TTY. Otherwise, connect
 	// std{in,out,err} directly.
 	if c.Terminal {
-		pty = os.NewFile(PTYFile, "/proc/self/fd/6")
-		tty = os.NewFile(TTYFile, "/proc/self/fd/7")
+		pty = os.NewFile(PTYFile, fmt.Sprintf("/proc/self/fd/%d", PTYFile))
+		tty = os.NewFile(TTYFile, fmt.Sprintf("/proc/self/fd/%d", TTYFile))
 		if pty == nil || tty == nil {
 			return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("pty and tty not found")
 		}
@@ -378,7 +388,7 @@ func RunCommand() (errw io.Writer, code int, err error) {
 		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", x11.DisplayEnv, c.X11Config.XAuthEntry.Display.String()))
 
 		// Open x11rdy fd to signal parent process once X11 forwarding is set up.
-		x11rdyfd := os.NewFile(X11File, "/proc/self/fd/5")
+		x11rdyfd := os.NewFile(X11File, fmt.Sprintf("/proc/self/fd/%d", X11File))
 		if x11rdyfd == nil {
 			return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("continue pipe not found")
 		}
@@ -401,19 +411,13 @@ func RunCommand() (errw io.Writer, code int, err error) {
 	}
 
 	// Start the command.
-	err = cmd.Start()
-	if err != nil {
+	if err := cmd.Start(); err != nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
 	}
 
 	parkerCancel()
 
-	// Wait for the command to exit. It doesn't make sense to print an error
-	// message here because the shell has successfully started. If an error
-	// occurred during shell execution or the shell exits with an error (like
-	// running exit 2), the shell will print an error if appropriate and return
-	// an exit code.
-	err = cmd.Wait()
+	err = waitForShell(termiantefd, cmd)
 
 	if uaccEnabled {
 		uaccErr := uacc.Close(c.UaccMetadata.UtmpPath, c.UaccMetadata.WtmpPath, tty)
@@ -425,6 +429,42 @@ func RunCommand() (errw io.Writer, code int, err error) {
 	return io.Discard, exitCode(err), trace.Wrap(err)
 }
 
+// waitForShell waits either for the command to return or the kill signal from the parent Teleport process.
+func waitForShell(termiantefd *os.File, cmd *exec.Cmd) error {
+	terminateChan := make(chan error)
+
+	go func() {
+		buf := make([]byte, 1)
+		// Wait for the terminate file descriptor to be closed. The FD will be closed when Teleport
+		// parent process wants to terminate the remote command and all childs.
+		_, err := termiantefd.Read(buf)
+		if err == io.EOF {
+			// Kill the shell process
+			err = trace.Errorf("shell process has been killed: %w", cmd.Process.Kill())
+		} else {
+			err = trace.Errorf("failed to read from terminate file: %w", err)
+		}
+		terminateChan <- err
+	}()
+
+	go func() {
+		// Wait for the command to exit. It doesn't make sense to print an error
+		// message here because the shell has successfully started. If an error
+		// occurred during shell execution or the shell exits with an error (like
+		// running exit 2), the shell will print an error if appropriate and return
+		// an exit code.
+		err := cmd.Wait()
+
+		terminateChan <- err
+	}()
+
+	// Wait only for the first error.
+	// If the command returns then we don't need to wait for the error from cmd.Process.Kill().
+	// If the command is being killed, then we don't care about the error code.
+	err := <-terminateChan
+	return err
+}
+
 // osWrapper wraps system calls, so we can replace them in tests.
 type osWrapper struct {
 	LookupGroup    func(name string) (*user.Group, error)
@@ -525,7 +565,7 @@ func RunForward() (errw io.Writer, code int, err error) {
 	errorWriter := os.Stderr
 
 	// Parent sends the command payload in the third file descriptor.
-	cmdfd := os.NewFile(CommandFile, "/proc/self/fd/3")
+	cmdfd := os.NewFile(CommandFile, fmt.Sprintf("/proc/self/fd/%d", CommandFile))
 	if cmdfd == nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("command pipe not found")
 	}
@@ -864,6 +904,7 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 		ExtraFiles: []*os.File{
 			ctx.cmdr,
 			ctx.contr,
+			ctx.killShellr,
 			ctx.x11rdyw,
 		},
 	}

lib/srv/sess.go

@@ -654,25 +654,45 @@ func (s *session) Stop() {
 	s.BroadcastMessage("Stopping session...")
 	s.log.Info("Stopping session")
 
-	// close io copy loops
+	// Close io copy loops
 	s.io.Close()
 
-	// Close and kill terminal
-	if s.term != nil {
-		if err := s.term.Close(); err != nil {
-			s.log.WithError(err).Debug("Failed to close the shell")
-		}
-		if err := s.term.Kill(context.TODO()); err != nil {
-			s.log.WithError(err).Debug("Failed to kill the shell")
-		}
-	}
+	// Make sure that the terminal has been closed
+	s.haltTerminal()
 
 	// Close session tracker and mark it as terminated
 	if err := s.tracker.Close(s.serverCtx); err != nil {
 		s.log.WithError(err).Debug("Failed to close session tracker")
 	}
 }
 
+// haltTerminal closes the terminal. Then is tried to terminate the terminal in a graceful way
+// and kill by sending SIGKILL if the graceful termination fails.
+func (s *session) haltTerminal() {
+	if s.term == nil {
+		return
+	}
+
+	if err := s.term.Close(); err != nil {
+		s.log.WithError(err).Debug("Failed to close the shell")
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	if err := s.term.KillUnderlyingShell(ctx); err != nil {
+		s.log.WithError(err).Debug("Failed to terminate the shell")
+	} else {
+		// Return before we send SIGKILL to the child process, as doing that
+		// could interrupt the "graceful shutdown" process.
+		return
+	}
+
+	if err := s.term.Kill(context.TODO()); err != nil {
+		s.log.WithError(err).Debug("Failed to kill the shell")
+	}
+}
+
 // Close ends the active session and frees all resources. This should only be called
 // by the creator of the session, other closers should use Stop instead. Calling this
 // prematurely can result in missing audit events, session recordings, and other