Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure nginx, matching current Apache config #8

Merged
merged 30 commits into from Sep 8, 2017
Merged

Conversation

sideshowbarker
Copy link
Contributor

No description provided.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments, have to make waffles now.

@@ -0,0 +1,16 @@
#!/bin/bash -e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you delete all the apache files and renumber these? They're in git history if we want to go back. certbot will need some tweaking. On bgsound I have mkdir -p /var/www/http/.well-known/acme-challenge/ in 00-nginx-conf and certbot certonly -n --agree-tos --webroot -m admin@whatwg.org -d $domain -w /var/www/http in 01-certbot. There will be a TODO about reloading nginx when new certs are issued too.

chown -R deploy:deploy /var/www
rm -rf /var/www/html

/usr/sbin/nginx -t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the path needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/usr/sbin/nginx -t

why is the path needed?

Not needed—I’ll change it to just nginx

rm -rf /var/www/html

/usr/sbin/nginx -t
systemctl start nginx.service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be running after the install, so restart. And is this different from nginx -s reload?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be running after the install, so restart.

OK

And is this different from nginx -s reload?

Yes, reload causes it to just re-read/re-evaluate the config, without restarting. But if nginx is for some reason not running at the point, then reload won’t start it but I guess will just fail.

But even if nginx is stopped restart will cause it start (after loading the config).

So it seems like restart is what we do want here, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nginx -s reload fails with nginx: [error] open() "/run/nginx.pid" failed (2: No such file or directory) if it's not running. I found sudo systemctl reload-or-restart nginx.service which seems to work even if not running, and with any luck it'll reload when possible. I'm guessing that'll avoid dropping ongoing connections.

cp nginx/sites/*.conf /etc/nginx/sites-available/

for domain in `cat DOMAINS | tr , ' '`; do
ln -s /etc/nginx/sites-available/$domain.conf /etc/nginx/sites-enabled/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nicer than what I'd been doing, thanks!

ln -s /etc/nginx/sites-available/$domain.conf /etc/nginx/sites-enabled/
done

/usr/sbin/nginx -t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/usr/sbin/nginx -t

Ditto questions.

Changed to just nginx and restart there too

server {
listen 80 default_server;
listen [::]:80 default_server;
server_name _;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server_name _;

Is this needed?

I’m actually not sure. The nginx docs say:

If no server_name is defined in a server block then nginx uses the empty name as the server name.

So I’ve always the convention of setting the name to _ and can say from experience I know that’s never caused any problems—but can’t say I know from experience I know that leaving it unset so the “empty name” gets used doesn’t cause any problems.

But anyway I’ll test it and see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server_name _;

Is this needed?

I’ll test it and see.

I’ve now tested and can’t find that it causes any problems. From reading the docs and thinking about it, I guess the only thing it would ever affect is requests that don’t send the Host header. So that’s something we don’t need to care about anyway—since no browsers ever do that, and it’s non-conforming anyway (in HTTP 1.1 and 2 at least). And nginx itself rejects such requests with a 400 Bad Request before ever evaluating against the config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this or omitting it is fine, whatever you think is clearer.

listen 80 default_server;
listen [::]:80 default_server;
server_name _;
return 301 https://$host$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the webroot certbot thing work, we'll need:

  location ^~ /.well-known/acme-challenge/ {
    default_type application/jose+json;
  }

  location / {
    return 301 https://$host$request_uri;
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, we don't redirect other URLs than / or does this mean something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is at http://nginx.org/en/docs/http/ngx_http_core_module.html#location, didn't actually read it, but this syntax amounts to a prefix match, not an exact match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the webroot certbot thing work, we'll need:

 location ^~ /.well-known/acme-challenge/ {
   default_type application/jose+json;
 }

 location / {
   return 301 https://$host$request_uri;
 }

OK, added

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super excited about this, and about learning more about nginx.

@@ -0,0 +1,13 @@
server {
listen 443 ssl http2;
listen [::]:443 ssl http2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move these to whatwg.conf?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or http.conf? What's the difference between http and whatwg?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think http.conf is for not-HTTPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listen 443 ssl http2;
listen [::]:443 ssl http2;

Can we move these to whatwg.conf?

No we can’t. Each server block needs to have its own listen directive; otherwise the server gets defaulted to port 80:

https://nginx.org/en/docs/http/ngx_http_core_module.html#listen

If the directive is not present then either *:80 is used if nginx runs with the superuser privileges, or *:8000 otherwise.

https://www.digitalocean.com/community/tutorials/understanding-nginx-server-and-location-block-selection-algorithms#parsing-the-quot-listen-quot-directive-to-find-possible-matches

By default, any server block that does not include a listen directive is given the listen parameters of 0.0.0.0:80 (or 0.0.0.0:8080 if Nginx is being run by a normal, non-root user).

And there’s no config option to override that; it’s hardcoded into the nginx sources:

https://github.com/nginx/nginx/blob/3900d1cb3cf39963d909604923f7e7af6ecf99fd/src/http/ngx_http_core_module.c#L2775-L2784

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or http.conf? What's the difference between http and whatwg?

I think http.conf is for not-HTTPS.

Yeah, the only purpose of http.conf is just to redirect non-HTTPS to HTTPS.

But as far as why both http.conf and whatwg.conf are needed: because http.conf gets put into the /etc/nginx/conf.d directory, and anything that’s put there gets included once in the main top-level nginx http {…} block. But the stuff in whatwg.conf needs to get included into multiple places, in the server {…} blocks for each virtual server (domain).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each server block needs to have its own listen directive

Huh, and including such a directive via include doesn't mean "having" it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each server block needs to have its own listen directive

Huh, and including such a directive via include doesn't mean "having" it?

Ah, I hadn’t thought about just putting it in whatwg.conf. But I’ve now moved it there and it works find that way, so I’ll commit that change—thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that work out, can you push those changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that work out, can you push those changes?

It did work, and I’ve now pushed the changes I‘ve made so far. I will wrap up the rest and push them later today.

server_name books.spec.whatwg.org;

ssl_certificate /etc/letsencrypt/live/books.spec.whatwg.org/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/books.spec.whatwg.org/privkey.pem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move these to whatwg.conf and use some kind of templating to determine the file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssl_certificate /etc/letsencrypt/live/books.spec.whatwg.org/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/books.spec.whatwg.org/privkey.pem; 

Could we move these to whatwg.conf and use some kind of templating to determine the file name?

No, because nginx doesn’t itself have any kind of templating mechanism or static macros for use in config files. So I think the only way to do that would be to use a runtime variable in the ssl_certificate and ssl_certificate_key values—specifically, $server_name —if that even worked, which it doesn’t.

To confirm that, I tried putting ssl_certificate /etc/letsencrypt/live/${server_name}/fullchain.pem and ssl_certificate_key /etc/letsencrypt/live/${server_name/privkey.pem in whatwg.conf and removing from all the sites/*.conf files. That caused nginx to fail at startup with BIO_new_file("/etc/letsencrypt/live/${server_name}/fullchain.pem").

Because nginx evaluates that value statically at startup, I guess at that point ${server_name} is undefined or null, since it’s something that only gets set at runtime.

But even if it worked, I think the problem there is that it would be expensive in that nginx variables are basically all runtime variables that require a lookup per-request.

The nginx docs have an FAQ that discusses the general issue:

https://nginx.org/en/docs/faq/variables_in_config.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great, thanks for the education!

It does make me think we should perhaps be generating these files using some templating system, but we can do that later, if ever. Not a big deal for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make me think we should perhaps be generating these files using some templating system

Agreed, that would be better for maintenance

but we can do that later, if ever. Not a big deal for now.

Yeah, I think once we finally have all the domains moved over, we can go and back and set something up that we can design and test with all cases as a complete set

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished my review, looking good!


apt install nginx

# a2dissite doesn't work if nothing is enabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was for the apache config, left behind.

# a2dissite doesn't work if nothing is enabled
rm -f /etc/nginx/sites-enabled/*

cp -p nginx/conf/http.conf /etc/nginx/conf.d/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why preserve ownership etc.? Isn't it better if these files are owned by root?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cp -p nginx/conf/http.conf /etc/nginx/conf.d/

Why preserve ownership etc.?

No good reason. Will drop the -p

Isn't it better if these files are owned by root?

yes

rm -rf /var/www/html

/usr/sbin/nginx -t
systemctl start nginx.service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nginx -s reload fails with nginx: [error] open() "/run/nginx.pid" failed (2: No such file or directory) if it's not running. I found sudo systemctl reload-or-restart nginx.service which seems to work even if not running, and with any luck it'll reload when possible. I'm guessing that'll avoid dropping ongoing connections.

server {
listen 80 default_server;
listen [::]:80 default_server;
server_name _;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this or omitting it is fine, whatever you think is clearer.

listen 80 default_server;
listen [::]:80 default_server;
server_name _;
return 301 https://$host$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is at http://nginx.org/en/docs/http/ngx_http_core_module.html#location, didn't actually read it, but this syntax amounts to a prefix match, not an exact match.


include /etc/nginx/whatwg.conf;

return 301 https://books.idea.whatwg.org$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For idea↔spec redirects I think we should use HTTP 302, as 301 may be cached, and we might move things between. This was a bit of a nuisance with https://whatwg.org/faq.


include /etc/nginx/whatwg.conf;

return 301 https://figures.idea.whatwg.org$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest 302 per above.


include /etc/nginx/whatwg.conf;

return 301 https://w3c.github.io/DOM-Parsing$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this matches the apache config, but maybe we should omit $request_uri, as basically nothing in https://github.com/whatwg/domparsing exists in https://github.com/w3c/DOM-Parsing


include /etc/nginx/whatwg.conf;

return 301 https://html.spec.whatwg.org/dev;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a slash to avoid another redirect.

ssl_certificate /etc/letsencrypt/live/fetch.spec.whatwg.org/fullchain.pem;
ssl_certificate_key /etc/letsencrypt/live/fetch.spec.whatwg.org/privkey.pem;

include /etc/nginx/whatwg.conf;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://fetch.spec.whatwg.org/commit-snapshots/ is now 403 forbidden but wasn't before. @domenic, does it matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I browse that directory index pretty often for the various specs; it would be nice to have one if nginx has that feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sideshowbarker, maybe a whatwg-spec.conf that does the extra things?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should have a directory listing of sorts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://fetch.spec.whatwg.org/commit-snapshots/ is now 403 forbidden but wasn't before. @domenic, does it matter?

I browse that directory index pretty often for the various specs; it would be nice to have one if nginx has that feature

OK, fixed that and now all /commit-snapshots paths are browsable.

autoindex_exact_size off;
}

rewrite /work https://html.spec.whatwg.org/multipage/microdata.html#licensing-works;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://n.whatwg.org/workz and https://n.whatwg.org/bla/work are also redirected with this. I guess that's harmless, but maybe ^/work$ or a location = /work { return ... } would be better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be location most likely. We don't want to redirect all the things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://n.whatwg.org/workz and https://n.whatwg.org/bla/work are also redirected with this. I guess that's harmless, but maybe ^/work$ or a location = /work { return ... } would be better?

Yeah, this should be location most likely. We don't want to redirect all the things.

OK, fixed.

@foolip
Copy link
Member

foolip commented Sep 4, 2017

@sideshowbarker, can you make sure 6b72735 (mimesniff and xhr) are also included? Those should be the last of the current "easy" ones, the rest have something special about deploy to resolve first.

@foolip
Copy link
Member

foolip commented Sep 4, 2017

@sideshowbarker, https://resources.whatwg.org/fonts/README.md now has the wrong content-type, application/octet-stream.

@sideshowbarker
Copy link
Contributor Author

@sideshowbarker, can you make sure 6b72735 (mimesniff and xhr) are also included? Those should be the last of the current "easy" ones, the rest have something special about deploy to resolve first.

Yup, added

@sideshowbarker
Copy link
Contributor Author

https://resources.whatwg.org/fonts/README.md how has the wrong content-type, application/octet-stream

Yeah, see https://github.com/nginx/nginx/blob/master/conf/mime.types. Apparently, by default nginx doesn’t recognize .md files as text/plain.

So, added a mapping in whatwg.conf

@sideshowbarker
Copy link
Contributor Author

OK, I’ve responded to all review comments here—so please re-review and merge if there are no new changes needed.

I think #8 (comment) (what cache headers+values we should send) is the only remaining thing from the review comments here that might still require further changes. And I’ve raised #10 for that, so if there are no new changes needed for anything else here otherwise, I suggest that we not block on that caching question but instead go ahead and merge this.

This overrides 03acfb9 to replaces the ssl_ciphers value with the
current value from using “Intermediate” at
https://mozilla.github.io/server-side-tls/ssl-config-generator/ instead
of “Modern”—because using the “Modern” ssl_ciphers value also apparently
disable both TLSv1.1 and TLSv1.0 (which it’s not clear we agree we want
to do yet).
@sideshowbarker
Copy link
Contributor Author

I found sudo systemctl reload-or-restart nginx.service which seems to work even if not running, and with any luck it'll reload when possible.

OK—thanks for finding that, switched to using it

I'm guessing that'll avoid dropping ongoing connections.

I thought a plain restart would also not drop any ongoing connections but would instead wait for them to close before stopping and restarting

@foolip
Copy link
Member

foolip commented Sep 5, 2017

I thought a plain restart would also not drop any ongoing connections but would instead wait for them to close before stopping and restarting

Could be, I haven't done any testing.

@foolip
Copy link
Member

foolip commented Sep 5, 2017

return 301 https://$host$request_uri;

Documentation is at http://nginx.org/en/docs/http/ngx_http_core_module.html#location, didn't actually read it, but this syntax amounts to a prefix match, not an exact match

OK, so is there some change you think we still need to make there?

That was about http.conf, and I think that's fine.

chown -R deploy:deploy /var/www
rm -rf /var/www/html

mkdir -p /var/www/http/.well-known/acme-challenge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say put this before the chown, but maybe it's just as well that it's after so that only root can create files there? On marquee it's currently owned by the deploy user, so one needs to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say put this before the chown, but maybe it's just as well that it's after so that only root can create files there? On marquee it's currently owned by the deploy user, so one needs to change.

I’ve moved it before the chown. It doesn’t seem like having only root be able to create files there needs to be a big concern

@@ -2,6 +2,6 @@

apt install certbot python-certbot-apache

for domains in `cat DOMAINS`; do
certbot certonly -n --apache --agree-tos -m admin@whatwg.org -d $domains
for domain in `cat DOMAINS`; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be plural, because it can be a value like "whatwg.org,www.whatwg.org" and documentation says "-d DOMAINS Comma-separated list of domains to obtain a cert for"

@@ -2,6 +2,6 @@

apt install certbot python-certbot-apache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop python-certbot-apache. Also rebase and rm -rf debian/marquee/apache/, some things are still left.


include whatwg-headers.conf;

location ~* \.(?:ico|css|js|gif|jpe?g|png|svg)$ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, let's figure it out separately.

include whatwg-headers.conf;
}

location /commit-snapshots {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

branch-snapshots too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

autoindex_exact_size off;
}

include mime.types;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about why this is necessary given that it's also included in the main nginx.conf?


include mime.types;
types {
text/plain md;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran find *whatwg.org -type f -name '*.*' | sed 's#.*/##' | sed 's/.*[.]//' | sort | uniq on the existing servers and found these file extensions:

appcache
cgi
css
gif
gz
htaccess (ignore)
html
ico
include (ignore)
jpeg
jpg
js
json
map
md
mp4
ogv
old
pdf
png
sh
svg
swp
ttf
txt
woff
woff2
xml
zip

Can you check that all of these have the correct type still?

(Also looked for extension-less files, updated description of #7 to not forget them. There are none affected by this PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran find *whatwg.org -type f -name '*.*' | sed 's#.*/##' | sed 's/.*[.]//' | sort | uniq on the existing servers and found these file extensions:

Can you check that all of these have the correct type still?

OK, I went through and checked all those and added any that were missing.

Some comment and questions:

cgi

We don’t need anything for .cgi, right?

map

What is that?

ogv

Added video/ogg per https://www.iana.org/assignments/media-types/video/ogg

old

What is that?

swp

Don’t think we need that :)

ttf

Added font/ttf per https://www.iana.org/assignments/media-types/font/ttf

woff2

Added font/woff2 per https://www.iana.org/assignments/media-types/font/woff2

ttf, woff2, and ogv lack mappings in /etc/apache2/mods-available/mime.conf too, so I left comments in whatwg.conf to make it clear where the mappings come from

@sideshowbarker
Copy link
Contributor Author

I think #8 (comment) (what cache headers+values we should send) is the only remaining thing from the review comments here that might still require further changes. And I’ve raised #10 for that, so if there are no new changes needed for anything else here otherwise, I suggest that we not block on that caching question but instead go ahead and merge this

In d5dd1f6 I’ve removed that one ad-hoc thing I had there that was adding Expires and Cache-Control. It’s not essential and whatever we add for the long term will just fall out from the #10 investigation and discussions

See mozilla/server-side-tls#135

> proper rotation of session ticket encryption key is not
> implemented in nignx or Apache. Thus it is easier to recommend against
> its use that suggest use of 3rd party software to fix it.
> The problem is not that they are insecure, or that they can't be made
> secure. The problem is that the way they are commonly implemented means
> that you don't provide forward secrecy if you use them - all encryption
> keys are ultimately encrypted with just one encryption key - the session
> ticket key.
>
> see here for more in-depth explanation:
> https://www.imperialviolet.org/2013/06/27/botchingpfs.ht
@sideshowbarker
Copy link
Contributor Author

https://mozilla.github.io/server-side-tls/ssl-config-generator/?
I think we should do modern btw and disable TLSv1.

If we do change to Modern, then we should also remember to remove the ssl_dhparam line:

ssl_dhparam /etc/ssl/certs/dhparam.pem;

Because with the Modern settings, that won’t be used at all any longer, so we could just drop it.

This change sets ssl_session_timeout & ssl_session_cache to the values
recommended by https://mozilla.github.io/server-side-tls/ssl-config-generator/

But I’m not sure what the rationale for those recommended values is, so
I’ve also raised mozilla/server-side-tls#198
in hopes of getting an explanation.
@sideshowbarker
Copy link
Contributor Author

I’ve changed ssl_session_timeout and ssl_session_cache to the values recommended at https://mozilla.github.io/server-side-tls/ssl-config-generator/ but I’m not sure what the specific rationale for those recommended values is, so I’ve also raised mozilla/server-side-tls#198

@foolip
Copy link
Member

foolip commented Sep 6, 2017

FWIW, I think we should err on the side of supporter older clients (keeping TLSv1) when we're just serving static content, can we have an issue about that change and start out matching the current config?

@annevk
Copy link
Member

annevk commented Sep 6, 2017

I guess we can start out conservative, sure. My main argument for going with TLSv12 only is that we're supposed to set an example and I'd expect all our end users to have modern hardware and software.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor nits.

gzip_comp_level 6;
gzip_buffers 16 8k;
gzip_http_version 1.1;
gzip_types text/plain text/css application/json application/x-javascript text/xml application/xml image/svg+xml application/xml+rss text/javascript;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove application/x-javascript as we don't care for it?


include /etc/nginx/whatwg.conf;

return 301 https://github.com/tc39/ecma262/labels/web%20reality;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this a 302?


include /etc/nginx/whatwg.conf;

return 301 https://w3c.github.io/DOM-Parsing/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

302?


include /etc/nginx/whatwg.conf;

return 301 https://wicg.github.io/mediasession/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

302?


include /etc/nginx/whatwg.conf;

return 301 https://w3c.github.io/webvtt$request_uri;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

302?


include whatwg-headers.conf;

location /commit-snapshots {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous comment has been collapsed, so: branch-snapshots too.

}
location / {
return 301 https://$host$request_uri;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, an important detail is missing here:

root /var/www/http;

Had to put that on marquee to get a new cert.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, thanks Mike!

@sideshowbarker sideshowbarker merged commit 526311f into master Sep 8, 2017
@sideshowbarker sideshowbarker deleted the mike/nginx branch September 8, 2017 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants