Use the first IP in the list from X-Forwarded-For #655

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

davidwilemski commented Dec 15, 2012

fixes #647

This works in the case of only a single IP address and with a list like "99.100.2.45, 10.10.2.5, 10.10.2.4". I have also added tests to reflect this change.

This could still be improved more by using the first public IP address rather than just the first in the list. Ben, if you think that Tornado should handle that case then I can extend this more.

Owner

bdarnell commented Dec 21, 2012

We don't want to use the first header in the list - A) it may be a private IP which is generally useless (but maybe not, for intranet applications), and B) it's unverified. Tornado's xheader support is designed to mimic what you would get from the socket address if there were no proxy (here we're talking about load-balancing proxies on the server side instead of any proxies that may be used by the client), so it should return the last IP (since that was added by your own proxy and is therefore the most trustworthy).

Also, I think we may want to support lists of IPs only for X-Forwarded-For and not for X-Real-IP. In environments I've seen that use X-Real-IP the proxy is configured to strip out any X-Real-IP headers that come in from the outside leaving only the one supplied by the proxy. (really the xheaders options should specify which headers you expect to see instead of being a simple bool)

Contributor

davidwilemski commented Dec 27, 2012

Yeah, sounds good - I misread the bug report. Is there a problem with running the fix on X-Real-IP headers? The code will return a correct value whether there is a list or not - so if X-Real-IP never sends a list then it still has the correct behavior. I suppose adding a conditional there isn't a big deal though.

I'll fix this up and submit again.

Contributor

davidwilemski commented Apr 16, 2013

I'm finally coming back to this.

Based on what I can tell from other web frameworks, the client IP is expected to be the first in the list and not the last. It does seem you're right about X-Real-IP though - it doesn't seem common for it to contain a list. Rack, gunicorn, and werkzug all seem to correct to the first IP when their xheaders equivalent option is on.

This is an example specifically related to Heroku but seems consistent with what I have read about X-Forwarded-For: http://stackoverflow.com/questions/8026281/parse-x-forwarded-for-to-get-ip-with-werkzeug-on-heroku

Wikipedia and the first example on this page in the Django book seem to suggest the same:
http://en.wikipedia.org/wiki/X-Forwarded-For
http://www.djangobook.com/en/2.0/chapter17.html

Owner

bdarnell commented Apr 17, 2013

The client IP is the first in the list, but the client IP is at best useless and at worst harmful. My client IP right now is 192.168.1.100, but knowing that doesn't do any site I'm visiting any good, and if that were showing up as remote_ip you couldn't then use it to whitelist your internal network since anyone can claim to be whatever IP they want. The last IP in the list is the only one that's actually useful (unless you are behind more than one reverse proxy, but that would require additional degrees of configurability)

Django removed their X-Forwarded-For handler in 1.1: https://docs.djangoproject.com/en/dev/releases/1.1/#removed-setremoteaddrfromforwardedfor-middleware
Werkzeug appears to return the whole list; it's just some stackoverflow poster who suggests using the first one. Gunicorn uses the last: https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/wsgi.py#L126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment