Consider supporting newer TLS cipher suites

[moderation]

https://github.com/lyft/envoy/blob/master/source/common/ssl/context_config_impl.cc specifies supported cipher suites for Envoy.

The recent Go 1.8 release included support for the CHACHA20_POLY1305 cipher suites.

The following modern cipher suites are recommended by the Mozilla SSL Configuration Generator

ECDHE-ECDSA-AES128-GCM-SHA256
ECDHE-ECDSA-AES128-SHA256
ECDHE-ECDSA-AES256-GCM-SHA384
ECDHE-ECDSA-AES256-SHA384
ECDHE-ECDSA-CHACHA20-POLY1305
ECDHE-RSA-CHACHA20-POLY1305

BoringSSL support all of these ciphers.

[moderation]

Have done some testing and the following ChaCha ciphers result in a runtime failure

ECDHE-ECDSA-CHACHA20-POLY1305
ECDHE-RSA-CHACHA20-POLY1305

Error

[2017-04-06 09:28:00.296][17553][critical][main] error initializing configuration 'sync-front-envoy.json': Unknown cipher specified in cipher suites ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA:AES128-SHA

In theory BoringSSL supports these ciphers. Will do further testing.

Edit: I think the reason for the errors is the the ciphers are hard coded to NID_X9_62_prime256v1 at https://github.com/lyft/envoy/blob/master/source/common/ssl/context_impl.cc#L150. I think in order to support the newer ciphers the curves NID_secp384r1 and NID_secp521r1 need to be enabled.

[PiotrSikora]

Thanks for bringing this up, @moderation. I'll look into why it doesn't work and update defaults.

@mattklein123 what are you thoughts on keeping OpenSSL support around? I'm asking, because if we're adding CHACHA20-POLY1305, then it would be useful to do that using "equal-preference groups", which allow clients to chose between CHACHA20 & AES-GCM (depending on hardware support for AES-NI, etc.).

However, OpenSSL doesn't support this yet (see: openssl/openssl#541), so it would require another #ifdef for BoringSSL vs OpenSSL, which as far as I recall, you're not the biggest fan of.

Also, feel free to assign this to me.

[mattklein123]

@PiotrSikora I'm not sure how much people care about openssl. Personally, I feel like if we tell people what SHA of boringssl to use it's probably fine for us to drop openssl support. Let me poll a few people.

[mattklein123]

@PiotrSikora as part of this, can you also advise on how best to handle TLS 1.3? Should we make it opt in on the context for now?

[moderation]

@PiotrSikora interesting that you mention equal preference groups as I just saw AGL on Twitter mention them. I was going to retest the ChaCha Poly ciphers with BoringSSL master but I can't see why they fail with the current SHA (see edit above re: hard coded curves). My current working config is:

const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = "ECDHE-ECDSA-AES256-GCM-SHA384:"
                                                             "ECDHE-ECDSA-AES256-SHA384:"
                                                             "ECDHE-ECDSA-AES128-GCM-SHA256:"
                                                             "ECDHE-ECDSA-AES128-SHA256:"
                                                             "ECDHE-RSA-AES128-GCM-SHA256:"
                                                             "ECDHE-RSA-AES128-SHA256:"
                                                             "ECDHE-RSA-AES128-SHA:"
                                                             "ECDHE-RSA-AES256-GCM-SHA384:"
                                                             "ECDHE-RSA-AES256-SHA384:"
                                                             "ECDHE-RSA-AES256-SHA:"
                                                             "AES256-GCM-SHA384:"
                                                             "AES256-SHA:"
                                                             "AES128-GCM-SHA256:"
                                                             "AES128-SHA256:"
                                                             "AES128-SHA";

[PiotrSikora]

See: #750, #751 & #752.

@moderation For some reason, I was unable to cc you on those pull requests.

[moderation]

Looks great @PiotrSikora. Do you think we need to add the additional curves as per my edited comment at the top of this thread?

[PiotrSikora]

They are all included in #752.