-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ensure that HTTP::WebSocket uses SNI, just like HTTP::Client. #5720
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch is fine. A test is great, yet I'm not sure we should rely on an external service in the spec suite?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do "wss://" + host + '/'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"wss://#{host}/"
, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.