Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260
+585
−78
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,32 +5,66 @@ import ( | |
| "context" | ||
| "crypto/tls" | ||
| "encoding/base64" | ||
| "io" | ||
| "net" | ||
| "net/http" | ||
| "net/url" | ||
| "sync" | ||
|
|
||
| "github.com/sourcegraph/sourcegraph/lib/errors" | ||
| ) | ||
|
|
||
| type connWithBufferedReader struct { | ||
| net.Conn | ||
| r *bufio.Reader | ||
| mu sync.Mutex | ||
| } | ||
|
|
||
| func (c *connWithBufferedReader) Read(p []byte) (int, error) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| return c.r.Read(p) | ||
| } | ||
|
Comment on lines
+23
to
+27
Member
There was a problem hiding this comment. net.Conn documents all methods to be concurrency safe. However, bufio.Reader is not. You need to add in a mutex here. > Multiple goroutines may invoke methods on a Conn simultaneously. |
||
|
|
||
| // proxyDialAddr returns proxyURL.Host with a default port appended if one is | ||
| // not already present (443 for https, 80 for http). | ||
| func proxyDialAddr(proxyURL *url.URL) string { | ||
| // net.SplitHostPort returns an error when the input doesn't contain a port | ||
| if _, _, err := net.SplitHostPort(proxyURL.Host); err == nil { | ||
| return proxyURL.Host | ||
| } | ||
| if proxyURL.Scheme == "https" { | ||
| return net.JoinHostPort(proxyURL.Hostname(), "443") | ||
| } | ||
| return net.JoinHostPort(proxyURL.Hostname(), "80") | ||
| } | ||
|
|
||
| // withProxyTransport modifies the given transport to handle proxying of unix, socks5 and http connections. | ||
| // | ||
| // Note: baseTransport is considered to be a clone created with transport.Clone() | ||
| // | ||
| // - If proxyPath is not empty, a unix socket proxy is created. | ||
| // - Otherwise, proxyURL is used to determine if we should proxy socks5 / http connections | ||
| func withProxyTransport(baseTransport *http.Transport, proxyURL *url.URL, proxyPath string) *http.Transport { | ||
| handshakeTLS := func(ctx context.Context, conn net.Conn, addr string) (net.Conn, error) { | ||
| // Extract the hostname (without the port) for TLS SNI | ||
| host, _, err := net.SplitHostPort(addr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cfg := baseTransport.TLSClientConfig.Clone() | ||
| if cfg.ServerName == "" { | ||
| cfg.ServerName = host | ||
| } | ||
| // Preserve HTTP/2 negotiation to the origin when ForceAttemptHTTP2 | ||
| // is enabled. Without this, the manual TLS handshake would not | ||
| // advertise h2 via ALPN, silently forcing HTTP/1.1. | ||
| if baseTransport.ForceAttemptHTTP2 && len(cfg.NextProtos) == 0 { | ||
| cfg.NextProtos = []string{"h2", "http/1.1"} | ||
| } | ||
| tlsConn := tls.Client(conn, cfg) | ||
| if err := tlsConn.HandshakeContext(ctx); err != nil { | ||
| tlsConn.Close() | ||
| return nil, err | ||
peterguy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| return tlsConn, nil | ||
|
|
@@ -54,67 +88,79 @@ func withProxyTransport(baseTransport *http.Transport, proxyURL *url.URL, proxyP | |
| baseTransport.Proxy = nil | ||
| } else if proxyURL != nil { | ||
| switch proxyURL.Scheme { | ||
| case "http", "socks5", "socks5h": | ||
| // HTTP and SOCKS proxies work out of the box - no need to manually dial | ||
| baseTransport.Proxy = http.ProxyURL(proxyURL) | ||
| case "https": | ||
| dial := func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| // Dial the proxy. For https:// proxies, we TLS-connect to the | ||
| // proxy itself and force ALPN to HTTP/1.1 to prevent Go from | ||
| // negotiating HTTP/2 for the CONNECT tunnel. Many proxy servers | ||
| // don't support HTTP/2 CONNECT, and Go's default Transport.Proxy | ||
| // would negotiate h2 via ALPN when TLS-connecting to an https:// | ||
| // proxy, causing "bogus greeting" errors. For http:// proxies, | ||
| // CONNECT is always HTTP/1.1 over plain TCP so this isn't needed. | ||
| // The target connection (e.g. to sourcegraph.com) still negotiates | ||
| // HTTP/2 normally through the established tunnel. | ||
| proxyAddr := proxyDialAddr(proxyURL) | ||
keegancsmith marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| var conn net.Conn | ||
| var err error | ||
| if proxyURL.Scheme == "https" { | ||
| raw, dialErr := (&net.Dialer{}).DialContext(ctx, "tcp", proxyAddr) | ||
| if dialErr != nil { | ||
| return nil, dialErr | ||
| } | ||
| cfg := baseTransport.TLSClientConfig.Clone() | ||
| cfg.NextProtos = []string{"http/1.1"} | ||
| if cfg.ServerName == "" { | ||
| cfg.ServerName = proxyURL.Hostname() | ||
| } | ||
| tlsConn := tls.Client(raw, cfg) | ||
| if err := tlsConn.HandshakeContext(ctx); err != nil { | ||
| raw.Close() | ||
| return nil, err | ||
| } | ||
| conn = tlsConn | ||
| } else { | ||
| conn, err = (&net.Dialer{}).DialContext(ctx, "tcp", proxyAddr) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| connectReq := &http.Request{ | ||
| Method: "CONNECT", | ||
| URL: &url.URL{Opaque: addr}, | ||
| Host: addr, | ||
| Header: make(http.Header), | ||
| } | ||
| if proxyURL.User != nil { | ||
| password, _ := proxyURL.User.Password() | ||
| auth := base64.StdEncoding.EncodeToString([]byte(proxyURL.User.Username() + ":" + password)) | ||
| connectReq.Header.Set("Proxy-Authorization", "Basic "+auth) | ||
| } | ||
| if err := connectReq.Write(conn); err != nil { | ||
| conn.Close() | ||
| return nil, err | ||
| } | ||
|
|
||
| br := bufio.NewReader(conn) | ||
|
Member
There was a problem hiding this comment. It isn't clear to me why you introduced the buffered reader?
Contributor
Author
There was a problem hiding this comment. Because I didn't want to lose any bytes after the headers that the reader may have buffered, which bytes could be the start of the tunneled connection data. Granted, it's probably not really a problem because the SG endpoint will in nearly every scenario also be TLS, and "TLS server will not speak until spoken to.", so there will not be any bytes beyond the header. But if the SG endpoint is plain http, we could lose bytes if we discard the buffered reader. |
||
| resp, err := http.ReadResponse(br, nil) | ||
| if err != nil { | ||
| conn.Close() | ||
| return nil, err | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| // For non-200, it's safe/appropriate to close the body (it’s a real response body here). | ||
| // Try to read a bit (4k bytes) to include in the error message. | ||
| b, _ := io.ReadAll(io.LimitReader(resp.Body, 4<<10)) | ||
| resp.Body.Close() | ||
| conn.Close() | ||
| return nil, errors.Newf("failed to connect to proxy %s: %s: %q", proxyURL.Redacted(), resp.Status, b) | ||
| } | ||
| // 200 CONNECT: do NOT resp.Body.Close(); it would interfere with the tunnel. | ||
| return &connWithBufferedReader{Conn: conn, r: br}, nil | ||
| } | ||
| dialTLS := func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| // Dial the underlying connection through the proxy | ||
|
|
@@ -126,7 +172,7 @@ func withProxyTransport(baseTransport *http.Transport, proxyURL *url.URL, proxyP | |
| } | ||
| baseTransport.DialContext = dial | ||
| baseTransport.DialTLSContext = dialTLS | ||
| // clear out the system proxy because we're defining our own dialers | ||
| baseTransport.Proxy = nil | ||
| } | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you changed this is some other places as well. go convention is lowercase on error messages since they get combined. eg
My outer error: My inner errordoesn't read as nicely asmy outer error: my inner error