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
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:
- Set up your webserver so it can serve requests over HTTPS.
- 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"
- Ensure your webserver is using the new configuration (e.g.:
sudo /etc/init.d/apache2 reload
). - Optionally, perform a fresh install of Drupal. Otherwise, have a working copy of Drupal.
- In
settings.php
, ensure the#$base_url = 'http://example.com";
line is commented. - In
settings.php
, un-comment the$conf['reverse_proxy'] = TRUE;
line. - In
settings.php
, un-comment the$conf['reverse_proxy_addresses'] = array();
line and add your client IP to the array (most likely127.0.0.1
). - Flush all Drupal caches (e.g.:
drush -y cc all
). - 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.
- If the patch is applied and working correctly, all your stylesheets, scripts, etc. will use the https protocol.
Comment | File | Size | Author |
---|---|---|---|
#167 | support-proto-header-313145-167.patch | 2.43 KB | oriol_e9g |
#152 | support_x_forwarded-313145-142.patch | 5.15 KB | znerol |
#142 | interdiff.txt | 7.29 KB | znerol |
#142 | support_x_forwarded-313145-142.patch | 5.15 KB | znerol |
#142 | support_x_forwarded-313145-142-TEST-ONLY.patch | 1.98 KB | znerol |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch looks good to me but I can't test it.
Comment #2
Bart Jansens CreditAttribution: Bart Jansens commentedX-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.
Comment #3
ghoti CreditAttribution: ghoti commentedGood 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.
Comment #4
ghoti CreditAttribution: ghoti commentedPatch 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....
Comment #5
c960657 CreditAttribution: c960657 commentedYou 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. Checkingreverse_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?
Comment #6
ghoti CreditAttribution: ghoti commentedI'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 tofunction 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're absolutely right. Thanks! I've updated the patch.
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).
Comment #7
ghoti CreditAttribution: ghoti commentedMeh, wrong diff format. :)
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #9
seanrI'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.
Comment #10
BerdirWe 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.
Comment #11
ghoti CreditAttribution: ghoti commentedUpdated for current HEAD
Comment #12
cburschkaInline 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? ;)
Comment #13
ghoti CreditAttribution: ghoti commentedArancaytar,
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".
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedErm? That condition makes no sense. It should probably be:
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:
Comment #15
ghoti CreditAttribution: ghoti commentedDamien, 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.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commented1. 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.
Comment #17
ghoti CreditAttribution: ghoti commentedDamien,
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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.Comment #19
ghoti CreditAttribution: ghoti commentedActually, 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 theHost
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 trustHost
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.Comment #20
ghoti CreditAttribution: ghoti commentedHere'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.
Comment #21
ghoti CreditAttribution: ghoti commentedSlightly 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.
Comment #22
hass CreditAttribution: hass commented+
Comment #23
hass CreditAttribution: hass commentedOnly 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.Comment #24
seanrI took the liberty of re-rolling the patch with your recommendations. Please see attached.
Comment #26
seanrNot 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.
Comment #27
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #28
pwolanin CreditAttribution: pwolanin commentedWe require settings.php to exist in order to install Drupal - so this part of the patch seems wrong:
Comment #29
pwolanin CreditAttribution: pwolanin commented@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.
Comment #30
pwolanin CreditAttribution: pwolanin commentedwhy does it need to be more than this?
Comment #31
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #32
pwolanin CreditAttribution: pwolanin commentedany reviews?
Comment #33
seanrI 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.
Comment #34
ghoti CreditAttribution: ghoti commented#21: bootstrap.inc_313145.patch queued for re-testing.
Comment #35
ghoti CreditAttribution: ghoti commentedpwolanin, 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...)
Comment #36
pwolanin CreditAttribution: pwolanin commented@ghoti - you have to manually wset it via settings.php - as you always do.
Comment #37
ghoti CreditAttribution: ghoti commented@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?
Comment #38
ghoti CreditAttribution: ghoti commentedRe-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:
Comment #39
c960657 CreditAttribution: c960657 commentedIs 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.
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is won't fix except if this is governed by an hidden variable that defaults to FALSE.
Comment #41
ghoti CreditAttribution: ghoti commentedc960657, 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. :-)
Comment #42
ghoti CreditAttribution: ghoti commentedHere'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.
Comment #43
ghoti CreditAttribution: ghoti commentedIt 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.
Comment #44
seanrCan 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.Comment #45
ghoti CreditAttribution: ghoti commentedHere it is, including the link to http://drupal.org/node/425990 .
Comment #46
pwolanin CreditAttribution: pwolanin commentedSeveral code style issues, and at a "XXX" code comment that looks like it's missing it s intended content.
Comment #47
ghoti CreditAttribution: ghoti commentedAh, 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.
Comment #48
ghoti CreditAttribution: ghoti commentedDebug line removed.
Comment #49
seanrJust 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.
Comment #50
seanrIt furthermore is getting the load balancer IP in the watchdog instead of the correct one. Looks like this isn't actually doing anything.
Comment #51
sun.core CreditAttribution: sun.core commentedSounds like 1) needs work and 2) a task (actually a feature request).
Comment #52
mo6Please 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.
Comment #53
pwolanin CreditAttribution: pwolanin commented@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?
Comment #54
neclimdul@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.
Comment #55
dave_robinson CreditAttribution: dave_robinson commentedAre 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.
Comment #56
marcingy CreditAttribution: marcingy commentedThis needs to be fixed in d8 first.
Comment #57
rwohlebSubscribe.
Comment #58
marcingy CreditAttribution: marcingy commentedThis really isn't major.
Comment #59
plingamn CreditAttribution: plingamn commentedHello 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
Comment #60
rwohlebI 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:
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.
Comment #61
tobiassjosten CreditAttribution: tobiassjosten commentedThis 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.
Comment #62
rwohlebI 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.
Comment #63
tobiassjosten CreditAttribution: tobiassjosten commentedSo 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.
Comment #64
rwohleb@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.
Comment #65
tobiassjosten CreditAttribution: tobiassjosten commentedThen 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.
Comment #66
rwohlebWe 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.
Comment #67
tobiassjosten CreditAttribution: tobiassjosten commentedI 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.
Comment #68
rwohlebAgreed. 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.
Comment #69
thedavidmeister CreditAttribution: thedavidmeister commentedI opened a duplicate at #2181941: Drupal should recognise $_SERVER['HTTP_X_FORWARDED_PROTO'] when attempting to detect a https request recently.
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedI'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.
Comment #71
thedavidmeister CreditAttribution: thedavidmeister commentedreview from https://drupal.org/comment/8438191#comment-8438191
Comment #72
Crell CreditAttribution: Crell commentedModifying $_SERVER is a Wrong Thing To Do(tm) on many levels.
Comment #73
thedavidmeister CreditAttribution: thedavidmeister commentedTell 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 :)
Comment #74
pwolanin CreditAttribution: pwolanin commentedfunction settings() was removed. Used Settings::get() instead
Comment #75
neclimdulGenerally 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.
Comment #77
mgifford75: 313145-75.patch queued for re-testing.
Comment #79
mgiffordThis 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.
Comment #80
t0xicCode CreditAttribution: t0xicCode commentedX-Forwarded-Proto
is a de facto standard (and Microsoft are the only ones implementingFront-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 theX-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 theX-Forwarded-Proto
header.Comment #81
t0xicCode CreditAttribution: t0xicCode commentedComment #82
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere is nothing wrong in setting
$_SERVER
this is what middlewares do.Comment #83
Crell CreditAttribution: Crell commentedWouldn't this make more sense upstream in Symfony proper?
Comment #84
Damien Tournoud CreditAttribution: Damien Tournoud commented@Crell: Symfony already supports this, so I'm not quite sure what this issue is about to begin with.
Comment #85
thedavidmeister CreditAttribution: thedavidmeister commentedCan 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.
Comment #86
t0xicCode CreditAttribution: t0xicCode commentedIn 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.Comment #87
thedavidmeister CreditAttribution: thedavidmeister commentedOK, 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.
Comment #88
t0xicCode CreditAttribution: t0xicCode commented86: support-x_forwarded_proto-313145-86.patch queued for re-testing.
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().Shouldn't those be generated using the aforementioned
Drupal::request()->isSecure()
? Rather than using$_SERVER['https']
directly?Comment #89
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #90
t0xicCode CreditAttribution: t0xicCode commentedThe 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)?
Comment #91
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis duplicates similar code in Symfony HttpFoundation. None of this should be necessary if this part of the code were using the request properly.
Comment #92
neclimdulI 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."
Comment #93
Damien Tournoud CreditAttribution: Damien Tournoud commentedIn 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.Comment #94
t0xicCode CreditAttribution: t0xicCode commentedThis 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.
Sounds right.
That should be done after the kernel bootstrap refactor IMO.
Comment #95
thedavidmeister CreditAttribution: thedavidmeister commentedThis issue badly needs an issue summary update in light of recent comments.
Comment #96
t0xicCode CreditAttribution: t0xicCode commented86: support-x_forwarded_proto-313145-86.patch queued for re-testing.
Comment #97
t0xicCode CreditAttribution: t0xicCode commentedComment #98
t0xicCode CreditAttribution: t0xicCode commentedI added some links to the description
Comment #99
mparker17Doesn't apply to HEAD anymore, probably because of #2016629: Refactor bootstrap to better utilize the kernel.
Comment #100
t0xicCode CreditAttribution: t0xicCode commentedNow 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.
Comment #101
t0xicCode CreditAttribution: t0xicCode commentedbackported to d7
Comment #102
t0xicCode CreditAttribution: t0xicCode commentedI was still using the d8 Settings object.
Comment #104
mparker17Code looks good except for two very very minor code style nitpicks...
In general, all lines of code should not be longer than 80 chars.
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!).
Comment #106
mparker17I 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 insettings.php
andbootstrap.inc
.Other than those two things, the patch works.
Comment #107
mparker17For other people looking to test this out, I've added some manual testing steps.
Comment #108
t0xicCode CreditAttribution: t0xicCode commentedI'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 rareFront-End-Https
header.I've also introduced some documentation regarding
$base_url
in settings.php.Comment #109
mgiffordComment #110
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease 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/1992030Comment #111
t0xicCode CreditAttribution: t0xicCode commentedI've removed the comment on
$base_url
, and introduced a new configuration setting (disabled by default), calledreverse_proxy_proto_change
that, when set toTRUE
, will tell Drupal to rewrite the scheme used by the base url.Comment #112
t0xicCode CreditAttribution: t0xicCode commentedActually, the scheme_rewrite snippet is not in d8. Should it be added to
\Drupal\Core\DrupalKernel::initializeRequestGlobals()
first?Comment #113
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes, until the bootstrap is refactored to use the request object directly, this is not actually supported in any ways in Drupal 8.
Comment #114
t0xicCode CreditAttribution: t0xicCode commentedI'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.
Comment #115
donSchoe CreditAttribution: donSchoe commentedPatch 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....
Comment #116
t0xicCode CreditAttribution: t0xicCode commentedThere 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.
Comment #117
b_sharpe CreditAttribution: b_sharpe commentedWhy is this postponed for D7? This really should be in core.
Comment #118
mgiffordYa, 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.
Comment #119
Georgique CreditAttribution: Georgique commentedFor me #111 works nice. Already on production.
Comment #121
neclimdulJust 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.
Comment #122
t0xicCode CreditAttribution: t0xicCode commentedI 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)?
Comment #123
neclimdulI 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.
Comment #124
t0xicCode CreditAttribution: t0xicCode commentedThat's exactly what I'm asking. On the other hand, the D8 default behaviour is to rewrite if
reverse_proxy
is set toTRUE
. 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?Comment #125
t0xicCode CreditAttribution: t0xicCode commentedComment #126
t0xicCode CreditAttribution: t0xicCode commentedI'll be updating the issue description to add the required things for drupal 8 beta review, the issue summary update and the change record.
Comment #127
thedavidmeister CreditAttribution: thedavidmeister commentedbike shed I know, but should this be called "reverse_proxy_proto_header" or "reverse_proxy_scheme_header"?
Comment #128
t0xicCode CreditAttribution: t0xicCode commentedI 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.Comment #129
thedavidmeister CreditAttribution: thedavidmeister commentedthere will likely be no consensus on variable names here.
probably needs manual testing though.
Comment #130
mgifford@thedavidmeister - What steps do we need for manual testing? Just run it behind a proxy? Anything else?
Comment #131
drzraf CreditAttribution: drzraf commentedAny 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:
Comment #132
drzraf CreditAttribution: drzraf commentedAdding 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.
Comment #133
t0xicCode CreditAttribution: t0xicCode at OpenConcept Consulting Inc. commentedJust 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.
Comment #134
t0xicCode CreditAttribution: t0xicCode at OpenConcept Consulting Inc. commentedComment #135
t0xicCode CreditAttribution: t0xicCode at OpenConcept Consulting Inc. commentedI'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.
Comment #136
t0xicCode CreditAttribution: t0xicCode at OpenConcept Consulting Inc. commentedComment #137
znerol CreditAttribution: znerol commentedWhile the original line is not strictly correct, the fix is neither (
Settings::get()
returnsNULL
if no default is given). So either leave the line alone or useassertNull()
.This is testing functionality which is already covered extensively elsewhere (Symfony). I would rather remove those tests and instead test that the settings are propagated correctly to the
Request
.Is there a reason why we do not check for
HEADER_CLIENT_HOST
, andHEADER_CLIENT_PORT
as well?Should be
$settings
, not$conf
.Comment #138
pwolanin CreditAttribution: pwolanin at Acquia commentedcode still applies it seems.
Can we add tests?
Comment #139
znerol CreditAttribution: znerol commentedNote that
setTrustedHeaderName()
and theHEADER_CLIENT_XY
consts are actually static. As a result, we actually do not require a request object in order to configure the proxy settings. This means it is currently possible to setup Symfony directly fromsettings.php
like so:I confirmed that this works with the following minimal nginx config (disclaimer: do not use this in production!):
The certificate can be generated like this:
Nginx can be started (as regular user) like this (assuming the config about is in
/etc/nginx.conf
)So: Why don't we ditch
ReverseProxyMiddleware
and instead add documentation/examples todefault.settings.php
on how to setup the SymfonyRequest
properly?Comment #140
drzraf CreditAttribution: drzraf commenteddid anyone read https://www.rfc-editor.org/rfc/rfc7239.txt before commiting to the enhancement?
Since 2014, Forwarded: is now a standard.
Examples:
Comment #141
znerol CreditAttribution: znerol commentedThanks for the pointer. In fact support for the
Forwarded
header has been added upstream. The only thing left here is to provide a way to customize the header name from withinsettings.php
/ReverseProxyMiddleware
.Comment #142
znerol CreditAttribution: znerol commentedFixes #137, removed the reduntant tests.
Comment #145
znerol CreditAttribution: znerol commentedDid manual testing again with the config in #139 and using the proper
$settings
and the patch from #142. The issue summary seems accurate.Setting the category to bug, since currently it is impossible to configure the
Symfony
part of Drupal appropriately.Comment #146
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - Looks great to me and makes sense to support the whole shebang of Symfony configuration.
Especially as 3.0 _might_ eventually go away from using global state for such things and hence making it impossible to set from settings.php. (spoke with some people)
Comment #147
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdate: nginx conf was wrong. With nginx + fastcgi and unix sockets I had to use fastcgi_param instead of add_header or proxy_set_header. Now it works with the patch.
I tried to do the same as #145 but no luck with Drupal 8 rc1 and nginx + fastcgi.
I have only https enabled in nginx and I set all above params but Drupal still generates http links to css/js files.
With $settings[‘file_public_base_url’] I managed to fix some of the links but http://mydrupal/core/assets/vendor/modernizr/modernizr.min.js is still fetched as HTTP and som other files like favicon all starting with http://mydrupal/core.
In Beta 15 I solved it with base_url setting but that is gone in rc1.
Comment #148
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedShould be triaged ...
Comment #149
catchI committed #2528988: Remove the option to specify a base_url from within settings.php last week on the basis this issue should fix some of the use cases for that setting (specifically #147), so agree on triaging during RC.
Comment #151
znerol CreditAttribution: znerol commentedRandom test fail?
Comment #152
znerol CreditAttribution: znerol commentedReuploading.
Comment #153
effulgentsia CreditAttribution: effulgentsia at Acquia commented@alexpott and I agree that #149 makes this a good issue for including during RC.
Comment #154
alexpottCommitted 238a239 and pushed to 8.0.x. Thanks!
Comment #156
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedPublished the change record.
Comment #157
t0xicCode CreditAttribution: t0xicCode at OpenConcept Consulting Inc. commentedShould we backport similar functionality to d7?
Comment #159
colanIf someone wants to upload a patch, he/she can reopen this and do so.
Comment #160
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #164
Wim LeersThis needs a separate D7 issue to be created, so this issue can be closed for D8.
Comment #167
oriol_e9gWe are under a load balancer and have detected problems with this.
In Drupal 7 maybe we can fix under drupal_is_https() function to ensure that if an external module calls this function return coherent response.
Comment #168
ioanmar CreditAttribution: ioanmar as a volunteer commentedI've tested #167 manually and it looks like RTBC. All resources are requested through https:// as suggested and expected in the description.
Comment #169
joseph.olstadOpenned D7 backport issue
Comment #170
Wim Leers@joseph.olstad Thanks!
Comment #172
crutch CreditAttribution: crutch commentedWe discovered that we are experiencing this issue https://www.drupal.org/project/drupal/issues/2984933
Comment #173
joseph.olstad@crutch , for D7, please use the D7 issue:
#2970929: [D7] Support X-Forwarded-* HTTP headers alternates
Comment #174
O'Briat CreditAttribution: O'Briat as a volunteer and at Capgemini for SNCF, E.voyageurs Technologies commentedHi
There's a small typo into the settinqs.php comments, the protocol line has been copy/past for all the headers :
"Set this value if your proxy server sends the client protocol in a header other than X-Forwarded-Host."
This should be :
I'll try to make a patch + diff...
Comment #175
colan#174: Please do that in a new issue as this one is closed/fixed. Thanks!
Comment #176
O'Briat CreditAttribution: O'Briat as a volunteer and at Capgemini for SNCF, E.voyageurs Technologies commentedYes, sorry.
Just for the record, there already two issues opened on this typo :
https://www.drupal.org/project/drupal/issues/3017957
https://www.drupal.org/project/drupal/issues/2673572