Fix ipv6 address format in Host header #28578

caesar-chen · 2018-03-29T01:50:02Z

Per discussion in #28557. If this change looks ok, I will fix the WinHttp as well.

Fix: #28557 (moved to dotnet/runtime#25661)

stephentoub · 2018-03-29T01:52:46Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                using (HttpClientHandler handler = CreateHttpClientHandler())
+                using (var client = new HttpClient(handler))
+                {
+                    handler.Proxy = new WebProxy(proxyUri);


Actually you taught me this. : )

Heh, ok, at least I'm consistent :)

stephentoub · 2018-03-29T01:54:15Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+        [Fact]
+        public async Task GetAsync_IPv6AddressInHostHeader_CorrectlyFormatted()
+        {
+            string host = "[::1234]:8080";


Can you make the test a theory and validate with and without a port?

akashkc · 2018-03-29T02:27:03Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -269,7 +269,8 @@ private void ConsumeFromRemainingBuffer(int bytesToConsume)
            else
            {
                Debug.Assert(_pool.UsingProxy);
-                await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false);
+                await WriteAsciiStringAsync(uri.HostNameType == UriHostNameType.IPv6 ?
+                    "[" + uri.IdnHost + "]" : uri.IdnHost).ConfigureAwait(false);


How about using string interpolation instead of string concatenation manually i.e. $"[{uri.IdnHost}]"

String interpolation would be more expensive here.

Does string interpolation has significant performance difference than string concatenation. I believe, string interpolation has better readability.

In this case it'll compile down to a call to string.Concat:

string.Concat("[", uri.IdnHost, "]");

which will incur just a single allocation for the destination string, with the data from each constituent string blitted into it. In contrast, the string interpolation would compile down to the equivalent of:

string.Format("[{0}]", new object[] { uri.IdnHost });

and would thus incur at least the destination string and an object array allocation, plus the work that has to be done inside of Format, which involves a StringBuilder.

Another useful note on string.Concat vs. string.Format #8025 (comment) (I want "bookmark favorite comment" feature in GitHub)

@stephentoub : Thank you for explaining it with implementation side. @kasper3 : Thank you for commenting with interesting thread.

Since we're now using uri.Host in the GetConnectionKey path, seems like it would make sense to use it here as well for consistency and to avoid the extra alloc (maybe?).

I agree, will change and add comment as well.

stephentoub · 2018-03-29T02:49:00Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

-        public async Task GetAsync_IPv6AddressInHostHeader_CorrectlyFormatted()
+        [Theory]
+        [InlineData("http://", "[::1234]")]
+        [InlineData("http://", "[::1234]:8080")]


Nit: the scheme doesn't need to be parameterized

caesar-chen · 2018-03-29T02:49:21Z

I was trying to add test data for https ws and wss as well, but those scheme will make the test to hang locally, not sure why.

stephentoub · 2018-03-29T02:49:40Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+        [Theory]
+        [InlineData("http://", "[::1234]")]
+        [InlineData("http://", "[::1234]:8080")]
+        public async Task GetAsync_IPv6AddressInHostHeader_CorrectlyFormatted(string scheme, string host)


I assume this test fails without your product change? Just checking.

Without product change:

Assert.Contains() Failure Not found: Host: [::1234] In value: List<String> ["GET http://::1234/ HTTP/1.1", "Host: ::1234"]

Only semi-related, is the Uri we're sending to the proxy correctly formatted, or does it also need [] around the IPv6 address?

On WinHttpHandler, we will append [] around the IPv6 address.
"GET http://[::1234]/ HTTP/1.1", "Proxy-Connection: Keep-Alive", "Host: [::1234]"

On SocketsHttpHandler, currently we don't:
"GET http://::1234/ HTTP/1.1", "Host: [::1234]"

Per RFC (https://tools.ietf.org/html/rfc3986#section-3.2.2), I think we should do that for the URI we are sending as well.

I found another issue that for SocketsHttpHandler, the port number is missing in uri sent to the proxy as well.

Opened #28609 to track this.

caesar-chen · 2018-03-29T02:52:29Z

To provide more confidence, by stepping into the code, I can see in Host header, ipv6 address is appended with [] for the above hanging schemes. (since no test added for them)

stephentoub · 2018-03-29T04:34:29Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -269,7 +269,8 @@ private void ConsumeFromRemainingBuffer(int bytesToConsume)
            else
            {
                Debug.Assert(_pool.UsingProxy);
-                await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false);


What about the if block above? Don't we need to fix that case as well?

Yes, we need to fix that as well. The fix there is more involved because it needs to happen as part of the construction of the connection key -- basically we should detect that the hostname is an IPv6 address when constructing the key, and construct the hostname using [].

You won't be able to test this using a proxy as you're doing here. But you should at least be able to test against IPv6Loopback. There's already a test for IP address based Uris -- GetAsync_IPBasedUri_Success. Unfortunately it's not validating the Host header. It should be modified to validate the Host header. And it would be nice to add https cases to that test as well.

Thanks for pointing this out! I will make a fix and add tests to it.

Fix the above if by changing GetConnectionKey method.

We still need to special case this else statement for Proxy case, because for this case, we pass null as host, thus _pool.HostHeaderValueBytes == null, and here is the only place to append []

geoffkizer · 2018-03-29T05:05:10Z

I was trying to add test data for https ws and wss as well, but those scheme will make the test to hang locally, not sure why.

That's because those schemes will try to tunnel through your proxy.

caesar-chen · 2018-03-29T19:07:24Z

New commit explanation:

Fix the IPv6 missing [ ] in GetConnectionKey() method. However, there is a special case, for Proxy state, we pass null as host, thus _pool.HostHeaderValueBytes == null. So we need to fix this case in WriteHostHeaderAsync() method, as the previous approach does. So I didn't change that.

caesar-chen · 2018-03-30T17:31:58Z

@dotnet-bot test Linux x64 Release Build
@dotnet-bot test OSX x64 Debug Build

CI hang.

stephentoub · 2018-03-30T20:11:44Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -140,11 +140,12 @@ private static string ParseHostNameFromHeader(string hostHeader)
        private static HttpConnectionKey GetConnectionKey(HttpRequestMessage request, Uri proxyUri, bool isProxyConnect)
        {
            Uri uri = request.RequestUri;
+            string host = uri.HostNameType == UriHostNameType.IPv6 ? "[" + uri.IdnHost + "]" : uri.IdnHost;


This is adding an allocation for every request using an IPv6 address as a host name. Is that necessary? Since the resulting HttpConnectionKey is used to choose the pool to use, can't this be cached on the pool instead? And if not, can we skip doing this if proxyUri == null?

can't this be cached on the pool instead?

I think it's possible. I didn't do that in the first place, because we will need a helper logic to determine if the address we passed in is IPv6 or not (we pass in string, not uri), and I thought the helper logic could result in additional allocation. I will try to explore this option, if not able to come up with efficient way, we may need to try the second appraoch -> skip doing this if proxyUri == null.

IPv6 host names are unusual, saving a few extra allocations for them is not a high priority.

caesar-chen · 2018-03-30T21:57:14Z

@stephentoub Do you think this change will result in smaller allocation?

karelz · 2018-03-30T22:28:26Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

@@ -117,12 +117,15 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK

            if (_host != null)
            {
+                bool isHostTypeIPv6 = _host.Split(':').Length - 1 > 1;


Should it be just _host.Contains(':')? Why does it check for 2+ :?

A valid IPv6 address contains at least two :, so I check it for two times.

Split allocates unnecessarily - even if just per ConnectionPool, what about using IndexOf on the string twice?

if it's performance critical, then I think more efficient way is:

var count = 0; var isHostTypeIPv6 = false; foreach (char c in _host) { if (c == ':' && ++count > 1) { isHostTypeIPv6 = true; break; } }

Consider host is not a long string, IndexOf can make the code cleaner.

caesar-chen · 2018-03-30T23:31:22Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+            from useSsl in new[] { true, false }
+            select new object[] { address, useSsl };
+
+        [OuterLoop]


Add to outerloop since the execution for this test is long, will cause Linux catastrophic failure in innerloop run.

Why is the text execution long?

Why does it cause catastrophic failure on Linux?

This seems like a straightforward test, what am I missing?

The Linux catastrophic failure confused me a lot as well. Here is my observation:

Before adding this test, the CI is green.

After I added the test, it will consistently (every time ) cause catastrophic failure on Centos, Fedora, RedHat, and Ubuntu 1804, while passing on other distros. No matter I put it in innerloop/outerloop.

After I disable the test on CurlHandler, no catastrophic failure with System.Net.Http. (There is one with System.Runtime.Extensions.Tests, I think it's totally unrelated).

Why is the text execution long?

That's my guess. The execution log didn't show any test failure. So I doubt the execution failed is because of hang, then timeout.

Why does it cause catastrophic failure on Linux?

The test consistently cause catastrophic failure on Centos, Fedora, RedHat, and Ubuntu 1804, while passing on other distros. I think it could be (very likely) different CurlHandler version causing the issue. (Since Windows run is green every time).

To summary: I think it's a strange issue with CurlHandler causing the catastrophic failure. To unblock the PR, I will open a new issue to tract this, and disable the test against CurlHandler.

stephentoub · 2018-03-31T01:17:32Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

@@ -117,12 +117,16 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK

            if (_host != null)
            {
+                int posColon = -1;
+                bool isHostTypeIPv6 = (-1 != (posColon = _host.IndexOf(':')) && -1 != (posColon = _host.IndexOf(':', posColon)));


Isn't the second _host.IndexOf always going to succeed, because you're passing in posColon rather than posColon+1? Seems like we need some more tests, too, to catch this kind of thing.

Is it true that every IPv6 address contains at least two colons, and that if an address contains two colons it must be an IPv6 address? If so, with the aforementioned change, the logic seems fine.

If it's not true, a better approach might be to augment the HttpConnectionPool ctor to accept a "Uri exemplarUri", then at the ctor call site pass in the request.Uri, and here use Uri.HostNameType instead of doing this check manually.

Yes, every ipv6 contains at least two colons and nothing else does.

Ok, then with the index fix, this looks good, e.g.

bool isHostTypeIPv6 = (posColon = _host.IndexOf(':')) != -1 && _host.IndexOf(':', posColon+1) != -1;

caesar-chen · 2018-03-31T07:43:38Z

The new test I added: GetAsync_SecureAndNonSecureIPBasedUri_CorrectlyFormatted will consistently causing catastrophic failure on Centos, Fedora, RedHat, and Ubuntu 1804, while passing on other distros. I think it could be different CurlHandler version causing the issue. (Since Windows run is green every time).

I will disable the test against Linux, to see if this is the issue.

caesar-chen · 2018-03-31T07:49:44Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build

geoffkizer · 2018-03-31T08:26:11Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

+                int posColon = -1;
+                bool isHostTypeIPv6 = (posColon = _host.IndexOf(':')) != -1 && _host.IndexOf(':', posColon+1) != -1;
+                string hostAddress = isHostTypeIPv6 ? "[" + _host + "]" : _host;
+


Can we just use Uri.CheckHostName here?

Alternatively, can we just go back to checking for HostNameType == IPv6 in GetConnectionKey, but use uri.Host instead of uri.IdnHost in that case? I think that will return the address with [], right?

I'm kinda queasy about doing our own parsing here. Uri does all sorts of stuff for us already, let's just use it if possible instead of trying to duplicate the logic ourselves.

I will also point out that retrieving parts of the Uri (e.g. hostname) is already a bit of a perf bottleneck. Uri.IdnHost is allocating a string. I believe we also end up allocating later when we retrieve path/query etc.

We should look at optimizing this better, but not for 2.1.

In the meantime let's just do something simple and correct, and we can optimize interaction with Uri in the future.

can we just go back to checking for HostNameType == IPv6 in GetConnectionKey, but use uri.Host instead of uri.IdnHost in that case?

WinHttp clearly documented that it will only accept Punycode host name. Changing to uri.Host may bring regression for SocketsHttpHandler, and I think that's risky for 2.1.

I think that will return the address with [], right?

Yes, uri.Host will return the address with [].

I'm kinda queasy about doing our own parsing here.

I will change to Uri.CheckHostName.

WinHttp clearly documented that it will only accept Punycode host name. Changing to uri.Host may bring regression for SocketsHttpHandler, and I think that's risky for 2.1.

I'm not suggesting we change it in all cases. Obviously that would break Punycode handling. Just in the case where it's an IP address.

I think that should work. I will push a new commit.

geoffkizer · 2018-04-01T20:29:45Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs


            if (isProxyConnect)
            {
                Debug.Assert(uri == proxyUri);
-                return new HttpConnectionKey(HttpConnectionKind.ProxyConnect, uri.IdnHost, uri.Port, null, proxyUri);
+                return new HttpConnectionKey(HttpConnectionKind.ProxyConnect, isIPv6Address ? uri.Host: uri.IdnHost, uri.Port, null, proxyUri);


Nit: space before colon in conditional expression, here and below

Oops T.T, will fix.

geoffkizer · 2018-04-01T20:33:08Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -140,11 +140,12 @@ private static string ParseHostNameFromHeader(string hostHeader)
        private static HttpConnectionKey GetConnectionKey(HttpRequestMessage request, Uri proxyUri, bool isProxyConnect)
        {
            Uri uri = request.RequestUri;
+            bool isIPv6Address = uri.HostNameType == UriHostNameType.IPv6;


We should add a comment here, something like:

// If the hostname is an IPv6 address, uri.IdnHost will return the address without enclosing [].
// In this case, use uri.Host instead, which will correctly enclose with [].
// Note we don't need punycode encoding if it's an IP address, so using uri.Host is fine.

stephentoub · 2018-04-02T00:59:08Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                {
+                    if (useSsl)
+                    {
+                        handler.ServerCertificateCustomValidationCallback = delegate { return true; };


You mentioned in a separate issue that you were seeing this test hang with CurlHandler on some OSes. Have you tried using HttpClientHandler.DangerousAcceptAnyServerCertificateValidator here instead of delegate { return true; }?

No I didn't. I will try it and enable the test to see if this can resolve the hang.

stephentoub · 2018-04-02T00:59:29Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+                    if (useSsl)
+                    {
+                        handler.ServerCertificateCustomValidationCallback = delegate { return true; };
+                        handler.ClientCertificateOptions = ClientCertificateOption.Automatic;


Why is this setting of ClientCertificateOptions needed?

Will remove.

caesar-chen · 2018-04-02T05:42:34Z

@stephentoub

Changing to HttpClientHandler.DangerousAcceptAnyServerCertificateValidator resolved the Linux catastrophic failure. Close #28703.

@dotnet-bot test NETFX x86 Release Build

geoffkizer · 2018-04-02T07:14:43Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

-                        handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;
+                        handler.ServerCertificateCustomValidationCallback =
+#if netcoreapp
+                            HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;


Use TestHelper.AllowAllCertificates

Will change.

caesar-chen · 2018-04-02T17:19:47Z

Unrelated failure.

@dotnet-bot test Linux x64 Release Build
@dotnet-bot test OSX x64 Debug Build

* fix ipv6 format in host header * address feedback * address feedback * fix common case * reduce allocation * address feedback * fix indexof issue * disable on curlhandler * revert to simple fix * address feedback and add comment * address test issue, enable on curlhandler * fix framework failure * address feedback Commit migrated from dotnet/corefx@24889e2

fix ipv6 format in host header

38620de

caesar-chen added the area-System.Net.Http.SocketsHttpHandler label Mar 29, 2018

caesar-chen added this to the 2.1.0 milestone Mar 29, 2018

caesar-chen self-assigned this Mar 29, 2018

caesar-chen requested review from Tratcher, stephentoub and a team March 29, 2018 01:50

stephentoub reviewed Mar 29, 2018

View reviewed changes

akashkc reviewed Mar 29, 2018

View reviewed changes

address feedback

a0ab64c

stephentoub reviewed Mar 29, 2018

View reviewed changes

stephentoub approved these changes Mar 29, 2018

View reviewed changes

stephentoub reviewed Mar 29, 2018

View reviewed changes

address feedback

1798408

stephentoub reviewed Mar 29, 2018

View reviewed changes

fix common case

a4b4fa4

caesar-chen force-pushed the ipv6_host branch from f6436d3 to a4b4fa4 Compare March 29, 2018 18:58

stephentoub reviewed Mar 30, 2018

View reviewed changes

reduce allocation

133ca85

karelz reviewed Mar 30, 2018

View reviewed changes

address feedback

7db9f75

caesar-chen commented Mar 30, 2018

View reviewed changes

stephentoub reviewed Mar 31, 2018

View reviewed changes

fix indexof issue

387d601

disable on curlhandler

cb9f9b1

geoffkizer reviewed Mar 31, 2018

View reviewed changes

revert to simple fix

0e685f5

geoffkizer reviewed Apr 1, 2018

View reviewed changes

address feedback and add comment

759993d

geoffkizer approved these changes Apr 1, 2018

View reviewed changes

stephentoub reviewed Apr 2, 2018

View reviewed changes

address test issue, enable on curlhandler

ed63124

fix framework failure

5da6346

geoffkizer reviewed Apr 2, 2018

View reviewed changes

address feedback

22633ac

caesar-chen merged commit 24889e2 into dotnet:master Apr 2, 2018

caesar-chen deleted the ipv6_host branch April 2, 2018 18:26

caesar-chen mentioned this pull request Apr 12, 2018

SocketsHttpHandler: Add workaround for ipv6 LLA address #28971

Merged

ghost mentioned this pull request Apr 27, 2018

Updating arch support for arm dotnet/core-setup#4085

Merged

caesar-chen mentioned this pull request Jan 31, 2020

[Uri] Fix IPv6 address format dotnet/runtime#25782

Open

Fix ipv6 address format in Host header #28578

Fix ipv6 address format in Host header #28578

Conversation

caesar-chen commented Mar 29, 2018 • edited by karelz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashkc Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Mar 29, 2018

caesar-chen commented Mar 29, 2018

caesar-chen commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Mar 31, 2018

caesar-chen commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Apr 2, 2018

caesar-chen commented Mar 29, 2018 •

edited by karelz

Loading

akashkc Mar 29, 2018 •

edited

Loading

stephentoub Apr 2, 2018 •

edited

Loading