From f31290b0e9a62b271b43f629c54f6b65ffebf718 Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Thu, 18 Sep 2025 12:43:20 -0700 Subject: [PATCH] Capture x11 and agent forwarding requests in trace. (#59292) --- lib/client/session.go | 3 +-- lib/client/x11_session.go | 2 +- lib/srv/forward/sshserver.go | 18 +++++++++--------- lib/srv/regular/sshserver_test.go | 11 ++++++----- lib/sshagent/client.go | 15 +++++++++++++++ lib/sshutils/x11/forward.go | 6 ++++-- 6 files changed, 36 insertions(+), 19 deletions(-) diff --git a/lib/client/session.go b/lib/client/session.go index 9edc9c5b7ab..700e13eb263 100644 --- a/lib/client/session.go +++ b/lib/client/session.go @@ -36,7 +36,6 @@ import ( "github.com/gravitational/trace" oteltrace "go.opentelemetry.io/otel/trace" "golang.org/x/crypto/ssh" - "golang.org/x/crypto/ssh/agent" "github.com/gravitational/teleport" tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" @@ -245,7 +244,7 @@ func (ns *NodeSession) createServerSession(ctx context.Context, chanReqCallback if err != nil { return nil, trace.Wrap(err) } - err = agent.RequestAgentForwarding(sess.Session) + err = sshagent.RequestAgentForwarding(ctx, sess) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/x11_session.go b/lib/client/x11_session.go index 7d83d867ab2..e6c23c3ac77 100644 --- a/lib/client/x11_session.go +++ b/lib/client/x11_session.go @@ -62,7 +62,7 @@ func (ns *NodeSession) handleX11Forwarding(ctx context.Context, sess *tracessh.S return trace.Wrap(err) } - if err := x11.RequestForwarding(sess.Session, ns.spoofedXAuthEntry); err != nil { + if err := x11.RequestForwarding(ctx, sess, ns.spoofedXAuthEntry); err != nil { // Notify the user that x11 forwarding request failed regardless of debug level fmt.Fprintln(os.Stderr, "X11 forwarding request failed") slog.DebugContext(ctx, "X11 forwarding request error", "err", err) diff --git a/lib/srv/forward/sshserver.go b/lib/srv/forward/sshserver.go index ed260527b0a..c865078773c 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -1260,7 +1260,7 @@ func (s *Server) dispatch(ctx context.Context, ch ssh.Channel, req *ssh.Request, // to maintain interoperability with OpenSSH, agent forwarding requests // should never fail, all errors should be logged and we should continue // processing requests. - err := s.handleAgentForward(ch, req, scx) + err := s.handleAgentForward(ctx, ch, req, scx) if err != nil { scx.Logger.DebugContext(ctx, "failure forwarding agent", "error", err) } @@ -1295,7 +1295,7 @@ func (s *Server) dispatch(ctx context.Context, ch ssh.Channel, req *ssh.Request, // to maintain interoperability with OpenSSH, agent forwarding requests // should never fail, all errors should be logged and we should continue // processing requests. - err := s.handleAgentForward(ch, req, scx) + err := s.handleAgentForward(ctx, ch, req, scx) if err != nil { scx.Logger.DebugContext(ctx, "failure forwarding agent", "error", err) } @@ -1313,9 +1313,9 @@ 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 { +func (s *Server) handleAgentForward(ctx context.Context, ch ssh.Channel, req *ssh.Request, scx *srv.ServerContext) error { // Check if the user's RBAC role allows agent forwarding. - err := s.authHandlers.CheckAgentForward(ctx) + err := s.authHandlers.CheckAgentForward(scx) if err != nil { return trace.Wrap(err) } @@ -1324,21 +1324,21 @@ func (s *Server) handleAgentForward(ch ssh.Channel, req *ssh.Request, ctx *srv.S // 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() + scx.ConnectionContext.SetForwardAgent(true) + userAgent, err = scx.StartAgentChannel() if err != nil { return trace.Wrap(err) } - ctx.AddCloser(userAgent) + scx.AddCloser(userAgent) } - err = agent.ForwardToAgent(ctx.RemoteClient.Client, userAgent) + err = agent.ForwardToAgent(scx.RemoteClient.Client, userAgent) if err != nil { return trace.Wrap(err) } // Make an "auth-agent-req@openssh.com" request on the remote host. - err = agent.RequestAgentForwarding(ctx.RemoteSession.Session) + err = sshagent.RequestAgentForwarding(ctx, scx.RemoteSession) if err != nil { return trace.Wrap(err) } diff --git a/lib/srv/regular/sshserver_test.go b/lib/srv/regular/sshserver_test.go index 95f3545b71b..a01cf9be73f 100644 --- a/lib/srv/regular/sshserver_test.go +++ b/lib/srv/regular/sshserver_test.go @@ -77,6 +77,7 @@ import ( "github.com/gravitational/teleport/lib/services/readonly" sess "github.com/gravitational/teleport/lib/session" "github.com/gravitational/teleport/lib/srv" + "github.com/gravitational/teleport/lib/sshagent" "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/sshutils/x11" "github.com/gravitational/teleport/lib/utils" @@ -506,13 +507,13 @@ func TestSessionAuditLog(t *testing.T) { sessionID := startEvent.SessionID // Request agent forwarding, no individual event emitted. - err = agent.RequestAgentForwarding(se.Session) + err = sshagent.RequestAgentForwarding(ctx, se) require.NoError(t, err) // Request x11 forwarding, event should be emitted immediately. clientXAuthEntry, err := x11.NewFakeXAuthEntry(x11.Display{}) require.NoError(t, err) - err = x11.RequestForwarding(se.Session, clientXAuthEntry) + err = x11.RequestForwarding(ctx, se, clientXAuthEntry) require.NoError(t, err) x11Event := nextEvent() @@ -1153,7 +1154,7 @@ func TestAgentForwardPermission(t *testing.T) { // to interoperate with OpenSSH, requests for agent forwarding always succeed. // however that does not mean the users agent will actually be forwarded. - require.NoError(t, agent.RequestAgentForwarding(se.Session)) + require.NoError(t, sshagent.RequestAgentForwarding(ctx, se)) // the output of env, we should not see SSH_AUTH_SOCK in the output output, err := se.Output(ctx, "env") @@ -1262,7 +1263,7 @@ func TestAgentForward(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { se.Close() }) - err = agent.RequestAgentForwarding(se.Session) + err = sshagent.RequestAgentForwarding(ctx, se) require.NoError(t, err) // prepare to send virtual "keyboard input" into the shell: @@ -1452,7 +1453,7 @@ func x11EchoSession(ctx context.Context, t *testing.T, clt *tracessh.Client) x11 // Client requests x11 forwarding for the server session. clientXAuthEntry, err := x11.NewFakeXAuthEntry(x11.Display{}) require.NoError(t, err) - err = x11.RequestForwarding(se.Session, clientXAuthEntry) + err = x11.RequestForwarding(ctx, se, clientXAuthEntry) require.NoError(t, err) // prepare to send virtual "keyboard input" into the shell: diff --git a/lib/sshagent/client.go b/lib/sshagent/client.go index 7224c878be3..bfb15d62f28 100644 --- a/lib/sshagent/client.go +++ b/lib/sshagent/client.go @@ -25,6 +25,8 @@ import ( "github.com/gravitational/trace" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" + + tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" ) // Client extends the [agent.ExtendedAgent] interface with an [io.Closer]. @@ -134,3 +136,16 @@ func ServeChannelRequests(ctx context.Context, client *ssh.Client, getForwardAge }() return nil } + +// RequestAgentForwarding sets up agent forwarding for the session. +// ForwardToAgent or ForwardToRemote should be called to route +// the authentication requests. +func RequestAgentForwarding(ctx context.Context, session *tracessh.Session) error { + ok, err := session.SendRequest(ctx, "auth-agent-req@openssh.com", true, nil) + if err != nil { + return trace.Wrap(err) + } else if !ok { + return trace.Errorf("agent forwarding request denied") + } + return nil +} diff --git a/lib/sshutils/x11/forward.go b/lib/sshutils/x11/forward.go index 0a67a01e418..1ced7708914 100644 --- a/lib/sshutils/x11/forward.go +++ b/lib/sshutils/x11/forward.go @@ -25,6 +25,8 @@ import ( "github.com/gravitational/trace" "golang.org/x/crypto/ssh" + + tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" ) const ( @@ -55,14 +57,14 @@ type ForwardRequestPayload struct { // authProto and authCookie are required to set up authentication with the Server. screenNumber is used // by the server to determine which screen should be connected to for X11 forwarding. singleConnection is // an optional argument to request X11 forwarding for a single connection. -func RequestForwarding(sess *ssh.Session, xauthEntry *XAuthEntry) error { +func RequestForwarding(ctx context.Context, sess *tracessh.Session, xauthEntry *XAuthEntry) error { payload := ForwardRequestPayload{ AuthProtocol: xauthEntry.Proto, AuthCookie: xauthEntry.Cookie, ScreenNumber: uint32(xauthEntry.Display.ScreenNumber), } - ok, err := sess.SendRequest(ForwardRequest, true, ssh.Marshal(payload)) + ok, err := sess.SendRequest(ctx, ForwardRequest, true, ssh.Marshal(payload)) if err != nil { return trace.Wrap(err) } else if !ok {