go icon indicating copy to clipboard operation
go copied to clipboard

x/crypto/ssh: should avoid window adjustment on every Channel.Read

Open willmo opened this issue 3 years ago • 2 comments

What version of Go are you using (go version)?

$ go version
go version go1.18.4 linux/amd64

golang.org/x/crypto v0.0.0-20220829220503-c86fa9a7ed90

Does this issue reproduce with the latest release?

Yes, channel.go is unchanged since 2017.

What did you do?

Used tcpdump/Wireshark to observe packets on a long-lived SSH connection where the only application-level activity is sporadic small writes or a long stream of writes in the same direction, or other easy-to-understand patterns.

What did you expect to see?

TCP segments with data generally correspond to application-level writes, with only occasional window adjustments sent in the other direction (and rarely a rekeying, etc.).

What did you see instead?

A window adjustment is sent after every Channel.Read on the receiving end, which wastes bandwidth.

Solution

OpenSSH has logic for deferring window adjustments, which I think usually waits until about 5% of the window has been consumed. It seemingly hasn't changed since 2007, so I assume it works well enough for them.

I added similar logic to channel.adjustWindow and it seems to work as expected in local testing. If this sounds like a reasonable change, I'll see about signing the CLA and submitting a patch.

willmo avatar Dec 21 '22 08:12 willmo

@golang/security if this is a bug, it has a proposed fix.

dr2chase avatar Dec 22 '22 00:12 dr2chase

Seems like a reasonable optimization, I'd be happy to take a look if you'd like to send a CL.

rolandshoemaker avatar Dec 22 '22 19:12 rolandshoemaker

Change https://go.dev/cl/459915 mentions this issue: ssh: defer channel window adjustment

gopherbot avatar Dec 29 '22 02:12 gopherbot