hyperium / tonic

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

Tonic 0.7 prevents using a custom TLS connector #980

Closed roblabla closed 2 years ago

roblabla commented 2 years ago

Bug Report

In tonic 0.6, we had code to create our own TLS connector based on boringssl instead of rustls. This is necessary for my use-case as I need to support a cert that uses an algorithm not currently supported by rustls (P521):

async fn try_connect() -> Result<Channel, Box<dyn Error + Send + Sync>> {
    let uri = "https://url-to-our-backend";

    let uri = uri.parse::<Uri>()?;

    let mut http_connector = HttpConnector::new();
    http_connector.enforce_http(false);
    let connector = BoxConnector::new(
        http_connector
            .map_response(|v| Box::new(v) as _)
            .map_err(|v| Box::new(v) as _),
    );

    // Create the TLS Connector.
    let mut builder = SslConnector::builder(SslMethod::tls()).unwrap();
    // <snip>: set custom root CA.
    builder.set_alpn_protos(b"\x02h2").unwrap();
    let mut tls_connector = HttpsConnector::with_connector(connector, builder).unwrap();
    tls_connector.set_callback(|connect_conf, _uri| {
        connect_conf.set_verify_hostname(false);
        Ok(())
    });
    let connector = BoxConnector::new(tls_connector.map_response(|v| Box::new(v) as _))

    Ok(Channel::builder(uri)
        .connect_with_connector(connector)
        .await?)
}

However, after updating to 0.7.1, we get an HttpsUriWithoutTlsSupport error. After further investigation, it looks like this commit is the culprit: https://github.com/hyperium/tonic/commit/ef6e245180936097e56f5f95ed8b182674f3131b

Essentially, with this commit, it becomes impossible to provide our own TLS connector, as we will always end up in the is_https codepath here.

Version

Platform

Darwin roblablas-MacBook-Pro.local 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 arm64

LucioFranco commented 2 years ago

So I think you're right that code does look a bit wrong and maybe we can relax it a bit. I would maybe suggest even just going ahead and using hyper directly similar to the client_rustls example for now. Or just adjusting the Uri to use http instead of https. If you're inclined to fix it I would accept a PR fixing this.

roblabla commented 2 years ago

If you're inclined to fix it I would accept a PR fixing this.

How would a fix look like? I'm not entirely sure what we want here. It looks like the error was introduced to provide a clearer error message to a common failure scenario (using an https URI without a tls connector configured).

At first I thought about reworking the code to match on the error returned by let io = connect.await, replacing it with an HttpsUriWithoutTlsSupport in the correct situation, but given the error result is generic, that doesn't seem really possible.

Should the error perhaps be moved to a higher layer? For instance, it could probably be moved as-is to endpoint::connect, which would be in the path of most high-level API uses (such as GreeterClient::connect("http://[::1]:50051").await?;) without inhibiting users of lower-level APIs.

LucioFranco commented 2 years ago

Fixed via https://github.com/hyperium/tonic/commit/1dd5ad2b07810fc6eb5015c152ec737b5f0ca39c