Wrong websocket scheme is chosen #437

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

kingel commented Jan 15, 2012

In our setup we have stud (sslproxy) in front of haproxy in front of tornado and nginx.

In this situation tornado sees the request protocol as http instead of https, so it will choose "ws", when we look at the origin we see "https" instead of "http" and we can choose for "wss".

Is this something other people encounter? Is the patch correct?

Owner

bdarnell commented Jan 15, 2012

This is what the xheaders option to HTTPServer is for: A proxy can insert special headers into the request to pass along information that would otherwise be lost, such as HTTP vs HTTPS and the client's IP address. It's easy to configure nginx to to do this, although it doesn't look like stud has the option to do so (you could have nginx add the headers for requests coming from stud since you're using both, although that's not very elegant).

Switching based on the Origin header is an interesting idea that would usually work, but it's not guaranteed (I think it's legal to access a secure websocket from an insecure page or vice versa, although some combinations will result in warnings from the browser).

I think maybe what's needed is an option at the httpserver level to unconditionally set request.protocol to https, for use with ssl proxies like stud or stunnel that cannot be configured to modify the request. Maybe instead of a binary xheaders=True that tries to guess what kind of proxy you have we need to let you select between several modes, or maybe just an arbitrary hook to tweak the request (an explicit mode would at least theoretically allow for supporting stud's ability to insert IP information before the HTTP request, although hooking that up in the right place might get messy).

kingel commented Jan 15, 2012

I'll look into the xheaders, but i don't think stud can do that, maybe there are some options in haproxy. Nginx is not in front of tornado, but on the same level serving static content.

I got the idea for this from the socket.io code (https://github.com/LearnBoost/socket.io/blob/master/lib/transports/websocket/default.js#L91) they introduced this extra parameter 'match origin protocol'

@bdarnell bdarnell closed this in cc671cb Jan 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment