perf(core): sanitize the X-Forwarded-* headers in the NGINX config (#13803)

Author: AlinsRan

apisix/cli/ngx_tpl.lua

@@ -273,6 +273,28 @@ stream {
 
 {% if enable_http then %}
 http {
+    # X-Forwarded-* sanitization, first half. The second is
+    # `handle_trusted_x_forwarded_headers` in apisix/init.lua.
+    #
+    # Every request is neutralized unconditionally, in the rewrite phase, in C.
+    # That is the case worth optimizing for: with no `apisix.trusted_addresses`
+    # configured no peer is trusted, so it is what every request gets.
+    #
+    # These keep the names the `set` directives they replace used, and hold the
+    # same thing: what X-Forwarded-Host and X-Forwarded-Port are given below.
+    #
+    # the port carried by the Host header, falling back to the listener's own
+    map $http_host $var_x_forwarded_port {
+        default         $server_port;
+        "~:(?<p>\\d+)$" $p;
+    }
+    # `$http_host` rather than `$host`: the port the client connected to belongs
+    # in X-Forwarded-Host, and `$host` drops it
+    map $http_host $var_x_forwarded_host {
+        default $http_host;
+        ""      $host;
+    }
+
     # put extra_lua_path in front of the builtin path
     # so user can override the source code
     lua_package_path  "{*extra_lua_path*}$prefix/deps/share/lua/5.1/?.lua;$prefix/deps/share/lua/5.1/?/init.lua;]=]
@@ -880,14 +902,43 @@ http {
 
             ### the following x-forwarded-* headers is to send to upstream server
 
-            set $var_x_forwarded_proto      $scheme;
-            set $var_x_forwarded_host       $host;
-            set $var_x_forwarded_port       $server_port;
-
+            # Take copies before neutralizing, so a trusted peer's own values can
+            # be put back. ngx_rewrite's `set` runs before headers_more's handler,
+            # which is what makes this ordering work -- do not reorder these.
+            #
+            # Reading `$http_x_forwarded_*` here indexes them, so they keep the
+            # client's raw value for the rest of the request. Nothing downstream
+            # derives from them -- the upstream headers come from `r->headers_in`
+            # and Lua's `ctx.var.http_x_forwarded_*` re-reads it through the prefix
+            # handler -- but an access log format that names them logs what the
+            # client sent. `$scheme` / `$var_x_forwarded_host` /
+            # `$var_x_forwarded_port` are the sanitized values.
+            set $original_x_forwarded_proto $http_x_forwarded_proto;
+            set $original_x_forwarded_host   $http_x_forwarded_host;
+            set $original_x_forwarded_port   $http_x_forwarded_port;
+            # X-Forwarded-For is the one that cannot be copied here. Unlike
+            # `$http_x_forwarded_proto` and friends, which are prefix variables and
+            # are re-evaluated on every read, `$http_x_forwarded_for` is a dedicated
+            # entry in `ngx_http_core_variables[]`; naming it in the configuration
+            # makes it indexed, and this `set` would then pin the client's value in
+            # `r->variables[]` for the whole request -- surviving the clear below and
+            # feeding it back to route `vars`, rate-limit keys and every other
+            # `ctx.var` reader. Lua fills the slot instead, in the one branch that
+            # destroys the value.
+            set $original_x_forwarded_for    '';
+            set $original_forwarded          $http_forwarded;
+            more_set_input_headers "X-Forwarded-Proto: $scheme";
+            more_set_input_headers "X-Forwarded-Host: $var_x_forwarded_host";
+            more_set_input_headers "X-Forwarded-Port: $var_x_forwarded_port";
+            more_set_input_headers "Forwarded: ";
+
+            # X-Forwarded-Proto/Host/Port are not set here: `r->headers_in` already
+            # holds the values this request should carry, and proxy_pass forwards it
+            # as it stands. That is also what lets a plugin rewrite them -- a
+            # `proxy_set_header` would overwrite the plugin's value with whatever the
+            # variable held. X-Forwarded-For is different: the connection address has
+            # to be appended, which only $proxy_add_x_forwarded_for does.
             proxy_set_header   X-Forwarded-For      $proxy_add_x_forwarded_for;
-            proxy_set_header   X-Forwarded-Proto    $var_x_forwarded_proto;
-            proxy_set_header   X-Forwarded-Host     $var_x_forwarded_host;
-            proxy_set_header   X-Forwarded-Port     $var_x_forwarded_port;
 
             {% if enabled_plugins["proxy-cache"] or enabled_plugins["graphql-proxy-cache"] then %}
             ###  the following configuration is to cache response content from upstream server
@@ -1000,10 +1051,13 @@ http {
             proxy_set_header   X-Real-IP         $remote_addr;
             proxy_pass_header  Date;
 
+            # X-Forwarded-Proto/Host/Port are not set here: `r->headers_in` already
+            # holds the values this request should carry, and proxy_pass forwards it
+            # as it stands. That is also what lets a plugin rewrite them -- a
+            # `proxy_set_header` would overwrite the plugin's value with whatever the
+            # variable held. X-Forwarded-For is different: the connection address has
+            # to be appended, which only $proxy_add_x_forwarded_for does.
             proxy_set_header   X-Forwarded-For      $proxy_add_x_forwarded_for;
-            proxy_set_header   X-Forwarded-Proto    $var_x_forwarded_proto;
-            proxy_set_header   X-Forwarded-Host     $var_x_forwarded_host;
-            proxy_set_header   X-Forwarded-Port     $var_x_forwarded_port;
 
             proxy_pass      $upstream_scheme://apisix_backend$upstream_uri;
 

apisix/core/ctx.lua

@@ -249,9 +249,8 @@ do
 
         rate_limiting_info         = true,
 
-        var_x_forwarded_proto      = true,
-        var_x_forwarded_port       = true,
-        var_x_forwarded_host       = true,
+        original_x_forwarded_for   = true,
+
     }
 
     -- sort in alphabetical

apisix/init.lua

@@ -686,88 +686,73 @@ function _M.handle_upstream(api_ctx, route, enable_websocket)
 end
 
 
-local function handle_x_forwarded_headers(api_ctx)
-    local addr_is_trusted = trusted_addresses_util.is_trusted(api_ctx.var.realip_remote_addr)
-
-    -- Only untrusted values need to be overwritten or cleared.
-    if not addr_is_trusted then
-        -- store the original x-forwarded-* headers
-        -- to allow future use by other plugins or processes
-        api_ctx.var.original_x_forwarded_proto = api_ctx.var.http_x_forwarded_proto
-        api_ctx.var.original_x_forwarded_host = api_ctx.var.http_x_forwarded_host
-        api_ctx.var.original_x_forwarded_port = api_ctx.var.http_x_forwarded_port
-        api_ctx.var.original_x_forwarded_for = api_ctx.var.http_x_forwarded_for
-
-        -- trusted ones
-        -- ref: ngx_tpl.lua#L831-L840
-        --
-        -- these values are observed directly by APISIX and cannot be forged,
-        -- making them highly credible.
-        local proto = api_ctx.var.scheme
-        local http_host = api_ctx.var.http_host or api_ctx.var.host
-        -- parse_addr handles IPv6 literals and bracketed host:port correctly.
-        local _, port_from_host = core.utils.parse_addr(http_host)
-        local host = http_host
-        local port = port_from_host or api_ctx.var.server_port
-
-        -- override the x-forwarded-* headers to the trusted ones.
-        -- make sure that the correct values ​​are obtained
-        -- in the subsequent stages using `core.request.header`.
-        core.request.set_header(api_ctx, "X-Forwarded-Proto", proto)
-        core.request.set_header(api_ctx, "X-Forwarded-Host", host)
-        core.request.set_header(api_ctx, "X-Forwarded-Port", port)
-        -- Clear RFC 7239 Forwarded header to prevent forgery.
-        core.request.set_header(api_ctx, "Forwarded", nil)
-
-        -- X-Forwarded-For: when a trust boundary is configured but this peer is
-        -- untrusted, reset it so the upstream only sees the APISIX-observed
-        -- connection IP via `$proxy_add_x_forwarded_for`, dropping the spoofable
-        -- inbound chain. When `trusted_addresses` is unset, keep the compatible
-        -- default of preserving the inbound chain (the connection IP is appended).
-        if trusted_addresses_util.is_configured() then
-            core.request.set_header(api_ctx, "X-Forwarded-For", nil)
-            api_ctx.var.http_x_forwarded_for = nil
-        end
-
-        -- update the cached value in http_x_forwarded_* to the trusted ones.
-        -- make sure that the correct values ​​are obtained
-        -- in the subsequent stages using `var.http_x_forwarded_*`.
-        api_ctx.var.http_x_forwarded_proto = proto
-        api_ctx.var.http_x_forwarded_host = host
-        api_ctx.var.http_x_forwarded_port = port
-        api_ctx.var.http_forwarded = nil
+-- X-Forwarded-Proto/Host/Port and Forwarded are already neutralized by the time
+-- this runs: `more_set_input_headers` in apisix/cli/ngx_tpl.lua does it in the
+-- rewrite phase, in C, on every request. That is unconditional because with no
+-- trust boundary configured -- the default -- it is what every request needs, and
+-- keeping it in the config keeps Lua off that path entirely.
+--
+-- What is left needs a trust decision, so it stays here, behind a check that is a
+-- constant for the worker's lifetime: with no `trusted_addresses` this returns on
+-- its first line and nothing else runs.
+--
+-- `set` captures an absent header as the empty string, so "" means the peer sent
+-- nothing and the value the config injected stays. That is a deliberate change
+-- for a trusted peer: the Lua-only implementation skipped the whole rewrite for
+-- one, so a header it did not send stayed absent and the upstream fell through to
+-- `$host` / `$server_port`. A trusted peer now gets the same observed values an
+-- untrusted one does -- the Host with its port and case, rather than the
+-- lower-cased portless `$host` -- which is the value the untrusted path has always
+-- produced. `ctx.var.http_x_forwarded_*` is updated alongside, so a plugin reading
+-- it in a later phase sees the restored value rather than the injected one.
+local function restore_if_sent(api_ctx, header_name, var_name, orig)
+    if not orig or orig == "" then
+        return
     end
+
+    core.request.set_header(api_ctx, header_name, orig)
+    api_ctx.var[var_name] = orig
 end
 
 
--- in ngx_tpl.lua#L831-L840,
--- there is such code: `proxy_set_header X-Forwarded-XXX $var_x_forwarded_xxx;`
--- that is, set the `X-Forwarded-XXX` header through `var_x_forwarded_xxx`.
---
--- therefore, it is necessary to set the trusted `http_x_forwarded_xxx` to `var_x_forwarded_xxx`.
--- So that the `X-Forwarded-XXX` header is updated to a trusted value.
---
--- currently, only following headers are updated through these variables:
--- - X-Forwarded-Proto
--- - X-Forwarded-Port
--- - X-Forwarded-Host
---
--- the `X-Forwarded-For` header is not updated through these variables.
--- because it is set by the `proxy_add_x_forwarded_for` directive.
-local function set_upstream_x_forwarded_headers(api_ctx)
-    local proto = api_ctx.var.http_x_forwarded_proto
-    if proto then
-        api_ctx.var.var_x_forwarded_proto = proto
+local function handle_trusted_x_forwarded_headers(api_ctx)
+    -- The other four originals are copied by the configuration; this one cannot be,
+    -- because naming `$http_x_forwarded_for` there would pin it in `r->variables[]`
+    -- and the clear below could not dislodge it. Copy it here instead, on every
+    -- path: the header is only destroyed further down, but a plugin reading
+    -- `ctx.var.original_x_forwarded_for` should not have to know that.
+    local inbound_xff = api_ctx.var.http_x_forwarded_for
+    if inbound_xff then
+        api_ctx.var.original_x_forwarded_for = inbound_xff
     end
 
-    local port = api_ctx.var.http_x_forwarded_port
-    if port then
-        api_ctx.var.var_x_forwarded_port = port
+    if not trusted_addresses_util.is_configured() then
+        return
     end
 
-    local host = api_ctx.var.http_x_forwarded_host
-    if host then
-        api_ctx.var.var_x_forwarded_host = host
+    if trusted_addresses_util.is_trusted(api_ctx.var.realip_remote_addr) then
+        -- a trusted peer's own values go back, from the copies the config took
+        -- before overwriting them
+        restore_if_sent(api_ctx, "X-Forwarded-Proto", "http_x_forwarded_proto",
+                        api_ctx.var.original_x_forwarded_proto)
+        restore_if_sent(api_ctx, "X-Forwarded-Host", "http_x_forwarded_host",
+                        api_ctx.var.original_x_forwarded_host)
+        restore_if_sent(api_ctx, "X-Forwarded-Port", "http_x_forwarded_port",
+                        api_ctx.var.original_x_forwarded_port)
+        restore_if_sent(api_ctx, "Forwarded", "http_forwarded",
+                        api_ctx.var.original_forwarded)
+
+        return
+    end
+
+    -- An untrusted peer, with a trust boundary to measure it against: drop the
+    -- inbound X-Forwarded-For so the upstream only sees the connection IP via
+    -- `$proxy_add_x_forwarded_for`. Without a boundary the chain is preserved,
+    -- which is the compatible default and is why this lives behind the check
+    -- above rather than in the config.
+    if inbound_xff then
+        core.request.set_header(api_ctx, "X-Forwarded-For", nil)
+        api_ctx.var.http_x_forwarded_for = nil
     end
 end
 
@@ -828,7 +813,7 @@ function _M.http_access_phase()
     -- var.request is read-only; copy to a writable variable so data-mask can redact query params
     api_ctx.var.request_line = api_ctx.var.request
 
-    handle_x_forwarded_headers(api_ctx)
+    handle_trusted_x_forwarded_headers(api_ctx)
 
     -- When match_uri_encoded_slash is on, match the route against a uri that
     -- keeps the encoded slash (%2F) so it is treated as part of a path
@@ -970,10 +955,6 @@ function _M.http_access_phase()
     end
     span:finish(ngx_ctx)
 
-    -- set before handle_upstream: grpc/dubbo/disable_proxy_buffering exit via
-    -- ngx.exec() and never return, so the trusted values must be applied first.
-    set_upstream_x_forwarded_headers(api_ctx)
-
     _M.handle_upstream(api_ctx, route, enable_websocket)
 end
 

conf/config.yaml.example

@@ -168,6 +168,23 @@ apisix:
                                   # address, it is reset so the upstream only sees the APISIX-observed
                                   # connection IP; when `trusted_addresses` is not configured, it is
                                   # preserved and the connection IP is appended (compatible default).
+                                  # The override happens in the NGINX configuration, before any Lua runs.
+                                  # Lua always sees the overridden values -- `core.request.header` and
+                                  # `ctx.var.http_x_forwarded_*` alike. At config level it differs:
+                                  # naming `$http_x_forwarded_proto`, `$http_x_forwarded_host`,
+                                  # `$http_x_forwarded_port` or `$http_forwarded` in an access log
+                                  # format, an `if`, or a `map` reads the value cached when the
+                                  # override was applied, which is what the client sent. Use `$scheme`,
+                                  # `$var_x_forwarded_host` and `$var_x_forwarded_port` for the
+                                  # overridden ones. `$http_x_forwarded_for` is not affected.
+                                  # What the client sent is kept in `$original_x_forwarded_proto`,
+                                  # `$original_x_forwarded_host`, `$original_x_forwarded_port`,
+                                  # `$original_x_forwarded_for` and `$original_forwarded` -- readable
+                                  # from a log format, or from Lua as `ctx.var.original_x_forwarded_*`,
+                                  # the same names the values were previously kept under, so a plugin
+                                  # reading them is unaffected. This matters most for X-Forwarded-For,
+                                  # which is cleared rather than overwritten when a trust boundary is
+                                  # configured and the peer is outside it.
   # fine tune the parameters of LRU cache for some features like secret
   lru:
     secret:

t/APISIX.pm

@@ -238,9 +238,6 @@ my $disable_proxy_buffering_location = <<_EOC_;
             proxy_pass_header  Date;
 
             proxy_set_header   X-Forwarded-For      \$proxy_add_x_forwarded_for;
-            proxy_set_header   X-Forwarded-Proto    \$var_x_forwarded_proto;
-            proxy_set_header   X-Forwarded-Host     \$var_x_forwarded_host;
-            proxy_set_header   X-Forwarded-Port     \$var_x_forwarded_port;
 
             proxy_pass         \$upstream_scheme://apisix_backend\$upstream_uri;
             mirror             /proxy_mirror;
@@ -626,6 +623,16 @@ _EOC_
     $http_config .= <<_EOC_;
     $lua_deps_path
 
+    # mirrors apisix/cli/ngx_tpl.lua
+    map \$http_host \$var_x_forwarded_port {
+        default          \$server_port;
+        "~:(?<p>\\\\d+)\$" \$p;
+    }
+    map \$http_host \$var_x_forwarded_host {
+        default \$http_host;
+        ""      \$host;
+    }
+
     lua_shared_dict plugin-limit-req 10m;
     lua_shared_dict plugin-limit-count 10m;
     lua_shared_dict plugin-limit-count-lock 10m;
@@ -953,16 +960,19 @@ _EOC_
             proxy_set_header   X-Real-IP         \$remote_addr;
             proxy_pass_header  Date;
 
-            ### the following x-forwarded-* headers is to send to upstream server
+            set \$original_x_forwarded_proto \$http_x_forwarded_proto;
+            set \$original_x_forwarded_host   \$http_x_forwarded_host;
+            set \$original_x_forwarded_port   \$http_x_forwarded_port;
+            set \$original_x_forwarded_for    '';
+            set \$original_forwarded          \$http_forwarded;
+            more_set_input_headers "X-Forwarded-Proto: \$scheme";
+            more_set_input_headers "X-Forwarded-Host: \$var_x_forwarded_host";
+            more_set_input_headers "X-Forwarded-Port: \$var_x_forwarded_port";
+            more_set_input_headers "Forwarded: ";
 
-            set \$var_x_forwarded_proto      \$scheme;
-            set \$var_x_forwarded_host       \$host;
-            set \$var_x_forwarded_port       \$server_port;
+            ### the following x-forwarded-* headers is to send to upstream server
 
             proxy_set_header   X-Forwarded-For      \$proxy_add_x_forwarded_for;
-            proxy_set_header   X-Forwarded-Proto    \$var_x_forwarded_proto;
-            proxy_set_header   X-Forwarded-Host     \$var_x_forwarded_host;
-            proxy_set_header   X-Forwarded-Port     \$var_x_forwarded_port;
 
             proxy_pass         \$upstream_scheme://apisix_backend\$upstream_uri;
             mirror             /proxy_mirror;