From 8a77cca341e4c0e6b0e0fbc3c50d32edaeadda23 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Fri, 16 Jun 2023 16:41:29 +0000 Subject: [PATCH] middleware: Detect reverse proxy misconfigurations. Combine nginx and Django middlware to stop putting misleading warnings about `CSRF_TRUSTED_ORIGINS` when the issue is untrusted proxies. This attempts to, in the error logs, diagnose and suggest next steps to fix common proxy misconfigurations. See also #24599 and zulip/docker-zulip#403. --- puppet/zulip/files/nginx/uwsgi_params | 1 + .../files/nginx/zulip-include-common/proxy | 1 + .../nginx/trusted-proto.template.erb | 9 ++++ zerver/middleware.py | 44 +++++++++++++++++++ zproject/computed_settings.py | 1 + 5 files changed, 56 insertions(+) diff --git a/puppet/zulip/files/nginx/uwsgi_params b/puppet/zulip/files/nginx/uwsgi_params index b18b74d026..09a1006932 100644 --- a/puppet/zulip/files/nginx/uwsgi_params +++ b/puppet/zulip/files/nginx/uwsgi_params @@ -16,5 +16,6 @@ uwsgi_param SERVER_NAME $server_name; uwsgi_param HTTP_X_REAL_IP $remote_addr; uwsgi_param HTTP_X_FORWARDED_PROTO $trusted_x_forwarded_proto; uwsgi_param HTTP_X_FORWARDED_SSL ""; +uwsgi_param HTTP_X_PROXY_MISCONFIGURATION $x_proxy_misconfiguration; uwsgi_pass django; diff --git a/puppet/zulip/files/nginx/zulip-include-common/proxy b/puppet/zulip/files/nginx/zulip-include-common/proxy index 4978da7fe8..e740420ccd 100644 --- a/puppet/zulip/files/nginx/zulip-include-common/proxy +++ b/puppet/zulip/files/nginx/zulip-include-common/proxy @@ -6,5 +6,6 @@ proxy_set_header Host $host; proxy_set_header X-Forwarded-Proto $trusted_x_forwarded_proto; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Real-Ip $remote_addr; +proxy_set_header X-Proxy-Misconfiguration $x_proxy_misconfiguration; proxy_next_upstream off; proxy_redirect off; diff --git a/puppet/zulip/templates/nginx/trusted-proto.template.erb b/puppet/zulip/templates/nginx/trusted-proto.template.erb index 6a342fc840..ea764f367c 100644 --- a/puppet/zulip/templates/nginx/trusted-proto.template.erb +++ b/puppet/zulip/templates/nginx/trusted-proto.template.erb @@ -3,6 +3,10 @@ map $remote_addr $trusted_x_forwarded_proto { default $scheme; } +map $http_x_forwarded_for $x_proxy_misconfiguration { + default ""; + "~." "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, @@ -18,4 +22,9 @@ 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"; + default ""; +} <% end %> diff --git a/zerver/middleware.py b/zerver/middleware.py index 0d0c1a866d..30df1fcfb0 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -600,6 +600,50 @@ class SetRemoteAddrFromRealIpHeader(MiddlewareMixin): request.META["REMOTE_ADDR"] = real_ip +class ProxyMisconfigurationError(JsonableError): + http_status_code = 500 + data_fields = ["proxy_reason"] + + def __init__(self, proxy_reason: str) -> None: + self.proxy_reason = proxy_reason + + @staticmethod + def msg_format() -> str: + return _("Reverse proxy misconfiguration: {proxy_reason}") + + +class DetectProxyMisconfiguration(MiddlewareMixin): + def process_view( + self, + request: HttpRequest, + view_func: Callable[Concatenate[HttpRequest, ParamT], HttpResponseBase], + args: List[object], + kwargs: Dict[str, Any], + ) -> None: + proxy_state_header = request.headers.get("X-Proxy-Misconfiguration", "") + # 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, + # but there was no X-Forwarded-Proto header + # + # Note that the first two may be false-positives. We only + # display the error if the request also came in over HTTP (and + # a trusted proxy didn't say they get it over HTTPS), which + # should be impossible because Zulip only supports external + # https:// URLs in production. nginx configuration ensures + # that request.is_secure() is only true if our nginx is + # serving the request over HTTPS, or it came from a trusted + # proxy which reports that it is doing so. This will result + # in false negatives if Zulip's nginx is serving responses + # over HTTPS to a proxy whose IP is not configured, or + # misconfigured, but we cannot distinguish this from a random + # client which is providing proxy headers to a correctly + # configured Zulip. + if proxy_state_header != "" and not request.is_secure(): + raise ProxyMisconfigurationError(proxy_state_header) + + def alter_content(request: HttpRequest, content: bytes) -> bytes: first_paragraph_text = get_content_description(content, request) placeholder_open_graph_description = RequestNotes.get_notes( diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 9f2bcba168..8ffebe12d7 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -176,6 +176,7 @@ MIDDLEWARE = [ "django.middleware.common.CommonMiddleware", "zerver.middleware.LocaleMiddleware", "zerver.middleware.HostDomainMiddleware", + "zerver.middleware.DetectProxyMisconfiguration", "django.middleware.csrf.CsrfViewMiddleware", # Make sure 2FA middlewares come after authentication middleware. "django_otp.middleware.OTPMiddleware", # Required by two factor auth.