Allow passing in a custom rustls ClientConfig to Channel

[tiziano88]

cc @ipetr0v @conradgrobler @djc @LucioFranco

Originally posted by @tiziano88 in #859 (comment)

[djc]

I wonder if we could use hyper's connector trait bounds for this? If we just require Connect and Clone, we can pass that into the hyper client builder. In that way, we can conceivably support native-tls as well different rustls versions through their appropriate hyper-rustls integrations.

[LucioFranco]

For now I would suggest following this example #968 this will currently work with tonic 0.7. The future plan for tonic is to actually remove the transport module. I will be submitting a proposal for this soon.

[stuhood]

For now I would suggest following this example #968 this will currently work with tonic 0.7.

That example involves quite a bit of code compared to using a tonic::transport::Endpoint with a custom rustls tls_config in tonic 0.6. I'm not sure that that really seems like a palatable alternative, since it seemingly has to ensure that the hyper and tower layers are configured "just so" for tonic.

[LucioFranco]

While I do agree the ergonomics are not as good, tying tonic to rustls releases is not feasible either.

Currently, the client_rustls example does have some rough edges but these are things we plan to fix in upcoming releases. A lot of the code that made transport easy to use doesn't belong in something like tonic, because it should b extended to use-cases beyond. So removing ClientConfig from the public API is a push in that direction. Now if you are running into specific issues with that example/using hyper etc. I am more than happy to help and understand where those rough edges are.

[bmwill]

Somewhat related to no longer being able to use a custom rustls::ClientConfig we're also unable to use a custom rustls:ServerConfig. From @LucioFranco's most recent comment and looking at some of discussion around this it seems like the main motivation for this change was to not have rustls be apart of the public API of tonic. During some experimentation trying to use a custom config for both client and server with tonic 0.7 I found that rustls is still sort of unintentionally a part of tonic's public api due to the implementation of Connected for tokio_rustls::server::TlsStream.

Specifically if i'm relying on the fact that tokio_rustls::server::TlsStream impls Connected in order to do this:

    // let tls_config = ... some custom rustls ServerConfig
    let listener = tokio_stream::wrappers::TcpListenerStream::new(
        tokio::net::TcpListener::bind("localhost:0")?,
    );
    let tls_acceptor = TlsAcceptor::from(Arc::new(tls_config));
    let listener = listener.and_then(move |stream| tls_acceptor.accept(stream));

    let greeter = MyGreeter::default();
    let server = Server::builder()
        .add_service(GreeterServer::new(greeter));
    // Requires that TlsStream impls `Connected`
    server.serve_with_incoming(listener).await;

Then when tonic bumps its internal rustls version and does a patched release you could break code out in the wild. Of course I may also be misunderstanding what would constitute a semver breaking change given this would be a somewhat obscure breakage.

[LucioFranco]

Thanks @bmwill that was missed, the next breaking release will remove that impl.

[kvc0]

For now I would suggest following this example #968 this will currently work with tonic 0.7. The future plan for tonic is to actually remove the transport module. I will be submitting a proposal for this soon.

Damn this is tough to keep up with. I'm updating a cli application that needs to be able to consume a --insecure option (like curl). Previously this was easy with rustls_client_config on the ClientConfig. Sure it was gross with the leaky transitive dependency and the ergonomics were poor but it was easy and it worked.

In the application I'm updating when --insecure is not passed I need to use rustls-native-certs or the like so I can support local users' certificates. 0.7 ClientTlsConfig, however, breaks both workflows by [1] not providing an override for the TLS verifier, [2] having no answer to RootCertStore for multiple trust roots and [3] offering no escape hatch to rustls.

This facility shouldn't have been removed without an alternative. I'm digging through https://github.com/hyperium/tonic/blob/master/examples/src/tls/client_rustls.rs to see how & whether I can integrate it; but this is a surprising upset in API stability, especially with no observable deprecation period & no discernable communication on the public documentation of a migration path.

I get that your semver leads with 0 so here be dragons and I'll take my licks for using a 0. library; but the Hyper family libraries are becoming ubiquitous and foundational - they're awesome libraries, tonic included: Stability is expected, 0. or not, and orderly migrations are paramount!

Thank you for your stewardship of these libraries. They're no small part of why I love Rust!

[LucioFranco]

Hey! @kvc0 I appreciate the feedback and I do agree there should be more proper paths for migrations and deprecations etc. That said, as you mentioned this project is still below 1.0 and with some of the other work around prost that has taken up most of my time there hasn't been any work towards a migration guides etc. I am currently in the works of writing up the full tonic 1.0 roadmap so hopefully we can get there by end of year. Or at least somewhere close enough where I would be happy to place proper deprecation notices.

That said, I don't believe hyper has bumped a major version in a while so the older versions of tonic should continue to work. If you are interested in helping out smooth the API and write some guides I would totally accept some PRs.

In addition, the plan moving forward for how people will use tonic will look closer to the rustls examples that use hyper directly as the transport module will go away. The reason I removed these APIs from that is to prep for those changes down the line.

[LucioFranco]

I've updated the example here with a more ergonomic change #1017 should make using hyper directly much easier.

[tdyas]

Are there any recommendations for gRPC friendly default values for setting up Hyper and rustls?

[Kixunil]

Hey, this is a double problem to me: first that you removed it, second that if this issue is resolved it'll be past my MSRV. So I ask to allow backporting whatever the fix for this will be.

As for possible solutions to not tie tonic to rustls (very reasonable choice, I agree!):

• Provide an API to set custom TLS acceptor closure - then people who need custom acceptor just rewrite theirs on top of it
• Provide a separate crate tonic-rustl which will bridge the two. Then people needing rustls integration will just use it accepting a bit more churn while others don't.
• Convince rustls maintainers to stabilize the TLS config interface in a separate crate that both rustls and tonic will depend on

Please also note that we're literally having a DoS vulnerability because of being unable to upgrade. Thankfully it shouldn't be a big deal since if an attacker can spoof certificate he can just DoS by cutting the connection but it's still annoying. It may be a good reason to at least put it back until a proper solution is found.

[tdyas]

In case it will help any one, for the https://github.com/pantsbuild/pants project, I wrote a bridge between tonic and rustls inspired in part on the tonic-openssl adapter I had seen elsewhere. See this grpc_util::Channel type for the Tonic/Rustls integration.

[djc]

Convince rustls maintainers to stabilize the TLS config interface in a separate crate that both rustls and tonic will depend on

I've thought about this quite a bit but I don't think it's feasible. There are too many changes happening that involve configuration for now.

[Kixunil]

@djc thanks a lot for giving it a thought!

[lvkv]

It would be great to be able to pass a Rustls ClientConfig to the Channel builder, as the TLS knobs exposed by tls_config(self, tls_config: ClientTlsConfig) are a bit lacking.

For example, I'd like to rely on a ClientConfig that uses an Arc<dyn tokio_rustls::rustls::client::ResolvesClientCert> to dynamically load my client authentication certificates (e.g. useful for when they expire).

The tonic Rustls client example looks like it will work this purpose, but it feels weird to use gRPC without building and reusing Channels. Maybe that's ok? I might also be misunderstanding how that example works.

[tdyas]

@lvkv: The earlier comment #891 (comment) which links to code which allows integrating Tonic and Rustls may be of interest to you.

[lvkv]

@tdyas Thanks for the pointer, I'm definitely looking into this as an alternative. However, I'm also interested in the cheap clone interface offered by the proper tonic::transport::channel::Channel, which my existing codebase makes extensive use of:

To work around this and to ease the use of the channel, Channel provides a Clone implementation that is cheap. This is because at the very top level the channel is backed by a tower_buffer::Buffer which runs the connection in a background task and provides a mpsc channel interface. Due to this cloning the Channel type is cheap and encouraged.

I'm sure I could plop a #[derive(Clone)] at the top of your struct and call it a day, but I don't know how performant that would be. Would it make sense to add a tower_buffer::Buffer in front of the internals like tonic::transport::channel::Channel does?

Edit: I just realized that plopping a #[derive(Clone)] at the top of your struct is exactly what you did 🙂

[joune]

I'm also trying to pass a custom rustls config to my tonic Client, namely to use rustls_platform_verifier instead of the default ClientTlsConfig.

following the example linked in this thread, I'm able to make it work with the following code

fn grpc_client<B>() -> hyper_util::client::legacy::Client<HttpsConnector<HttpConnector>, B>
where
    B: http_body::Body + Send,
    B::Data: Send,
{
    hyper_util::client::legacy::Client::builder(TokioExecutor::new()).build(https_connector())
}

fn https_connector() -> HttpsConnector<HttpConnector> {
    HttpsConnectorBuilder::new()
        .with_tls_config(rustls_platform_verifier::tls_config())
        .https_only()
        .enable_http2()
        .build()
}

...
let client = crate::grpc_client();
let mut client = MyApiClient::with_origin(client, Uri::from_static("https://myapp:443"));

This works, but this forces me to pass specifically the hyper_util client with https everywhere, instead of a tonic Channel abstraction.

Now I noticed it is also possible to create a tonic Channel with the same https connector, like this:

pub async fn grpc_channel() -> Result<Channel, TransportError> {
        Channel::from_static("https://myapp:443")
            .connect_with_connector(https_connector())
            .await
}

But in this case the connection fails with source: None

I fail to understand what the hyper_util Client does that the tonic Channel doesn't.
Could someone provide some guidance?

[tdyas]

@joune: fyi this "Channel" implementation in another project may be of value: https://github.com/pantsbuild/pants/blob/76ef3e4f83dc99075bb205d1fdc6a6011e501dce/src/rust/engine/grpc_util/src/channel.rs

It is initialized with a rustls::ClientConfig and can be passed to Tonic as is.

[joune]

thanks @tdyas 👍
I ended up making something just like this indeed, but it does feel like a workaround.

[bmwill]

I had been using a custom connector with the build-in Channel type that handled using a custom rustls config but it only worked if the "tls" feature in tonic was disabled. Recently I needed to pull in another dependency that enabled the "tls" feature, and needed a standard tls config, and my workaround broke since even if you provide your own connector, tonic will check for the https scheme and error out if a tls config isn't present. In order to work around this issue I ended up forking the tonic "transport" code, removing the tls config wrappers and instead exposing a rustls config directly and published it as tonic-rustls.

[shikhar]

I just ran into this very issue.

Specifically the problem is, when using a custom connector [1] and an https endpoint, this error branch is being hit

tonic/tonic/src/transport/channel/service/connector.rs

Line 68 in 758d4f9

Err(HttpsUriWithoutTlsSupport(()).into())

https://gist.github.com/shikhar/2677ca111e82b2a8ebfbada2d2bf1582 "fixes" this problem. What do maintainers think about getting rid of this error?

---

[1] HttpsConnector from hyper_rustls layered on top of HttpConnector from hyper_util, to be able to use a custom rustls::ClientConfig

[djc]

I think your diff has tonic silently fall back to plaintext HTTP in case of a misconfiguration? That seems like a non-starter.

IMO the integration crate proposed by @bmwill is probably the right approach for this kind of stuff.

There might also be a change in the future when the internal transport offered by tonic is rethought.

[bmwill]

There might also be a change in the future when the internal transport offered by tonic is rethought.

My assumption is that the main reason that the rustls types aren't exposed today is due to not wanting to require a version bump of tonic when rustls releases a new version.

Until the project decides to rethink how rustls integrations with the batteries-included transport (maybe by moving the transport to a separate crate), we can use tonic-rustls as an experiment.

[shikhar]

I think your diff has tonic silently fall back to plaintext HTTP in case of a misconfiguration? That seems like a non-starter.

That's fair, I found an alternate approach that is much better: #2015