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

Proper way to close a connection? #23

Open
tyrbo opened this issue Mar 10, 2014 · 12 comments
Open

Proper way to close a connection? #23

tyrbo opened this issue Mar 10, 2014 · 12 comments

Comments

@tyrbo
Copy link

tyrbo commented Mar 10, 2014

I've noticed if I access the hijacked controller from directly (and not through JS, where I call socket.close), the resource will sit loading forever.

How can I go about properly closing a socket from the hijacked controller? I've tried calling the close method, but run into an IOError issue. Is the close method the proper way of closing a socket?

My code is very similar to what you used for your chat example: https://github.com/ngauthier/sock-chat/blob/master/app/controllers/chat_controller.rb

@ngauthier
Copy link
Owner

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

Yep. When testing with a Puma server, Puma would end up dying from an uncaught IOError when I closed it.
Rescuing it saved it from crashing, but yeah.

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

Might have been my code. I adjusted it and no longer get the IOError.

However, again if I open it in a browser it still hangs as if it's loading, even after I've sent the close method. Is there any way around that? I feel like the request shouldn't sit at pending after I've triggered the close method through tubesock.

I'm just concerned that someone could come along and send a request and have it just sit there, wasting resources. I can add some timeouts on the web server itself, but if there's a way to fix it in the Rails side of things, that'd be awesome.

@ngauthier
Copy link
Owner

I am not sure how a websocket endpoint is supposed to behave when a non-websocket connection comes in. Does it behave properly when you make a normal JS websocket connection?

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

I'm pretty new with websockets, but I removed the close call from the JavaScript side, and I still see the PING/PONG frames coming through when I watch the request in Chrome.

With the close call added back to JavaScript, the frames stop coming in as expected.

It seems like the connection remains open when I call tubesock.close whether I'm doing it through websockets or through a normal request.

@ngauthier
Copy link
Owner

Are you sure you're calling the close, and that it is the only socket open?

Also, please log when the socket opens, because maybe the websocket is
re-establishing the connection when it's closed by the server?

I'm not really sure either, just trying to give some ideas.

On Tue, Mar 11, 2014 at 2:59 PM, tyrbo notifications@github.com wrote:

I'm pretty new with websockets, but I removed the close call from the
JavaScript side, and I still see the PING/PONG frames coming through when I
watch the request in Chrome.

With the close call added back to JavaScript, the frames stop coming in as
expected.

It seems like the connection remains open when I call tubesock.close
whether I'm doing it through websockets or through a normal request.


Reply to this email directly or view it on GitHubhttps://github.com//issues/23#issuecomment-37335718
.

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

Narrowed things down. I had originally been calling tubesock.close from a thread. Clearly a mistake. It would output the "Closed" message from the onclose handler, but it seems like the connection wasn't actually closed.

I've removed the threading stuff I had added and just called tubesock.close as soon as we are connected.

Back to the Puma crash:

Exiting
/Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:92:in `select': closed stream (IOError)
from /Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:92:in 'each_frame'
from /Users/Jon/.rvm/gems/ruby-2.1.0/gems/tubesock-0.2.2/lib/tubesock.rb:63:in 'block in listen'

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

Seems like the current released version (0.2.2) doesn't have the rescues that the current github version has - including IOError.

@tyrbo
Copy link
Author

tyrbo commented Mar 11, 2014

Problem solved when using the latest from github.
New issue:

/Users/Jon/.rvm/gems/ruby-2.1.0/bundler/gems/tubesock-1952e99cbae4/lib/tubesock.rb:96:in `select': Bad file descriptor (Errno::EBADF)

Here is the code in question:
https://gist.github.com/tyrbo/83af7a6bc9d79c3c4884

Nothing too complicated, and the "onclose" handler kills the thread. Am I doing something wrong?

@ngauthier
Copy link
Owner

I'm not sure. It looks ok to me.

On Tue, Mar 11, 2014 at 3:57 PM, tyrbo notifications@github.com wrote:

Problem solved when using the latest from github.
New issue:

/Users/Jon/.rvm/gems/ruby-2.1.0/bundler/gems/tubesock-1952e99cbae4/lib/tubesock.rb:96:in
`select': Bad file descriptor (Errno::EBADF)

Here is the code in question:
https://gist.github.com/tyrbo/83af7a6bc9d79c3c4884

Nothing too complicated, and the "onclose" handler kills the thread. Am I
doing something wrong?


Reply to this email directly or view it on GitHubhttps://github.com//issues/23#issuecomment-37342269
.

@jonathanhoskin
Copy link
Contributor

I've opened a PR to make tubesock.close less crashy on Puma.

@teddythetwig
Copy link
Contributor

From what I understand on this thread, the main issue here is that a hijacked controller method is not responding properly to a regular HTTP request. According to the WebSocket specification, we should only be expecting an HTTP Upgrade request, which is not what you get when connecting to the WS endpoint over regular HTTP.

The opening handshake is intended to be compatible with HTTP-based
server-side software and intermediaries, so that a single port can be
used by both HTTP clients talking to that server and WebSocket
clients talking to that server. To this end, the WebSocket client's
handshake is an HTTP Upgrade request:

    GET /chat HTTP/1.1
    Host: server.example.com
    Upgrade: websocket
    Connection: Upgrade
    Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==
    Origin: http://example.com
    Sec-WebSocket-Protocol: chat, superchat
    Sec-WebSocket-Version: 13

In compliance with [RFC2616], header fields in the handshake may be
sent by the client in any order, so the order in which different
header fields are received is not significant.

The "Request-URI" of the GET method [RFC2616] is used to identify the
endpoint of the WebSocket connection, both to allow multiple domains
to be served from one IP address and to allow multiple WebSocket
endpoints to be served by a single server.

The client includes the hostname in the |Host| header field of its
handshake as per [RFC2616], so that both the client and the server
can verify that they agree on which host is in use.

source

In response to @tyrbo i.e. a regular HTTP request connecting and just wasting cycles, I believe that is a legitimate concern. Not only is it a vulnerability, it doesn't provide definitive behavior in the case of his first question of why it is not closing.

I would propose that we modify the behavior of Tubesock to respond to an improper HTTP request with a 403, instead of trying to continue with the handshake. I'm not sure if this is the specified behavior, but I can't find anything definitive, and this sounds reasonable enough.

Let me know what you guys think.

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

No branches or pull requests

4 participants