SOCKS4/4a/5 proxy support in SocketsHttpHandler #48883

huoyaoyuan · 2021-02-28T16:07:52Z

@MihaZupan Opening earlier for opinions. Code not normalized (like exceptions).

I'm not sure how to implement the GSSAPI authentication (https://tools.ietf.org/html/rfc1961). It seems covering more scope.

Manually tested with a local socks5 proxy and working. However, I don't have an instance running with authentication support. No idea about how to test it in CI.

ghost · 2021-02-28T16:08:01Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

#17740

@MihaZupan Opening earlier for opinions. Code not normalized (like exceptions).

I'm not sure how to implement the GSSAPI authentication (https://tools.ietf.org/html/rfc1961). It seems covering more scope.

Manually tested with a local socks5 proxy and working. However, I don't have an instance running with authentication support. No idea about how to test it in CI.

Author:	huoyaoyuan
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

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

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

MihaZupan · 2021-03-01T03:48:34Z

@MihaZupan Opening earlier for opinions. Code not normalized (like exceptions).

Thanks for working on this!

I'm not sure how to implement the GSSAPI authentication (https://tools.ietf.org/html/rfc1961). It seems covering more scope.

I only had support for username/pass authentiation in mind. I have not seen a single request for GSSAPI support in the past (from my HttpToSocks5Proxy project) - I would treat it as out-of-scope for this PR.
It turns out that socks proxy servers often don't end up requesting authentication at all.

I realize this is a draft PR, but would you be interested in adding support for Socks4(a) as well? It should only affect the implementation in the SocksHelper (based on the scheme in the proxy uri).
There are for some edge-cases, like having to resolve the IP address ahead-of-time for Socks4, since it does not support domain names.

huoyaoyuan · 2021-03-01T04:38:02Z

I realize this is a draft PR, but would you be interested in adding support for Socks4(a) as well?

Sure. I opened it early just to get some opinion.

huoyaoyuan · 2021-03-01T06:06:31Z

4/4a added.
SOCKS6 has been draft for a long time and I'm also interested. Does it need to be draft PR until finalization?

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

huoyaoyuan · 2021-03-02T07:18:24Z

Since SOCKS6 is still draft, more complex, and may not have a existing server, I think it shouldn't be supported now.

Can we start to normalize this, for example which exception to throw?

MihaZupan

Since SOCKS6 is still draft, more complex, and may not have a existing server, I think it shouldn't be supported now.

Agreed.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

MihaZupan

The host could be an IP address - we have to check for this and use the appropriate address type for requests.

If IPv4, use that address type. Socks4a need not be used (it will be the same as 4).
If IPv6:

Socks5: Use the IPv6 address type
Socks4/4a: check IPAddress.IsIPv4MappedToIPv6 - if it is not, can we still use the Socks4a and pretend it's a domain name?

If it's a domain name:

Socks5/Socks4a: Use the domain name
Socks4: Try to resolve to IPv4

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/SocksProxyTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

MihaZupan · 2021-04-16T11:50:50Z

Failures are all #51346

scalablecory · 2021-04-17T17:06:29Z

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/LoopbackSocksServer.cs

+                string username = Encoding.UTF8.GetString(usernameBuffer.AsSpan(0, usernameBytes));
+                if (username != _username)
+                {
+                    ns.WriteByte(4);


no need for async?

It's a test only server. I just want it to be simple.

There isn't a helper to write 1 byte asynchronously. So I'm not willing to bother with it.

scalablecory · 2021-04-17T17:06:34Z

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/LoopbackSocksServer.cs

+                }
+            }
+
+            ns.WriteByte(4);


no need for async?

scalablecory · 2021-04-21T15:17:08Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

+            }
+            catch
+            {
+                Debug.Assert(Encoding.UTF8.GetByteCount(chars) > 255);


This assert might fail if you pass in invalid Unicode.

How do you mean? Afaik the default Encoding.UTF8 doesn't throw but instead replaces chars with U+FFFD

scalablecory · 2021-04-21T15:17:40Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

+            while (buffer.Length != 0)
+            {
+                int bytesRead = async
+                    ? await stream.ReadAsync(buffer).ConfigureAwait(false)


Should this have cancellation support?

This is using the same approach as HttpConnection, where we register for cancellation once and have the callback dispose the stream. All other calls to stream Read/Write don't pass the CT and instead rely on the Dispose to wake them up.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

Line 34 in 95400e2

using (cancellationToken.Register(s => ((Stream)s!).Dispose(), stream))

scalablecory · 2021-04-21T15:18:33Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs

+                    totalLength += hostLength + 1;
+                }
+
+                await WriteAsync(stream, buffer.AsMemory(0, totalLength), async).ConfigureAwait(false);


Same here, does this need cancellation?

scalablecory · 2021-04-21T15:19:23Z

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/LoopbackSocksServer.cs

+        {
+            if (password != null && username == null)
+            {
+                throw new ArgumentException("Password must be used together with username.", nameof(password));


Is it valid to have a non-null username and a null password?

Yes in case of socks4(a), which only uses the username as an identifier, but doesn't actually do user/pass auth.

rfc1929 spec implies username and password shouldn't be 0 length. Should we add a guard for that? Looks overkill.

I wouldn't bother, we can let the server decide whether to reject it.

scalablecory

lgtm once feedback addressed.

MihaZupan · 2021-04-22T12:31:19Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-04-22T12:31:34Z

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan · 2021-04-22T17:21:45Z

Thanks @huoyaoyuan!

karelz · 2021-04-22T17:57:38Z

Thank you @huoyaoyuan for pushing it through and @MihaZupan for your help!

huoyaoyuan added 6 commits February 28, 2021 00:31

Separate method for proxy scheme validation.

5f2407a

Pass socks connection kind.

7a70c33

Unauthorized socks5 connection.

d0aa8fc

Username and password auth.

b091355

Fix response address.

98a1d12

Fix proxyUri value and assertion.

c4cdd8e

dotnet-issue-labeler bot added the area-System.Net.Http label Feb 28, 2021

huoyaoyuan commented Feb 28, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocksHelper.cs Outdated Show resolved Hide resolved

huoyaoyuan added 3 commits March 1, 2021 13:04

Use HttpConnectionKind for SOCKS.

b5e3701

Handle more connection kind assertions.

15b2a8f

SOCKS4/4a support.

56bf28b

huoyaoyuan changed the title ~~Draft implementation of SOCKS5 proxy support~~ Draft implementation of SOCKS4/4a/5 proxy support Mar 1, 2021

MihaZupan reviewed Mar 1, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Show resolved Hide resolved

Move version selection into SocksHelper.

a2a80f7

Base automatically changed from master to main March 1, 2021 09:08

MihaZupan reviewed Mar 2, 2021

View reviewed changes

huoyaoyuan added 2 commits March 2, 2021 19:26

Call sync version of read write.

1d7fa76

Cancellation by disposing stream.

44cce2e

MihaZupan reviewed Mar 2, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 2, 2021

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

huoyaoyuan added 5 commits March 3, 2021 17:08

Dispose cancellation registration.

140bbc1

IP addressing for SOCKS5.

694b869

IP addressing for SOCKS4.

e68224a

Wrap write method.

24e1e2d

Cancellation and optimization.

d1a8eae

huoyaoyuan commented Apr 15, 2021

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/SocksProxyTest.cs Show resolved Hide resolved

Expand test matrix to include Sync HTTP1

0ae546d

scalablecory reviewed Apr 15, 2021

View reviewed changes

MihaZupan added 6 commits April 16, 2021 07:22

Read received bytes before returning error response in Socks4 loopback

b06f145

Use named bool arguments

7f20191

Improve exception messages

2ff779a

!IsEmpty => Length != 0

c08e717

Improve exception messages 2

52df362

Avoid enforing Socks4 VN value

95400e2

MihaZupan approved these changes Apr 16, 2021

View reviewed changes

MihaZupan requested a review from scalablecory April 16, 2021 08:52

scalablecory reviewed Apr 17, 2021

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/Socks/LoopbackSocksServer.cs

}

}

ns.WriteByte(4);

Copy link

Contributor

scalablecory Apr 17, 2021

Choose a reason for hiding this comment

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

no need for async?

scalablecory reviewed Apr 21, 2021

View reviewed changes

scalablecory approved these changes Apr 21, 2021

View reviewed changes

MihaZupan merged commit 32d1a25 into dotnet:main Apr 22, 2021

huoyaoyuan deleted the socks-proxy branch April 22, 2021 17:56

karelz added this to the 6.0.0 milestone May 20, 2021

joelverhagen mentioned this pull request May 21, 2021

Add support for Tor HTTPTunnelPort joelverhagen/TorSharp#75

Closed

MihaZupan mentioned this pull request Jun 10, 2021

What's new in .NET 6 Preview 5 dotnet/core#6099

Closed

AbeniMatteo mentioned this pull request Jun 19, 2021

Consider cross platform managed http to socks proxy joelverhagen/TorSharp#36

Closed

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOCKS4/4a/5 proxy support in SocketsHttpHandler #48883

SOCKS4/4a/5 proxy support in SocketsHttpHandler #48883

huoyaoyuan commented Feb 28, 2021 •

edited by MihaZupan

Loading

ghost commented Feb 28, 2021

MihaZupan commented Mar 1, 2021

huoyaoyuan commented Mar 1, 2021

huoyaoyuan commented Mar 1, 2021

huoyaoyuan commented Mar 2, 2021

MihaZupan left a comment

MihaZupan left a comment

MihaZupan commented Apr 16, 2021

scalablecory Apr 17, 2021

huoyaoyuan Apr 18, 2021

huoyaoyuan Apr 22, 2021

scalablecory Apr 17, 2021

scalablecory Apr 21, 2021

MihaZupan Apr 21, 2021

scalablecory Apr 21, 2021

MihaZupan Apr 21, 2021

scalablecory Apr 21, 2021

scalablecory Apr 21, 2021

MihaZupan Apr 21, 2021

huoyaoyuan Apr 22, 2021

MihaZupan Apr 22, 2021

scalablecory left a comment

MihaZupan commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

MihaZupan commented Apr 22, 2021

karelz commented Apr 22, 2021

SOCKS4/4a/5 proxy support in SocketsHttpHandler #48883

SOCKS4/4a/5 proxy support in SocketsHttpHandler #48883

Conversation

huoyaoyuan commented Feb 28, 2021 • edited by MihaZupan Loading

ghost commented Feb 28, 2021

MihaZupan commented Mar 1, 2021

huoyaoyuan commented Mar 1, 2021

huoyaoyuan commented Mar 1, 2021

huoyaoyuan commented Mar 2, 2021

MihaZupan left a comment

Choose a reason for hiding this comment

MihaZupan left a comment

Choose a reason for hiding this comment

MihaZupan commented Apr 16, 2021

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

Choose a reason for hiding this comment

scalablecory left a comment

Choose a reason for hiding this comment

MihaZupan commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

MihaZupan commented Apr 22, 2021

karelz commented Apr 22, 2021

huoyaoyuan commented Feb 28, 2021 •

edited by MihaZupan

Loading