Skip to content

[vnet] fix: close proxied channel only after data and requests are complete #56020

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

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

Conversation

nklaassen
Copy link
Contributor

This PR fixes two bugs in VNet's SSH proxy.

The first is that in the existing version utils.ProxyConn will close each proxied SSH channel as soon as it reaches EOF reading from either channel. First of all this is logically racy, if the source channel is closed the proxy may react to an EOF and close the target channel before the final channel request has been forwarded. It's also just incorrect, SSH clients or servers can send SSH_MSG_CHANNEL_EOF to signal that they are done sending data but they may still send channel requests that still need to be forwarded. This was causing simple commands like ssh [email protected] true to return a non-zero exit code because the channel was closed before the exit_status channel request was sent.

The fix for the first is to avoid closing the target channel until both the data and requests from the source channel have ended.

The second is that extended channel data of type SSH_EXTENDED_DATA_STDERR was not forwarded at all. This is necessary to forward stderr for SSH exec commands, it's the only extended data type defined in the spec or supported by golang.org/x/crypto/ssh. While changing the channel data forwarding logic here I thought it made sense to include the stderr data since it also effects the concurrency logic, otherwise the code you're reviewing here would be changing in a following PR.

I noticed both of these while trying Zed's remote SSH support via VNet SSH, on the bright side with these fixes in place it works fine.

@nklaassen nklaassen requested a review from espadolini June 24, 2025 01:33
@nklaassen nklaassen added backport-required no-changelog Indicates that a PR does not require a changelog entry vnet labels Jun 24, 2025
@nklaassen nklaassen requested a review from rosstimothy June 24, 2025 01:34
@github-actions github-actions bot requested a review from camscale June 24, 2025 01:34
@nklaassen nklaassen force-pushed the nklaassen/fix-vnet-ssh-proxy branch from 639ae8f to 67e93ae Compare June 24, 2025 01:43
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from camscale June 24, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-required no-changelog Indicates that a PR does not require a changelog entry size/md vnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants