From c50094fc9d34099efd705700e6d2efa2fa065412 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sun, 6 Mar 2022 18:51:55 -0500 Subject: reverseproxy: Implement trusted proxies for `X-Forwarded-*` headers (#4507) --- modules/caddyhttp/reverseproxy/reverseproxy.go | 187 ++++++++++++++++++++++--- 1 file changed, 164 insertions(+), 23 deletions(-) (limited to 'modules/caddyhttp/reverseproxy/reverseproxy.go') 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 "" -- cgit v1.2.3