Skip to content

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

Merged
nklaassen merged 2 commits intomasterfrom
nklaassen/fix-vnet-ssh-proxy
Jun 24, 2025
Merged

[vnet] fix: close proxied channel only after data and requests are complete#56020
nklaassen merged 2 commits intomasterfrom
nklaassen/fix-vnet-ssh-proxy

Conversation

@nklaassen
Copy link
Copy Markdown
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 user@server.cluster 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
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from camscale June 24, 2025 17:00
@nklaassen nklaassen added this pull request to the merge queue Jun 24, 2025
Merged via the queue into master with commit b475077 Jun 24, 2025
39 checks passed
@nklaassen nklaassen deleted the nklaassen/fix-vnet-ssh-proxy branch June 24, 2025 20:42
nklaassen added a commit that referenced this pull request Jun 24, 2025
nklaassen added a commit that referenced this pull request Jun 24, 2025
github-merge-queue Bot pushed a commit that referenced this pull request Jul 22, 2025
* [v17][vnet] feat: TCP dial to SSH targets

Backport #55087 to branch/v17

* [v17][vnet] feat: accept incoming SSH connections

Backport #55155 to branch/v17

* [v17][vnet] feat: forward SSH connections to target

Backport #55156 to branch/v17

* [v17][vnet] feat: write VNet SSH keys to TELEPORT_HOME

Backport #55228 to branch/v17

* [v17][vnet] feat: write OpenSSH-compatible config file for VNet SSH

Backport #55239 to branch/v17

* [v17][vnet] fix: support <hostname>.<leaf-cluster> for VNet SSH

Backport #55688 to branch/v17

* fix BlockUntil API for backport

* [v17][vnet] feat: add "Connect with VNet" button to SSH servers

Backport #55623 to branch/v17

* [v17][vnet] feat: support VNet SSH when cluster name does not match proxy public addr

Backport #55655 to branch/v17

* [v17][vnet] feat: add SSH configuration diagnostic

Backport #55594 to branch/v17

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

* [v17][vnet] feat: show SSH status in VNet slider

Backport #55755 to branch/v17

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

* [v17][vnet] feat: support proxy recording mode with VNet SSH

Backport #55788 to branch/v17

* [v17][vnet] feat: support diag checks on windows

Backport #55856 to branch/v17

* [v17] fix: data race in vnet.TestSSH

Backport #55980 to branch/v17

* [v17][vnet] feat: mention SSH on VNet info page

Backport #55973 to branch/v17

* [v17][vnet] feat: serve DNS on IPv4

Backport #55539 to branch/v17

* [v17][vnet] fix: close proxied channel only after data and requests are complete

Backport #56020 to branch/v17

* [v17][vnet] feat: automatic SSH client configuration

Backport #55923 to branch/v17

* VNet docs: Provide clear instructions for getting debug logs (#56068)

* VNet diag notification: Do not show button to open report if there's no workspace selected (#56067)

* VNet diag report: Don't show button in notification if there's no workspace

* Replace deprecated MutableRefObject with RefObject

* Make openReport not depend on value of rootClusterUri

Otherwise the effect that uses setInterval re-runs whenever the user
switches to another workspace.

* [v17][vnet] feat: automatic SSH client configuration in Connect

Backport #55924 to branch/v17

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>
Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>

* [v17][vnet] fix: avoid empty host matchers in generated SSH config

Backport #56103 to branch/v17

* avoid t.Context() pre go1.24

* fix cspell lint

* [v17][docs] VNet SSH

Backport #56147 to branch/v17

* [v17][vnet] feat: SSH usage reporting

Backport #56537 to branch/v17

* [v17][vnet] fix: mask default IP route on windows

Backport #56957 to branch/v17

---------

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>
Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>
github-merge-queue Bot pushed a commit that referenced this pull request Jul 22, 2025
* [v18][vnet] feat: TCP dial to SSH targets

Backport #55087 to branch/v18

* [v18][vnet] feat: accept incoming SSH connections

Backport #55155 to branch/v18

* [v18][vnet] feat: forward SSH connections to target

Backport #55156 to branch/v18

* [v18][vnet] feat: write VNet SSH keys to TELEPORT_HOME

Backport #55228 to branch/v18

* [v18][vnet] feat: write OpenSSH-compatible config file for VNet SSH

Backport #55239 to branch/v18

* [v18][vnet] fix: support <hostname>.<leaf-cluster> for VNet SSH

Backport #55688 to branch/v18

* [v18][vnet] feat: add "Connect with VNet" button to SSH servers

Backport #55623 to branch/v18

* fix test in backport

* [v18][vnet] feat: support VNet SSH when cluster name does not match proxy public addr

Backport #55655 to branch/v18

* [v18][vnet] feat: add SSH configuration diagnostic

Backport #55594 to branch/v18

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

* [v18][vnet] feat: show SSH status in VNet slider

Backport #55755 to branch/v18

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>

* [v18][vnet] feat: support proxy recording mode with VNet SSH

Backport #55788 to branch/v18

* [v18][vnet] feat: support diag checks on windows

Backport #55856 to branch/v18

* [v18] fix: data race in vnet.TestSSH

Backport #55980 to branch/v18

* [v18][vnet] feat: mention SSH on VNet info page

Backport #55973 to branch/v18

* [v18][vnet] feat: serve DNS on IPv4

Backport #55539 to branch/v18

* [v18][vnet] fix: close proxied channel only after data and requests are complete

Backport #56020 to branch/v18

* [v18][vnet] feat: automatic SSH client configuration

Backport #55923 to branch/v18

* VNet diag notification: Do not show button to open report if there's no workspace selected (#56067)

* VNet diag report: Don't show button in notification if there's no workspace

* Replace deprecated MutableRefObject with RefObject

* Make openReport not depend on value of rootClusterUri

Otherwise the effect that uses setInterval re-runs whenever the user
switches to another workspace.

* [v18][vnet] feat: automatic SSH client configuration in Connect

Backport #55924 to branch/v18

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>
Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>

* [v18][vnet] fix: avoid empty host matchers in generated SSH config

Backport #56103 to branch/v18

* [v18][docs] VNet SSH

Backport #56147 to branch/v18

* [v18][docs] add VNet warnings

Backport #56601 to branch/v18

* [v18][vnet] feat: SSH usage reporting

Backport #56537 to branch/v18

* [v18][vnet] fix: mask default IP route on windows

Backport #56957 to branch/v18

---------

Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com>
Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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