Skip to content

Commit

Permalink
reverseproxy: Make shallow-ish clone of the request (#4551)
Browse files Browse the repository at this point in the history 
* reverseproxy: Make shallow-ish clone of the request

* Refactor request cloning into separate function

Co-authored-by: Matthew Holt <[email protected]>
  • Loading branch information
@francislavoie @mholt
francislavoie and mholt authored Mar 3, 2022
1 parent 6b385a3 commit f5e1049
Showing 1 changed file with 76 additions and 61 deletions.
137 changes: 76 additions & 61 deletions modules/caddyhttp/reverseproxy/reverseproxy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,54 +358,20 @@ func (h *Handler) Cleanup() error {
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)

// if enabled, buffer client request;
// this should only be enabled if the
// upstream requires it and does not
// work with "slow clients" (gunicorn,
// etc.) - this obviously has a perf
// overhead and makes the proxy at
// risk of exhausting memory and more
// susceptible to slowloris attacks,
// so it is strongly recommended to
// only use this feature if absolutely
// required, if read timeouts are set,
// and if body size is limited
if h.BufferRequests {
r.Body = h.bufferedBody(r.Body)
}

// prepare the request for proxying; this is needed only once
err := h.prepareRequest(r)
clonedReq, err := h.prepareRequest(r)
if err != nil {
return caddyhttp.Error(http.StatusInternalServerError,
fmt.Errorf("preparing request for upstream round-trip: %v", err))
}

// we will need the original headers and Host value if
// header operations are configured; and we should
// restore them after we're done if they are changed
// (for example, changing the outbound Host header
// should not permanently change r.Host; issue #3509)
reqHost := r.Host
reqHeader := r.Header

// sanitize the request URL; we expect it to not contain the scheme and host
// since those should be determined by r.TLS and r.Host respectively, but
// some clients may include it in the request-line, which is technically
// valid in HTTP, but breaks reverseproxy behaviour, overriding how the
// dialer will behave. See #4237 for context.
origURLScheme := r.URL.Scheme
origURLHost := r.URL.Host
r.URL.Scheme = ""
r.URL.Host = ""

// restore modifications to the request after we're done proxying
defer func() {
r.Host = reqHost // TODO: data race, see #4038
r.Header = reqHeader // TODO: data race, see #4038
r.URL.Scheme = origURLScheme
r.URL.Host = origURLHost
}()
// header operations are configured; this is so that each
// retry can apply the modifications, because placeholders
// may be used which depend on the selected upstream for
// their values
reqHost := clonedReq.Host
reqHeader := clonedReq.Header

start := time.Now()
defer func() {
Expand All @@ -416,12 +382,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht
var proxyErr error
for {
// choose an available upstream
upstream := h.LoadBalancing.SelectionPolicy.Select(h.Upstreams, r, w)
upstream := h.LoadBalancing.SelectionPolicy.Select(h.Upstreams, clonedReq, w)
if upstream == nil {
if proxyErr == nil {
proxyErr = fmt.Errorf("no upstreams available")
}
if !h.LoadBalancing.tryAgain(h.ctx, start, proxyErr, r) {
if !h.LoadBalancing.tryAgain(h.ctx, start, proxyErr, clonedReq) {
break
}
continue
Expand All @@ -430,7 +396,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht
// the dial address may vary per-request if placeholders are
// used, so perform those replacements here; the resulting
// DialInfo struct should have valid network address syntax
dialInfo, err := upstream.fillDialInfo(r)
dialInfo, err := upstream.fillDialInfo(clonedReq)
if err != nil {
return statusError(fmt.Errorf("making dial info: %v", err))
}
Expand All @@ -451,17 +417,17 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht

// mutate request headers according to this upstream;
// because we're in a retry loop, we have to copy
// headers (and the r.Host value) from the original
// headers (and the Host value) from the original
// so that each retry is identical to the first
if h.Headers != nil && h.Headers.Request != nil {
r.Header = make(http.Header)
copyHeader(r.Header, reqHeader)
r.Host = reqHost
h.Headers.Request.ApplyToRequest(r)
clonedReq.Header = make(http.Header)
copyHeader(clonedReq.Header, reqHeader)
clonedReq.Host = reqHost
h.Headers.Request.ApplyToRequest(clonedReq)
}

// proxy the request to that upstream
proxyErr = h.reverseProxy(w, r, repl, dialInfo, next)
proxyErr = h.reverseProxy(w, clonedReq, repl, dialInfo, next)
if proxyErr == nil || proxyErr == context.Canceled {
// context.Canceled happens when the downstream client
// cancels the request, which is not our failure
Expand All @@ -480,22 +446,35 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht
h.countFailure(upstream)

// if we've tried long enough, break
if !h.LoadBalancing.tryAgain(h.ctx, start, proxyErr, r) {
if !h.LoadBalancing.tryAgain(h.ctx, start, proxyErr, clonedReq) {
break
}
}

return statusError(proxyErr)
}

// prepareRequest modifies req so that it is ready to be proxied,
// except for directing to a specific upstream. This method mutates
// headers and other necessary properties of the request and should
// be done just once (before proxying) regardless of proxy retries.
// This assumes that no mutations of the request are performed
// by h during or after proxying.
func (h Handler) prepareRequest(req *http.Request) error {
// most of this is borrowed from the Go std lib reverse proxy
// prepareRequest clones req so that it can be safely modified without
// changing the original request or introducing data races. It then
// modifies it so that it is ready to be proxied, except for directing
// to a specific upstream. This method adjusts headers and other relevant
// properties of the cloned request and should be done just once (before
// proxying) regardless of proxy retries. This assumes that no mutations
// of the cloned request are performed by h during or after proxying.
func (h Handler) prepareRequest(req *http.Request) (*http.Request, error) {
req = cloneRequest(req)

// if enabled, buffer client request; this should only be
// enabled if the upstream requires it and does not work
// with "slow clients" (gunicorn, etc.) - this obviously
// has a perf overhead and makes the proxy at risk of
// exhausting memory and more susceptible to slowloris
// attacks, so it is strongly recommended to only use this
// feature if absolutely required, if read timeouts are
// set, and if body size is limited
if h.BufferRequests {
req.Body = h.bufferedBody(req.Body)
}

if req.ContentLength == 0 {
req.Body = nil // Issue golang/go#16036: nil Body for http.Transport retries
Expand Down Expand Up @@ -560,7 +539,7 @@ func (h Handler) prepareRequest(req *http.Request) error {
req.Header.Set("X-Forwarded-Proto", proto)
}

return nil
return req, nil
}

// reverseProxy performs a round-trip to the given backend and processes the response with the client.
Expand Down Expand Up @@ -807,7 +786,7 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, proxyErr er

// directRequest modifies only req.URL so that it points to the upstream
// in the given DialInfo. It must modify ONLY the request URL.
func (h Handler) directRequest(req *http.Request, di DialInfo) {
func (Handler) directRequest(req *http.Request, di DialInfo) {
// we need a host, so set the upstream's host address
reqHost := di.Address

Expand Down Expand Up @@ -845,6 +824,42 @@ func (h Handler) bufferedBody(originalBody io.ReadCloser) io.ReadCloser {
}
}

// cloneRequest makes a semi-deep clone of origReq.
//
// Most of this code is borrowed from the Go stdlib reverse proxy,
// but we make a shallow-ish clone the request (deep clone only
// the headers and URL) so we can avoid manipulating the original
// request when using it to proxy upstream. This prevents request
// corruption and data races.
func cloneRequest(origReq *http.Request) *http.Request {
req := new(http.Request)
*req = *origReq
if origReq.URL != nil {
newURL := new(url.URL)
*newURL = *origReq.URL
if origReq.URL.User != nil {
newURL.User = new(url.Userinfo)
*newURL.User = *origReq.URL.User
}
// sanitize the request URL; we expect it to not contain the
// scheme and host since those should be determined by r.TLS
// and r.Host respectively, but some clients may include it
// in the request-line, which is technically valid in HTTP,
// but breaks reverseproxy behaviour, overriding how the
// dialer will behave. See #4237 for context.
newURL.Scheme = ""
newURL.Host = ""
req.URL = newURL
}
if origReq.Header != nil {
req.Header = origReq.Header.Clone()
}
if origReq.Trailer != nil {
req.Trailer = origReq.Trailer.Clone()
}
return req
}

func copyHeader(dst, src http.Header) {
for k, vv := range src {
for _, v := range vv {
Expand Down

0 comments on commit f5e1049

Please sign in to comment.