From c50094fc9d34099efd705700e6d2efa2fa065412 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sun, 6 Mar 2022 18:51:55 -0500 Subject: [PATCH] reverseproxy: Implement trusted proxies for `X-Forwarded-*` headers (#4507) --- .../reverse_proxy_trusted_proxies.txt | 56 +++++ modules/caddyhttp/reverseproxy/caddyfile.go | 23 +++ .../caddyhttp/reverseproxy/reverseproxy.go | 193 +++++++++++++++--- 3 files changed, 246 insertions(+), 26 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/reverse_proxy_trusted_proxies.txt diff --git a/caddytest/integration/caddyfile_adapt/reverse_proxy_trusted_proxies.txt b/caddytest/integration/caddyfile_adapt/reverse_proxy_trusted_proxies.txt new file mode 100644 index 00000000..5e112847 --- /dev/null +++ b/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" + } + ] + } + ] + } + ] + } + } + } + } +} diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go index a1fbeb32..e1272375 100644 --- a/modules/caddyhttp/reverseproxy/caddyfile.go +++ b/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] // header_up [+|-] [ []] // header_down [+|-] [ []] // @@ -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]) diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 53911c86..a5bdc310 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,34 +548,105 @@ 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) - } - } - - 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" - } - req.Header.Set("X-Forwarded-Proto", proto) + // 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) + } + + // 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) + } + + // 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. // (This method is mostly the beginning of what was borrowed from the net/http/httputil package in the // Go standard library which was used as the foundation.) @@ -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 ""