Runtime: [Uri] Fix IPv6 address format

[caesar-chen]

Here are the issues in Uri with IPv6 address we may want to fix:

1. [Pending discussion] Uri.IdnHost should include [] around IPv6 address.
   
   
   
   • Currently it returns ::1234, it should return [::1234].
   
   
2. [Originally done with PR dotnet/corefx#29829 but reverted with dotnet/corefx#30062] Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part.
   
   
   
   • Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18].
   
   
   • Note: Uri.IdnHost correctly contains the %number part.
   
   

If we choose to fix these problems, we can undo workarounds in dotnet/corefx#28740, dotnet/corefx#28578, dotnet/corefx#28849 and dotnet/corefx#28971.

/cc: @dotnet/ncl

[stephentoub]

This issue is marked as future, but isn't there a bug here in SocketsHttpHandler we need to fix in 2.1? That we're incorrectly dropping the scope from a link-local address?

[caesar-chen]

but isn't there a bug here in SocketsHttpHandler we need to fix in 2.1?

Yes, we have dotnet/runtime#25726 opened for it, I will be working on it. Opened this issue to track future Uri work.

[stephentoub]

Ah, ok.

[MarcoRossignoli]

@caesar1995 one question LLA start with fe: or fe80: ?

[caesar-chen]

IPv6 LLA starts with fe80. See RFC 4291:

Link-Local addresses have the following format:

   |   10     |
   |  bits    |         54 bits         |          64 bits           |
   +----------+-------------------------+----------------------------+
   |1111111010|           0             |       interface ID         |
   +----------+-------------------------+----------------------------+

IPv6 notation will be like: FE80::/10

[MarcoRossignoli]

Currently it returns [fe::ee:b8e9], it should return [fe::ee:b8e9%18].

@caesar1995 ok i ask this because in you sample for LLA on issue address start with fe.

After this change i think we need also to update sample on docs https://msdn.microsoft.com/en-us/library/system.uri.dnssafehost(v=vs.110).aspx#Anchor_3, again porting on netfx?

[caesar-chen]

ok i ask this because in you sample for LLA on issue address start with fe.

Oops, corrected in sample to avoid confusion. Thanks!

After this change i think...

This is a proposal which need investigation (so don't start working on it yet). If we choose to accept it, then yes we need to do those following steps.

[MarcoRossignoli]

@caesar1995 i did a pass through parsing code, let me know if i can try a PR(if this proposal success).

[rmkerr]

Hey @MarcoRossignoli if you'd like to take this issue in two segments I'm confident that we would accept a fix to part 2 here, since that behavior is clearly a bug. Part 1 requires a little more discussion, so I think splitting the fix up would be best.

[MarcoRossignoli]

@rmkerr sure! I'll work on it! Thank's for consideration!

[MarcoRossignoli]

@rmkerr i'm working on issue and when compile i get compile error

UriRelativeResolutionTest.cs(26,116): error CS0117: 'PlatformDetection' does not contain a definition for 'IsNetfx472OrNewer'

i'm able to go on if remove field, any idea?

[caesar-chen]

Did you try to clean && sync && build the entire corefx project? That attribute is introduced recently.

[rmkerr]

I would pull the latest version of the repo, then clean and build the whole project again. You can find more info on that here: developer guide. You probably last built the test utilities before this PR was merged, adding the property you mentioned.

[MarcoRossignoli]

@caesar1995 @rmkerr my fault...i work on more host, i was convinced to work on updated repo, it all ok!

[ghost]

cc @wfurt, this is also related: https://github.com/dotnet/corefx/issues/27529

[MarcoRossignoli]

@caesar1995 we've merged fix to part 2 you could update issue description.

[karelz]

Thanks @MarcoRossignoli! Top post updated.

[Tratcher]

This last change is now failing in Kestrel tests and we need to make sure we have a consistent story (https://github.com/aspnet/KestrelHttpServer/issues/2637). Note these tests don't use HttpClient directly, only System.Uri and Sockets.

Do you expect the various http clients to send this scope id in the Host header? Walking the spec has an interesting history here.

https://tools.ietf.org/html/rfc7230#section-5.4
Host = uri-host [ ":" port ] ; Section 2.7.1
uri-host =
https://tools.ietf.org/html/rfc3986#section-3.2.2
host = IP-literal / IPv4address / reg-name
IP-literal = "[" ( IPv6address / IPvFuture ) "]"
IPv6address = {... long and messy, but without scope ids ...}
A host identified by an IPv6 literal address is represented inside
the square brackets without a preceding version flag. The ABNF
provided here is a translation of the text definition of an IPv6
literal address provided in [RFC3513]. This syntax does not support
IPv6 scoped addressing zone identifiers.

So no, but...

Updated by https://tools.ietf.org/html/rfc6874#section-2
IP-literal = "[" ( IPv6address / IPv6addrz / IPvFuture ) "]"
ZoneID = 1*( unreserved / pct-encoded )
IPv6addrz = IPv6address "%25" ZoneID

So yes, but then https://tools.ietf.org/html/rfc6874#section-4
An HTTP client, proxy, or other intermediary MUST remove any ZoneID
attached to an outgoing URI, as it has only local significance at the
sending host.

That looks like a pretty definitive No for putting them in Host headers.

Chrome, IE, and Edge do not accept scope Ids in the address bar so they won't send one. Curl.exe (windows) allows the scope but strips it from the Host. HttpClient currently does send a scope id in the Host and the request is rejected by Kestrel with a 400. That looks like a bug you'll need to address either in HttpClient or System.Uri.

[davidsh]

Do you expect the various http clients to send this scope id in the Host header?

What does .NET Framework HttpWebRequest / HttpClient do?

[Tratcher]

.NET 4.6.1 HttpClient strips the scope id from the Host header.

[Tratcher]

With this URI change getting the host without the scopeid is a bit obnoxious.

internal static string GetHost(Uri requestUri)
        {
            var authority = requestUri.Authority;
            if (requestUri.HostNameType == UriHostNameType.IPv6)
            {
                // Make sure there's no % scope id. https://github.com/aspnet/KestrelHttpServer/issues/2637
                var address = IPAddress.Parse(requestUri.Host);
                address = new IPAddress(address.GetAddressBytes()); // Drop scope Id.
                authority = $"[{address}]:{requestUri.Port.ToString(CultureInfo.InvariantCulture)}";
            }
            return authority;
        }

[davidsh]

I think we may need to revert dotnet/corefx#29829 in the Master branch right away and reconsider how best to solve the overall problems.

[rmkerr]

I don't know if we need to revert this immediately or not, but we should definitely discuss the approach. It seems like a case where what's best from a pure URI standpoint conflicts with what's best for the HTTP stack. I'm inclined to say that the HTTP stack takes precedence, but I'd like to hear what others think.

[Tratcher]

What's best for Uri is what's best for it's consumers. What other consumers use Host, Authority, IdnHost, and what are their requirements?
Http can work around it if needed.

[davidsh]

I'm inclined to say that the HTTP stack takes precedence

I tend to agree with this. I think perhaps if we really want a Uri property to return the host portion with scope-ids we might need a new property to do it. Otherwise, we risk breaking more things.

[MarcoRossignoli]

I think perhaps if we really want a Uri property to return the host portion with scope-ids we might need a new property to do it

This could be the safest choice...even if i think it would be a "strange" property, may lead to confusion.

EDIT:

Otherwise, we risk breaking more things.

I mean you have a "big picture" if it's highly likely issues other than "HttpClients" that's okay IMO.

[Tratcher]

Hmm, I just realized this changed the output of ToString as well. .NET ToString does not include the scope, but now core does.

[davidsh]

Hmm, I just realized this changed the output of ToString as well. .NET ToString does not include the scope, but now core does.

I don't think that's necessarily a bad thing. The .OriginalString still has the scope-id. In fact, we've seen an interesting interop issue using UWP apps when System.Uri needs to get converted at the lower layers to WinRT Windows.Foundation.Uri. These uri's with scope-id that are valid System.Uri are broken when Windows.Foundation.Uri tries to re-create them (using .OriginalString). That is a WinRT bug tracking also in GitHub corefx.

[rmkerr]

Agreed -- ideally I think the scope ID should be included in ToString. It's a meaningful part of the URI, and by removing it we change the meaning. Obviously we do have to consider app compat though, so it is worth considering what use cases we might break.

[karelz]

Triage: We may not be able to fix it at all, the goal is to decide in 5.0 timeframe.

[scalablecory]

Ran into this one today!

CC @MihaZupan

[wfurt]

I was thinking about it more and I'm not sure this is a problem other than surprise. According to documentation:

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.

If we put scopeID or '[]` the value will no longer be safe to use for DNS IMHO and additional work would be needed by the caller to make it passable to NameResolution API.

I'm wondering what was the original intention for this property.