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(tls): Remove tls roots implicit configuration #1731

Merged

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Jun 16, 2024

Adds ability to add CA certificates and removes tls-webpki-roots and tls-roots features.

@tottoto tottoto changed the title Remove tls webpki roots and tls roots feature feat(tls): Remove tls webpki roots and tls roots feature Jun 16, 2024
@tottoto tottoto changed the title feat(tls): Remove tls webpki roots and tls roots feature feat(tls): Remove tls-webpki-roots and tls-roots feature Jun 16, 2024
@djc
Copy link
Collaborator

djc commented Jun 18, 2024

Mentioned this on Discord, just repeating it here so it doesn't get lost: why do you want to remove the TLS roots features? In #1724 you didn't want to require the caller to have to depend on rustls-pemfile directly, so this seems to be reasoning in the opposite direction.

@tottoto
Copy link
Collaborator Author

tottoto commented Jun 18, 2024

They share the same philosophy of increasing the user's freedom of configuration without compromising ease of use as much as possible, and reducing the overall complexity of tonic to lower maintenance costs.

Considering that using PEM encoded certificate files when using tls is a typical use case, I think it is a redundant interface to leave the user to make that conversion as web framework.

Removing tls roots and adding interfaces to add them (in exchage for increasing internal Certificate implementation's complexity a little) simplify the tls feature config the and config implementation by removing Endpoint's tls_assume_http2 config and reducing feature config and its conditional compilation. And users are free to chose tls roots or versions of crates which provide tls roots (e.g. rustls-native-certs, tls-webpki-roots) without tonic's releases.

These are intended to balance easiness to use, configurability, and tonic's maintenance costs.

@LucioFranco
Copy link
Member

I am okay with adding the ability to use your own CA certs but I like that idea that we provide the ability to just work with either the system or webpki root certs. I think removing this kinda makes it more tough for users. If they want more flexibility they should be using their own customer connector imo.

@djc
Copy link
Collaborator

djc commented Jun 19, 2024

hyper-rustls offers high-level APIs to use rustls-platform-verifier, rustls-native-certs and webpki-roots. Maybe we can reuse that code here? (I forget if tonic can build on hyper-rustls or whether there's some reason it needs to work with tokio-rustls directly.)

@LucioFranco
Copy link
Member

probably legacy stuff, though I am inclined to not change this code that much. We have a new transport on the way anyways.

@tottoto tottoto force-pushed the remove-tls-webpki-roots-and-tls-roots-feature branch from a575f87 to 9bdf3a9 Compare June 21, 2024 03:27
@tottoto tottoto changed the title feat(tls): Remove tls-webpki-roots and tls-roots feature feat(tls): Remove tls roots implicit configuration Jun 21, 2024
@tottoto
Copy link
Collaborator Author

tottoto commented Jun 21, 2024

Instead of removing these features, added options to enable tls roots, and removes the implicit configuration of them. I think this change simplifies the implementation with keeping the user friendly features.

@djc djc added this pull request to the merge queue Jun 21, 2024
@djc
Copy link
Collaborator

djc commented Jun 21, 2024

This makes sense to me.

Merged via the queue into hyperium:master with commit de73617 Jun 21, 2024
16 checks passed
@tottoto tottoto deleted the remove-tls-webpki-roots-and-tls-roots-feature branch June 21, 2024 08:41
@matze
Copy link
Contributor

matze commented Jul 9, 2024

Would've been nice to mark this as a breaking change. Was confused that I could not connect after bumping tonic.

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

4 participants