From fc4ac15a1ced78e6c778678e37172b4ad6f5be61 Mon Sep 17 00:00:00 2001
From: Tim Ross <tim.ross@goteleport.com>
Date: Wed, 13 Dec 2023 17:58:25 -0500
Subject: [PATCH] Always reply to x-teleport-terminal-size if requested

The ssh handlers were only replying to the request in the happy path
and never if something went wrong. This causes our clients to hang
indefinitely waiting since they are reliant on the response to
determine the correct terminal size when joining an existing session.
---
 lib/srv/regular/sshserver.go      |  9 ++++++-
 lib/srv/regular/sshserver_test.go | 42 +++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go
index 124203ebf6489..b447cf3be5e6c 100644
--- a/lib/srv/regular/sshserver.go
+++ b/lib/srv/regular/sshserver.go
@@ -1160,7 +1160,14 @@ func (s *Server) HandleRequest(ctx context.Context, r *ssh.Request) {
 	case teleport.VersionRequest:
 		s.handleVersionRequest(r)
 	case teleport.TerminalSizeRequest:
-		s.termHandlers.HandleTerminalSize(r)
+		if err := s.termHandlers.HandleTerminalSize(r); err != nil {
+			s.Logger.WithError(err).Warn("failed to handle terminal size request")
+			if r.WantReply {
+				if err := r.Reply(false, nil); err != nil {
+					s.Logger.Warnf("Failed to reply to %q request: %v", r.Type, err)
+				}
+			}
+		}
 	default:
 		if r.WantReply {
 			if err := r.Reply(false, nil); err != nil {
diff --git a/lib/srv/regular/sshserver_test.go b/lib/srv/regular/sshserver_test.go
index a8ffc69cdde8b..23dbd1c159d21 100644
--- a/lib/srv/regular/sshserver_test.go
+++ b/lib/srv/regular/sshserver_test.go
@@ -18,6 +18,7 @@ package regular
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 	"io/fs"
@@ -40,6 +41,7 @@ import (
 	"github.com/gravitational/trace"
 	"github.com/jonboulle/clockwork"
 	"github.com/mailgun/timetools"
+	"github.com/moby/term"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
@@ -302,6 +304,46 @@ func newCustomFixture(t *testing.T, mutateCfg func(*auth.TestServerConfig), sshO
 	return f
 }
 
+// TestTerminalSizeRequest validates that terminal size requests are processed and
+// responded to appropriately. Namely, it ensures that a response is sent if the client
+// requests a reply whether processing the request was successful or not.
+func TestTerminalSizeRequest(t *testing.T) {
+	f := newFixtureWithoutDiskBasedLogging(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	t.Run("Invalid session", func(t *testing.T) {
+		ok, resp, err := f.ssh.clt.SendRequest(ctx, teleport.TerminalSizeRequest, true, []byte("1234"))
+		require.NoError(t, err)
+		require.False(t, ok)
+		require.Empty(t, resp)
+	})
+
+	t.Run("Active session", func(t *testing.T) {
+		se, err := f.ssh.clt.NewSession(ctx)
+		require.NoError(t, err)
+		defer se.Close()
+
+		require.NoError(t, se.Shell(ctx))
+
+		expectedSize := term.Winsize{Height: 100, Width: 200}
+		require.NoError(t, se.WindowChange(ctx, int(expectedSize.Height), int(expectedSize.Width)))
+
+		sessions, err := f.ssh.srv.termHandlers.SessionRegistry.SessionTrackerService.GetActiveSessionTrackers(ctx)
+		require.NoError(t, err)
+		require.Len(t, sessions, 1)
+
+		ok, resp, err := f.ssh.clt.SendRequest(ctx, teleport.TerminalSizeRequest, true, []byte(sessions[0].GetSessionID()))
+		require.NoError(t, err)
+		require.True(t, ok)
+		require.NotNil(t, resp)
+
+		var ws term.Winsize
+		require.NoError(t, json.Unmarshal(resp, &ws))
+		require.Empty(t, cmp.Diff(expectedSize, ws, cmp.AllowUnexported(term.Winsize{})))
+	})
+}
+
 // TestMultipleExecCommands asserts that multiple SSH exec commands can not be
 // sent over a single SSH channel, which is disallowed by the SSH standard
 // https://www.ietf.org/rfc/rfc4254.txt