AlinsRan opened a new pull request, #13783:
URL: https://github.com/apache/apisix/pull/13783
### Description
`is_trusted()` returns `false` whenever `apisix.trusted_addresses` is unset,
and unset is the default — so on a default install **every** request runs the
"untrusted peer" branch that #12551 added to `handle_x_forwarded_headers`: four
`ngx.req.set_header()` calls plus a string parse. Configuring a trust boundary
was the only way out, i.e. you had to opt *into* the feature to stop paying for
it.
A second cost hides behind that branch. `set_upstream_x_forwarded_headers`
reads three `http_x_forwarded_*` variables through `api_ctx.var`, and
`core/ctx.lua`'s `__index` runs a `key:lower()` **and an `ngx.re.gsub()`** for
every `http_*` key, then does not cache a nil result. A request carrying no
`X-Forwarded-*` header — the common case — pays three PCRE calls per request,
on the trusted path too.
Neither is necessary. The security requirement is to neutralize a value *the
client supplied*; when the client supplied none, there is nothing to neutralize.
- `handle_x_forwarded_headers` returns early when the request carries none
of `X-Forwarded-Proto/Host/Port/For` and `Forwarded`. It still writes
`var_x_forwarded_host` / `var_x_forwarded_port` in the two cases where the `set
$var_x_forwarded_* $host/$server_port` defaults in `ngx_tpl.lua` would lose
information: an explicit port in the Host header (the port the client actually
connected to, and the only correct `X-Forwarded-Port` on a port-mapped
deployment), and a Host differing from the lower-cased `$host`.
- The `$realip_remote_addr` lookup and the IP match are skipped when no
trust boundary is configured; the header reads sit below the trusted
early-return; `Forwarded` and `X-Forwarded-For` are only cleared when actually
present.
- Both functions read request headers through `ngx.var`, skipping the
`api_ctx.var` wrapper's per-key regex.
**Benchmark** — single worker, wrk2 at saturation, upstream on localhost,
`master` and this branch measured back to back, two rounds each:
| scenario | master | this PR |
|---|---|---|
| default, client sends no `X-Forwarded-*` | 79,370 / 77,531 | **89,942 /
90,498** |
≈ **+15%** on the default configuration.
### Behaviour
**Trust semantics are unchanged.** A forged `X-Forwarded-*` or `Forwarded`
from an untrusted peer is still overridden or cleared exactly as before
(`t/core/trusted-addresses.t` TESTs 1, 8, 9 unchanged, 13/14 new;
`t/plugin/proxy-mirror4.t` TEST 2). An empty-valued header
(`X-Forwarded-Proto:` with no value) reads back as `""`, truthy in Lua, so it
takes the slow path — the fast path cannot be entered while carrying a
forgeable value.
Values reaching the upstream through `location /` and
`@disable_proxy_buffering` are identical to today in all trust states; those
read `$var_x_forwarded_*` via `proxy_set_header`.
Three deviations, all deliberate:
1. **gRPC, Dubbo and mirrored backends no longer receive injected
`X-Forwarded-*`.** Those exits read `r->headers_in` rather than
`$var_x_forwarded_*` — `ngx_http_grpc_module` always passes request headers and
`@grpc_pass` has no `grpc_set_header X-Forwarded-*`, `@dubbo_pass` sets
`dubbo_pass_all_headers on`, and `ngx_http_subrequest` copies the header list
for the mirror. A request carrying **no** `X-Forwarded-*` reached those
backends with all three injected after #12551, and now reaches them with none —
which is what they saw *before* #12551 (`git show <#12551>^` has no
`core.request.set_header` for these headers, only the `var_x_forwarded_*`
copies). Closing the gap would mean adding `grpc_set_header` /
`proxy_set_header` to those locations, which would *also* start sending these
headers to gRPC and mirrored backends for trusted peers that send none — a
separate behaviour change, so it belongs in its own PR.
`t/plugin/proxy-mirror4.t` TEST 1 pins the current behaviour; i
t fails on `master`.
2. **A plugin-set `X-Forwarded-*` now reaches the upstream in the untrusted
states.** `t/plugin/proxy-rewrite2.t` TEST 10 **fails on `master`** (`got:
'1984', expected: '10080'`) and passes here. `core.request.set_header`
invalidates `ctx.var` under `http_x-forwarded-port` (hyphens,
`core/request.lua`) while `ctx.lua`'s `__index` caches `http_x_forwarded_port`
(underscores), so the value `handle_x_forwarded_headers` had cached won over
the plugin's and the upstream received the gateway's own value. Reading
`ngx.var` bypasses the stale cache. This is an improvement — it makes the
untrusted state behave like the trusted state, which TEST 7 shows already
honoured the plugin — and it is not a trust weakening, since the value comes
from plugin configuration rather than client input. The underlying key mismatch
in `core/request.lua` is untouched here and deserves its own fix; it is also
the mechanism behind reports like #13753.
3. **Request headers are no longer synthesised when the client sends none.**
`core.request.header(ctx, "X-Forwarded-Proto")` and
`ctx.var.http_x_forwarded_proto` return `nil` rather than a value APISIX made
up. That was the behaviour before #12551, and it is what a trusted peer sending
no `X-Forwarded-*` gets today. Worth knowing:
- `redirect.lua` is the only plugin that reads one of these by name, and
its `or core.request.get_scheme(ctx)` fallback yields the same value that was
being injected, so it does not change.
- Plugins forwarding the *whole* header table now send a different set
(`opa/helper.lua`, `authz-casbin.lua`, `batch-requests.lua`,
`serverless/generic-upstream.lua`, `soap.lua`). An OPA policy keyed on
`x-forwarded-proto` would see it absent.
- A route matching on `vars: [["http_x_forwarded_proto", "==", "http"]]`
stops matching on a default install.
Also: `set_upstream_x_forwarded_headers` now reads `ngx.var`, so a plugin
that assigned `ctx.var.http_x_forwarded_proto` directly — cache only, without
writing the real header — would no longer be propagated. No in-tree plugin does
this.
### Which issue(s) this PR fixes
Follow-up to #12551. Deviation 2 above is the mechanism behind #13753,
though this PR only bypasses it rather than fixing the underlying
`core/request.lua` cache-key mismatch.
### 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 (if not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
--
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]
