getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
38.52k stars 4.12k forks source link

Sentry ignores remote IP address with nginx proxy (X-Forwarded-For header) #1254

Closed renomateo closed 10 years ago

renomateo commented 10 years ago

The proxy's IP is showing up on the event details page instead of the user's. I've configured this correctly for my other application running behind the same proxy and I've verified that the header is making its way to sentry using a simple node.js express app that spits out everything about the request.

I can't find this header string anywhere in the code. Looks like something like this will be necessary: http://stackoverflow.com/a/5976065

mattrobenolt commented 10 years ago

@zephyrweb This is not a thing that I'd be comfortable adding into Sentry itself. Determining the real IP address is more complicated than just choosing the left-most IP address from the X-Forwarded-For header.

If you're running your own Sentry, you should use a module that fixes the header before it even gets to the python backend, such as the nginx realip module: http://nginx.org/en/docs/http/ngx_http_realip_module.html

renomateo commented 10 years ago

@mattrobenolt How is it more complicated? Sentry displays the client's IP address. It also supports an nginx proxy (https://sentry.readthedocs.org/en/latest/quickstart/nginx.html). Displaying the IP of the proxy, although correct and not error prone, isn't very useful. It should be an option or the implicit support in the configuration documentation should be removed.

I also don't see how that module would help. Sentry will report REMOTE_ADDR as the OS reports it, right, not based on a header. If it were as simple as a header, then why not just use X-Forwarded-For?

mattrobenolt commented 10 years ago

How is it more complicated?

It's not complicated, but it's naive and potentially insecure. How do I know which IP address to trust? What happens if I forge a request that looks like:

GET / HTTP/1.1
X-Forwarded-For: 127.0.0.1

And send that along? Our app is going to check that header and get back 127.0.0.1. What if your proxy is setting the X-Forwarded-For properly? It'll end up looking like X-Forwarded-For: 127.0.0.1, <client ip>. How do we know which value to choose? That's the security issue.

There are many different ways that proxies deal with this, so it's impossible to generalize into a simple rule, which is why Django actually stopped even providing this in core.

So, how does the realip module help this? With the set_real_ip_from and real_ip_recursive directives. You declare what subnets and ip addresses are trusted, and it'll select the ip address before that one in the chain.

To put an example to that, say your internal network is 10/8. And you tell nginx that you trust 10.0.0.0/8 subnet entirely. A request comes in with X-Forwarded-For: 127.0.0.1. It hits a proxy on our end, and it appends the ip address of the client. Let's say it's 1.1.1.1. So now our header looks like: X-Forwarded-For: 127.0.0.1, 1.1.1.1. Maybe it hits another internal proxy. It gets the IP address of the first proxy appended, so it's now: X-Forwarded-For: 127.0.0.1, 1.1.1.1, 10.2.5.200. Now the request hits nginx. It sees these 3 ip addresses. Which one does it choose? If you've told it to trust 10.0.0.0/8, it'll give you back the correct IP 1.1.1.1 which is the one we're looking for and it discards 127.0.0.1.

Displaying the IP of the proxy, although correct and not error prone, isn't very useful. It should be an option or the implicit support in the configuration documentation should be removed.

Correct, it's not useful at all. Which is why you should use the realip module to fix it.

Sentry will report REMOTE_ADDR as the OS reports it, right, not based on a header.

No, the REMOTE_ADDR comes from the IP that nginx passes along. It doesn't make a syscall or anything to retrieve the real value. So with the realip module, nginx will pass along the correct IP address and REMOTE_ADDR will be what you're expecting. The client's real ip address.

Now, we could replicate this feature in Sentry and let you specify a list of trusted subnets and we could have a middleware that handles it, blah blah blah. But why? There is already a module that does this safely and faster than python could do it.

I hope this sheds some light onto the complications. :)

renomateo commented 10 years ago

@mattrobenolt Thanks for the clarification.

What if your proxy is setting the X-Forwarded-For properly?

Is this just an issue where there are multiple proxy's involved? I only have one, and it's not on the same server as Sentry, so I only ever see a single IP address in that header.

There is already a module that does this safely and faster than python could do it.

Yes, but the tradeoff is that it is difficult to install and configure. An option to use the left-most IP in X-Forwarded-For for setups with only one proxy in front of Sentry would be really convenient.

And why doesn't setting the remote_addr header (or X-Real-IP for that matter) in nginx work? Does set_real_ip_from work at a lower-level than HTTP?

mattrobenolt commented 10 years ago

Is this just an issue where there are multiple proxy's involved? I only have one, and it's not on the same server as Sentry, so I only ever see a single IP address in that header.

Can you guarantee that? ;) What if there are proxies between the client and your server? I'm just throwing out possibilities and other ways that this header can be mutated.

Yes, but the tradeoff is that it is difficult to install and configure. An option to use the left-most IP in X-Forwarded-For for setups with only one proxy in front of Sentry would be really convenient.

I'll just spoof my left most IP address and give you anything I want. So no, not good. It's also equally difficult to configure if it were in Python. If we provide the directions with our nginx config, it'd just be copy/paste like the rest.

And why doesn't setting the remote_addr header (or X-Real-IP for that matter) in nginx work? Does set_real_ip_from work at a lower-level than HTTP?

REMOTE_ADDR isn't a header. It's coming from the wsgi environment. X-Real-IP is literally the same thing as X-Forwarded-For just with a different name. So yeah, good thing you brought that up, because that's another header to look for if we were to support this. Is it X-Forwarded-For or X-Real-IP? :)

And yes, what nginx is doing is not at the HTTP level. It doesn't set a header.

renomateo commented 10 years ago

OK, I've installed and configured ngx_http_realip_module on my proxy server and it still sees (reports) the IP address of the proxy's interface. I've debugged what Sentry is supposed to see using a simple node.js server, netcat and tcpdump, and none of them report the remote_addr (equivalent) as the "real" ip. Here's my configuration (nginx's IP is 10.10.10.100):

set_real_ip_from 10.10.10.0/24;
set_real_ip_from 10.10.10.100;
set_real_ip_from 127.0.0.1;
real_ip_header X-Forwarded-For;
real_ip_recursive on;

location / {
    proxy_set_header Host $host;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_pass http://10.10.10.101:9000/;
    break;
}

Everything I've read about the module is that it's just a filtering mechanism for the "real_ip_header" field and nothing more. If you check out nginx's website http://wiki.nginx.org/Modules you'll notice "For using nginx as backend" right next to that module, which makes sense, because it's designed to work as a client of another proxy. It's up to the backend server to use the appropriate header, otherwise, why would the "real_ip_header" field exist in the first place?

renomateo commented 10 years ago

Restoring Django's SetRemoteAddrFromForwardedFor feature (http://www.micahcarrick.com/django-ip-address-behind-nginx-proxy.html) in lib/python2.7/site-packages/sentry-6.4.4-py2.7.egg/sentry/middleware.py seems to do the trick. But having ngx_http_realip_module installed in front of it is definitely a good idea.

dcramer commented 10 years ago

We should probably just load our own version of that middleware to make it easy to configure.  Also sentry never runs outside of a proxy so it's a same default. 

On Fri, Aug 22, 2014 at 2:02 PM -0700, "zephyrweb" notifications@github.com wrote:

Restoring Django's SetRemoteAddrFromForwardedFor feature (http://www.micahcarrick.com/django-ip-address-behind-nginx-proxy.html) in lib/python2.7/site-packages/sentry-6.4.4-py2.7.egg/sentry/middleware.py seems to do the trick. But having ngx_http_realip_module installed in front of it is definitely a good idea.

— Reply to this email directly or view it on GitHub.

mattrobenolt commented 10 years ago

why would the "real_ip_header" field exist in the first place

It's used to read the header for which the chain of IP addresses are already listed. So you tell it to look at either X-Forwarded-For or X-Real-IP, and nginx takes that value and uses it as the client IP address when proxying it. Which is what ends up in REMOTE_ADDR inside wsgi.

With that said, we could write a middleware that recreates exactly what this nginx modules does to give us the security that it'd require and avoid doing this through nginx if people don't want to.

I'm indifferent on it, but I'll write the middleware to support it.

naw commented 9 years ago

nginx takes that value and uses it as the client IP address when proxying it. Which is what ends up in REMOTE_ADDR inside wsgi.

I don't think that's true. When nginx proxies to an upstream, the upstream will assign the meta-variable REMOTE_ADDR to be the IP of the client (where nginx resides).

RealIP affects what nginx "thinks" of as the client IP, and would be reflected in nginx's own logs. However, this won't affect what the upstream sees as REMOTE_ADDR at all --- nginx has no control over that.

WSGI spec defers to the CGI spec for the REMOTE_ADDR meta-variable, and the CGI spec says REMOTE_ADDR is the IP of the immediate client (i.e. the proxy, if it exists), which may or may not be the IP of the original client.

As far as I can tell, your whole line of thinking in this thread regarding using nginx's RealIp module is completely flawed.

I'd welcome your correction if I've missed something.

marclennox commented 9 years ago

So has this been resolved? I'm running our application behind a proxy, and the event details in Sentry show the ip address as that of the proxy server, not of the client that generated the error.

ferrix commented 8 years ago

FYI, I had this problem and solved it for a Django app by writing https://github.com/codetry/xff

As you can see, there are some considerations raven cannot make on its own.

nateberkopec commented 7 years ago

IMO it makes sense to duplicate nginx's behavior here. Rails includes a middleware that does this by default.

The behavior is "the client's IP address is the oldest parent in the forwarding chain AFTER any trusted proxies", which is secure and a more accurate representation than just taking REMOTE_ADDR.

Given an array of IP addresses, Sentry should consider the IP address real_ip as secure:

[not_secure_ip, not_secure_ip2, real_ip, trusted_proxy_1, trusted_proxy_2]

What @marclennox mentioned is a separate issue, because spoofing an Event IP doesn't have any security implications, just data accuracy concerns. The original issue here was about running a Sentry server behind a proxy, and Sentry definitely uses REMOTE_ADDR for some auth. So we should prefer the secure IP there.

But for Event IPs, I think it makes sense to pick the leftmost IP address from X-Forwarded-For. It may be spoofed, but taking the "secure" approach means that if a user is using a (transparent) HTTP proxy, we'll return the IP of the proxy rather than the actual end-user's IP. I'm working on an implementation for raven-ruby.

fletom commented 7 years ago

We're running Cloudfront -> Heroku loadbalancers -> our app, and we get nothing but AWS internal IP addresses in Sentry which is a real pain. This issue needs to be reopened I think.

It's absolutely true that you can't trust that the leftmost entry in X-Forwarded-For is the user's real IP. However, if you think about it, that doesn't actually matter at all for this use case.

What is a hypothetical attacker going to do? Generate a generate a bunch of errors and make you think they came from someone else's IP? You can also fake the User-Agent header and pretend the error was generated in Safari instead of Chrome, but that doesn't stop Sentry from displaying the user's browser.

Sentry should respect the X-Forwarded-For header and display in the event details the IP address that the error really (almost certainly) came from.

mattrobenolt commented 7 years ago

We've been reading X-Forwarded-For for a long time now by default and is controlled via SENTRY_USE_X_FORWARDED_FOR.

For what it's worth, spoofing X-Forwarded-For would potentially allow to elevated administration privileges if they were configured for INTERNAL_IPS.

zeroX-tj commented 7 years ago

SENTRY_USE_X_FORWARDED_FOR didn't work for me with the latest version. After changing my nginx from proxy_pass to uwsgi_pass(and sentry config) it picked up the real ip without issues.

https://docs.sentry.io/server/performance/#performance-web-server

tino commented 6 years ago

Hmm, I'm confused. I'm on 8.22.0, I didn't set SENTRY_USE_X_FORWARDED_FOR anywhere and looking at the source it defaults to True, yet I'm seeing:

screenshot_2018-03-11_14_58_25_v4uvjj
bartvollebregt commented 6 years ago

What I've tried:

But I'm still having the exact same result as @tino

Anyone able to get the X-Forwarded-For header in the REMOTE_ADDR field?