Updates `desktopPlaybackHandle` to new ws paradigm by ibeckermayer · Pull Request #37981 · gravitational/teleport

ibeckermayer · 2024-02-09T05:34:23Z

This was mistakenly left out of #37520.

This commit also refactors WithClusterAuthWebSocket slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode.

This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode.

zmb3 · 2024-02-09T06:45:41Z

 	}

-	upgrader := websocket.Upgrader{
-		ReadBufferSize:  4096,


Note that we will now be using buffers that are 75% smaller. Does this make sense for desktop recordings where we have larger messages?

My sense is that it can make a difference on the margins, but we'd need to do some sort of profiling to know the true impact.

lxea

LGTM

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

public-teleport-github-review-bot · 2024-02-09T18:32:23Z

@ibeckermayer See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Failed

* Updates `desktopPlaybackHandle` to new ws paradigm This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode. * Update lib/web/apiserver.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Read the bearer token over websocket endpoints instead of query parameter (#37520) * Read the bearer token over WS endpoints use the request context, not session Dont pass websocket by context lint resolve some comments Add TestWSAuthenticateRequest Close ws in handler deprecation notices, doc resolve comments resolve comments give a longer read/write deadline dont set write deadline, ws endpoints never did before and it breaks things convert frontend to use ws access token Resolove comments, move to using an explicit state fix ci reset read deadline prettier * update connectToHost * linter * read errors from websocket * missing /ws on ttyWsAddr and fix wrong onmessage * fix race in test * lint * skip TestTerminal as it takes 11 seconds to run * dont skip the test * resolve apiserver comments * Add an AuthenticatedWebSocket class * convert other clients to use AuthenticatedWebSocket * Converts `AuthenticatedWebSocket` into drop-in replacement for `WebSocket` (#37699) * Converts `AuthenticatedWebSocket` into drop-in replacement for `WebSocket` that automatically goes through Teleport's custom authentication process before facilitating any caller-defined communication. This also reverts previous-`WebSocket` users to their original state (sans the code for passing the bearer token in the query string), swapping in `AuthenticatedWebSocket` in place of `WebSocket`. * Create a single authnWsUpgrader with a comment justifying why we turn off CORS * recieving to receiving * resolve comments --------- Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> * Updates `desktopPlaybackHandle` to new ws paradigm (#37981) * Updates `desktopPlaybackHandle` to new ws paradigm This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode. * Update lib/web/apiserver.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Alex McGrath <alex.mcgrath@goteleport.com> Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Updates `desktopPlaybackHandle` to new ws paradigm This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode. * Update lib/web/apiserver.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Introduce a new streaming player API (#31754) This new API can be used to play back sessions of any type. The player accepts a session ID and a streamer, and provides the caller with an API for playback controls (speed, play/pause, seek, etc) as well as a channel that receives events with the proper timing delay applied. The design for this change is discussed in RFD 91. Updates #10578 Updates #10579 Updates gravitational/teleport-private#665 Updates gravitational/teleport-private#1024 * Convert the desktop sesssion player to the new player API (#34070) This makes a few changes to the player API to ensure that errors are correctly propagated. * Switch desktop playback to gorilla/websocket (#36405) We use gorilla/websocket throughout the app, but desktop playback leveraged x/net/websocket instead. Convert to gorilla so that we are consistent and use the same library everywhere websockets are used. * Read the bearer token over websocket endpoints instead of query parameter (#37520) * Read the bearer token over WS endpoints use the request context, not session Dont pass websocket by context lint resolve some comments Add TestWSAuthenticateRequest Close ws in handler deprecation notices, doc resolve comments resolve comments give a longer read/write deadline dont set write deadline, ws endpoints never did before and it breaks things convert frontend to use ws access token Resolove comments, move to using an explicit state fix ci reset read deadline prettier * update connectToHost * linter * read errors from websocket * missing /ws on ttyWsAddr and fix wrong onmessage * fix race in test * lint * skip TestTerminal as it takes 11 seconds to run * dont skip the test * resolve apiserver comments * Add an AuthenticatedWebSocket class * convert other clients to use AuthenticatedWebSocket * Converts `AuthenticatedWebSocket` into drop-in replacement for `WebSocket` (#37699) * Converts `AuthenticatedWebSocket` into drop-in replacement for `WebSocket` that automatically goes through Teleport's custom authentication process before facilitating any caller-defined communication. This also reverts previous-`WebSocket` users to their original state (sans the code for passing the bearer token in the query string), swapping in `AuthenticatedWebSocket` in place of `WebSocket`. * Create a single authnWsUpgrader with a comment justifying why we turn off CORS * recieving to receiving * resolve comments --------- Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> * Updates `desktopPlaybackHandle` to new ws paradigm (#37981) * Updates `desktopPlaybackHandle` to new ws paradigm This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode. * Update lib/web/apiserver.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * polishing off merge errors * fixes Path in makeTerminal to reflect new /ws suffix --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Alex McGrath <alex.mcgrath@goteleport.com>

ibeckermayer · 2024-02-15T02:31:02Z

Foregoing v13 backport because "[I]t's not directly exploitable. It would require a client side compromise to have any exposure from it" (@jentfoo), and a full backport would require many additional changes being backported that it was determined not to be worth our while.

ibeckermayer added backport/branch/v13 no-changelog Indicates that a PR does not require a changelog entry labels Feb 9, 2024

ibeckermayer requested review from jakule, lxea, r0mant and ryanclark February 9, 2024 05:34

github-actions Bot requested review from avatus, probakowski and rudream February 9, 2024 05:34

github-actions Bot added size/sm ui labels Feb 9, 2024

This was referenced Feb 9, 2024

[v14] backport #37520 and #37981 #37918

Closed

[v13] Read the bearer token over websocket endpoints instead of query parameter #37919

Closed

[v15] Read the bearer token over websocket endpoints instead of query parameter #37915

Closed

Updates desktopPlaybackHandle to new ws paradigm

f7ec4ed

This was mistakenly left out of #37520. This commit also refactors `WithClusterAuthWebSocket` slightly for easier comprehension, and updates the vite config to facilitate the new websocket endpoints in development mode.

ibeckermayer force-pushed the isaiah/fix-desktop-playback branch from 5e6a5d4 to f7ec4ed Compare February 9, 2024 06:13

ibeckermayer mentioned this pull request Feb 9, 2024

Accept bearer token over websocket for desktopplayback #37931

Closed

zmb3 reviewed Feb 9, 2024

View reviewed changes

lxea approved these changes Feb 9, 2024

View reviewed changes

zmb3 approved these changes Feb 9, 2024

View reviewed changes

Comment thread lib/web/apiserver.go Outdated

public-teleport-github-review-bot Bot removed request for avatus, jakule, probakowski, r0mant, rudream and ryanclark February 9, 2024 15:31

Update lib/web/apiserver.go

880d4a4

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

ibeckermayer enabled auto-merge February 9, 2024 17:41

probakowski mentioned this pull request Feb 9, 2024

Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. #37962

Merged

ibeckermayer added this pull request to the merge queue Feb 9, 2024

Merged via the queue into master with commit b8d8568 Feb 9, 2024

ibeckermayer deleted the isaiah/fix-desktop-playback branch February 9, 2024 18:30

ibeckermayer mentioned this pull request Feb 9, 2024

[v15] backport #37520 and #37981 #38032

Merged

ibeckermayer mentioned this pull request Feb 10, 2024

[v14] backports for new ws paradigm #38070

Merged

ibeckermayer removed the backport/branch/v13 label Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates `desktopPlaybackHandle` to new ws paradigm#37981

Updates `desktopPlaybackHandle` to new ws paradigm#37981
ibeckermayer merged 2 commits intomasterfrom
isaiah/fix-desktop-playback

ibeckermayer commented Feb 9, 2024

Uh oh!

zmb3 Feb 9, 2024

Uh oh!

ibeckermayer Feb 9, 2024

Uh oh!

lxea left a comment

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Feb 9, 2024

Uh oh!

ibeckermayer commented Feb 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ibeckermayer commented Feb 9, 2024

Uh oh!

zmb3 Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

lxea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Feb 9, 2024

Uh oh!

ibeckermayer commented Feb 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants