Added SOCKS5 support to libfetch

[farhan_farhan.codes]

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

[lattera-gmail.com]

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]

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.

[farhan_farhan.codes]

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

[farhan_farhan.codes]

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

[0mp]

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 .

[farhan_farhan.codes]

Updated the man page + some macros

[0mp]

ok from manpages

[farhan_farhan.codes]

Any updates on this?

[farhan_farhan.codes]

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

[kevans]

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]

Updates from @kevans comments

[farhan_farhan.codes]

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?

[kevans]

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.

[farhan_farhan.codes]

Minor style(9) change

[farhan_farhan.codes]

Updated!

lib/libfetch/common.c

568 ↗

(On Diff #58821)

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

[farhan_farhan.codes]

Rebased against current head

[farhan_farhan.codes]

Any updates on this, @des and @thj ? :)

[farhan_farhan.codes]

bump :)

[farhan_farhan.codes]

dump :(

[kevans]

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.

[farhan_farhan.codes]

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

[0mp]

Seems fine from the manpage formatting perspective!

[farhan_farhan.codes]

Bump

[kevans]

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]

Updated per comments from kevans@

[farhan_farhan.codes]

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