Fix SSH agent forwarding to agentless nodes (#22567)

* lazily forward SSH agents when connecting to agentless nodes

When connecting to unregistered OpenSSH nodes, the SSH agent is always
forwarded. When connecting to registered OpenSSH (agentless) nodes
however, the SSH agent doesn't *need* to be forwarded, so only do so
if the user explicitly asks to.

* test SSH agent forwarding in agentless integration test

Author: capnspacehook

integration/integration_test.go

@@ -49,6 +49,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
+	"golang.org/x/crypto/ssh/agent"
 	"golang.org/x/exp/slices"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/connectivity"
@@ -7333,8 +7334,15 @@ func testAgentlessConnection(t *testing.T, suite *integrationTestSuite) {
 	}, tc.Username)
 	require.NoError(t, err)
 
-	_, _, err = nodeClient.Client.Client.SendRequest("test-request", true, nil)
+	// forward SSH agent
+	sshClient := nodeClient.Client.Client
+	session, err := sshClient.NewSession()
 	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, session.Close())
+	})
+	require.NoError(t, agent.ForwardToAgent(sshClient, tc.LocalAgent()))
+	require.NoError(t, agent.RequestAgentForwarding(session))
 
 	require.NoError(t, nodeClient.Close())
 }
@@ -7366,21 +7374,41 @@ func startSSHServer(t *testing.T, caPubKeys []ssh.PublicKey, hostKey ssh.Signer)
 
 	go func() {
 		nConn, err := lis.Accept()
-		require.NoError(t, err)
+		assert.NoError(t, err)
 		t.Cleanup(func() {
+			// the error is ignored here to avoid failing on net.ErrClosed
 			_ = nConn.Close()
 		})
 
-		conn, _, reqs, err := ssh.NewServerConn(nConn, &sshCfg)
-		require.NoError(t, err)
+		conn, channels, reqs, err := ssh.NewServerConn(nConn, &sshCfg)
+		assert.NoError(t, err)
 		t.Cleanup(func() {
+			// the error is ignored here to avoid failing on net.ErrClosed
 			_ = conn.Close()
 		})
+		go ssh.DiscardRequests(reqs)
 
-		req := <-reqs
-		require.NoError(t, req.Reply(true, nil))
+		var agentForwarded bool
+		for channelReq := range channels {
+			assert.Equal(t, "session", channelReq.ChannelType())
+			channel, reqs, err := channelReq.Accept()
+			assert.NoError(t, err)
+			t.Cleanup(func() {
+				// the error is ignored here to avoid failing on net.ErrClosed
+				_ = channel.Close()
+			})
 
-		require.NoError(t, conn.Close())
+			for req := range reqs {
+				if req.WantReply {
+					assert.NoError(t, req.Reply(true, nil))
+				}
+				if req.Type == sshutils.AgentForwardRequest {
+					agentForwarded = true
+					break
+				}
+			}
+		}
+		assert.True(t, agentForwarded)
 	}()
 
 	return lis.Addr().String()

lib/proxy/router.go

@@ -285,7 +285,6 @@ func (r *Router) DialHost(ctx context.Context, clientSrcAddr, clientDstAddr net.
 		if err != nil {
 			return nil, "", trace.Wrap(err)
 		}
-		// TODO(capnspacehook): remove when forwarding SSH agent to agentless node works
 		agentGetter = nil
 	}
 

lib/srv/forward/sshserver.go

@@ -514,7 +514,10 @@ func (s *Server) GetLockWatcher() *services.LockWatcher {
 }
 
 func (s *Server) Serve() {
-	config := &ssh.ServerConfig{}
+	var (
+		succeeded bool
+		config    = &ssh.ServerConfig{}
+	)
 
 	// Configure callback for user certificate authentication.
 	config.PublicKeyCallback = s.authHandlers.UserKeyAuth
@@ -538,15 +541,22 @@ func (s *Server) Serve() {
 	s.log.Debugf("Supported KEX algorithms: %q.", s.kexAlgorithms)
 	s.log.Debugf("Supported MAC algorithms: %q.", s.macAlgorithms)
 
-	sconn, chans, reqs, err := ssh.NewServerConn(s.serverConn, config)
-	if err != nil {
+	// close
+	defer func() {
+		if succeeded {
+			return
+		}
+
 		if s.userAgent != nil {
 			s.userAgent.Close()
 		}
 		s.targetConn.Close()
 		s.clientConn.Close()
 		s.serverConn.Close()
+	}()
 
+	sconn, chans, reqs, err := ssh.NewServerConn(s.serverConn, config)
+	if err != nil {
 		s.log.Errorf("Unable to create server connection: %v.", err)
 		return
 	}
@@ -558,13 +568,6 @@ func (s *Server) Serve() {
 	// Take connection and extract identity information for the user from it.
 	s.identityContext, err = s.authHandlers.CreateIdentityContext(sconn)
 	if err != nil {
-		if s.userAgent != nil {
-			s.userAgent.Close()
-		}
-		s.targetConn.Close()
-		s.clientConn.Close()
-		s.serverConn.Close()
-
 		s.log.Errorf("Unable to create server connection: %v.", err)
 		return
 	}
@@ -578,17 +581,12 @@ func (s *Server) Serve() {
 		s.rejectChannel(chans, err.Error())
 		sconn.Close()
 
-		if s.userAgent != nil {
-			s.userAgent.Close()
-		}
-		s.targetConn.Close()
-		s.clientConn.Close()
-		s.serverConn.Close()
-
 		s.log.Errorf("Unable to create remote connection: %v", err)
 		return
 	}
 
+	succeeded = true
+
 	// The keep-alive loop will keep pinging the remote server and after it has
 	// missed a certain number of keep-alive requests it will cancel the
 	// closeContext which signals the server to shutdown.
@@ -646,12 +644,12 @@ func (s *Server) newRemoteClient(ctx context.Context, systemLogin string) (*trac
 	var signers []ssh.Signer
 	if s.agentlessSigner != nil {
 		signers = []ssh.Signer{s.agentlessSigner}
-	} else if s.userAgent != nil {
-		s, err := s.userAgent.Signers()
+	} else {
+		var err error
+		signers, err = s.userAgent.Signers()
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
-		signers = s
 	}
 	authMethod := ssh.PublicKeysCallback(signersWithSHA1Fallback(signers))
 
@@ -1140,19 +1138,24 @@ func (s *Server) dispatch(ctx context.Context, ch ssh.Channel, req *ssh.Request,
 }
 
 func (s *Server) handleAgentForward(ch ssh.Channel, req *ssh.Request, ctx *srv.ServerContext) error {
-	// TODO(capnspacehook): remove once SSH agent forwarding issue is fixed
-	if s.userAgent == nil {
-		return trace.BadParameter("SSH agent is not set")
-	}
-
 	// Check if the user's RBAC role allows agent forwarding.
 	err := s.authHandlers.CheckAgentForward(ctx)
 	if err != nil {
 		return trace.Wrap(err)
 	}
 
 	// Route authentication requests to the agent that was forwarded to the proxy.
-	err = agent.ForwardToAgent(ctx.RemoteClient.Client, s.userAgent)
+	// If no agent was forwarded to the proxy, create one now.
+	userAgent := s.userAgent
+	if userAgent == nil {
+		ctx.ConnectionContext.SetForwardAgent(true)
+		userAgent, err = ctx.StartAgentChannel()
+		if err != nil {
+			return trace.Wrap(err)
+		}
+	}
+
+	err = agent.ForwardToAgent(ctx.RemoteClient.Client, userAgent)
 	if err != nil {
 		return trace.Wrap(err)
 	}

lib/sshutils/ctx.go

@@ -132,9 +132,7 @@ func (a *agentChannel) Close() error {
 func (c *ConnectionContext) StartAgentChannel() (teleagent.Agent, error) {
 	// refuse to start an agent if forwardAgent has not yet been set.
 	if !c.GetForwardAgent() {
-		// TODO(capnspacehook): update SSH agent in forwarding SSH server
-		// when connecting to agentless nodes
-		return nil, trace.AccessDenied("agent forwarding required in proxy recording mode")
+		return nil, trace.AccessDenied("agent forwarding has not been requested")
 	}
 	// open a agent channel to client
 	ch, _, err := c.ServerConn.OpenChannel(AuthAgentRequest, nil)