-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Read the bearer token over websocket endpoints instead of query parameter #37520
Conversation
9a95546
to
f3581b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the changes made here have broken TestSiteNodeConnectInvalidSessionID
The issue is previously errors returned were handled by an error writer, however this doesnt work with websockets, so I've changed the test to read the error over the websocket |
f3581b6
to
3981eb7
Compare
65f7027
to
046907c
Compare
|
||
// TestTerminal takes 11 seconds to run. | ||
"TestTerminal", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this from the list and ask for an admin to bypass instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Like force the pr to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An admin can leave a comment on this PR which allows the bot to bypass a subset of tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that not cause problems for future prs that run the TestTerminal test in the flaky check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but we've got several slow tests and this list hasn't grown to include many of them. If we added all of our slow tests to this list there would be little incentive to taking the time to try and make them faster.
Just confirming that I tested again this morning and the SSH issue is fixed. |
… parameter #37520 (#37540) * 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
…digm as it was forgotten in #37520
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.
* 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]>
…eter (#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 <[email protected]>
* 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]>
* 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 <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]> --------- Co-authored-by: Alex McGrath <[email protected]> Co-authored-by: Zac Bergquist <[email protected]>
…eter (#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 <[email protected]>
* 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]>
…dpoints These were missed in the initial PR: #37520
…eter (#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 <[email protected]>
* 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]>
* 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 <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Zac Bergquist <[email protected]> * polishing off merge errors * fixes Path in makeTerminal to reflect new /ws suffix --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: Alex McGrath <[email protected]>
… IronRDP master commit hash. (#37962) * Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists
* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists * Adds the KeyboardHandler class that manages the keyboard events, including new functionality to handle withholding the Windows/Alt keys to prevent such keys from "sticking" when the user switches between windows. This technique works to stop the sticking, though there is still an edge case where the user cmd/alt-tabs super quickly, in a manner that the browser sees both the down and up events for the Windows key, despite the user's intention being to change windows. In the case of the Windows key, this causes the start menu to pop open, and the user must press the Windows key again to close it. This is not a showstopper, but it is a bit annoying. A similar problem would occur with the Alt key, where such behavior would cause the current window (on the Windows machine) to focus on the menu bar. The next commit will be to add a timeout mechanism for the relevant keyup events in order to mitigate this edge case. * Adds a delayUp cache for delaying the up event of the sticky windows key. This is to prevent the edge case where the user presses cmd/alt-tab really quickly in a manner that causes the down + up events to be registered in spite of the user's intention to be to change out of the window. * Refactoring KeyboardHandler * Convert caches into Maps * Moves the rest of keyboard functionality into KeyboardHandler, rearranges methods, adds an onUnmount for clearing all the intervals on unmount * Moves KeyboardHandler to its own file * simplifies withholding logic by including a Withholder class with a single array that manages all the withheld keystrokes * Makes useEffect symmetrical * Rename onUnmount to dispose * sp * Adds unit tests for Withholder * Splitting Withholder out into its own file * nits * CR * make fix-license
* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists * Adds the KeyboardHandler class that manages the keyboard events, including new functionality to handle withholding the Windows/Alt keys to prevent such keys from "sticking" when the user switches between windows. This technique works to stop the sticking, though there is still an edge case where the user cmd/alt-tabs super quickly, in a manner that the browser sees both the down and up events for the Windows key, despite the user's intention being to change windows. In the case of the Windows key, this causes the start menu to pop open, and the user must press the Windows key again to close it. This is not a showstopper, but it is a bit annoying. A similar problem would occur with the Alt key, where such behavior would cause the current window (on the Windows machine) to focus on the menu bar. The next commit will be to add a timeout mechanism for the relevant keyup events in order to mitigate this edge case. * Adds a delayUp cache for delaying the up event of the sticky windows key. This is to prevent the edge case where the user presses cmd/alt-tab really quickly in a manner that causes the down + up events to be registered in spite of the user's intention to be to change out of the window. * Refactoring KeyboardHandler * Convert caches into Maps * Moves the rest of keyboard functionality into KeyboardHandler, rearranges methods, adds an onUnmount for clearing all the intervals on unmount * Moves KeyboardHandler to its own file * simplifies withholding logic by including a Withholder class with a single array that manages all the withheld keystrokes * Makes useEffect symmetrical * Rename onUnmount to dispose * sp * Adds unit tests for Withholder * Splitting Withholder out into its own file * nits * CR * make fix-license
… IronRDP master commit hash. (#37962) * Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists
* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists * Adds the KeyboardHandler class that manages the keyboard events, including new functionality to handle withholding the Windows/Alt keys to prevent such keys from "sticking" when the user switches between windows. This technique works to stop the sticking, though there is still an edge case where the user cmd/alt-tabs super quickly, in a manner that the browser sees both the down and up events for the Windows key, despite the user's intention being to change windows. In the case of the Windows key, this causes the start menu to pop open, and the user must press the Windows key again to close it. This is not a showstopper, but it is a bit annoying. A similar problem would occur with the Alt key, where such behavior would cause the current window (on the Windows machine) to focus on the menu bar. The next commit will be to add a timeout mechanism for the relevant keyup events in order to mitigate this edge case. * Adds a delayUp cache for delaying the up event of the sticky windows key. This is to prevent the edge case where the user presses cmd/alt-tab really quickly in a manner that causes the down + up events to be registered in spite of the user's intention to be to change out of the window. * Refactoring KeyboardHandler * Convert caches into Maps * Moves the rest of keyboard functionality into KeyboardHandler, rearranges methods, adds an onUnmount for clearing all the intervals on unmount * Moves KeyboardHandler to its own file * simplifies withholding logic by including a Withholder class with a single array that manages all the withheld keystrokes * Makes useEffect symmetrical * Rename onUnmount to dispose * sp * Adds unit tests for Withholder * Splitting Withholder out into its own file * nits * CR * make fix-license
* Bump the rust group with 2 updates (#37739) Bumps the rust group with 2 updates: [tokio](https://github.com/tokio-rs/tokio) and [time](https://github.com/time-rs/time). Updates `tokio` from 1.35.1 to 1.36.0 - [Release notes](https://github.com/tokio-rs/tokio/releases) - [Commits](tokio-rs/tokio@tokio-1.35.1...tokio-1.36.0) Updates `time` from 0.3.31 to 0.3.34 - [Release notes](https://github.com/time-rs/time/releases) - [Changelog](https://github.com/time-rs/time/blob/main/CHANGELOG.md) - [Commits](time-rs/time@v0.3.31...v0.3.34) --- updated-dependencies: - dependency-name: tokio dependency-type: direct:production update-type: version-update:semver-minor dependency-group: rust - dependency-name: time dependency-type: direct:production update-type: version-update:semver-patch dependency-group: rust ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. (#37962) * Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists * Fix sticky windows key (#38198) * Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. * cleaning up disconnect result * Pipes disconnect messages through to the client. This required a change to the state management for DesktopSession. State is still quite complex to get right here, but this is a step in the right direction. * Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520 * prettier formatting * adding clarifying comments * Further refactoring to make DesktopSession's state machine more comprehensible * Remove unnecessary items from dependency lists * Adds the KeyboardHandler class that manages the keyboard events, including new functionality to handle withholding the Windows/Alt keys to prevent such keys from "sticking" when the user switches between windows. This technique works to stop the sticking, though there is still an edge case where the user cmd/alt-tabs super quickly, in a manner that the browser sees both the down and up events for the Windows key, despite the user's intention being to change windows. In the case of the Windows key, this causes the start menu to pop open, and the user must press the Windows key again to close it. This is not a showstopper, but it is a bit annoying. A similar problem would occur with the Alt key, where such behavior would cause the current window (on the Windows machine) to focus on the menu bar. The next commit will be to add a timeout mechanism for the relevant keyup events in order to mitigate this edge case. * Adds a delayUp cache for delaying the up event of the sticky windows key. This is to prevent the edge case where the user presses cmd/alt-tab really quickly in a manner that causes the down + up events to be registered in spite of the user's intention to be to change out of the window. * Refactoring KeyboardHandler * Convert caches into Maps * Moves the rest of keyboard functionality into KeyboardHandler, rearranges methods, adds an onUnmount for clearing all the intervals on unmount * Moves KeyboardHandler to its own file * simplifies withholding logic by including a Withholder class with a single array that manages all the withheld keystrokes * Makes useEffect symmetrical * Rename onUnmount to dispose * sp * Adds unit tests for Withholder * Splitting Withholder out into its own file * nits * CR * make fix-license --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dpoints (#38130) * Updates the Path variables in tests to use the new-style websocket endpoints These were missed in the initial PR: #37520 * send auth requests over websocket in updated tests * Fix web shutdown integraiton test --------- Co-authored-by: Alex McGrath <[email protected]> Co-authored-by: Steam Deck User <[email protected]>
…dpoints These were missed in the initial PR: #37520
…dpoints These were missed in the initial PR: #37520
Original PR here: gravitational/teleport-private#1201
Fixes gravitational/teleport-private#637
Changelog: Read bearer token via websocket on websocket endpoints