1468621 - symbols.mozilla.org is sending HTTP headers twice

Status: RESOLVED FIXED

(Tecken :: General, task)

Description

[April King [:April]]

symbols.mozilla.org is sending a couple HTTP headers twice, breaking their function:

$ curl -I -L 'https://symbols.mozilla.org/'
...
x-content-type-options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
x-xss-protection: 1; mode=block
X-XSS-Protection: 1; mode=block
...

Could the errant additional "x-content-type-options" and "x-xss-protection" please be removed?

Thanks!

Comment 1

[Peter Bengtsson [:peterbe]]

It must be because the Django part [0] sets these AND the nginx config [1] sets these.

I can solve this either day. I can rip out any security header stuff from Django. Or, I can remove it from the cloudops-deployment config. But I'm not sure what's best [practice]. 

If I keep it entirely in Django I can more easily control it with (Django) unit testing etc. And if I keep it entirely in cloudop's nginx it's one less thing to worry about in Django. 


[0] https://github.com/mozilla-services/tecken/blob/12d267c8057f06dac848398fdfba91723e21d13a/tecken/settings.py#L111
[1] https://github.com/mozilla-services/cloudops-deployment/blob/8544448b225099481c9261e6b367df9f3953b3bb/projects/symbols/puppet/modules/symbols/templates/http_symbols.conf.erb#L80-L85

Comment 2

[Greg G]

Usually we return security headers from nginx when the application doesn't set them (e.g. https://github.com/mozilla-services/cloudops-deployment/blob/2e077a10e415138efea6839cb592328d110dd300/libs/puppet/modules/openresty/templates/conf.d/http_dockerflow.erb#L23-L37). This is useful to include them in nginx error responses.

Comment 3

[Greg G]

Attachment: use conditional sec headers for location /

Comment 4

[Peter Bengtsson [:peterbe]]

Neat! After that was merged, I merged a PR so it upgraded Dev. However, it did NOT appear to have fixed it. :(


▶ curl -I https://symbols.dev.mozaws.net
HTTP/1.1 200 OK
...
strict-transport-security: max-age=31536000; preload
Strict-Transport-Security: max-age=31536000
x-content-type-options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
x-xss-protection: 1; mode=block
X-XSS-Protection: 1; mode=block
...

Comment 5

[Greg G]

(In reply to Peter Bengtsson [:peterbe] from comment #4)
> Neat! After that was merged, I merged a PR so it upgraded Dev. However, it
> did NOT appear to have fixed it. :(
> 
> 
> ▶ curl -I https://symbols.dev.mozaws.net
> HTTP/1.1 200 OK
> ...
> strict-transport-security: max-age=31536000; preload
> Strict-Transport-Security: max-age=31536000
> x-content-type-options: nosniff
> X-Content-Type-Options: nosniff
> X-Frame-Options: DENY
> x-xss-protection: 1; mode=block
> X-XSS-Protection: 1; mode=block
> ...

Ugh I thought nginx or openresty would normalize the header names, but I guess we need case-insensitive comparison on the sec header name. Actually I'll just downcase the nginx headers to match.

Comment 6

[Greg G]

Attachment: downcase nginx sec header names

Comment 7

[Peter Bengtsson [:peterbe]]

Greg and I chatted about it and decided that the "new best practice" is that the security headers responsibility belongs in config. I.e. the puppet/nginx config in cloudops-deployment. 

So with that in mind I'm going to remove the Django security middleware.

Comment 8

[Peter Bengtsson [:peterbe]]

Attachment: GitHub PR

Comment 9

[Peter Bengtsson [:peterbe]]

https://github.com/mozilla-services/tecken/commit/8cc6c77ec6981c7a3924b2ed0d8f2a2138c32d53

That worked for Dev :)

▶ curl -I https://symbols.dev.mozaws.net
HTTP/1.1 200 OK
Cache-Control: max-age=3600
Cache-Control: no-cache, no-store, must-revalidate, private
Content-Length: 560
Content-Security-Policy: font-src 'self'; object-src 'none'; script-src 'self'; img-src 'self'; default-src 'self'; style-src 'self'; connect-src 'self'; frame-ancestors 'none'; report-uri /__cspreport__
Content-Type: text/html
Date: Thu, 14 Jun 2018 18:48:18 GMT
Last-Modified: Thu, 14 Jun 2018 18:25:34 GMT
Pragma: no-cache
Strict-Transport-Security: max-age=31536000
Vary: Cookie
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-XSS-Protection: 1; mode=block
Connection: keep-alive

Comment 10

[Peter Bengtsson [:peterbe]]

I'm going to test this in Stage and try to push for a Prod deploy next week after the All-hands.

Comment 11

[Greg G]

(In reply to Peter Bengtsson [:peterbe] from comment #7)
> Greg and I chatted about it and decided that the "new best practice" is that
> the security headers responsibility belongs in config. I.e. the puppet/nginx
> config in cloudops-deployment. 
> 
> So with that in mind I'm going to remove the Django security middleware.

Give me a chance to run it by the rest of secops next week.

The benefits of having the app handle the security headers are: the app can unit test them, consistency with other services, and having a fallback if nginx is misconfigured.

Comment 12

[Peter Bengtsson [:peterbe]]

Mind you, I don't really like the idea of writing unit tests. It's not business logic. It's 90% stock Django and 10% configuration. No actual code. Meaning, it's not important to write "unit tests" for it.

Comment 13

[Greg G]

OK so the feedback from the secops meeting was: as long as one copy of the headers is present on all pages and we can check it in before prod we don't care whether nginx or the app serves them.

This might vary per header e.g. HSTS, HPKP, XCTO from nginx and CSP from the app, but however the ops and devs decide to do that for a service such that it minimizes mistakes works for us.

Also, we're planning to add a duplicate security header check to the baseline to catch this specific bug.

Comment 14

[Peter Bengtsson [:peterbe]]

Awesome! Let's do it this way going forward. Devs don't have to worry about security headers but the baseline nginx config will make sure it works either way. 

Now I'm just waiting for a stage or prod deploy to make sure we made the right changes. Then we can close this bug.

Comment 15

[Miles Crabill [:miles]]

As of right now https://symbols.stage.mozaws.net/__heartbeat__ returns a 500 and warns "You do not have 'django.middleware.security.SecurityMiddleware' in your MIDDLEWARE so the SECURE_HSTS_SECONDS, SECURE_CONTENT_TYPE_NOSNIFF, SECURE_BROWSER_XSS_FILTER, and SECURE_SSL_REDIRECT settings will have no effect."

Comment 16

[Miles Crabill [:miles]]

This is after I deployed a recently tagged version of tecken that I presume has the change Peter mentioned in Comment 7.

Comment 17

[Will Kahn-Greene [:willkg] ET needinfo? me]

Moving Tecken service bugs to Tecken component.

Comment 18

[Peter Bengtsson [:peterbe]]

Fixed in Prod now.


▶ curl -I -L 'https://symbols.mozilla.org/'
HTTP/1.1 200 OK
Cache-Control: max-age=3600
Content-Length: 560
content-security-policy: font-src 'self'; object-src 'none'; script-src 'self'; img-src 'self'; default-src 'self'; style-src 'self'; connect-src 'self'; frame-ancestors 'none'; report-uri /__cspreport__
Content-Type: text/html
Date: Mon, 25 Jun 2018 18:21:25 GMT
Last-Modified: Fri, 22 Jun 2018 18:56:58 GMT
pragma: no-cache
strict-transport-security: max-age=31536000
Vary: Cookie
x-content-type-options: nosniff
x-frame-options: DENY
x-xss-protection: 1; mode=block
Connection: keep-alive

Comment 19

[Will Kahn-Greene [:willkg] ET needinfo? me]

Moving from Socorro product to Tecken product.