Proxy support
Closed this issue · 12 comments
Does faye-websocket-node support proxy servers? If so, is there documentation available on how to set the proxy to be used?
No, it doesn't right now. I'm not sure what would need to be done to make it support proxies; a lot of the docs I've found on this are inconsistent on how WS proxying works. Do you have some example use cases, or suggestions for how I can test against a representative proxy server?
I implemented proxy support in Meteor using websocket-driver. There totally might be bugs in error handling or something, though. https://github.com/meteor/meteor/blob/devel/packages/ddp/stream_client_nodejs.js
@craig-miller @glasser I've now added proxy support in 05cc735 and faye/websocket-driver-node@bdc9bfe. Can you check it looks okay?
Wow, thanks! And much simpler when you do it at the low level like that. I'm going to review this and then see if I can revert my Meteor changes which swapped it over to websocket-driver! (I was doubtful that I got all the details of error handling right around my simulated faye-websocket anyway.)
I made some comments on the commits. While it mostly looks good, it only allows you to connect to HTTP targets (over HTTPS or HTTP proxies), not to HTTPS targets, since it can't do a TLS handshake in the middle.
Another thing I noticed with your current code: there's no error handler set on the SecurePair. An error gets emitted on the pair in some cases; eg if you tell it to proxy to a wss URL but the server is actually HTTP:
$ node examples/echo_client.js wss://todos.meteor.com:80/websocket http://localhost:4000/
[socket close] 1006
events.js:72
throw er; // Unhandled 'error' event
^
Error: socket hang up
at SecurePair.error (tls.js:1008:23)
at EncryptedStream.CryptoStream._done (tls.js:700:22)
at CleartextStream.read [as _read] (tls.js:496:24)
at CleartextStream.Readable.read (_stream_readable.js:323:10)
at EncryptedStream.onCryptoStreamFinish (tls.js:301:47)
at EncryptedStream.g (events.js:180:16)
at EncryptedStream.emit (events.js:117:20)
at finishMaybe (_stream_writable.js:360:12)
at endWritable (_stream_writable.js:367:3)
at EncryptedStream.Writable.end (_stream_writable.js:345:5)
It's probably worth looking at the implementation of tls.connect to see what it does that you're not doing here (eg, it delegates errors from the pair object to the cleartext object, which does seem to fix this issue). I also note that tls.connect appears to be what checks the server's certificate to see if it has the expected hostname, so that probably isn't happening with just the SecurePair. Also, you're not waiting for the pair secure event (which looks like it happens before the normal tls.connect callback would be called), which maybe is problematic.
@glasser The code now on master should be more robust for all combos of secure/non-secure proxies and origin servers, and there are integration tests around ws: and wss: through an http: proxy. Let me know if it looks okay to you.
The driver API has also been simplified to prevent the caller having to shuffle I/O streams around when using a proxy.
@glasser I'm getting close to having this feature in a releasable shape. Could you feed back as soon as you can on whether it's working for your use case(s)?
Yup, will try to look at it tonight or tomorrow. Thanks!
Looks like it works. My only quibble is that it would be nice if the error returned on non-200 results contained the status code (since knowing that something is eg 403 can be helpful for debugging). (I guess Node's http parser drops the human-readable part after the number.)
@glasser I've now released websocket-driver@0.4.0 and faye-websocket@0.8.0. After a few rounds of redesigning the API, I've gone with putting the TLS details in this module rather than the driver, it's much simpler this way. The integration tests cover all combinations of ws/wss origins and http/https proxies so I think it's good to go.