[backport] #716: [fix] Stopped trusting X-Forwarded-For for the client IP address (to 26.09)

docs/user/settings.rst

@@ -1025,18 +1025,45 @@ Nginx
 - **Valid Values:** ``True``, ``False``.
 - **Default:** ``False``.
 
-``NINGX_REAL_REMOTE_ADDR``
-~~~~~~~~~~~~~~~~~~~~~~~~~~
+.. warning::
+
+    When enabled, the generated nginx configuration settings do not apply:
+    ``SSL_CERT_MODE``, ``NGINX_HTTP2``, ``NGINX_CLIENT_BODY_SIZE``,
+    ``NGINX_IP6_STRING``, ``NGINX_IP6_80_STRING``,
+    ``NGINX_ADMIN_ALLOW_NETWORK``, ``NGINX_SERVER_NAME_HASH_BUCKET``,
+    ``NGINX_SSL_CONFIG``, ``NGINX_80_CONFIG``, ``NGINX_GZIP_SWITCH``,
+    ``NGINX_GZIP_LEVEL``, ``NGINX_GZIP_PROXIED``,
+    ``NGINX_GZIP_MIN_LENGTH``, ``NGINX_GZIP_TYPES``,
+    ``NGINX_BROTLI_SWITCH``, ``NGINX_HTTPS_ALLOWED_IPS``,
+    ``NGINX_HTTP_ALLOW``, and ``NGINX_TRUSTED_PROXIES``. Configure these
+    settings in the mounted nginx configuration instead. ``NGINX_PORT``
+    and ``NGINX_SSL_PORT`` continue to control the published Docker ports.
+
+``NGINX_TRUSTED_PROXIES``
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- **Explanation:** IP addresses or subnets of the reverse proxies placed
+  in front of nginx (e.g. a load balancer or an ingress). The client IP
+  address is read from the ``X-Forwarded-For`` header only when the
+  request comes from one of these addresses. The client IP address is also
+  used by the ``allow`` rules of ``NGINX_ADMIN_ALLOW_NETWORK`` and
+  ``NGINX_HTTPS_ALLOWED_IPS``.
+- **Valid Values:** A comma separated string of valid IP addresses or IP
+  networks.
+- **Default:** ``''`` (empty, ``X-Forwarded-For`` is ignored).
+- **Example:** ``192.0.2.10``.
+
+.. note::
+
+    List only the addresses of your reverse proxies. Any host in these
+    networks, including the Docker network of the containers, can set its
+    own IP address through the ``X-Forwarded-For`` header.
+
+.. warning::
 
-- **Explanation:** The nginx header to get the value of the real IP
-  address of Access points. Example if a reverse proxy is used in your
-  cluster (Example if you are using an Ingress), then the real IP of the
-  AP is most likely the ``$http_x_forwarded_for``. If
-  ``$http_x_forwarded_for`` returns a list, you can use ``$real_ip`` for
-  getting first element of the list.
-- **Valid Values:** ``$remote_addr``, ``$http_x_forwarded_for``,
-  ``$realip_remote_addr``, ``$real_ip``.
-- **Default:** ``$real_ip``.
+    This setting does not apply when ``NGINX_CUSTOM_FILE`` is ``True``, in
+    that case, you have to configure trusted proxies in the mounted nginx
+    configuration instead.
 
 OpenVPN
 -------

images/common/init_command.sh

@@ -52,12 +52,28 @@ elif [ "$MODULE_NAME" = 'openvpn' ]; then
 elif [ "$MODULE_NAME" = 'nginx' ]; then
 	rm -rf /etc/nginx/conf.d/default.conf
 	if [ "$NGINX_CUSTOM_FILE" = 'True' ]; then
-		nginx -g 'daemon off;'
+		exec nginx -g 'daemon off;'
 	fi
 	# Expand escape sequences in the optional events block so it can be injected
 	# correctly into the nginx configuration template.
 	NGINX_EVENTS_BLOCK=$(printf "%b" "${NGINX_EVENTS_BLOCK:-}")
 	export NGINX_EVENTS_BLOCK
+	NGINX_REAL_IP_BLOCK=""
+	# Disable glob expansion and trust X-Forwarded-For only from valid proxy IPs.
+	set -f
+	for proxy in $(printf "%s" "${NGINX_TRUSTED_PROXIES:-}" | tr ',' ' '); do
+		if ! python3 -c 'import ipaddress, sys; ipaddress.ip_network(sys.argv[1], strict=False)' "$proxy" >/dev/null 2>&1; then
+			echo "ERROR: invalid NGINX_TRUSTED_PROXIES entry: ${proxy}" >&2
+			exit 1
+		fi
+		NGINX_REAL_IP_BLOCK="${NGINX_REAL_IP_BLOCK}set_real_ip_from ${proxy};\n    "
+	done
+	set +f
+	if [ -n "$NGINX_REAL_IP_BLOCK" ]; then
+		NGINX_REAL_IP_BLOCK="${NGINX_REAL_IP_BLOCK}real_ip_header X-Forwarded-For;\n    real_ip_recursive on;"
+	fi
+	NGINX_REAL_IP_BLOCK=$(printf "%b" "$NGINX_REAL_IP_BLOCK")
+	export NGINX_REAL_IP_BLOCK
 	# Use a sentinel value when the variable is unset. Since envsubst cannot
 	# conditionally omit directives, we later remove any line containing this
 	# sentinel from the generated nginx.conf.

images/openwisp_nginx/Dockerfile

@@ -136,7 +136,7 @@ ENV MODULE_NAME=nginx \
     NGINX_WORKER_CONNECTIONS=1024 \
     NGINX_WORKER_RLIMIT_NOFILE=__UNSET__ \
     NGINX_EVENTS_BLOCK='' \
-    NINGX_REAL_REMOTE_ADDR='$real_ip' \
+    NGINX_TRUSTED_PROXIES='' \
     NGINX_SSL_PORT=443 \
     # USWGI pass_port
     DASHBOARD_APP_PORT=8000 \

images/openwisp_nginx/nginx.template.conf

@@ -26,7 +26,7 @@ http {
                       'status: ${DOLLAR}status ${DOLLAR}body_bytes_sent "${DOLLAR}http_referer" '
                       '"${DOLLAR}http_user_agent" http_x_forwarded_for: '
                       '${DOLLAR}http_x_forwarded_for - remote_addr: ${DOLLAR}remote_addr - '
-                      'realip_remote_addr: ${DOLLAR}realip_remote_addr - real_ip: ${DOLLAR}real_ip';
+                      'realip_remote_addr: ${DOLLAR}realip_remote_addr';
 
     # Nginx Logging
     access_log /dev/stdout main;
@@ -35,11 +35,7 @@ http {
     sendfile           on;
     keepalive_timeout  65;
 
-    # Map $real_ip
-    map ${DOLLAR}http_x_forwarded_for ${DOLLAR}real_ip {
-        ~^(\d+\.\d+\.\d+\.\d+) ${DOLLAR}1;
-        default ${DOLLAR}remote_addr;
-    }
+    ${NGINX_REAL_IP_BLOCK}
 
     server_names_hash_bucket_size $NGINX_SERVER_NAME_HASH_BUCKET;
     server_tokens off;

images/openwisp_nginx/openwisp.ssl.template.conf

@@ -42,7 +42,7 @@ server {
     location /ws/ {
         rewrite  ^/(.*)  /${DOLLAR}1 break;
         proxy_set_header X-Real-IP ${DOLLAR}remote_addr;
-        proxy_set_header X-Forwarded-For ${DOLLAR}proxy_add_x_forwarded_for;
+        proxy_set_header X-Forwarded-For ${DOLLAR}remote_addr;
         proxy_set_header Host ${DOLLAR}http_host;
         proxy_redirect off;
         proxy_pass http://${WEBSOCKET_APP_SERVICE}:${WEBSOCKET_APP_PORT};
@@ -76,6 +76,6 @@ server {
         uwsgi_pass ${APP_SERVICE}:${APP_PORT};
         include uwsgi_params;
         uwsgi_param HTTP_X_FORWARDED_PROTO https;
-        uwsgi_param REMOTE_ADDR ${NINGX_REAL_REMOTE_ADDR};
+        uwsgi_param REMOTE_ADDR ${DOLLAR}remote_addr;
     }
 }

images/openwisp_nginx/openwisp.template.conf

@@ -24,7 +24,7 @@ server {
     location /ws/ {
         rewrite  ^/(.*)  /${DOLLAR}1 break;
         proxy_set_header X-Real-IP ${DOLLAR}remote_addr;
-        proxy_set_header X-Forwarded-For ${DOLLAR}proxy_add_x_forwarded_for;
+        proxy_set_header X-Forwarded-For ${DOLLAR}remote_addr;
         proxy_set_header Host ${DOLLAR}http_host;
         proxy_redirect off;
         proxy_pass http://${WEBSOCKET_APP_SERVICE}:${WEBSOCKET_APP_PORT};
@@ -56,7 +56,7 @@ server {
         uwsgi_pass ${APP_SERVICE}:${APP_PORT};
         include uwsgi_params;
         uwsgi_param HTTP_X_FORWARDED_PROTO http;
-        uwsgi_param REMOTE_ADDR ${NINGX_REAL_REMOTE_ADDR};
+        uwsgi_param REMOTE_ADDR ${DOLLAR}remote_addr;
     }
     location @deny {
         return 301 https://${DOLLAR}host${DOLLAR}request_uri;

tests/runtests.py

@@ -1403,6 +1403,78 @@ def test_freeradius_debug_mode_is_independent(self):
                     self.assertEqual(result.returncode, 0, result.stderr)
                     self.assertEqual(result.stdout, expected)
 
+    def test_nginx_trusted_proxies(self):
+        repository_root = Path(__file__).resolve().parents[1]
+        with tempfile.TemporaryDirectory() as tmpdir:
+            tmpdir = Path(tmpdir)
+            config_directory = tmpdir / "nginx_config"
+            config_directory.mkdir()
+            (config_directory / "nginx.template.conf").write_text(
+                (
+                    repository_root
+                    / "images"
+                    / "openwisp_nginx"
+                    / "nginx.template.conf"
+                ).read_text()
+            )
+            (tmpdir / "init_command.sh").write_text(
+                (repository_root / "images" / "common" / "init_command.sh")
+                .read_text()
+                .replace("/etc/nginx", str(config_directory))
+            )
+            (tmpdir / "utils.sh").write_text(
+                "init_conf() { :; }\nenvsubst_create_config() { :; }\n"
+            )
+            (tmpdir / "envsubst").write_text(
+                "#!/usr/bin/env python3\n"
+                "import os\n"
+                "import sys\n"
+                "content = sys.stdin.read()\n"
+                "for key, value in os.environ.items():\n"
+                "    content = content.replace(f'${{{key}}}', value)\n"
+                "sys.stdout.write(content)\n"
+            )
+            (tmpdir / "nginx").write_text("#!/bin/sh\n:\n")
+            (tmpdir / "envsubst").chmod(0o755)
+            (tmpdir / "nginx").chmod(0o755)
+            environment = os.environ.copy()
+            environment.update(
+                {
+                    "MODULE_NAME": "nginx",
+                    "SSL_CERT_MODE": "No",
+                    "NGINX_CUSTOM_FILE": "False",
+                    "PATH": f"{tmpdir}:{environment['PATH']}",
+                }
+            )
+            for proxy, expected_returncode in (
+                ("deadbeef", 1),
+                ("192.0.2.10/33", 1),
+                ("2001:db8::1/129", 1),
+                ("999.999.999.999/32", 1),
+                ("192.0.2.10,2001:db8::/32", 0),
+            ):
+                with self.subTest(proxy=proxy):
+                    environment["NGINX_TRUSTED_PROXIES"] = proxy
+                    result = subprocess.run(
+                        ["bash", "init_command.sh"],
+                        cwd=tmpdir,
+                        check=False,
+                        capture_output=True,
+                        text=True,
+                        env=environment,
+                    )
+                    self.assertEqual(result.returncode, expected_returncode)
+                    if expected_returncode:
+                        self.assertIn(
+                            "invalid NGINX_TRUSTED_PROXIES entry", result.stderr
+                        )
+                    else:
+                        config = (config_directory / "nginx.conf").read_text()
+                        self.assertIn("set_real_ip_from 192.0.2.10;", config)
+                        self.assertIn("set_real_ip_from 2001:db8::/32;", config)
+                        self.assertIn("real_ip_header X-Forwarded-For;", config)
+                        self.assertIn("real_ip_recursive on;", config)
+
     def test_nginx_development_headers_clear_hsts(self):
         repository_root = Path(__file__).resolve().parents[1]
         header_file = (