Add security headers #597

sgrif · 2017-03-06T19:59:55Z

These headers are all pretty straightforward except CSP. For CSP I
defined the sources based on what was loaded from visiting the main page
and all crates. Images should be safe, so I've allowed them from all
sources.

This should be checked on staging before deploying.

Fixes #586.

carols10cents · 2017-03-08T14:57:32Z

I have this up on staging now, the deploy went fine, the observatory now gives staging an A+, installing a crate via cargo from staging worked fine. Anything else we should check?

sgrif · 2017-03-08T17:07:06Z

Not that I can think of.

sgrif · 2017-03-08T17:11:39Z

Actually I'm seeing a console error when I run it locally. ~~It looks like google analytics is trying to use unsafe eval.~~

We have an inline script tag from google analytics that I guess we need to fix.

g-k · 2017-04-13T16:57:10Z

👍 Looks good.

Putting the inline script in its own file with the async ga analytics.js should workaround the error.

diff --git a/app/index.html b/app/index.html
index 3de7338..a4ebf8c 100644
--- a/app/index.html
+++ b/app/index.html
@@ -7,15 +7,8 @@

     {{content-for 'head'}}

-    <script>
-      (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
-      (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
-      m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
-      })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
-
-      ga('create', 'UA-58390457-3', 'auto');
-      ga('send', 'pageview');
-    </script>
+    <script src="{{rootURL}}assets/ga-pageview.js"></script>
+    <script async src="https://www.google-analytics.com/analytics.js"></script>

     <link rel="stylesheet" href="{{rootURL}}assets/vendor.css">
     <link rel="stylesheet" href="{{rootURL}}assets/cargo.css">
diff --git a/dist/assets/ga-pageview.js b/dist/assets/ga-pageview.js
new file mode 100644
index 0000000..68f8501
--- /dev/null
+++ b/dist/assets/ga-pageview.js
@@ -0,0 +1,3 @@
+window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date;
+ga('create', 'UA-58390457-3', 'auto');
+ga('send', 'pageview');

bluetech · 2017-05-18T09:08:21Z

Just a note that you should be careful with Strict-Transport-Security; I don't know if someone runs crates.io privately, but if they do, and expose it to users, then setting HSTS as done here is a commitment to only use HTTPS for 1 year for the domain and all subdomains.

You might want to make this one opt-in, unless this is really not a concern.

This should not be taken as being against HSTS - https://crates.io should definitely use it (and looks like it already does, though without includeSubdomains).

carols10cents · 2017-06-06T15:12:43Z

Just a note that you should be careful with Strict-Transport-Security; I don't know if someone runs crates.io privately, but if they do, and expose it to users, then setting HSTS as done here is a commitment to only use HTTPS for 1 year for the domain and all subdomains.

You might want to make this one opt-in, unless this is really not a concern.

I don't feel like I have enough knowledge in this area to make this call :-/ I'll try to do some research around this soon.

carols10cents · 2017-06-06T15:13:23Z

@sgrif are you up for making this suggested change to handle the GA script in the meantime though?

g-k · 2017-06-09T00:03:14Z

+1 on rolling out HSTS slowly as mentioned here.

I don't remember the exact value, but services usually start with a low HSTS max-age (minutes) and make sure everything keeps working for awhile before bumping it to a year (like DNS changes).

vignesh-sankaran · 2017-07-11T02:55:59Z

What's the status on this pull request? What needs to be done to unblock this?

carols10cents · 2017-07-11T12:37:32Z

What's the status on this pull request? What needs to be done to unblock this?

I think the ember folks changed google analytics recently, I'm not sure if it's now async or not or whatever we need google analytics to be-- that needs to be researched and changed and tested if necessary.

I need to read up on HSTS and understand the implications before deploying-- that's unfortunately not something you can unblock for me :)

carols10cents · 2017-07-11T20:47:57Z

Ok reading a bit about HSTS on these sites:

it sounds like until we get https://doc.crates.io working (see #621), we definitely should NOT be sending includeSubdomains, which means we're not eligible to be preload either.

This should not be taken as being against HSTS - https://crates.io should definitely use it (and looks like it already does, though without includeSubdomains).

Indeed it is! Running this:

curl -v 'https://crates.io/' -H 'Accept: text/html'

shows this in the response headers:

< Strict-Transport-Security: max-age=31536000

Looks like this is happening because we use the nginx buildpack and we've specified that in config/nginx.conf.erb. People deploying crates.io on their own servers may not be using that file at all, and if they do, they can choose to remove or change that line.

So I don't think we need to be setting the STS headers in the spot that this PR does.

HOWEVER, mozilla observatory is not seeing this. In their "raw server headers", I see "Content-Type | application/json; charset=utf-8", which means they're not requesting a content type, and crates.io defaults to JSON which returns a 404 for /:

$ curl -v https://crates.io
* Rebuilt URL to: https://crates.io/
*   Trying 50.16.231.196...
* TCP_NODELAY set
* Connected to crates.io (50.16.231.196) port 443 (#0)
* TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate: crates.io
* Server certificate: Let's Encrypt Authority X3
* Server certificate: DST Root CA X3
> GET / HTTP/1.1
> Host: crates.io
> User-Agent: curl/7.51.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Connection: keep-alive
< Server: nginx
< Date: Tue, 11 Jul 2017 20:45:58 GMT
< Content-Type: application/json; charset=utf-8
< Content-Length: 35
< Set-Cookie: cargo_session=--3Mz45cH3itfVkmY41+nVjWRm9+0=; HttpOnly; Secure; Path=/
< Via: 1.1 vegur
<
* Curl_http_done: called premature == 0
* Connection #0 to host crates.io left intact
{"errors":[{"detail":"Not Found"}]}

Soo it looks like we need to solve #163 before we can get the observatory to see that :-/

jtgeibel · 2017-07-22T00:44:49Z

src/http.rs

+            );
+            response.headers.insert(
+                "Strict-Transport-Security".into(),
+                vec!["max-age=31536000; includeSubDomains".into()],


Assuming that #895 is merged, this should be removed. This should probably be configured further up the stack anyway since mirrors could be deployed over HTTP.

If we want to continue setting the header here then as mentioned by @carols10cents includeSubdomains should be removed.

jtgeibel · 2017-07-22T01:07:17Z

src/lib.rs

@@ -150,6 +150,9 @@ pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
    m.add(conduit_cookie::Middleware::new(app.session_key.as_bytes()));
    m.add(conduit_cookie::SessionMiddleware::new("cargo_session",
                                                 env == Env::Production));
+    if env == Env::Production {
+        m.add(http::SecurityHeadersMiddleware);
+    }


Its unclear to me exactly what the order of middleware layers will be. Lower in the file dist::Middleware is added with around so I think that static files will be served without these headers. In particular, that would include index.html. The security middleware should probably be the outermost middleware if it isn't already.

URLs matching ^/assets/ will be served directly by nginx, but static files in the root (such as index.html and robots.txt) are served through this static dist middleware. I have no idea if any of these headers should also be applied to static assets.

Since these headers were picked up when this was deployed on staging it looks like the middleware layers are stacked in the correct order.

Given that in production most static assets are served directly from nginx, I think X-Content-Type-Options: nosniff should also be included in nginx.conf.erb. Looking over the other headers, I don't think any of the rest apply to such assets (except HSTS).

vignesh-sankaran

Just going through the PRs to get documentation comments added for new types :)

vignesh-sankaran · 2017-07-25T09:07:45Z

src/http.rs

@@ -76,3 +78,38 @@ pub fn token(token: String) -> Token {
        token_type: String::new(),
    }
 }
+
+pub struct SecurityHeadersMiddleware;


Could we add a documentation comment here?

895: Enable HSTS headers on all responses r=carols10cents **Deployment Warning:** This pull request uses a configuration syntax added in nginx-1.7.5 and requires switching to a new buildpack. ~~If this pull request is merged the buildpack configuration on *staging* and *production* will need to be changed on Heroku.~~ (This is no longer a concern with the updated PR as this is specified in `.buildpacks`.) It is unclear if there are any existing mirror deployments on Heroku or otherwise depending on our `nginx.conf.erb` template. This pull request enables HSTS headers for all responses. My tests show an upgrade from an F to a D on the Mozilla Observatory. (Out of 100 points, this scores 35 instead of 15.) Please also see the discussion in #597 which is what prompted my review. This pull request also fixes the buildpack URLs defined in `app.json` so that the Heroku deploy button works. # Nginx buildpack upgrade The previous buildpack was based on nginx-1.5.7 which was released in November of 2013. Fortunately for us, it looks like travis-ci recently cloned this buildpack and upgraded it to the latest release of 1.13.3. Note that the odd minor version number represents that this (as well as our previous release) is from the development branch. The stable release of 1.12.1 may be preferable, but I expect that the travis-ci clone is our best bet for a responsive upstream at this time. # Nginx configuration change Two changes to the nginx configuration file where necessary. See http://nginx.org/en/docs/http/ngx_http_headers_module.html for more details on both of these. ## Use the always parameter The `always` parameter was introduced in nginx-1.7.5. The HSTS header is now included for all response codes. ## Duplicate header declaration in both blocks This surprised me, but the documentation states: > There could be several add_header directives. These directives are inherited from the previous level **if and only if** there are no add_header directives defined on the current level. I figured it was best to explicitly duplicate the declaration rather than rely on inheritance at all. # Fix the Deploy to Heroku button This pull request also fixes the Deploy to Heroku button in `docs/MIRROR.md` by updating the `app.json` file. ~~It appears that Heroku does not support URLs with the #SHA fragment. If we want to fix ourselves to specific commits for the buildpacks it may be possible to do so with `archive/*.tar.gz` links, but I suspect that we are currently targeting the master branches for these in production anyway.~~ The PR now sets up `app.json` to use heroku-buildpack-multi which will pull in the other buildpacks from `.buildpacks`. New mirror deployments should now match our staging and production environments.

These headers are all pretty straightforward except CSP. For CSP I defined the sources based on what was loaded from visiting the main page and all crates. Images should be safe, so I've allowed them from all sources. This should be checked on staging before deploying. Fixes rust-lang#586.

More stuff that goes with 1aecb45.

carols10cents · 2017-08-12T03:44:33Z

Ok. I poked at this a bit more, it's up on https://staging-crates-io.herokuapp.com right now and gets an A+.

There were some content security policy violations, but not from google analytics:

We had some SVGs with inline style attributes, that's not allowed
We had an iframe, that's not allowed (but we weren't using it anyway)
Since this PR was made, we added AJAX requests to docs.rs so those needed to be whitelisted

The only problem I still see and haven't figured out yet is that the google chart on crate pages (ex: https://staging-crates-io.herokuapp.com/crates/blah-types) isn't working because google's stuff uses eval :( I'd rather not go through all this trouble of adding these headers and then just have to allow eval anyway :( (afaik you have to allow eval for scripts/styles from any domain, not a whitelist)

I tried upgrading to the "new" way of loading google charts because this comment claimed the "current" version (in march) didn't use eval, but as far as i could tell it does use eval today :(

I guess some headers are better than none? Idk. I'm sleepy and out of ideas.

Always leave a note.

carols10cents · 2017-08-12T14:32:02Z

Adding unsafe-eval lets the charts work. It gets an A-, still better than D. There are no more errors reported in Chrome; Firefox still complains about an overflow: hidden (on crate pages only) but I can't tell where it's coming from. Nothing looks broken though.

carols10cents · 2017-08-12T17:16:03Z

@kureuil says this works in Edge (thank you!!!), so let's do this.

bors: r+

kureuil · 2017-08-12T17:19:48Z

@carols10cents I couldn't find anything about the FF Nightly warning, but everything seems to be working perfectly though :)

597: Add security headers r=carols10cents These headers are all pretty straightforward except CSP. For CSP I defined the sources based on what was loaded from visiting the main page and all crates. Images should be safe, so I've allowed them from all sources. This should be checked on staging before deploying. Fixes #586.

bors-voyager · 2017-08-12T17:39:30Z

Build succeeded

continuous-integration/travis-ci/push

ghost · 2019-10-17T06:05:05Z

src/http.rs

+                      script-src 'self' 'unsafe-eval' \
+                                 https://www.google-analytics.com https://www.google.com; \


Upstream issue seems fixed, is this still needed?

carols10cents added the blocked label Mar 18, 2017

carols10cents assigned sgrif Mar 24, 2017

carols10cents added the S-waiting-on-author label Jun 6, 2017

jtgeibel mentioned this pull request Jul 22, 2017

Enable HSTS headers on all responses #895

Merged

jtgeibel requested changes Jul 22, 2017

View reviewed changes

jtgeibel reviewed Jul 22, 2017

View reviewed changes

vignesh-sankaran reviewed Jul 25, 2017

View reviewed changes

sgrif and others added 2 commits August 9, 2017 20:04

Remove STS header setting from middleware; it's set in nginx config

ed3d856

carols10cents force-pushed the sg-security-headers branch from 013eb66 to ed3d856 Compare August 10, 2017 00:06

carols10cents added 6 commits August 9, 2017 20:26

Add missing derives that we lint for now

d09f79c

cargo fmt

99a7931

Move inline svg styles to CSS because of Content Security Policy

b0d8c48

Remove more code related to unused download button

68b9175

More stuff that goes with 1aecb45.

Allow docs.rs to make ajax requests

9141e31

Allow stylesheets for charts

be92c84

Give up and unsafe eval

93ad0d8

Leave a note about why we need unsafe-eval for now

3cba145

Always leave a note.

bors-voyager bot merged commit 3cba145 into rust-lang:master Aug 12, 2017

carols10cents mentioned this pull request Sep 14, 2017

Recheck Mozilla's Observatory after #586 is fixed #585

Closed

sgrif deleted the sg-security-headers branch September 26, 2017 21:43

Turbo87 removed the S-waiting-on-author label Dec 3, 2017

ghost reviewed Oct 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security headers #597

Add security headers #597

sgrif commented Mar 6, 2017

carols10cents commented Mar 8, 2017

sgrif commented Mar 8, 2017

sgrif commented Mar 8, 2017 •

edited

Loading

g-k commented Apr 13, 2017

bluetech commented May 18, 2017

carols10cents commented Jun 6, 2017

carols10cents commented Jun 6, 2017

g-k commented Jun 9, 2017

vignesh-sankaran commented Jul 11, 2017

carols10cents commented Jul 11, 2017

carols10cents commented Jul 11, 2017

jtgeibel Jul 22, 2017

jtgeibel Jul 22, 2017

jtgeibel Jul 22, 2017

vignesh-sankaran left a comment

vignesh-sankaran Jul 25, 2017

carols10cents commented Aug 12, 2017

carols10cents commented Aug 12, 2017 •

edited

Loading

carols10cents commented Aug 12, 2017

kureuil commented Aug 12, 2017

bors-voyager bot commented Aug 12, 2017

ghost Oct 17, 2019

		script-src 'self' 'unsafe-eval' \
		https://www.google-analytics.com https://www.google.com; \

Add security headers #597

Add security headers #597

Conversation

sgrif commented Mar 6, 2017

carols10cents commented Mar 8, 2017

sgrif commented Mar 8, 2017

sgrif commented Mar 8, 2017 • edited Loading

g-k commented Apr 13, 2017

bluetech commented May 18, 2017

carols10cents commented Jun 6, 2017

carols10cents commented Jun 6, 2017

g-k commented Jun 9, 2017

vignesh-sankaran commented Jul 11, 2017

carols10cents commented Jul 11, 2017

carols10cents commented Jul 11, 2017

jtgeibel Jul 22, 2017

Choose a reason for hiding this comment

jtgeibel Jul 22, 2017

Choose a reason for hiding this comment

jtgeibel Jul 22, 2017

Choose a reason for hiding this comment

vignesh-sankaran left a comment

Choose a reason for hiding this comment

vignesh-sankaran Jul 25, 2017

Choose a reason for hiding this comment

carols10cents commented Aug 12, 2017

carols10cents commented Aug 12, 2017 • edited Loading

carols10cents commented Aug 12, 2017

kureuil commented Aug 12, 2017

bors-voyager bot commented Aug 12, 2017

Build succeeded

ghost Oct 17, 2019

Choose a reason for hiding this comment

sgrif commented Mar 8, 2017 •

edited

Loading

carols10cents commented Aug 12, 2017 •

edited

Loading