SocketsHttpHandler: Add workaround for ipv6 LLA address by caesar-chen · Pull Request #28971 · dotnet/corefx

caesar-chen · 2018-04-10T06:38:06Z

Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number (scope) part, and scope is mandatory for LLA. We will use uri.IdnHost for IPv6 LLA address on SocketsHttpHandler.

This PR is a workaround for #28863, and should be reverted when #28863 is fixed.

Close #28749

caesar-chen · 2018-04-10T06:42:05Z

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

+            // Since scope is mandatory for LLA, we will use uri.IdnHost for IPv6 LLA address.
+            // From RFC 4291, IPv6 LLA has formate like: fe80::/10.
+            bool isNonLLAIPv6Address = (uri.HostNameType == UriHostNameType.IPv6 &&
+                uri.IdnHost.IndexOf("fe80") != 0);


Alternatively, we can use IPAddress.IsIPv6LinkLocal Property, but that requirs convert uri to IPAddress first, which is an expensive operation.

The logic here to determine isNonLLAIPv6Address is similar to IPAddress.IsIPv6LinkLocal implementation. (I should change to case insensitive IndexOf).

If anything it must start with the prefix: fe80::/10 The IndexOf will try to find it anywhere in the string, right?
The documentation says uri.IdnHost is string suitable for DNS lookup.
It should not include [] IMHO but the constructed URL should e.g. "[fe80::1%22]"

The IndexOf will try to find it anywhere in the string, right?

Yes, but here is intentionally checked for the index != 0 case, to make sure that it's not IPv6 LLA.

It should not include [] IMHO but the constructed URL should e.g. "[fe80::1%22]"

I think so. Will make URL to contain [] and add more test cases in next commit.

Yes, but here is intentionally checked for the index != 0 case, to make sure that it's not IPv6 LLA.

But that still does the extra work to find it anywhere in the string, whereas StartsWith will bail if it doesn't match the beginning.

stephentoub · 2018-04-10T12:42:15Z

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

+                // From RFC 4291, IPv6 LLA has formate like: fe80::/10.
+                await WriteAsciiStringAsync((uri.HostNameType == UriHostNameType.IPv6 &&
+                    uri.IdnHost.IndexOf("fe80") != 0) ?
+                        uri.Host : uri.IdnHost).ConfigureAwait(false);


Why are we limiting it to fe80? If the user specifies a scope for something else, we want to ignore it? e.g.

using System; class Program { static void Main() { Uri uri = new Uri("http://[fec0::1%123]:81/"); Console.WriteLine(uri.Host); Console.WriteLine(uri.IdnHost); } }

Why not just do:

if (uri.HostNameType == UriHostNameType.IPv6) { await WriteByteAsync((byte)'[').ConfigureAwait(false); await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false); await WriteByteAsync((byte)']').ConfigureAwait(false); } else { await WriteAsciiStringAsync(uri.IdnHost).ConfigureAwait(false); }

?

Why are we limiting it to fe80? If the user specifies a scope for something else, we want to ignore it?

I don't know, I will investigate it.

Why not just do:

This is definitely a better way. I didn't do that first because I want to make the change consistent, like below isNonLLAIPv6Address logic.

According to the RFC for IPv6, "interface id" can be present for non LLA IPv6 addresses.

https://tools.ietf.org/html/rfc4291

It is required to be present for LLA because fe80:: addresses can be reused across multiple interfaces on a machine. So, the interface-id is used to differentiate them.

Most common uses of IPv6 don't include the interface-id, though, for non-LLA addresses.

stephentoub · 2018-04-10T12:53:12Z

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

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


Can you remind me why we need to distinguish between Host and IdnHost in these connection keys? Is the host from the connection key ever written out anywhere? It looks like "no", in which case why does it matter whether it's wrapped in []?

Can you remind me why we need to distinguish between Host and IdnHost in these connection keys? Is the host from the connection key ever written out anywhere?

The host from the connection key will be written out to the Host header.

We need to distinguish because IdnHost doesn't include [] around IPv6 address, thus the Host header we are sending out for IPv6 address will be missing []. (#28557)

in which case why does it matter whether it's wrapped in []?

As the example in #28557 showed, if it not wrapped in [], for example, Host: ::1:5001. For IPv6 address, it will be hard to tell which part is address, which part is port number.

The host from the connection key will be written out to the Host header.

It doesn't have to be though. This code is executed for every request. In contrast, this code is executed only when we create a new pool:
https://github.com/caesar1995/corefx/blob/22633ac67e1b59ddc058b7e1d700e90e5c202934/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs#L211
and it's that ctor that creates the bytes to be written out. We can pass whatever we want into that ctor, such as the host bytes to use or something the ctor can use to compute the host bytes. We should be keeping the connection key logic as lean as possible.

We should be keeping the connection key logic as lean as possible.

I agree. But the tricky part here is to determine the value we want to pass into that ctor without additional allocation. I will explore this approach to see if able to find efficient way to do so, so that we can make a trade-off decision.

An extra string allocation when constructing the pool is fine if there's no better way. It's not fine when constructing a key.

stephentoub · 2018-04-10T12:57:06Z

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

            else
            {
-                return new HttpConnectionKey(HttpConnectionKind.Http, isIPv6Address ? uri.Host : uri.IdnHost, uri.Port, null, null);
+                return new HttpConnectionKey(HttpConnectionKind.Http, isNonLLAIPv6Address ? uri.Host : uri.IdnHost, uri.Port, null, null);


What about other places we're using IdnHost, like:
https://github.com/caesar1995/corefx/blob/0c08d53b12c4f93e224b447e850746fd1a9b6660/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L385-L389
Do those need to be updated as well?

That's the content in the URL case Tomas has mentioned, I will update them all.

davidsh · 2018-04-11T00:49:48Z

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

+
+                // TODO: #28863 Part 2: Uri.Host LLA (Link-local address) for IPv6 address doesn't contain %number part.
+                // Since scope is mandatory for LLA, we will use uri.IdnHost for IPv6 LLA address.
+                // From RFC 4291, IPv6 LLA has formate like: fe80::/10.


"formate" -> "format"

davidsh · 2018-04-11T00:55:21Z

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

+                // From RFC 4291, IPv6 LLA has formate like: fe80::/10.
+                await WriteAsciiStringAsync((uri.HostNameType == UriHostNameType.IPv6 &&
+                    uri.IdnHost.IndexOf("fe80") != 0) ?
+                        uri.Host : uri.IdnHost).ConfigureAwait(false);


According to the RFC for IPv6, "interface id" can be present for non LLA IPv6 addresses.

https://tools.ietf.org/html/rfc4291

It is required to be present for LLA because fe80:: addresses can be reused across multiple interfaces on a machine. So, the interface-id is used to differentiate them.

Most common uses of IPv6 don't include the interface-id, though, for non-LLA addresses.

caesar-chen · 2018-04-12T05:25:05Z

The new commit redo the product change in pr: #28578 #28740 (while keep the tests of course, nothing is regressed).

The change in above old PRs for SocketsHttpHandler is to use uri.Host instead of uri.IdnHost if the host name type is IPv6, so that square brackets are included around the IPv6 host. The good thing about that change is allocation free. However, there is an issue using uri.Host for IPv6 link-local address: if it's IPv6 LLA address, uri.Host doesn't contain %number part (scope). If we are specifying a LLA IPAddress without scope to establish socket connection, will result in System.Net.Sockets.SocketException : Invalid argument on Linux.

To satisfy both cases, the best way is that we will always use uri.IdnHost for all IPv6 address (as the original code does before PR #28578 and #28740), and append [] around it. The only extra allocation is needed when a new connection pool is created (we don't need to allocate/detect if IPv6 per request, and leave connection key logic untouched).

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

caesar-chen · 2018-04-12T06:14:43Z

I re-enable the disabled GetAsync_IPv6LinkLocalAddressUri_Success and didn't add new test to verify scope is included for IPv6 address in Host header and URL, because that behavior is not consistent across different platforms.

For example, we will use a IPv6 LLA address (with interface-id/scope) to establish the connection, on Framework/Linux; however, content sent to network will not have the scope in it: "GET http://[fe80::e077:c9a3:eeba:b8e9]:8181/ HTTP/", "Host: [fe80::e077:c9a3:eeba:b8e9]:8181"....

The GetAsync_IPv6LinkLocalAddressUri_Success test can prove that connection is successfully established (Previously on Linux, missing scope will result in System.Net.Sockets.SocketException : Invalid argument).

caesar-chen · 2018-04-12T16:44:07Z

Unrelated failure & build stuck.

@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please
@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test OSX x64 Debug Build

caesar-chen · 2018-04-13T16:41:44Z

Failures not related. Ping reviewers @dotnet/ncl

davidsh · 2018-04-13T17:28:41Z

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

            {
                Debug.Assert(_pool.UsingProxy);

+                // TODO: #28863 Part 1: Uri.IdnHost should include [] around IPv6 address.


I would simplify these comments (which are repeated in a few places in this PR).

I would simply say:

// TODO: #28863 Uri.IdnHost is missing '[', ']' characters around IPv6 address. So, we need to add them manually for now.

I would remove all the text in the comments talking about a bug in Uri.Host. It's not necessary to talk about that here because the handler should only be using .IdnHost anyways. The bug with Uri.Host not including %number for LLA already describes that problem in that GitHub issue. It's not relevant for SocketsHttpHandler.

Will change comment.

caesar-chen · 2018-04-13T18:36:39Z

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

caesar-chen · 2018-04-13T20:21:16Z

Resolve merge conflict #29081: https://github.com/dotnet/corefx/pull/29081/files#diff-2675d24b3a3482c87ea2b9a41e2bdfb7R220

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

caesar-chen · 2018-04-13T22:20:50Z

@dotnet-bot test Outerloop OSX x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

…x#28971) * add workaround for ipv6 LLA address * address comment * fix comment Commit migrated from dotnet/corefx@0698997

add workaround for ipv6 LLA address

0c08d53

caesar-chen added the area-System.Net.Http label Apr 10, 2018

caesar-chen added this to the 2.1.0 milestone Apr 10, 2018

caesar-chen self-assigned this Apr 10, 2018

caesar-chen commented Apr 10, 2018

View reviewed changes

stephentoub reviewed Apr 10, 2018

View reviewed changes

davidsh added area-System.Net.Http.SocketsHttpHandler and removed area-System.Net.Http labels Apr 10, 2018

davidsh reviewed Apr 11, 2018

View reviewed changes

address comment

f7bc727

caesar-chen requested a review from a team April 13, 2018 16:41

davidsh reviewed Apr 13, 2018

View reviewed changes

fix comment

90c8be5

davidsh approved these changes Apr 13, 2018

View reviewed changes

Merge branch 'master' into ipv6_lla

1507c26

caesar-chen merged commit 0698997 into dotnet:master Apr 14, 2018

caesar-chen deleted the ipv6_lla branch April 14, 2018 00:34

caesar-chen mentioned this pull request Jan 31, 2020

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

Open

Copilot AI mentioned this pull request Feb 10, 2026

Refine Uri Host property documentation: clarify IdnHost behavior and focus on Host/IdnHost dotnet/dotnet-api-docs#12301

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SocketsHttpHandler: Add workaround for ipv6 LLA address#28971

SocketsHttpHandler: Add workaround for ipv6 LLA address#28971
caesar-chen merged 4 commits intodotnet:masterfrom
caesar-chen:ipv6_lla

caesar-chen commented Apr 10, 2018

caesar-chen Apr 10, 2018 •

edited

Loading

wfurt Apr 10, 2018

caesar-chen Apr 10, 2018

stephentoub Apr 10, 2018

stephentoub Apr 10, 2018

stephentoub Apr 10, 2018

caesar-chen Apr 10, 2018 •

edited

Loading

davidsh Apr 11, 2018 •

edited

Loading

stephentoub Apr 10, 2018

caesar-chen Apr 10, 2018

stephentoub Apr 10, 2018

caesar-chen Apr 11, 2018

stephentoub Apr 11, 2018

stephentoub Apr 10, 2018

caesar-chen Apr 10, 2018

davidsh Apr 11, 2018

davidsh Apr 11, 2018 •

edited

Loading

caesar-chen commented Apr 12, 2018 •

edited

Loading

caesar-chen commented Apr 12, 2018

caesar-chen commented Apr 12, 2018

caesar-chen commented Apr 13, 2018

davidsh Apr 13, 2018 •

edited

Loading

caesar-chen Apr 13, 2018

caesar-chen commented Apr 13, 2018

caesar-chen commented Apr 13, 2018

caesar-chen commented Apr 13, 2018

Labels

5 participants

Conversation

caesar-chen commented Apr 10, 2018

caesar-chen Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

davidsh Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

davidsh Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

caesar-chen commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

caesar-chen commented Apr 12, 2018

caesar-chen commented Apr 12, 2018

caesar-chen commented Apr 13, 2018

davidsh Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Apr 13, 2018

caesar-chen commented Apr 13, 2018

caesar-chen commented Apr 13, 2018

Labels

5 participants

caesar-chen Apr 10, 2018 •

edited

Loading

caesar-chen Apr 10, 2018 •

edited

Loading

davidsh Apr 11, 2018 •

edited

Loading

davidsh Apr 11, 2018 •

edited

Loading

caesar-chen commented Apr 12, 2018 •

edited

Loading

davidsh Apr 13, 2018 •

edited

Loading