@indirect I suspect you might have some Opinions here 😃

So many opinions >_>

This change is explicitly trying to partially disable proxy-removal during IP address detection. I'm okay with trying to return the IP that is actually making requests. I really don't like the way that this changes the semantics of IP detection. As the tests demonstrate, your changes effectively ignore un-trusted IPs in the proxy list. Both 'unknown,::1' and '::1,unknown' would return '::1'.

Why do you need to both allow proxies and count requests from proxies? Why can't you fix this problem by modifying the proxy detection regex?

It seems like what you want is for requests to count the same IP address as BOTH proxy and non-proxy, and that worries me. Too clever. :P

Mostly, my concern is over the default behaviour. Right now, any client IP on my (10/8) LAN gets reported to the application as being 127.0.0.1, because of the local nginx proxy.

My contention is if we trust all the IPs, then by definition, they are all viable proxies. Why would we pick the most-directly-connected one as the only one that's allowed to be "dual-role"?

Similarly, if you run curl from your proxy, why would said proxy's IP not be the right thing to report?

Instead of the ::1 case, I'll pick on this one, because it seems easier to discuss by not being localhost:

    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
    assert_equal '192.168.0.1', request.remote_ip

(that used to be expected to return nil)

If I give it a REMOTE_ADDR, things get more interesting. With my implementation:

    request = stub_request 'REMOTE_ADDR' => '127.0.0.1',
                           'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
    assert_equal '192.168.0.1', request.remote_ip

and before:

    request = stub_request 'REMOTE_ADDR' => '127.0.0.1',
                           'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
    assert_equal '127.0.0.1', request.remote_ip

Which one of those is really worse? 😐

That said, I could certainly get behind an effort to be more aggressive that if our "outer" trusted proxy can't give us a valid IP, we should return nil. (On the other hand, if it can't reliably manage to record which IP it's talking to, why do we trust it? 😏)

It actually feels like the logic would be simplified (and the "spoofing!!!" warning mollified) if we just treated the REMOTE_ADDR as yet another potential proxy address: if we don't trust it, we look no further (and so ignore any supplied X-Forwarded-For)... if we do, we move on to the next-closest. To my reading, REMOTE_ADDR is currently special-cased more than it needs to be... move it to the front of ips, and walk away.

(But that would create a real behavioural change, for people currently using proxies that aren't marked as trusted. So I'm not sure that's viable. And probably opens up a deeper philosophical discussion.)

Mostly, my concern is over the default behaviour. Right now, any client IP on my (10/8) LAN gets reported to the application as being 127.0.0.1, because of the local nginx proxy.

So, you have a use-case that is unusual (clients on your LAN), and you can fix that particular case by feeding the RemoteIP middleware a different regex to use for trusted IPs. Why don't you do that, instead of patching Rails? It's configurable on purpose. :)

As for the underlying philosophical differences: The point of special-casing REMOTE_ADDR is because it isn't just another potential proxy. Additionally, if a trusted proxy says it is proxying for a nil IP address, a nil address is exactly what we should be recording. We don't get to special-case ignoring the trusted proxy just because it's telling us something we would rather not be told.

Admittedly, the RemoteIP middleware isn't hugely important in the grant scheme of things, but it is at least one form of potential security issue, so I don't think it should be changed in this particular way. Especially since you can get exactly what you want by simply telling the middleware that you have clients in your LAN instead of proxies.

Well firstly, I'm not trying to solve my problem: my concern is that the default can have a badly surprising effect. And while Heroku and AWS might be very popular, I do reject the notion that connecting to a Rails app from a local IP is ignorably boutique.

Let's assume I have the best interests of the framework at heart, and focus on the example:

if a trusted proxy says it is proxying for a nil IP address, a nil address is exactly what we should be recording

That's not what's actually happening at the moment. As I said, it sure sounds like a reasonable notion... but the handling of 'unknown' is currently far from that, and the tests that currently show nil are lying.

I'll put together something that actually treats 'unknown' entries as we both seem to think they should be, and see whether we agree on how those tests look.

In the meantime, I'll try to catch you on Campfire -- that might be a quicker option to converge on expected behaviour.

Unfortunately I'm in Thailand at the moment, so coordinating might be tough.

As for "ignorably botique", that's just ridiculous. :P It's a well-known convention that Rails apps are deployed with proxies and/or load balancers. As a result, the convention is to assume that LAN IPs are proxies rather than end clients. If your setup diverges from convention that is completely fine, because it's easily configurable. The existence of non-conventional setups is not a reason to remove the convention.

I'm still open to patches that change the behaviour, as long as the reason is more substantial than "some people have non-conventional setups", and I'm happy to look at tests anytime.

On Sat, Apr 5, 2014 at 6:15 PM, Matthew Draper [email protected]
wrote:

Well firstly, I'm not trying to solve my problem: my concern is that the default can have a badly surprising effect. And while Heroku and AWS might be very popular, I do reject the notion that connecting to a Rails app from a local IP is ignorably boutique.
Let's assume I have the best interests of the framework at heart, and focus on the example:

if a trusted proxy says it is proxying for a nil IP address, a nil address is exactly what we should be recording
That's not what's actually happening at the moment. As I said, it sure sounds like a reasonable notion... but the handling of 'unknown' is currently far from that, and the tests that currently show nil are lying.
I'll put together something that actually treats 'unknown' entries as we both seem to think they should be, and see whether we agree on how those tests look.

In the meantime, I'll try to catch you on Campfire -- that might be a quicker option to converge on expected behaviour.

Reply to this email directly or view it on GitHub:
#14600 (comment)

To be clear, I definitely agree that we should trust local IPs to act as honest proxies by default, and don't mean to suggest that should change.

It's only the behaviour when all known IPs are identified as trusted that seems a bit unexpected to me, and that's the only thing I'm suggesting we adjust -- so, it's certainly about a less-conventional request path, but I have no wish to affect the behaviour when the stronger convention is in play.

Anyway, I'll do another thing, and we can see how that looks.

That sounds good. I think the initial reasoning behind the current behaviour was a variant of what you are suggesting: if we only see trusted IPs, then the request must originate from a trusted IP, so we should allow trusted IPs. Tests for this sound great.

On Sat, Apr 5, 2014 at 10:11 PM, Matthew Draper [email protected]
wrote:

To be clear, I definitely agree that we should trust local IPs to act as honest proxies by default, and don't mean to suggest that should change.
It's only the behaviour when all known IPs are identified as trusted that seems a bit unexpected to me, and that's the only thing I'm suggesting we adjust -- so, it's certainly about a less-conventional request path, but I have no wish to affect the behaviour when the stronger convention is in play.

Anyway, I'll do another thing, and we can see how that looks.

Reply to this email directly or view it on GitHub:
#14600 (comment)

the behaviour when all known IPs are identified as trusted

+1 - by falling back to remote_addr in this case, we're guaranteed to be wrong. The "furthest" IP should be where we land.

Just stumbled upon this.
Seems like you guys found a consensus.
How about merging?

This looks like a great PR.
@matthewd anyway I can help? You want me to rebase / fix up?

Any news here?

Any news here? :)

If anyone's up for rebasing this (and possibly comparing the result with rack/rack#1224), please do.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

I updated with PR #38150

Thanks for the constructive discussion and fix