AlinsRan opened a new pull request, #13803:
URL: https://github.com/apache/apisix/pull/13803
### Description
`handle_x_forwarded_headers` runs on every request to overwrite
`X-Forwarded-Proto/Host/Port` and clear `Forwarded`, and
`set_upstream_x_forwarded_headers` then copies the result into
`$var_x_forwarded_*` for `proxy_set_header`. Both do work the configuration can
do in C, and both run on the path that matters most: with no
`apisix.trusted_addresses` set — the default — no peer is trusted, so every
request takes the same branch.
**Config side.** `more_set_input_headers` neutralizes `r->headers_in` in the
rewrite phase, and two maps derive the observed host and port from the Host
header:
```nginx
map $http_host $apisix_observed_port {
default $server_port;
"~:(?<p>\d+)$" $p;
}
map $http_host $apisix_observed_host {
default $http_host;
"" $host;
}
```
```nginx
set $apisix_orig_xf_proto $http_x_forwarded_proto;
set $apisix_orig_xf_host $http_x_forwarded_host;
set $apisix_orig_xf_port $http_x_forwarded_port;
set $apisix_orig_forwarded $http_forwarded;
more_set_input_headers "X-Forwarded-Proto: $scheme";
more_set_input_headers "X-Forwarded-Host: $apisix_observed_host";
more_set_input_headers "X-Forwarded-Port: $apisix_observed_port";
more_set_input_headers "Forwarded: ";
```
**The upstream-facing `proxy_set_header X-Forwarded-Proto/Host/Port` are
removed**, along with `$var_x_forwarded_*`. `r->headers_in` already holds the
values the request should carry and `proxy_pass` forwards it as it stands, so
there is nothing left to copy — and nothing that can overwrite a plugin's
rewrite of those headers, which is what the Lua copier existed to preserve.
`proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for` stays: only that
variable appends the connection address.
**Lua side.** What is left needs a trust decision, so it stays in Lua behind
a check that is a constant for the worker's lifetime:
```lua
local function handle_trusted_x_forwarded_headers(api_ctx)
if not trusted_addresses_util.is_configured() then
return
end
...
end
```
With no `trusted_addresses` configured this returns on its first line. When
a boundary does exist, a trusted peer's values are restored from the
`$apisix_orig_*` copies and an untrusted peer additionally loses the inbound
`X-Forwarded-For` chain.
Two details worth calling out for review:
- The `set $apisix_orig_*` copies are rendered unconditionally rather than
behind a template guard on `trusted_addresses`. The guard is tempting — the
copies are only ever read when a boundary exists — but it makes correctness
depend on the CLI seeing the same configuration the worker will, which is not
guaranteed for a deployment whose configuration can arrive after render time.
Getting it wrong is silent and inverts the trust semantics, so the four `set`
directives are always emitted.
- Evaluating `$http_x_forwarded_*` in the rewrite phase caches the
pre-neutralization value for the rest of the request. That is harmless here
only because nothing downstream derives from those variables any more. It is
the reason the upstream-facing headers must come from `r->headers_in` rather
than from a variable, and the comment in the template says so.
### Behaviour
Unchanged. Verified by running each new test case against the previous
implementation and taking the expectation from what it produced.
One assertion moved: `t/core/trusted-addresses.t` TEST 1 no longer expects
`trusted_addresses_matcher is not initialized` in the error log, because with
no boundary configured the new code returns before consulting the matcher. The
assertion is kept, inverted, in a `--- no_error_log` block.
`ctx.var.original_x_forwarded_*` is removed. It had no consumer in-tree, but
it was an externally visible ctx variable — a custom plugin reading it will now
see `nil`. `var_x_forwarded_proto/port/host` are likewise dropped from the
writable-variable list in `core/ctx.lua`; a plugin that wants to change what
the upstream receives should use `core.request.set_header`, which now works for
these headers where before it was overwritten.
### Tests
`t/core/trusted-addresses.t` gains five cases:
| | |
|---|---|
| TEST 11 | `Host: example.com:8443`, no trust boundary → `X-Forwarded-Host:
example.com:8443`, `X-Forwarded-Port: 8443` |
| TEST 12 | HTTP/1.0 request with no `Host` header → falls back to `$host` |
| TEST 13 | trusted peer that sent no `X-Forwarded-*` → upstream still
receives the observed values |
| TEST 14 | trusted peer sending `X-Forwarded-Proto: grpc`, route rewrites
it to `https` via `proxy-rewrite` → upstream receives `https` |
| TEST 15 | untrusted peer with a boundary configured, forged headers →
observed values only, no `Forwarded`, `X-Forwarded-For` reduced to the
connection address |
Ran against a pristine `master` worktree for comparison:
| suite | master | this branch |
|---|---|---|
| `t/core/trusted-addresses.t` | ok | ok |
| `t/core/request.t` | ok | ok |
| `t/plugin/real-ip.t` | ok | ok |
| `t/plugin/redirect.t` | ok | ok |
| `t/plugin/proxy-rewrite2.t` | ok | ok |
| `t/plugin/ip-restriction.t` | ok | ok |
| `t/plugin/proxy-mirror2.t` | ok | ok |
| `t/plugin/forward-auth.t` | Failed 43-44 | Failed 43-44 |
| `t/plugin/proxy-rewrite3.t` | Failed 112 | Failed 112 |
The two remaining failures are pre-existing on `master` in this environment
and unrelated.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (please
explain if not)
--
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]
