cloudflare / sslconfig

Cloudflare's Internet facing SSL configuration
BSD 3-Clause "New" or "Revised" License
1.3k stars 132 forks source link

Update NGINX SPDY patch for 1.9.15 #36

Open felixbuenemann opened 8 years ago

felixbuenemann commented 8 years ago

This updates the patch to work with NGINX 1.9.15 and also fixes a problem with the previous patch, that did not re-enable the spdy_* directives that were marked as deprecated by the http2 module as well as fixing a compile problem when the http2 module is enabled, but spdy is disabled.

felixbuenemann commented 8 years ago

Btw. the patch also applies to 1.11.0 with some fuzz, but I have only tested it on 1.9.15.

felixbuenemann commented 8 years ago

@jgrahamc If you want to keep the 1.9.7 patch around, I could also rename the updated patch and send a separate bugfix for the spdy directives in the old patch.

jgrahamc commented 8 years ago

This is cool. Can you submit an updated patch instead so we can keep the old one around as well?

felixbuenemann commented 8 years ago

@jgrahamc I have separated the patch and fixed the non-working spdy_* directives in the 1.9.7 patch. I have also renamed the old patch to add the version number in order to avoid confusion.

felixbuenemann commented 8 years ago

Btw. I tested the 1.9.15 patch against 1.11.0 and it works fine. Current stable 1.10.0 is identical to 1.9.15 so it'll work too.

kn007 commented 8 years ago

Using with both --with-http_spdy_module and --with-http_v2_module, it's work. Thank you.


But when I test this patch in 1.9.15 and 1.11.0, it's return same error when without both --with-http_spdy_module and --with-http_v2_module :

src/http/modules/ngx_http_ssl_module.c: In function  ngx_http_ssl_npn_advertised :
src/http/modules/ngx_http_ssl_module.c:445:5: error: expected expression before  }  token
make[1]: *** [objs/src/http/modules/ngx_http_ssl_module.o] Error 1

src/http/modules/ngx_http_ssl_module.c :

 397 static int
 398 ngx_http_ssl_npn_advertised(ngx_ssl_conn_t *ssl_conn,
 399     const unsigned char **out, unsigned int *outlen, void *arg)
 400 {
 401 #if (NGX_HTTP_V2 || NGX_HTTP_SPDY || NGX_DEBUG)
 402     ngx_connection_t  *c;
 403
 404     c = ngx_ssl_get_connection(ssl_conn);
 405     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "SSL NPN advertised");
 406 #endif
 407
 408 #if (NGX_HTTP_V2 || NGX_HTTP_SPDY)
 409     {
 410     ngx_http_connection_t  *hc;
 411
 412     hc = c->data;
 413 #endif
 414
 415 #if (NGX_HTTP_V2 && NGX_HTTP_SPDY)
 416     if (hc->addr_conf->http2 && hc->addr_conf->spdy) {
 417         *out = (unsigned char *) NGX_HTTP_V2_NPN_ADVERTISE
 418                                  NGX_SPDY_NPN_ADVERTISE
 419                                  NGX_HTTP_NPN_ADVERTISE;
 420         *outlen = sizeof(NGX_HTTP_V2_NPN_ADVERTISE NGX_SPDY_NPN_ADVERTISE
 421                          NGX_HTTP_NPN_ADVERTISE) - 1;
 422
 423         return SSL_TLSEXT_ERR_OK;
 424     } else
 425 #endif
 426 #if (NGX_HTTP_V2)
 427     if (hc->addr_conf->http2) {
 428         *out =
 429             (unsigned char *) NGX_HTTP_V2_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE;
 430         *outlen = sizeof(NGX_HTTP_V2_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE) - 1;
 431
 432         return SSL_TLSEXT_ERR_OK;
 433     } else
 434 #endif
 435 #if (NGX_HTTP_SPDY)
 436     if (hc->addr_conf->spdy) {
 437         *out = (unsigned char *) NGX_SPDY_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE;
 438         *outlen = sizeof(NGX_SPDY_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE) - 1;
 439
 440         return SSL_TLSEXT_ERR_OK;
 441     }
 442 #endif
 443
 444 #if (NGX_HTTP_V2 || NGX_HTTP_SPDY)
 445     }
 446 #endif
 447
 448     *out = (unsigned char *) NGX_HTTP_NPN_ADVERTISE;
 449     *outlen = sizeof(NGX_HTTP_NPN_ADVERTISE) - 1;
 450
 451     return SSL_TLSEXT_ERR_OK;
 452 }
 453
 454 #endif

gcc (GCC) 4.9.1 20140922 (Red Hat 4.9.1-10)

del line 409 & 445, it's work.

frostieDE commented 8 years ago

Works fine against nginx 1.11.0 and OpenSSL 1.0.2h (with ChaCha20 patch). I am building nginx as an Ubuntu 16.04 package.

Thanks for your work @felixbuenemann

felixbuenemann commented 8 years ago

@kn007 I believe the extra block was added for C89 compatibility, where all variables have do be declared at the start of a block. I'm not sure why that leads to a compile error for you. One thing that looks problematic about the above code is that if only HTTP2 is compiled, there's an unmatched else branch. I think the block can be avoided by re-ordering the init and debug log code.

felixbuenemann commented 8 years ago

@kn007 Oh, I just noticed I misread your first comment, you were talking about the case where only --with-http_v2_module is enabled. I'll look into that.

felixbuenemann commented 8 years ago

@kn007 I have updated the 1.9.7 and 1.9.15 patches to fix the compile problem --with-http_v2_module but without --with-http_spdy_module.

Please do not use your fix removing lines 409 and 445, it is wrong and will only work by accident.

Thanks for the report!

felixbuenemann commented 8 years ago

@jgrahamc I have kept the 1.9.7 patch fixes separate. I can combine them into a single commit and/or move them over to a separate PR if you prefer.

kn007 commented 8 years ago

@felixbuenemann Got it, thank you. That's very kind of you.

felixbuenemann commented 8 years ago

@jgrahamc It would be nice if the nginx spdy patch was mentioned in the README.md. I can't speak on behalf of CloudFlare, so it's best if you did that. A short paragraph linking to the blog post would probably be sufficient.

xetorixik commented 8 years ago

The least I can do is write a big thank you for your time, effort and eventually the SPDY patch for Nginx 1.10 @felixbuenemann

kn007 commented 8 years ago

nginx 1.11.1 test pass

angristan commented 8 years ago

Not working anymore for me :

src/http/modules/ngx_http_ssl_module.c: In function ‘ngx_http_ssl_npn_advertised’:
src/http/modules/ngx_http_ssl_module.c:445:5: error: expected expression before ‘}’ token
     }
     ^
objs/Makefile:1170: recipe for target 'objs/src/http/modules/ngx_http_ssl_module.o' failed
make[1]: *** [objs/src/http/modules/ngx_http_ssl_module.o] Error 1
make[1]: *** Attente des tâches non terminées....
make[1]: Leaving directory '/opt/nginx-1.11.1'
Makefile:8: recipe for target 'build' failed

But still working with https://raw.githubusercontent.com/felixbuenemann/sslconfig/b8ebac6a337e8e4e373dfee76e7dfac3cc6c56e6/patches/nginx_1_9_15_http2_spdy.patch

felixbuenemann commented 8 years ago

@Angristan Can't reproduce, works fine for me on 1.11.1 with only ssl module, ssl + http2, ssl + spdy and ssl + http2 + spdy. So you'd have to be a bit more specific.

Maybe the patch got applied incorrectly?

This script should work:

#!/bin/sh
set -e
git clone https://github.com/felixbuenemann/sslconfig.git
cd sslconfig
git checkout updated-nginx-1.9.15-spdy-patch
cd ..
git clone https://github.com/nginx/nginx.git
cd nginx
git checkout release-1.11.1
git apply ../sslconfig/patches/nginx_1_9_15_http2_spdy.patch
./auto/configure --prefix=/opt/nginx-1.11.1 \
  --with-http_ssl_module \
  --with-http_spdy_module \
  --with-http_v2_module
make
angristan commented 8 years ago

I'm using the patch this way : https://github.com/Angristan/nginx-autoinstall/blob/master/nginx-autoinstall.sh#L420

kn007 commented 8 years ago

@Angristan Please using the laster version patch and try it again: https://raw.githubusercontent.com/felixbuenemann/sslconfig/updated-nginx-1.9.15-spdy-patch/patches/nginx_1_9_15_http2_spdy.patch

See this: https://github.com/cloudflare/sslconfig/pull/36#issuecomment-222386934

angristan commented 8 years ago

@kn007 this one is working thx

RaeesBhatti commented 8 years ago

How do you test if it is actually using SPDY? Chrome doesn't support it anymore and Firefox says it's using HTTP/1.1

HansVanEijsden commented 8 years ago

@raeesiqbal NGINX Amplify gives insights: https://amplify.nginx.com/

felixbuenemann commented 8 years ago

You can test with Firefox by disabling http2 for the port in nginx or add another port with only spdy.

angristan commented 8 years ago

@raeesiqbal with https://www.ssllabs.com/ssltest/index.html and the openssl command from the blog article https://blog.cloudflare.com/open-sourcing-our-nginx-http-2-spdy-code/

felixbuenemann commented 8 years ago

Firefox also add s a fake(?) header X-Firefox-Spdy in the developer console which shows the SPDY or HTTP2 version used.

RaeesBhatti commented 8 years ago

Oh yeah. It has a header X-Firefox-SPDY: 3.1, which means that it is working. Thanks everyone.

kn007 commented 8 years ago

@raeesiqbal Testing like this: https://spdycheck.org/#kn007.net Also can get protocols from ssllabs.com

angristan commented 8 years ago

@kn007 I think this site doesn't work very well, it says my website does not use HTTPS :') https://spdycheck.org/#angristan.fr

FernandoMiguel commented 8 years ago

@Angristan cause it's HTTP2 and not 1.1 SDPY screenshot 2016-06-14 17 02 02

HansVanEijsden commented 8 years ago

But https://spdycheck.org/#www.weblogzwolle.nl uses SPDY and HTTP/2... also that same error. Same with all my other sites and servers.

kn007 commented 8 years ago

@Angristan I don't know why, maybe because your site support Forward Secrecy?

Or something notice from spdycheck

Notice:
Many of the SPDY implementations out there are starting to switch from NPN to APLN negotiation. SPDYCheck does not yet support APLN negotiation. Sites like Facebook and Twitter have different SPDY implementations deployed behind load balancers, which can lead to some results saying SPDY is supported while others saying it is not. If you receive a results that you know is incorrect. Try running the check again.

I test your site in ssllab, it was return h2 spdy/3.1 http/1.1.

So, maybe spdycheck doesn't work very well.

angristan commented 8 years ago

@FernandoMiguel

root@lyra ~# openssl s_client -connect angristan.fr:443 -nextprotoneg ''
CONNECTED(00000003)
Protocols advertised by server: h2, spdy/3.1, http/1.1

@kn007 Yep that's maybe ALPN

kn007 commented 8 years ago

@HansVanEijsden Maybe because you and @Angristan website support Forward Secrecy.

HansVanEijsden commented 8 years ago

@kn007 that must be it. Thanks.

kn007 commented 8 years ago

@Angristan my website also support ALPN and NPN, I guess maybe because Forward Secrecy.

kn007 commented 8 years ago

@Angristan @HansVanEijsden Got the reseon, spdycheck not support ECC CA. I test few website with ECC CA, return failed.

felixbuenemann commented 8 years ago

You could use NGINX 1.11+ multiple certificate support to supply both ECC and RSA certificates, so that older clients can connect to your server.

angristan commented 8 years ago

@felixbuenemann Thoses who do not support ECC certs are thoses who do not support TLS 1.2 so I prefer to avoid them ^^

kn007 commented 8 years ago

@wangqiliang 请用英文。而且不要以链接方式来评论。你可以尝试使用Google Translate来附上你想评论的内容。

wangqiliang sending a feedback about when using IE9 to visit website with nginx 1.11.1&spdy patch, sometime will return ~4-bits anomaly contents and TTFB has reached 30000ms.

When not using spdy patch, it was quite normal.

ghost commented 8 years ago

@kn007 Sorry, it is my fault. I am sending a feedback that when I access the port 80 which I set a spdy parameter in the ngx/1.11.1 with patch, there is unexpected ~4-bit contents output with a timeout (in Firefox 46, 30000ms) . ( I should correct a mistake. It was not a 30000-ms-TTFB but a ERR_TIME_OUT.) when I am not using this patch, it returns 301 to HTTPS normally.

kn007 commented 8 years ago

@wangqiliang It's OK. Please excuse any mistakes for my english.

felixbuenemann commented 8 years ago

@wangqiliang You can only use spdy on port 80, if you have a load balancer like haproxy in front of your nginx which handles SSL termination and ALPN/NPN negotiation. If you use spdy or http2 on a port without ssl in NGINX, it will always use that protocol without negotiation.

So the simple answer is, don't set spdy on port 80, only use it on 443.

ghost commented 8 years ago

@felixbuenemann It's so kind of you. I will check for it later.

ghost commented 8 years ago

@felixbuenemann @kn007 From Wikepedia,

For use within HTTPS, SPDY needs the TLS extension Next Protocol Negotiation (NPN),[24] thus browser and server support depends on the HTTPS library.

So that is my fault. Thanks for your kindness!

kn007 commented 7 years ago

Do not work in 1.11.5. Found a repo to fix it: https://github.com/cujanovic/nginx-http2-spdy-patch

fmunteanu commented 7 years ago

@kn007, that repo patch does not work with you add dynamic modules.

natnet00 commented 7 years ago

@felixbuenemann Your patch works flawlessly up to version 1.11.4 But starting with nginx 1.11.5 the patch unfortunately gives errors... would be great if this could be fixed.

felixbuenemann commented 7 years ago

@natnet00 Have you tried the patch linked by kn007 above?

natnet00 commented 7 years ago

@felixbuenemann nope have not tried it yet

lenovouser commented 7 years ago

@felixbuenemann both also doesn't work with 1.11.6 :/