Page MenuHomeFreeBSD

Added SOCKS5 support to libfetch
ClosedPublic

Authored by farhan_farhan.codes on Jan 21 2019, 12:26 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 6:14 AM
Unknown Object (File)
Sat, Mar 2, 6:15 PM
Unknown Object (File)
Feb 7 2024, 8:02 PM
Unknown Object (File)
Feb 1 2024, 5:15 PM
Unknown Object (File)
Jan 27 2024, 4:37 PM
Unknown Object (File)
Jan 27 2024, 4:37 PM
Unknown Object (File)
Jan 27 2024, 4:37 PM
Unknown Object (File)
Jan 27 2024, 4:37 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 Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lib/libfetch/common.c
351

Should this size be macro-ized?

359

Magic values should be macro-ized.

389

ptr is already set to buf.

395

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

No need for the breaks after goto.

484

Need to check for NULL return value.

495

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

Remember to bump the date.

656

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

658

Typo:

If no port

660

.Ev HTTP_PROXY

720

.Ev SOCKS5_PROXY

724

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
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

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

469

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

472

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

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

483

Move this a line up, } else {

485

ext == NULL

490

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

491

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

494

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

else should move to the line above it

503

ext == NULL

507

Moved to the line above it

508

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

511

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

533

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

544

sockshost == NULL

575

Moved up to the line above

580

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.

599

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
580

This one seems correct, no?

lib/libfetch/common.c
580

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

Updated!

lib/libfetch/common.c
580

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
495

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.

510

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.

598

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.