diff --git a/sftp/server.go b/sftp/server.go index d40cf3c4..355fa722 100644 --- a/sftp/server.go +++ b/sftp/server.go @@ -12,6 +12,7 @@ import ( "regexp" "strconv" "strings" + "sync" "emperror.dev/errors" "github.com/apex/log" @@ -125,6 +126,12 @@ func (c *SFTPServer) AcceptInbound(conn net.Conn, config *ssh.ServerConfig) erro defer sconn.Close() go ssh.DiscardRequests(reqs) + // Tracks every "Handle" goroutine spawned for this connection so that we can + // wait for them to finish before this function returns. Without this, channels + // could still be in use after "sconn" is closed by the caller's deferred call. + var wg sync.WaitGroup + defer wg.Wait() + for ch := range chans { // If not a session channel we just move on because it's not something we // know how to handle at this point. @@ -148,11 +155,26 @@ func (c *SFTPServer) AcceptInbound(conn net.Conn, config *ssh.ServerConfig) erro } }(requests) - if srv, ok := c.manager.Get(sconn.Permissions.Extensions["uuid"]); ok { + srv, ok := c.manager.Get(sconn.Permissions.Extensions["uuid"]) + if !ok { + // No matching server instance for this connection's UUID: nothing will + // ever consume this channel, so close it immediately to avoid leaking it. + _ = channel.Close() + continue + } + + // Handle each channel concurrently so a slow or stuck client on one channel + // (e.g. gvfs holding a session open) can't block the rest of the connection. + wg.Add(1) + go func(channel ssh.Channel) { + defer wg.Done() if err := c.Handle(sconn, srv, channel); err != nil { - return err + _ = channel.Close() + log.WithField("error", err). + WithField("ip", conn.RemoteAddr().String()). + Error("sftp: error handling channel") } - } + }(channel) } return nil } @@ -168,18 +190,26 @@ func (c *SFTPServer) Handle(conn *ssh.ServerConn, srv *server.Server, channel ss ctx := srv.Sftp().Context(handler.User()) rs := sftp.NewRequestServer(channel, handler.Handlers()) + // Signals the supervisor goroutine below to stop watching "ctx" once this + // function returns, so it doesn't linger until the server's protected-state + // context is eventually cancelled (which may be much later, or never). + done := make(chan struct{}) + defer close(done) + go func() { select { case <-ctx.Done(): srv.Log().WithField("user", conn.User()).Warn("sftp: terminating active session") _ = rs.Close() + case <-done: } }() - if err := rs.Serve(); err == io.EOF { - _ = rs.Close() + err = rs.Serve() + _ = rs.Close() + if err != nil && !errors.Is(err, io.EOF) { + return err } - return nil } @@ -258,4 +288,4 @@ func (c *SFTPServer) makeCredentialsRequest(conn ssh.ConnMetadata, t remote.Sftp // PrivateKeyPath returns the path the host private key for this server instance. func (c *SFTPServer) PrivateKeyPath() string { return path.Join(c.BasePath, ".sftp/id_ed25519") -} +} \ No newline at end of file