Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix ipv6 address format in Host header #28578

Merged
merged 13 commits into from
Apr 2, 2018
Merged

Fix ipv6 address format in Host header #28578

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
38620de
fix ipv6 format in host header
caesar-chen Mar 29, 2018
a0ab64c
address feedback
caesar-chen Mar 29, 2018
1798408
address feedback
caesar-chen Mar 29, 2018
a4b4fa4
fix common case
caesar-chen Mar 29, 2018
133ca85
reduce allocation
caesar-chen Mar 30, 2018
7db9f75
address feedback
caesar-chen Mar 30, 2018
387d601
fix indexof issue
caesar-chen Mar 31, 2018
cb9f9b1
disable on curlhandler
caesar-chen Mar 31, 2018
0e685f5
revert to simple fix
caesar-chen Apr 1, 2018
759993d
address feedback and add comment
caesar-chen Apr 1, 2018
ed63124
address test issue, enable on curlhandler
caesar-chen Apr 2, 2018
5da6346
fix framework failure
caesar-chen Apr 2, 2018
22633ac
address feedback
caesar-chen Apr 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,8 @@ private async Task WriteHostHeaderAsync(Uri uri)
else
{
Debug.Assert(_pool.UsingProxy);
await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 []

await WriteAsciiStringAsync(uri.HostNameType == UriHostNameType.IPv6 ?
"[" + uri.IdnHost + "]" : uri.IdnHost).ConfigureAwait(false);
Copy link

@akashkc akashkc Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String interpolation would be more expensive here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, will change and add comment as well.


if (!uri.IsDefaultPort)
{
Expand Down
11 changes: 6 additions & 5 deletions src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (isProxyConnect)
{
Debug.Assert(uri == proxyUri);
return new HttpConnectionKey(HttpConnectionKind.ProxyConnect, uri.IdnHost, uri.Port, null, proxyUri);
return new HttpConnectionKey(HttpConnectionKind.ProxyConnect, host, uri.Port, null, proxyUri);
}

string sslHostName = null;
Expand All @@ -170,7 +171,7 @@ private static HttpConnectionKey GetConnectionKey(HttpRequestMessage request, Ur
if (HttpUtilities.IsNonSecureWebSocketScheme(uri.Scheme))
{
// Non-secure websocket connection through proxy to the destination.
return new HttpConnectionKey(HttpConnectionKind.ProxyTunnel, uri.IdnHost, uri.Port, null, proxyUri);
return new HttpConnectionKey(HttpConnectionKind.ProxyTunnel, host, uri.Port, null, proxyUri);
}
else
{
Expand All @@ -183,16 +184,16 @@ private static HttpConnectionKey GetConnectionKey(HttpRequestMessage request, Ur
else
{
// Tunnel SSL connection through proxy to the destination.
return new HttpConnectionKey(HttpConnectionKind.SslProxyTunnel, uri.IdnHost, uri.Port, sslHostName, proxyUri);
return new HttpConnectionKey(HttpConnectionKind.SslProxyTunnel, host, uri.Port, sslHostName, proxyUri);
}
}
else if (sslHostName != null)
{
return new HttpConnectionKey(HttpConnectionKind.Https, uri.IdnHost, uri.Port, sslHostName, null);
return new HttpConnectionKey(HttpConnectionKind.Https, host, uri.Port, sslHostName, null);
}
else
{
return new HttpConnectionKey(HttpConnectionKind.Http, uri.IdnHost, uri.Port, null, null);
return new HttpConnectionKey(HttpConnectionKind.Http, host, uri.Port, null, null);
}
}

Expand Down
62 changes: 62 additions & 0 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,68 @@ public async Task GetAsync_SetAutomaticDecompression_ContentDecompressed(Uri ser
}
}

[Theory]
[InlineData("[::1234]")]
[InlineData("[::1234]:8080")]
public async Task GetAsync_IPv6AddressInHostHeader_CorrectlyFormatted(string host)
{
string ipv6Address = "http://" + host;
bool connectionAccepted = false;

await LoopbackServer.CreateClientAndServerAsync(async proxyUri =>
{
using (HttpClientHandler handler = CreateHttpClientHandler())
using (var client = new HttpClient(handler))
{
handler.Proxy = new WebProxy(proxyUri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you taught me this. : )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try { await client.GetAsync(ipv6Address); } catch { }
}
}, server => server.AcceptConnectionAsync(async connection =>
{
connectionAccepted = true;
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();
Assert.Contains($"Host: {host}", headers);
}));

Assert.True(connectionAccepted);
}

public static IEnumerable<object[]> SecureAndNonSecure_IPBasedUri_MemberData() =>
from address in new[] { IPAddress.Loopback, IPAddress.IPv6Loopback }
from useSsl in new[] { true, false }
select new object[] { address, useSsl };

[Theory]
[MemberData(nameof(SecureAndNonSecure_IPBasedUri_MemberData))]
public async Task GetAsync_SecureAndNonSecureIPBasedUri_CorrectlyFormatted(IPAddress address, bool useSsl)
{
var options = new LoopbackServer.Options { Address = address, UseSsl= useSsl };
bool connectionAccepted = false;
string host = "";

await LoopbackServer.CreateClientAndServerAsync(async url =>
{
host = $"{url.Host}:{url.Port}";
using (HttpClientHandler handler = CreateHttpClientHandler())
using (var client = new HttpClient(handler))
{
if (useSsl)
{
handler.ServerCertificateCustomValidationCallback = delegate { return true; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

handler.ClientCertificateOptions = ClientCertificateOption.Automatic;
Copy link
Member

@stephentoub stephentoub Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this setting of ClientCertificateOptions needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove.

}
try { await client.GetAsync(url); } catch { }
}
}, server => server.AcceptConnectionAsync(async connection =>
{
connectionAccepted = true;
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();
Assert.Contains($"Host: {host}", headers);
}), options);

Assert.True(connectionAccepted);
}

[OuterLoop] // TODO: Issue #11345
[Theory, MemberData(nameof(CompressedServers))]
public async Task GetAsync_SetAutomaticDecompression_HeadersRemoved(Uri server)
Expand Down