reverseproxy: Implement trusted proxies for `X-Forwarded-*` headers (#4507)

1 files changed, 164 insertions, 23 deletions

diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 53911c8..a5bdc31 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -90,13 +90,20 @@ type Handler struct {
 	// to the client immediately.
 	FlushInterval caddy.Duration `json:"flush_interval,omitempty"`
 
+	// A list of IP ranges (supports CIDR notation) from which
+	// X-Forwarded-* header values should be trusted. By default,
+	// no proxies are trusted, so existing values will be ignored
+	// when setting these headers. If the proxy is trusted, then
+	// existing values will be used when constructing the final
+	// header values.
+	TrustedProxies []string `json:"trusted_proxies,omitempty"`
+
 	// Headers manipulates headers between Caddy and the backend.
 	// By default, all headers are passed-thru without changes,
 	// with the exceptions of special hop-by-hop headers.
 	//
-	// X-Forwarded-For and X-Forwarded-Proto are also set
-	// implicitly, but this may change in the future if the official
-	// standardized Forwarded header field gains more adoption.
+	// X-Forwarded-For, X-Forwarded-Proto and X-Forwarded-Host
+	// are also set implicitly.
 	Headers *headers.Handler `json:"headers,omitempty"`
 
 	// If true, the entire request body will be read and buffered
@@ -133,6 +140,9 @@ type Handler struct {
 	Transport http.RoundTripper `json:"-"`
 	CB        CircuitBreaker    `json:"-"`
 
+	// Holds the parsed CIDR ranges from TrustedProxies
+	trustedProxies []*net.IPNet
+
 	// Holds the named response matchers from the Caddyfile while adapting
 	responseMatchers map[string]caddyhttp.ResponseMatcher
 
@@ -192,6 +202,30 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 		h.CB = mod.(CircuitBreaker)
 	}
 
+	// parse trusted proxy CIDRs ahead of time
+	for _, str := range h.TrustedProxies {
+		if strings.Contains(str, "/") {
+			_, ipNet, err := net.ParseCIDR(str)
+			if err != nil {
+				return fmt.Errorf("parsing CIDR expression: %v", err)
+			}
+			h.trustedProxies = append(h.trustedProxies, ipNet)
+		} else {
+			ip := net.ParseIP(str)
+			if ip == nil {
+				return fmt.Errorf("invalid IP address: %s", str)
+			}
+			if ipv4 := ip.To4(); ipv4 != nil {
+				ip = ipv4
+			}
+			mask := len(ip) * 8
+			h.trustedProxies = append(h.trustedProxies, &net.IPNet{
+				IP:   ip,
+				Mask: net.CIDRMask(mask, mask),
+			})
+		}
+	}
+
 	// ensure any embedded headers handler module gets provisioned
 	// (see https://caddy.community/t/set-cookie-manipulation-in-reverse-proxy/7666?u=matt
 	// for what happens if we forget to provision it)
@@ -514,32 +548,103 @@ func (h Handler) prepareRequest(req *http.Request) (*http.Request, error) {
 		req.Header.Set("Upgrade", reqUpType)
 	}
 
-	if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
-		// If we aren't the first proxy retain prior
-		// X-Forwarded-For information as a comma+space
-		// separated list and fold multiple headers into one.
-		prior, ok := req.Header["X-Forwarded-For"]
-		omit := ok && prior == nil // Issue 38079: nil now means don't populate the header
-		if len(prior) > 0 {
-			clientIP = strings.Join(prior, ", ") + ", " + clientIP
-		}
-		if !omit {
-			req.Header.Set("X-Forwarded-For", clientIP)
-		}
+	// Add the supported X-Forwarded-* headers
+	err := h.addForwardedHeaders(req)
+	if err != nil {
+		return nil, err
+	}
+
+	return req, nil
+}
+
+// addForwardedHeaders adds the de-facto standard X-Forwarded-*
+// headers to the request before it is sent upstream.
+//
+// These headers are security sensitive, so care is taken to only
+// use existing values for these headers from the incoming request
+// if the client IP is trusted (i.e. coming from a trusted proxy
+// sitting in front of this server). If the request didn't have
+// the headers at all, then they will be added with the values
+// that we can glean from the request.
+func (h Handler) addForwardedHeaders(req *http.Request) error {
+	// Parse the remote IP, ignore the error as non-fatal,
+	// but the remote IP is required to continue, so we
+	// just return early. This should probably never happen
+	// though, unless some other module manipulated the request's
+	// remote address and used an invalid value.
+	clientIP, _, err := net.SplitHostPort(req.RemoteAddr)
+	if err != nil {
+		// Remove the `X-Forwarded-*` headers to avoid upstreams
+		// potentially trusting a header that came from the client
+		req.Header.Del("X-Forwarded-For")
+		req.Header.Del("X-Forwarded-Proto")
+		req.Header.Del("X-Forwarded-Host")
+		return nil
+	}
+
+	// Client IP may contain a zone if IPv6, so we need
+	// to pull that out before parsing the IP
+	if idx := strings.IndexByte(clientIP, '%'); idx >= 0 {
+		clientIP = clientIP[:idx]
+	}
+	ip := net.ParseIP(clientIP)
+	if ip == nil {
+		return fmt.Errorf("invalid client IP address: %s", clientIP)
 	}
 
-	prior, ok := req.Header["X-Forwarded-Proto"]
-	omit := ok && prior == nil
-	if len(prior) == 0 && !omit {
-		// set X-Forwarded-Proto; many backend apps expect this too
-		proto := "https"
-		if req.TLS == nil {
-			proto = "http"
+	// Check if the client is a trusted proxy
+	trusted := false
+	for _, ipRange := range h.trustedProxies {
+		if ipRange.Contains(ip) {
+			trusted = true
+			break
 		}
+	}
+
+	// If we aren't the first proxy, and the proxy is trusted,
+	// retain prior X-Forwarded-For information as a comma+space
+	// separated list and fold multiple headers into one.
+	clientXFF := clientIP
+	prior, ok, omit := allHeaderValues(req.Header, "X-Forwarded-For")
+	if trusted && ok && prior != "" {
+		clientXFF = prior + ", " + clientXFF
+	}
+	if !omit {
+		req.Header.Set("X-Forwarded-For", clientXFF)
+	}
+
+	// Set X-Forwarded-Proto; many backend apps expect this,
+	// so that they can properly craft URLs with the right
+	// scheme to match the original request
+	proto := "https"
+	if req.TLS == nil {
+		proto = "http"
+	}
+	prior, ok, omit = lastHeaderValue(req.Header, "X-Forwarded-Proto")
+	if trusted && ok && prior != "" {
+		proto = prior
+	}
+	if !omit {
 		req.Header.Set("X-Forwarded-Proto", proto)
 	}
 
-	return req, nil
+	// Set X-Forwarded-Host; often this is redundant because
+	// we pass through the request Host as-is, but in situations
+	// where we proxy over HTTPS, the user may need to override
+	// Host themselves, so it's helpful to send the original too.
+	host, _, err := net.SplitHostPort(req.Host)
+	if err != nil {
+		host = req.Host // OK; there probably was no port
+	}
+	prior, ok, omit = lastHeaderValue(req.Header, "X-Forwarded-Host")
+	if trusted && ok && prior != "" {
+		host = prior
+	}
+	if !omit {
+		req.Header.Set("X-Forwarded-Host", host)
+	}
+
+	return nil
 }
 
 // reverseProxy performs a round-trip to the given backend and processes the response with the client.
@@ -868,6 +973,42 @@ func copyHeader(dst, src http.Header) {
 	}
 }
 
+// allHeaderValues gets all values for a given header field,
+// joined by a comma and space if more than one is set. If the
+// header field is nil, then the omit is true, meaning some
+// earlier logic in the server wanted to prevent this header from
+// getting written at all. If the header is empty, then ok is
+// false. Callers should still check that the value is not empty
+// (the header field may be set but have an empty value).
+func allHeaderValues(h http.Header, field string) (value string, ok bool, omit bool) {
+	values, ok := h[http.CanonicalHeaderKey(field)]
+	if ok && values == nil {
+		return "", true, true
+	}
+	if len(values) == 0 {
+		return "", false, false
+	}
+	return strings.Join(values, ", "), true, false
+}
+
+// lastHeaderValue gets the last value for a given header field
+// if more than one is set. If the header field is nil, then
+// the omit is true, meaning some earlier logic in the server
+// wanted to prevent this header from getting written at all.
+// If the header is empty, then ok is false. Callers should
+// still check that the value is not empty (the header field
+// may be set but have an empty value).
+func lastHeaderValue(h http.Header, field string) (value string, ok bool, omit bool) {
+	values, ok := h[http.CanonicalHeaderKey(field)]
+	if ok && values == nil {
+		return "", true, true
+	}
+	if len(values) == 0 {
+		return "", false, false
+	}
+	return values[len(values)-1], true, false
+}
+
 func upgradeType(h http.Header) string {
 	if !httpguts.HeaderValuesContainsToken(h["Connection"], "Upgrade") {
 		return ""