Problem/Motivation

D7 had the ability to nominate an alternative HTTP header from which to extract the client IP e.g. several CDNs add a True-Client-IP header or similar:

https://git.drupalcode.org/project/drupal/-/blob/7.81/sites/default/defa...

D8/9 instead recommend this:

https://git.drupalcode.org/project/drupal/-/blob/9.2.1/sites/default/def...

 * In order for this setting to be used you must specify every possible
 * reverse proxy IP address in $settings['reverse_proxy_addresses'].
 * If a complete list of reverse proxies is not available in your
 * environment (for example, if you use a CDN) you may set the
 * $_SERVER['REMOTE_ADDR'] variable directly in settings.php.
 * Be aware, however, that it is likely that this would allow IP
 * address spoofing unless more advanced precautions are taken.

However, the Symfony Request has already been initialised by the time that settings.php is parsed, so it's too late to change the REMOTE_ADDR as the Request has its own copy of the relevant values from the _SERVER superglobal, from which it extracts the client IP.

Therefore, following the instructions in settings.php does not work.

Steps to reproduce

Assign a test value to $_SERVER['REMOTE_ADDR'] variable directly in settings.php per the instructions.

Observe that this value is not in the Request's server property, and will not be returned by e.g.

\Drupal::requestStack()->getCurrentRequest()->getClientIp();

(This module can be useful for debugging proxy headers etc..: https://www.drupal.org/project/trusted_proxy_headers_debug ).

I believe this is because (from index.php):

$kernel = new DrupalKernel('prod', $autoloader);

$request = Request::createFromGlobals();   <== the Request hoovers up values from $_SERVER
$response = $kernel->handle($request);     <== the Drupal Kernel parses settings.php here
$response->send();

$kernel->terminate($request, $response);

Proposed resolution

I'm not yet sure how a site should be configured to derive the Client IP from a value supplied by a CDN if the headers don't fit within what Symfony supports (e.g. FORWARDED / X-Forwarded-For etc..).

Remaining tasks

Figure out a solution.

User interface changes

n/a

API changes

?

Data model changes

n/a

Release notes snippet

tbc

Issue fork drupal-3223280

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mcdruid created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Symfony's docs suggest you should override HTTP_X_FORWARDED_* in index.php:

https://symfony.com/doc/4.4/deployment/proxies.html#custom-headers-when-...

Instead of doing this in PHP, you could also do this in the web server configuration, so the header gets translated correctly before PHP even sees it. This is still a docs bug though if it doesn't work as described.

mcdruid’s picture

mcdruid commented

https://www.drupal.org/project/reverse_proxy_header addresses this issue.

Looks like the developer / maintainer reached the same conclusion about settings.php being too late to manipulate header values in $_SERVER.

Should we do something about this in core other than just fixing the docs? What would we change the recommendation in the docs to?

It seems like a regression that D7 can do this cleanly (acknowledging the reason for the change is down to Symfony doing the heavy lifting here as opposed to core's own code).

willempje2’s picture

willempje2 commented

I think you should change the value the other way around.
Do not change $_SERVER['REMOTE_ADDR'] but use the remote REMOTE_ADDR to set the reverse_proxy_addresses.

Like so: $settings['reverse_proxy_addresses'] = [$_SERVER['REMOTE_ADDR']];

mcdruid’s picture

mcdruid commented

Doing something like this:

$settings['reverse_proxy_addresses'] = [$_SERVER['REMOTE_ADDR']];

...would often work okay I think, but it may not always work and could in fact be risky.

If just one proxy is involved in the CDN's handling of the request, the approach of adding the REMOTE_ADDR as a trusted proxy will probably work fine (this will be the CDN's IP), and Drupal will correctly derive the client IP from the X-Forwarded-For (XFF) header.

However, if more than one proxy is involved in the CDN's handling of the request (which is perhaps not typical, but I'd say not uncommon), doing this won't work.

Even worse, if it's possible to bypass the CDN and send requests directly to the origin (again not typical / ideal, but not unheard of in the real world), adding REMOTE_ADDR as a trusted proxy may open Drupal up to extracting a spoofed IP from the XFF header.

You could work around that by only adding REMOTE_ADDR as a trusted proxy if other conditions are satisfied (e.g. checking for other headers from the CDN). This is the sort of thing the existing comments probably mean when they say "this would allow IP address spoofing unless more advanced precautions are taken."

Generally this seems like a workaround, and a regression in terms of no longer being able to simply set up a configuration which says "this header from the CDN will contain the client IP".

mcdruid’s picture

mcdruid commented

Again this is a workaround rather than an elegant solution, but if the CDN is supplying a client IP in an alternative HTTP header, and we can only alter the list of trusted proxies... so long as that client IP appears in the X-Forwarded-For header, we can add any IPs to the right of client IP as trusted proxies (in addition to the IP in REMOTE_ADDR).

It's actually very similar to the idea of marking the REMOTE_ADDR IP as a trusted proxy, but covers more scenarios.

I've done some basic testing of this approach within settings.php and it seems to work.

Whether this is something we should consider adding to core, and if so how... I'm not certain.

manarth’s picture

manarth commented

To elaborate Drew's comment with code, here's an example implementation.

if (isset($_SERVER['HTTP_CF_CONNECTING_IP'])) {
  // If the CloudFlare header is contained in the X-Forwarded-For header, then
  // all IP addresses to the right of that entry are reverse-proxies, which are
  // additional to the value in $_SERVER['REMOTE_ADDR].
  // E.g. <client> --- <CDN> --- <Varnish> --- <drupal>.
  $client = $_SERVER['HTTP_CF_CONNECTING_IP'];
  $ips = explode(', ', $_SERVER['HTTP_X_FORWARDED_FOR']);
  if ($keys = array_keys($ips, $client)) {
    $position = end($keys);
    $reverseProxies = array_slice($ips, $position + 1);
    $reverseProxies[] = $_SERVER['REMOTE_ADDR'];

    $settings['reverse_proxy'] = TRUE;
    $settings['reverse_proxy_addresses'] = $reverseProxies;
  }
}

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

deviantintegral’s picture

deviantintegral commented

I just got bit by the incorrect documentation on setting $_SERVER['REMOTE_ADDR'].

Given there are various workarounds, and https://www.drupal.org/project/reverse_proxy_header exists, I agree we should just fix the docs on this.

deviantintegral opened merge request !3218

deviantintegral’s picture

deviantintegral commented

I've opened an MR with a first pass at correcting the docs.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs work
deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
deviantintegral’s picture

deviantintegral commented

The only existing docs I found are at https://www.drupal.org/node/425990 but it's also in a "To be migrated" section: https://www.drupal.org/to-be-migrated.

I imagine when docs are moved, redirects are created, so it's probably safe to link there and update it?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Reviewed & tested by the community

Please lets not hold up useful doc changes for trivial improvements.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

We can add short note to https://www.drupal.org/node/425990 and link to that. It would have taken very little time for someone to do that or just confirm #17 was OK, instead this has been held up further by status pong.

sboto’s picture

sboto commented

Please merge the merge request of 'deviantintegral' after all this time the wrong comment still exists.
https://git.drupalcode.org/project/drupal/-/merge_requests/3218

aliartos’s picture

aliartos commented

Based on https://symfony.com/doc/current/deployment/proxies.html we can add a proposed configuration:
$settings['reverse_proxy_addresses'] = ['127.0.0.1','REMOTE_ADDR'];

mcdruid’s picture

mcdruid commented

Adding REMOTE_ADDR as a trusted proxy may work in some cases, but it can also cause problems such as making it possible to spoof the client IP via X-Forwarded-For (per #5 and the existing cautionary note in the comments).

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.