WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 6 weeks ago

#9235 reopened enhancement

Extract real IP behind a load balancer

Reported by: Denis-de-Bernardy Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: has-patch close
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 5 years ago.
9235.2.diff (1.8 KB) - added by Denis-de-Bernardy 5 years ago.
9235.3.diff (1.1 KB) - added by Denis-de-Bernardy 5 years ago.
9235.4.diff (590 bytes) - added by Denis-de-Bernardy 5 years ago.
wp_get_ip.diff (1.4 KB) - added by scribu 4 years ago.
Introduce wp_get_ip()
wp_remote_ip.diff (1.4 KB) - added by scribu 4 years ago.
get_remote_ip.patch (2.2 KB) - added by peetaur 6 weeks ago.
patch for configurable proxy IP plus replacing for comments

Download all attachments as: .zip

Change History (51)

comment:1 Denis-de-Bernardy5 years ago

  • Keywords has-patch added

comment:2 johnbillion5 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?

comment:3 Denis-de-Bernardy5 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.

Denis-de-Bernardy5 years ago

comment:4 Denis-de-Bernardy5 years ago

  • Keywords commit added; dev-feedback removed

comment:6 Denis-de-Bernardy5 years ago

  • Component changed from General to Optimization

comment:7 ryan5 years ago

HTTP_X_FORWARDED_FOR can be a comma separated list.

Denis-de-Bernardy5 years ago

comment:8 Denis-de-Bernardy5 years ago

new patch should deal with this fine.

comment:9 Denis-de-Bernardy5 years ago

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

comment:10 ryan5 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.

comment:11 Denis-de-Bernardy5 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.

comment:12 robertaccettura5 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.

comment:13 Denis-de-Bernardy5 years ago

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

comment:14 Denis-de-Bernardy5 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.

comment:15 robertaccettura5 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?

Denis-de-Bernardy5 years ago

comment:16 Denis-de-Bernardy5 years ago

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

comment:17 follow-up: ryan5 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.

comment:18 in reply to: ↑ 17 robertaccettura5 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.

Denis-de-Bernardy5 years ago

comment:19 Denis-de-Bernardy5 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.

comment:20 robertaccettura5 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)
	}
}

comment:21 Denis-de-Bernardy5 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.

comment:22 Denis-de-Bernardy5 years ago

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

comment:23 Denis-de-Bernardy5 years ago

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

comment:24 dd325 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"

comment:25 dd325 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..

comment:26 markjaquith5 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.

comment:27 Denis-de-Bernardy5 years ago

  • Milestone 2.9 deleted

comment:28 checkers5 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.

comment:29 westi5 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

comment:30 hpatoio4 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.

comment:31 scribu4 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.

scribu4 years ago

Introduce wp_get_ip()

comment:32 scribu4 years ago

  • Keywords has-patch added; needs-patch removed

wp_get_ip.diff introduces wp_get_ip()

comment:33 robertaccettura4 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.

comment:34 nacin4 years ago

  • Keywords commit added

Patch makes sense to me.

comment:35 nacin4 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.

scribu4 years ago

comment:36 scribu4 years ago

See wp_remote_ip.diff.

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

comment:37 nacin4 years ago

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

comment:38 follow-up: westi4 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?

comment:39 in reply to: ↑ 38 nacin4 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.

comment:40 nacin4 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.

comment:41 hakre3 years ago

Related: #16932

comment:42 archon8102 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.

peetaur6 weeks ago

patch for configurable proxy IP plus replacing for comments

comment:43 peetaur6 weeks 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).

comment:44 SergeyBiryukov6 weeks ago

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

See [15560] and comment:39.

Note: See TracTickets for help on using tickets.