Fix proxied ipv6 address request formatting and fix port handling for proxied IP address on SocketsHttpHandler #28740

caesar-chen · 2018-04-02T22:24:16Z

Fix IPv6 missing [] in proxied request.
Fix missing port number in the request.

If default ports 80 (for http connection) and 443 (for https connection) are specified for proxied request, 80 will be stripped off, while 443 will not. Because for https scheme, it will try to tunnel through the proxy (CONNECT), and port will not be stripped off.

Tests are added to document the behavior.

Fix: #28609 (moved to dotnet/runtime#25677)

…address

caesar-chen · 2018-04-02T22:25:12Z

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

davidsh · 2018-04-03T00:56:20Z

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

+                        if (!request.RequestUri.IsDefaultPort)
+                        {
+                            await WriteByteAsync((byte)':').ConfigureAwait(false);
+                            await WriteDecimalInt32Async(request.RequestUri.Port).ConfigureAwait(false);


It seem costly to separate out the ":" character and the small number of decimal digits of the port number into separate async operations requiring Task's to be created and awaited, etc.

In fact, this entire request line construction involves too many async Task-based operations. It probably would be more efficient to buffer all these bytes into a single async Write call.

You don't need to block on this for the PR but we should investigate this for future perf gains.

cc: @geoffkizer

We do want to revisit this, as there is overhead here we can likely do away with. Note, though, that on the fast path here, we don't actually create any tasks; these methods end up returning an already completed task, such that nothing is allocated and the await sees IsCompleted==true and skips hooking up any callbacks. That said, it's still extra code being executed that we could benefit from doing away with.

We may also want to optimize here. WriteHostHeaderAsync

davidsh · 2018-04-03T01:04:20Z

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

+            {
+                connectionAccepted = true;
+                List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();
+                Assert.Contains($"GET {ipAddress}/ HTTP/1.1", headers);


I wouldn't expect this test to ever work. The "GET" line sent thru a proxy for a non-tunneled request would look more like this:

GET http://servername/ HTTP/1.1 Host: servername

etc.

So, you need to check for full URI with scheme such as:

GET http://321.123.456.789:8080/ HTTP/1.1
Host: 321.123.456.789:8080

See: https://tools.ietf.org/html/rfc7230#section-5.3.2

When making a request to a proxy, other than a CONNECT or server-wide OPTIONS request (as detailed below), a client MUST send the target URI in absolute-form as the request-target.... An example absolute-form of request-line would be:
GET http://www.example.org/pub/WWW/TheProject.html HTTP/1.1

It looks like the code adds the "http://" part above. But yeah, it's confusing.

Will rename the variable.

geoffkizer · 2018-04-03T01:57:24Z

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

+        [InlineData("[::1234]:8080")]
+        public async Task ProxiedIPAddressRequest_NotDefaultPort_CorrectlyFormatted(string host)
+        {
+            string ipAddress = "http://" + host;


"ipAddress" is confusing; call this "uri" or something like that.

Will change.

geoffkizer · 2018-04-03T02:06:30Z

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

@@ -497,6 +498,91 @@ public static IEnumerable<object[]> GetAsync_IPBasedUri_Success_MemberData()
            Assert.True(connectionAccepted);
        }

+        [Theory]
+        [InlineData("321.123.456.789")]


This is kind of weird. Do you mean for this to be a valid IPv4 address, or not?

and similarly below

Any non-existent ipv4 endpoint.

We only care about the traffic through proxy. I think if the endpoint doesn't exist, we can save time connecting to it. I could be wrong.

Do you have any suggestions on generating IPv4 address?

Why this should not exist? We should be able to verify CONNECT parameter without actually fetching the content, right? If you really want to try failing connect you can probably use multicast address or address from special blocks (like 127.0.0.127)

As @wfurt said, we're not actually using the IPv4 address, so it can be anything.

caesar-chen · 2018-04-03T22:05:04Z

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

stephentoub · 2018-04-04T15:33:14Z

@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-04T18:30:28Z

Failures unrelated.

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

caesar-chen · 2018-04-04T19:51:33Z

OSX failure: #21037
Linux: tests passed successfully, not sure why showed catastrophic failure. (cc: @rmkerr )

2018-04-04 19:09:32,958: INFO: proc(54): run_and_log_output: Output: === TEST EXECUTION SUMMARY ===
2018-04-04 19:09:32,967: INFO: proc(54): run_and_log_output: Output:    System.Runtime.Extensions.Tests  Total: 5532, Errors: 0, Failed: 0, Skipped: 15, Time: 9.754s
2018-04-04 19:09:33,127: INFO: proc(54): run_and_log_output: Output: ~/dotnetbuild/work/b9c3c002-cfe7-4c4c-897f-0ea854f49fef/Work/cb486b70-9fc8-4a18-a782-0c4273188742/Unzip
2018-04-04 19:09:33,128: INFO: proc(54): run_and_log_output: Output: ----- end 19:09:33 ----- exit code 0 ----------------------------------------------------------
2018-04-04 19:09:33,129: INFO: proc(54): run_and_log_output: Output: exit code 0 means Exited Successfully
2018-04-04 19:09:33,130: INFO: proc(54): run_and_log_output: Output: Looking around for any Linux dump...
2018-04-04 19:09:33,132: INFO: proc(54): run_and_log_output: Output: Looking for files matching core.36748 core.36756 ...
2018-04-04 19:09:33,132: INFO: proc(54): run_and_log_output: Output: Printing info from core file core.36748
2018-04-04 19:09:33,132: INFO: proc(54): run_and_log_output: Output: ----- start =============== GDB Output =====================================================

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

caesar-chen · 2018-04-05T05:50:27Z

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

caesar-chen · 2018-04-05T16:41:16Z

CI is green now.

@dotnet/ncl @stephentoub PTAL

davidsh · 2018-04-05T16:45:25Z

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

+        {
+            // Https proxy request will use CONNECT tunnel, even the default 443 port is specified, it will not be stripped off.
+            string expectedAddressUri = "corefx-net.cloudapp.net:443";
+            string addressUri = "https://corefx-net.cloudapp.net:443/";


We usually don't hard-code these external server addresses.

You should use the information in the Common\test\ files, i.e.
https://github.com/dotnet/corefx/blob/master/src/Common/tests/System/Net/Configuration.Http.cs#L11

Will change.

I'm also thinking about changing the inline test data to DefaultAzureServer in ProxiedRequest_DefaultPort_PortStrippedOffInUri. Not sure if this will work, I will explore it.

InlineData might not work. But you could change that to a MemberData and use a method that does "yield return" for each test case.

davidsh · 2018-04-05T16:49:38Z

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

+        public async Task ProxyTunnelRequest_PortSpecified_NotStrippedOffInUri()
+        {
+            // Https proxy request will use CONNECT tunnel, even the default 443 port is specified, it will not be stripped off.
+            string expectedAddressUri = "corefx-net.cloudapp.net:443";


nit: And strictly speaking, expectedAddressUri isn't really a Uri. It is a string consisting of hostname and port. So, perhaps a better name for this local variable?

I would use "requestTarget" since that is the term used in the RFC:
https://tools.ietf.org/html/rfc7231#page-30

A client sending a CONNECT request MUST send the authority form of
request-target (Section 5.3 of [RFC7230]); i.e., the request-target
consists of only the host name and port number of the tunnel
destination, separated by a colon. For example,
CONNECT server.example.com:80 HTTP/1.1
Host: server.example.com:80

Will change the naming.

caesar-chen · 2018-04-05T19:37:35Z

@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

[EDIT]
The new commit changed test data naming/format. Re-run outerloop tests just in case.

caesar-chen · 2018-04-05T22:12:23Z

Failures unrelated.

… proxied IP address on SocketsHttpHandler (dotnet#28740) * fix proxied ipv6 address request format and fix port handling for ip address * address feedback * change naming for test data

… proxied IP address on SocketsHttpHandler (dotnet/corefx#28740) * fix proxied ipv6 address request format and fix port handling for ip address * address feedback * change naming for test data Commit migrated from dotnet/corefx@18d685f

fix proxied ipv6 address request format and fix port handling for ip …

0d89103

…address

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

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

caesar-chen self-assigned this Apr 2, 2018

caesar-chen requested review from stephentoub and a team April 2, 2018 22:24

caesar-chen requested a review from geoffkizer April 2, 2018 22:28

karelz approved these changes Apr 2, 2018

View reviewed changes

davidsh reviewed Apr 3, 2018

View reviewed changes

geoffkizer reviewed Apr 3, 2018

View reviewed changes

address feedback

859c0d5

davidsh reviewed Apr 5, 2018

View reviewed changes

change naming for test data

3e7f564

davidsh approved these changes Apr 5, 2018

View reviewed changes

caesar-chen merged commit 18d685f into dotnet:master Apr 5, 2018

caesar-chen deleted the ipaddr_proxy branch April 5, 2018 22:17

caesar-chen mentioned this pull request Apr 12, 2018

SocketsHttpHandler: Add workaround for ipv6 LLA address #28971

Merged

caesar-chen mentioned this pull request Jan 31, 2020

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix proxied ipv6 address request formatting and fix port handling for proxied IP address on SocketsHttpHandler #28740

Fix proxied ipv6 address request formatting and fix port handling for proxied IP address on SocketsHttpHandler #28740

caesar-chen commented Apr 2, 2018 •

edited by karelz

Loading

caesar-chen commented Apr 2, 2018

davidsh Apr 3, 2018 •

edited

Loading

stephentoub Apr 3, 2018

caesar-chen Apr 3, 2018 •

edited

Loading

davidsh Apr 3, 2018 •

edited

Loading

davidsh Apr 3, 2018

geoffkizer Apr 3, 2018

caesar-chen Apr 3, 2018

geoffkizer Apr 3, 2018

caesar-chen Apr 3, 2018

geoffkizer Apr 3, 2018

geoffkizer Apr 3, 2018

caesar-chen Apr 3, 2018

wfurt Apr 3, 2018

geoffkizer Apr 3, 2018

caesar-chen commented Apr 3, 2018

stephentoub commented Apr 4, 2018

caesar-chen commented Apr 4, 2018

caesar-chen commented Apr 4, 2018

caesar-chen commented Apr 5, 2018

caesar-chen commented Apr 5, 2018

davidsh Apr 5, 2018 •

edited

Loading

caesar-chen Apr 5, 2018

davidsh Apr 5, 2018

davidsh Apr 5, 2018

davidsh Apr 5, 2018 •

edited

Loading

caesar-chen Apr 5, 2018

caesar-chen commented Apr 5, 2018 •

edited

Loading

caesar-chen commented Apr 5, 2018

Fix proxied ipv6 address request formatting and fix port handling for proxied IP address on SocketsHttpHandler #28740

Fix proxied ipv6 address request formatting and fix port handling for proxied IP address on SocketsHttpHandler #28740

Conversation

caesar-chen commented Apr 2, 2018 • edited by karelz Loading

caesar-chen commented Apr 2, 2018

davidsh Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

davidsh Apr 3, 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

Choose a reason for hiding this comment

caesar-chen commented Apr 3, 2018

stephentoub commented Apr 4, 2018

caesar-chen commented Apr 4, 2018

caesar-chen commented Apr 4, 2018

caesar-chen commented Apr 5, 2018

caesar-chen commented Apr 5, 2018

davidsh Apr 5, 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

davidsh Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented Apr 5, 2018 • edited Loading

caesar-chen commented Apr 5, 2018

caesar-chen commented Apr 2, 2018 •

edited by karelz

Loading

davidsh Apr 3, 2018 •

edited

Loading

caesar-chen Apr 3, 2018 •

edited

Loading

davidsh Apr 3, 2018 •

edited

Loading

davidsh Apr 5, 2018 •

edited

Loading

davidsh Apr 5, 2018 •

edited

Loading

caesar-chen commented Apr 5, 2018 •

edited

Loading