calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.47k stars 232 forks source link

Allow clients that don't trust the SSL certificate to opt-in to connecting insecurely #602

Closed JorgenPhi closed 8 years ago

JorgenPhi commented 8 years ago

Server Problem

Please confirm whether you've tried the following debugging steps:

This should also take advantage of upstream proxies passing x-forwarded-proto for determining which version to pass on to the client.

calzoneman commented 8 years ago

This is by design. The only reason that the site does not auto-redirect to HTTPS when it is available is because of cross-origin content security policy which breaks certain video types that do not support HTTPS yet (such as Hitbox). I can't think of any reason why you would want the socket to connect to an insecure endpoint when a secure one is available; what is your use case?

JorgenPhi commented 8 years ago

Some users have issues getting their browser to trust Let's Encrypt. Notably android clients. Should probably just suck up the money for a real certificate. Wasn't aware that this was intentional.

calzoneman commented 8 years ago

Yes, there is an issue with older clients and LetsEncrypt certificates, which is unfortunate. This was an issue for cytu.be as well; I had to buy a cert for the main server but now I've added a secondary one using LetsEncrypt so there will need to be a solution for that.

I think a better solution than defaulting to insecure would be to programmatically detect when the connection fails and prompt the user to accept that they are using a browser that does not support the CA, but if they check the box then they can proceed with an insecure connection.

calzoneman commented 8 years ago

To make the issue more clear, I'm updating the title.

JorgenPhi commented 8 years ago

On another note, the ACP seems to attempt to choose the corresponding protocol's socket.io URL, but if the server is configured to use a non-standard port it breaks. https://github.com/calzoneman/sync/blob/3.0/src/web/acp.js#L29

calzoneman commented 8 years ago

Oh that actually brings up another point which is that the socket.io library is loaded over the default endpoint which is HTTPS, so that will be tricky to handle for clients where HTTPS is broken.

With regard to the ACP, "configured to use a non-standard port it breaks" -- to be a bit pedantic, if you set "https.default-port" to a port that is not actually bound to any HTTPS listener then it breaks. The ACP should be updated to share the same endpoint selection logic as the channel page, but it is still currently possible to use the ACP on non-standard HTTPS ports as long as it's configured correctly.

JorgenPhi commented 8 years ago

So I use nginx as a cache for static elements hosted on my cytube instance, and i was always under the impression that "default-port" was only for generating links. The socket.io URLs are still correctly generated for all pages except the acp. I'd also like to mention that I would prefer to not have to proxy websockets through nginx.

I also had to override how ipv4-ssl was generated in sync/src/config.js as it would ignore the different subdomain passed in cfg.io["domain"].

calzoneman commented 8 years ago

The configuration I'm using:

listeners:
  - io: true
    https: true
    port: 10443

https:
  default-port: 10443
  full-address: 'https://cytu.be'

I'm not proxying any socket.io connections (only HTTPS), so I've never run into this particular issue with the ACP, which I believe is off topic for this issue anyways and should probably be reported separately.

Anyways, the real solution for that is to untangle what is probably one of the biggest and longest lasting hacks in CyTube (the HTTP/HTTPS/IO listener configuration logic) and implement a more sane configuration that will likely involve server administrators explicitly specifying base URLs instead of trying to autogenerate them with complicated and confusing logic.

It's worth noting that proxying socket.io was not officially supported until recently (there are very few reasons you would want to do so, and it carries a performance loss for higher volume sites) so the ACP is just one issue that slipped by because it was never tested and no one complained.

calzoneman commented 8 years ago

Please try the changes from the related commit (https://github.com/calzoneman/sync/commit/6e416fea8ab191303be2f3c3b5c41859aa773d48) and edit callbacks.js for your server to set USING_LETS_ENCRYPT = true to enable it.

JorgenPhi commented 8 years ago

I'll deploy this on the server and get back to you later tonight. Thanks for looking into this.