x/crypto/ssh: ssh connections and channels should be cancelable

[glycerine]

When using x/crypto/ssh in a long-running program for communicating with many nodes that may come and go, it is necessary to be able to shutdown started SSH connections and channels cleanly. Currently there are many goroutines leaked.

For example, https://github.com/golang/crypto/blob/master/ssh/connection.go#L79 the DiscardRequests function is launched in its own go routine for each new connection, and there is no way, currently, to shut it down. It thus leaks memory.

Moreover in order to make the internals of the (brilliant, fabulous) ssh library cancellable, each instance of a raw send and receive will need to be put with a select statement that includes a cancellation signal. I suggest implenting cancellation by adding context.CancelFunc and context.Context to the Config structure.

I've made these changes in a branch, here https://github.com/glycerine/crypto/tree/cancelable

These likely are not perfect or to anyone else's taste exactly, but feel free to mold to taste.

@hanwen this may be of interest.

[ianlancetaylor]

CC @hanwen @agl

[glycerine]

In addition to the branch cited above, my latest working copy is here (https://github.com/glycerine/xcryptossh). Caveat: be aware that this is my current working branch, and as I work on it, it may or may not be build-able. Nonetheless, it is a better and later version (no races, no known leaks, context.Context threaded through the API so this is a non-backcompatible version). If you wish to pull from a repo, the glycerine/xcryptossh one should be preferred over the glycerine/crypto in general. Be aware that it may have debugging printfs in.

[nhooyr]

The example you cite is not a leaked goroutine because the reqs channel will be closed when the connection closes.

I think we should have deadlines on channel receives (not go channels).

As of now, each new session will call the go session.wait() function when the remote remote command/shell is run. This is to provide a exit status via the public session.Wait() function. But my issue is that even if I call session.Close() (whose sole purpose is to close the underlying channel), it's possible for the remote server to never respond to the channel being closed, causing the goroutine that called session.Wait() and the goroutine for go session.wait() to deadlock. Its definitely very unlikely, but possible.

Another possible issue is with channel.SendRequest, it could block if wantReply is true but the server decides to never send back a reply. Unlikely, but possible.

My two examples are special because the server could handle other channels fine, so TCP keep alive and any other lower level timeout would not work. Waiting for a channel response from the server should have deadlines to prevent blockages like this.

[glycerine]

update: The idle timeouts work quite well. They are very solid. I returned briefly to an experiment with write deadlines, with the speculative aim of implementing net.Conn. Read deadlines were quickly done and worked fine. However, on the write side, I could not get write deadlines to work. It appears that SSH will happily Write() to an ssh.Channel and return nil error, even if there is never any corresponding Read() on the other end.

If someone smarter than me wants to try their hand at getting write deadlines to work, I invite you to try :) See the branch write_deadlines. https://github.com/glycerine/xcryptossh/tree/write_deadlines

In particular, run the TestSimpleWriteDeadline test in timeout_test.go, e.g.

$ go test -v -timeout 120m -race -run WriteDeadline

[glycerine]

@nhooyr On master of https://github.com/glycerine/xcryptossh/ I shipped SetReadDeadline() as well as SetIdleTimeout(). Give them a try and see if they meet your needs. update: As I don't use sessions much, if you have tests for missing functionality, they would be welcome.

[hanwen]

"It appears that SSH will happily Write() to an ssh.Channel and return nil error, even if there is never any corresponding Read() on the other end."

Did you expect anything else? In order to know that something is reading on the other end, you'd have to do a network round trip, which is expensive.

go SSH has a 2MB buffer (https://go.googlesource.com/crypto/+/81e90905daefcd6fd217b62423c0908922eadb30/ssh/channel.go#23) similar to OpenSSH.

[hanwen]

I don't fully understand the bug.

If a ssh.Channel is closed, all the associated goroutines should exit. If a ssh.Conn is closed, all associated goroutines should exit too. If you find cases where this is not happen, please file bug reports about them.

Similar on close of the channel or connection, all reads/writes will be interrupted, please file bug reports if you find cases where this does not happen.

About cancelation, adding Context throughout will be an invasive API change, which I would rather combine with other API changes, and the net effect would be that the package calls conn.Close() or channel.Close() on timeout. You might as well do this in the piece of code that has access to the incoming context.

[hanwen]

" if I call session.Close() (whose sole purpose is to close the underlying channel), it's possible for the remote server to never respond to the channel being closed"

the channel close should cause session.Wait to return. Can you file a bug if this is not the case?

[nhooyr]

@glycerine Nice work but it doesn't fit my needs. What I think needs to happen is that every single read from the channel should have a deadline with it so that it does not block forever, if the deadline expires before the read finishes, then the entire connection should be closed. I don't think a idle timeout is necessary if we go with what I described.

@hanwen filed #21699

[glycerine]

every single read from the channel should have a deadline with it so that it does not block forever

exactly what SetIdleTimeout() does

if the deadline expires before the read finishes, then the entire connection should be closed.

trivial to add in your application logic when you receive a timeout error from a Channel read.

[nhooyr]

SetIdleTimeout() doesn't do that because it resets on writes.

[glycerine]

SetIdleTimeout() doesn't do that because it resets on writes.

So if a write succeeds but a read doesn't, then that should be a timeout... hmm. maybe. after all, write success (today I learned) doesn't really mean anything.... thinking...