middleware: Do not trust X-Forwarded-For; use X-Real-Ip, set from nginx.

The `X-Forwarded-For` header is a list of proxies' IP addresses; each
proxy appends the remote address of the host it received its request
from to the list, as it passes the request down.  A naïve parsing, as
SetRemoteAddrFromForwardedFor did, would thus interpret the first
address in the list as the client's IP.

However, clients can pass in arbitrary `X-Forwarded-For` headers,
which would allow them to spoof their IP address.  `nginx`'s behavior
is to treat the addresses as untrusted unless they match an allowlist
of known proxies.  By setting `real_ip_recursive on`, it also allows
this behavior to be applied repeatedly, moving from right to left down
the `X-Forwarded-For` list, stopping at the right-most that is
untrusted.

Rather than re-implement this logic in Django, pass the first
untrusted value that `nginx` computer down into Django via `X-Real-Ip`
header.  This allows consistent IP addresses in logs between `nginx`
and Django.

Proxied calls into Tornado (which don't use UWSGI) already passed this
header, as Tornado logging respects it.

puppet/zulip/files/nginx/zulip-include-common/proxy

@@ -4,5 +4,6 @@ proxy_http_version 1.1;
 proxy_set_header Connection "";
 proxy_set_header Host $host;
 proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+proxy_set_header X-Real-Ip $remote_addr;
 proxy_next_upstream off;
 proxy_redirect off;

puppet/zulip/files/nginx/zulip-include-frontend/app

@@ -52,6 +52,7 @@ location /api/v1/events {
 # Send everything else to Django via uWSGI
 location / {
     include uwsgi_params;
+    uwsgi_param HTTP_X_REAL_IP $remote_addr;
     uwsgi_pass django;
 }
 
@@ -65,12 +66,14 @@ location /thumbnail {
     include /etc/nginx/zulip-include/api_headers;
 
     include uwsgi_params;
+    uwsgi_param HTTP_X_REAL_IP $remote_addr;
     uwsgi_pass django;
 }
 location /avatar {
     include /etc/nginx/zulip-include/api_headers;
 
     include uwsgi_params;
+    uwsgi_param HTTP_X_REAL_IP $remote_addr;
     uwsgi_pass django;
 }
 
@@ -79,6 +82,7 @@ location /api/ {
     include /etc/nginx/zulip-include/api_headers;
 
     include uwsgi_params;
+    uwsgi_param HTTP_X_REAL_IP $remote_addr;
     uwsgi_pass django;
 }
 

puppet/zulip/templates/accept-loadbalancer.conf.template.erb

@@ -1,7 +1,11 @@
 # Configuration for making Zulip app frontends accept the
-# X-Forwarded-For header used by the Zulip proxy.  The accepted IP
+# X-Forwarded-For header used by proxies.  The trusted IP addresses
-# addresses here are set in /etc/zulip/zulip.conf.
+# here are set by `loadbalancer.ips` in /etc/zulip/zulip.conf.
+#
+# This causes us to update $remote_addr, which we use in logging, and
+# also pass down to Zulip as X-Real-Ip.
 real_ip_header X-Forwarded-For;
+real_ip_recursive on;
 <% @loadbalancers.each do |host| -%>
 set_real_ip_from <%= host %>;
 <%

zerver/middleware.py

@@ -519,25 +519,29 @@ class HostDomainMiddleware(MiddlewareMixin):
         return None
 
 
-class SetRemoteAddrFromForwardedFor(MiddlewareMixin):
+class SetRemoteAddrFromRealIpHeader(MiddlewareMixin):
-    """
+    """Middleware that sets REMOTE_ADDR based on the X-Real-Ip header.
-    Middleware that sets REMOTE_ADDR based on the HTTP_X_FORWARDED_FOR.
+
+    This middleware is similar to Django's old
+    SetRemoteAddrFromForwardedFor middleware.  We use X-Real-Ip, and
+    not X-Forwarded-For, because the latter is a list of proxies, some
+    number of which are trusted by us, and some of which could be
+    arbitrarily set by the user.  nginx has already parsed which are
+    which, and has set X-Real-Ip to the first one, going right to
+    left, which is untrusted.
+
+    Since we are always deployed behind nginx, we can trust the
+    X-Real-Ip which is so set.  In development, we fall back to the
+    REMOTE_ADDR supplied by the server.
 
-    This middleware replicates Django's former SetRemoteAddrFromForwardedFor middleware.
-    Because Zulip sits behind a NGINX reverse proxy, if the HTTP_X_FORWARDED_FOR
-    is set in the request, then it has properly been set by NGINX.
-    Therefore HTTP_X_FORWARDED_FOR's value is trusted.
     """
 
     def process_request(self, request: HttpRequest) -> None:
         try:
-            real_ip = request.META["HTTP_X_FORWARDED_FOR"]
+            real_ip = request.META["HTTP_X_REAL_IP"]
         except KeyError:
             return None
         else:
-            # HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs.
-            # For NGINX reverse proxy servers, the client's IP will be the first one.
-            real_ip = real_ip.split(",")[0].strip()
             request.META["REMOTE_ADDR"] = real_ip
 
 

zproject/computed_settings.py

@@ -172,7 +172,7 @@ MIDDLEWARE = (
     # With the exception of it's dependencies,
     # our logging middleware should be the top middleware item.
     "zerver.middleware.TagRequests",
-    "zerver.middleware.SetRemoteAddrFromForwardedFor",
+    "zerver.middleware.SetRemoteAddrFromRealIpHeader",
     "zerver.middleware.RequestContext",
     "zerver.middleware.LogRequests",
     "zerver.middleware.JsonErrorHandler",