Restore original remote_ip algorithm. #7980

steveklabnik · 2012-10-18T00:19:53Z

We incorrectly were using the first address, rather than the last one.

steveklabnik · 2012-10-18T00:20:18Z

I am 99% sure this is right, the tests were new, so I wasn't sure how to 'revert' them. Please check this request well before merging. ;)

indirect · 2012-10-18T01:24:56Z

I put some notes into the extended commit messages, but the upshot is that it works correctly now, it ignores private IPv6 IPs now (which it didn't do before), and the code is much shorter. Yay. :)

rafaelfranca · 2012-10-18T01:28:24Z

actionpack/lib/action_dispatch/middleware/remote_ip.rb

@@ -105,22 +99,15 @@ def to_s
        @ip = calculate_ip
      end

-      private
+    private


Opps. Take care to not change the code conventions.

In my defense, he changed my outdented protected to an indented private. :) I'll restore it to what it was before the commit I am reverting here.

rafaelfranca · 2012-10-18T01:29:54Z

I did some comments inline. The feature seems fine but we need to remove the comment tests (if they are not needed) or uncomment they (if they are needed)

indirect · 2012-10-18T01:40:39Z

Oops! Fixing, thank you.

On Oct 17, 2012, at 6:30 PM, Rafael Mendonça França notifications@github.com wrote:

I did some comments inline. The feature seems fine but we need to remove the comment tests (if they are not needed) or uncomment they (if they are needed)

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

gazay · 2012-10-18T02:13:19Z

I'm not sure that this is right to use last address from header
X-Forwarded-For, in every specification about this header written that
proxy should add initiator's IP addres to the end of list, so if we go
through proxy1, proxy2, proxy3 we get list like clientIP, proxy1IP,
proxy2IP, proxy3IP. You can see it in wikipedia and in referenced links
from there, also from w3c specification.
http://www.w3.org/TR/2009/WD-ct-guidelines-20091006/#sec-additional-headers

Here is the usage of mod_extract_forwarded for apache for example:
http://www.openinfo.co.uk/apache/index.html

Sorry, I have problems with accessing github.com right now, so I comment
from mail.

On Thu, Oct 18, 2012 at 9:41 AM, André Arko notifications@github.comwrote:

Oops! Fixing, thank you.

On Oct 17, 2012, at 6:30 PM, Rafael Mendonça França <
notifications@github.com> wrote:

I did some comments inline. The feature seems fine but we need to remove
the comment tests (if they are not needed) or uncomment they (if they are
needed)

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

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/7980#issuecomment-9550556.

indirect · 2012-10-18T02:41:52Z

Ohhh, I understand what the problem is now. Many proxies don't actually modify the X-Forwarded-For header. Instead, they add another X-Forwarded-For header. According to the HTTP spec, that means that the values should be concatenated, joined by a comma. If you're using a ruby server that understands repeated headers, the correct IP will then be the last value. If the proxy conforms to the W3C spec that you linked, the correct IP will be the first value.

Um. How can we resolve this? FWIW, I've only ever seen the "adds a second header" proxy type in the wild (I know HAProxy does this).

indirect · 2012-10-18T02:44:06Z

In addition, other blog posts I have read on the subject seem to indicate that the last IP is the correct one in real-world scenarios.

indirect · 2012-10-18T02:45:23Z

And indeed, the Apache bug on this subject also seems to imply we should be using the last IP address in the concatenated list.

gazay · 2012-10-18T02:47:49Z

I did my pull last time because I used this functionality on groupon
russia, and there getting first IP address from header was right solution.
Maybe we can do preconfigured usage of remote_ip with your algorithm as
default?

On Thu, Oct 18, 2012 at 10:45 AM, André Arko notifications@github.comwrote:

And indeed, the Apache bug on this subjecthttps://issues.apache.org/bugzilla/show_bug.cgi?id=50453also seems to imply we should be using the last IP address in the
concatenated list.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/7980#issuecomment-9551581.

indirect · 2012-10-18T15:53:20Z

It seems like an option on the middleware is the way to go if this is a common need, and a replacement middleware that removes the calls to reverse is the way to go if its not common. WDYT, people who can merge this?

steveklabnik · 2012-10-18T17:40:06Z

Ugh.

indirect · 2012-10-18T17:41:54Z

@steveklabnik inorite? 😭

rafaelfranca · 2012-10-20T18:58:41Z

I don't like the idea to add Yet Another Configuration Option, but I don't know if this is not common

steveklabnik · 2012-10-20T19:00:08Z

I always prefer what is correct to what is not. It's not our job to fix bugs in other projects, we should follow the spec.

Then again, I don't have anything currently deployed in production that depends on this in either direction ;)

indirect · 2012-10-20T19:38:56Z

If it helps at all, this pull does fix at least one bug in the trusted proxies regex, and also restores the same behaviour as e.g. Apache. I personally think that's the way it should work, but hey. I'm biased. :)

On Oct 20, 2012, at 12:00 PM, Steve Klabnik notifications@github.com wrote:

I always prefer what is correct to what is not. It's not our job to fix bugs in other projects, we should follow the spec.

Then again, I don't have anything currently deployed in production that depends on this in either direction ;)

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

ehoch · 2012-10-22T14:11:23Z

Figured I'd weigh in here. I use Google Page Speed Service in front of Heroku. Not the craziest, most unheard of Rails environment. There, the client's IP is the FIRST in the X-Forwarded-For csv.

Can we make this configurable?

indirect · 2012-10-22T15:35:06Z

I guess the upside is that it's always going to be first or last? Unless you combine Google Page Speed and HAProxy, then it'll be in the middle. -_-

I'm ready to implement an option that lets you tell the middleware whether you want the first or last IP. I'm not sure which option should be the default, though, since the "spec correct" one is first and the "works like Apache etc" one is last. :|

gazay · 2012-10-22T15:39:13Z

I think that default should be last IP (your implementation), because long time nobody noticed that, and my pull was opened about 8 months before merged in. So I think first IP is like exception.

I can help you with implementation

indirect · 2012-10-22T15:43:36Z

Makes sense to me. Thanks for offering to help! I imagine @steveklabnik can get you hooked up with push to his repo so you can add to this pull request if you have some patches.

On Oct 22, 2012, at 8:39 AM, Alexey Gaziev notifications@github.com wrote:

I think that default should be last IP (your implementation), because long time nobody noticed that, and my pull was opened about 8 months before merged in. So I think first IP is like exception.

I can help you with implementation

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

steveklabnik · 2012-10-22T15:45:55Z

Yes, @gazay let me know if you have some commits and I can give you access to my repo.

gazay · 2012-10-22T15:47:02Z

Great, thanks guys!

gazay · 2012-10-22T18:09:34Z

So, I'm ready to push some commits. I did the same option usage as with ip_spoofing_check. And I wrote some tests to check different order usage

And btw I think that we shouldn't filter values from X_FORWARDED_FOR and give to user unexpected IP of unknown proxy server. We should give him only first or last ip from list as we decided here. I changed code in remote_ip.rb and fix tests for it. What do you think @indirect, @steveklabnik?

steveklabnik · 2012-10-22T18:25:24Z

You're added to my repo.

gazay · 2012-10-22T18:33:10Z

I've added commits. I'm not sure about name of option - x_forwarded_for_last_ip, but that's all I could think of

indirect · 2012-10-22T18:54:16Z

I think the ability to pass in proxies to be filtered is very important, because its the only way to get the real IP in a situation where one proxy adds to the front but another proxy adds to the end. So I think we should keep it. Since the option is already scoped to the remote IP middleware, and applies to more than one header, I would suggest naming it "first_ip" or "first_client_ip".

On Oct 22, 2012, at 11:09 AM, Alexey Gaziev notifications@github.com wrote:

So, I'm ready to push some commits. I did the same option usage as with ip_spoofing_check. And I wrote some tests to check different order usage

And btw I think that we shouldn't filter values from X_FORWARDED_FOR and give to user unexpected IP of unknown proxy server. We should give him only first or last ip from list as we decided here. I changed code in remote_ip.rb and fix tests for it. What do you think @indirect, @steveklabnik?

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

indirect · 2012-10-22T18:55:52Z

And of course when I just suggested "first_ip", I actually meant "last_ip". :P

On Oct 22, 2012, at 11:33 AM, Alexey Gaziev notifications@github.com wrote:

I've added commits. I'm not sure about name of option - x_forwarded_for_last_ip, but that's all I could think of

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

gazay · 2012-10-23T01:26:06Z

About filtering – I see the problem, you right, I'll change it back. About naming - last_ip or last_client_ip not really clear when someone will read this code or try to change option. It's like if ip_spoofing_check will be just ip_check - not sure what this option allow to check. Maybe last_forwardable_ip?

indirect · 2012-10-23T05:09:43Z

Ahh, I see. I was thinking the option should apply to REMOTE_ADDR too, but it seems that that header will only ever have a list of IPs due to duplicated headers, and so reverse is always the correct direction.

How about calling the option "last_forwarded_ip"? That includes both the important part of the header name, and literally describes what you will get if you turn the option on -- the last IP from the proxy's list of forwarded IPs.

indirect · 2012-11-18T19:21:17Z

@gingerlime ahh, you make a very good point. I didn't connect the dots during the earlier discussion, but the option shouldn't be needed at all if you can explicitly list the proxies whose addresses you don't want to count. @gazay, would that work in your situation?

Given the potential for IP spoofing vulnerabilities, I think we might want to ultimately revert to the original behaviour without an option, and suggest in the comments/docs that anyone who needs something custom should remove this middleware and insert their own instead.

NZKoz · 2012-11-18T20:34:52Z

Yes, while we read from X-Forwarded-For in the implementation of remote_ip then you'll always be potentially vulnerable to spoofing attacks. I'd suggest just adding something like:

  request.forwarded_ips # => ['x.x.x.x', 'y.y.y.y', 'z.z.z.z']

Then the config just becomes a question of first or last from that method.

FWIW, if you're using remote_ip in a security setting you're completely crazy ;)

indirect · 2012-11-18T20:38:15Z

The config in question is pretty much exactly that (mod some filtering of obviously not useful IPs). So that seems okay, I guess.

On Nov 18, 2012, at 12:35 PM, Michael Koziarski notifications@github.com wrote:

Yes, while we read from X-Forwarded-For in the implementation of remote_ip then you'll always be potentially vulnerable to spoofing attacks. I'd suggest just adding something like:

request.forwarded_ips # => ['x.x.x.x', 'y.y.y.y', 'z.z.z.z']
Then the config just becomes a question of first or last from that method.

FWIW, if you're using remote_ip in a security setting you're completely crazy ;)

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

gingerlime · 2012-11-22T21:28:23Z

@NZKoz I just re-read your comment:

while we read from X-Forwarded-For in the implementation of remote_ip then you'll always be potentially vulnerable to spoofing attacks

This is not entirely accurate. If you trust your proxy server(s), and by trust I also mean you trust them to handle X-Forwarded-For appropriately (and barring any vulnerabilities those might have). Then you should be able to trust a portion of the header's content, which should produce the IP address of the device connected to your outer-most trusted proxy.

If you don't use any (trusted) proxy servers, then you should simply ignore the X-Forwarded-For header completely.

steveklabnik · 2012-11-28T22:01:27Z

So: what exactly needs to be done to move this forward?

indirect · 2012-11-28T22:31:30Z

...better...docs?

indirect · 2012-11-28T22:42:27Z

I've thought about this some more, and I'd like to propose a different final solution: how about we remove the first/last IP option, revert to the (safe) last IP, and beef up the docs explaining how to use the trusted proxy option to remove IPs from the end until what's left is the IP that you're interested in?

On Nov 28, 2012, at 2:01 PM, Steve Klabnik notifications@github.com wrote:

So: what exactly needs to be done to move this forward?

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

steveklabnik · 2012-11-28T22:44:03Z

I can see that too. @NZKoz what do you think?

NZKoz · 2012-11-28T22:45:53Z

sounds good to me

indirect · 2012-11-28T22:50:05Z

Great. I'll try to get that implemented today.

steveklabnik · 2012-12-18T08:10:58Z

Ping! @indirect did you get a chance to play with this?

indirect · 2012-12-18T17:57:39Z

Oh man. Totally fell off my list. Today or tomorrow, I hope.

steveklabnik · 2012-12-18T18:34:30Z

No worries bro, know you've been busy.

jfirebaugh · 2012-12-24T19:46:16Z

I agree with reverting to last IP and documenting the trusted proxy option.

What I would like to see is an option to ignore HTTP_CLIENT_IP. In an EC2+ELB environment that header is effectively garbage and should be ignored (ELB only uses X-Forwarded-For, not Client-IP). The fact that it is used to try to detect spoofing results in legitimate users getting locked out. If not an option, then at least extract some finer-grained methods so I can monkey patch more easily.

def client_ip
  @env['HTTP_CLIENT_IP']
end

def forwarded_ips
  ips_from('HTTP_X_FORWARDED_FOR')
end

def remote_addrs
  ips_from('REMOTE_ADDR').reverse
end

def candidate_ips
  [client_ip, forwarded_ips, remote_addrs].flatten.compact
end

def remote_ip
  remove_proxies(candidate_ips).first || remote_addrs.first
end

indirect · 2012-12-24T20:28:00Z

That seems reasonable to me.

On Dec 24, 2012, at 11:46 AM, John Firebaugh notifications@github.com wrote:

I agree with reverting to last IP and documenting the trusted proxy option.

What I would like to see is an option to ignore HTTP_CLIENT_IP. In an EC2+ELB environment that header is effectively garbage and should be ignored (ELB only uses X-Forwarded-For, not Client-IP). The fact that it is used to try to detect spoofing results in legitimate users getting locked out. If not an option, then at least extract some finer-grained methods so I can monkey patch more easily.

def client_ip
@env['HTTP_CLIENT_IP']
end

def forwarded_ips
ips_from('HTTP_X_FORWARDED_FOR')
end

def remote_addrs
ips_from('REMOTE_ADDR').reverse
end

def candidate_ips
[client_ip, forwarded_ips, remote_addrs].flatten.compact
end

def remote_ip
remove_proxies(candidate_ips).first || remote_addrs.first
end
�
Reply to this email directly or view it on GitHub.

indirect · 2013-01-02T01:04:14Z

@jfirebaugh after editing this, I'm not sure why you want to be able to monkeypatch this at all. You can completely resolve your stated use-case by adding a middleware farther out that strips the "garbage" header, and then disable the spoof checking using the option that is provided.

I personally would like to remove the spoof checking, because AFAIK there aren't any proxies that set both Client-Ip and X-Forwarded-For, but I'm sure that's a totally separate PR and discussion. :)

jfirebaugh · 2013-01-02T01:16:17Z

Figuring out the correct IP is the responsibility of the RemoteIp middleware, right? Folks shouldn't have to write a second middleware that deals in the same protocol details as RemoteIp in order to get RemoteIp to work correctly in what is a very common environment.

I'm all for removing the spoof checking as well, but I think there needs to be a configuration option to say which of Client-Ip and X-Forwarded-For is accurate.

indirect · 2013-01-02T02:54:22Z

The job of this middleware is to return the IP address reported in the headers of the request. IMO, if you want to ignore one of those headers, then yes, you should have to write code that deletes that header. Have you asked the Amazon guys about fixing their headers so that code isn't needed?

On Jan 1, 2013, at 5:16 PM, John Firebaugh notifications@github.com wrote:

Figuring out the correct IP is the responsibility of the RemoteIp middleware, right? Folks shouldn't have to write a second middleware that deals in the same protocol details as RemoteIp in order to get RemoteIp to work correctly in what is a very common environment.

I'm all for removing the spoof checking as well, but I think there needs to be a configuration option to say which of Client-Ip and X-Forwarded-For is accurate.

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

indirect · 2013-01-02T23:23:25Z

Okay. I have coded and documented the fuck out of this thing. /cc @steveklabnik

rafaelfranca · 2013-01-02T23:24:33Z

needs rebase

indirect · 2013-01-02T23:25:21Z

@rafaelfranca bro, I just rebased this 4 minutes ago. It needs another one?!

Proxy servers add X-Forwarded-For headers, resulting in a list of IPs. We remove trusted IP values, and then take the last given value, assuming that it is the most likely to be the correct, unfaked value. See [1] for a very thorough discussion of why that is the best option we have at the moment. [1]: http://blog.gingerlime.com/2012/rails-ip-spoofing-vulnerabilities-and-protection/ Fixes rails#7979

indirect · 2013-01-02T23:26:59Z

Huh, stupid CHANGELOG. :P Rebased! Again! QUICK SOMEBODY PUSH THE MERGE BUTTON

Restore original remote_ip algorithm.

steveklabnik · 2013-01-03T03:06:45Z

🤘

Thanks guys. ❤️

amoose · 2013-04-17T21:32:22Z

o_O

rafaelfranca reviewed Oct 18, 2012
View reviewed changes

guilleiguaran added a commit that referenced this pull request Jan 2, 2013

Merge pull request #7980 from steveklabnik/issue_7979

c79fb2a

Restore original remote_ip algorithm.

guilleiguaran merged commit c79fb2a into rails:master Jan 2, 2013

steveklabnik deleted the issue_7979 branch January 3, 2013 03:06

ashtuchkin mentioned this pull request May 4, 2014

Better handling of X-Forwarded-For header to get client ip address expressjs/express#2099

Closed

rafaelfranca mentioned this pull request Feb 8, 2017

Remote ip should use the first ip address #27942

Closed

Restore original remote_ip algorithm. #7980

Restore original remote_ip algorithm. #7980

Conversation

steveklabnik commented Oct 18, 2012

steveklabnik commented Oct 18, 2012

indirect commented Oct 18, 2012

rafaelfranca Oct 18, 2012

Choose a reason for hiding this comment

indirect Oct 18, 2012

Choose a reason for hiding this comment

rafaelfranca commented Oct 18, 2012

indirect commented Oct 18, 2012

gazay commented Oct 18, 2012

indirect commented Oct 18, 2012

indirect commented Oct 18, 2012

indirect commented Oct 18, 2012

gazay commented Oct 18, 2012

indirect commented Oct 18, 2012

steveklabnik commented Oct 18, 2012

indirect commented Oct 18, 2012

rafaelfranca commented Oct 20, 2012

steveklabnik commented Oct 20, 2012

indirect commented Oct 20, 2012

ehoch commented Oct 22, 2012

indirect commented Oct 22, 2012

gazay commented Oct 22, 2012

indirect commented Oct 22, 2012

steveklabnik commented Oct 22, 2012

gazay commented Oct 22, 2012

gazay commented Oct 22, 2012

steveklabnik commented Oct 22, 2012

gazay commented Oct 22, 2012

indirect commented Oct 22, 2012

indirect commented Oct 22, 2012

gazay commented Oct 23, 2012

indirect commented Oct 23, 2012

indirect commented Nov 18, 2012

NZKoz commented Nov 18, 2012

indirect commented Nov 18, 2012

gingerlime commented Nov 22, 2012

steveklabnik commented Nov 28, 2012

indirect commented Nov 28, 2012

indirect commented Nov 28, 2012

steveklabnik commented Nov 28, 2012

NZKoz commented Nov 28, 2012

indirect commented Nov 28, 2012

steveklabnik commented Dec 18, 2012

indirect commented Dec 18, 2012

steveklabnik commented Dec 18, 2012

jfirebaugh commented Dec 24, 2012

indirect commented Dec 24, 2012

indirect commented Jan 2, 2013

jfirebaugh commented Jan 2, 2013

indirect commented Jan 2, 2013

indirect commented Jan 2, 2013

rafaelfranca commented Jan 2, 2013

indirect commented Jan 2, 2013

indirect commented Jan 2, 2013

steveklabnik commented Jan 3, 2013

amoose commented Apr 17, 2013