Defer setting up enhanced recording until after PAM has completed initializing

lib/srv/ctx.go

@@ -389,6 +389,11 @@ type ServerContext struct {
 	contr *os.File
 	contw *os.File
 
+	// ready{r,w} is used to send the ready signal from the child process
+	// to the parent process.
+	readyr *os.File
+	readyw *os.File
+
 	// killShell{r,w} are used to send kill signal to the child process
 	// to terminate the shell.
 	killShellr *os.File
@@ -561,6 +566,15 @@ func NewServerContext(ctx context.Context, parent *sshutils.ConnectionContext, s
 	child.AddCloser(child.contr)
 	child.AddCloser(child.contw)
 
+	// Create pipe used to signal continue to parent process.
+	child.readyr, child.readyw, err = os.Pipe()
+	if err != nil {
+		childErr := child.Close()
+		return nil, nil, trace.NewAggregate(err, childErr)
+	}
+	child.AddCloser(child.readyr)
+	child.AddCloser(child.readyw)
+
 	child.killShellr, child.killShellw, err = os.Pipe()
 	if err != nil {
 		childErr := child.Close()

lib/srv/exec.go

@@ -76,6 +76,10 @@ type Exec interface {
 	// Wait will block while the command executes.
 	Wait() *ExecResult
 
+	// WaitForChild blocks until the child process has completed any required
+	// setup operations before proceeding with execution.
+	WaitForChild() error
+
 	// Continue will resume execution of the process after it completes its
 	// pre-processing routine (placed in a cgroup).
 	Continue()
@@ -181,6 +185,12 @@ func (e *localExec) Start(ctx context.Context, channel ssh.Channel) (*ExecResult
 			Code:    exitCode(err),
 		}, trace.ConvertSystemError(err)
 	}
+	// Close our half of the write pipe since it is only to be used by the child process.
+	// Not closing prevents being signaled when the child closes its half.
+	if err := e.Ctx.readyw.Close(); err != nil {
+		e.Ctx.Logger.WithError(err).Warn("Failed to close parent process ready signal write fd")
+	}
+	e.Ctx.readyw = nil
 
 	go func() {
 		if _, err := io.Copy(inputWriter, channel); err != nil {
@@ -219,6 +229,14 @@ func (e *localExec) Wait() *ExecResult {
 	return execResult
 }
 
+func (e *localExec) WaitForChild() error {
+	err := waitForSignal(e.Ctx.readyr, 20*time.Second)
+	closeErr := e.Ctx.readyr.Close()
+	// Set to nil so the close in the context doesn't attempt to re-close.
+	e.Ctx.readyr = nil
+	return trace.NewAggregate(err, closeErr)
+}
+
 // Continue will resume execution of the process after it completes its
 // pre-processing routine (placed in a cgroup).
 func (e *localExec) Continue() {
@@ -284,28 +302,26 @@ func checkSCPAllowed(scx *ServerContext, command string) (bool, error) {
 	return true, trace.Wrap(scx.CheckFileCopyingAllowed())
 }
 
-// waitForContinue will wait 10 seconds for the continue signal, if not
+// waitForSignal will wait 10 seconds for the other side of the pipe to signal, if not
 // received, it will stop waiting and exit.
-func waitForContinue(contfd *os.File) error {
+func waitForSignal(fd *os.File, timeout time.Duration) error {
 	waitCh := make(chan error, 1)
 	go func() {
-		// Reading from the continue file descriptor will block until it's closed. It
-		// won't be closed until the parent has placed it in a cgroup.
-		buf := make([]byte, 1)
-		_, err := contfd.Read(buf)
+		// Reading from the file descriptor will block until it's closed.
+		_, err := fd.Read(make([]byte, 1))
 		if errors.Is(err, io.EOF) {
 			err = nil
 		}
 		waitCh <- err
 	}()
 
-	// Wait for 10 seconds and then timeout if no continue signal has been sent.
-	timeout := time.NewTimer(10 * time.Second)
-	defer timeout.Stop()
+	// Timeout if no signal has been sent within the provided duration.
+	timer := time.NewTimer(timeout)
+	defer timer.Stop()
 
 	select {
-	case <-timeout.C:
-		return trace.BadParameter("timed out waiting for continue signal")
+	case <-timer.C:
+		return trace.LimitExceeded("timed out waiting for continue signal")
 	case err := <-waitCh:
 		return err
 	}
@@ -389,6 +405,8 @@ func (e *remoteExec) Wait() *ExecResult {
 	}
 }
 
+func (e *remoteExec) WaitForChild() error { return nil }
+
 // Continue does nothing for remote command execution.
 func (e *remoteExec) Continue() {}
 

lib/srv/exec_linux_test.go

@@ -29,6 +29,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 )
 
@@ -134,7 +135,13 @@ func TestContinue(t *testing.T) {
 	// Re-execute Teleport and run "ls". Signal over the context when execution
 	// is complete.
 	go func() {
-		cmdDone <- cmd.Run()
+		if err := cmd.Start(); err != nil {
+			cmdDone <- err
+		}
+
+		// Close the read half of the pipe to unblock the ready signal.
+		closeErr := scx.readyw.Close()
+		cmdDone <- trace.NewAggregate(closeErr, cmd.Wait())
 	}()
 
 	// Wait for the process. Since the continue pipe has not been closed, the
@@ -145,10 +152,11 @@ func TestContinue(t *testing.T) {
 	case <-time.After(5 * time.Second):
 	}
 
-	// Close the continue and terminate pipe to signal to Teleport to now execute the
-	// requested program.
-	err = scx.contw.Close()
-	require.NoError(t, err)
+	// Wait for the child process to indicate its completed initialization.
+	require.NoError(t, scx.execRequest.WaitForChild())
+
+	// Signal to child that it may execute the requested program.
+	scx.execRequest.Continue()
 
 	// Program should have executed now. If the complete signal has not come
 	// over the context, something failed.

lib/srv/mock.go

@@ -96,6 +96,9 @@ func newTestServerContext(t *testing.T, srv Server, roleSet services.RoleSet) *S
 	scx.contr, scx.contw, err = os.Pipe()
 	require.NoError(t, err)
 
+	scx.readyr, scx.readyw, err = os.Pipe()
+	require.NoError(t, err)
+
 	scx.killShellr, scx.killShellw, err = os.Pipe()
 	require.NoError(t, err)
 

lib/srv/reexec.go

@@ -60,6 +60,10 @@ const (
 	// it can continue after the parent process assigns a cgroup to the
 	// child process.
 	ContinueFile
+	// ReadyFile is used to communicate to the parent process that
+	// the child has completed any setup operations that must occur before
+	// the child is placed into its cgroup.
+	ReadyFile
 	// 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
@@ -69,7 +73,7 @@ const (
 	// X11File is used to communicate to the parent process that the child
 	// process has set up X11 forwarding.
 	X11File
-	// ErrorFile  is used to communicate any errors terminating the child process
+	// ErrorFile is used to communicate any errors terminating the child process
 	// to the parent process
 	ErrorFile
 	// PTYFile is a PTY the parent process passes to the child process.
@@ -213,6 +217,21 @@ func RunCommand() (errw io.Writer, code int, err error) {
 	if contfd == nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("continue pipe not found")
 	}
+	readyfd := os.NewFile(ReadyFile, fdName(ReadyFile))
+	if readyfd == nil {
+		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("ready pipe not found")
+	}
+
+	// Ensure that the ready signal is sent if a failure causes execution
+	// to terminate prior to actually becoming ready to unblock the parent process.
+	defer func() {
+		if readyfd == nil {
+			return
+		}
+
+		_ = readyfd.Close()
+	}()
+
 	termiantefd := os.NewFile(TerminateFile, fdName(TerminateFile))
 	if termiantefd == nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("terminate pipe not found")
@@ -317,6 +336,14 @@ func RunCommand() (errw io.Writer, code int, err error) {
 		pamEnvironment = pamContext.Environment()
 	}
 
+	// Alert the parent process that the child process has completed any setup operations,
+	// and that we are now waiting for the continue signal before proceeding. This is needed
+	// to ensure that PAM changing the cgroup doesn't bypass enhanced recording.
+	if err := readyfd.Close(); err != nil {
+		return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
+	}
+	readyfd = nil
+
 	localUser, err := user.Lookup(c.Login)
 	if err != nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
@@ -330,7 +357,7 @@ func RunCommand() (errw io.Writer, code int, err error) {
 
 	// Wait until the continue signal is received from Teleport signaling that
 	// the child process has been placed in a cgroup.
-	err = waitForContinue(contfd)
+	err = waitForSignal(contfd, 10*time.Second)
 	if err != nil {
 		return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
 	}
@@ -900,6 +927,7 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er
 		ExtraFiles: []*os.File{
 			ctx.cmdr,
 			ctx.contr,
+			ctx.readyw,
 			ctx.killShellr,
 			ctx.x11rdyw,
 			ctx.errw,

lib/srv/sess.go

@@ -1190,6 +1190,11 @@ func (s *session) startInteractive(ctx context.Context, scx *ServerContext, p *p
 		Events:         scx.Identity.AccessChecker.EnhancedRecordingSet(),
 	}
 
+	if err := s.term.WaitForChild(); err != nil {
+		s.log.WithError(err).Error("Child process never became ready")
+		return trace.Wrap(err)
+	}
+
 	if cgroupID, err := scx.srv.GetBPF().OpenSession(sessionContext); err != nil {
 		s.log.WithError(err).Error("Failed to open enhanced recording (interactive) session")
 		return trace.Wrap(err)
@@ -1353,6 +1358,12 @@ func (s *session) startExec(ctx context.Context, channel ssh.Channel, scx *Serve
 		User:           scx.Identity.TeleportUser,
 		Events:         scx.Identity.AccessChecker.EnhancedRecordingSet(),
 	}
+
+	if err := execRequest.WaitForChild(); err != nil {
+		s.log.WithError(err).Error("Child process never became ready")
+		return trace.Wrap(err)
+	}
+
 	cgroupID, err := scx.srv.GetBPF().OpenSession(sessionContext)
 	if err != nil {
 		s.log.WithError(err).Errorf("Failed to open enhanced recording (exec) session: %v", execRequest.GetCommand())

lib/srv/term.go

@@ -61,6 +61,10 @@ type Terminal interface {
 	// Wait will block until the terminal is complete.
 	Wait() (*ExecResult, error)
 
+	// WaitForChild blocks until the child process has completed any required
+	// setup operations before proceeding with execution.
+	WaitForChild() error
+
 	// Continue will resume execution of the process after it completes its
 	// pre-processing routine (placed in a cgroup).
 	Continue()
@@ -208,6 +212,13 @@ func (t *terminal) Run(ctx context.Context) error {
 		return trace.Wrap(err)
 	}
 
+	// Close our half of the write pipe since it is only to be used by the child process.
+	// Not closing prevents being signaled when the child closes its half.
+	if err := t.serverContext.readyw.Close(); err != nil {
+		t.serverContext.Logger.WithError(err).Warn("Failed to close parent process ready signal write fd")
+	}
+	t.serverContext.readyw = nil
+
 	// Save off the PID of the Teleport process under which the shell is executing.
 	t.pid = t.cmd.Process.Pid
 
@@ -236,6 +247,14 @@ func (t *terminal) Wait() (*ExecResult, error) {
 	}, nil
 }
 
+func (t *terminal) WaitForChild() error {
+	err := waitForSignal(t.serverContext.readyr, 20*time.Second)
+	closeErr := t.serverContext.readyr.Close()
+	// Set to nil so the close in the context doesn't attempt to re-close.
+	t.serverContext.readyr = nil
+	return trace.NewAggregate(err, closeErr)
+}
+
 // Continue will resume execution of the process after it completes its
 // pre-processing routine (placed in a cgroup).
 func (t *terminal) Continue() {
@@ -592,6 +611,10 @@ func (t *remoteTerminal) Wait() (*ExecResult, error) {
 	}, nil
 }
 
+func (t *remoteTerminal) WaitForChild() error {
+	return nil
+}
+
 // Continue does nothing for remote command execution.
 func (t *remoteTerminal) Continue() {}
 

lib/srv/term_test.go

@@ -116,9 +116,11 @@ func TestTerminal_KillUnderlyingShell(t *testing.T) {
 		errors <- err
 	}()
 
+	// Wait for the child process to indicate its completed initialization.
+	require.NoError(t, scx.execRequest.WaitForChild())
+
 	// Continue execution
-	err = scx.contw.Close()
-	require.NoError(t, err)
+	scx.execRequest.Continue()
 
 	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
 	t.Cleanup(cancel)