lxbme commented on issue #13753:
URL: https://github.com/apache/apisix/issues/13753#issuecomment-5138575360
Reproduced on 3.17.0 and on current `master`. There are two distinct defects
here, and the more serious one is a regression, not a design limitation.
`X-Forwarded-Host` (and `X-Forwarded-Port`, `X-Forwarded-Proto`) can no
longer be rewritten by any plugin whenever the client is *not* in
`apisix.trusted_addresses` — which is the default, since `trusted_addresses` is
unset out of the box. This broke in 3.14.0 with #12551. Separately, there has
never been a way to *delete* `X-Forwarded-Host` on the way upstream.
## Reproduction
Standalone APISIX 3.17.0 plus an nginx echo upstream. No etcd, no ingress
controller needed.
`echo.conf`:
```nginx
server {
listen 80;
location / {
default_type text/plain;
return 200 "path=$uri host=[$http_host]
xfh=[$http_x_forwarded_host]\n";
}
}
```
`config.yaml`:
```yaml
apisix:
node_listen: 9080
enable_admin: false
deployment:
role: data_plane
role_data_plane:
config_provider: yaml
```
`apisix.yaml`:
```yaml
routes:
- id: baseline
uri: /baseline
upstream_id: echo
- id: remove
uri: /remove
upstream_id: echo
plugins:
proxy-rewrite:
headers:
remove:
- "X-Forwarded-Host"
- id: set-empty
uri: /set-empty
upstream_id: echo
plugins:
proxy-rewrite:
headers:
set:
X-Forwarded-Host: ""
- id: set-value
uri: /set-value
upstream_id: echo
plugins:
proxy-rewrite:
headers:
set:
X-Forwarded-Host: "my-upstream.example.com"
- id: before-proxy
uri: /before-proxy
upstream_id: echo
plugins:
serverless-pre-function:
phase: before_proxy
functions:
- "return function(conf, ctx) ngx.var.var_x_forwarded_host = ''
end"
upstreams:
- id: echo
type: roundrobin
nodes:
"echo:80": 1
#END
```
`docker-compose.yml`:
```yaml
services:
apisix:
image: apache/apisix:3.17.0
volumes:
- ./config.yaml:/usr/local/apisix/conf/config.yaml:ro
- ./apisix.yaml:/usr/local/apisix/conf/apisix.yaml:ro
ports:
- "9080:9080"
depends_on: [echo]
echo:
image: nginx:alpine
volumes:
- ./echo.conf:/etc/nginx/conf.d/default.conf:ro
```
Then:
```console
$ for p in baseline remove set-empty set-value before-proxy; do
printf '%-14s ' "$p"; curl -s -H 'Host: apisix.example.com'
http://127.0.0.1:9080/$p
done
baseline path=/baseline host=[apisix.example.com]
xfh=[apisix.example.com]
remove path=/remove host=[apisix.example.com]
xfh=[apisix.example.com]
set-empty path=/set-empty host=[apisix.example.com]
xfh=[apisix.example.com]
set-value path=/set-value host=[apisix.example.com]
xfh=[apisix.example.com]
before-proxy path=/before-proxy host=[apisix.example.com] xfh=[]
```
| Route | Expected upstream `X-Forwarded-Host` | Actual | |
|---|---|---|---|
| `remove` | absent | `apisix.example.com` | ❌ |
| `set-empty` | absent | `apisix.example.com` | ❌ |
| `set-value` | `my-upstream.example.com` | `apisix.example.com` | ❌
regression |
| `before-proxy` | absent | absent | ✅ works |
Adding the client network to `apisix.trusted_addresses` makes `set-value`
start working again, while `remove` and `set-empty` stay broken. That split is
the clearest signal that these are two separate defects.
## Defect 1 — plugin rewrites of `X-Forwarded-*` are silently discarded for
untrusted clients (regression, 3.14.0+)
`handle_x_forwarded_headers()` (`apisix/init.lua:689`) runs before route
matching, and for an untrusted peer it writes the APISIX-observed value into
the `ctx.var` cache at `apisix/init.lua:736`:
```lua
api_ctx.var.http_x_forwarded_host = host
```
A plugin that later rewrites the header goes through
`core.request.set_header()`, which tries to invalidate that cache entry at
`apisix/core/request.lua:155-156`:
```lua
-- when the header is updated, clear cache of ctx.var
ctx.var["http_" .. str_lower(header_name)] = nil
```
For `X-Forwarded-Host` this computes the key `http_x-forwarded-host`
(hyphens), but the `ctx.var` cache is keyed `http_x_forwarded_host`
(underscores). **The invalidation never matches**, so the stale value survives,
and `set_upstream_x_forwarded_headers()` (`apisix/init.lua:768`) copies that
stale value into `$var_x_forwarded_host`, which is what `proxy_set_header
X-Forwarded-Host $var_x_forwarded_host` (`apisix/cli/ngx_tpl.lua:883`) actually
sends.
Instrumenting a `before_proxy` plugin on a route that also has
`proxy-rewrite` setting the header shows the two views diverging:
```
DIAG ctxvar_http=apisix.example.com ngxvar_http=my-upstream.example.com
var_x_forwarded_host=apisix.example.com
```
`ngx.var` has the plugin's value; the `ctx.var` cache still has the
pre-plugin one; the cached one wins.
The mis-keyed invalidation is not specific to `X-Forwarded-*` — any
hyphenated header read back through `ctx.var.http_*` after
`core.request.set_header()` will return a stale value. It only became
load-bearing here because #12551 started pre-populating that cache entry.
### This is a regression, and APISIX's own tests say so
`proxy-rewrite`'s ability to rewrite these headers is intended, tested
behavior: PR #8200 ("fix: can not modify x-forwarded-host in proxy rewrite",
merged 2022-11-03) added the propagation specifically to support it, and
`t/plugin/proxy-rewrite3.t` TEST 12/13 locks it in.
Those tests still pass only because the test harness trusts the client.
`t/APISIX.pm:106-107` sets:
```yaml
trusted_addresses:
- "127.0.0.1"
```
so every test request takes the trusted branch and
`handle_x_forwarded_headers()` never populates the cache. Delete those two
lines from `t/APISIX.pm` and re-run the *unmodified* suite on `master`:
```console
$ prove -Itest-nginx/lib -I. t/plugin/proxy-rewrite3.t
# Failed test 't/plugin/proxy-rewrite3.t TEST 13: rewrite X-Forwarded-Host
- header X-Forwarded-Host ok'
# got: 'localhost'
# expected: 'test.com'
# Failed test 't/plugin/proxy-rewrite3.t TEST 34: test if X-Forwarded-Port
can be set before proxy - header X-Forwarded-Port ok'
# got: '1984'
# expected: '9882'
Result: FAIL
```
Two existing regression tests, written to guarantee exactly this capability,
fail the moment the harness stops trusting the client. The behavior contradicts
the project's own encoded intent, and `X-Forwarded-Port` is affected the same
way as `X-Forwarded-Host`. That is a bug, not a design decision — and it is
also why CI has not caught it since 3.14.0.
## Defect 2 — `X-Forwarded-Host` cannot be deleted at all (pre-existing)
`set_upstream_x_forwarded_headers()` only ever writes a value:
```lua
local host = api_ctx.var.http_x_forwarded_host
if host then
api_ctx.var.var_x_forwarded_host = host
end
```
When the request header is gone — `headers.remove`, or `headers.set` with
`""`, which OpenResty treats as removal — `host` is `nil`, the branch is
skipped, and `$var_x_forwarded_host` keeps the location-level default `set
$var_x_forwarded_host $host;` (`apisix/cli/ngx_tpl.lua:878`). The original host
is forwarded regardless. Fixing defect 1 alone does not fix this; there is
currently no "unset" path.
This one predates #12551 and is better described as a gap than a regression,
but it does contradict the `proxy-rewrite` docs, which describe
`headers.remove` as removing headers before the request reaches the upstream.
## Workaround that does work today
A `before_proxy` plugin setting the nginx variable directly is effective on
3.17.0 — verified above, both clearing it and setting a value:
```yaml
plugins:
serverless-pre-function:
phase: before_proxy
functions:
- "return function(conf, ctx) ngx.var.var_x_forwarded_host = '' end"
```
This runs after `set_upstream_x_forwarded_headers()` and therefore wins. If
it appeared not to work in the original report, the cause is likely elsewhere
(plugin not enabled in `plugins:`, or a different `phase`) — worth re-checking,
because it is the only per-route knob that currently works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
