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

caddytest/integration/caddyfile_adapt/reverse_proxy_trusted_proxies.txt

@@ -0,0 +1,56 @@
+:8884
+
+reverse_proxy 127.0.0.1:65535 {
+	trusted_proxies 127.0.0.1
+}
+
+reverse_proxy 127.0.0.1:65535 {
+	trusted_proxies private_ranges
+}
+----------
+{
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":8884"
+					],
+					"routes": [
+						{
+							"handle": [
+								{
+									"handler": "reverse_proxy",
+									"trusted_proxies": [
+										"127.0.0.1"
+									],
+									"upstreams": [
+										{
+											"dial": "127.0.0.1:65535"
+										}
+									]
+								},
+								{
+									"handler": "reverse_proxy",
+									"trusted_proxies": [
+										"192.168.0.0/16",
+										"172.16.0.0/12",
+										"10.0.0.0/8",
+										"127.0.0.1/8",
+										"fd00::/8",
+										"::1"
+									],
+									"upstreams": [
+										{
+											"dial": "127.0.0.1:65535"
+										}
+									]
+								}
+							]
+						}
+					]
+				}
+			}
+		}
+	}
+}

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -82,6 +82,7 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //         buffer_requests
 //
 //         # header manipulation
+//         trusted_proxies [private_ranges] <ranges...>
 //         header_up   [+|-]<field> [<value|regexp> [<replacement>]]
 //         header_down [+|-]<field> [<value|regexp> [<replacement>]]
 //
@@ -485,6 +486,22 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				}
 				h.MaxBufferSize = int64(size)
 
+			case "trusted_proxies":
+				for d.NextArg() {
+					if d.Val() == "private_ranges" {
+						h.TrustedProxies = append(h.TrustedProxies, []string{
+							"192.168.0.0/16",
+							"172.16.0.0/12",
+							"10.0.0.0/8",
+							"127.0.0.1/8",
+							"fd00::/8",
+							"::1",
+						}...)
+						continue
+					}
+					h.TrustedProxies = append(h.TrustedProxies, d.Val())
+				}
+
 			case "header_up":
 				var err error
 
@@ -504,9 +521,15 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					if strings.EqualFold(args[0], "host") && (args[1] == "{hostport}" || args[1] == "{http.request.hostport}") {
 						log.Printf("[WARNING] Unnecessary header_up ('Host' field): the reverse proxy's default behavior is to pass headers to the upstream")
 					}
+					if strings.EqualFold(args[0], "x-forwarded-for") && (args[1] == "{remote}" || args[1] == "{http.request.remote}" || args[1] == "{remote_host}" || args[1] == "{http.request.remote.host}") {
+						log.Printf("[WARNING] Unnecessary header_up ('X-Forwarded-For' field): the reverse proxy's default behavior is to pass headers to the upstream")
+					}
 					if strings.EqualFold(args[0], "x-forwarded-proto") && (args[1] == "{scheme}" || args[1] == "{http.request.scheme}") {
 						log.Printf("[WARNING] Unnecessary header_up ('X-Forwarded-Proto' field): the reverse proxy's default behavior is to pass headers to the upstream")
 					}
+					if strings.EqualFold(args[0], "x-forwarded-host") && (args[1] == "{host}" || args[1] == "{http.request.host}" || args[1] == "{hostport}" || args[1] == "{http.request.hostport}") {
+						log.Printf("[WARNING] Unnecessary header_up ('X-Forwarded-Host' field): the reverse proxy's default behavior is to pass headers to the upstream")
+					}
 					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], "")
 				case 3:
 					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], args[2])

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
+	// Add the supported X-Forwarded-* headers
+	err := h.addForwardedHeaders(req)
+	if err != nil {
+		return nil, err
 	}
-		if !omit {
-			req.Header.Set("X-Forwarded-For", clientIP)
+
+	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)
+	}
+
+	// Check if the client is a trusted proxy
+	trusted := false
+	for _, ipRange := range h.trustedProxies {
+		if ipRange.Contains(ip) {
+			trusted = true
+			break
 		}
 	}
 
-	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
+	// 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 ""