[v11] Session Control + UI SSH Performance#18797
Merged
rosstimothy merged 5 commits intobranch/v11from Nov 28, 2022
Merged
Conversation
Session control logic existed within `HandleNewConn` of `srv/regular.Server`. This prevented any of it from being used by other components that also needed to enforce session control. All the logic from within `HandleNewConn` was refactored to a new `srv.SessionController` object which the `regular.Server` now uses to perform session control. There were a few additional changes needed to accomadate that session control now exists outside the server and to make tests easier to write. Namely, altering `srv.ComputLockTargets` to not take a `Server` as a parameter and leveraging a clock within `services.SemaphoreLock`. This is step 2 in addressing #15167. Before the web apiserver can leverage the newly introduced proxy.Router and bypass making ssh connections to the proxy ssh server it needs to be able to perfrom session control.
Reduces latency creating ssh sessions via the web ui by: 1) No longer uses `TeleportClient.SSH` to establish a session 2) Reuses the user auth client for the web session to perform MFA ceremony 3) Ensures that connection attempts follow the flow outlined in RFD 93 The web api server now leverages the `proxy.Router` and `srv.SessionController` directly, instead of doing so indirectly via `TeleportClient.SSH`. Using the `TeleportClient` required an ssh connection to be established from the web api server to the proxy ssh server, which are in the same process. This added overhead can be avoided now that the routing logic and session control logic exists in a reusable component. To create an interactive session on the node once the connection is established, `client.NodeClient` is used. A new constructor was added to facilitate creating one and remove duplicated creation code and a `RunInteractiveShell` receiver method was added to allow callers outside of `lib/client` to spawn a session. `TerminalHandler.issueSessionMFACerts` used to check if per-session mfa was enabled and perform the mfa ceremony via the `client.ProxyClient` which was constructed with the `TeleportClient` established from connecting to the proxy ssh server. This would dial the Auth server under the hood directly and call `IsMFARequired` and do the ceremony if required. Each web session established via the web ui already established an auth client with the credentials of the logged in user. Again overhead is removed by leveraging the existing auth client and performing the mfa ceremony manually. Finally `TerminalHandler.makeClient` always attempted to perform the mfa ceremony prior to returning the `TeleportClient`. As outlined in [RFD 93](https://github.com/gravitational/teleport/blob/master/rfd/0093-offline-access.md), this causes additional latency and requires Auth connectivity to connect to nodes. The connection flow is now modified to attempt connection to the nodes first, and fall back to the mfa ceremony and reconnecting only if the node denies access. Partially addresses #15167
zmb3
approved these changes
Nov 28, 2022
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| h.log.Debugf("New terminal request for ns=%s, server=%s, login=%s, sid=%s, websid=%s.", | ||
| req.Namespace, req.Server, req.Login, req.SessionID, ctx.GetSessionID()) | ||
| req.Namespace, req.Server, req.Login, req.SessionID, sessionCtx.GetSessionID()) |
Check failure
Code scanning / CodeQL
Log entries created from user input
|
|
||
| h.log.Debugf("New terminal request for ns=%s, server=%s, login=%s, sid=%s, websid=%s.", | ||
| req.Namespace, req.Server, req.Login, req.SessionID, ctx.GetSessionID()) | ||
| req.Namespace, req.Server, req.Login, req.SessionID, sessionCtx.GetSessionID()) |
Check failure
Code scanning / CodeQL
Log entries created from user input
|
|
||
| h.log.Debugf("New terminal request for ns=%s, server=%s, login=%s, sid=%s, websid=%s.", | ||
| req.Namespace, req.Server, req.Login, req.SessionID, ctx.GetSessionID()) | ||
| req.Namespace, req.Server, req.Login, req.SessionID, sessionCtx.GetSessionID()) |
Check failure
Code scanning / CodeQL
Log entries created from user input
|
|
||
| h.log.Debugf("New terminal request for ns=%s, server=%s, login=%s, sid=%s, websid=%s.", | ||
| req.Namespace, req.Server, req.Login, req.SessionID, ctx.GetSessionID()) | ||
| req.Namespace, req.Server, req.Login, req.SessionID, sessionCtx.GetSessionID()) |
Check failure
Code scanning / CodeQL
Log entries created from user input
hatched
approved these changes
Nov 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports #18565 and #18656 to branch/v11