libfetch: Fix proxy authentication

[kp]

Summary

This patch is being used on pfSense to fix pkg when proxy is configured and many users reported it works as expected.

libfetch: Retry with proxy authentication when server returns 407

PR:	220468
Submitted by:	Egil Hasting <[e-mail]> (based on)
Reviewed by:	kevans, kp
Approved by:	kp	
Sponsored by:	Rubicon Communications, LLC ("Netgate")
Differential Revision:	https://reviews.freebsd.org/D29533

Test Plan

Configure an authenticated proxy server and define HTTP_PROXY and HTTP_PROXY_AUTH environment variables. Without this patch the result is like:

garga@x230 ~ ❯❯❯ env | grep PROXY
HTTP_PROXY_AUTH=basic:*:testproxy:testproxy
HTTP_PROXY=172.21.4.1:3128
garga@x230 ~ ❯❯❯ fetch -v http://ifconfig.me
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
proxy requires authorization
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
302 redirect to https://ifconfig.me/
resolving server address: 172.21.4.1:3128
fetch: http://ifconfig.me: Proxy Authentication Required

After apply the patch:

garga@x230 ~ ❯❯❯ fetch -v http://ifconfig.me
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
proxy requires authorization
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
302 redirect to https://ifconfig.me/
resolving server address: 172.21.4.1:3128
resolving server address: 172.21.4.1:3128
SSL options: 82004854
Peer verification enabled
Using CA cert file: /usr/local/etc/ssl/cert.pem
Verify hostname
TLSv1.3 connection established using TLS_AES_256_GCM_SHA384
Certificate subject: /CN=ifconfig.me
Certificate issuer: /C=US/O=Google Trust Services/CN=GTS CA 1D2
requesting https://ifconfig.me/
remote size / mtime: 8826 / 0
ifconfig.m                                    8826  B   38 MBps    00s

[kp]

lib/libfetch/http.c

1405

I'm not sure about using warnx() from a library. There are others, but they're for unimplemented functionality (so should arguably be assertions for the calling app), not for runtime errors (i.e. proxy auth information not set).

1453

I don't like '> 0' for an int that's used as a boolean, but that's a question of taste more than anything.

1509

This is wrong though. 'fetch_close(conn)' frees conn, so we can't use or return it.

I think we should just not fetch_close().

[kevans]

re: commit message:

Submitted by: Egil Hasting <[e-mail]>

Drop this line and --amend the commit with --author="Egil Hasting <[e-mail]>", git log should show their name and you will be attributed as the committer.

lib/libfetch/http.c

1408

Just before this, we should explicitly check if aparams.user == NULL || aparams.password == NULL and return an error (clean_http_auth_parms first) to propagate the ENOMEM condition properly.

1509

Yes, the caller (below) will close it in ouch if it's not NULL, so just drop this fetch_close() entirely since we're returning a live object.

[kevans]

lib/libfetch/http.c

1509

Actually, that's a lie. The caller below needs to observe conn->err then close it.

[kp]

Address a few comments (lightly edited version of Renato's work).

[kp]

lib/libfetch/http.c

1408

The code is already somewhat inconsistent about checking for memory allocation failures.

On the whole I tend to prefer not to clutter userspace code with such checks, because in the vast majority of cases the system will be overcommitting memory anyway, so the first thing you'll know about an out-of-memory condition is when the OOM killer comes round and terminates you.

[kevans]

Sure