diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4201,6 +4201,7 @@ 'PhabricatorPolicyTestObject' => 'applications/policy/__tests__/PhabricatorPolicyTestObject.php', 'PhabricatorPolicyType' => 'applications/policy/constants/PhabricatorPolicyType.php', 'PhabricatorPonderApplication' => 'applications/ponder/application/PhabricatorPonderApplication.php', + 'PhabricatorPreambleTestCase' => 'infrastructure/util/__tests__/PhabricatorPreambleTestCase.php', 'PhabricatorPrimaryEmailUserLogType' => 'applications/people/userlog/PhabricatorPrimaryEmailUserLogType.php', 'PhabricatorProfileMenuEditEngine' => 'applications/search/editor/PhabricatorProfileMenuEditEngine.php', 'PhabricatorProfileMenuEditor' => 'applications/search/editor/PhabricatorProfileMenuEditor.php', @@ -10681,6 +10682,7 @@ ), 'PhabricatorPolicyType' => 'PhabricatorPolicyConstants', 'PhabricatorPonderApplication' => 'PhabricatorApplication', + 'PhabricatorPreambleTestCase' => 'PhabricatorTestCase', 'PhabricatorPrimaryEmailUserLogType' => 'PhabricatorUserLogType', 'PhabricatorProfileMenuEditEngine' => 'PhabricatorEditEngine', 'PhabricatorProfileMenuEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/docs/user/configuration/configuring_preamble.diviner b/src/docs/user/configuration/configuring_preamble.diviner --- a/src/docs/user/configuration/configuring_preamble.diviner +++ b/src/docs/user/configuration/configuring_preamble.diviner @@ -15,10 +15,9 @@ environment and some parts of Phabricator's configuration in order to fix these problems and set up the environment which Phabricator expects. -NOTE: This is an advanced feature. Most installs should not need to configure -a preamble script. -= Creating a Preamble Script = +Creating a Preamble Script +========================== To create a preamble script, write a file to: @@ -37,6 +36,7 @@ request, allowing you to adjust the environment. For common adjustments and examples, see the next sections. + Adjusting Client IPs ==================== @@ -44,9 +44,15 @@ all requests as originating from the load balancer, rather than from the correct client IPs. -If this is the case and some other header (like `X-Forwarded-For`) is known to -be trustworthy, you can read the header and overwrite the `REMOTE_ADDR` value -so Phabricator can figure out the client IP correctly. +In common cases where networks are configured like this, the `X-Forwarded-For` +header will have trustworthy information about the real client IP. You +can use the function `preamble_trust_x_forwarded_for_header()` in your +preamble to tell Phabricator that you expect to receive requests from a +load balancer or proxy which modifies this header: + +```name="Trust X-Forwarded-For Header", lang=php +preamble_trust_x_forwarded_for_header(); +``` You should do this //only// if the `X-Forwarded-For` header is known to be trustworthy. In particular, if users can make requests to the web server @@ -54,30 +60,29 @@ spoof an arbitrary client IP. The `X-Forwarded-For` header may also contain a list of addresses if a request -has been forwarded through multiple loadbalancers. Using a snippet like this -will usually handle most situations correctly: +has been forwarded through multiple load balancers. If you know that requests +on your network are routed through `N` trustworthy devices, you can specify +that `N` to tell the function how many layers of `X-Forwarded-For` to discard: +```name="Trust X-Forwarded-For Header, Multiple Layers", lang=php +preamble_trust_x_forwarded_for_header(3); ``` -name=Overwrite REMOTE_ADDR with X-Forwarded-For - '1.2.3.4', + 'layers' => 1, + 'expect' => '1.2.3.4', + ), + + // In this case, the LB received a request which already had an + // "X-Forwarded-For" header. This might be legitimate (in the case of + // a CDN request) or illegitimate (in the case of a client making + // things up). We don't want to trust it. + array( + 'header' => '9.9.9.9, 1.2.3.4', + 'layers' => 1, + 'expect' => '1.2.3.4', + ), + + // Multiple layers of load balancers. + array( + 'header' => '9.9.9.9, 1.2.3.4', + 'layers' => 2, + 'expect' => '9.9.9.9', + ), + + // Multiple layers of load balancers, plus a client-supplied value. + array( + 'header' => '8.8.8.8, 9.9.9.9, 1.2.3.4', + 'layers' => 2, + 'expect' => '9.9.9.9', + ), + + // Multiple layers of load balancers, but this request came from + // somewhere inside the network. + array( + 'header' => '1.2.3.4', + 'layers' => 2, + 'expect' => '1.2.3.4', + ), + + array( + 'header' => 'A, B, C, D, E, F, G, H, I', + 'layers' => 7, + 'expect' => 'C', + ), + ); + + foreach ($tests as $test) { + $header = $test['header']; + $layers = $test['layers']; + $expect = $test['expect']; + + $actual = preamble_get_x_forwarded_for_address($header, $layers); + + $this->assertEqual( + $expect, + $actual, + pht( + 'Address after stripping %d layers from: %s', + $layers, + $header)); + } + } + +} diff --git a/support/startup/preamble-utils.php b/support/startup/preamble-utils.php new file mode 100644 --- /dev/null +++ b/support/startup/preamble-utils.php @@ -0,0 +1,77 @@ +): '. + '"layers" parameter must an integer larger than 0.'."\n"; + echo "\n"; + exit(1); + } + + if (!isset($_SERVER['HTTP_X_FORWARDED_FOR'])) { + return; + } + + $forwarded_for = $_SERVER['HTTP_X_FORWARDED_FOR']; + if (!strlen($forwarded_for)) { + return; + } + + $address = preamble_get_x_forwarded_for_address($forwarded_for, $layers); + + $_SERVER['REMOTE_ADDR'] = $address; +} + +function preamble_get_x_forwarded_for_address($raw_header, $layers) { + // The raw header may be a list of IPs, like "1.2.3.4, 4.5.6.7", if the + // request the load balancer received also had this header. In particular, + // this happens routinely with requests received through a CDN, but can also + // happen illegitimately if the client just makes up an "X-Forwarded-For" + // header full of lies. + + // We can only trust the N elements at the end of the list which correspond + // to network-adjacent devices we control. Usually, we're behind a single + // load balancer and "N" is 1, so we want to take the last element in the + // list. + + // In some cases, "N" may be more than 1, if the network is configured so + // that that requests are routed through multiple layers of load balancers + // and proxies. In this case, we want to take the Nth-to-last element of + // the list. + + $addresses = explode(',', $raw_header); + + // If we have more than one trustworthy device on the network path, discard + // corresponding elements from the list. For example, if we have 7 devices, + // we want to discard the last 6 elements of the list. + + // The final device address does not appear in the list, since devices do + // not append their own addresses to "X-Forwarded-For". + + $discard_addresses = ($layers - 1); + + // However, we don't want to throw away all of the addresses. Some requests + // may originate from within the network, and may thus not have as many + // addresses as we expect. If we have fewer addresses than trustworthy + // devices, discard all but one address. + + $max_discard = (count($addresses) - 1); + + $discard_count = min($discard_addresses, $max_discard); + if ($discard_count) { + $addresses = array_slice($addresses, 0, -$discard_count); + } + + $original_address = end($addresses); + $original_address = trim($original_address); + + return $original_address; +} diff --git a/webroot/index.php b/webroot/index.php --- a/webroot/index.php +++ b/webroot/index.php @@ -85,6 +85,7 @@ require_once $root.'/support/startup/PhabricatorClientLimit.php'; require_once $root.'/support/startup/PhabricatorClientRateLimit.php'; require_once $root.'/support/startup/PhabricatorClientConnectionLimit.php'; + require_once $root.'/support/startup/preamble-utils.php'; // If the preamble script exists, load it. $t_preamble = microtime(true);