Prevent ssh.Session SendRequest from wrapping payload twice (#16018)

* Prevent ssh.Session SendRequest from wrapping payload twice

Payloads were inadvertently being wrapped twice by sessions making
the ssh server unable to parse the request payload. This is largely
only an issue for window-change requests, as they are the only request
being sent directly via a session that contains a payload. All window-change
requests now use ssh.Session WindowChange instead or sending the request
manually.

api/observability/tracing/ssh/session.go

@@ -49,7 +49,9 @@ func (s *Session) SendRequest(ctx context.Context, name string, wantReply bool,
 	)
 	defer span.End()
 
-	return s.Session.SendRequest(name, wantReply, wrapPayload(ctx, s.wrapper.capability, config.TextMapPropagator, payload))
+	// no need to wrap payload here, the session's channel wrapper will do it for us
+	s.wrapper.addContext(ctx, name)
+	return s.Session.SendRequest(name, wantReply, payload)
 }
 
 // Setenv sets an environment variable that will be applied to any

lib/client/session.go

@@ -446,15 +446,7 @@ func (ns *NodeSession) updateTerminalSize(ctx context.Context, s *tracessh.Sessi
 			}
 
 			// Send the "window-change" request over the channel.
-			_, err = s.SendRequest(
-				ctx,
-				sshutils.WindowChangeRequest,
-				false,
-				ssh.Marshal(sshutils.WinChangeReqParams{
-					W: uint32(currWidth),
-					H: uint32(currHeight),
-				}))
-			if err != nil {
+			if err = s.WindowChange(ctx, int(currHeight), int(currWidth)); err != nil {
 				log.Warnf("Unable to send %v reqest: %v.", sshutils.WindowChangeRequest, err)
 				continue
 			}

lib/srv/term.go

@@ -26,19 +26,16 @@ import (
 	"sync"
 	"syscall"
 
+	"github.com/creack/pty"
+	"github.com/gravitational/trace"
+	"github.com/moby/term"
+	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 
 	"github.com/gravitational/teleport"
 	tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh"
 	"github.com/gravitational/teleport/lib/services"
 	rsession "github.com/gravitational/teleport/lib/session"
-	"github.com/gravitational/teleport/lib/sshutils"
-
-	"github.com/creack/pty"
-	"github.com/moby/term"
-	log "github.com/sirupsen/logrus"
-
-	"github.com/gravitational/trace"
 )
 
 // LookupUser is used to mock the value returned by user.Lookup(string).
@@ -614,20 +611,7 @@ func (t *remoteTerminal) SetTerminalModes(termModes ssh.TerminalModes) {
 }
 
 func (t *remoteTerminal) windowChange(ctx context.Context, w int, h int) error {
-	type windowChangeRequest struct {
-		W   uint32
-		H   uint32
-		Wpx uint32
-		Hpx uint32
-	}
-	req := windowChangeRequest{
-		W:   uint32(w),
-		H:   uint32(h),
-		Wpx: uint32(w * 8),
-		Hpx: uint32(h * 8),
-	}
-	_, err := t.session.SendRequest(ctx, sshutils.WindowChangeRequest, false, ssh.Marshal(&req))
-	return err
+	return trace.Wrap(t.session.WindowChange(ctx, h, w))
 }
 
 // prepareRemoteSession prepares the more session for execution.

lib/srv/termhandlers.go

@@ -202,9 +202,5 @@ func parseWinChange(req *ssh.Request) (*rsession.TerminalParams, error) {
 	}
 
 	params, err := rsession.NewTerminalParamsFromUint32(r.W, r.H)
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-
-	return params, nil
+	return params, trace.Wrap(err)
 }

lib/sshutils/req.go

@@ -53,22 +53,21 @@ type PTYReqParams struct {
 // TerminalModes converts encoded terminal modes into a ssh.TerminalModes map.
 // The encoding is described in: https://tools.ietf.org/html/rfc4254#section-8
 //
-//   All 'encoded terminal modes' (as passed in a pty request) are encoded
-//   into a byte stream.  It is intended that the coding be portable
-//   across different environments.  The stream consists of opcode-
-//   argument pairs wherein the opcode is a byte value.  Opcodes 1 to 159
-//   have a single uint32 argument.  Opcodes 160 to 255 are not yet
-//   defined, and cause parsing to stop (they should only be used after
-//   any other data).  The stream is terminated by opcode TTY_OP_END
-//   (0x00).
+//	All 'encoded terminal modes' (as passed in a pty request) are encoded
+//	into a byte stream.  It is intended that the coding be portable
+//	across different environments.  The stream consists of opcode-
+//	argument pairs wherein the opcode is a byte value.  Opcodes 1 to 159
+//	have a single uint32 argument.  Opcodes 160 to 255 are not yet
+//	defined, and cause parsing to stop (they should only be used after
+//	any other data).  The stream is terminated by opcode TTY_OP_END
+//	(0x00).
 //
 // In practice, this means encoded terminal modes get translated like below:
 //
-//  0x80 0x00 0x00 0x38 0x40  0x81 0x00 0x00 0x38 0x40  0x35 0x00 0x00 0x00 0x00  0x00
-//  |___|__________________|  |___|__________________|  |___|__________________|  |__|
-//         0x80: 0x3840              0x81: 0x3840              0x35: 0x00         0x00
-//  ssh.TTY_OP_ISPEED: 14400  ssh.TTY_OP_OSPEED: 14400         ssh.ECHO:0
-//
+//	0x80 0x00 0x00 0x38 0x40  0x81 0x00 0x00 0x38 0x40  0x35 0x00 0x00 0x00 0x00  0x00
+//	|___|__________________|  |___|__________________|  |___|__________________|  |__|
+//	       0x80: 0x3840              0x81: 0x3840              0x35: 0x00         0x00
+//	ssh.TTY_OP_ISPEED: 14400  ssh.TTY_OP_OSPEED: 14400         ssh.ECHO:0
 func (p *PTYReqParams) TerminalModes() (ssh.TerminalModes, error) {
 	terminalModes := ssh.TerminalModes{}
 

lib/web/terminal.go

@@ -552,15 +552,7 @@ func (t *TerminalHandler) windowChange(ctx context.Context, params *session.Term
 		return
 	}
 
-	_, err := t.sshSession.SendRequest(
-		ctx,
-		sshutils.WindowChangeRequest,
-		false,
-		ssh.Marshal(sshutils.WinChangeReqParams{
-			W: uint32(params.W),
-			H: uint32(params.H),
-		}))
-	if err != nil {
+	if err := t.sshSession.WindowChange(ctx, params.H, params.W); err != nil {
 		t.log.Error(err)
 	}
 }