[vnet] fix: close proxied channel only after data and requests are complete #56020
+200
−58
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.
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 sendSSH_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 likessh [email protected] true
to return a non-zero exit code because the channel was closed before theexit_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 bygolang.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.