Support X-Forwarded-Proto HTTP header

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

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.

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?

Meh, wrong diff format. :)

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.

Updated for current HEAD

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, 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,

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.

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.

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.

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.

The last submitted patch failed testing.

subscribe

@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.

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.

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.

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, 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?

@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.

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. :-)

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.

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

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.

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.

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

@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?

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.

Subscribe.

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

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.

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.

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.

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.

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.