search

hyperium / tonic

A native gRPC client & server implementation with async/await support.
https://docs.rs/tonic
MIT License
9.32k stars 954 forks source link

feat(tls): Remove tls roots implicit configuration #1731

Closed tottoto closed 2 weeks ago

tottoto commented 2 weeks ago

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

djc commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

tottoto commented 2 weeks ago

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 commented 2 weeks ago

This makes sense to me.