From 7ed307ff0cc0b6952c2912bc2e14fb825a4eb9cb Mon Sep 17 00:00:00 2001
From: Andrew LeFevre <andrew.lefevre@goteleport.com>
Date: Fri, 21 Oct 2022 15:33:19 -0400
Subject: [PATCH] block SFTP connections when moderated sessions are enforced

Fixes #17341
---
 lib/srv/ctx.go               | 27 ++++++++++++++++++++++++++-
 lib/srv/ctx_test.go          | 27 +++++++++++++++++++++++++--
 lib/srv/regular/sshserver.go | 12 ++++++++----
 lib/srv/sess.go              |  2 +-
 4 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go
index de748118ffb62..771080e72f0f9 100644
--- a/lib/srv/ctx.go
+++ b/lib/srv/ctx.go
@@ -71,7 +71,10 @@ var (
 	)
 )
 
-var ErrNodeFileCopyingNotPermitted = trace.AccessDenied("node does not allow file copying via SCP or SFTP")
+var (
+	ErrNodeFileCopyingNotPermitted  = trace.AccessDenied("node does not allow file copying via SCP or SFTP")
+	errCannotStartUnattendedSession = trace.AccessDenied("lacking privileges to start unattended session")
+)
 
 func init() {
 	prometheus.MustRegister(serverTX)
@@ -647,6 +650,28 @@ func (c *ServerContext) CheckFileCopyingAllowed() error {
 	return nil
 }
 
+// CheckSFTPAllowed returns an error if remote file operations via SCP
+// or SFTP are not allowed by the user's role or the node's config, or
+// if the user is not allowed to start unattended sessions.
+func (c *ServerContext) CheckSFTPAllowed() error {
+	if err := c.CheckFileCopyingAllowed(); err != nil {
+		return trace.Wrap(err)
+	}
+
+	// ensure moderated session policies allow starting an unattended session
+	policySets := c.Identity.AccessChecker.SessionPolicySets()
+	checker := auth.NewSessionAccessEvaluator(policySets, types.SSHSessionKind, c.Identity.TeleportUser)
+	canStart, _, err := checker.FulfilledFor(nil)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	if !canStart {
+		return errCannotStartUnattendedSession
+	}
+
+	return nil
+}
+
 // OpenXServerListener opens a new XServer unix listener.
 func (c *ServerContext) OpenXServerListener(x11Req x11.ForwardRequestPayload, displayOffset, maxDisplays int) error {
 	l, display, err := x11.OpenNewXServerListener(displayOffset, maxDisplays, x11Req.ScreenNumber)
diff --git a/lib/srv/ctx_test.go b/lib/srv/ctx_test.go
index b627eaf4ff0e1..aeeedc71ffcac 100644
--- a/lib/srv/ctx_test.go
+++ b/lib/srv/ctx_test.go
@@ -24,7 +24,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestCheckFileCopyingAllowed(t *testing.T) {
+func TestCheckSFTPAllowed(t *testing.T) {
 	srv := newMockServer(t)
 	ctx := newTestServerContext(t, srv, nil)
 
@@ -107,6 +107,29 @@ func TestCheckFileCopyingAllowed(t *testing.T) {
 			},
 			expectedErr: errRoleFileCopyingNotPermitted,
 		},
+		{
+			name:                 "moderated sessions enforced",
+			nodeAllowFileCopying: true,
+			roles: []types.Role{
+				&types.RoleV5{
+					Kind: types.KindNode,
+					Spec: types.RoleSpecV5{
+						Allow: types.RoleConditions{
+							RequireSessionJoin: []*types.SessionRequirePolicy{
+								{
+									Name:   "test",
+									Filter: `contains(user.roles, "auditor")`,
+									Kinds:  []string{string(types.SSHSessionKind)},
+									Modes:  []string{string(types.SessionModeratorMode)},
+									Count:  3,
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedErr: errCannotStartUnattendedSession,
+		},
 	}
 
 	for _, tt := range tests {
@@ -123,7 +146,7 @@ func TestCheckFileCopyingAllowed(t *testing.T) {
 				roles,
 			)
 
-			err := ctx.CheckFileCopyingAllowed()
+			err := ctx.CheckSFTPAllowed()
 			if tt.expectedErr == nil {
 				require.NoError(t, err)
 			} else {
diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go
index 7d73dfdda8faf..ed077e3a51603 100644
--- a/lib/srv/regular/sshserver.go
+++ b/lib/srv/regular/sshserver.go
@@ -2040,6 +2040,12 @@ func (s *Server) handleProxyJump(ctx context.Context, ccx *sshutils.ConnectionCo
 	}
 }
 
+// TODO: tsh scp will display neither the message sent in stderr or in
+// the reply; github.com/pkg/sftp ignores the SSH channel stderr, and
+// golang.org/x/crypto/ssh.channel.SendRequest ignores the message in
+// a channel reply. This is bad UX for users, as
+// 'ssh: subsystem request failed' will be the only error displayed when
+// access is denied.
 func (s *Server) replyError(ch ssh.Channel, req *ssh.Request, err error) {
 	s.Logger.Error(err)
 	// Terminate the error with a newline when writing to remote channel's
@@ -2067,10 +2073,8 @@ func (s *Server) parseSubsystemRequest(req *ssh.Request, ch ssh.Channel, ctx *sr
 	case s.proxyMode && strings.HasPrefix(r.Name, "proxysites"):
 		return parseProxySitesSubsys(r.Name, s)
 	case r.Name == sftpSubsystem:
-		if err := ctx.CheckFileCopyingAllowed(); err != nil {
-			// Add an extra newline here to separate this error message
-			// from a potential OpenSSH error message
-			writeStderr(ch, err.Error()+"\n")
+		if err := ctx.CheckSFTPAllowed(); err != nil {
+			s.replyError(ch, req, err)
 			return nil, trace.Wrap(err)
 		}
 
diff --git a/lib/srv/sess.go b/lib/srv/sess.go
index 8e272a114ffc7..6d77cde02f263 100644
--- a/lib/srv/sess.go
+++ b/lib/srv/sess.go
@@ -289,7 +289,7 @@ func (s *SessionRegistry) OpenExecSession(ctx context.Context, channel ssh.Chann
 	}
 
 	if !canStart {
-		return trace.AccessDenied("lacking privileges to start unattended session")
+		return errCannotStartUnattendedSession
 	}
 
 	// Start a non-interactive session (TTY attached). Close the session if an error