Make it easier to parse "X-Forwarded-For" with one or more load balancers

Summary:
Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.

We want to select the 17th-from-last element, since prior elements are not trustworthy.

This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier.

Test Plan:
  - Ran unit tests.
  - Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`.

```
$ curl http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR =
   FINAL REMOTE_ADDR = 127.0.0.1
</pre>
```

```
$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 3.3.3.3
</pre>
```

```
$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 5.5.5.5
</pre>
```

Maniphest Tasks: T13392

Differential Revision: https://secure.phabricator.com/D20785

src/__phutil_library_map__.php

@@ -4201,6 +4201,7 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     ),
     'PhabricatorPolicyType' => 'PhabricatorPolicyConstants',
     'PhabricatorPonderApplication' => 'PhabricatorApplication',
+    'PhabricatorPreambleTestCase' => 'PhabricatorTestCase',
     'PhabricatorPrimaryEmailUserLogType' => 'PhabricatorUserLogType',
     'PhabricatorProfileMenuEditEngine' => 'PhabricatorEditEngine',
     'PhabricatorProfileMenuEditor' => 'PhabricatorApplicationTransactionEditor',

src/docs/user/configuration/configuring_preamble.diviner

@@ -15,10 +15,9 @@ You can use a special preamble script to make arbitrary adjustments to the
 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 @@ If present, this script will be executed at the very beginning of each web
 request, allowing you to adjust the environment. For common adjustments and
 examples, see the next sections.
 
+
 Adjusting Client IPs
 ====================
 
@@ -44,9 +44,15 @@ If your install is behind a load balancer, Phabricator may incorrectly detect
 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
+In common cases where networks are configured like this, the `X-Forwarded-For`
-be trustworthy, you can read the header and overwrite the `REMOTE_ADDR` value
+header will have trustworthy information about the real client IP. You
-so Phabricator can figure out the client IP correctly.
+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 @@ directly, they can provide an arbitrary `X-Forwarded-For` header, and thereby
 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
+has been forwarded through multiple load balancers. If you know that requests
-will usually handle most situations correctly:
+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
-<?php
 
-// Overwrite REMOTE_ADDR with the value in the "X-Forwarded-For" HTTP header.
+If you have an unusual network configuration (for example, the number of
+trustworthy devices depends on the network path) you can also implement your
+own logic.
 
-// Only do this if you're certain the request is coming from a loadbalancer!
+Note that this is very odd, advanced, and easy to get wrong. If you get it
-// If the request came directly from a client, doing this will allow them to
+wrong, users will most likely be able to spoof any client address.
-// them spoof any remote address.
 
-// The header may contain a list of IPs, like "1.2.3.4, 4.5.6.7", if the
+```name="Custom X-Forwarded-For Handling", lang=php
-// request the load balancer received also had this header.
 
 if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
-  $forwarded_for = $_SERVER['HTTP_X_FORWARDED_FOR'];
+  $raw_header = $_SERVER['X_FORWARDED_FOR'];
-  if ($forwarded_for) {
+
-    $forwarded_for = explode(',', $forwarded_for);
+  $real_address = your_custom_parsing_function($raw_header);
-    $forwarded_for = end($forwarded_for);
+
-    $forwarded_for = trim($forwarded_for);
+  $_SERVER['REMOTE_ADDR'] = $raw_header;
-    $_SERVER['REMOTE_ADDR'] = $forwarded_for;
-  }
 }
 ```
 

src/infrastructure/env/PhabricatorEnv.php

@@ -135,6 +135,11 @@ final class PhabricatorEnv extends Phobject {
     // TODO: Add a "locale.default" config option once we have some reasonable
     // defaults which aren't silly nonsense.
     self::setLocaleCode('en_US');
+
+    // Load the preamble utility library if we haven't already. On web
+    // requests this loaded earlier, but we want to load it for non-web
+    // requests so that unit tests can call these functions.
+    require_once $phabricator_path.'/support/startup/preamble-utils.php';
   }
 
   public static function beginScopedLocale($locale_code) {

src/infrastructure/util/__tests__/PhabricatorPreambleTestCase.php

@@ -0,0 +1,74 @@
+<?php
+
+final class PhabricatorPreambleTestCase
+  extends PhabricatorTestCase {
+
+  /**
+   * @phutil-external-symbol function preamble_get_x_forwarded_for_address
+   */
+  public function testXForwardedForLayers() {
+    $tests = array(
+      // This is normal behavior with one load balancer.
+      array(
+        'header' => '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));
+    }
+  }
+
+}

support/startup/preamble-utils.php

@@ -0,0 +1,77 @@
+<?php
+
+/**
+ * Parse the "X_FORWARDED_FOR" HTTP header to determine the original client
+ * address.
+ *
+ * @param  int  Number of devices to trust.
+ * @return void
+ */
+function preamble_trust_x_forwarded_for_header($layers = 1) {
+  if (!is_int($layers) || ($layers < 1)) {
+    echo
+      'preamble_trust_x_forwarded_for_header(<layers>): '.
+      '"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;
+}

webroot/index.php

@@ -85,6 +85,7 @@ function phabricator_startup() {
   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);