Make WordPress Core

Opened 16 years ago

Closed 10 years ago

#9235 closed enhancement (wontfix)

Extract real IP behind a load balancer

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: has-patch
Focuses: Cc:

Description

http://wordpress.org/extend/plugins/real-ip/

It's a 4 liner, that seems like good candidate for inclusion in wp-settings.php after the IIS fixes.

Attachments (7)

9235.diff (1.5 KB) - added by Denis-de-Bernardy 16 years ago.
9235.2.diff (1.8 KB) - added by Denis-de-Bernardy 15 years ago.
9235.3.diff (1.1 KB) - added by Denis-de-Bernardy 15 years ago.
9235.4.diff (590 bytes) - added by Denis-de-Bernardy 15 years ago.
wp_get_ip.diff (1.4 KB) - added by scribu 15 years ago.
Introduce wp_get_ip()
wp_remote_ip.diff (1.4 KB) - added by scribu 15 years ago.
get_remote_ip.patch (2.2 KB) - added by peetaur 11 years ago.
patch for configurable proxy IP plus replacing for comments

Download all attachments as: .zip

Change History (52)

#1 @Denis-de-Bernardy
16 years ago

  • Keywords has-patch added

#2 @johnbillion
16 years ago

  • Type changed from defect (bug) to enhancement

Rather than modifying the $_SERVER variable, wouldn't it be better to introduce (or modify) some sort of get_ip() function?

#3 @Denis-de-Bernardy
16 years ago

I doubt it. you could reasonably expect a beginner to think that the $_SERVER variable contains relevant data. Few php coders think about sanitizing things such as the referrer... Even less imagine that the IPs in $_SERVER are incorrect.

#4 @Denis-de-Bernardy
16 years ago

  • Keywords commit added; dev-feedback removed

#6 @Denis-de-Bernardy
15 years ago

  • Component changed from General to Optimization

#7 @ryan
15 years ago

HTTP_X_FORWARDED_FOR can be a comma separated list.

#8 @Denis-de-Bernardy
15 years ago

new patch should deal with this fine.

#9 @Denis-de-Bernardy
15 years ago

  • Owner changed from anonymous to Denis-de-Bernardy
  • Status changed from new to accepted

#10 @ryan
15 years ago

From what I've been told, you can't rely on HTTP_X_FORWARDED_FOR being set to something sane. This should be enabled only when you are sure the host uses HTTP_X_FORWARDED_FOR and uses it correctly. If we build this into core, we would probably need a USE_FORWARDED_IP flag that users can define in wp-config.php.

#11 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch early added; has-patch commit removed
  • Milestone changed from 2.8 to 2.9

Seems there are a few more, too:

http://coding-talk.com/f14/vbulletin-fix-for-apache-nginx-3384/

Let's punt this one to 2.9 early.

#12 @robertaccettura
15 years ago

I've gotten to deal with WordPress behind a CDN or reverse proxy a few times now. From what I've seen there are at least a dozen different HTTP headers that can be used for the origin/source IP. WordPress can't "out of the box" handle everyone's situation perfectly, so it should be made easy to adjust:

The most flexible would be to switch to a get_ip() or wp_get_ip() function, then let users be able to override it with their own by creating a plugin:

function get_ip(){
  return $_SERVER['X-REAL-IP'];
}

Though not sure if that would happen early enough to be useful for init.

The other option is to leave a mapping option in wp-config.php with the default and let users change if needed. Something like:

// If your server sits behind a CDN or proxy you may need to adjust this
$real_ip = $_SERVER['REMOTE_ADDR']

The easiest fix I've seen right now is to simply overwrite $_SERVERREMOTE_ADDR? via wp-config.php, as REMOTE_ADDR is rarely if ever useful in this situation.

#13 @Denis-de-Bernardy
15 years ago

I like the idea of a mapping function in wp-config.php.

#14 @Denis-de-Bernardy
15 years ago

We could also use a define, actually. It would make things even easier. As in:

// add a WP_REMOTE_ADDR in your wp-config.php
if ( !defined('WP_REMOTE_ADDR') ) {
  // run our best guess as what the IP is
}

That way, users can override the WP procedure if it doesn't work for them.

#15 @robertaccettura
15 years ago

That would work as well, though I think this looks somewhat odd looks somewhat odd:

define('WP_REMOTE_ADDR', $_SERVER['X-REAL-IP']);


But that's likely because you don't normally assign predefined variables to constants.

Perhaps leave an example in wp-config.php commented out so that it's discoverable?

#16 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch tested commit added; needs-patch removed

#17 follow-up: @ryan
15 years ago

Hmm, should we even try to set REMOTE_ADDR if WP_REMOTE_ADDR is not defined? I don't think we can safely do anything aside from leaving REMOTE_ADDR alone.

#18 in reply to: ↑ 17 @robertaccettura
15 years ago

Replying to ryan:

Hmm, should we even try to set REMOTE_ADDR if WP_REMOTE_ADDR is not defined? I don't think we can safely do anything aside from leaving REMOTE_ADDR alone.

Agreed. Not to mention you shouldn't blindly trust HTTP_X_FORWARDED_FOR. This can cause trouble:

http://marc.info/?l=bugtraq&m=108239864203144&w=2

I don't think anything but REMOTE_ADDR should be done on it's own. Leave it up to the user to decide if they should be trusting an arbitrary header.

That said, it might be best to do some sort of validation if a non-remote_addr is used to ensure the response is sane. I think remote_addr is considered safe because it's calculated by PHP. Other arbitrary headers are not.

#19 @Denis-de-Bernardy
15 years ago

Amen to that. New patch sticks to adding a comment into the wp-config-sample.php file to highlight extra defines and potential IP address issues for people behind load balancers.

There isn't much point in adding a define if it's were not trying to fix anything: we might as well have users set the $_SERVER variable as needed.

#20 @robertaccettura
15 years ago

Just kinda freehanding... but maybe this can be simplified to handle things like TP_X_FORWARDED_FOR a little easier...

I just freehanded this for a proposed example... not saying this is perfect, or even sane... just putting ideas forward.

/**
 * If you're behind a load-balancer or CDN, you may want to set
 * WP_REMOTE_ADDR_HEADER to the correct header.  If comma delimited,
 * it will fetch the first IP address.
 */
define('WP_REMOTE_ADDR_HEADER', 'X-Real-IP');

/**
 * There are quite a few more defines for advanced users. See the wp-settings.php
 * file for details.
 */

/**
 * Fix remote address behind a load balancer
 *
 * If what follows doesn't work with your setup, configure the define in your wp-config.php file
 *
 * @since 2.9
 */
if( defined('WP_REMOTE_ADDR_HEADER') ) {
	if( isset($_SERVER[WP_REMOTE_ADDR_HEADER]) ){
		if( strpos($_SERVER[WP_REMOTE_ADDR_HEADER],',') !== false){
			$remote_ip = explode(',', $_SERVER[WP_REMOTE_ADDR_HEADER]);
 			$remote_ip = $remote_ip[0];
		} else {
                	$remote_ip = $_SERVER['WP_REMOTE_ADDR_HEADER'];
		}
		if( preg_match('/(\d+).(\d+).(\d+).(\d+)/',$remote_ip) ){
			$_SERVER['REMOTE_ADDR'] = $remote_ip;
		}
		unset($remote_ip)
	}
}

#21 @Denis-de-Bernardy
15 years ago

There's actually a separate potential issue if we try to fix this -- we'd potentially need to strip out local addresses. The bottom-line is, as was suggested further up, that we probably shouldn't even try to fix this -- but 9235.4.diff could come in handy to highlight the variable we're using.

#22 @Denis-de-Bernardy
15 years ago

@ryan - shall we get the extra comment in wp-config-sample.php, or close as wontfix?

#23 @Denis-de-Bernardy
15 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from accepted to assigned

#24 @dd32
15 years ago

+1 for including a check in core.. -1 for just having a comment.

This doesn't only affect load balances, It also affects extracting the correct IP when the web browser is behind a proxy.

Look at what other software which relies on IP addresses a bit more has done, for example, docuwiki: http://dev.splitbrain.org/reference/dokuwiki/inc/common.php.source.html#l591

Sure, Its a bit complex for what we need.. but it shows that if IP's are to be stored/mentioned (ie. comments), WordPress might as well get it right.

It can even be in a function such as that and only called when needed (ie. comment being added) to reduce server overheads, most page loads dont need the "real ip"

#25 @dd32
15 years ago

Of course, If something like this was included in core, The "proxies" ip would also have to be taken into consideration for spam checks and coment-flow-control, Else a client could continuously spoof a proxied header resulting in bypassing the comment-flow limits..

#26 @markjaquith
15 years ago

  • Resolution set to wontfix
  • Status changed from assigned to closed

Using another header for the real IP requires knowledge about how the server is setup. You'd need a whitelist of internal addresses so that people couldn't fool your system into accepting an incorrect IP address. Take a look at Apache's mod_rpaf:

http://stderr.net/apache/rpaf/

That's what I use to give Apache the correct IP address from nginx. But it's whitelisted so that if you do manage to connect to Apache directly, you can't fill in that header and have Apache use it.

I don't think that this belongs at the application level. Closing as WONTFIX, but if someone feels really strongly about this, reopen and make your case for why doing it at the app level is better than doing it at the HTTP server level.

#27 @Denis-de-Bernardy
15 years ago

  • Milestone 2.9 deleted

#28 @checkers
15 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I'd like to see support for this (probably as commented out code in wp-config.php) . Doing this at the application level is a Good Thing. An example of another webapp that does this is MediaWiki, which uses a somewhat obscure setting to control what source IPs are considered 'trusted proxies'.

I don't think this is a good thing to hand off to the http layer. mod_rpaf obliterates the real source IP, so while you can see what the client's address is, it's no longer possible to tell which of your upstream servers a request came from. It's possible some web servers don't even support this, I know Apache & nginx do but unsure about IIS.

#29 @westi
15 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

The correct solution here is very much server specific and not something to go into the core code.

Re-closing as WONTFIX

#30 @hpatoio
15 years ago

  • Cc hpatoio added
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Version changed from 2.7 to 2.9

@westi I do agree but how do I fix this without hacking the core ?
I think that at least a hook/filter should be added.

#31 @scribu
15 years ago

  • Keywords needs-patch added; has-patch tested commit early removed
  • Milestone set to 3.0

I agree that a filterable get_ip() function would be useful.

@scribu
15 years ago

Introduce wp_get_ip()

#32 @scribu
15 years ago

  • Keywords has-patch added; needs-patch removed

wp_get_ip.diff introduces wp_get_ip()

#33 @robertaccettura
15 years ago

I think that filtering is a good solution here. It would be trivial to then create a plugin for any load balancer/proxy.

#34 @nacin
15 years ago

  • Keywords commit added

Patch makes sense to me.

#35 @nacin
15 years ago

I would go with probably wp_remote_ip(). We should probably eat our dog food and use this in place of REMOTE_ADDR checks.

@scribu
15 years ago

#36 @scribu
15 years ago

See wp_remote_ip.diff.

wp-includes/comment.php is the only place where REMOTE_ADDR is used (that I could find).

#37 @nacin
15 years ago

Looks like it's also in ms-functions.php. (And also akismet, and referenced in simplepie and phpmailer.)

#38 follow-up: @westi
15 years ago

  • Keywords commit removed

Why do we need a filter?

A plugin can play with the REMOTE_ADDR superglobal on plugins_loaded - is that not early enough?

#39 in reply to: ↑ 38 @nacin
15 years ago

  • Milestone 3.0 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Replying to westi:

Why do we need a filter?

A plugin can play with the REMOTE_ADDR superglobal on plugins_loaded - is that not early enough?

I agree, I was just coming here to make the same comment. A plugin should be fixing the global.

#40 @nacin
14 years ago

Cross referencing [15560] as well as this potential snippet, good for wp-config.

if ( ! empty( $_SERVER['HTTP_X_FORWARDED_FOR'] ) && preg_match( '/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/', $_SERVER['HTTP_X_FORWARDED_FOR'] ) )
	$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR'];

Again, specific to your environment. You also may want to check for HTTP_X_FORWARDED_PROTO for HTTPS.

#41 @hakre
14 years ago

Related: #16932

#42 @archon810
13 years ago

  • Cc admin@… added

This is a pretty serious problem for those of us moving behind a load balancer, the comment anti-flood kind of creeps up on you. Ideally, Wordpress would notice 127.0.0.1 IPs and put up a little one-time alert in the admin interface and at a minimum provide an out-of-the-box wp-config solution, like the one nacin proposed (which worked great).

There is bad advice on the web (for example, modifying comments.php, and comment replies in that thread are closed), so if wp core can help, it'll be best for everyone.

@peetaur
11 years ago

patch for configurable proxy IP plus replacing for comments

#43 @peetaur
11 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I have solved the problem of being server specific. I have added a patch created with ideas from the above, plus an optional configuration setting. Now you just set the PROXY_IP config setting to a space separated list of proxies, and it will use HTTP_X_FORWARDED_FOR instead of REMOTE_ADDR it if it is in PROXY_IP, and the HTTP_X_FORWARDED_FOR header is set and non-empty.

I tested that the IP for the comment is correct when filled with my proxy (127.0.0.1 which is my apache2), or with blank (the default in config-sample.php), or with the PROXY_IP undefined (commented out).

#44 @SergeyBiryukov
11 years ago

  • Component changed from Optimization to General
  • Keywords close added
  • Milestone set to Awaiting Review

See [15560] and comment:39.

#45 @nacin
10 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

This PROXY_IP implementation isn't scalable — it's possible to have a significant and/or variable number of proxies. As covered previously I'd still like to leave this alone.

Note: See TracTickets for help on using tickets.