feature: Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY

[peterguy]

Adds support for the standard environment variables HTTP_PROXY, HTTPS_PROXY, and NO_PROXY.

SRC_PROXY still takes precedence, including over NO_PROXY.

Test plan

Added CI tests validating proxy connection behavior.

Manual testing

Install mitmproxy, socat and squid (brew install mitmproxy socat squid for me).

Create an SSL cert for squid:

openssl req -x509 -newkey rsa:2048 \
  -keyout ${HOME}/squid-key.pem -out ${HOME}/squid-cert.pem \
  -days 365 -nodes -subj "/CN=localhost"
cat ${HOME}/squid-key.pem ${HOME}/squid-cert.pem > ${HOME}/squid-proxy.pem

Create a config for squid:

cat << EOF >${HOME}/squid.conf
https_port 8445 cert=${HOME}/squid-proxy.pem
sslproxy_session_cache_size 0
acl localnet src 127.0.0.1/32
http_access allow localhost
acl SSL_ports port 443
acl Safe_ports port 80
acl Safe_ports port 443
acl Safe_ports port 1025-65535
http_access deny !Safe_ports
http_access deny CONNECT !SSL_ports
http_access deny all
http_port 3128
EOF

Launch three terminals to run the proxies:

• run socat -d -d unix-listen:${HOME}/src-proxy.sock,fork tcp:localhost:8080 in one.
• run mitmproxy -v in another.
• run squid - squid -f ${HOME}/squid.conf -N -d 1 - in the third.

In a fourth terminal, in the src-cli repo, build the binary: go build -o ./src-cli ./cmd/src

Ensure you have SRC_ENDPOINT and SRC_ACCESS_TOKEN set in your environment, and unset SRC_PROXY. SRC_ENDPOINT needs to be https://.

Connect with each proxy (mitmproxy is 8080, squid is 8445) and with no proxy:

for x in http://localhost:8080 http://localhost:3128 https://localhost:8445
do
HTTPS_PROXY=${x} ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${x} ./src-cli login --insecure-skip-verify=true
HTTPS_PROXY=${x} NO_PROXY=${${SRC_ENDPOINT#*://}%%/*} ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${x} NO_PROXY=${${SRC_ENDPOINT#*://}%%/*} ./src-cli login --insecure-skip-verify=true
done
HTTPS_PROXY="" SRC_PROXY="" ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${HOME}/src-proxy.sock ./src-cli login --insecure-skip-verify=true

You should get 14 "Authenticated as [USER] on [ENDPOINT]" messages in your terminal. You should see four connections in mitmproxy, six in squid, and several lines in socat indicating one connection.

This test ensures:

• all of the proxy environment variables are supported (SRC_PROXY, HTTPS_PROXY, NO_PROXY)
• NO_PROXY trumps HTTPS_PROXY
• SRC_PROXY trumps HTTPS_PROXY and NO_PROXY
• both plain HTTP and TLS-enabled proxies are supported
• the ability to connect to UNIX Domain Sockets is preserved
• communication with no proxy at all is successful

Cleanup from the test:

• terminate all running proxy processes.
• rm -f ${HOME}/squid-proxy.pem ${HOME}/squid-key.pem ${HOME}/squid-cert.pem ${HOME}/squid.conf ${HOME}/src-proxy.sock

Closes https://linear.app/sourcegraph/issue/CPL-236/src-cli-support-standard-http-proxy

[keegancsmith]

Nice! Inline comments, but otherwise LGTM

[keegancsmith]

You might wanna rebase, I think william just did some CI updates which include things like go-lint/etc.

[peterguy]

Thanks for all of your commentary; I forgot to open this PR in draft mode! Been trying to address what looks like flakey tests in the Ubuntu CI runner; I'll go through your very helpful comments soon.

[peterguy]

This change is part of the following stack:

• refactor: validate and parse the endpoint and proxy at program load #1267
  • feature: Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260 ◀

_{Change managed by git-spice.}

[keegancsmith]

@peterguy alright just re-request review when ready.

[keegancsmith]

Requesting changes since I just wanna re-review once you have addressed the comments. FYI I pushed some commits to resolve the CI issues.

[bahrmichael]

I would wait for an update for Keegan's requested changes before doing my review pass if that's okay.

[bahrmichael]

Thank you for simplifying this so much! I'm surprised that we can delete so much code, but as you said it still works after removing the socks5 code.

Is the PR description still up to date?

[peterguy]

Description is accurate, Added some better smoke tests that I think are important to guard against future changes to withProxyTransport, even though the code is much more simple.