-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reverse Proxy SSL Support / Security Improvements #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I like the changes to the rsync, good catch. I am not sure on the implications for auto adding https if it is visited by https. Is this the recommended way to handle turning on https for wordpress? |
Hmm. I hear what you are saying. That line is maybe a bit more questionable. The HTTP_X_FORWARDED_PROTO test is specifically for HTTPS setups behind a reverse proxy, which is particularly relevant to docker installs IMO bc if you want to host more than one wordpress site on the same IP and use SSL you need this line in your wp-config. I don't believe HTTP_X_FORWARDED_PROTO=https has many uses other than indicating a reverse proxy ssl setup. Perhaps the below would be safer.
FWIW this is the correct approach according to the wp people |
@@ -82,6 +82,8 @@ for unique in "${UNIQUES[@]}"; do | |||
fi | |||
done | |||
|
|||
echo "if (\$_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') \$_SERVER['HTTPS']='on';" >> wp-config.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see the logic (thanks for the explaining), but this will need to move up inside the if
statement where we do the rsync
or else we'll end up adding this if statement again to the end of wp-config.php
every time we docker restart
our container.
Also, I think it'll be much, much simpler to read and maintain if we do:
cat >> wp-config.php <<'EOPHP'
// If we're behind a proxy server and using HTTPS, we need to alert Wordpress of that fact
// see also http://codex.wordpress.org/Administration_Over_SSL#Using_a_Reverse_Proxy
if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https') {
$_SERVER['HTTPS'] = 'on';
}
EOPHP
That seems correct to me. Writing unescaped PHP in heredoc definitely easier to read && it's commented too! Personally, I'm confused as to why reverse proxy support is not a part of wordpress core... seems like this is a modification without significant side effects. |
Did you want to amend and force push your commit, or do you want me to just take over from here? (I don't mind either way) |
…se-proxy SSL configurations. Amended with changes suggested by tianon.
I have amended the commit. |
if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https') { | ||
$_SERVER['HTTPS'] = 'on'; | ||
} | ||
EOPHP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran a test build on this and got: /usr/src/wordpress/docker-entrypoint.sh: line 117: syntax error: unexpected end of file
. It worked when I unindented the heredoc.
Also, @tianon had you put that in the wrong spot (it was supposed to be in the if [ ! -e wp-config.php ]
so that we get the default config too.
I already made these changes locally, so I'll just merge in your PR with my changes too.
Thanks for the contribution @drewwestphal!
Shell scripts and configuration files are currently copied into ht root folder and accessible via apache if this container is linked to the web. At the least this reveals system configuration information.
Added a line to wp-config.php that allows the instance to support reverse proxied ssl connections.