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

Add compiler error when enabling rustls without pki. #282

Closed

Conversation

finnbear
Copy link

@finnbear finnbear commented May 9, 2023

Thanks for this library!

This PR makes it impossible to enable the __rustls-tls feature on its own, which would have prevented surrealdb/surrealdb#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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@finnbear finnbear closed this May 15, 2023
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.

None yet

2 participants