-
Couldn't load subscription status.
- 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_addressesX-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_addressesconfiguration 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-Protoin 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 <qianyong@api7.ai>
Signed-off-by: Nic <qianyong@api7.ai>
…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
redirectplugin, even ifhttp_to_httpsis 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_addressesto take effect.apisix.trusted_addressessupports matching both IPs and CIDRs.Solve part of #12500
Which issue(s) this PR fixes:
Fixes #
Checklist