Skip to content

[vnet] feat: add transparent SSH proxy#54525

Merged
nklaassen merged 6 commits intomasterfrom
nklaassen/vnet-ssh-proxy
May 20, 2025
Merged

[vnet] feat: add transparent SSH proxy#54525
nklaassen merged 6 commits intomasterfrom
nklaassen/vnet-ssh-proxy

Conversation

@nklaassen
Copy link
Copy Markdown
Contributor

Part of RFD 207: VNet SSH

This PR adds a transparent SSH proxy that takes an established incoming SSH connection from the client and an outgoing SSH connection to the target, and proxies all SSH requests, channels, and channel requests between the two.

@nklaassen nklaassen added no-changelog Indicates that a PR does not require a changelog entry vnet backport/branch/v16 backport/branch/v17 labels May 5, 2025
@github-actions github-actions Bot requested review from kopiczko and probakowski May 5, 2025 22:30
@rosstimothy rosstimothy requested a review from espadolini May 6, 2025 14:02
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
@nklaassen nklaassen requested a review from espadolini May 13, 2025 15:30
Comment thread lib/vnet/ssh_proxy.go
log = log.With("request_layer", "channel")
sendRequest := func(name string, wantReply bool, payload []byte) (bool, []byte, error) {
ok, err := targetChan.SendRequest(name, wantReply, payload)
// Replies to channel requests never have a payload.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worst protocol decision ever btw. 😔

Comment thread lib/vnet/ssh_proxy.go Outdated
Comment on lines +57 to +61
ctx, cancel := context.WithCancel(ctx)
defer cancel()
go func() {
defer wg.Done()
<-ctx.Done()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't the wg.Wait() below block forever (or rather until ctx is externally cancelled, which might be never) since nothing else will call cancel() and the waitgroup is waiting on this wg.Done()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah my bad and my test conveniently missed this 🤦 I think context.AfterFunc solves it by avoiding a goroutine

Comment thread lib/vnet/ssh_proxy.go Outdated
serverConn sshConn,
clientConn sshConn,
) {
closeConnections := func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we can split the responsibilities of this by mutually closing the connections after waiting on each one (as in (ssh.Conn).Wait), so each "side" only needs to close its own side on error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered it but I also realized that each connection or channel should really only be closed once and I think in the latest I got a somewhat cleaner solution here

@nklaassen nklaassen requested a review from espadolini May 15, 2025 18:34
Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

At which point it becomes easier to fork x/crypto to copy SSH packets rather than deal with the high level API? 🥴

edit: not gonna lie, this looks pretty easy

Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
Comment thread lib/vnet/ssh_proxy.go Outdated
@nklaassen
Copy link
Copy Markdown
Contributor Author

At which point it becomes easier to fork x/crypto to copy SSH packets rather than deal with the high level API? 🥴

edit: not gonna lie, this looks pretty easy

i'd rather not fork x/crypto/ssh if we don't have to, especially a fork that's not likely to ever be upstreamed. but let me know what you think of the latest changes 😅

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.

While this is a bit verbose as a result of the SSH API, I agree with Nic that we probably don't want to fork x/crypt/ssh just to make the code for this use case simpler.

@nklaassen nklaassen added this pull request to the merge queue May 20, 2025
Merged via the queue into master with commit 47e70fb May 20, 2025
40 checks passed
@nklaassen nklaassen deleted the nklaassen/vnet-ssh-proxy branch May 20, 2025 17:08
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@nklaassen See the table below for backport results.

Branch Result
branch/v16 Create PR
branch/v17 Create PR

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

Labels

backport/branch/v17 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