Make the TLS cipher suite configurable

[jpeach]

Please describe the problem you have

It's common for sites to need to configure the TLS cipher suite. This should be globally configurable, even better if we can allow it to be configures per host.

xref #2878
xref #2401

[sunjayBhatia]

WIP branch: https://github.com/sunjayBhatia/contour/tree/configurable-cipher-suites

[youngnick]

For this @sunjayBhatia, how are you planning on having the cipher suites configurable? Could you put a summary in here? That way, we can get any bikeshedding about config file formatting, etc out of the way before you get too much coding done.

[sunjayBhatia]

Basically following how the minimum TLS version was added:

• global option in config file, if not set/empty will use default list we already use
• Configurable per ingress with an annotation
• Configurable per httpproxy with a field under TLS
• Ingress/httpproxy setting overrides global config setting
• Open question about validation: should we allow invalid ciphers or ciphers that are insecure? If not, how do we fail, ignore, overwrite?

[youngnick]

I think that the default list is our best effort. If you're taking the time to specify a cipher list, then you get what you ask for, and it's on you if it's insecure.

For cipher validity, though, we probably need to have an allow-list for valid cipher names, so we don't run afoul of the NACK problem (#1176) if we pass an invalid cipher name to Envoy, but also so that we can surface typos back to the user quickly, instead of waiting for the config to go to Envoy.

[sunjayBhatia]

Working through this, and noticing that the Min TLS version is not really validated, if it's not 1.1, 1.2, or 1.3 we set it to 1.2 silently and don't seem to error or give any feedback to the user.

We could keep this and do the same with TLS ciphers for consistency (if you provide a set of invalid ciphers, silently set the default set), just wondering if this is intended. Also of course, we could and maybe should give more feedback via logs or contour failing to start (at least if there are mistakes in the config file).

Interesting conditions to think about (omitted easy ones):

• config file has some invalid ciphers for global override, subset smaller than entire list -> fail contour startup? just don't set the invalid ones?
• config file has all invalid ciphers for global override -> fail contour startup? set to default set?
• ingress/httpproxy have some invalid ciphers, subset smaller than entire list -> don't add to dag and set failure status on object? ignore invalid ones and continue?
• ingress/httpproxy have all invalid ciphers -> don't add to dag and set failure status on object? ignore invalid ones and add with global override?

Right now the answer for the equivalent questions on min TLS version are to silently replace any invalid TLS version with 1.2.

[sunjayBhatia]

Basically got most of the pieces done, but the above seems like something to iron out before polishing the change

[youngnick]

I thought we had changed the TLS version to at least log that the setting was unusable. Part of me wants to make that fail fast and hard - there are only three valid options currently, if you don't supply one of those, then fail Contour startup. That's a pretty big behavior change though, and needs its own ticket.

I think that cipher suite selection is critical enough that we should hard-fail rather than trying to fix things automagically for users. So, my suggestions for your use cases would be:

• config file has some invalid ciphers for global override, subset smaller than entire list -> fail contour startup, yes.
• config file has all invalid ciphers for global override -> fail contour startup, yes.
• ingress/httpproxy have some invalid ciphers, subset smaller than entire list, this is a fatal error, don't add it. For Ingress, log at error that it's failed, for HTTPProxy, add an error condition and set the proxy to invalid. Don't add to DAG.
• ingress/httpproxy have all invalid ciphers, as above. Error, don't add to DAG, inform user.

I think we should hear from some of the others though as well, don't only take my word for it.

[sunjayBhatia]

Looks like this change might have been what removed the warning on TLS version: https://github.com/projectcontour/contour/pull/3112/files

[skriss]

@youngnick's proposals make sense to me in the current design. Separately, we want to prevent invalid HTTPProxy config changes resulting in traffic not being served, but that's a separate issue.

If we can decide on a standard approach here, then we should document it as part of the API design guidelines or some other developer documentation so folks know what the expected pattern is going forward, and add issues for fixing any gaps in the current implementation (e.g. min TLS version).

[tsaarni]

There might be some corner cases when:

(1) Contour removes weak cipher from default list: Assuming that the user has previously configured a list of ciphers, Contour security update would not automatically override this list any longer. It can be likely that initially the user created their own configuration for security reasons - to disable some weak cipher before Contour update. But do they remember that once doing that, it become their burden to keep the list up-to-date now and forever?

(2) New ciphers are added to Contour & Envoy: This is variant of (1), but assume that instead of removing a weak cipher, we add a new strong cipher. A number of users are likely not aware of new ciphers being introduced, and therefore will not benefit from the security update if they ever configured their own cipher list. Note that I don't know how often new ciphers are introduced nowadays, especially since TLS 1.3 changed how the cipher suites are working (currently they are not configurable at all)...

(3) Previously valid ciphers are removed from Contour & Envoy completely: Assume that the user configured a list of ciphers that previously was valid, then the user upgrades Contour & Envoy to a version that removed some cipher - not just disabled, but really made the cipher invalid. Contour would fail to start after upgrade if that kind of change is ever made. The user would need to know to update the configuration before the upgrade.

Maybe for these corner cases, having a user configurable list of disabled cipher suites could be safer than allowing user to provide list of enabled ciphers? That would cover (1) and (2).

Assuming disabling weak ciphers is the target use case for this feature, then invalid disabled ciphers could be safely ignored from user provided configuration, taking care of (3). Other use cases could be intentionally asking for weak ciphers to be re-enabled like mentioned in previous comments and maybe re-ordering of ciphers could be one as well, though I'm not sure if ordering of ciphers returned by server has impact on selection (was it driven by client?). Envoy configuration of course lists only enabled ciphers, not disabled. But there are others that do have capability of listing disabled ciphers, such as OpenSSL/BoringSSL cipher filters with the - operator (doc).

[sunjayBhatia]

But do they remember that once doing that, it become their burden to keep the list up-to-date now and forever?

IMO configuring ciphers yourself means you have looked at what Contour configures by default and have found that is unsuitable (to permissive or not permissive enough). I would think that having taken that responsibility on it is appropriate to continue to do so.

A number of users are likely not aware of new ciphers being introduced, and therefore will not benefit from the security update if they ever configured their own cipher list.

I think this can be classified similarly to the above point, and we can make it easier to discover for those advanced users who have configured cipher suite overrides with release notes and documentation.

Assume that the user configured a list of ciphers that previously was valid, then the user upgrades Contour & Envoy to a version that removed some cipher - not just disabled, but really made the cipher invalid. Contour would fail to start after upgrade if that kind of change is ever made. The user would need to know to update the configuration before the upgrade.

This would be a similar issue if users were to use a newer Envoy (that removed support for some cipher) than Contour had validated (not sure if this ever happens in practice, with the operator it would not). In this case the outcome would be worse, LDS updates would be rejected.

Maybe for these corner cases, having a user configurable list of disabled cipher suites could be safer than allowing user to provide list of enabled ciphers? That would cover (1) and (2).

Assuming the target of the feature is not intentionally asking for weak ciphers to be re-enabled, this would also make validation logic easier and reduce the possibility of misconfiguration leading to Contour startup failures and rejected Ingress/HTTPProxy updates.

Looks like we've already had some discussion around this here: #823 and looks like the consensus was to not do this, however this issue was pulled into the backlog again

[tsaarni]

I was looking about this bit more and have few more observations that I'd like to discuss.

Working through this, and noticing that the Min TLS version is not really validated, if it's not 1.1, 1.2, or 1.3 we set it to 1.2 silently and don't seem to error or give any feedback to the user.

I could not enable TLS 1.1 anymore at all, even if the string was correctly set as "1.1". There seems to be two reasons:

(a) Parser will always return 1.2 if minimum protocol version is less than 1.2. I assume this was unintentional since documentation was not changed?

contour/internal/xdscache/v3/listener.go

Line 212 in 5633539

if minTLSVersion > envoy_tls_v3.TlsParameters_TLSv1_2 {

(b) Since Contour follows SSL Labs A+ there are actually very few ciphers left anymore. Latest update for removing weak ciphers was #3154 and I now created #3237 to finalize that (I believe some were missed). Only weak ciphers would be left for TLS 1.1 and since we remove those, it is not possible to enable TLS 1.1 anymore.

[sunjayBhatia]

I assume this was unintentional since documentation was not changed?

Maybe it was intentional given #2777 (comment) ? The TLS 1.1 issue has not been closed yet so ostensibly theres something left to finish?

Since Contour follows SSL Labs A+ there are actually very few ciphers left anymore. Latest update for removing weak ciphers was #3154 and I now created #3237 to finalize that (I believe some were missed). Only weak ciphers would be left for TLS 1.1 and since we remove those, it is not possible to enable TLS 1.1 anymore.

Yeah that's a good point, what value are we providing by making the ciphers configurable if we are on top of the allowed list, the list is small, TLS 1.1 is basically disabled, etc.?

[tsaarni]

Here is a summary:

TLS 1.1

no ciphers -> currently disabled though documentation remains.

TLS 1.2

There are really just three ciphers, but due to the way how the ciphers are named, there are two variants of each:

# ciphers that can be used when there is a server certificate with RSA key
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384

# ciphers that can be used when there is a server certificate with EC key
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384

Actually, for EC there still is few more weak ones currently, which I did not list here. These would be removed by #3237.

TLS 1.3

TLS_AES_128_GCM_SHA256
TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256

Previously there was a lot of ciphers but now that TLS 1.1 is gone and TLS 1.2 is reduced to the remaining three ciphers, I started doubting as well if configurability is worth the effort anymore? Unless someone wants to enable weak ciphers for backwards compatibility...

Note: I think that having to update the software for disabling weak TLS ciphers has not been considered that great strategy for security, but maybe times have changed - there is not much left there anymore to configure.

[skriss]

Sigh, yeah the hard drop of TLS 1.1 was unintended; it was still supposed to be allowed if needed, just not the default. Over-eager find/replace :/. That said, your other point about there no longer being any 1.1-compatible ciphers is valid, so perhaps the hard drop is OK and we just need to update docs. Let's see what @stevesloka @youngnick have to say here.

[stevesloka]

Being TLS 1.1 dropped I think it's probably fine to leave them as-is and move forward, just need to document this is the case if the docs don't say that now. Probably not a good idea for folks to enable TLS 1.1 anyway so a good step forward.

[youngnick]

Yeah, I think that we've ended up shortcutting removing 1.1, unfortunately, but now it's gone, we should just remove it everywhere.

The idea about disabling ciphers rather than specifying is interesting, but I think there's one use case we've all missed, around super regulated environments. If you have got a list of acceptable ciphers from some authority you can't negotiate with, then being able to specify exactly that list makes compliance easy. Obviously, the downside here is that we end up with all the issues that @tsaarni mentioned. I'm not sure what the relative frequencies/importance are here though, maybe @xaleeks may have some thoughts?

[xaleeks]

A few thoughts on why making the list of ciphers configurable could be advantageous

• The actual list of cipher suites is most likely maintained by separate team from the cluster admin or k8s admin in an enterprise setting, each list having undergone a certification process that’s not easily changed (need to consider multi-platforms, service level agreements, Toolchains). This means it could be highly customized for each customer
• It may also need to change due to performance / scale testing impacts if there are any (when considered as part of the entire FIPs package). I don't think these have been thoroughly tested yet
• May need to configure TLS suites to use proprietary ciphers and configure the TLS version

And when this introduces conflicts, we can use the resolution mechanisms that Sunjay and Nick discussed above to sort it out

Also as another datapoint regarding security customization requirements - the envoy images will likely need to be customized as well for a lot of customers, ie for Envoy to be compiled with vendor-specific rebranding of google’s BoringCrypto for ex.

[xaleeks]

This would be a similar issue if users were to use a newer Envoy (that removed support for some cipher) than Contour had validated (not sure if this ever happens in practice, with the operator it would not). In this case the outcome would be worse, LDS updates would be rejected.

Operator does make it easier to swap out components but we should call out in docs explicitly to not do this. All bets are off from a support perspective for now

[xaleeks]

Separately, we want to prevent invalid HTTPProxy config changes resulting in traffic not being served, but that's a separate issue.

hijacking the thread a bit sorry, but this sounds like a good feature to have :) do you mean some kind of static code analysis during the kubectl apply phase? So that an httpproxy with unwanted behavior is at least better than a broken one?

[tsaarni]

Having custom Envoy builds is interesting viewpoint for the configurability. When building with FIPS certified BoringSSL / BoringCrypto there will be a different set of possible ciphers than "standard" builds. Validating against "known" list of correct ciphers might conflict with this use case. I think there can be even ciphers that have been elsewhere stated as weak, and therefore removed from Contour's list.

[sunjayBhatia]

May need to configure TLS suites to use proprietary ciphers and configure the TLS version

this seems hard to support unless we also require users to supply a list of “valid ciphers” in some global config, otherwise we can’t check if the configuration they’re sending works for their custom version of Envoy (to prevent xDS NACKs caused by mistakes/misconfiguration)

generally supporting Envoys that are customized to this detail (not just excluded extensions) seems hairy

When building with FIPS certified BoringSSL / BoringCrypto there will be a different set of possible ciphers than "standard" builds

Good point, could get around this with a “is FIPS” flag intended to signify the set of envoys this contour will configure are all FIPS builds? however that assumes the deployment is homogeneous, again another case which seems a bit convoluted since we need to validate the ciphers the user asks for

[skriss]

Separately, we want to prevent invalid HTTPProxy config changes resulting in traffic not being served, but that's a separate issue.

hijacking the thread a bit sorry, but this sounds like a good feature to have :) do you mean some kind of static code analysis during the kubectl apply phase? So that an httpproxy with unwanted behavior is at least better than a broken one?

@xaleeks see e.g. #2019 (there may be some other related issues), can continue discussion there.

[sunjayBhatia]

this seems hard to support unless we also require users to supply a list of “valid ciphers” in some global config, otherwise we can’t check if the configuration they’re sending works for their custom version of Envoy (to prevent xDS NACKs caused by mistakes/misconfiguration)

After testing this out, Envoy doesn't seem to actually reject unknown ciphers (does not send a NACK), though we would still have the issue of validating whether some custom cipher is actually usable because Envoy will not configure ciphers it does not recognize (via looking at the admin API config dump) so a user would think something is working when it is not if we did not do validation

[sunjayBhatia]

this seems hard to support unless we also require users to supply a list of “valid ciphers” in some global config, otherwise we can’t check if the configuration they’re sending works for their custom version of Envoy (to prevent xDS NACKs caused by mistakes/misconfiguration)

After testing this out, Envoy doesn't seem to actually reject unknown ciphers (does not send a NACK), though we would still have the issue of validating whether some custom cipher is actually usable because Envoy will not configure ciphers it does not recognize (via looking at the admin API config dump) so a user would think something is working when it is not if we did not do validation

I think I did something wrong testing it out the first time 👀 when attempting to configure ciphers Envoy doesn't recognize, I do get an error/error state on listeners (INVALID rejected for obvious reasons and the other rejected b/c BoringSSL doesn't implement Diffie Helman key exchange I believe):

"details": "Failed to initialize cipher suites [ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-CHACHA20-POLY1305]:[ECDHE-RSA-AES128-GCM-SHA256|ECDHE-RSA-CHACHA20-POLY1305]:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:AES128-SHA:ECDHE-ECDSA-AES256-SHA:DES-CBC3-SHA:INVALID. The following ciphers were rejected when tried individually: DHE-RSA-AES128-GCM-SHA256, INVALID"

[sunjayBhatia]

We've decided #3292 will close this issue as we believe operator configurability of cipher suites should resolve this and we do not have a need to offer configurability at the HTTPProxy/Ingress level. If you would like to configure TLS ciphers at that level, please open a new issue so we can track the request!