Page MenuHomeFreeBSD

Added SOCKS5 support to libfetch
ClosedPublic

Authored by farhan_farhan.codes on Jan 21 2019, 12:26 AM.
Tags
None
Referenced Files
F86525971: D18908.id67584.diff
Fri, Jun 21, 4:00 PM
Unknown Object (File)
Mon, Jun 10, 2:57 PM
Unknown Object (File)
Mon, Jun 10, 2:50 AM
Unknown Object (File)
Thu, Jun 6, 8:12 PM
Unknown Object (File)
Sat, May 25, 4:44 AM
Unknown Object (File)
May 22 2024, 6:13 PM
Unknown Object (File)
May 21 2024, 7:13 PM
Unknown Object (File)
May 21 2024, 2:31 PM

Details

Summary

This change adds SOCKS5 support to the library fetch(3) and updates the man page.

Details: Within the fetch_connect() function, fetch(3) checks if the SOCKS5_PROXY environment variable is set. If so, it connects to this host rather than the end-host. It then initializes the SOCKS5 connection in accordance with RFC 1928 and returns the resulting conn_t (file descriptor) for usage by the regular FTP/HTTP handlers.

Design Decision: This change defaults all DNS resolutions through the proxy by sending all IPs as hostnames. Going forward, another feature might be to create another environmental variable to toggle resolutions through the proxy or not..

For Special Review: There is error reporting in fetch_socks5_init to stderr. I could not figure out the proper method to set fetchLastErrString to a SOCKS5-specific error message, which is consumed by end-clients like fetch(1). If you know how, we can include SOCKS5-specific error messages. This might be useful so that fetch(1) will report a proper error.

Test Plan

Set the SOCKS5_PROXY environment variable in any of the formats:

SOCKS5_PROXY=proxy.example.com
SOCKS5_PROXY=proxy.example.com:1080
SOCKS5_PROXY=192.0.2.0
SOCKS5_PROXY=198.51.100.0:1080
SOCKS5_PROXY=[2001:db8::1]
SOCKS5_PROXY=[2001:db8::2]:1080

Then perform a request with fetch(1).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lib/libfetch/common.c
351 ↗(On Diff #53046)

Should this size be macro-ized?

359 ↗(On Diff #53046)

Magic values should be macro-ized.

389 ↗(On Diff #53046)

ptr is already set to buf.

395 ↗(On Diff #53046)

Since you're doing pointer arithmetic, strlen(host) needs to be validated to be 256 characters or less. As you modify where ptr points to, you need to ensure ptr is still within bounds prior to dereferencing it.

426 ↗(On Diff #53046)

No need for the breaks after goto.

484 ↗(On Diff #53046)

Need to check for NULL return value.

495 ↗(On Diff #53046)

Need to check for NULL return value.

0mp requested changes to this revision.Jan 22 2019, 10:33 AM
0mp added a subscriber: 0mp.

Cool!

BTW, you may take a look at style.mdoc(5).

lib/libfetch/fetch.3
29 ↗(On Diff #53046)

Remember to bump the date.

656 ↗(On Diff #53046)

Please lint your changes using mandoc -Tlint fetch.3 and igor fetch.3 (https://www.freshports.org/textproc/igor).

658 ↗(On Diff #53046)

Typo:

If no port

660 ↗(On Diff #53046)

.Ev HTTP_PROXY

719 ↗(On Diff #53046)

.Ev SOCKS5_PROXY

723 ↗(On Diff #53046)

I'd use something like .Bd -literal -offset indent instead of multiple .Dl. See NO_PROXY above for example.

This revision now requires changes to proceed.Jan 22 2019, 10:33 AM

I'm so sorry, I missed your comment from months ago!

farhan_farhan.codes marked 12 inline comments as done.

Updated per @lattera-gmail.com and @0mp comments.

0mp requested changes to this revision.Jun 19 2019, 8:41 AM

Only some minor issues left in the manpage :)

lib/libfetch/fetch.3
660 ↗(On Diff #53046)
This setting will supercede a connection to an
. Ev HTTP_PROXY .
This revision now requires changes to proceed.Jun 19 2019, 8:41 AM

Updated the man page + some macros

This revision is now accepted and ready to land.Jun 20 2019, 1:45 AM

Hi all. I see that this was changed to "Accepted" a while ago, but there has not been any movement on it.

kevans added inline comments.
lib/libfetch/common.c
383 ↗(On Diff #58821)

Move this else up to the line above it; if we could get rid of the magic number here, that'd be great.

469 ↗(On Diff #58821)

This should read: (socks5env = getenv("SOCKS5_PROXY")) == NULL || *socks5env == '\0' (prefer explicitly comparisons as per style(9))

472 ↗(On Diff #58821)

I'll leave this note here once; this function returns 0, 1, or 2 -- the difference between 1 and 2 doesn't seem to be significant from a callers' perspective, and I'm not sure this matches the prevailing style for libfetch which tends to return 0 for success, -1 for error.

477 ↗(On Diff #58821)

Binary operators should have spaces around them (strlen(socks5env) - 1)

483 ↗(On Diff #58821)

Move this a line up, } else {

485 ↗(On Diff #58821)

ext == NULL

490 ↗(On Diff #58821)

See above w.r.t. binary operators and spaces around them.

491 ↗(On Diff #58821)

See above w.r.t. binary operators and spaces around them; ext and socks5env should also likely be unparenthesized here.

494 ↗(On Diff #58821)

See above w.r.t. binary operators and spaces around them.

Also, why strtoimax here and below? You're given an int to work with here, so this should likely be something more like:

long portconv;

portconv = strtol(...);
if (portconv <= 0 || portconv > INT_MAX)
    ... error ...
*port = portconv;

strtoimax and strtol are effectively the same, just with different min/max limits -- you might as well use the smallest set of limits larger than what you can handle, even if efficiency isn't all that much of a concern here.

501 ↗(On Diff #58821)

else should move to the line above it

503 ↗(On Diff #58821)

ext == NULL

507 ↗(On Diff #58821)

Moved to the line above it

508 ↗(On Diff #58821)

See above w.r.t. binary operators and spaces around them.

511 ↗(On Diff #58821)

See above w.r.t. binary operators and spaces around them.

536 ↗(On Diff #58821)

This should move down to the line with other int declarations, sorted alphabetically there.

547 ↗(On Diff #58821)

sockshost == NULL

563 ↗(On Diff #58821)

Moved up to the line above

568 ↗(On Diff #58821)

Braces around this single-statement block should match the prevailing style elsewhere in the file for single-statement blocks; it looks like the braces should be dropped here.

604 ↗(On Diff #58821)

Added line should go away

farhan_farhan.codes marked 17 inline comments as done.

Updates from @kevans comments

This revision now requires review to proceed.Aug 20 2019, 2:05 AM

I believe I got all updates, except one. I can go ahead and make that change ASAP.

lib/libfetch/common.c
568 ↗(On Diff #58821)

This one seems correct, no?

lib/libfetch/common.c
568 ↗(On Diff #58821)

Correct, but stylistically inconsistent with the rest of the file- take a look ,for instance, at lines 556/557.

Updated!

lib/libfetch/common.c
568 ↗(On Diff #58821)

oh, I misunderstood your original comment. My mistake. Removing now.

For Special Review: There is error reporting in fetch_socks5_init to stderr. I could not figure out the proper method to set fetchLastErrString to a SOCKS5-specific error message, which is consumed by end-clients like fetch(1). If you know how, we can include SOCKS5-specific error messages. This might be useful so that fetch(1) will report a proper error.

This is actually pretty easy, from the looks of it- refer to common.c:67 (netdb_errlist) -- you'll want to define another structure like this for socks5_errlist mapping internal error codes (you'll need to create these) to FETCH_* error codes (see fetch.h:73) and error strings. The common theme is then to define socks5_seterr() like the others in common.h and use that with your error codes.

The downside of this scheme is that it doesn't let you include additional formatting specifiers, but those details are perhaps better left to a verbose option kind of thing.

Added libfetch-style error reporting for SOCKS5. Examples below:

Bad port demonstration:

$ export SOCKS5_PROXY=127.0.0.1:999999
$ fetch -v http://www.google.com
fetch: http://www.google.com: SOCKS5: Bad port

Clear protocol mismatch example:

$ export SOCKS5_PROXY=www.google.com:443
$ fetch -v http://www.googe.com
resolving SOCKS5 server address: www.google.com:443
Initializing SOCKS5 connection: www.google.com:80
fetch: http://www.google.com: SOCKS5: Failed to read method

On a separate terminal I ran :

nc -l 4444

to demonstrate a protocol version mismatch:

$ export SOCKS5_PROXY=127.0.0.1:4444
$ fetch -v http://www.google.com
resolving SOCKS5 server address: 127.0.0.1:4444
Initializing SOCKS5 connection: www.google.com:80
fetch: http://www.googe.com: SOCKS5: Only version 5 is implemented

Seems fine from the manpage formatting perspective!

This revision is now accepted and ready to land.Feb 1 2020, 4:17 PM

I have been informed that des will not have time for this- I'll take over as reviewer/committer.

lib/libfetch/common.c
553 ↗(On Diff #67584)

You must set errno = 0 before the calls to strtoimax to know for sure that this errno check is valid. Further, don't check specifically for EINVAL -- errno != 0.

568 ↗(On Diff #67584)

You must set errno = 0 before the calls to strtoimax to know for sure that this errno check is valid. Further, don't check specifically for EINVAL -- errno != 0.

659 ↗(On Diff #67584)

This socks5_seterr is currently blocked behind an if(verbose) -- I suspect you want to break that one out above before the verbose blocks start and set it if sockshost != NULL

farhan_farhan.codes marked 2 inline comments as done.

Updated per comments from kevans@

This revision now requires review to proceed.Feb 14 2020, 5:23 AM

Updated per @kevans comments, the previous update contained a misunderstanding on my part.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2020, 6:03 PM
Closed by commit rS357968: fetch(3): Add SOCKS5 support (authored by kevans). · Explain Why
This revision was automatically updated to reflect the committed changes.