Runtime: IPv6 addresses not correctly formatted in Host headers

[Tratcher]

The new SocketsHttpHandler does not correctly format IPv6 addresses in host headers.

Repro:

            var client = new HttpClient();
            var response = await client.GetAsync("http://[::1]:5001");

Actual output:

      GET / HTTP/1.1
      Host: ::1:5001

Expected output: Host: [::1]:5001

Always include the brackets, even if there isn't a port.

Code: https://github.com/dotnet/corefx/blob/f14366baaa80a893e53934b98e9786dac54c136f/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L124
https://github.com/dotnet/corefx/blob/f14366baaa80a893e53934b98e9786dac54c136f/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs#L191-L195

Found by ASP.NET Core Kestrel functional tests. https://github.com/aspnet/KestrelHttpServer/issues/2406

Priority: Not preview blocking, but RTM blocking.

@karelz

[karelz]

Thanks @Tratcher!
@caesar1995 can you please help with it?

[caesar-chen]

According to
https://tools.ietf.org/html/rfc7230#section-5.4
https://tools.ietf.org/html/rfc3986#section-3.2.2

IPv6 addresses in Host header should be enclosed in square brackets.

I will fix it.

[geoffkizer]

We should probably fix this at a higher level, e.g. GetConnectionKey, and ensure that we consistently use the right hostname (i.e. including []) for the key and the pool.

[geoffkizer]

I do kinda wonder why Uri.IdnHost doesn't return the [] itself. Is there some other magic way to get Uri to give us the []?

[karelz]

@rmkerr for the Uri question

[Tratcher]

https://msdn.microsoft.com/en-us/library/system.uri.hostnametype(v=vs.110).aspx should help if you need to special case IPv6. Host should give you the brackets for IPv6.

[rmkerr]

@geoffkizer It looks like Host treats IPv6 addresses specially:
https://github.com/dotnet/corefx/blob/9e8d443ff78c4f0a9a6bedf7f95961cf96ceff0a/src/System.Private.Uri/src/System/Uri.cs#L903-L905
We should be doing the same thing with IdnHost, but we aren't currently.

The fix would be to special case Flags.IPv6HostType just like we do Flags.DnsHostType here:
https://github.com/dotnet/corefx/blob/9e8d443ff78c4f0a9a6bedf7f95961cf96ceff0a/src/System.Private.Uri/src/System/Uri.cs#L1223-L1237

@karelz how do you feel about the risk of making a behavior change to System.Uri here?

[caesar-chen]

We explicitly strip the [] for IPv6 address for DnsSafeHost

https://github.com/dotnet/corefx/blob/9e8d443ff78c4f0a9a6bedf7f95961cf96ceff0a/src/System.Private.Uri/src/System/Uri.cs#L1195-L1201

[rmkerr]

Good find. I wonder why the decision was made to do things that way.

[caesar-chen]

From MSDN: https://msdn.microsoft.com/en-us/library/system.uri.dnssafehost(v=vs.110).aspx

For IPv6 addresses, the brackets ([]) are removed and the ScopeId property is set, if one was specified when this instance was constructed.

Probably we cannot fix it in the Uri level.

[wfurt]

The help for Uri class says:
The RFC 3490 compliant International Domain Name of the host, using Punycode as appropriate. This string, after being unescaped if necessary, is safe to use for DNS resolution.

I think that should not include [] as they are not legal in DNS.

[caesar-chen]

For WinHttpHandler, we directly pass Host for WinHttpConnect instead of idnHost. Probably we need to do the same for SocketsHttpHandler.

https://github.com/dotnet/corefx/blob/9e8d443ff78c4f0a9a6bedf7f95961cf96ceff0a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs#L786-L790

[davidsh]

Actually, that is a bug. Passing in Uri.Host will potentially pass in Unicode host names to the WinHTTP API. And that API can't accept that. Unfortunately, we have no tests around this.

We should use Uri.IdnHost for passing into WinHTTP.

See:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384091(v=vs.85).aspx

pswzServerName [in]
Pointer to a null-terminated string that contains the host name of an HTTP server. Alternately, the string can contain the IP address of the site in ASCII, for example, 10.0.1.45. Note that WinHttp does not accept international host names without converting them first to Punycode. For more information, see Handling Internationalized Domain Names (IDNs).

[caesar-chen]

1. According to MSDN: idnHost is DNS safe:

This property is provided as the preferred alternative to using DnsSafeHost, because IdnHost is guaranteed to always be DNS safe, no matter what the current app.config settings might be.

1. From previous comment, [] are not legal in DNS. So idnHost should not include [].

If we use Uri.IdnHost for passing into WinHTTP/SocketsHttpHanlder, then...the Host header cannot be formatted correctly.

I think:
a) we should not change the idnHost in URI level.
b) the fix should be append [] around idnHost before we send it to network.

I will send a PR so people can comment on the code.

[caesar-chen]

Opened https://github.com/dotnet/corefx/issues/28722 to track WinHttp bug.