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

Endpoint.connect() with TLS fails with InvalidDNSNameError #239

Closed
corhere opened this issue Jan 14, 2020 · 5 comments · Fixed by #244
Closed

Endpoint.connect() with TLS fails with InvalidDNSNameError #239

corhere opened this issue Jan 14, 2020 · 5 comments · Fixed by #244

Comments

@corhere
Copy link

corhere commented Jan 14, 2020

Bug Report

Version

tonic v0.1.0
tonic-build v0.1.0

Platform

Darwin x86_64

Crates

webpki v0.21.0

Description

Creating a channel by calling connect() on an Endpoint configured with a TLS config fails with the error

    Error(hyper::Error(Connect, InvalidDNSNameError))

unless the domain_name option on the ClientTlsConfig is set. Contrary to the docs, the domain_name option is used (and required) even when the rustls_client_config option is set on the config.

The inner InvalidDNSNameError comes from the webpki crate, returned at this line:

let dns = DNSNameRef::try_from_ascii_str(self.domain.as_str())?.to_owned();

When the ClientTlsConfig builds a TlsConnector, the entire connection URI string is used as the domain name if the domain name has not been set on the config.

let domain = match &self.domain {
None => uri.to_string(),
Some(domain) => domain.clone(),
};

I would expect the domain_name option to actually be optional, and for connections to succeed in the common case when the host component of the connection URI matches the domain name for TLS SNI.

@LucioFranco
Copy link
Member

@corhere thanks for writing this up! If I understand correctly you are suggesting that we use uri.host() instead of uri.to_string()?

@corhere
Copy link
Author

corhere commented Jan 14, 2020

@LucioFranco that is correct

@LucioFranco
Copy link
Member

@corhere I think I got a fix up #244 here, would you be able to confirm this works for you? If it does, I can put a release out this week.

@corhere
Copy link
Author

corhere commented Jan 20, 2020

@LucioFranco that fix works for me. Thank you very much for the quick response!

@LucioFranco
Copy link
Member

v0.1.1 is out with this fix.

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 a pull request may close this issue.

2 participants