-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: only trust X-Forwarded-*
headers from trusted_addresses
#12551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: only trust X-Forwarded-*
headers from trusted_addresses
#12551
Conversation
|
||
local proto = api_ctx.var.http_x_forwarded_proto | ||
if proto then | ||
api_ctx.var.var_x_forwarded_proto = proto | ||
end | ||
|
||
local x_forwarded_host = api_ctx.var.http_x_forwarded_host | ||
if x_forwarded_host then | ||
api_ctx.var.var_x_forwarded_host = x_forwarded_host | ||
end | ||
|
||
local port = api_ctx.var.http_x_forwarded_port | ||
if port then | ||
api_ctx.var.var_x_forwarded_port = port | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 277 to 282 in 09f6e36
apisix.handle_upstream = orig_handle_upstream | |
apisix.http_balancer_phase = orig_http_balancer_phase | |
else | |
-- replace the upstream and balancer module | |
apisix.handle_upstream = ai_upstream | |
apisix.http_balancer_phase = ai_http_balancer_phase |
Because the code above will replace handle_upstream
with ai_upstream
below:
Lines 115 to 117 in 09f6e36
local function ai_upstream() | |
core.log.info("enable sample upstream") | |
end |
This will cause this deleted part of the code to be skipped.
Now that we need to handle X-Forwarded-*
, we need to update these var_x_forwarded_*
, so we need to extract this part of the code as update_var_x_forwarded_headers
.
Lines 835 to 838 in 3260931
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; |
trusted_addresses
to handle X-Forwarded-*
headersX-Forwarded-*
headers from trusted_addresses
X-Forwarded-*
headers from trusted_addresses
X-Forwarded-*
headers from trusted_addresses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements security hardening by restricting APISIX to only trust X-Forwarded-*
headers from configured trusted IP addresses/CIDRs. Previously, APISIX accepted these headers from any source, creating security vulnerabilities where malicious clients could bypass features like HTTP-to-HTTPS redirection.
- Adds
trusted_addresses
configuration to specify which IPs/CIDRs can send trustedX-Forwarded-*
headers - Creates utility module to validate and match trusted addresses using IP/CIDR notation
- Modifies request processing to override untrusted
X-Forwarded-*
headers with server-generated values
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
apisix/utils/trusted-addresses.lua | New utility module for validating and matching trusted IP addresses/CIDRs |
apisix/init.lua | Core logic to handle X-Forwarded headers based on client trust status |
conf/config.yaml.example | Configuration example for trusted_addresses setting |
t/core/trusted-addresses.t | Comprehensive tests for trusted address functionality |
docs/en/latest/plugins/*.md | Documentation updates explaining trusted address requirements |
docs/zh/latest/plugins/*.md | Chinese documentation updates for trusted address requirements |
t/APISIX.pm | Default test configuration with localhost as trusted address |
t/plugin/*.t | Test configuration updates to include trusted addresses |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
t/plugin/dubbo-proxy/upstream.t
Outdated
node_listen: 1984 | ||
enable_admin: false | ||
trusted_addresses: | ||
- "127.0.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in batches, removed
my $yaml_config = $block->yaml_config // <<_EOC_; | ||
apisix: | ||
node_listen: 1984 | ||
trusted_addresses: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to pass the original tests.
t/plugin/sls-logger.t
Outdated
enable_encrypt_fields: true | ||
keyring: | ||
- edd1c9f0985e76a2 | ||
trusted_addresses: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in batches, removed
@@ -0,0 +1,219 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to test the situation where trusted-addresses contain multiple IP addresses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and need test cases in plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than chaitin-waf
, the original PR didn't find any tests covering the real_client_ip=true
scenario, and after spending time researching, I also couldn't find the actual purpose of this parameter.
conf/config.yaml.example
Outdated
# When set to true, it overrides all upstream healthcheck configurations and globally disabling healthchecks. | ||
# trusted_addresses: # When enabled, APISIX will trust the `X-Forwarded-*` Headers | ||
# - 127.0.0.1 # passed in requests from the IP/CIDR in the list. | ||
# - 0.0.0.0/0 # CAUTION: When not configured, APISIX will remove `X-Forwarded-*` headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No IP restriction is a bad example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
conf/config.yaml.example
Outdated
# or (standalone mode) the config isn't loaded yet either via file or Admin API. | ||
# disable_upstream_healthcheck: false # A global switch for healthcheck. Defaults to false. | ||
# When set to true, it overrides all upstream healthcheck configurations and globally disabling healthchecks. | ||
# trusted_addresses: # When enabled, APISIX will trust the `X-Forwarded-*` Headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# trusted_addresses: # When enabled, APISIX will trust the `X-Forwarded-*` Headers | |
# trusted_addresses: # When enabled, APISIX will trust the `X-Forwarded-*` Headers |
how to disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not configured, it is disabled by default.
This statement references
apisix/conf/config.yaml.example
Line 138 in 1e3950e
# status: # When enabled, APISIX will provide `/status` and `/status/ready` endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write it in this file and docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
default = false, | ||
description = "a global switch to disable upstream health checks", | ||
}, | ||
trusted_addresses = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref:
apisix/apisix/plugins/real-ip.lua
Lines 32 to 36 in bb71dfc
trusted_addresses = { | |
type = "array", | |
items = {anyOf = core.schema.ip_def}, | |
minItems = 1 | |
}, |
we can add more restrction to schema like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding core.schema.ip_def doesn't seem to work.
https://github.com/apache/apisix/blob/8bb6802bd2b2ccd4dabe78e90beb8f87ec046035/apisix/schema_def.lua depends on https://github.com/apache/apisix/blob/0fd582bc299a9d90893d3843cd3ec21f217cb40e/apisix/core/schema.lua
Using it will cause an error /usr/local/openresty//luajit/bin/luajit: ./apisix/core/lrucache.lua:22: module '\''resty.lrucache'\'' not found
:


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using it will cause an error /usr/local/openresty//luajit/bin/luajit: ./apisix/core/lrucache.lua:22: module '''resty.lrucache''' not found:
What is the reason for this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this error is that there is no openresty environment in the apisix cli, which makes it impossible to use the resty.lru
library.
I have considered the dependency relationship between schema_def.lua
and schema.lua
, and I think their dependency relationship is reversed. It should be that schema.lua
depends on schema_def.lua
, allowing schema_def.lua
to retain only the definition of schema, in line with the naming of this file.
Added a new commit to this PR to fix this dependency issue: 30ce9c2
t/core/trusted-addresses.t
Outdated
--- request | ||
GET /old_uri | ||
--- error_log | ||
invalid IP/CIDR '1.0.0' exists in trusted_addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to check the value of trusted_addresses when apisix starts, not only at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apisix/cli/schema.lua
Outdated
type = "array", | ||
minItems = 1, | ||
items = { | ||
type = "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"oneOf": [
{"format": "ipv4"},
{"format": "ipv6"}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few problems with doing this:
- Need CIDR support
- As discussed in fix: only trust
X-Forwarded-*
headers fromtrusted_addresses
#12551 (comment), try to use the existing schema_def. A feasible solution is to copy the corresponding schema again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I will try to copy a schema from schema_def to implement this function.
apisix/init.lua
Outdated
api_ctx.var.http_x_forwarded_port = port | ||
api_ctx.var.http_x_forwarded_for = nil | ||
|
||
-- override the x-forwarded-* headers to the trusted ones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the if not trusted
logic of this code, where the trusted ones
come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 830 to 837 in 33b9632
set $var_x_forwarded_proto $scheme; | |
set $var_x_forwarded_host $host; | |
set $var_x_forwarded_port $server_port; | |
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; |
Referenced the implementation of APISIX.
local proto = api_ctx.var.http_x_forwarded_proto | ||
if proto then | ||
api_ctx.var.var_x_forwarded_proto = proto | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not set X-Forwarded-Proto
as http_x_forwarded_proto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand.
- This function is extracted from the following code without any adjustments.
X-Forwarded-Proto
in the header ==http_x_forwarded_proto
apisix/utils/trusted-addresses.lua
Outdated
local _M = {} | ||
|
||
|
||
local function validate_trusted_addresses(trusted_addresses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to check this when apisix starts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
end | ||
|
||
|
||
local function handle_x_forwarded_headers(api_ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some comments to explain the purpose of each code segment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apisix/cli/schema.lua
Outdated
required = {"prefix", "host"} | ||
} | ||
|
||
-- copy from apisix/schema_def.lua |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not copy. we should find the root reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to extract this schema separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part has been handled by @nic-6443 . Thanks to nic for the assistance.
Signed-off-by: Nic <[email protected]>
Signed-off-by: Nic <[email protected]>
…to young/feat/trusted_addresses
Description
In the past, APISIX trusted the
X-Forwarded-*
headers from all IPs, which caused many problems. For example:When using the
redirect
plugin, even ifhttp_to_https
is enabled, as long as the user tries to requestcurl http://route -H "X-Forwarded-Proto: https"
(or uses any value other thanhttp
. Allow any value will be fixed in #12561), they can bypass the redirection logic and downgrade to http.apisix/apisix/plugins/redirect.lua
Lines 192 to 195 in 167b655
This PR implements that
X-Forwarded-*
headers are only trusted if they are sent by IPs or CIDRs configured inapisix.trusted_addresses
. Therefore, all plugins or features that useX-Forwarded-*
headers require configuration ofapisix.trusted_addresses
to take effect.apisix.trusted_addresses
supports matching both IPs and CIDRs.Solve part of #12500
Which issue(s) this PR fixes:
Fixes #
Checklist