Fixing Admin force SSL redirect, adding more options

[gimelfarb]

Fixing SSL redirect handling behind a proxy (sever.enable('trust proxy')). Also fixing handling of Admin theme resources, which should be
served via HTTP when requested.

Adding more options:

1. Send a non-301 response - for example, 401 Unauthorized
2. Configure a different hostname for SSL (e.g. 'https://secure.myblog.com')

Fixing #1836, #1837. Implementing #1838.

[ErisDS]

@gimelfarb Thanks for taking the time to work on Ghost :)
It seems to me you're making pretty big changes here without really documenting what you're doing or why, which makes it very hard for me to test this and get it merged.

For the bugs, please can you ensure that you have documented the full reproduction steps, preferably in a bug issue, otherwise it is not possible for us to verify the bugs.

For the new options / features, it's up to you to make a case for them and sell the idea.

It seems you've made significant changes to how the handling of SSL works here, including removing everything to do with the X-Forwarded-Proto header, without explaining why.

Additionally, your change breaks the build, and doesn't add or update any tests which I would expect you would need to do.

Sorry to add such a big list of things to do, but without a little more explanation on your side I am not able to merge this.

[sebgie]

I think that this PR is not going to work :-(. config().url is used in multiple places and has to be changed a second URL into account. Instead of server.enable('trust proxy') the X-Forwarded-Proto is currently used which needs some testing if a switch to another method makes sense.

With that in mind, please ensure that your issues follow our contribution guidelines (https://github.com/TryGhost/Ghost/blob/master/CONTRIBUTING.md#reporting-an-issue), open an issue and get a discussion started :-). The enhancements you are proposing are in my opinion worth adding.

[gimelfarb]

Hi guys, thanks for the "welcome" ;) Alright, firstly my apologies for barging in with big changes unannounced like this. You are right, I should have created an issue and bug ticket first, so that it can be referenced. My bad for not reading the contribution guidelines carefully. I just rushed in the PR late at night, and didn't even see that Travis build broke (which was a JSLint code styling validation failure).

I am pretty new to the open source contribution thing, so please bear with me. All the issues I am fixing in my fork of Ghost are related directly to me trying to host it on Heroku for myself. So I do make sure it works.

It is a pretty long to-do list, and I will endeavor to tick those off. Probably too long to do in one evening though. I will raise a separate issue that can be referenced here - there are 2 really, one is a bug and the other is enhancement. I have also reviewed the code again following your comments, and I do agree about not being careful with config().url. So I have modified my branch and so this PR is now updated with a different variation of this change, which is a bit less intrusive. (Oh, and also Travis build is now green ;))

Let me just outline here the changes to provide more background. As I said, I'll create Issues for these as well, like you requested.

1. Move expressServer.use(checkSSL) after the admin static resources line. This is important because admin theme static resources are used in the 404 error pages. If 'forceAdminSSL' option is set, then anyone who browses to invalid link via HTTP, will be redirected to 404 page where resources might not be able to load (because they would be attempting to load via SSL - it's not only unnecessary, but can potentially fail, leaving an unstyled 404 page).

2. expressServer.enable('trust proxy') - this is a more elegant way of checking X-Forwarded-Proto header, and is consistent with express framework guidelines. As of their current code, it only affects req.protocol, req.ip and req.host, setting them to real values that the reverse proxy would have received. This is only a good thing, and eliminates explicit checking for the "X-Forwarded-" headers. That means req.secure is now properly set regardless of whether we are running behind proxy or not.

   Yes, one can argue that there can possibly be proxy implementation which does not set this header. In this case, the current code would also break, and you'd have to patch in support for such non-conforming proxy, if that was required. So it's not an issue.

3. I removed the current 'X-Forwarded-Proto' handling for 2 reasons. One is already discussed above - 'trust proxy' setting is way more elegant and conforming. Second reason is that the code assumes httpsHeader to be true when it cannot find 'X-Forwarded-Proto' header. The comment argues about an infinite redirect loop, but the reasoning is flawed. We cannot assume secure HTTPS connection when it isn't one. This is a bug. Very easy to reproduce just by running locally against "localhost" - the redirect to HTTPS would not be happening, since the header is not there.

   It is safer to assume that connection is not HTTPS, if the header is missing and URL's protocol is "http", since as far as Ghost is concerned that is HTTP connection. If that's because of a non-conforming reverse proxy, then it has to be either fixed on the proxy or with a different setting (which brings me to the enhancement point).

4. Redirect to "https" version of the same URL may not always be appropriate. The scenario is Heroku (free account), where custom domain affords you HTTP connection, but HTTPS is only provided via myblog.herokuapp.com. What's more, is that in my case I wanted to forbid access to /ghost/ management console, and return 401 Forbidden response instead. Forcing myself to always go to back-end https://myblog.herokuapp.com/ghost/ to manage the blog and input my credentials. And would also deter anyone trying to poke around, and trying to access /ghost/ URL via my public domain.

   In any case, flexibility is the argument here. I left original 301 as the default handling, but allowing other responses as well. In fact, I tried to make it flexible by allowing to configure any kind of response - i.e. the arguments to res.send(code, [body]) method.

   I have modified my original submission, as I mentioned, because you were right - that .urlSecure property was hacky, and I did it too quickly. Now I only use a single .forceAdminSSLResponse property to handle all the configuration variability:

   • Without being specified, the default is as it is now, 301 redirect to https:// version of configured blog URL (config().url)
   • If it is a number, then it is the response code. 301/302 follow the same path as above. Any other results in just sending the HTTP response code back to the client
   • If it is an array, then 2nd argument is the body of response. For 301/302 responses that's the redirect URL, so it overrides the config().url and can be used to specify another domain for secure admin connections (like I wanted) - we append the current request path to this URL before issuing the redirect. Just like the current implementation, but with a different blog URL for SSL.

And that's it!

I am interested to know what you think. I will raise those Issues in the meantime to reference here.

[sebgie]

I appreciate the improvements you have implemented for SSL support. And we would like to add the bugfixes to the 0.4-maintenance release (aka 0.4.1). All changes are in one commit right now and it would simplify merging for us, if you could split up the bugfixes (#1836 and #1837) and open new PRs against the 0.4-maintenance branch.

#1838 is a new feature and should go into the next regular release of Ghost. After some research I think that a proper status code for denying access without SSL would be 403 - Forbidden (suggested by OWASP). In my opinion it would be enough to add a config switch to turn redirects to SSL on and off (redirectToSSL=true/false)? Assuming that a second domain for SSL is a common use case (see Heroku) I would also like to suggest to use the second URL throughout the application instead of redirects for every request.

It would be great, if you have a moment to go over the PR and split it up! Thank you :-).

[gimelfarb]

Ok sure I'll split out the bug fixes to be on top of 0.4-maintenace. And create a separate PR for the new feature.

Agree with you about the 403 code being better. Now the forceAdminSSL is the boolean switch you are talking about. Or are you talking about an SSL for the entire site? In the common case SSL is not needed for the entire site, since it's most likely a public blog. SSL is needed to protected admin credentials on the login page and the auth cookie on the rest of admin pages. That's why I limited SSL redirect for the admin part only. Does that make sense?

I think your point is that this can be rolled into a larger feature of enabling a second (SSL) domain for the entire app. Fair call - there could be a scenario of www.myblog.com vs secure.myblog.com, which will be helped by such feature. Let me have a think how this can be best fitted into the app in general. Perhaps with an optional config which can selectively declare certain pages as SSL-required, enabling, for example, for someone to host secure forms on their blog site, all powered by Ghost.

Correct me if I got it wrong.

[sebgie]

I'll try to structure what I wrote above. Adding a second domain adds some complexity we should think about. At the moment we have two configuration options to force the use of SSL.

• url: https://my-ghost-blog.com

If the scheme of url is https:// all traffic to frontend and admin is forced to use SSL and all requests sent using HTTP will be redirected to HTTPS. If the scheme is http:// and SSL is available HTTP and HTTPS requests are answered.

• forceAdminSSL

The forceAdminSSL switch will force the use of SSL on all admin pages (/ghost/*). If it is set to true all requests sent using HTTP will be redirected to HTTPS. In the answer above I was suggesting to add a separate switch (redirectToSSL: true) to enable/disable the redirect and send a 403 forbidden response if the redirect is disabled. After reading your response I think that it would make sense to change the behavior of forceAdminSSL and have all possible combinations covered with one configuration entry.

• forceAdminSSL: false default, SSL is not forced
• forceAdminSSL: true eq to forceAdminSSL: {redirect: true}
• forceAdminSSL: {redirect: true} SSL is forced and all HTTP traffic is redirected to HTTPS
• forceAdminSSL: {redirect: false} SSL is forced and all HTTP traffic to /ghost/* returns 403 forbidden

Now the tricky part. I would suggest using urlSSL as config name in config.js. I think that it is not necessary to force single pages to SSL? In my opinion it is enough to distinguish between frontend and admin? That would allow the following options and need implementation in Ghost:

• url: https://: all traffic should be HTTPS; set url to the SSL domain
• url: http://: HTTP and HTTPS traffic is allowed; add logic to use either url or urlSSL for frontend requests
• forceAdminSSL: false: add logic to use either url or urlSSL for admin requests
• forceAdminSSL: true: redirect all traffic to urlSSL and add logic to use urlSSL where needed
• forceAdminSSL: {redirect: false}: don't redirect and add logic to use urlSSL where needed

Have I forgotten anything?

[ErisDS]

Question for clarity:

Given the changes that SSL will work without a proxy, what will happen to all the people who have configured their headers on their proxy? Will SSL continue to work for them, or will they need to make further changes?

Also, if you do want to get these bug fixes into 0.4.1 as we have suggested, the closing date for PRs is tomorrow (27th Jan).

[sebgie]

@ErisDS It should work with and without the headers on the proxy.

[gimelfarb]

Submitted PRs #2047 and #2048 to close #1836 and #1837 respectively. They are based on the 0.4-maintenance branch as a base.

@ErisDS - the server.enable('trust proxy') will work for both cases, with or without SSL, and detection of X-Forwarded-Proto header will be responsibility of connect framework. It will then appropriately set req.secure property. It's the same header, so no changes there - just code is more elegant by only having to check req.secure property.

@sebgie - the structure you propose certainly makes sense, and does achieve the same goal I set out, plus a bit more. I need to find all places in code that use url to display an absolute link on the pages, since adding urlSSL changes this behavior for HTTPS connections.

[gimelfarb]

P.S. In any case I'll need to wait till the #2047 and #2048 have been merged all the way to 'master' branch, since the #1838 feature implementation will rely on them.

[sebgie]

I should read documentation before commenting :-). @gimelfarb is right, 'trust proxy' will still need the proxy headers. From Expressjs docs:

Using Express behind a reverse proxy such as Varnish or Nginx is trivial, however it does require configuration. By enabling the "trust proxy" setting via app.enable('trust proxy'), Express will have knowledge that it's sitting behind a proxy and that the X-Forwarded-* header fields may be trusted, which otherwise may be easily spoofed.

Enabling this setting has several subtle effects. The first of which is that X-Forwarded-Proto may be set by the reverse proxy to tell the app that it is https or simply http. This value is reflected by req.protocol.

The second change this makes is the req.ip and req.ips values will be populated with X-Forwarded-For's list of addresses.

Adding server.enable('trust proxy') will probably also address #2041.

[sebgie]

Would it be okay to close this PR because it starts to become messy?

#2047 and #2048 have been merged and we can continue the discussion about a second URL for SSL on issue #1838. I'll copy my suggestion for handling a second URL over to the issue.

[ErisDS]

Cool yeah, lets close it and start afresh on getting this sorted out 👍

[gimelfarb]

Yeah absolutely. I am hoping to have some time this weekend to finally look at and close #1838.

[sebgie]

👍 thanks :-)