Skip to content

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

Merged
merged 1 commit into from
Jul 18, 2014
Merged

Reverse Proxy SSL Support / Security Improvements #1

merged 1 commit into from
Jul 18, 2014

Conversation

drewwestphal
Copy link

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.

@yosifkit
Copy link
Member

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?

@drewwestphal
Copy link
Author

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.

if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') $_SERVER['HTTPS']='on';

FWIW this is the correct approach according to the wp people
http://codex.wordpress.org/Administration_Over_SSL#Using_a_Reverse_Proxy

@@ -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
Copy link
Member

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

@drewwestphal
Copy link
Author

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.

@tianon
Copy link
Member

tianon commented Jul 18, 2014

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.
@drewwestphal
Copy link
Author

I have amended the commit.

if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https') {
$_SERVER['HTTPS'] = 'on';
}
EOPHP
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants