Support socks5h proxy using unix sockets #8668

balki · 2022-04-01T16:34:05Z

Usage:

curl -x "socks5h://unix/run/tor/socks" "https://example.com"

Ref: https://curl.se/mail/archive-2021-03/0012.html

This is a proof of concept to make sure it is working. Please take over from here or guide next steps - tests, documentation etc.,

bagder

For tests, I think test case 1435 can serve as good inspiration as it already uses --unix-socket

bagder · 2022-04-02T13:49:44Z

lib/url.c

@@ -2519,6 +2519,20 @@ static CURLcode parse_proxy(struct Curl_easy *data,
  }

  /* now, clone the proxy host name */
+#ifdef USE_UNIX_SOCKETS
+  if(proxytype == CURLPROXY_SOCKS5_HOSTNAME


Is there any reason to limit this to SOCKS5_HOSTNAME and not allow SOCKS4a too?

I thought it may not work as dns resolution is done locally in SOCKS4a using the same function. But after quick test, looks like it should work. I will update for all socks proxies.

Updated to support all socks proxies and not depend on the scheme prefix in proxy string. Please confirm if implementation/syntax is ok. I will then add a test.

bagder · 2022-04-02T13:50:28Z

lib/url.c

@@ -2519,6 +2519,20 @@ static CURLcode parse_proxy(struct Curl_easy *data,
  }

  /* now, clone the proxy host name */
+#ifdef USE_UNIX_SOCKETS
+  if(proxytype == CURLPROXY_SOCKS5_HOSTNAME
+      && strncmp("unix/", proxy + 10, 5) == 0) { /* len("socks5h://") = 10 */


The host name might start with that scheme, the type could also be set with a separate option and then the proxy name starts with the host name. This cannot assume the first 10 bytes are known.

I also find it a little unfortunate to reserve the unix host name like this, as there might actually be a proxy in use that is named like that. Maybe we should instead make the --unix-socket option accept a socks... prefix?

The host name might start with that scheme, the type could also be set with a separate option and then the proxy name starts with the host name. This cannot assume the first 10 bytes are known.

I will update to handle that case.

there might actually be a proxy in use that is named like that.

In that case, unix/path cannot be a valid URI for a socks proxy as it can only have host:port syntax. Since I am checking for unix/, I think it should not affect anyone having a server named unix with a socks proxy running in it.

Maybe we should instead make the --unix-socket option accept a socks... prefix

I wish to have the ability to set the proxy via environment variable like ALL_PROXY=socks5h://unix/run/tor/socks, so that programs that internally use curl does not have to change the command line.

dfandrich · 2022-04-02T20:39:02Z

How about a syntax like this: ALL_PROXY=socks5h:///unix/run/tor/socks i.e. an extra slash, making the host part of the URL empty. This is similar to how a file: URL would refer to the same file.

bagder · 2022-04-02T21:54:50Z

i.e. an extra slash, making the host part of the URL empty. This is similar to how a file: URL would refer to the same file.

Unfortunately, the URL parser accepts one, two or three slashes where there should be two so three slashes will be accepted as if they were two:

$ curl -x socks5h:///moo/hello localhost
curl: (7) Failed to connect to moo port 1080 after 0 ms: Connection refused

Another option is to use a short but otherwise illegal host name. For example just two dots (..):

$ curl -x socks5h://../run/tor/socks localhost
curl: (6) Could not resolve host: ..

dfandrich · 2022-04-02T22:52:56Z

On Sat, Apr 02, 2022 at 02:55:00PM -0700, Daniel Stenberg wrote: Unfortunately, the URL parser accepts one, two or three slashes where there should be two so three slashes will be accepted as if they were two:

That sounds wrong—it goes against RFC 1738 and RFC 8089, anyway. Is this a WHATWG thing?

bagder · 2022-04-03T09:40:25Z

That sounds wrong—it goes against RFC 1738 and RFC 8089, anyway. Is this a WHATWG thing?

Yes. 😢 The lax nature of WHATWG's URL spec makes such URLs occasionally get used in the wild, so this is our adjustment to accept such URLs better.The WHATWG spec actually allows any amount of slashes (even zero), and both backslashes and slashes (mixed) but curl only allows regular forward slashes and only one, two or three of them.

balki · 2022-04-04T13:00:37Z

For tests, I think test case 1435 can serve as good inspiration as it already uses --unix-socket

I looked at the test setup. I think the following changes are required along with writing a testcase.

Update this server to support taking unixsocket as argument. https://github.com/curl/curl/blob/master/tests/server/socksd.c
Pass the option from here: https://github.com/curl/curl/blob/master/tests/runtests.pl

Q: Where should the temporary socket be created and how/when should it be cleaned up?

With last update, If the hostname is unix and path is not just '/', the path is considered to to be a unix socket.

Please correct if I am missing anything.

balki · 2022-04-19T18:57:45Z

@bagder Please take a look again.

Changed syntax to socks5h://localhost/path/to/sock
Added tests

I don't understand the torture test failure but it does not fail consistently.

bagder · 2022-05-08T21:29:11Z

The torture test fail of 1467 means that there is a memory/resource leak somewhere in the exit path.

bagder

There is no added documentation?

balki · 2022-05-10T19:26:29Z

The torture test fail of 1467 means that there is a memory/resource leak somewhere in the exit path.

Reviewed again. Could not find the leak.. Can you please share instructions to reproduce failure locally? I am on a ubuntu desktop. make tests-full pass fine.

Added documentation for the command line flags.

lib/url.c

bagder · 2022-05-10T21:28:18Z

The clue is torture tests. It's a special way to run the tests, which verifies the exit paths heavily.

Reviewed again. Could not find the leak.. Can you please share instructions to reproduce failure locally? I am on a ubuntu desktop. make tests-full pass fine.

$ ./configure --enable-debug (+ c-ares or non-threaded resolver)
$ make
$ cd tests
$ make
$ ./runtests -n -t 1467

This then makes all allocations in the test fail, one by one and it verifies that it never leaks memory in any case.

The 1467 failure you got looked like this:

test 1467...[HTTP GET via SOCKS5 proxy via unix sockets]
 144 functions found, but only fail 25 (17.36%)
** MEMORY FAILURE
Leak detected: memory still allocated: 10 bytes
At 7f7f53e04bc8, there's 10 bytes.
 allocated by escape.c:144
LIMIT urlapi.c:1415 strdup reached memlimit
 Failed on function number 93 in test.
 invoke with "-t93" to repeat this single case.

It shows that escape.c:144 allocated a line that was never freed. It was when fallible function number 93 was made to fail. If you wanted reproduce that exact fail number, you can do this:

./runtests -t94 1467

I usually first try with -t to run all loops torture to see that it reproduces, it might not be the exact same number in a local build. Also, use -n to switch off valgrind since running with valgrind makes every loop take several times longer, which can make this test really slow. Using valgrind in the last step when you re-run a single number is very useful because valgrind can help you pinpoint the entire leak call stack much better.

bagder · 2022-05-10T21:30:20Z

if you still have problems rerunning the test, I can have a go at it soon.

balki · 2022-05-10T23:00:47Z

if you still have problems rerunning the test, I can have a go at it soon.

Thank you. Was able to reproduce and fix it. Waiting for CI run.

balki · 2022-05-11T01:00:44Z

Fixed torture test and added documentation. Please review. curl/check failure does not show any logs or errors except for message "Build Failed". Please help understand the error.

bagder

This looks very good and I'm ready to merge. Just one thing left that I can think of:

You added docs for the command line tool but not for the libcurl option that actually passes the string to libcurl. CURLOPT_PROXY. It needs to be similarly extended so that users can learn how to specify a socks proxy over unix domain socket with it.

Usage: curl -x "socks5h://localhost/run/tor/socks" "https://example.com" Updated runtests.pl to run a socksd server listening on unix socket Added tests test1467 test1468 Added documentation for proxy command line option and socks proxy options

balki · 2022-05-17T19:40:26Z

This looks very good and I'm ready to merge. Just one thing left that I can think of:

You added docs for the command line tool but not for the libcurl option that actually passes the string to libcurl. CURLOPT_PROXY. It needs to be similarly extended so that users can learn how to specify a socks proxy over unix domain socket with it.

Thank you. Updated CURLOPT_PROXY documentation.

bagder · 2022-05-19T13:36:19Z

Thanks!

bagder added the connecting & proxies label Apr 1, 2022

bagder reviewed Apr 2, 2022

View reviewed changes

balki force-pushed the master branch from 46aa66b to 186a6a0 Compare April 2, 2022 22:20

balki mentioned this pull request Apr 9, 2022

Refactor tests/server/socksd.c to support --unix-socket #8687

Closed

balki force-pushed the master branch 2 times, most recently from 3a6e94b to 9c68b8e Compare April 19, 2022 15:05

balki requested a review from bagder April 19, 2022 18:56

bagder requested changes May 8, 2022

View reviewed changes

balki force-pushed the master branch from 9c68b8e to 45a0010 Compare May 10, 2022 16:29

balki requested a review from bagder May 10, 2022 19:26

bagder reviewed May 10, 2022

View reviewed changes

lib/url.c Outdated Show resolved Hide resolved

lib/url.c Outdated Show resolved Hide resolved

lib/url.c Outdated Show resolved Hide resolved

lib/url.c Outdated Show resolved Hide resolved

balki force-pushed the master branch from 69f6a04 to 61458b6 Compare May 10, 2022 22:48

balki requested a review from bagder May 11, 2022 00:54

bagder reviewed May 17, 2022

View reviewed changes

Balakrishnan Balasubramanian added 3 commits May 17, 2022 09:06

Fix memory leak from torture test

af76adb

Add documentation to CURLOPT_PROXY

c62b318

balki force-pushed the master branch from 61458b6 to c62b318 Compare May 17, 2022 13:07

balki requested a review from bagder May 17, 2022 19:40

bagder approved these changes May 19, 2022

View reviewed changes

bagder closed this in dfa84a0 May 19, 2022

kofalt mentioned this pull request May 24, 2022

FR: Make /var/run/tailscale/tailscaled.sock also support SOCKS5 tailscale/tailscale#4657

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support socks5h proxy using unix sockets #8668

Support socks5h proxy using unix sockets #8668

balki commented Apr 1, 2022

bagder left a comment

bagder Apr 2, 2022

balki Apr 2, 2022

balki Apr 2, 2022

bagder Apr 2, 2022

balki Apr 2, 2022

dfandrich commented Apr 2, 2022 via email

bagder commented Apr 2, 2022

dfandrich commented Apr 2, 2022 via email

bagder commented Apr 3, 2022

balki commented Apr 4, 2022

balki commented Apr 19, 2022 •

edited

bagder commented May 8, 2022

bagder left a comment

balki commented May 10, 2022

bagder commented May 10, 2022

bagder commented May 10, 2022

balki commented May 10, 2022

balki commented May 11, 2022

bagder left a comment

balki commented May 17, 2022

bagder commented May 19, 2022

Support socks5h proxy using unix sockets #8668

Support socks5h proxy using unix sockets #8668

Conversation

balki commented Apr 1, 2022

bagder left a comment

Choose a reason for hiding this comment

bagder Apr 2, 2022

Choose a reason for hiding this comment

balki Apr 2, 2022

Choose a reason for hiding this comment

balki Apr 2, 2022

Choose a reason for hiding this comment

bagder Apr 2, 2022

Choose a reason for hiding this comment

balki Apr 2, 2022

Choose a reason for hiding this comment

dfandrich commented Apr 2, 2022 via email

bagder commented Apr 2, 2022

dfandrich commented Apr 2, 2022 via email

bagder commented Apr 3, 2022

balki commented Apr 4, 2022

balki commented Apr 19, 2022 • edited

bagder commented May 8, 2022

bagder left a comment

Choose a reason for hiding this comment

balki commented May 10, 2022

bagder commented May 10, 2022

bagder commented May 10, 2022

balki commented May 10, 2022

balki commented May 11, 2022

bagder left a comment

Choose a reason for hiding this comment

balki commented May 17, 2022

bagder commented May 19, 2022

balki commented Apr 19, 2022 •

edited