search

Chocobozzz / PeerTube

ActivityPub-federated video streaming platform using P2P directly in your web browser
https://joinpeertube.org/
GNU Affero General Public License v3.0
13.09k stars 1.51k forks source link

Peertube is sending more than one HSTS header #1034

Closed Nutomic closed 6 years ago

Nutomic commented 6 years ago

See these SSL scan results: https://www.ssllabs.com/ssltest/analyze.html?d=peertube.video https://www.ssllabs.com/ssltest/analyze.html?d=peertube.mastodon.host&s=163.172.7.58

I had the same error on Peertube.social, but after commenting out the following line in the nginx config, I get a perfect SSL score:

  #add_header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload";

https://www.ssllabs.com/ssltest/analyze.html?d=peertube.social

I have no idea where the duplicate header is coming from, as the Peertube repo and /etc/nginx only contain the header once (both in the nginx config). Peertube shouldn't send this header on it's own, as the site admin should be able to decide if HSTS is enabled, and for how long.

rigelk commented 6 years ago

It is due to our recent switch to helmet, which adds HSTS headers automatically by default. Doing it in PeerTube allows a less complex reverse-proxy configuration. Admittedly in Nginx it's simple to setup but other reverse proxies less so (looking at you, Traefik).

We should probably remove the line from the Nginx config.

Nutomic commented 6 years ago

This seems like a bad idea, because it prevents admins from changing the HSTS parameters. For example, I want to add my site to the HSTS preload list, but this is only possible if I can edit the header and add the preload flag. As the website says, this should not be set by default.

https://hstspreload.org/

rigelk commented 6 years ago

Following their advice we shouldn't even include the line in the reverse-proxy config.

Nutomic commented 6 years ago

That seems fine to me, because the admin can easily change or remove it. But I don't think it's possible to change the header inside Peertube (or it would be very complicated).

rigelk commented 6 years ago

Indeed. And it seems important to not even include it in the Nginx configuration, as this is something that should be tested first by the admin apparently.

Nutomic commented 6 years ago

We can leave it in, but just comment it out by default (and put a comment explaining it).

rigelk commented 6 years ago

Done in https://github.com/Chocobozzz/PeerTube/commit/6328da8c017cf00d3c0ac8824ec5af128f6db42e :slightly_smiling_face: