Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix web terminal ssh session creation race #52408

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GavinFrazar
Copy link
Contributor

@GavinFrazar GavinFrazar commented Feb 22, 2025

This fixes a data race I observed in an unrelated v16 backport test run: https://github.com/gravitational/teleport/actions/runs/13466661993/job/37634639889?pr=52403

==================
WARNING: DATA RACE
Read at 0x00c0209469f8 by goroutine 37792:
  github.com/gravitational/teleport/lib/web/terminal.(*Stream).Close()
      /__w/teleport/teleport/lib/web/terminal/terminal.go:595 +0xa4
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).Close.func1()
      /__w/teleport/teleport/lib/web/terminal.go:423 +0x95
  sync.(*Once).doSlow()
      /opt/go/src/sync/once.go:76 +0xe1
  sync.(*Once).Do()
      /opt/go/src/sync/once.go:67 +0x44
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).Close()
      /__w/teleport/teleport/lib/web/terminal.go:418 +0xe5
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).handler.func2()
      /__w/teleport/teleport/lib/web/terminal.go:465 +0x195
  github.com/gorilla/websocket.(*Conn).advanceFrame()
      /go/pkg/mod/github.com/gorilla/[email protected]/conn.go:991 +0x167d
  github.com/gorilla/websocket.(*Conn).NextReader()
      /go/pkg/mod/github.com/gorilla/[email protected]/conn.go:1034 +0x290
  github.com/gorilla/websocket.(*Conn).ReadMessage()
      /go/pkg/mod/github.com/gorilla/[email protected]/conn.go:1120 +0x2e
  github.com/gravitational/teleport/lib/web/terminal.(*WSStream).processMessages()
      /__w/teleport/teleport/lib/web/terminal/terminal.go:146 +0x1c7
  github.com/gravitational/teleport/lib/web/terminal.NewWStream.gowrap1()
      /__w/teleport/teleport/lib/web/terminal/terminal.go:447 +0x4f

Previous write at 0x00c0209469f8 by goroutine 37109:
  github.com/gravitational/teleport/lib/web/terminal.(*Stream).SessionCreated()
      /__w/teleport/teleport/lib/web/terminal/terminal.go:590 +0xe8
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).makeClient.func1()
      /__w/teleport/teleport/lib/web/terminal.go:531 +0x5b
  github.com/gravitational/teleport/lib/client.(*NodeClient).RunInteractiveShell.(*NodeSession).runShell.func2()
      /__w/teleport/teleport/lib/client/session.go:539 +0x212
  github.com/gravitational/teleport/lib/client.(*NodeSession).interactiveSession()
      /__w/teleport/teleport/lib/client/session.go:318 +0x3cf
  github.com/gravitational/teleport/lib/client.(*NodeSession).runShell()
      /__w/teleport/teleport/lib/client/session.go:522 +0xde4
  github.com/gravitational/teleport/lib/client.(*NodeClient).RunInteractiveShell()
      /__w/teleport/teleport/lib/client/client.go:403 +0xc53
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).streamTerminal()
      /__w/teleport/teleport/lib/web/terminal.go:867 +0x1009
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).handler()
      /__w/teleport/teleport/lib/web/terminal.go:478 +0xd8d
  github.com/gravitational/teleport/lib/web.(*TerminalHandler).ServeHTTP()
      /__w/teleport/teleport/lib/web/terminal.go:358 +0x524
  github.com/gravitational/teleport/lib/httplib.MakeTracingHandler.func1()
      /__w/teleport/teleport/lib/httplib/httplib.go:104 +0x281
  net/http.HandlerFunc.ServeHTTP()
      /opt/go/src/net/http/server.go:2220 +0x47
  go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*middleware).serveHTTP()
      /go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]/handler.go:212 +0x18bb
  go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.NewMiddleware.func1.1()
      /go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]/handler.go:73 +0x67
  net/http.HandlerFunc.ServeHTTP()
      /opt/go/src/net/http/server.go:2220 +0x47
  github.com/gravitational/teleport/lib/web.(*Handler).siteNodeConnect()
      /__w/teleport/teleport/lib/web/apiserver.go:3636 +0x1b12
  github.com/gravitational/teleport/lib/web.(*Handler).siteNodeConnect-fm()
      <autogenerated>:1 +0xe9
  github.com/gravitational/teleport/lib/web.(*Handler).bindDefaultEndpoints.(*Handler).WithClusterAuthWebSocket.func31()
      /__w/teleport/teleport/lib/web/apiserver.go:4499 +0x235
  github.com/gravitational/teleport/lib/web.(*Handler).bindDefaultEndpoints.(*Handler).WithClusterAuthWebSocket.MakeHandler.MakeHandlerWithErrorWriter.func206()
      /__w/teleport/teleport/lib/httplib/httplib.go:127 +0xa5
  github.com/julienschmidt/httprouter.(*Router).ServeHTTP()
      /go/pkg/mod/github.com/gravitational/[email protected]/router.go:399 +0xfcb
  github.com/gravitational/teleport/lib/web.(*Handler).ServeHTTP()
      <autogenerated>:1 +0x53
  github.com/gravitational/teleport/lib/web.NewHandler.func1.StripPrefix.1()
      /opt/go/src/net/http/server.go:2282 +0x471
  net/http.HandlerFunc.ServeHTTP()
      /opt/go/src/net/http/server.go:2220 +0x47
  github.com/gravitational/teleport/lib/web.NewHandler.func1()
      /__w/teleport/teleport/lib/web/apiserver.go:629 +0x1fc
  net/http.HandlerFunc.ServeHTTP()
      /opt/go/src/net/http/server.go:2220 +0x47
  github.com/julienschmidt/httprouter.(*Router).ServeHTTP()
      /go/pkg/mod/github.com/gravitational/[email protected]/router.go:460 +0xda1
  github.com/gravitational/teleport/lib/web.(*APIHandler).ServeHTTP()
      /__w/teleport/teleport/lib/web/apiserver.go:454 +0xae9
  net/http.serverHandler.ServeHTTP()
      /opt/go/src/net/http/server.go:3210 +0x2a1
  net/http.(*conn).serve()
      /opt/go/src/net/http/server.go:2092 +0x12a4
  net/http.(*Server).Serve.gowrap3()
      /opt/go/src/net/http/server.go:3360 +0x4f

@GavinFrazar GavinFrazar added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 backport/branch/v17 labels Feb 22, 2025
This makes it easier to verify that the sshSession is only accessed
after it is ready.
@GavinFrazar GavinFrazar marked this pull request as ready for review February 22, 2025 01:51
@github-actions github-actions bot requested review from r0mant and Tener February 22, 2025 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants