Hi Ananth,
On Thu, Jan 22, 2015 at 06:38:31PM +0530, Ananth RK wrote:
That looks like (if X-Forwarded-For is “valid”) your jumping straight to considering the X-Forwarded-For header. You might want to check beforehand whether the client ip is a “trusted” proxy. And only if it is, you should walk the X-Forwarded-For.
Also, it sounds a bit like you're going through X-Forwarded-For from left to right. Make sure to walk it from right to left, as proxies are expected to append (not prepend) the client IP they forward for.
Okay. Assuming we append client_ip to X-Forwarded-For and process the list from right-to-left, do you still think we should explicitly check whether client_ip is a trusted proxy? Because, per the modified algorithm, the client_ip will be the first item to be processed and if it is not a trusted proxy, it will be returned as the result - which is what even the explicit initial check will result in this case as well. Correct?
Right, if you implement the pseudo-code algorithm I sketched, you can skip that step.
But if you optimize and loop-unroll the first iteration (like your implementation did), you want to check whether or not it's a trusted proxy. But whether one would label that loop-unrolling “premature optimization” or “making the code more readable” depends a bit on the concrete formulation in code.
The way you combine the looping and if looks like you might be skipping across entries that you cannot parse. But if you find invalid entries while backtracking the IPs, you're probably in parts of the X-Forwarded-For value that you shouldn't trust.
(X-Forwarded-For can easily be spoofed by clients)
We only check that each entry is a valid IPv4 or IPv6 address and not just a random string.
If we find invalid entries, then the only option we have is to continue the search further for a valid IP.
Ex falso quodlibet.
Hence, the only option one has is to stop at untrusted values in the chain of X-Forwarded-For IPs.
list_of_ips = append client_ip to X-Forwarded-For for ip in reverse( list_of_ips ) do if ip is not a trusted proxy or the iterator does not have more
elements
then return ip fidone
(That should give invalid IP addresses for some requests. That seems to be the correct thing from my point of view. But if you rather geolocate wrong than not geolocate at all, you can choose to pick the last known good IP address instead. I guess that's more a matter of taste.)
What would picking the last known good IP address in this context? An IP that matches the IPv4/IPv6 pattern _AND_ is not a trusted proxy?
As indicated above, I'd rather indicate “Unable to geolocate” than knowingly geolocate wrong.
But by the “last known good IP address” above I meant (in case of faulty X-Forwarded-For), the last IP address that was not obviously wrong could be picked.
Let's have some examples. For the examples, I am assuming that only 1.0.0.0/8 are trusted proxies.
* Case 1:
Client IP: 2.0.0.1 X-Forwarded-For: 1.0.0.2, 1.0.0.3
-> last known good IP address: “2.0.0.1”
* Case 2:
Client IP: 1.0.0.1 X-Forwarded-For: 1.0.0.2, 2.0.0.1
-> last known good IP address: “2.0.0.1”
* Case 3:
Client IP: 1.0.0.1 X-Forwarded-For: foo, 2.0.0.1
-> last known good IP address: “2.0.0.1”
* Case 4:
Client IP: 1.0.0.1 X-Forwarded-For: 1.0.0.2, 1.0.0.3, foo
-> last known good IP address: “1.0.0.1”
* Case 5:
Client IP: 1.0.0.1 X-Forwarded-For: 1.0.0.2, foo, 1.0.0.3
-> last known good IP address: “1.0.0.3”
* Case 6:
Client IP: 1.0.0.1 X-Forwarded-For: foo, 1.0.0.2, 1.0.0.3
-> last known good IP address: “1.0.0.2”
* Case 7:
Client IP: foo
-> This should never happen. File a bug in phabricator.
But let me repeat that I think “Unable to geolocate” is a valid and good response. Better than using some proxy in the chain.
Have fun, Christian