fix(client): pass dns:// hostname to Mint for TLS SNI and verification - #575
Open
atirna wants to merge 2 commits into
Open
fix(client): pass dns:// hostname to Mint for TLS SNI and verification#575atirna wants to merge 2 commits into
atirna wants to merge 2 commits into
Conversation
A dns:// target resolves to an IP, and the resolved IP was used as the connect host, so TLS saw the IP instead of the target hostname: SNI, certificate hostname verification, and the :authority pseudo-header all failed against servers like *.googleapis.com. Keep the hostname alongside each resolved address, carry it onto the channel, and pass it to Mint as the hostname option, which dials the resolved address while TLS uses the hostname. Fixes elixir-grpc#572
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A
dns://target resolves the hostname to an IP and then dials that IP, so TLSsees the IP where the hostname belongs: SNI carries the IP, certificate hostname
verification fails (
hostname_check_failed, requested IP vs*.googleapis.comSANs), and the
:authoritypseudo-header is wrong too.Root cause
GRPC.Client.Resolver.DNSresolvesdns://host:portinto%{address: ip, port: port}and the hostname is dropped right there. Theresolved IP becomes
channel.host, and the Mint adapter passes it toMint.HTTP.connect/4, which uses the connect host for SNI, certificatehostname verification, and
:authority.Fix
Keep the hostname alongside each resolved address, carry it onto the channel as
hostname, and pass it to Mint through its:hostnameoption, which isdocumented for exactly this case: dial the address, but use the given hostname
for the
Hostheader, hostname verification, and SNI.Verified end to end against the real path: a
dns://target resolving to127.0.0.1now reachesMint.HTTP.connect(:https, "127.0.0.1", port, hostname: "my-service.local").Fixes #572
Test plan
test "keeps the hostname of a dns:// target alongside the resolved IP"—resolver-level regression test (fails on master: the address map has no
:hostnamekey)test "carries the dns:// hostname onto the channel for TLS SNI"— theconnection layer threads the resolver's hostname onto the channel (fails on
master with a
KeyErroronchannel.hostname)test "dials the resolved address but hands the hostname to Mint for TLS"—the Mint adapter hands the hostname to the connection process connect opts
while still dialing the resolved IP (fails on master:
state.connect_opts[:hostname]isnil)Full suite locally:
mix testpasses except one pre-existing timing-sensitiveReResolveTestflake, which reproduces on master with the same frequency(reproduced across multiple runs on both trees).
Notes
The Gun adapter derives SNI from the host it dials as well (gun's
ensure_tls_opts/3usesorigin_host), so it has the same shape of problemfor
dns://targets. I left it out of this PR to keep the change scoped to theadapter from the report; passing
server_name_indicationthroughtransport_optsthere would follow the same idea.