-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
reverseproxy: Implement trusted proxies for X-Forwarded-*
headers
#4507
Changes from all commits
833d246
d9913e3
f3518c5
3830d5d
037b6bc
a7de48b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
] | ||
} | ||
] | ||
} | ||
] | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually let users add that one on their own, because it's usually redundant since we keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm considering that maybe we should add It's not uncommon to add it when proxying over HTTPS, less common over HTTP since WDYT @mholt ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, I added this in the latest commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How much you wanna bet we'll get a report that this breaks someone because the incoming Host is sensitive for some reason, and they want the proxy to NOT send it on to the upstream, which may be an arbitrary user application (or something untrusted)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's probably fine, they can just do |
||
// 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 "" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we remove the existing XFF headers coming from untrusted servers even in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added
.Del()
calls here.I'm still not 100% sure just "return early" is the right thing to do here, I'd like @mholt to chime in on what we should do if
req.RemoteAddr
can't be split 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses that can't be split are almost always missing a port. I usually disregard these errors mostly, except that I assume the entire input was already just the host.
But I'm not sure what the implications are if a RemoteAddr can't be split, especially in this context. We could try this current logic and see if anyone complains... I'm not sure I've ever really seen a RemoteAddr without a port (or a malformed RemoteAddr in general).