hyper-rustls icon indicating copy to clipboard operation
hyper-rustls copied to clipboard

Per-request Server Name Indication configuration

Open agraven opened this issue 3 years ago • 13 comments

Currently, the rustls::ServerName value passed to rustls for SNI is always based on the hostname of the URI. However, sometimes it is necessary to override the SNI for specific requests, for example when performing matrix federation well-known lookups. A mechanism for controlling what SNI to use would be very useful.

agraven avatar Apr 01 '22 13:04 agraven

Presumably you're trying to do this with hyper? It looks like to me like the integration API hyper exposes doesn't easily allow this. However, if you have a credible design proposal I'm open to reviewing it... Seems pretty niche.

djc avatar Apr 04 '22 13:04 djc

I also need this, seconding to prove it is not niche

randomairborne avatar Apr 06 '22 02:04 randomairborne

What do you need this for? Please explain your use case/context in order to help design the right API.

djc avatar Apr 06 '22 07:04 djc

As I'm too getting notifications for this repo I can say for me if this were available I might use it some day to do domain fronting

paolobarbolini avatar Apr 06 '22 07:04 paolobarbolini

I'm not familiar with the "domain fronting" term. Can you explain more? Any suggestions on how the API should look?

djc avatar Apr 06 '22 07:04 djc

Basically you provide an SNI different from the Host header so that someone looking at the traffic thinks you're requesting the website in the SNI field, when in reality you're requesting the virtual host specified in the Host header, but they can't obviously see that.

I'm not too sure about the API, it's just an option I have in mind in case some country's firewall starts breaking our things

paolobarbolini avatar Apr 06 '22 07:04 paolobarbolini

The wikipedia page for domain fronting covers the subject the subject quite well, I think: https://en.wikipedia.org/wiki/Domain_fronting.

On the subject of the API, it does make things quite difficult that a Uri is all the function has to work with. If it was a http::Request it would be trivial to add an extension value to the request and then extract that in the HttpsConnector. Adding a mapping between hostname and servername along the lines of HashMap<String, rustls::ServerName> in the tls configuration could be one option, though that would come with the downside of not being configurable per-request, only per-connector/client.

agraven avatar Apr 06 '22 08:04 agraven

The wikipedia page for domain fronting covers the subject the subject quite well, I think: https://www.youtube.com/watch?v=vbWKnuW16L0.

@agraven I don't think that's the link you intended to post? 😄

I think I will stick with saying I'm happy to review an API design proposal or PR for this.

djc avatar Apr 06 '22 08:04 djc

Indeed I didn't, my clipboard is deceiving me once again 😅. Here's the correct link: https://en.wikipedia.org/wiki/Domain_fronting

agraven avatar Apr 06 '22 08:04 agraven

In addition, I would like the ability to override the ServerName. My use case is a bit more simple but in https://github.com/hyperium/tonic I'd like to have tls examples that run out of the box using rustls. My certs have a SNI for example.com but I would like my clients to connect as 127.0.0.1/[::1] but currently this is not possible with this connector since it pulls the host name from the passed in Uri.

Though I think this problem naturally lies with the fact that hyper wants a Service<Uri> which is very limiting.

I have found a current work around leveraging the Service trait:

    let tls = ClientConfig::builder()
        .with_safe_defaults()
        .with_root_certificates(roots)
        .with_no_client_auth();

    let mut http = HttpConnector::new();
    http.enforce_http(false);

    let connector = tower::ServiceBuilder::new()
        .layer_fn(move |s| {
            let tls = tls.clone();

            hyper_rustls::HttpsConnectorBuilder::new()
                .with_tls_config(tls)
                .https_or_http()
                .enable_http2()
                .wrap_connector(s)
        })
        .map_request(|_| Uri::from_static("https://[::1]:50051"))
        .service(http);

    let client = hyper::Client::builder().build(connector);

    let uri = Uri::from_static("https://example.com:50051");
    let mut client = EchoClient::new2(uri, client);

    let request = tonic::Request::new(EchoRequest {
        message: "hello".into(),
    });

    let response = client.unary_echo(request).await?;

All this does is intercept the Uri passed from the https layer into the http layer and replaces it with the correct ip to connect. This is pretty annoying meaning you'd have to manage the map yourself but the potential HashMap<String, ServerName> that @agraven suggested could be implemented this way outside of this crate.

This leads me to think that we should just improve the ergonomics of the connector builder to actually provide a Layer impl that just clones the ClientConfig and wraps the incoming service. Allowing you to layer things how you want via the ServiceBuilder.

LucioFranco avatar Apr 06 '22 20:04 LucioFranco

I'm not much of a fan of the Layer approach as I experienced it in tower or tracing. While it is definitely a powerful/expressive abstraction, it's also pretty complex compared to what is a relatively simple need. I would agree that the Service definition hyper accepts here is ultimately at fault, given that hyper is in the process of changing interfaces anyway I think it would be good to pursue a change to that. In the meantime, putting a HashMap in the HttpsConnector seems like a reasonable way to support this.

djc avatar Apr 07 '22 09:04 djc

We will be working on the abstraction for hyper but I am not sure how much actual wiggle room there is there.

Talking to Sean its likely we are going to keep some sort of service layering thing. If the builder just implements layer then its a drop into the builder which makes it pretty trivial to work with. You're just using it just like how you might use map/fold on Iterator.

I honestly would urge to not add the HashMap support if its possible to do it externally like I showed since I don't think its really the correct solution long term anyways.

LucioFranco avatar Apr 07 '22 14:04 LucioFranco

There's a simpler proposal for something like this in #195.

djc avatar Mar 15 '23 08:03 djc

We've since added a trait for this in #269, so I'm going to consider this solved.

djc avatar Apr 23 '24 08:04 djc