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

x/crypto/ssh: session.Close() should unblock session.Wait() #21699

Open
nhooyr opened this Issue Aug 30, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@nhooyr
Copy link
Contributor

nhooyr commented Aug 30, 2017

The session.Close only sends the channel close message to the server. It's not until the server responds with a channel close message that session.Wait() returns because of the reqs channel being closed. session.Wait() should return immediately after session.Close().

See #21423 for previous discussion.

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Sep 4, 2017

can you provide more details under what circumstances the Close takes longer to process than the roundtrip that the RFC prescribes for channel close?

I don't think we can trigger the channel close mechanics without receiving an ack from the remote side, as we wouldn't know where to deliver any messages, so it would lead to dropped data.

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Sep 11, 2017

ping.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Sep 12, 2017

I don't follow.

My issue is that a poorly implemented SSH server could potentially take forever to respond to a channel close which would cause session.Wait() to deadlock.

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Sep 12, 2017

the channel mechanism is not really suited for error handling/timeout, since both sides have to be in agreement on what to do with the channel. If the remote end can't do that, the proper course of action is shutdown the entire connection.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Sep 12, 2017

That's fine with me, if the server doesn't respond in a reasonable amount of time to the channel close, we should just close the connection.

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Sep 13, 2017

have you seen this problem in practice?

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Sep 13, 2017

Nope.

@hanwen

This comment has been minimized.

Copy link
Contributor

hanwen commented Sep 18, 2017

you can setup a keepalive. One does this by sending periodic requests (global or channel) with wantReply=true, and closing the connection if replies take too long.

Maybe the SSH client should provide this out of the box, though?

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

nhooyr commented Oct 21, 2017

Nah, I think we're talking about different things. Even with SSH Keep Alive, a poorly implemented ssh server could take forever to respond to a channel close, which would cause session.Wait() to deadlock.

It could handle other requests and respond to keep alive fine, just not respond to the channel close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.