hyperium / tonic

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

Dynamic inclusion of system truststores #1232

Open j-baker opened 1 year ago

j-baker commented 1 year ago

Feature Request

Crates

tonic

Motivation

I have a client library using tonic which connects to a server. Depending on deployment configuration, this server may be deployed with a real, publicly issued TLS certificate or a self-signed certificate. In the self-signed instance, the CA is provisioned to disk by deployment infrastructure and it's important that only that CA is trusted.

Tonic presently allows specific CAs to be added by using ClientTlsConfig::ca_certificate, and these are registered at an appropriate time. Separately, one can add the tls-roots feature, which adds a dependency, and then the system CA roots are added forcibly in each use case.

For this use case, tonic may be used by multiple libraries in the eventual application, so it's undesirable for another application pulling in the crate with the tls-roots feature to change the behaviour of our library. Additionally, it's undesirable for us to need to compile two versions of our application, one with the system roots added, one without.

Proposal

Either:

Add an enum to ClientTlsConfig named something like

#[non_exhaustive]
pub enum TlsRoots {
  CustomRoots(Certificate),
  #[cfg(feature = "tls-roots")]
  SystemRoots,
  #[cfg(feature = "tls-webpki-roots")]
  WebPkiRoots,
}

Then, the configuration could contain a HashSet of TlsRoots which could be added to or overwritten via the config, and the system roots only added if the set contains the relevant variants.

Alternatively, one could add methods

#[cfg(feature = "tls-roots")]
pub fn disable_system_roots(self) -> Self;

#[cfg(feature = "tls-webpki-roots")]
pub fn disable_webpki_roots(self) -> Self;

which has the same effect of causing them to not be added. This has the drawback that in order to maintain present behaviour, the flag is a 'disable' type flag and not an enable flag.

Alternatives

Haven't considered any alternatives.

LucioFranco commented 1 year ago

This has the drawback that in order to maintain present behaviour, the flag is a 'disable' type flag and not an enable flag.

Could you explain what you mean here, Im not totally following.

j-baker commented 1 year ago

So with these certs, conceptually you're starting with no trust. So, it ideally would be the case that:

  1. You use base tonic+tls+tls-roots l, you get no trust at all.
  2. You add your own CA, now certs chaining to that CA are trusted.
  3. You add the system roots bundle, now those are trusted too.

Whereas to maintain API backwards compatibility, it'd be more like:

  1. You use base tonic + tls+tls-roots, you get all the system trusted certs.
  2. You hit .remove_system_roots() to remove that trust, now you're at 0.

With the first proposal, you might end up with something like:

tls_conf.trust(vec![custom_cert])

Literally meaning that there are no system certs, and if you want system certs then maybe you do

tls_conf.trust(vec![SystemCertificates]) as well, the point would be to overwrite whatever is set.

LucioFranco commented 1 year ago

Ok got it.

So while this may not be the ideal answer this should unblock you for now, I might suggest if you want to differ from what tonic has now that you use this example https://github.com/hyperium/tonic/blob/master/examples/src/tls/client_rustls.rs and just configure rustls to your liking.

The big reason I say this is that the tls configuration as you see is a bit off and not ideal. I'd like to actually remove all tls support and make it so that users can just directly use rustls/openssl without having it tied to tonic. So I would suggest that route and as we make the ability to hook in tls easier this will become more ergonomic over time.

rbtcollins commented 9 months ago

So in 0.9 that example - https://github.com/hyperium/tonic/blob/v0.8.4/examples/src/tls/client_rustls.rs - got removed. Is it considered flawed? I wanted to use the rustls client directly to enable an insecure mode per the rustls mio example... in lieu of being able to just ask ClientTlsConfig to accept an arbitrary rustls TlsConfig, which seems ideal