Add security related headers to build.servo.org #511

jarondl · 2016-10-17T20:10:42Z

Add some headers to build.servo.org to improve our Mozilla Observatory
score. The helps the cause of #473, by raising our score from F (0) to a
whopping B- (65). The testing is still basic, the docker-observatory
based testing will go in a different PR.

To get to an A+ we must first implement https.

This is the local observatory report:

Score Rule                       Description
      -20 redirection                Does not redirect to an https site.
      -20 strict-transport-security  HTTP Strict Transport Security (HSTS)
        header cannot be set for sites not available over https.
       -5 contribute                 Contribute.json file missing from root
        of website.
        5 content-security-policy    Content Security Policy (CSP)
        implemented without 'unsafe-inline' or 'unsafe-eval'.
        5 x-frame-options            X-Frame-Options (XFO) implemented via
        the CSP frame-ancestors directive.

    Score: 65
    Grade: B-

This change is


        Add security related headers to build.servo.org

Add some headers to `build.servo.org` to improve our Mozilla Observatory score. The helps the cause of #473, by raising our score from F (0) to a whopping B- (65). The testing is still basic, the docker-observatory based testing will go in a different PR. To get to an A+ we must first implement https. This is the local observatory report: Score Rule Description -20 redirection Does not redirect to an https site. -20 strict-transport-security HTTP Strict Transport Security (HSTS) header cannot be set for sites not available over https. -5 contribute Contribute.json file missing from root of website. 5 content-security-policy Content Security Policy (CSP) implemented without 'unsafe-inline' or 'unsafe-eval'. 5 x-frame-options X-Frame-Options (XFO) implemented via the CSP frame-ancestors directive. Score: 65 Grade: B-

highfive · 2016-10-17T20:10:45Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

aneeshusa · 2016-10-17T20:14:00Z

@jarondl Can you point me to some documentation about the contribute.json file? I haven't heard of this and didn't see it on the observatory results.

jarondl · 2016-10-17T21:53:45Z

Yes, contribute.json is quite obscure. I have found out about it only when I've browsed through the observatory code to resolve some issues.
According to the observatory, only Mozilla related websites must have it, see for example:

https://observatory.mozilla.org/analyze.html?host=mozilla.org

The list of mozilla related sites is hard coded here: https://github.com/mozilla/http-observatory/blob/7dd04ff5b51e39698a8b683df33ef430b4ed4213/httpobs/conf/httpobs.conf#L33
and servo is not there, but in all fairness it should be. We should submit a PR to add it, and lower our grade from 70 to 65.

jarondl · 2016-10-18T02:03:34Z

I believe the tests have failed because buildbot is down in travis. This should be solved by #505, right?

aneeshusa · 2016-10-30T06:49:21Z

Buildbot immediately stops on Travis after being started (due to our fake test credentials not validating IIRC), which I don't have an easy fix for. In the meantime, I believe this is why I added the make_cors_request function in #375, since Nginx should still add the headers even though the status code indicates error (since Buildbot is down). Try adding/using that in this test.

aneeshusa

Sorry about the delay in reviewing. The headers themselves and the test case look good modulo a few nits, the Salt side needs a little more work.

aneeshusa · 2016-10-30T07:04:59Z

nginx/https_headers.conf

@@ -0,0 +1,15 @@
+# The following headers were added to pass the mozilla http observatory


nit: mozilla -> Mozilla

aneeshusa · 2016-10-30T07:04:59Z

nginx/https_headers.conf

@@ -0,0 +1,15 @@
+# The following headers were added to pass the mozilla http observatory
+# test suit.


spelling: suit -> suite

aneeshusa · 2016-10-30T07:04:59Z

nginx/https_headers.conf

+# All of them are very simple in this case, because both homu and buildbot
+# do not use external resources.
+
+# CSP is the most important one


The options we're setting are fairly basic and IMO the headers are readable enough as is that we don't need an expository comment for each one.

aneeshusa · 2016-10-30T07:04:59Z

nginx/init.sls

@@ -14,6 +14,15 @@ nginx:
    - watch_in:
      - service: nginx

+/etc/nginx/conf.d/https_headers.conf:
+  file.managed:
+    - source: salt://nginx/https_headers.conf


Use salt://{{ tpldir }}/files/https_headers.conf, and make a files subdir to put the config file in.

I'm not sure how well you know Salt, but we'll also want the /etc/nginx/conf.d state from #375 and some requisites to order things correctly/restart nginx automatically on config change. I can also take care of this if you'd like.

aneeshusa · 2016-10-30T07:04:59Z

tests/sls/nginx/headers.py

+        failures = []
+        for header in expected_headers:
+            if header not in actual_headers:
+                failures.append('Missing or changed header - {}:{}'.format(*header))


This line is failing tidy because it is too long.

aneeshusa · 2016-10-30T07:04:59Z

tests/sls/nginx/headers.py

+            "default-src 'self'; frame-ancestors 'none'"),
+        ('X-Frame-Options', 'DENY'),
+        ('X-XSS-Protection', '1; mode=block'),
+        ('X-Content-Type-Options', 'nosniff')]


style nit: leave a trailing comma after this last tuple, and put the closing ] by itself on the next line.

aneeshusa · 2016-10-30T07:04:59Z

tests/sls/nginx/headers.py

+                failures.append('Missing or changed header - {}:{}'.format(*header))
+
+    if len(failures) > 0:
+        return Failure('nginx is serving wrong securirty headers',


spelling: securirty -> security

aneeshusa · 2016-10-30T07:06:42Z

Also, please open a separate issue about contribute.json to keep this PR about headers.

jarondl · 2016-10-30T15:51:38Z

Hi @aneeshusa , thanks for reviewing, I will work on it.

aneeshusa · 2018-01-07T22:15:45Z

I'm going to close this as it's been inactive for a while; anyone is welcome to pick this work up again.

highfive assigned aneeshusa Oct 17, 2016

highfive added the S-awaiting-review label Oct 17, 2016

jarondl mentioned this pull request Oct 20, 2016

add http-observatory tests #518

Closed

aneeshusa suggested changes Oct 30, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 30, 2016

aneeshusa closed this Jan 7, 2018

servo / saltfs

Add security related headers to build.servo.org #511

Add security related headers to build.servo.org #511

jarondl commented Oct 17, 2016 •

edited

highfive commented Oct 17, 2016

aneeshusa commented Oct 17, 2016

jarondl commented Oct 17, 2016 •

edited

jarondl commented Oct 18, 2016 •

edited

aneeshusa commented Oct 30, 2016

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Oct 30, 2016

jarondl commented Oct 30, 2016

aneeshusa commented Jan 7, 2018

		@@ -0,0 +1,15 @@
		# The following headers were added to pass the mozilla http observatory

		@@ -0,0 +1,15 @@
		# The following headers were added to pass the mozilla http observatory
		# test suit.

servo / saltfs

Join GitHub today

Add security related headers to build.servo.org #511

Add security related headers to build.servo.org #511

Conversation

jarondl commented Oct 17, 2016 • edited

highfive commented Oct 17, 2016

aneeshusa commented Oct 17, 2016

jarondl commented Oct 17, 2016 • edited

jarondl commented Oct 18, 2016 • edited

aneeshusa commented Oct 30, 2016

aneeshusa left a comment

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

This comment has been minimized.

aneeshusa Oct 30, 2016

aneeshusa commented Oct 30, 2016

jarondl commented Oct 30, 2016

aneeshusa commented Jan 7, 2018

jarondl commented Oct 17, 2016 •

edited

jarondl commented Oct 17, 2016 •

edited

jarondl commented Oct 18, 2016 •

edited