Navigation Menu

Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

feat(router): enable HSTS when enforceHTTPS is set #3866

Merged
merged 3 commits into from Jun 18, 2015
Merged

feat(router): enable HSTS when enforceHTTPS is set #3866

merged 3 commits into from Jun 18, 2015

Conversation

wenzowski
Copy link
Contributor

When browsers see the HSTS header on an HTTPS request then they rewrite
all links for the current domain that point at HTTP resources to point
to HTTPS resources. When /deis/router/enforceHTTPS is set, using HSTS
avoids the extranneous 301 redirect to the HTTPS resource and prevents
some threats. The HTTPS Strict Transport Security header mechanism
is defined in RFC-6797

@wenzowski
Copy link
Contributor Author

The add_header mechanism would set Strict-Transport-Security blindly for both HTTP and HTTPS requests. Since the header is only valid on HTTPS requests a map is used in this PR to ensure it is only set on HTTPS requests.

The always keyword ensures the header field will be added regardless of the HTTP response code.

@bacongobbler
Copy link
Member

Hmm. I'm wondering if this should be hidden behind a feature flag or not, considering that this is an opt-in security enhancement. \cc @deis/core-maintainers

@wenzowski
Copy link
Contributor Author

I chose this to be triggered by /deis/router/enforceHTTPS since it really is an extension of that: hsts forces the browser to use https on the domain for the specified number of seconds unless a subsequent request overwrites the maxAge. There should probably be a way to opt out of the hsts header but still enforce 301 redirects to https, even if the default is to trigger this with the same key.

@croemmich
Copy link
Contributor

This is great, I was just thinking about doing this yesterday! I'm with @bacongobbler, this should probably be behind a feature flag. Some user's may not want it and others may not understand it and break things if they decide to disable SSL.

Maybe use these keys instead?
/deis/router/hsts/enabled (default: false)
/deis/router/hsts/maxAge (default: 2628000)
/deis/router/hsts/includeSubDomains (default: false)

Now, off to create a pull request for the Public-Key-Pins header :)

@wenzowski
Copy link
Contributor Author

Great suggestions @croemmich, updated.

I've updated the default maxAge so the preload key will produce a header that is compatible with the Chrome preload list

@wenzowski
Copy link
Contributor Author

I also don't see a use case for disabling enforceHTTPS but enabling HSTS, so setting /deis/router/hsts/enabled to true also enables enforceHTTPS but /deis/router/enforceHTTPS no longer enables HSTS so this is now completely backwards compatible.

@wenzowski
feat(router): enable HSTS when enforceHTTPS is set
b1a52f4 
When browsers see the HSTS header on an HTTPS request then they rewrite
all links for the current domain that point at HTTP resources to point
to HTTPS resources. When /deis/router/enforceHTTPS is set, using HSTS
avoids the extranneous 301 redirect to the HTTPS resource and prevents
[some threats][1]. The HTTPS Strict Transport Security header mechanism
is defined in [RFC-6797][2]

[1]: https://www.owasp.org/index.php/HTTP_Strict_Transport_Security
[2]: https://tools.ietf.org/html/rfc6797
@wenzowski
feat(router): use one folder for all HSTS keys
8cb5028
@wenzowski
feat(router): set default HSTS maxAge to 18 weeks for preload list co…
08c84ed 
…mpat
@wenzowski wenzowski changed the title feature(router): enable HSTS when enforceHTTPS is set feat(router): enable HSTS when enforceHTTPS is set Jun 16, 2015
@mboersma
Copy link
Member

so setting /deis/router/hsts/enabled to true also enables enforceHTTPS but /deis/router/enforceHTTPS no longer enables HSTS so this is now completely backwards compatible.

That sounds like the way to go. Code LGTM. This needs some manual testing IMHO to ensure Strict-Transport-Security is working as we think it is on a real cluster with SSL.

@carmstrong
Copy link
Contributor

Code LGTM.

@mboersma mboersma added this to the v1.8 milestone Jun 18, 2015
@mboersma
Copy link
Member

Tested, LGTM.

@mboersma mboersma self-assigned this Jun 18, 2015
mboersma added a commit that referenced this pull request Jun 18, 2015
@mboersma
Merge pull request #3866 from dialoghq/feature/HSTS
11a6698 
feat(router): enable HSTS when enforceHTTPS is set
@mboersma mboersma merged commit 11a6698 into deis:master Jun 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@mboersma mboersma

Labels
requires-manual-testing router security
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wenzowski @bacongobbler @croemmich @mboersma @carmstrong