Added HttpsConnector::new function

[MarkusTieger]

As you can see here ,

HttpsConnector implements

From<(H, C)>  for HttpsConnector<H>
where
    C: Into<Arc<rustls::ClientConfig>>

which just sets the server_name_resolver to DefaultServerNameResolver.

This adds

impl<H, C> From<(H, C, Arc<dyn ResolveServerName + Send + Sync>)> for HttpsConnector<H>
where
    C: Into<Arc<rustls::ClientConfig>>

which does the same, but can be used to specify another server_name_resolver.

[cpu]

Seems pretty reasonable to me. Thanks!

[djc]

Hmm, I'm inclined to add a method for this to the builder instead of adding yet another From impl. Tuples get a bit unwieldy and this isn't quite a full solution since it doesn't support force_https: true, for example.

[MarkusTieger]

Hmm, I'm inclined to add a method for this to the builder instead of adding yet another From impl. Tuples get a bit unwieldy and this isn't quite a full solution since it doesn't support force_https: true, for example.

Suggestions how? The missing method in the builder, is to use an rustls::ClientConfig which has alpn_protocols already set (instead of using enable_http1 and so on). At least to me, there is no obvious way to do it, without breaking the builder api.

[djc]

Actually, it looks like you can already do this? The ConnectorBuilder in WantsProtocols1 state has a with_server_name_resolver() method.

[MarkusTieger]

Yeah, but the builder doesn't accept a TlsConfig with a predefined alpn_protocols. Thats, at least to me, the main use of the From implementation that exists already.

[djc]

Just for my understanding, why do you need/want to set alpn_protocols yourself?

[MarkusTieger]

Just for my understanding, why do you need/want to set alpn_protocols yourself?

Actually, I just want to set the server_name_resolver on reqwest (a crate which uses hyper-rustls). I made a draft pr here: seanmonstar/reqwest#2708 .

Reqwest sets alpn_protocols it self (and does also allow the user to set a custom ClientConfig with alpn_protocols already set, see https://github.com/seanmonstar/reqwest/blob/8cf142bd1f1722a1728a89af21747260bb993294/src/async_impl/client.rs#L2018 ). For this it uses the current From implementation, which I can't use in combination with the server_name_resolver.

[djc]

I guess that just moves the question to, why do you want a custom ResolveServerName impl?

Looks like downstream doesn't necessarily like this solution anyway...

[MarkusTieger]

I guess that just moves the question to, why do you want a custom ResolveServerName impl?

Looks like downstream doesn't necessarily like this solution anyway...

I won't go into much detail, but I have a project (not public, yet) in which there is like a central controlling server and some clients (which also kinda act as servers).

The central server is able to communicate with the "clients" via https (the clients host the https server). They are publicly reachable. As authentication on both sites, there is an ca certificate. The central server has one for "client authentication" and each client has one for "server authentication".

The certificate doesn't have the actual dns name or ip address in it, but instead the database id (which is used on the central server) it should get verified against.

[MarkusTieger]

Also, does the extra From implementation in the codebase really hurt that much?

[djc]

Also, does the extra From implementation in the codebase really hurt that much?

It does not hurt that much -- but it's not great API in the first place and we're potentially stuck with it for a long time, so I'd rather be careful about adding public API. And given that it's not yet clear how reqwest might accomodate your use case, we probably want for that to become more clear.

[MarkusTieger]

As an alternative, how would be a

impl<H> HttpsConnector<H> {

  pub fn new(http: H, tls_config: impl Into<Arc<rustls::ClientConfig>>, force_https: bool, server_name_resolver: Arc<dyn ResolveServerName + Send + Sync>) -> Self {
    Self {
      http,
      tls_config: tls_config.into(),
      force_https,
      server_name_resolver,
    }
  }

}

instead of the From implementation? (ofc I will write rustdoc for the method then etc., also haven't tested the snippet, just as a proposal)

[djc]

Yeah, I'd be more comfortable with that (which I'm aware might be silly).