Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: x/crypto/ssh: add deadlines support for channels #65930

Open
drakkan opened this issue Feb 25, 2024 · 11 comments
Open

proposal: x/crypto/ssh: add deadlines support for channels #65930

drakkan opened this issue Feb 25, 2024 · 11 comments
Labels
Milestone

Comments

@drakkan
Copy link
Member

drakkan commented Feb 25, 2024

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

// ChannelWithDeadlines is a channel with deadlines support.
type ChannelWithDeadlines interface {
	Channel

	// SetDeadline sets the read and write deadlines associated with the
	// channel. It is equivalent to calling both SetReadDeadline and
	// SetWriteDeadline. Deadlines errors are not fatal, the Channel can be used
	// again after resetting the deadlines.
	SetDeadline(deadline time.Time) error

	// SetReadDeadline sets the deadline for future Read calls and unblock Read
	// calls waiting for data. A zero value for t means Read will not time out.
	SetReadDeadline(deadline time.Time) error

	// SetWriteDeadline sets the deadline for future Write calls and unblock
	// Write calls waiting for window capacity. A zero value for t means Write
	// will not time out.
	SetWriteDeadline(deadline time.Time) error
}

cc @golang/proposal-review

@gopherbot gopherbot added this to the Proposal milestone Feb 25, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/562756 mentions this issue: ssh: add deadlines support for channels

@ianlancetaylor
Copy link
Contributor

CC @golang/security

@hdm
Copy link

hdm commented Apr 8, 2024

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.

@espadolini
Copy link

espadolini commented Apr 8, 2024

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?

@hdm
Copy link

hdm commented Apr 8, 2024

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.

@adrianosela
Copy link

adrianosela commented Jun 11, 2024

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:

Here's a related github issue opened by someone else: #67152

@drakkan
Copy link
Member Author

drakkan commented Jun 16, 2024

@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!

@adrianosela
Copy link

@drakkan code changes LGTM. I'll give your CL a go sometime in the next 2 weeks.

@adrianosela
Copy link

adrianosela commented Jun 16, 2024

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

  • Got rid of the ChannelWithDeadlines iface and moved the deadline-related methods to the Channel iface definition
  • Modified chanConn in tcpip.go so that it actually calls the Channel methods for deadlines instead of unconditionally return errors as it does today
  • Changed the unit test for channel with deadlines in session_test.go to use Channel instead of ChannelWithDeadlines
Click here to see whole diff
14:14 $ git diff
diff --git a/ssh/channel.go b/ssh/channel.go
index 7a986a6..f5ef39f 100644
--- a/ssh/channel.go
+++ b/ssh/channel.go
@@ -77,11 +77,6 @@ type Channel interface {
        // safely be read and written from a different goroutine than
        // Read and Write respectively.
        Stderr() io.ReadWriter
-}
-
-// ChannelWithDeadlines is a channel with deadlines support.
-type ChannelWithDeadlines interface {
-       Channel

        // SetDeadline sets the read and write deadlines associated with the
        // channel. It is equivalent to calling both SetReadDeadline and
diff --git a/ssh/session_test.go b/ssh/session_test.go
index fb236d2..5270532 100644
--- a/ssh/session_test.go
+++ b/ssh/session_test.go
@@ -248,7 +248,7 @@ func TestChannelWithDeadlinesImplementation(t *testing.T) {
                t.Fatalf("unexpected error opening a session channel: %v", err)
        }

-       if _, ok := ch.(ChannelWithDeadlines); !ok {
+       if _, ok := ch.(Channel); !ok {
                t.Errorf("the returned channel does not support deadlines")
        }

diff --git a/ssh/tcpip.go b/ssh/tcpip.go
index ef5059a..c2859ee 100644
--- a/ssh/tcpip.go
+++ b/ssh/tcpip.go
@@ -497,13 +497,11 @@ func (t *chanConn) SetDeadline(deadline time.Time) error {
 // After the deadline, the error from Read will implement net.Error
 // with Timeout() == true.
 func (t *chanConn) SetReadDeadline(deadline time.Time) error {
-       // for compatibility with previous version,
-       // the error message contains "tcpChan"
-       return errors.New("ssh: tcpChan: deadline not supported")
+       return t.Channel.SetReadDeadline(deadline)
 }

 // SetWriteDeadline exists to satisfy the net.Conn interface
 // but is not implemented by this type.  It always returns an error.
 func (t *chanConn) SetWriteDeadline(deadline time.Time) error {
-       return errors.New("ssh: tcpChan: deadline not supported")
+       return t.Channel.SetWriteDeadline(deadline)
 }

@drakkan
Copy link
Member Author

drakkan commented Jun 17, 2024

@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!

@adrianosela
Copy link

adrianosela commented Jun 17, 2024

Comments left in PS 11 :) going forward let's use the CL for all communication @drakkan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants