Skip to content

Move ssh session helper function to api/sshutils#59009

Closed
Joerger wants to merge 2 commits intomasterfrom
joerger/move-ssh-session-helpers
Closed

Move ssh session helper function to api/sshutils#59009
Joerger wants to merge 2 commits intomasterfrom
joerger/move-ssh-session-helpers

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Sep 11, 2025

In a follow up PR, I plan on providing extra session parameters via client.OpenChannel("session", sessionParams). Before making these changes with no relation to the api/observability/tracing/ssh package, I wanted to move these helper functions to a more ubiquitous location.

@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 backport/branch/v18 labels Sep 11, 2025
@github-actions github-actions bot requested a review from tcsc September 11, 2025 00:07
@Joerger Joerger removed the request for review from r0mant September 11, 2025 00:08
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be exposing the crimes from the tracessh package in a generic ssh helper package. Can we keep this internal to tracessh and extend tracessh with any additional receiver methods to achieve the desired behavior?

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Sep 16, 2025

I don't think we should be exposing the crimes from the tracessh package in a generic ssh helper package. Can we keep this internal to tracessh and extend tracessh with any additional receiver methods to achieve the desired behavior?

This isn't really tracing related at all - this is the session request callback logic added in #42123 to support the server->client "current-session-id@goteleport.com" request. It was only added in the tracing package for its proximity to the lowest level sshclient.NewSession call. I should have included this in the PR description for clarity.

exposing the crimes from the tracessh package

This portion is moreso covering a missing component of the x/crypto/ssh package in giving the consumer the ability to handle session requests on the crypto/ssh.Session. I think this commit might shed light on what I mean, as it cleans up the logic a bit to mirror the HandleChannelOpen logic offered by the crypto/ssh package for global channel requests, which it does not offer for session requests.

Let me know if you'd still just like this isolated to the tracing package.

@Joerger Joerger force-pushed the joerger/move-ssh-session-helpers branch from f82ec1c to b1c6aaf Compare September 16, 2025 17:37
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Sep 16, 2025

@rosstimothy I forgot to mention, the main motivation behind this is actually this change, which further needs to work around the limitations of the x/crypto/ssh package.

@Joerger Joerger closed this Sep 16, 2025
@rosstimothy
Copy link
Copy Markdown
Contributor

I think it would be more appropriate to use the tracessh.Client directly over an ssh.Client if possible, and add a new API to the tracessh.Client to support this use case. Most of our clients today use tracessh so that we get trace coverage on all requests so we should prefer that where possible.

If that's not possible I think it'd make more sense to leverage a copy of this closer to where you actually need it rather than exposing this in an SSH package in the API module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 backport/branch/v18 no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants