From an email from Benjamin Schrauwen
> As for the 172 issue - this has driven me crazy frankly - has nothing to do with mollom. When I switched hosts about 90 days ago, all of the comments, trackbacks, everything said the 172 address. No one on drupal forums, email support list could seem to help me. Finally my hosting provider told me it's because I am hosted in the cloud and that I have to pass along another code instead of asking for the ip normally. The code is:
> $_SERVER['HTTP_X_CLUSTER_CLIENT_IP']
>
> So I went into trackback module and comment module and changed the code in the actual module. And it started working great immediately - I saw the ip addresses correctly. Then yesterday, I looked again and it's back to 172 again. Now I can't figure out why. Any ideas would be very much appreciated.Yes, getting a correct IP address of a HTTP connection seems to be an
issue all over the place. I found this list of variables which might
hold the visitor's IP address:HTTP_CLIENT_IP
HTTP_X_FORWARDED_FOR
HTTP_X_CLUSTER_CLIENT_IP
HTTP_PROXY_USER
REMOTE_ADDRCould you try too see which works.
With kind regards,
Benjamin.
Comment | File | Size | Author |
---|---|---|---|
#32 | 258397_1.patch | 3.86 KB | Berdir |
#28 | 258397_reroll.patch | 3.85 KB | grendzy |
#17 | core-258397_ip_address.patch | 3.04 KB | Bart Jansens |
#13 | ip_address.patch | 3 KB | John Morahan |
#12 | ip_address.patch | 3 KB | John Morahan |
Comments
Comment #1
Dries CreditAttribution: Dries commentedSee also http://drupal.org/node/172364.
Comment #2
Dries CreditAttribution: Dries commentedThis is a problem for Mollom so I came up with a fix for DRUPAL-6 and CVS HEAD.
I don't have a cluster environment so I can't actually test the patch. I doubt anyone has.
Comment #3
Dries CreditAttribution: Dries commented*My turn to bump a patch*
Comment #4
R.Muilwijk CreditAttribution: R.Muilwijk commentedAdds a test for the ip address function
Comment #5
keith.smith CreditAttribution: keith.smith commentedMinor style issue: if you have to reroll, please end comments with periods so that they form complete sentences. The variations of IP, ip and Ip should all probably be standardized as IP (I guess).
Comment #6
Dries CreditAttribution: Dries commentedI've made the changes suggested by Keith and committed the patch to CVS HEAD. The bootstrap.inc part should go into DRUPAL-6 as well as we're not always capturing the right address.
Comment #7
John Morahan CreditAttribution: John Morahan commentedI don't know what happens in a cluster environment, but under normal circumstances this header can be spoofed.
Comment #8
grandcat CreditAttribution: grandcat commentedThat's right, I'm of the same opinion like John Morahan. If it's a well configured cluster, the real IP should be passed through to the servers behind the proxy. And so, it's their task to perform this step. And I'm also sure that all these servers like APACHE or LIGHTTPD support this feature.
Comment #9
Dries CreditAttribution: Dries commentedYou were right. The brackets were not placed properly. I fixed this in CVS HEAD. Thanks.
Comment #10
Gábor HojtsyHm, what HEAD? What would be the patch to commit to 6.x?
Comment #11
Gábor HojtsyComment #12
John Morahan CreditAttribution: John Morahan commentedThis one, I guess? This is a combination of #4 (minus the test) and http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/bootstrap.inc?r1=...
Comment #13
John Morahan CreditAttribution: John Morahan commentedHere's one with the style changes from #5 too.
Comment #14
markus_petrux CreditAttribution: markus_petrux commentedThe above code is a bit "dangerous". The X-Cluster-Client-Ip header ought to be checked against a whitelist (the same way it is done for the X-Forwarded-For header).
Maybe checks here could reuse the values stored in $reverse_proxy_addresses (see default.settings.php), but then the name of that variable would have to be changed to something more generic.
Comment #15
c960657 CreditAttribution: c960657 commentedRaising to critical, because this patch introduced a security issue in HEAD.
I think there need to be a variable containing the name of the HTTP header that can be trusted. If the reverse proxy is e.g. a Squid, X-Forwarded-For can be trusted, but X-Cluster-Client-Ip can not.
Comment #17
Bart Jansens CreditAttribution: Bart Jansens commentedx-cluster-client-ip is just another non-standard header used by a load balancer. Calling it a cluster may sound fancy but it is no different from any other reverse proxy setup, so why add special code for it? As mentioned before, this is a security hole so this either needs to be fixed or rolled back.
Attached is a proposed fix: add another variable that can be set in settings.php to override which header to check. It defaults to x-forwarded-for as that is more or less the standard way of passing the client ip to the application, but can be changed to whatever header the hosting provider uses.
Comment #18
Bart Jansens CreditAttribution: Bart Jansens commentedComment #19
Dries CreditAttribution: Dries commentedShould we write tests for this? It seems like one of those things we'd want to have tests for ...
Comment #21
c960657 CreditAttribution: c960657 commentedThis should be fixed (or backed out) prior to the release of D7.
Comment #22
alexanderpas CreditAttribution: alexanderpas commentedhow about we only check the HTTP_X_FORWARDED_FOR header (or a custom one like in #17) if REMOTE_ADDR seems to be an RFC 1918 address (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)
Comment #23
markus_petrux CreditAttribution: markus_petrux commented@alexanderpas: You may not want to trust all the nodes on an intranet. ;-)
Comment #24
sun.core CreditAttribution: sun.core commentedThis sounds critical, tests for it are critical, and it should keep critical. (just wandering through the critical queue)
Comment #25
catchMarked #621748: ip_address() is broken for multi-tier architectures as duplicate since this is much the same.
Comment #26
sun.core CreditAttribution: sun.core commentedStill critical. @Dries or anyone involved with this actual change: Any chance to write tests for starters? It's much harder for strangers to properly understand + test the proper behavior. We just need a baseline, others can fill out.
Comment #27
Bart Jansens CreditAttribution: Bart Jansens commentedActually there already is a test for this, it was added by the original patch.
#17 just needs a reroll and probably some explanation to go in default.settings.php
Comment #28
grendzy CreditAttribution: grendzy commentedreroll with documentation in default.settings.php.
Comment #30
grendzy CreditAttribution: grendzy commented#28: 258397_reroll.patch queued for re-testing.
Comment #31
c960657 CreditAttribution: c960657 commentedI agree on the approach here. Just a few nits:
I know the array_key_exists() was there already, but could you change this to empty() instead? This is more in line with what we do elsewhere.
Elsewhere in default.settings.php the header is referred to as “X-Forwarded-For”. For consistency, I think the same should be used here (possibly with a brief mention about how the string is prefixed and converted to upper-case). When “X-Forwarded-For” is first mentioned in each function/file, it may be worth adding that the actual header used is configurable.
I think it should be mentioned that the header value format must be the same as that used by X-Forwarded-For, i.e. if the header contains more than one IP address (separated by comma), the last one is used. It is important, because it may be possible to circumvent the check if a proxy uses another convention.
The data type is obvious from the example code line below. We don't specify the data type for other variables in default.settings.php, except for reverse_proxy and reverse_proxy_addresses (for the latter, the current description is wrong). I suggest we remove these altogether.
Comment #32
BerdirRe-roll.. changed to !empty() and tried to improve the docs a bit.
Comment #34
BerdirCan't reproduce the test fails, lets try again...
Comment #35
Berdir#32: 258397_1.patch queued for re-testing.
Comment #36
c960657 CreditAttribution: c960657 commentedLooks good.
Comment #37
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #39
teastburn85 CreditAttribution: teastburn85 commentedWhy does ip_address() use the rightmost entry from X-Forwarded-For header? Isn't the purpose of ip_address() to get the end-user/client's IP address? The documentation for X-Forwarded-For on wikipedia seems to state that the client's IP is the first mentioned IP (leftmost) and any other servers that touch the request append their IPs to the end of the list (rightmost). Am I misunderstanding how X-Forwarded-For is used in this situation?
Selecting the rightmost IP is breaking our Drupal setup where our servers are behind a CDN that passes the client's IP as the first IP in the X-Forwarded-For header.
Also posted this question on http://drupal.org/node/309586 since they are newest issues I could find.
Thanks in advance
Comment #40
grendzy CreditAttribution: grendzy commentedI think the reason is the left-most one can be forged. Perhaps an additional $conf variable is needed that instructs Drupal how many levels can be trusted?
It might help to know more about your setup. If the remote IP is on the left, what address is on the right in X-Forwarded-For? Does the CDN itself connect through another proxy?
Comment #41
teastburn85 CreditAttribution: teastburn85 commentedThat's true, the leftmost IP is the most easily forged. But what's the purpose of getting the proxy server's address (the rightmost) if what you need is the client's IP, forged or not? If the worry is a forged IP, then presumably the forger is trying to get around your IP ban, in which case retrieving your proxy's IP is rather useless. I think I'm not understanding something here because it really doesn't add up for me.
Our CDN configuration is an origin pull set up with our DNS pointing at their servers, which in turn reach out to ours when necessary. So for every request the user makes that actually gets to our servers (e.g. for pages that specify no caching at all or are dynamic), the CDN appends the X-Forwarded-For header with the user's IP first, then the the IP of the CDN server that fielded the call. An edited header from our site (with fake IPs):
This request came from the CDN to our servers, so for us the $_SERVER['REMOTE_ADDRESS'] variable is useless for tracking a user by IP. The X-Forwarded-For header is in this form:
Drupal's ip_address function returns [CDN_RECEIVER_IP] which only varies between the number of servers our CDN uses to accept user requests to our domain. When looking for the client's IP, I don't understand why the rightmost IP makes any sense or is helpful, even if the leftmost is forged.
Comment #42
roychri CreditAttribution: roychri commentedSame problem here. We have some LB pointing to proxies pointing to a proxy and then the origin server.
The rightmost ip on the list is NOT the client's IP.
Someone posted a patch on a issue (duplicate to this one) where he was removing all trusted ips from the list before using the
leftmostrightmost. That would seem more prudent/broad, no?http://drupal.org/files/issues/7X29YcLCtUMmeqrdQz67VW3RXwU.patch
Edited after @grendzy mentioned my typo.
Comment #43
grendzy CreditAttribution: grendzy commentedThis patch looks pretty good to me, though it no longer applies and I haven't tested it yet. (btw roychri I think you mean the *rightmost* address after removing the trusted IPs, which is what the patch you linked to implements.) Additional test coverage for multiple values in XFF would be good.
BTW the link above came from #621748: ip_address() is broken for multi-tier architectures
Comment #44
roychri CreditAttribution: roychri commented@grendzy - A patch was already committed so I find it normal that the patch I mentioned cannot apply now.
I was not implying that this patch was better than the one that was committed. I only wanted to show a reference to something that was working in our case (where we have multiple proxies which appear in the XFF).
I would expect a new patch that would modify the existing code. Maybe I'll have some free time later to propose a patch.
Comment #45
catchHmm, looks like this issue never actually covered the case on #621748: ip_address() is broken for multi-tier architectures, so we should rather unduplicate that one and leave this closed.
Comment #46
grendzy CreditAttribution: grendzy commentedteastburn85, roychri: a new patch is up at #621748: ip_address() is broken for multi-tier architectures, if you have time to test it out that would be appreciated.
Comment #47
teastburn85 CreditAttribution: teastburn85 commented@grendzy That patch looks good, but unfortunately doesn't apply to my situation. Since we are behind a CDN, it is bad practice and just plain unlikely for us to know all the IPs of server's that could be serving our content. The list would be very long since our provider has farms all over the world. Also, neither would they want to nor would we want them to notify us every time an IP changes or they add a new server or remove an old one.
I think there is a fundamental problem here in the approach, since it seems to work well for the vast majority of Drupal users, but doesn't work well for the ones that need to scale Drupal, which as I understand is the minority. Also, at this time in our particular situation, we're not worried about IP spoofing so much of the ip_address function isn't worth much for us. I don't know if this is the approach you might want to take in the future, but we've implemented a few $conf variables that allow bypassing of the reverse proxy address list check and instead might introduce a callback function or a hook to take care of the work so Drupal doesn't have to cover every situation on its own.
Thanks again
Comment #48
Damien Tournoud CreditAttribution: Damien Tournoud commented@teastburn85: your problem is completely unrelated to what is being solved here. Your CDN probably has a way to give you the correct IP address of the client in a non-spoofable way. Drupal cannot and will not support every proprietary headers of every CDN over the world.
You can implement the logic you need to get the correct value from
$_SERVER['REMOTE_ADDR']
right into your settings.php file, without any need to patch core.Comment #49
roychri CreditAttribution: roychri commented@teastburn85: I agree with Damien. Even if you are not concerned with spoofed IP, the code should still handle that.
And the workaround of settings.php still works for you in your special case.
Comment #50
teastburn85 CreditAttribution: teastburn85 commented@Damien I wouldn't say my issue is completely unrelated, but maybe unrelated enough to be out of the scope of this discussion. In any case, I appreciate your help, but if
$_SERVER['REMOTE_ADDR']
was all I needed you can trust I wouldn't be here now. I am here because the only way I could get Drupal to play nice with a standard XFF header was to hack the ip_address function, which I thought Drupal devs might like to know about. Agreed that Drupal shouldn't support every CDN, thats why my suggestion of a hook was included. If Drupal wants to be seriously considered as a production CMS for high traffic sites, it will have to grapple with these very same CDN issues.Thanks
Comment #51
grendzy CreditAttribution: grendzy commentedI think you misread Damien's post. What you need can be accomplished very simply without hacking core. The suggestion is assigning something to $_SERVER['REMOTE_ADDR'] when custom logic is needed. For example, if you must have the left-most address regardless of possible forgery, this one line in
settings.php
will solve your problem without hacking core:Comment #52
joelcollinsdc CreditAttribution: joelcollinsdc commentedI found this very confusing and agree with teastburn85. For the future would allowing a user to specify a header as being "trusted" be a solution?
I created an issue to improve the documentation related to this:
http://drupal.org/node/1310250