When the trusted proxies are set and reverse_proxy is enabled, Symfony will automatically pickup the default X-Forwarded-* headers. However, Drupal only allows you to change the X-Forwarded-For header. Drupal does not support changing the name of any other header in the X-Forwarded family.

Taking, for instance, the X-Forwarded-Proto (XFP) header, which is included as part of the request sent to the back-end web server by many crypto-offloaders and load balancer. This header allows the load-balanced/crypto-offloaded application to know the protocol used by the client to reach the load balancer, allowing the application to generate links with the correct protocol. However, some systems use different headers for the same purpose. An example is the "Microsoft Internet Security and Acceleration Server", which uses the Front-End-Https for similar purposes.

Benefit:

Without a correct scheme/protocol, various things break -- most visibly the stylesheets and javascripts, AJAX calls and the Drupal installer, because callback URLs are built on an incorrect scheme.

Cost:

The patch only introduces extra function calls for users already using a reverse proxy. Since that is not the case for most users, there is no cost for them.

Note:

It is possible to set $base_root manually in settings.php. But using this patch will allow Drupal to work in this sort of environment as it is already documented, without special care being taken to create settings.php ahead of time.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this functionality is present in symfony, but Drupal only allows the client ip header to be changed. It also affects the usability of Drupal, as Drupal doesn't work with some crypto offloaders.
Issue priority Major because this prevents all crypto offloaders/load balancers not using the de-facto standard X-Forwarded headers from working with Drupal.
Prioritized changes The main goal of this issue is to improve the usability of Drupal from a system administrator point of view, by improving the support for of crypto offloaders.

Manual testing steps

If you want to test this patch manually, without setting up Varnish, you must:

  1. Set up your webserver so it can serve requests over HTTPS.
  2. Set up your webserver so it adds an X-Forwarded-Proto header set to the value "https". In Apache, this looks like: RequestHeader set X-Forwarded-Proto "https"
  3. Ensure your webserver is using the new configuration (e.g.: sudo /etc/init.d/apache2 reload).
  4. Optionally, perform a fresh install of Drupal. Otherwise, have a working copy of Drupal.
  5. In settings.php, ensure the #$base_url = 'http://example.com"; line is commented.
  6. In settings.php, un-comment the $conf['reverse_proxy'] = TRUE; line.
  7. In settings.php, un-comment the $conf['reverse_proxy_addresses'] = array(); line and add your client IP to the array (most likely 127.0.0.1).
  8. Flush all Drupal caches (e.g.: drush -y cc all).
  9. Go to your example site over the HTTP protocol (e.g.: http://example.com). View the source.
    • If the patch is applied and working correctly, all your stylesheets, scripts, etc. will use the https protocol.
      Note that most links on the front page of a default Drupal installation are generated relative to the server root (e.g.: <a href="/user/login"></a>) so clicking on them will not generally take you to the HTTPS version (you'll need the Secure Login or Secure Pages modules for that).
    • If the patch is not applied, or does not work correctly, all your stylesheets, scripts, etc. will use the http protocol.

Comments

earnie’s picture

The patch looks good to me but I can't test it.

Bart Jansens’s picture

Status:Needs review» Needs work
<?php
+    elseif (isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) {
+     
// We're behind a proxy that talks to the web server via HTTP.
+      $base_root = $_SERVER['HTTP_X_FORWARDED_PROTO'];
+    }
?>

X-Forwarded-Proto could potentially be set by a user, check if it is really "http" or "https" and not something malicious.

Also, we have a "reverse_proxy" setting. It would be logical to only attempt protocol detection when the admin enabled reverse proxy support.

ghoti’s picture

Good point about the security issue; I'll think about that. Would it be too costly to compare against a regexp, or would it be better merely to use a few IFs comparing simple strings? I'm highly cognizant of the fact that this is in bootstrap.inc and will be executed for every hit.

As for having the admin enable something ... how does that help the person installing for the first time? There is no admin option until that has completed.

How could trusting X-Forwarded-Proto to reflect proxy requirement represent a security risk? I'm trying to imagine a way, and I can't come up with a mechanism.

ghoti’s picture

Status:Needs work» Needs review
StatusFileSize
new1.45 KB
Failed: Failed to install HEAD.
[ View ]

Patch updated to foil malicious header padding and respect $conf['reverse_proxy'] if it is set. This should allow the header to be respected during install.

I haven't actually tested this yet....

c960657’s picture

You should use variable_get() rather than accessing $conf directly.

If you check for reverse_proxy with X-Forwarded-Proto, you should check it for Front-End-Https as well. I cannot see a security risk in trusting X-Forwarded-Proto either, but when it comes to security-related stuff it is generally better to be safe than sorry. Checking reverse_proxy takes about 0.00 seconds and IMO even makes the code slightly easier to read.

What if the user talks HTTP with the reverse proxy, but the reverse proxy talks HTTPS with the webserver? AFAICT this isn't supported by this patch - is that in any way relevant?

ghoti’s picture

StatusFileSize
new1.52 KB
Failed: Failed to apply patch.
[ View ]
You should use variable_get() rather than accessing $conf directly.

I'd like to, but variable_get() requires a default, and can't be used to see *whether* a variable has been set. Except perhaps through inelegant use of $default, which I'd like to avoid. Note that the changes this patch applies are to function conf_init(), which is the thing that actually *reads* settings.php and sets the $conf global in the first place. So we're guaranteed to have $conf close-by. :)

you should check it for Front-End-Https as well.

You're absolutely right. Thanks! I've updated the patch.

What if the user talks HTTP with the reverse proxy, but the reverse proxy talks HTTPS with the webserver?

I don't think this is an issue. I can't see why you'd set that up in real life, and I know it's not even possible using Pound or Varnish. Pound does not support back-end HTTPS, and Varnish has no HTTPS support anywhere (one of the ways Varnish docs suggest for adding HTTPS support is to put it behind Pound).

ghoti’s picture

StatusFileSize
new1.77 KB
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]

Meh, wrong diff format. :)

Anonymous’s picture

Status:Needs review» Needs work

The last submitted patch failed testing.

seanr’s picture

I'll be able to start testing this and am very interested in it. Won't be right away (awaiting PHP 5.2 upgrade in progress), though, so just subscribing for the time being.

Berdir’s picture

We used F5 BIG-IP where I worked before (not together with Drupal). I did some research and it seems that BIG-IP does not send such a header by default but can easily be configured to send any header you want. So it should be fine if we document which headers are supported.

ghoti’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
Failed: 11815 passes, 18 fails, 1 exception
[ View ]

Updated for current HEAD

cburschka’s picture

Status:Needs review» Needs work

Inline comment: I looked "spoor" up in the dictionary, since I wasn't sure what it meant (even though it's Dutch and I speak German). I even first googled to see if it was a specific term associated with HTTPS. The dictionary says that, in English, the word is chiefly used for animal droppings. Even if it can mean a generic trail, surely it would be better to use a word that doesn't make developers reach for the dictionary? ;)

ghoti’s picture

Arancaytar,

http://drupal.org/coding-standards#comment seems to contain no restrictions on language. Are you suggesting that being imprecise is preferable if it keeps the language more simple? "Trail" is a path. "Spoor" is an indicator of something's existence or presence.

If there are criteria for selecting which English words are appropriate for use in PHP comments, I'll be happy to follow them. If not, and if nobody else objects, I'll move this back to "needs review".

Damien Tournoud’s picture

<?php
if (!isset($conf['reverse_proxy']) || $conf['reverse_proxy'] === TRUE)
?>

Erm? That condition makes no sense. It should probably be:

<?php
if (isset($conf['reverse_proxy']) && $conf['reverse_proxy'])
?>

We don't want to use the block of code below if reverse_proxy is not set.

The whole block of code could be simplified this way:

<?php
+    if (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') {
+     
// We're running HTTPS natively in the web server.
+      $base_root = 'https';
+    }
+    elseif (isset(
$conf['reverse_proxy']) && $conf['reverse_proxy']) {
+     
// Only trust this header if reverse_proxy is on or unset.  Note that
+      // this header is provided by the client and therefore can't be trusted.
+      if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == "https") {
+       
// We appear to be behind a proxy.
+        $base_root = "https";
+      }
+      elseif (isset(
$_SERVER['HTTP_FRONT_END_HTTPS']) && $_SERVER['HTTP_FRONT_END_HTTPS'] == 'on') {
+       
// The proxy follows the Microsoft convention for passing protocol
+        // information back to the web server per MS KB document Q307347.
+        $base_root = 'https';
+      }
+    }
+
+    if (!isset(
$base_root)) {
+     
// Defaults to HTTP if we have not found a clue that this connection is using HTTPS.
+      $base_root = 'http';
+    }
?>
ghoti’s picture

Damien, thanks for your input.

Note that the original purpose of this patch was to make it possible to install Drupal on HTTP web servers behind an HTTPS reverse proxy. I'm suggesting that we trust $_SERVER['HTTP_X_FORWARDED_PROTO'] if reverse_proxy is TRUE, or unset.

Your suggestion has an entirely different meaning, and will fail on a new installation where reverse_proxy has not been set because settings.php does not yet exist. If you're going to reverse the logic, you need to update the comments accordingly and provide another "else" that deals with cases where reverse_proxy is unset. If an administrator wants to "protect" their server from malicious HTTP headers, they can set $reverse_proxy in settings.php. (Perhaps proxy detection as part of the setup would also be useful, but that would be a different patch.)

Also, while removing the embedded elses makes the code easier to read, adding an isset() to the bottom will add a few ms of processing time to each hit. A faster approach might be to set $base_root to its default before the set of ifs, then overwrite its value, but I have the sense that overwriting variables is a little lazy, so I tend to avoid it.

What are your thoughts? Do we mind the extra CPU for an isset() on each hit? Seems like an avoidable waste to me. I haven't done any real performance testing to determine just how much extra CPU it would use.

Damien Tournoud’s picture

1. We don't trust HTTP headers. Never. You have to create the settings.php file before the installation, so you should be able to set $conf['reverse_proxy'] there if you want to.

2. While the performance difference is not measurable, setting the default before the big if blocks makes sense. So please go ahead the way you are suggesting.

ghoti’s picture

Damien,

We "trust" that the "Host:" header is correct, because it provides vital information regarding what web site is to be served. How is this any different? As far as I can tell, a forged "Host:" line puts us at just as much risk as a forged "X-Forwarded-Proto:" line. The only possible impact of adding that line on a server without HTTP is that AJAX callbacks and other functions that required URLs to be generated will stop working.

Per discussion above, nobody has suggested a way that a forged header might cause a problem or vulnerability, but without knowledge of the front-end protocol on a load balancer, all Drupal's AJAX callbacks, including those in the installer, will fail. If the community has decided that we WILL NOT support HTTPS-only installation behind load balancers, it should be documented somewhere. My impression over the last few years is that Drupal is leaning towards support of larger scale, enterprise environments.

Damien Tournoud’s picture

@ghoti: Nothing prevent you from configuring your settings.php properly before launching the installation process. You already have to copy default.settings.php to settings.php anyway.

And no, we obviously don't trust the Host header.

ghoti’s picture

Actually, if the directory is writable, Drupal will take care of writing a settings file by itself. For a single-site installation, this is the quickest and easiest way to get things running. IIRC, this change was made in response to complaints that Drupal was too difficult or cumbersome to install, especially for those who aren't comfortable editing .php files.

Would you be comfortable if we trusted a syntactically valid X-Forwarded-Proto header only in cases where settings.php does not already exist? Then at least the install would not fail.

As for the Host header ... we USE the Host header, and we certainly trust it to tell us what web site is being requested, because that's the only place we can get such information via HTTP. As long as it passes validation checks, we trust its content. If we didn't trust Host in this way, we'd have to go back to numbered virtualhosts. All I'm suggesting is that we do the same thing for another line in the header which is also the only place where certain other vital information is located.

ghoti’s picture

Status:Needs work» Needs review
Issue tags:+reverse proxy, +https
StatusFileSize
new1.55 KB
Failed: Failed to apply patch.
[ View ]

Here's an update which implements Damien's suggestion to tighten up the code. It certainly is easier to read!

The previous version trusted the X-Forwarded-Proto header if 'reverse_proxy' was TRUE or unset.

This version trusts the header only if 'reverse_proxy' is unset due to the non-existence of settings.php, as mentioned in #19. This means that a completed installation has a 'reverse_proxy' that defaults to FALSE, and new installations will not fail.

ghoti’s picture

StatusFileSize
new1.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap.inc_313145_1.patch.
[ View ]

Slightly refactored version of the previous patch. This one checks for 'reverse_proxy' before checking for settings.php, in the hope that we can avoid accessing the filesystem more often than necessary.

hass’s picture

+

hass’s picture

Status:Needs review» Needs work

Only for code readability I would move the $base_root = "http"; // default to "else {}". Additionally // default is not the way how we write core documentation. Also do not surround a string with double quotes if not absolutely required. if ( (is... - this blank is not core code style... remove the blank, please.

seanr’s picture

Status:Needs work» Needs review
StatusFileSize
new2.07 KB
Failed: 12845 passes, 2 fails, 0 exceptions
[ View ]

I took the liberty of re-rolling the patch with your recommendations. Please see attached.

Status:Needs review» Needs work

The last submitted patch failed testing.

seanr’s picture

Not sure why that's failing - nothing has really changed from the previous patch which succeeded. When I ran the tests on my own server I get "Login form action is secure" in session.test line 303, and "Page has been cached" in bootstrap.test line 266, but I can't figure out why either one is failing. The login form seems to work fine and I stay on HTTPS, so what else is it expecting? And I have no clue about the other one.

bjaspan’s picture

subscribe

pwolanin’s picture

We require settings.php to exist in order to install Drupal - so this part of the patch seems wrong:

+    elseif (!file_exists(DRUPAL_ROOT . '/' . conf_path() . '/settings.php')) {
pwolanin’s picture

@DamZ - unless the installer has changed - we actually overwrite all contents of the original settings.php - in fact you can just you touch to make an empty file, you don't need to copy the default.settings.php.

Still - you can install with a customized settings.php by manually populating the database settings - I did it recently for D7 behind a reverse-proxy, since with the request going across multiple web nodes, unless you have you docroot in a netowrk file system you can't have Drupal write the settings anyhow.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
Passed: 14713 passes, 0 fails, 0 exceptions
[ View ]

why does it need to be more than this?

pwolanin’s picture

I can imagine some possible security issues with this - e.g. if you are trying to force all authenticated traffic (e.g. admins) over SSL, but allow non-SSL for anonymous users for performance reasons, this header in combination with a CSRF attack might be able to expose via non-SSL the session cookie for the user. If you are behind a reverse proxy it seems like http://www.php.net/manual/en/session.configuration.php#ini.session.cooki... could not be used, so you'd have to be relying on this header (for example).

A bit of an edge case, but worth considering. Clearly the easiest way to prevent it is to block these headers at the reverse proxy.

pwolanin’s picture

any reviews?

seanr’s picture

I can't test it on the server I actually need it on since we haven't been able to get php5.2 installed for Drupal 7 (fark RedHat!), but on my personal server (no reverse proxy) it seems to run just fine with this patch.

ghoti’s picture

#21: bootstrap.inc_313145.patch queued for re-testing.

ghoti’s picture

pwolanin, with your patch, how will bootstrap.inc know that a web server is running behind a HTTPS reverse proxy?

Note the original purpose of this issue: "The attached patch adds X-Forwarded-Proto and Front-End-Https support to Drupal."

Until a configuration has been created (i.e. during initial installation), variable_get() will not return accurate data. That's why in http://drupal.org/node/313145#comment-1709094 I had the patch check $conf itself rather than use the function with a default.

(I'm still trying to figure out why that patch failed...)

pwolanin’s picture

@ghoti - you have to manually wset it via settings.php - as you always do.

ghoti’s picture

@pwolanin, the original point of this issue was to make it possible to install Drupal without pre-setting any variables in settings.php.

The security issues that were brought up were due to the concern that client HTTP headers could not be trusted, and that by basing our protocol on the available headers we might introduce an unknown risk.

I still think the patch at #21 is on-target for that goal, while introducing negligible risk.

Your simplification of most of this in #30 is great, but would you consider it unduly risky to support fresh Drupal installations by "believing" X-Forwarded-Proto headers if settings.php hasn't yet been created?

ghoti’s picture

Priority:Normal» Major
StatusFileSize
new738 bytes
PASSED: [[SimpleTest]]: [MySQL] 23,288 pass(es).
[ View ]

Re-cut for today's -dev.

Note that this is now a one-liner, since the previous performance concerns I had about checking for the existence of settings.php at each hit is moot, since we're now doing it anyway.

The "security risk" of trusting this header are not addressed in this patch because:

  1. filtering non-legitimate headers at the proxy is the responsibility of the proxy administrator,
  2. adding this header can only reduce availability of content, notwithstanding CSRF attacks, and
  3. if we trust this header only when settings.php does not exist, we need also to modify install.inc so that that the rest of the install (after settings.php is created) will work. That makes this a much bigger patch....
c960657’s picture

@pwolanin, the original point of this issue was to make it possible to install Drupal without pre-setting any variables in settings.php.

Is it really something people actually do, i.e. install Drupal from scratch using the installer on a server that is behind a reverse proxy that does HTTPS-HTTP mapping? That doesn't sound like the typical development environment but rather like a place where you deploy your site afterwords.

Damien Tournoud’s picture

Status:Needs review» Needs work

This is won't fix except if this is governed by an hidden variable that defaults to FALSE.

ghoti’s picture

c960657, I've installed quite a few sites that way. And I certainly believe that there are IT managers and webmasters using large-scale server environments who may as yet be unfamiliar with Drupal but might wish to install it using the web UI method. Wouldn't you expect things to "just work", rather than just fail?

Damien, per our discussion, I'll work on the patch to add reverse_proxy to settings.php if proxy indicators exist at install time.

I may be asking for help. :-)

ghoti’s picture

Status:Needs work» Needs review
StatusFileSize
new3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 24,765 pass(es).
[ View ]

Here's the latest incarnation.

Per discussion with Damien, we continue to by-default assume client-supplied headers are untrustworthy, but that if reverse_proxy is on, headers are received from the proxy, and are therefore trustworthy.

Then, in order to make setup work, I've added a line to the install.php initial summary showing proxy status, and told install.core.inc to set the reverse_proxy variable if it sees proxy indicators during setup. I've included a link to additional documentation, which I'll write well before D7 is released.

Last thing I've done is to remove the function drupal_detect_baseurl() from install.inc, as it's not being (and should not be) used.

ghoti’s picture

StatusFileSize
new3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 24,759 pass(es).
[ View ]

It turns out, I *did* start writing this documentation already.

The attached file is identical to the previous one except that it points to existing documentation, which I will expand.

seanr’s picture

Can you do this as a unified diff? cvs diff -up > example.patch See here for more info: http://drupal.org/patch/create (scroll down to "Patch Readability"). Thanks.

ghoti’s picture

StatusFileSize
new4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 24,807 pass(es).
[ View ]

Here it is, including the link to http://drupal.org/node/425990 .

pwolanin’s picture

Status:Needs review» Needs work

Several code style issues, and at a "XXX" code comment that looks like it's missing it s intended content.

ghoti’s picture

Several code style issues, and at a "XXX" code comment that looks like it's missing it s intended content.

Ah, I see the XXX - yes, that was just in for debugging; no code is missing. I'll remove it.

Can you point out the style issues, or how I might detect them? I believed I was following our coding style fairly well, based on what I read at http://drupal.org/coding-standards . Aside from the debugging line which I'll remove, what did I miss?

Thanks for your help.

ghoti’s picture

Status:Needs work» Needs review
StatusFileSize
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 24,819 pass(es).
[ View ]

Debug line removed.

seanr’s picture

Just tested #48, doesn't look like the reverse proxy variable was set in settings.php where I expected it. Is there somewhere else I should look to determine that it worked correctly? Nothing broke, but I can't tell whether it actually did what it was supposed to.

seanr’s picture

It furthermore is getting the load balancer IP in the watchdog instead of the correct one. Looks like this isn't actually doing anything.

sun.core’s picture

Category:bug» task
Status:Needs review» Needs work

Sounds like 1) needs work and 2) a task (actually a feature request).

george@dynapres.nl’s picture

Please note that Squid sets the HTTP_FRONT_END_HTTPS variable to "On" instead of "on", so the

$_SERVER['HTTP_FRONT_END_HTTPS'] == 'on'

should be case-insensitive.

See: Squid cache_peer settings.

pwolanin’s picture

@george - we are already using strtolower() in the patch, probably for that reason.

@seanr - what in the patch do you expect to be adding something to settings.php?

neclimdul’s picture

@pwolanin actually, it looks like that patch forgets to wrap HTTP_FRONT_END_HTTPS in a strtolower. And I assume seanr is referring to the $conf['reverse_proxy'] = TRUE; to the settings file if you're behind a proxy. Personally, I see support as more important than seamless install but that's a nice to have I'm sure.

Couple feedback points:
- There seem to be extra spaces around parenthesis in if statements.
- drupal_detect_baseurl is removed but I don't see why. Its lack of use seems a separate issue.
- $_SERVER['HTTP_FRONT_END_HTTPS'] isn't wrapped in strtolower as mentioned above.
- Seems like we could wrap the "from proxy protocol" logic up into one function and call that so we don't have this giant string of compares spread out and repeated all over the place. Maybe I'm wrong though.

dave_robinson’s picture

Are there any risks in also having this set $_SERVER['HTTPS'] to 'on'?
The patches in this issue seem to work for us ( Squid terminating SSL and forwarding to haproxy and then finally Apache) but the problem I've hit is that other code such as PHP libraries will just check for the $_SERVER variable. Rather than doing an equivalent fix for every third-party piece of software which comes along it would be great to have $_SERVER['HTTPS'] set once we've come out of bootstrap.

marcingy’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

This needs to be fixed in d8 first.

rwohleb’s picture

Subscribe.

marcingy’s picture

Priority:Major» Normal

This really isn't major.

plingamn’s picture

Hello All,

I am stuck at a point where I could not be able to get $_SERVER['HTTPS'] value from the Apache environment which has SSL on it. Could any body please tell me how can I decide from the $_SERVER that the server is HTTPS or not ???

--
PLN

rwohleb’s picture

I stuck this variant at the end of my settings.php file to get something working with a Zeus Load Balancer configured as the SSL end-point:

$is_https = (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') ||
            (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == "https") ||
            (isset($_SERVER['HTTP_FRONT_END_HTTPS']) && $_SERVER['HTTP_FRONT_END_HTTPS'] == 'on') ||
            (isset($_SERVER['HTTP_SSLSESSIONID']) && !empty($_SERVER['HTTP_SSLSESSIONID']));
$_SERVER['HTTPS'] = $is_https ? 'on' : 'off';

I have Zeus setup with 'ssl_headers' set to true, but this doesn't give anything like HTTP_X_FORWARDED_PROTO. I've had it key off the existence of HTTP_SSLSESSIONID for the moment since it only exists during an SSL session. If anyone has a better way to get the protocol from Zeus I'd love to hear it.

tobiassjosten’s picture

This should be fixed by using Symfony2's HTTP Foundation component.

http://api.drupal.org/api/drupal/core--includes--Symfony--Component--Htt...

As for the FRONT-END-HTTPS header. That's seems to generally be for configuring SSL offloading proxies to workaround Outlook Web Access not reading the X-FORWARDED-PROTO header. See BIG-IP's documentation and Microsoft's report.

The Zeus Load Balancer problem seems like way more of an edge case and should probably go in a separate issue.

rwohleb’s picture

I think the Zeus stuff is still valid in this issue because it demonstrates an edge case. Specifically, there will be edge cases and we need to make sure that there is a good way to handle them in the future, such as with Symphony.

tobiassjosten’s picture

So we agree that Zeus Load Balancer is an edge case and you have proven you can fix it with your settings.php. Then I don't really see the merit in bloating core further. Do we really want every Drupal site to check for Zeus headers on every page load?

However a write-up about how you fixed it for Zeus would be a nice addition to the documentation. There are probably other load balancers out there with their respective, proprietary header to convey the edge protocol.

rwohleb’s picture

@tobiassjosten: Yes and no. As you said this can get into the realm of Symfony2 once we get past D7, and this issue is no longer listed as D7 only. I think it's important to identify these types of edge-cases so that any solution brought to the table does not make it more difficult than it needs to be to get around said edge-case. It does not need to check for Zeus in core if there is an easy documented solution. I have a hack (I wouldn't call it more than that) for D7, but I want to make sure I have a solution moving forward.

tobiassjosten’s picture

Then I think we agree? :)

Documentation is still needed to cover SSL proxying with proprietary headers. Maybe you want to create a documentation page for this?

Btw, I found this excellent write-up that covers the solution very nicely: http://www.metaltoad.com/blog/running-drupal-secure-pages-behind-proxy.

rwohleb’s picture

We agree that a specific check for Zeus does not need to be in core, but I don't think you get the rest of what I'm saying. The symphony code, as it stands now, covers the major cases. Unfortunately, I still don't see a "clean" way to handle the edge cases in D8 and beyond.

A good example of what I want is the 'reverse_proxy_header' variable. It was added to provide an easy way to handle when something other than 'X-Forwarded-For' was used by an upstream proxy. It makes it so we don't have to do a hack like I did with Zeus. It would be nice if we also provided a variable to override the use of 'X_FORWARDED_PROTO' in Symphony.

tobiassjosten’s picture

I get what you're saying. Only I disagree with implementing it in Drupal core.

What it all boils down to is getting $_SERVER['HTTPS'] to say "on", before the bootstrap kicks in. A clean way of achieving this with proprietary proxy headers is to implement the logic in your settings.php - the file dedicated to holding your environment specific configurations.

// Zeus Load Balancer proxies SSL.
if (!empty($_SERVER['HTTP_SSLSESSIONID'])) {
  $_SERVER['HTTPS'] = 'on';
}

Done. This way no one but those with these edge cases will be bogged down by the extra checks and balances.

However, code speaks louder than words so if you have a core implementation in mind - please share and we can talk more specifically.

rwohleb’s picture

Agreed. I don't want it in core. I just want to make sure I can do it easily with 1-3 lines of code, rather than 10+, as this moves forward.

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 63,617 pass(es).
[ View ]

I'm not so sure about the use-case of needing to install Drupal through https, behind a reverse proxy.

I do know that if you don't have $_SERVER['HTTPS'] set to 'on' then you run the risk of a site full of insecure/mixed content warnings for image styles generated with an incorrect $base_path - at least, this is the case in D7.

Here is an attempt at doing something for D8 as simply as possible, without messing with the installer and ignoring the Microsoft headers that were questioned earlier.

The reason for setting $_SERVER['HTTPS'] to 'on' instead of working with $is_https inside drupal_settings_initialize() is that, as @dave_robinson pointed out in #55, and I did in my duplicate issue, other PHP code (like in vendor/) generally expects $_SERVER['HTTPS'] so setting this ensures that 3rd party code is more likely to work too.

thedavidmeister’s picture

Crell’s picture

Modifying $_SERVER is a Wrong Thing To Do(tm) on many levels.

thedavidmeister’s picture

Tell that to Acquia >.<

"Handling incoming HTTPS requests"
https://docs.acquia.com/cloud/configure/https

And what is "The Right Way" to fix this while supporting 3rd party PHP libraries? I'll roll a patch if you tell me :)

pwolanin’s picture

function settings() was removed. Used Settings::get() instead

neclimdul’s picture

StatusFileSize
new914 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Generally I'd be against this but here I agree, the HTTPS environment variable actually has to be set. As has been said repeatedly, this is because this variable is expected by convention to be set correctly outside of our or Symfony's code including in libraries we're using in core. https://github.com/zendframework/zf2/blob/master/library/Zend/Feed/PubSu...
I can't find where that code would actually be called in our code base, but it serves as an example of why we need to do this.

Let me put it this way. You could be handling setting the environment correctly outside of PHP code, for example in your .htaccess. That might even be the best way. However, the entire point of our reverse proxy code is to do that in software based on user configured values. So if we're going to do it we need to do it right and have it just work.

Here's a little bit tighter version that very well may fail by building a request too early. However after #2016629: Refactor bootstrap to better utilize the kernel goes in there's a handy place in the new bootstrap that already had the request object available that's a perfect place for this code.

Status:Needs review» Needs work

The last submitted patch, 75: 313145-75.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review

75: 313145-75.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 75: 313145-75.patch, failed testing.

mgifford’s picture

This is a simple enough patch that's been going back/forth now since 2008. @neclimdul your patch looks good but I don't know how to debug the problem here:

Fatal error: Call to a member function get() on a non-object in /home/s9f53f623164402c/www/core/lib/Drupal.php on line 187

@Crell, would be really good if you could give us some direction about what you think the way forward should be.

This is a potential security problem for a lot of sites. It shouldn't sit this long without a fix.

It's currently a big note on the Using a load balancer or reverse proxy Handbook page.

t0xicCode’s picture

StatusFileSize
new1.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,732 pass(es).
[ View ]

X-Forwarded-Proto is a de facto standard (and Microsoft are the only ones implementing Front-End-Https).

It is implemented in many ssl termination front-ends (ELB, GeekISP, etc.), and can be easily added to most that do not implement it (nginx, haproxy, etc.).

While I agree that we shouldn't modify the $_SERVER array for vendors, we should be checking for the existence of the X-Forwarded-Proto header and using it ourselves. We should then turn around and try to get the vendor modules we use to also check for the presence and value of the X-Forwarded-Proto header.

t0xicCode’s picture

Status:Needs work» Needs review
Damien Tournoud’s picture

There is nothing wrong in setting $_SERVER this is what middlewares do.

Crell’s picture

Wouldn't this make more sense upstream in Symfony proper?

Damien Tournoud’s picture

@Crell: Symfony already supports this, so I'm not quite sure what this issue is about to begin with.

thedavidmeister’s picture

While I agree that we shouldn't modify the $_SERVER array for vendors, we should be checking for the existence of the X-Forwarded-Proto header and using it ourselves. We should then turn around and try to get the vendor modules we use to also check for the presence and value of the X-Forwarded-Proto header.

Can somebody please explain to me what is bad about modifying $_SERVER so early in bootstrap that nothing will be reading from it before we write to it?

A concrete example of something that can go wrong, not just "correctness". if it is "incorrect" somehow to modify $_SERVER, could somebody reference some documentation stating that this can be a problem?

The reason that I ask, is that currently we do not support X-Forwarded-Proto, which is bad but developers can see their https clearly not working and track down a course of action (which is usually to set $_SERVER['https'] - this is recommended by Acquia and I think Omega-8 too). If we claim to fully support it, but some of our shipped Vendor code is silently failing, developers are much less likely to notice, leaving bugs or security issues in sites.

If we formally "support" X-Forwarded-Proto, I would like to see it done in a way that strives for maximum compatibility with all third party code and not just the vendor directory we ship with.

The strategy of setting $_SERVER['https'] manually in bootstrap also makes it easy to support new conventions in the future and be guaranteed that they will work equally well as currently supported conventions (such as if we decide to support the Microsoft naming convention some day). This strategy is not at all exclusive of contacting upstream vendors and asking them to improve their code, we should still do that, it simply ensures that we've taken reasonable steps to protect ourselves in the meantime.

Currently, in Core, I can see /core/vendor/zendframework/zend-feed/Zend/Feed/PubSubHubbub/AbstractCallback.php references $_SERVER['https'] and not X-Forwarded-Proto.

t0xicCode’s picture

StatusFileSize
new1.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,737 pass(es).
[ View ]
new643 bytes

In an ideal world, all our third party code would support the X-Forwarded-* headers natively. I have opened an issue with zf2 (#6356) to have X-Forwarded-Proto supported in PubSubHubbub.

The reason that I am uneasy with modifying $_SERVER['https'] is that drupal itself wouldn't actually be accessed over HTTPS. The HTTPS session is terminated at the crypto offloader/reverse proxy, and then an unencrypted HTTP connection is made to drupal. So drupal is accessed over http ($_SERVER['https'] should be false), but the links should use the https scheme (because that's what the public face of the website is on).

In any case, I've updated the patch to modify $_SERVER['HTTPS'], with a mention that it's being done to ensure support for third-party code that doesn't support X-Forwarded-Proto.

I'll also mention that, on Friday, the IETF approved RFC 7239, which defines a standard Forwarded HTTP header, so we'll have to update our code once a few crypto offloaders implement the RFC.

thedavidmeister’s picture

The reason that I am uneasy with modifying $_SERVER['https'] is that drupal itself wouldn't actually be accessed over HTTPS.

OK, that's fair. And I agree this is not ideal. What real repercussions does this have though? Drupal renders web pages, and if the web page is to be served over HTTPS, then we need to render that correctly.

It isn't just links, it's images, scripts, stylesheets, any static content at all that doesn't use the https protocol in the URL and gets sent over https will trigger insecure content warnings.

t0xicCode’s picture

86: support-x_forwarded_proto-313145-86.patch queued for re-testing.

What real repercussions does this have though? Drupal renders web pages, and if the web page is to be served over HTTPS, then we need to render that correctly.

It might not have any real repercussion, but in my opinion, this is why we have $is_https Drupal::request()->isSecure() #1960344: Replace $is_https global with Request::isSecure().

It isn't just links, it's images, scripts, stylesheets, any static content at all that doesn't use the https protocol in the URL and gets sent over https will trigger insecure content warnings.

Shouldn't those be generated using the aforementioned Drupal::request()->isSecure()? Rather than using $_SERVER['https'] directly?

thedavidmeister’s picture

#88 - Yes, that totally makes sense, but then we're back to discussing what to do about 3rd party code - including that which might not ship with D8 core but developers might not know how to handle properly without doing a lot of research.

IMO, configuring Drupal to work with https should be as easy as possible for developers of all skill levels to achieve without risk of mixed content, even if we throw a few external PHP libraries into the mix.

t0xicCode’s picture

The patch as it is now (#86) will work with third-party code since it does modify the value of $_SERVER['https'] to 'on'.

Using a crypto offloader to do https will be as simple as setting 'reverse_proxy' to TRUE, no other configuration necessary. All drupal code and third-party code will pick up on the changed $_SERVER['https'].

Does anyone see any reason why it shouldn't be merged (or rather RTBC)?

Damien Tournoud’s picture

Status:Needs review» Needs work

This duplicates similar code in Symfony HttpFoundation. None of this should be necessary if this part of the code were using the request properly.

neclimdul’s picture

I think it didn't use the request object because it wasn't available yet. After the kernel bootstrap refactor that will be different. If this is "needs work" its probably postponed.

As far as the rest of the discussion I think we should be clear this discussion isn't really about Drupal. Drupal code above the lowest levels of bootstrap should not be using $_SERVER for anything in 8.x. The problem is without relying on other libraries, third party code has to read from it. It might implement its own reverse proxy detection but it would require a lot of work to provide the same level of integration that we can(trust, etc).

I think this issue in terms of Drupal 8 can be summarized as "We are making the assertion that between our community and Symfony's we can provide a better implementation of reverse proxy detection then external libraries. Since we won't be accessing $_SERVER anyway, we can mock the protocol and there should be no impact on Drupal."

Damien Tournoud’s picture

In the case we want to mock $_SERVER to properly support vendors, we should really use Request::overrideGlobals() directly. It doesn't currently supports setting $_SERVER["https"] properly, but that should be an easy PR.

t0xicCode’s picture

I think it didn't use the request object because it wasn't available yet. After the kernel bootstrap refactor that will be different. If this is "needs work" its probably postponed.

This patch will be redundant after the kernel bootstrap refactor, since symfony already checks for the X-Forwarded-Proto header. I mainly want to get it in D8 now so that we can backport this to D7 without causing a regression.

As far as the rest of the discussion I think we should be clear this discussion isn't really about Drupal. Drupal code above the lowest levels of bootstrap should not be using $_SERVER for anything in 8.x. The problem is without relying on other libraries, third party code has to read from it. It might implement its own reverse proxy detection but it would require a lot of work to provide the same level of integration that we can(trust, etc).

I think this issue in terms of Drupal 8 can be summarized as "We are making the assertion that between our community and Symfony's we can provide a better implementation of reverse proxy detection then external libraries. Since we won't be accessing $_SERVER anyway, we can mock the protocol and there should be no impact on Drupal."

Sounds right.

In the case we want to mock $_SERVER to properly support vendors, we should really use Request::overrideGlobals() directly. It doesn't currently supports setting $_SERVER["https"] properly, but that should be an easy PR.

That should be done after the kernel bootstrap refactor IMO.

thedavidmeister’s picture

This issue badly needs an issue summary update in light of recent comments.

t0xicCode’s picture

t0xicCode’s picture

Assigned:Unassigned» t0xicCode
Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs issue summary update
t0xicCode’s picture

Issue summary:View changes

I added some links to the description

mparker17’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Doesn't apply to HEAD anymore, probably because of #2016629: Refactor bootstrap to better utilize the kernel.

t0xicCode’s picture

Version:8.x-dev» 7.x-dev

Now that Drupal uses the new Symfony kernel, this Just Workstm. The symfony kernel has a working test (\Symfony\Component\HttpFoundation\Tests\RequestTest::testTrustedProxies()) for this feature.

For that reason, I'm moving this issue to 7.x given that no regression will be introduced and a "fix" is present in 8.x.

t0xicCode’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.3 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

backported to d7

t0xicCode’s picture

StatusFileSize
new1.2 KB
FAILED: [[SimpleTest]]: [MySQL] 41,147 pass(es), 51 fail(s), and 2 exception(s).
[ View ]

I was still using the d8 Settings object.

The last submitted patch, 101: support-x_forwarded_proto-313145-101.patch, failed testing.

mparker17’s picture

Issue summary:View changes

Code looks good except for two very very minor code style nitpicks...

  1. +++ b/includes/bootstrap.inc
    @@ -718,6 +718,21 @@ function drupal_settings_initialize() {
    +  // broken $base_path unless we check $_SERVER['HTTP_X_FORWARDED_PROTO'] to 'on'.
    ...
    +      // They should however implement support for X-Forwarded-Proto on their own

    In general, all lines of code should not be longer than 80 chars.

  2. +++ b/includes/bootstrap.inc
    @@ -718,6 +718,21 @@ function drupal_settings_initialize() {
    +      // To ensure that third-party code continues working
    +      // They should however implement support for X-Forwarded-Proto on their own

    Comments should begin with a capital letter and end in a period, question mark or exclaimation mark.

... but I'm not changing the issue status because I don't think those should prevent the patch from going in (we've been waiting 6 years, after all!).

Status:Needs review» Needs work

The last submitted patch, 102: support-x_forwarded_proto-313145-102.patch, failed testing.

mparker17’s picture

I tested it out: if $conf['reverse_proxy'] = TRUE; is not uncommented in settings.php, then I get a Undefined Index error from the code added in the patch. This should be fixed so it can handle that case.

Also, if $base_url is uncommented and set to 'http://example.com', for example, Drupal will still generate all URLs (including stylesheets, etc.) using that. This should be noted in the documentation in settings.php and bootstrap.inc.

Other than those two things, the patch works.

mparker17’s picture

Issue summary:View changes
Issue tags:-needs backport to D7

For other people looking to test this out, I've added some manual testing steps.

t0xicCode’s picture

StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 41,259 pass(es).
[ View ]
new2.27 KB

I've revised the patch with your feedback, and I've introduced a new configuration variable reverse_proxy_proto_header, which would allow us to support the rare Front-End-Https header.

I've also introduced some documentation regarding $base_url in settings.php.

mgifford’s picture

Status:Needs work» Needs review
Damien Tournoud’s picture

Status:Needs review» Needs work
<?php
+ * If you are using a reverse proxy as a crypto offloader, you must ensure that
+ * this variable is commented.
?>

Please don't add this. This is a lot more complicated than that, and not setting $base_url explicitly is unfortunately a security vulnerability in most setups. See https://www.drupal.org/node/1992030

t0xicCode’s picture

Status:Needs work» Needs review
StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 41,127 pass(es).
[ View ]
new2.47 KB

I've removed the comment on $base_url, and introduced a new configuration setting (disabled by default), called reverse_proxy_proto_change that, when set to TRUE, will tell Drupal to rewrite the scheme used by the base url.

t0xicCode’s picture

Actually, the scheme_rewrite snippet is not in d8. Should it be added to \Drupal\Core\DrupalKernel::initializeRequestGlobals() first?

Damien Tournoud’s picture

Version:7.x-dev» 8.x-dev

Yes, until the bootstrap is refactored to use the request object directly, this is not actually supported in any ways in Drupal 8.

t0xicCode’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Postponed
Issue tags:+blocked
Related issues:+#2296555: Make the kernel rewrite the base_url scheme

I've created a new issue against 8.x because only base_url rewriting needs to be added to 8.x. I'll work on the patch, submit it to 8.x and once it's commited, I'll change this to CNR.

donSchoe’s picture

Patch in #111 applies to latest D7 core but it seems not to work. I followed all steps in issue summary but all css and scripts are still loaded via http.

Do I have to wait until this is fixed in D8? I really need a solution for Drupal 7....

t0xicCode’s picture

There were a bunch of changes that affected the boot process in D8, so I'd have to dig in again to see the status of both this and the related issue.

b_sharpe’s picture

Why is this postponed for D7? This really should be in core.

mgifford’s picture

Status:Postponed» Needs review

Ya, it shouldn't be postponed unless there's a clear issue that we're waiting for.

#111 still applies nicely though not sure it fixes this issue for D7.

Georgique’s picture

For me #111 works nice. Already on production.

neclimdul’s picture

Just a note, went through the manual testings steps in the summary and as I expected D8 already supports this because we setup Symfony's Request class with our reverse proxy settings and use the Request for all url generation. So D8 isn't really part of the discussion anymore.

t0xicCode’s picture

Issue tags:-blocked

I haven't taken a look at d8 in a while, but following testing done by @neclimdul in #121, I belive that scheme rewriting works. However, there is no extra setting in d8 for scheme rewriting. Is there an issue to introduce a setting in d7 that won't exist in d8 (because it behaves differently)?

neclimdul’s picture

I haven't really been following the D7 discussion or patches here. Are you asking if there might be a problem with adding a setting to D7 that doesn't exist in D8? That would be up a core maintainer to decide but if you can argue that the behaviour without the settings would cause an unexpected regression or change in behaviour then I don't see how it would be a problem. Just clearly note those things in the summary so the maintainer know why the new setting exists.

t0xicCode’s picture

That's exactly what I'm asking. On the other hand, the D8 default behaviour is to rewrite if reverse_proxy is set to TRUE. The setting I've added allows the rewriting to be turned on and off (off by default). I could make it so that D7 works the same way as D8 (basically force url rewriting on reverse proxy connection). It'd be nice if a core maintainer could weigh in on this. @mgifford Could you ping someone?

t0xicCode’s picture

Assigned:t0xicCode» Unassigned
t0xicCode’s picture

Version:7.x-dev» 8.0.x-dev
Assigned:Unassigned» t0xicCode
Issue tags:+Needs issue summary update, +Needs change record, +Needs beta evaluation
StatusFileSize
new7.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,945 pass(es).
[ View ]

I'll be updating the issue description to add the required things for drupal 8 beta review, the issue summary update and the change record.

thedavidmeister’s picture

+++ b/sites/default/default.settings.php
@@ -351,6 +351,24 @@
+ * Set this value if your proxy server sends the client protocol in a header
+ * other than X-Forwarded-Proto.
+ */
+# $conf['reverse_proxy_proto_header'] = 'HTTP_X_FORWARDED_PROTO';
+

bike shed I know, but should this be called "reverse_proxy_proto_header" or "reverse_proxy_scheme_header"?

t0xicCode’s picture

I decided to call it reverse_proxy_proto_header because that's what the common header is called. I'm not opposed to changing it if consensus is that scheme is better than proto, but it does feel like bike shedding.

thedavidmeister’s picture

Issue tags:+Needs manual testing

there will likely be no consensus on variable names here.

probably needs manual testing though.

mgifford’s picture

@thedavidmeister - What steps do we need for manual testing? Just run it behind a proxy? Anything else?

drzraf’s picture

Any reason for not using Network-Path Reference URI / Scheme relative URLs when generating absolute URL if $base_url is not set in settings.php?

see:

drzraf’s picture

Adding that a HTTP Drupal behing an SSL-offloader and a reverse-proxy should notify the later not to cache the same way according to the scheme (if the site can be acceded in both HTTP and HTTPS mode)
This is best done by adding a Very: X-Forwarded-Proto to the header.

t0xicCode’s picture

Just a quick update regarding this. We have been using the patch at #108 and #111 with success in production on d7, but I haven't had the time to push for the more recent patches regarding d8. I'll try to get some help on this from someone at drupalCon, that'd be great.

t0xicCode’s picture

Title:Support X-Forwarded-Proto HTTP header» Support X-Forwarded-* HTTP headers and alternates
Issue summary:View changes
Issue tags:-Needs issue summary update, -Needs beta evaluation
t0xicCode’s picture

Title:Support X-Forwarded-* HTTP headers and alternates» Support X-Forwarded-* HTTP headers alternates
Assigned:t0xicCode» Unassigned
Issue tags:-Needs change record

I've created a new change record draft for this, updated the issue summary and added a beta evaluation. The testing steps should not have changed, but I could be wrong about that.

t0xicCode’s picture

Priority:Normal» Major