Skip to content
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

feat: add support WSS #54

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add support WSS #54

wants to merge 2 commits into from

Conversation

c-git
Copy link

@c-git c-git commented Dec 30, 2024

Not sure if this is useful but sharing it in case someone else might find it of value. If you do not think it is a good fit, please feel free to close the PR no hard feelings. I wanted to test connecting to secured endpoints and needed this modification to do it.

Comment on lines 23 to 27
// Setup a CryptoProvider to be able to use wss
match rustls::crypto::ring::default_provider().install_default() {
Ok(()) => {} // Do nothing crypto provider install successful
Err(_) => log::warn!("failed to install CryptoProvider"),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this strictly required to connect over wss:/? If so, we should maybe add it to ewebsock proper (perhaps opt-in), or at least document it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is strictly required to have a provider to connect to WSS. That's a good idea. It would need to be feature flagged so users can disable it if they want to user a different provider. I can do a PR for that if you think that's a good idea.

Copy link
Author

@c-git c-git Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem only affects the native client. It works fine in the browser.

Screenshot of error

Must be done exactly once. Will try to document it and add a optional function the users can call.

Screenshot from docs

I created a copy of the echo server on shuttle to test it at wss://ws-echo-i2m7.shuttle.app/

@c-git c-git marked this pull request as draft January 2, 2025 04:47
@c-git
Copy link
Author

c-git commented Jan 2, 2025

I'm was looking over my comment and found the error message looked wrong. It was wrong. I used https instead of wss. https still works correctly in the browser but not native. I commented out the code you referenced above and... it still worked. So I went back to main and tried again and... it's back to not working on native (the browser still works). It panics on with the following message on native:

thread 'ewebsock' panicked at /home/one/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.7/src/cr
 ypto/mod.rs:260:14:
 no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I honestly don't really understand but it seems I only need to bring in the dependency and that's sufficient for it to work so that's easy and I just put it directly into ewebsocket behind a feature flag and added a note to the documentation.

I also added documentation for the tls feature to the best of my ability, not sure if it had been intentionally not documented.

Let me know if you want me to squash the commits. I should also mention that there is another provider but I didn't try that one as it sounded more complicated in their docs.

@c-git c-git marked this pull request as ready for review January 2, 2025 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants