From a73ce89cb3e47e7d6eca029e2725f6083cb0c5b0 Mon Sep 17 00:00:00 2001
From: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
Date: Wed, 27 Nov 2024 13:59:32 -0500
Subject: [PATCH] Restore interactive PAM authentication (#49487)

https://github.com/gravitational/teleport/pull/29279 caused PAM to
deadlock when performing interactive authentication. To restore
the previous semblance of functional PAM, this reverts waiting
for PAM to be complete if BPF is disabled. #29279 was specifically
added to prevent systemd, which may be invoked via a PAM module,
from moving the exec subprocess to a different cgroup. Since
cgroups are not used outside of Enhanced Session Recording this
is a stop-gap measure that can allow mose users of PAM to get an
immediate restoration of behavior while a more long term and sane
approach to performing PAM during the SSH handshake can be
considered, evaluated, and tested.

Closes #49028.
---
 lib/srv/sess.go | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/lib/srv/sess.go b/lib/srv/sess.go
index a0eea46511f8c..9f0d967df055d 100644
--- a/lib/srv/sess.go
+++ b/lib/srv/sess.go
@@ -1366,12 +1366,31 @@ func (s *session) startInteractive(ctx context.Context, scx *ServerContext, p *p
 		Events:         scx.Identity.AccessChecker.EnhancedRecordingSet(),
 	}
 
-	if err := s.term.WaitForChild(); err != nil {
-		s.logger.ErrorContext(ctx, "Child process never became ready.", "error", err)
-		return trace.Wrap(err)
+	bpfService := scx.srv.GetBPF()
+
+	// Only wait for the child to be "ready" if BPF is enabled. This is required
+	// because PAM might inadvertently move the child process to another cgroup
+	// by invoking systemd. If this happens, then the cgroup filter used by BPF
+	// will be looking for events in the wrong cgroup and no events will be captured.
+	// However, unconditionally waiting for the child to be ready results in PAM
+	// deadlocking because stdin/stdout/stderr which it uses to relay details from
+	// PAM auth modules are not properly copied until _after_ the shell request is
+	// replied to.
+	if bpfService.Enabled() {
+		if err := s.term.WaitForChild(); err != nil {
+			s.logger.ErrorContext(ctx, "Child process never became ready.", "error", err)
+			return trace.Wrap(err)
+		}
+	} else {
+		// Clean up the read half of the pipe, and set it to nil so that when the
+		// ServerContext is closed it doesn't attempt to a second close.
+		if err := s.scx.readyr.Close(); err != nil {
+			s.logger.ErrorContext(ctx, "Failed closing child ready pipe", "error", err)
+		}
+		s.scx.readyr = nil
 	}
 
-	if cgroupID, err := scx.srv.GetBPF().OpenSession(sessionContext); err != nil {
+	if cgroupID, err := bpfService.OpenSession(sessionContext); err != nil {
 		s.logger.ErrorContext(ctx, "Failed to open enhanced recording (interactive) session.", "error", err)
 		return trace.Wrap(err)
 	} else if cgroupID > 0 {
@@ -1380,8 +1399,7 @@ func (s *session) startInteractive(ctx context.Context, scx *ServerContext, p *p
 		go func() {
 			// Close the BPF recording session once the session is closed
 			<-s.stopC
-			err = scx.srv.GetBPF().CloseSession(sessionContext)
-			if err != nil {
+			if err := bpfService.CloseSession(sessionContext); err != nil {
 				s.logger.ErrorContext(ctx, "Failed to close enhanced recording (interactive) session.", "error", err)
 			}
 		}()