Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to set the SNI host for the connections #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olksdr
Copy link

@olksdr olksdr commented Jul 22, 2020

Sometimes the URI and the exact host which we want to connect have different domains. And we need to have a way to set the domain for the TLS connection which will be used to verify the TLS cert.
The domain from the URI will still be used to create a underlying TCP connection.

For example:

Target host: example.com
Target host's TLS: example.net

with setting the SNI host to example.net we can still easily connect to example.com and get the correct resources as for example.net

Sometimes the URI  and the exact host which we want to connect
have different domains. And we need to have a way to set the domain for
the TLS connection which will be used to verify the TLS cert.
The domain from the URI will still be used to create a underlying TCP
connection.

For example:

Target host: example.com
Target host's TLS: example.net

with setting the SNI host to `example.net` we can still easily connect to
`example.com` and get the correct resources as for `example.net`
@olksdr
Copy link
Author

olksdr commented Jul 23, 2020

I don't think the failing CI job is related to my changes.

/// Set the host which should be used for Server Name Indication (SNI).
///
/// if the `sni_host` is not set the default host from URI will be used
pub fn sni_host(&mut self, host: &str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SNI host is already sent normally, just extracted from the Uri hostname. So this would really be more an override, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another concern that can be had is that this would force all requests to always use the same SNI value. It seems simple to accidentally configure this option and then use the connector for several different domains.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SNI host is already sent normally, just extracted from the Uri hostname. So this would really be more an override, right?

Yes, this is correct. In some cases we need to override the hostname with different value.

Another concern that can be had is that this would force all requests to always use the same SNI value. It seems simple to accidentally configure this option and then use the connector for several different domains.

yeah, this is the valid concern, but I haven't found a better way how I could provide the different hostname for the TLS connection. Maybe I missed something here, but if I could have access to the underlying Request I could tell http connector to extract the domain from the Host header instead if provided (in this case it might be need to have such an option exposes to configure http connecter before building the client), which might be a bit better way to set this up.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it that typical for servers to have certificates with SNI requirements that don't match their DNS values?

/// Set the host which should be used for Server Name Indication (SNI).
///
/// if the `sni_host` is not set the default host from URI will be used
pub fn sni_host(&mut self, host: &str) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another concern that can be had is that this would force all requests to always use the same SNI value. It seems simple to accidentally configure this option and then use the connector for several different domains.

@olksdr
Copy link
Author

olksdr commented Jul 24, 2020

hey @seanmonstar , thanks for the review!

Is it that typical for servers to have certificates with SNI requirements that don't match their DNS values?

Yes, this is very typical for proxies/gateways. For example:

  • we have a host example.com and the TLS termination happens on the rust service (gateway), but we still need to connect to third party over https, e.g. https://203.0.113.5
  • third party web server configured with valid certificate for example.com - but we cannot use this domain for connection since the DNS points to our service with completely different IP
  • in this case we need to connect to https://203.0.113.5 but tell TLS layer to use example.com for connection and certificate verification
  • remote http server will route our connection correctly to the correct virtualhost.

Very good check for this:

openssl s_client -connect <IP_ADDRESS>:443 -servername <HOSTNAME>

So, this PR adds the possibility to configure same behavior as you can see inthe openssl example about. And which this configuration we SSl between gateway and the third party service provider.

This is very important feature which I would like to see supported.

What do you think?

@olksdr
Copy link
Author

olksdr commented Aug 4, 2020

Hi @seanmonstar, did you have time to consider and think about proposed changes?
Maybe you have some idea how to make the handling the described above workflow better?

@Absolucy
Copy link

This would be useful, as I want to manually resolve a host's IP myself using DNS, then connect to that and set the HOST header.

@ermakov-oleg
Copy link

I agree, this is an important feature for http proxies. And now I guess I'll just have to copy the crate...

@seanmonstar Is this project alive at all? No commits since 2020...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants