-
Notifications
You must be signed in to change notification settings - Fork 53
Eliminate per-connection 407 probe for known-auth proxies #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 11 commits
69b9fc1
3d22a32
8dc49a3
43590de
eb14ca3
7736d5c
d81b935
a745a83
1de5b48
1c8a67d
23f54a5
242fd20
86d27ec
6924817
008e35f
0eb260b
be9a3d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,7 @@ import ( | |||||||
| "net/http" | ||||||||
| "net/url" | ||||||||
| "strings" | ||||||||
| "sync" | ||||||||
|
|
||||||||
| "golang.org/x/net/proxy" | ||||||||
| ) | ||||||||
|
|
@@ -133,7 +134,7 @@ func isToken(s string) bool { | |||||||
|
|
||||||||
| // parseProxyAuthenticateSchemes returns the deduplicated, lower-cased | ||||||||
| // scheme names from a 407 response's Proxy-Authenticate header(s), | ||||||||
| // preserving the order in which they appeared. RFC 7235 §4.3 allows | ||||||||
| // preserving the order in which they appeared. RFC 9110 §11.3 allows | ||||||||
| // either multiple Proxy-Authenticate header fields OR a single header | ||||||||
| // field containing a comma-separated list of challenges (RFC 7230 list | ||||||||
| // extension), so we honour both. The portion after the scheme token | ||||||||
|
|
@@ -157,11 +158,11 @@ func parseProxyAuthenticateSchemes(header http.Header) []string { | |||||||
|
|
||||||||
| // splitChallengeNames extracts the scheme names from a single | ||||||||
| // Proxy-Authenticate header field value that may contain multiple | ||||||||
| // challenges joined by commas (RFC 7235 §4.3 + RFC 7230 §7 list | ||||||||
| // challenges joined by commas (RFC 9110 §11.3 + RFC 7230 §7 list | ||||||||
| // extension). It walks the value byte-by-byte so that commas inside | ||||||||
| // quoted-strings are not treated as challenge separators. | ||||||||
| // | ||||||||
| // RFC 7235's challenge grammar is: | ||||||||
| // RFC 9110's challenge grammar is: | ||||||||
| // | ||||||||
| // challenge = auth-scheme [ 1*SP ( token68 / | ||||||||
| // [ ( "," / auth-param ) *( OWS "," [ OWS auth-param ] ) ] ) ] | ||||||||
|
|
@@ -178,7 +179,7 @@ func parseProxyAuthenticateSchemes(header http.Header) []string { | |||||||
| // will cause `bar` to be misclassified as a scheme name. This is benign | ||||||||
| // because no real authenticator's scheme() returns `bar`, so the picker | ||||||||
| // silently ignores it. We accept this rather than implementing full | ||||||||
| // RFC 7235 grammar parsing for the tail of unparseable proxies. | ||||||||
| // RFC 9110 grammar parsing for the tail of unparseable proxies. | ||||||||
| // | ||||||||
| // The returned strings are the raw auth-scheme tokens; the caller | ||||||||
| // validates them via isToken. | ||||||||
|
|
@@ -220,17 +221,23 @@ func splitChallengeNames(value string) []string { | |||||||
| return names | ||||||||
| } | ||||||||
|
|
||||||||
| type proxyAuthInfo struct { | ||||||||
| // schemes are lower-cased, matching parseProxyAuthenticateSchemes output. | ||||||||
| schemes []string | ||||||||
| } | ||||||||
|
|
||||||||
| type ProxyHandler struct { | ||||||||
| transport *http.Transport | ||||||||
| auth *authChain | ||||||||
| block func(string) | ||||||||
| authCache *sync.Map | ||||||||
| } | ||||||||
|
|
||||||||
| type proxyFunc func(*http.Request) (*url.URL, error) | ||||||||
|
|
||||||||
| func NewProxyHandler(auth *authChain, proxy proxyFunc, block func(string)) ProxyHandler { | ||||||||
| tr := &http.Transport{Proxy: proxy, TLSClientConfig: tlsClientConfig} | ||||||||
| return ProxyHandler{tr, auth, block} | ||||||||
| return ProxyHandler{tr, auth, block, &sync.Map{}} | ||||||||
| } | ||||||||
|
|
||||||||
| func (ph ProxyHandler) WrapHandler(next http.Handler) http.Handler { | ||||||||
|
|
@@ -270,7 +277,7 @@ func (ph ProxyHandler) handleConnect(w http.ResponseWriter, req *http.Request) { | |||||||
| if proxyURL == nil { | ||||||||
| server, err = connectDirect(req) | ||||||||
| } else { | ||||||||
| server, err = connectViaProxy(req, proxyURL, ph.auth) | ||||||||
| server, err = connectViaProxy(req, proxyURL, ph.auth, ph.authCache) | ||||||||
| var oe *net.OpError | ||||||||
| if errors.As(err, &oe) && oe.Op == "proxyconnect" { | ||||||||
| log.Printf("[%d] Temporarily blocking proxy: %q", id, proxyURL.Host) | ||||||||
|
|
@@ -345,7 +352,8 @@ func connectDirect(req *http.Request) (net.Conn, error) { | |||||||
| return server, err | ||||||||
| } | ||||||||
|
|
||||||||
| func connectViaProxy(req *http.Request, proxyURL *url.URL, auth *authChain) (net.Conn, error) { | ||||||||
| func connectViaProxy(req *http.Request, proxyURL *url.URL, auth *authChain, | ||||||||
| authCache *sync.Map) (net.Conn, error) { | ||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function requires both
Suggested change
|
||||||||
| id := req.Context().Value(contextKeyID) | ||||||||
|
|
||||||||
| // SOCKS5 short-circuit: SOCKS5 has its own authentication model | ||||||||
|
|
@@ -377,26 +385,78 @@ func connectViaProxy(req *http.Request, proxyURL *url.URL, auth *authChain) (net | |||||||
|
|
||||||||
| var tr transport | ||||||||
| defer tr.Close() //nolint:errcheck | ||||||||
| if err := tr.dial(proxyURL); err != nil { | ||||||||
| log.Printf("[%d] Error dialling proxy %s: %v", id, proxyURL.Host, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| resp, err := tr.RoundTrip(req) | ||||||||
| if err != nil { | ||||||||
| log.Printf("[%d] Error reading CONNECT response: %v", id, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| if resp.StatusCode == http.StatusProxyAuthRequired && auth != nil { | ||||||||
| log.Printf("[%d] Got %q response, retrying with auth", id, resp.Status) | ||||||||
| schemes := parseProxyAuthenticateSchemes(resp.Header) | ||||||||
| _ = resp.Body.Close() | ||||||||
| // resp is now stale; the retry helper returns a fresh one. | ||||||||
| authResp, err := retryConnectWithAuth(req, proxyURL, auth, schemes, &tr) | ||||||||
| activeTr := &tr // points to the transport holding the live tunnel | ||||||||
|
|
||||||||
| var resp *http.Response | ||||||||
| if cached, ok := authCache.Load(proxyURL.Host); ok && auth != nil { | ||||||||
| // Phase 1: cache hit — skip unauthenticated probe. | ||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It says "phase 1" here, which to me suggests that two things will happen in sequence, i.e. phase 1 and then phase 2. But this is an if-else statement, with phase 2 in the else block, so we'll do either one or the other? |
||||||||
| info := cached.(proxyAuthInfo) | ||||||||
| authResp, err := retryConnectWithAuth(req, proxyURL, auth, info.schemes, &tr) | ||||||||
| if err != nil { | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| if authResp.StatusCode == http.StatusProxyAuthRequired { | ||||||||
| // Stale cache entry — evict and fall through to cold probe. | ||||||||
| authCache.Delete(proxyURL.Host) | ||||||||
| _ = authResp.Body.Close() | ||||||||
| tr2 := transport{} | ||||||||
| defer tr2.Close() //nolint:errcheck | ||||||||
| activeTr = &tr2 | ||||||||
| if err := tr2.dial(proxyURL); err != nil { | ||||||||
| log.Printf("[%d] Error dialling proxy %s: %v", id, proxyURL.Host, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| req.Header.Del("Proxy-Authorization") | ||||||||
| resp2, err := tr2.RoundTrip(req) | ||||||||
| if err != nil { | ||||||||
| log.Printf("[%d] Error reading CONNECT response: %v", id, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| if resp2.StatusCode == http.StatusProxyAuthRequired && auth != nil { | ||||||||
| log.Printf("[%d] Got %q response, retrying with auth", id, resp2.Status) | ||||||||
| schemes := parseProxyAuthenticateSchemes(resp2.Header) | ||||||||
| _ = resp2.Body.Close() | ||||||||
| authResp2, err := retryConnectWithAuth(req, proxyURL, auth, schemes, &tr2) | ||||||||
| if err != nil { | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| if authResp2.StatusCode != http.StatusProxyAuthRequired { | ||||||||
| authCache.Store(proxyURL.Host, proxyAuthInfo{schemes: schemes}) | ||||||||
| } | ||||||||
| log.Printf("[%d] Got %q response", id, authResp2.Status) | ||||||||
| resp = authResp2 | ||||||||
| } else { | ||||||||
| resp = resp2 | ||||||||
| } | ||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I've missed something, but this looks largely similar to "phase 2", and I think this duplication (the "stale cache entry" case above, and the "cold probe - no cached entry" case below) is a result of handling the cache hit and miss in two separate if-else branches. In my mind, the logic would've been something like this:
Let me know if I've oversimplified this in my head, and I'm missing something?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost. Correction on step 4: A stale 407 isn't a dead end. It evicts the cache entry and runs a cold probe on a fresh transport, which can actually succeed. Self-healing cycle, not an error return. Good catch on the duplication. It's been extracted to a helper function.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for taking so long to reply to this, I've had a lot on my plate over the last week. In the case of a cache hit, we'll skip step 2 which is the cold probe - this is the optimisation we want? In step 4, we either reach it after a cold probe + retry with auth (steps 2-3), or using a cached scheme. In both cases we should evict the cache entry and return an error response to the client (eg bad gateway). So I thought it would be a dead end, let me know if I'm missing something? Everything else looks fine to me, if we can close this one item I think we're ready to merge.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good. I've been up watching World Cup. Argentina vs Egypt was wow. Let me think about this one. I thought Step 4 should self-heal (evict + cold probe again). The scenario the self-heal was designed for is the proxy changing its auth scheme mid-session, but I just realised that scenario is already handled by the PAC flush being wired up in proxyfinder.go. When the network changes (VPN, Wi-Fi switch), the PAC re-downloads and onPACUpdate clears the cache. The likely failure case for 407 after retryConnectWithAuth is bad credentials, not network change anyways. TLDR; The self-heal adds complexity and an extra round-trip for an edge case that's not going to happen, whilst not handling the common failure case well. Good spot, let me go back to the drawing board on this one.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed it completely, as extraneous logic. Been in World Cup mode, and forgot to mention that had been done. |
||||||||
| } else { | ||||||||
| log.Printf("[%d] Got %q response", id, authResp.Status) | ||||||||
| resp = authResp | ||||||||
| } | ||||||||
| } else { | ||||||||
| // Phase 2: cold probe — no cached entry. | ||||||||
| if err := tr.dial(proxyURL); err != nil { | ||||||||
| log.Printf("[%d] Error dialling proxy %s: %v", id, proxyURL.Host, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| var err error | ||||||||
| resp, err = tr.RoundTrip(req) | ||||||||
| if err != nil { | ||||||||
| log.Printf("[%d] Error reading CONNECT response: %v", id, err) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| log.Printf("[%d] Got %q response", id, authResp.Status) | ||||||||
| resp = authResp | ||||||||
| if resp.StatusCode == http.StatusProxyAuthRequired && auth != nil { | ||||||||
| log.Printf("[%d] Got %q response, retrying with auth", id, resp.Status) | ||||||||
| schemes := parseProxyAuthenticateSchemes(resp.Header) | ||||||||
| _ = resp.Body.Close() | ||||||||
| authCache.Store(proxyURL.Host, proxyAuthInfo{schemes: schemes}) | ||||||||
| // resp is now stale; the retry helper returns a fresh one. | ||||||||
| authResp, err := retryConnectWithAuth(req, proxyURL, auth, schemes, &tr) | ||||||||
| if err != nil { | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| log.Printf("[%d] Got %q response", id, authResp.Status) | ||||||||
| resp = authResp | ||||||||
| } | ||||||||
| } | ||||||||
| _ = resp.Body.Close() | ||||||||
| if resp.StatusCode == http.StatusProxyAuthRequired { | ||||||||
|
|
@@ -406,7 +466,7 @@ func connectViaProxy(req *http.Request, proxyURL *url.URL, auth *authChain) (net | |||||||
| if resp.StatusCode != http.StatusOK { | ||||||||
| return nil, fmt.Errorf("[%d] Unexpected response status: %s", id, resp.Status) | ||||||||
| } | ||||||||
| return tr.hijack(), nil | ||||||||
| return activeTr.hijack(), nil | ||||||||
| } | ||||||||
|
|
||||||||
| // retryConnectWithAuth iterates the configured auth chain over a CONNECT | ||||||||
|
|
||||||||
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.
The term "load-bearing" was a bit confusing to me, maybe we can reword it to something like this?