snapview / tokio-tungstenite

Future-based Tungstenite for Tokio. Lightweight stream-based WebSocket implementation
MIT License
1.88k stars 236 forks source link

Add compiler error when enabling rustls without pki. #282

Closed finnbear closed 1 year ago

finnbear commented 1 year ago

Thanks for this library!

This PR makes it impossible to enable the __rustls-tls feature on its own, which would have prevented https://github.com/surrealdb/surrealdb/issues/1949

$ cargo check
    Checking tokio-tungstenite v0.19.0 (/home/finnb/git/finnbear/tokio-tungstenite)
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s

$ cargo check --features __rustls-tls
    Checking tokio-tungstenite v0.19.0 (/home/finnb/git/finnbear/tokio-tungstenite)
error: __rustls-tls is private; use one of the documented features to choose root certificate vendor
   --> src/tls.rs:119:29
    |
119 | ...                   compile_error!("__rustls-tls is private; use one of the documented features to choose root certificate vendor");
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `tokio-tungstenite` due to previous error

$ cargo check --features rustls-tls-native-roots
    Checking tokio-tungstenite v0.19.0 (/home/finnb/git/finnbear/tokio-tungstenite)
    Finished dev [unoptimized + debuginfo] target(s) in 0.88s

$ cargo check --features rustls-tls-webpki-roots
    Checking tokio-tungstenite v0.19.0 (/home/finnb/git/finnbear/tokio-tungstenite)
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s
daniel-abramov commented 1 year ago

Thanks for the PR, I understand how annoying it could be to get this error when the feature flags were not specified, but I've noticed that the cargo hack pipeline fails (sorry for the late reply, GitHub pipelines were down for a couple of days).

I wonder if we really want to have a compile_error! there as it would mean that __rustls-tls cannot be used alone, however, it seems like there are some legit use cases where people might want to use __rustls-tls without the root certificates, i.e. when they call the client_async_tls_with_config(), in which case they may not necessarily need to enable root certificate features as they might supply their own connector (which will entail wrap_stream() choosing a different branch that uses the config supplied by the user instead of creating the root certificates).

Does it make any sense?

finnbear commented 1 year ago

Does it make any sense?

Yes, except that if __rustls-tls is intended to be used by end-users in isolation, it probably shouldn't begin with __ which implies that it is a hidden/private feature. The docs could also be updated to clarify this :)

daniel-abramov commented 1 year ago

Yeah, you're right. I think originally it was indeed introduced for the connect feature, so it was not intended to be used alone I think, but back then we added the integration with cargo-hack and ensured that it compiles with different feature sets and now I'm not sure if anyone uses it without root certificates).

We did describe the common features but did not list __rustls-tls. To be honest it's only after checking the code I realized that it could be useful alone 🙂 (cargo-hack failed and so I started checking if there are any legit use-cases for the feature).

finnbear commented 1 year ago

Closing as wontfix in hopes that you consider renaming and documenting the __rustls-tls feature (which, FWIW, does sound useful on its own!)