Ensure that HTTP::WebSocket uses SNI, just like HTTP::Client. #5720
Conversation
spec/manual/sni_required.cr
Outdated
| describe "HTTP::WebSocket uses SNI" do | ||
| host = "certstream.calidog.io" | ||
| it "should connect to #{host} with the websocket protocol" do | ||
| ws = HTTP::WebSocket.new "wss://certstream.calidog.io/" |
There was a problem hiding this comment.
You can do "wss://" + host + '/'
There was a problem hiding this comment.
"wss://#{host}/", please
There was a problem hiding this comment.
We can get rid of the slash / - I don't know if "wss://" + host is better/worse than "wss://#{host}"
There was a problem hiding this comment.
The slash should stay, that's the normalized form. Interpolation should be preferred over concatenation, although the latter is a little bit faster for simple strings.
|
@ysbaddaden A self-contained test would be great. Question is, how do we get a simple SNI server working to test this locally? I don't think |
|
Specs should pass when you're offline, for example on a long flight. If we are to spec this in this way, we should spawn the tls server locally. |
|
I've open an issue for supporting SNI in the HTTP server. I'll get feedback on that (issue #5722), and redo this after the PR for that discussion has been submitted. |
|
I think this is a quite valuable fix. So if we can't come to an agreement/plan how to spec this within two weeks I would vote to merge this with a stub/commented out/todo spec FWIW it's not spec'd in |
|
Let's drop the spec and merge the fix. It was wonderful to have a test, but relying on external service availability is flaky. |
|
there is already spec that uses external service: https://github.com/crystal-lang/crystal/blob/master/spec/manual/badssl_spec.cr |
|
@kostya ugh, we should fix that too. |
|
Thank you! |
Please see the manual spec for a use-case for this. Websockets over Cloudflare fail without SNI. I can modify the manual spec to whatever you guys would like.