Open drakkan opened 6 months ago
Change https://go.dev/cl/562756 mentions this issue: ssh: add deadlines support for channels
CC @golang/security
The handshake (version, kex, etc) can stall unless the underlying Conn is closed. I would love to see deadlines on channels, but it may make sense to have a general read/write timeout capability, otherwise we still need the conn.Close() to unblock when communicating with misbehaving peers.
but it may make sense to have a general read/write timeout capability
@hdm isn't that just Set(Read|Write)Deadline
on the underlying connection?
but it may make sense to have a general read/write timeout capability
@hdm isn't that just
Set(Read|Write)Deadline
on the underlying connection?
The challenge is that the deadlines need to be reset on each read/write and setting it at the connection level doesn't allow re-extension of the deadline by operation. For an example, with a long-lived session, you may want want a relatively short deadline for version exchange and kex, but then to re-extend it after each input line is passed in via the stdin reader.
I get your point though, if a deadline on the Conn is enough to get us into the session, then a Channel deadline could take over from there for most use cases.
I have a real life use-case in case that helps in any way...
I have an outbound ssh tunnel over which I serve an HTTP reverse proxy (just an httputil.ReverseProxy
). I recently discovered that my reverse proxy does not support WebSockets. After a painful debugging session I tracked down the problem:
httputil.ReverseProxy
uses http.Hijacker
when handling the upgrade to WebSocketshttp.Hijacker
relies on the http.ResponseWriter
's underlying net.Conn
's ability to support deadlines i.e. abort pending reads, etc.net.Conn
is an ssh.chanConn
(x/crypto/ssh) - which does not respect deadlines :)Here's a related github issue opened by someone else: https://github.com/golang/go/issues/67152
@adrianosela I rebased the linked CL, testing with a real use case would help us understand how useful this addition would be and speed up proposal approval. The main concern here is that deadlines work at a logical level and don't unblock the underlying net.Conn
if it hangs.
Please try the CL and provide your feedbacks. Thank you!
@drakkan code changes LGTM. I'll give your CL a go sometime in the next 2 weeks.
Hey @drakkan -- just gave your code a test and I can now use websockets over ssh channels 🚀 and I don't see any unintended side effects.
Would be good to run a performance test aside from functional testing.
Here's the exact diff of my local code vs what's in PS 9 of your CL...
TL;DR; I just
ChannelWithDeadlines
iface and moved the deadline-related methods to the Channel
iface definitionchanConn
in tcpip.go
so that it actually calls the Channel
methods for deadlines instead of unconditionally return errors as it does todaysession_test.go
to use Channel instead of ChannelWithDeadlines
@adrianosela thanks for your testing, much appreciated.
Changing the Channel
interface is a breaking change, we cannot do this before a v2.
Please give a try to PS 10 which add deadlines support also for chanConn
, we always return a ChannelWithDeadlines
so it should work without breaking backward compatibility.
I think it's unlikely that anyone implements their own Channel interface, but we can't know. In a v2 we should evaluate to convert Channel
to a struct.
Please keep testing and let us know if you encounter any issues. Thanks!
Comments left in PS 11 :) going forward let's use the CL for all communication @drakkan
If anyone happens to be waiting for this to be released - here's a band-aid solution you can try at your own risk :) https://github.com/adrianosela/deaconn
Proposal Details
Currently ssh channels does not support deadlines, so the only way to unblock reads/writes is to set a deadline on the underlying
net.Conn
, but this will affect all channels using the connection.Channels are typically blocked on reads while waiting for data and on writes while waiting for window capacity.
I propose adding deadlines to channels to fix these typically use cases, I don't think we can unblock reads/writes blocked on the underlying
net.Conn
.Proposed API
cc @golang/proposal-review