search

deis / router

Edge router for Deis Workflow
https://deis.com
MIT License
80 stars 57 forks source link

feat(router): Add annotation for disabling http2 #225

Closed mattk42 closed 8 years ago

mattk42 commented 8 years ago

I realized today that HTTP2 was causing issues specifically with Firefox for my apps. Seems that being able to toggle HTTP2 could be useful for others down the road as well.

deis-admin commented 8 years ago

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

deis-bot commented 8 years ago

@felixbuenemann, @krancour and @arschles are potential reviewers of this pull request based on my analysis of git blame information. Thanks @mattk42!

codecov-io commented 8 years ago

Current coverage is 45.42% (diff: 100%)

Merging #225 into master will increase coverage by 0.17%

@@             master       #225   diff @@
==========================================
  Files             3          3          
  Lines           305        306     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            138        139     +1   
  Misses          162        162          
  Partials          5          5

Powered by Codecov. Last update c1585eb...f12d8bf

krancour commented 8 years ago

@mattk42 #222 might address your issue. Would you consider testing with a router built from that branch to see if it helps you?

mattk42 commented 8 years ago

@krancour I can definitely give that a shot, it would be great to know if that helps.

I do still believe that it may be worth it to support disabling HTTP2. The first to come to mind are the recent vulnerabilities discovered with the protocol https://www.nginx.com/blog/the-imperva-http2-vulnerability-report-and-nginx/

krancour commented 8 years ago

Jenkins, add to whitelist.

felixbuenemann commented 8 years ago

@mattk42 I though about adding a flag when I added this feature, but didn't think about potential bugs in nginx :-(

I've been running spdy and later http/2 on nginx for quite some time and never ran into issues.

I think upgrading to a newer NGINX is the way to go, but it doesn't hurt to be able to disable HTTP/2 as well. I would leave it enabled by default at least as soon as the issue are resolved because it does improve many aspects of page load performance, especially when loading lots of small resources and because http/2 can reprioritize resources on the fly.

mattk42 commented 8 years ago

@felixbuenemann Yeah I definitely agree on leaving it on by default, which should be how this PR behaves. I know I worked on an application in the past that fell over as soon as I enabled http2. It was definitely due to poor design, but more concurrent requests just killed it. So I could see the ability to disable being potentially useful in some cases.

@krancour I am going to try out the nginx update this morning just for the sake of knowing that it solved the issue as well.

felixbuenemann commented 8 years ago

Btw. I checked the router's SSL configuration against dev.ssllabs.com and noticed some problems with the SSL configuration that should be addressed. One problem is that no strong DH param is used, which prohibits use of forward secrecy for older clients that do not yet support ECDHE. The other is a bad cipher suite selection, which leads to cipher negotiation which is blacklisted in the HTTP/2 spec, most notably the server only offers ECDHE CBC SHA1 but no SHA256 or SHA384 CBC ciphers.

I'll try to look into that once I find some free time and propose a better cipher suite selection. Generating a strong DH param would probably have to be handled in the DEIS charts.

@mattk42 Where you issues by any chance related to Firefox wversion prior to 49 with the error NS_ERROR_NET_INADEQUATE_SECURITY?

According to the SSL-Labs report the bad cipher selection affects Firefox 46 and 47 on Windows 7. I also checked on my Mac with FF 46.0b4 (nothing happened, so probably negotiation failed), FF 48.0 (Error: NS_ERROR_NET_INADEQUATE_SECURITY), FF 49.0b2 (negotiated TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 and worked fine) and FF 50.0a2 (OK, same result).

So it seems to me the problem might be related to a bad SSL config and not bugs in the HTTP/2 implementation.

mattk42 commented 8 years ago

@felixbuenemann Such perfect timing, I was actually just going over that with my boss. When we were seeing those issues yesterday, two of us were on FF 47 and it just white-screened. One other person (I don't know what version they were on) actually got back the NS_ERROR_NET_INADEQUATE_SECURITY), sounds like they may have been on 48 from your description.

I realized at the time that it was likely a combination if the negotiated cipher and HTTP2. Seems like FF has different cipher rules when HTTP2 is involved. The ssllabs scan I ran yesterday is what pointed me towards that, and disabling HTTP2 for the time being was a bit easier than mucking around with the cipher list. I should have mentioned that earlier, so thanks for pointing it out now.

felixbuenemann commented 8 years ago

I just noticed that the cipher list can be set through an annotation, I'll try that out.

felixbuenemann commented 8 years ago

@mattk42 Firefox 48.0 connects just fine if you use kubectl edit --namespace=deis rc/deis-router to add the following annotation:

metadata:
  annotations:
    router.deis.io/nginx.ssl.ciphers: ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA:EDH-RSA-DES-CBC3-SHA:DES-CBC3-SHA

This could ne shortened, but there's currently a bug in the regex used to verify the ciiphers value, so ! cannot be used to deny groups of ciphers.

The cipher suites posted above should work fine with IE8 and later browsers. It also prefers 128-Bit over 256-Bit compression to reduce encryption overhead.

krancour commented 8 years ago

@felixbuenemann

I checked the router's SSL configuration against dev.ssllabs.com and noticed some problems with the SSL configuration that should be addressed.

I actually spent quite a bit of time testing the router's defaults against https://www.ssllabs.com/ssltest/ and they were mostly favorable. That being said... it was a while ago...

dev.ssllabs.com, which you used, is the development version of Qualys SSL Labs. I am torn between not trusting its results and assuming the development version is always, progressively, more strict than what's currently in production.

In either event, I should probably repeat my earlier exercise anyway now that HTTP2 is in the picture.

Specifically on the subject of dhparams... out-of-the-box the router includes a default dhparam that is not the Nginx default. So, while it's a known dhparam, it's a "lesser known" dhparam. It is possible, easy, and documented that you can swap in your own custom dhparam in its place. (See https://github.com/deis/router#production-considerations.)

There are a few reasons we don't do this in a generator currently:

  1. It makes a giant assumption that openssl is available locally. i.e. It won't work on Windows. I can easily see, however, that this particular point may be moot, as I believe our generators also leverage some local bash scripts. It's been proposed by others that the sprig templating library, which helmc uses, expand to include various cryptographic functions. I proposed, specifically, that among them, generation of dhparams would be a nice-to-have (assuming it will work across platforms). I'm digressing...
  2. Generating even a 1024 bit dhparam can take a while on underpowered hardware. We didn't want this to utterly ruin the install experience with lots of waiting. (And generating a 2048 bit dhparam is almost completely out of the question, as even a well-powered machine can take hours to do that.)

Bottom line, however... I'm going to give all of this another run through Qualys SSL Labs (the production version).

felixbuenemann commented 8 years ago

Regarding the dhparam:

@krancour: generating a 2048 bit dhparam is almost completely out of the question, as even a well-powered machine can take hours to do that

Well it's certainly slow, but it takes less than 2 minutes not hours on my recent Core i7.

However, there's no need to generate a new dhparam for each installation with helm. You can always ship a pre-generated 2048-Bit dhparam and allow the user to override it (as is already possible).

For example both apache and nginx ship with a built-in 1024-Bit DH param.

Regarding the cipher suite:

Currently the router is using the default suite as specified by openssl, which is suboptimal and should be tailored to the use https use case. I'll open a new issue to discuss a good cipher suite.

krancour commented 8 years ago

For example both apache and nginx ship with a built-in 1024-Bit DH param.

SSL Labs dings you for using those. They're too well known. iirc, iit would cap you at a B.

felixbuenemann commented 8 years ago

I don't think including a pre-generated 2048-Bit dhparam would fall under that category. It is not insecure and the user can add his own dhparam if he wants to.

krancour commented 8 years ago

Oh. I misunderstood. I must have misread this:

You can always ship a pre-generated 2048-Bit dhparam

That seems reasonable. Open a separate issue and we can get that done.

felixbuenemann commented 8 years ago

Yes, that's exactly what I mean.

felixbuenemann commented 8 years ago

@krancour I checked the deis charts and they already use a 2048-Bit DH, so that should be fine.

@mattk42 Work on fixing the default ciphers is done in PR #231.

krancour commented 8 years ago

@felixbuenemann you're right. I committed that back in 919d579. Just for some reason, my recollection was it being 1024 bits. I am glad I'm wrong.