fix(#3817): send servername for SNI on TLS

[metcoder95]

This relates to...

Fixes #3817

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[metcoder95]

The test should cover the problem, but if anything better, let me know

[DTrombett]

This does solve the certificate error but it still doesn't correctly pass the SNI, for example the following code returns a 404:

request("https://example.com", {
  dispatcher: new Agent().compose(interceptors.dns()),
}).then(res => res.body.text()).then(console.log);

Is this just a limitation of interceptors?

[ronag]

it still doesn't correctly pass the SNI, for example the following code returns a 404:

Not sure I follow. 404 is not an SNI error?

[ronag]

@metcoder95 don't we also need to update the host header to the original url before dns lookup? See https://github.com/nxtedition/nxt-undici/blob/45de148436285d5063ec78aaf6360d0c121fe8ad/lib/interceptor/dns.js#L71-L88

i.e. we convert a dns name to an ip adress before reaching the connection layer, however the intended dns name still needs to be represented in the host header.

[DTrombett]

Yeah, I'm sorry, I confused the server name with host; the problem is indeed the Host header and can confirm it works fine when it is manually passed

[mcollina]

lgtm

[ronag]

the problem is indeed the Host header and can confirm it works fine when it is manually passed

Interesting that this PR doesn't fix that. Don't we always pass servername as host header?

[metcoder95]

the problem is indeed the Host header and can confirm it works fine when it is manually passed

Interesting that this PR doesn't fix that. Don't we always pass servername as host header?

Noup, I tried to assess that but we were always passing the resolved IP instead (which was set as the new origin). Maybe I overlooked that but can double check.

Update: Yeah, it wasn't doing it. It was setting the host header as the new origin instead of the servername.

Good catch on the host header, I will add it 👍

[metcoder95]

@ronag ptal

[ronag]

You should do it regardless of https or http

[ronag]

@metcoder95 ping?

[metcoder95]

Sorry, was off last week and still catching up. I'll work on this over the week

[github-actions]

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-3821-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b93a83447a99fecf41a09c1b6857ae855a2254c9
# Push it to GitHub
git push --set-upstream origin backport-3821-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x

Then, create a pull request where the base branch is v6.x and the compare/head branch is backport-3821-to-v6.x.