Skip to content
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

IP Restriction and Rate-Limiting should make X-Forwarded-For parsing configurable #2020

Closed
subnetmarco opened this issue Jan 26, 2017 · 3 comments

Comments

@subnetmarco
Copy link
Member

subnetmarco commented Jan 26, 2017

We are using the ngx_http_realip_module to properly handle the real client IP address and parse it from the X-Forwarded-For header if it exists.

This behavior works fine in most use-cases, but there are some plugins which should make it configurable for security reasons:

IP Restriction, Rate-Limiting and Response Rate-Limiting plugins should support a config.disable_forwarded_for if the real IP address should be used always (ignoring the X-Forwarded-For header).

@bungle
Copy link
Member

bungle commented Mar 23, 2017

@thefosk, if #2236 is about to be merged, then this changes quite a bit. Before merging we are by default running this kind of config (trust all IPv4):

real_ip_header X-Forwarded-For;
real_ip_recursive on;
set_real_ip_from 0.0.0.0/0;

After this patch all of these are configurable, but the default config will be (trust none — aka unconfigured Nginx default):

real_ip_header X-Real-IP;
real_ip_recursive off;

On production I think it is best to use currently something like this:

real_ip_header X-Forwarded-For;
real_ip_recursive on;
real_ip_from [trusted proxy addresses or cidr blocks]

Now we do have $remote_addr that these plugins are using (the real_ip module changes that according to its configuration values), and we do have others, most notably realip_remote_addr.

I think those plugins should be configurable to use other Nginx variables than the default remote_addr. One thing worth to note is that e.g. ip-restriction plugin currently only supports IPv4. lua-resty-mediador is a new component that supports the both IPv4 and IPv6.

@p0pr0ck5
Copy link
Contributor

@bungle is this done now that your X-F-F work is merged in?

@bungle
Copy link
Member

bungle commented Apr 21, 2017

@p0pr0ck5, as far as it can be considered fixed by that? No sure if there is still need for overriding this per API basis, though. But that easily leads to a question that whether every plugin that wants to access client IP should have some configuration variable to do so. I am against adding something like config.disable_forwarded_for but maybe a way to select between $remote_addr and $realip_remote_addr. It is important to know that realip module (possibly, debending on configs and headers) overrides $remote_addr, and when it does so, it stores the previous value in $realip_remote_addr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants