tunnel through an https proxy to reach a host - #58
Merged
Merged
Conversation
Revocation and issuer fetches went through the proxy the environment named, because they are ordinary http and net/http reads HTTP_PROXY and HTTPS_PROXY for itself. The connection to the host did not: it was a plain net.Dial, and go consults those variables nowhere but in an http transport. So on a network where 443 leaves only through a proxy, the one thing this tool exists to do failed, while the checks it can make about a certificate it could not fetch worked. The connection is now opened with CONNECT when HTTPS_PROXY names a proxy, and the handshake runs end to end inside that tunnel, so the certificates reported are the target's own and SNI still names the target. -starttls is tunnelled the same way, the negotiation happening with the target rather than with the proxy. NO_PROXY is honoured with go's own rules, since a reader who sets it expects it to mean what it means everywhere else: * excludes everything, an entry may be a host, a domain suffix, an ip address or a cidr block, and may carry a port that must match too. localhost and loopback are never proxied whatever it says, which is also what makes the existing tests keep passing on a machine that has a proxy set. HTTP_PROXY is deliberately not read for this. It names a proxy for plaintext http requests, and none is made to the target. An https:// proxy is spoken to over tls before the CONNECT, and -insecure carries to that hop: it is the same instruction not to verify, one hop earlier. A socks5:// or other scheme is an error rather than something quietly ignored, since ignoring it would connect directly and report a certificate the user did not think they were looking at. A proxy that terminates tls rather than tunnelling it will present its own certificate and that is what gets printed. The README says so, since reading it as the target's is the mistake this tool should not invite. Tested against a proxy that records what it was asked to connect to, so the assertions are that the tunnel named the target, that sni did, and that the certificate came back from behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v
There was a problem hiding this comment.
🟡 Changes recommended
Proxy URL parsing can incorrectly accept malformed explicit-scheme values (e.g. http:// with no host), which can lead to dialing an unintended proxy address.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds first-class support for reaching target hosts through an HTTPS proxy by tunneling the TLS (and StartTLS) handshake via HTTP CONNECT, aligning host-certificate fetching with existing proxied HTTP fetches (issuer/revocation).
Changes:
- Implement proxy selection/parsing and
NO_PROXYmatching for host connections, plusCONNECTtunnel establishment (including optional proxy TLS and Basic auth). - Update network certificate loading to optionally dial via proxy tunnel and to tunnel StartTLS negotiations as well.
- Document proxy behavior in the README and add comprehensive tests for tunneling, SNI, auth,
NO_PROXY, and error cases.
File summaries
| File | Description |
|---|---|
| README.md | Documents new proxy/tunneling behavior and environment variable handling. |
| pkg/cert/proxy.go | Adds proxy env parsing, NO_PROXY matching, and CONNECT tunnel dialing logic. |
| pkg/cert/proxy_test.go | Adds a fake proxy + backend test harness covering proxy tunneling behaviors and edge cases. |
| pkg/cert/location.go | Routes target connections (including StartTLS) through the proxy tunnel when configured. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+48
to
+59
| proxy, err := url.Parse(value) | ||
| if err != nil || proxy.Host == "" || proxy.Scheme == "" { | ||
| // a value such as "proxy.example.com:3128" carries no scheme, and | ||
| // parses as one rather than as a host | ||
| if bare, bareErr := url.Parse("http://" + value); bareErr == nil && bare.Host != "" { | ||
| return bare, nil | ||
| } | ||
| } | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid proxy address %q: %w", value, err) | ||
| } | ||
|
|
Comment on lines
+324
to
+329
| {name: "an http url", value: "http://proxy.example.com:3128", host: "proxy.example.com:3128"}, | ||
| {name: "an https url", value: "https://proxy.example.com:8443", host: "proxy.example.com:8443"}, | ||
| {name: "a bare host and port, which means http", value: "proxy.example.com:3128", host: "proxy.example.com:3128"}, | ||
| {name: "a bare address without a port", value: "proxy.example.com", host: "proxy.example.com"}, | ||
| {name: "an unsupported scheme", value: "socks5://proxy.example.com:1080", error: "unsupported proxy scheme"}, | ||
| } |
Two faults in the CONNECT support, one found by review and one it uncovered by accident. HTTPS_PROXY=http:// was accepted. A value that parsed with no host fell back to being read as a bare address, so "http://" was re-parsed as "http://http://" and became the host "http:", dialled as http:80. It is a nonsense address that will normally fail to resolve, but in a domain with a search suffix it need not, and a proxy address a user did not write is not one to dial on their behalf. A value naming a scheme is now held to being a whole url, and a value with no host is refused either way, so "://proxy:3128" and "http://:3128" are errors rather than dials of ":80" and of the local machine. The second was mine. The CONNECT response was read through a buffer that the handshake did not share, so anything left in it was treated as a proxy misbehaving. That is what net/http does, and it is right for http, where the client speaks first. It is wrong here: smtp, imap, pop3, ftp and nntp all have the server speak first, so through a proxy the greeting can arrive on the heels of the response and land in that same buffer. The connection was then dropped with "sent 22 bytes after the CONNECT response", naming the proxy for something the target had done. It was a race rather than a certainty, which is why it passed when the tests were written and failed when they were next run. The buffer is carried on the connection now, so nothing is lost whoever speaks first, and a proxy that really does send junk fails in the handshake, which says so more precisely than the check did. The test drops the timing: the proxy sends the greeting in the same write as the response, so the greeting is buffered before anything asks for it. It fails against the old code every time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revocation and issuer fetches already went through the proxy the environment named, because they are ordinary HTTP and
net/httpreadsHTTP_PROXY/HTTPS_PROXYfor itself. The connection to the host did not: it was a plainnet.Dial, and go consults those variables nowhere but in an HTTP transport. On a network where 443 leaves only through a proxy, the one thing this tool exists to do failed, while the checks it can make about a certificate it could not fetch worked.what changed
HTTPS_PROXY/https_proxynow opens the connection with an HTTPCONNECTtunnel, and the handshake runs end to end inside it — so the certificates reported are the target's own and SNI still names the target.-starttlsis tunnelled the same way, the plaintext negotiation happening with the target rather than with the proxy.NO_PROXY/no_proxyis honoured with go's own rules:*excludes everything, an entry may be a host, a domain suffix, an IP address or a CIDR block, and may carry a port that must match too.localhostand loopback are never proxied whatever it says.host:port, which means http. Credentials in the URL are sent asProxy-Authorization: Basic, and-verbosenames the proxy without its password.tls.DialWithDialer.judgement calls
HTTP_PROXYis deliberately not read for the host connection. It names a proxy for plaintext HTTP requests, and none is made to the target. Revocation and issuer fetches still honour it.https://proxy is spoken to over TLS before theCONNECT, and-insecurecarries to that hop — the same instruction not to verify, one hop earlier.socks5://or other scheme is an error rather than quietly ignored, since ignoring it would connect directly and report a certificate the user did not think they were looking at.testing
A fake proxy that records what it was asked to connect to, in front of a TLS backend that records the name each client asked for, so the assertions are that the tunnel named the target, that SNI did, and that the certificate came back from behind it. Also covers
-server-name, credentials, starttls through the tunnel,NO_PROXY, a 407 refusal, an unreachable proxy, and a rejected scheme, plus tables for theNO_PROXYrules and URL parsing.make test,go test -race ./...andgolangci-lint run ./...all clean. Smoke-tested with the built binary against a local CONNECT proxy:CONNECT smoke.example.test:443sent, target's certificate returned, and loopback targets andNO_PROXYmatches went direct.🤖 Generated with Claude Code
https://claude.ai/code/session_01WcPAJjNzG6bqy2FKqKKY1v