silverstripe / cwp-installer

CWP project template
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

Add HTTP Strict Transport Security by default #24

Closed chillu closed 5 years ago

chillu commented 5 years ago

Overview

See https://docs.silverstripe.org/en/4/developer_guides/security/secure_coding/#security-headers. If we add this to Page_Controller rather than BasePageController, it only applies to new projects automatically - so doesn't constitute a semver breakage that'll catch out sites which aren't on HTTPS already.

Aside: In the CWP ecosystem, Incapsula provides default certs, so every site has the ability to be served with or without TLS/SSL. But not all sites can enforce SSL only, because third party client-side assets aren't available via SSL, and would cause mixed content warnings. The one example I had in mind (http://www.journeys.nzta.govt.nz/ using Metservice APIs) has since been fixed. We could treat this as an exception rather than the rule, and allow opt-out?

Mention this in the relevant docs:

Pull Requests

madmatt commented 5 years ago

We should ideally add this to htaccess rather than in PHP code, so that it also applies to all requests for static files on CWP. However that's a little more annoying as it's harder to control based on environment.

In particular, you'd normally want to exclude dev environments from enforcing this - while provisioners like Vagrant and Docker could handle this for people automatically during bootstrapping, I don't think they're in common enough use.

Note: The above applies more for the Director::forceSSL() call than the Strict-Transport-Security header application, because STS only kicks in if the site you're on is delivered over https. So you need to have both Director::forceSSL() and HTTPResponse->addHeader('Strict-Transport-Security', 'max-age=x') headers to actually make this work.

In the past, we've done this via a middleware/request processor rather than in PageController, so it applies everywhere including the CMS, and then backed that up with htaccess rules, although realistically only one is really necessary.

madmatt commented 5 years ago

Aside: In the CWP ecosystem, Incapsula provides default certs, so every site has the ability to be served with or without TLS/SSL.

This is true for the default domains (so UAT is fine for example), but production is always served from a client domain and that does require the purchase and installation of certificates.

This will be very important to highlight clearly and in lots of places - if we enforce this, then the process of 'go-live' is more risky because if you don't have an HTTPS cert, your golive will fail, and it can't be tested in advance because of Incapsula / CNAME shenanigans making it impossible to test the site from the actual production domain ahead of actually making the DNS change.

chillu commented 5 years ago

This is true for the default domains (so UAT is fine for example), but production is always served from a client domain and that does require the purchase and installation of certificates.

@madmatt Are you sure? I thought Incapsula had multi-domain SNI certs for all domains configured there automatically.

madmatt commented 5 years ago

They can be configured, but are not done so automatically by Incapsula [1]. The CWP Service Desk team can add Lets Encrypt certificates, a custom cert, or the Incap-generated SNI certificate, but none are done by default (and we probably can't do any of them by default without offending/annoying some agencies - many have rules around where/how certificates can be purchased, and who within the agency can do it).

[1] https://www.imperva.com/blog/add-ssl-support-to-incapsula-protected-site/

chillu commented 5 years ago

OK I've added three pull requests to enable this by default for new projects created with cwp/installer:2.4, release announcements which encourage existing projects to adopt this, and permanent docs in "working with projects: security".

Note that I've chosen to set up defaults to max-age: 300, and added inline docs to the config encouraging devs to increase that once they're happy that it runs fine in prod. More rationale about why and where I've added this in the main PR at https://github.com/silverstripe/cwp-core/pull/73/commits/95193dccbddef29f373e4232a4ef2a218b6af0bb

List of PRs:

robbieaverill commented 5 years ago

All merged, thanks @chillu