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

rustls-tls-native-roots does not imply rustls-tls-native-roots for tungstenite #313

Open
DCNick3 opened this issue Dec 7, 2023 · 3 comments

Comments

@DCNick3
Copy link

DCNick3 commented Dec 7, 2023

I have encountered something that is is not a bug per se, but rather a hard-to-debug gotcha.

It is a combination of two facts:

  1. tokio-tungstenite re-exports the entire tungstenite API surface
  2. rustls-tls-native-roots feature in tokio-tungstenite does not enable the rustls-tls-native-roots feature in tungstenite
  3. still, it enables some support through the internal __rustls-tls feature

In our project we provide both the async and sync versions of the API by using either tungstenite or tokio-tungstenite in the implementation. Some time ago we decided not to depend on tungstenite directly, but use the re-exported version of tokio-tungstenite.

However, even though rustls-tls-native-roots on tokio-tungstenite is enabled, an attempt to made a sync connection with tungstenite to a server fails.

And the hard-to-debug part of this is the error message: Io(Custom { kind: InvalidData, error: InvalidCertificate(UnknownIssuer) }). It suggests that there's some kind of problem with the TLS certificate.

However, in actuality, it is due to the fact that the re-exported tungstenite has TLS support enabled, but doesn't have any way to verify the certificate, so it just fails with this message. It doesn't point to the root issue (lack of the TLS verification features on the `tungstenite). I think this is not a good UX.

It might have been better if rustls-tls-native-roots feature on tokio-tungstenite (and, I think, the same problem affects rustls-tls-webpki-roots) also enabled rustls-tls-native-roots feature of tungstenite. If this is infeasible, tungstenite might detect such a condition and provide an error message pointing out the lack of configured cert verification methods.

@agalakhov
Copy link
Member

Thank you. Indeed it is a bug. We just forgot to add corresponding feature dependencies.

@jbg
Copy link

jbg commented May 20, 2024

Why does tokio_tungstenite need to enable TLS features in tungstenite at all? It looks to me that the TLS is implemented using tokio-rustls or tokio-native-tls here in this crate, and unencrypted data is being passed to/from tungstenite, so enabling those features in tungstenite should be unnecessary, shouldn't it?

@daniel-abramov
Copy link
Member

In principle, your assumptions are correct. However, we're still using some types from tungstenite such as Error::TlsError and some stuff from the stream.rs that are only available when certain features are enabled for tungstenite. But in theory, decoupling these should be possible.

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

No branches or pull requests

4 participants