From 135acfea93687aa82f430736009e6ebcf78f755e Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Tue, 12 Sep 2023 15:09:42 +0000 Subject: [PATCH] nginx: Suppress proxy warnings when the proxy itself sent the request. This is common in cases where the reverse proxy itself is making health-check requests to the Zulip server; these requests have no X-Forwarded-* headers, so would normally hit the error case of "request through the proxy, but no X-Forwarded-Proto header". Add an additional special-case for when the request's originating IP address is resolved to be the reverse proxy itself; in these cases, HTTP requests with no X-Forwarded-Proto are acceptable. --- .../nginx/trusted-proto.template.erb | 26 ++++++++++++++----- zerver/middleware.py | 14 ++++++---- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/puppet/zulip/templates/nginx/trusted-proto.template.erb b/puppet/zulip/templates/nginx/trusted-proto.template.erb index ea764f367c..4fbc122483 100644 --- a/puppet/zulip/templates/nginx/trusted-proto.template.erb +++ b/puppet/zulip/templates/nginx/trusted-proto.template.erb @@ -8,9 +8,9 @@ map $http_x_forwarded_for $x_proxy_misconfiguration { "~." "No proxies configured in Zulip, but proxy headers detected from proxy at $remote_addr; see https://zulip.readthedocs.io/en/latest/production/deployment.html#putting-the-zulip-application-behind-a-reverse-proxy"; } <% else %> -# We do this in two steps because `geo` does not support variable -# interpolation in the value, but does support CIDR notation, -# which the loadbalancer list may use. +# Check if the request's original `REMOTE_ADDR` header was from a +# trusted host -- that is, the request did bounce through a proxy and +# we should trust `X-Forwarded-Proto` geo $realip_remote_addr $is_x_forwarded_proto_trusted { default 0; <% @loadbalancers.each do |host| -%> @@ -18,13 +18,27 @@ geo $realip_remote_addr $is_x_forwarded_proto_trusted { <% end -%> } +# Check if the IP address that we resolved the request as coming +# (after looking at X-Fowarded-For, if any) from is actually the proxy +# itself. +geo $remote_addr $is_from_proxy { + default 0; +<% @loadbalancers.each do |host| -%> + <%= host %> 1; +<% end -%> +} + +# We set $trusted_x_forwarded_proto in two steps because `geo` does +# not support variable interpolation in the value, but does support +# CIDR notation, which the loadbalancer list may use. map $is_x_forwarded_proto_trusted $trusted_x_forwarded_proto { 0 $scheme; 1 $http_x_forwarded_proto; } -map "$is_x_forwarded_proto_trusted:$http_x_forwarded_proto" $x_proxy_misconfiguration { - "~^0:" "Incorrect reverse proxy IPs set in Zulip (try $remote_addr?); see https://zulip.readthedocs.io/en/latest/production/deployment.html#putting-the-zulip-application-behind-a-reverse-proxy"; - "~^1:$" "No X-Forwarded-Proto header sent from trusted proxy $realip_remote_addr; see example configurations in https://zulip.readthedocs.io/en/latest/production/deployment.html#putting-the-zulip-application-behind-a-reverse-proxy"; + +map "$is_from_proxy:$is_x_forwarded_proto_trusted:$http_x_forwarded_proto" $x_proxy_misconfiguration { + "~^0:0:" "Incorrect reverse proxy IPs set in Zulip (try $remote_addr?); see https://zulip.readthedocs.io/en/latest/production/deployment.html#putting-the-zulip-application-behind-a-reverse-proxy"; + "~^0:1:$" "No X-Forwarded-Proto header sent from trusted proxy $realip_remote_addr; see example configurations in https://zulip.readthedocs.io/en/latest/production/deployment.html#putting-the-zulip-application-behind-a-reverse-proxy"; default ""; } <% end %> diff --git a/zerver/middleware.py b/zerver/middleware.py index ae5c886abe..80167f5a42 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -622,7 +622,7 @@ class DetectProxyMisconfiguration(MiddlewareMixin): # Our nginx configuration sets this header if: # - there is an X-Forwarded-For set but no proxies configured in Zulip # - proxies are configured but the request did not come from them - # - proxies are configured and the request came from them, + # - proxies are configured and the request came through them, # but there was no X-Forwarded-Proto header # # Note that the first two may be false-positives. We only @@ -639,10 +639,14 @@ class DetectProxyMisconfiguration(MiddlewareMixin): # client which is providing proxy headers to a correctly # configured Zulip. # - # There is a complication to the above logic -- we do expect - # that requests not through the proxy may happen from - # localhost over HTTP (e.g. the email gateway). Skip warnings - # if the remote IP is localhost. + # There are a couple complications to the above logic -- + # first, we do expect that requests not through the proxy may + # happen from localhost over HTTP (e.g. the email gateway). + # Second, we also expect that the proxy itself may make + # healthcheck requests, which will not have an + # X-Forwarded-Proto or X-Forwarded-For. We handle the latter + # case in the nginx config (as it involves CIDRs and proxy + # ranges) and the former case here. if ( proxy_state_header != "" and not request.is_secure()