Add cipherSuites to DR ClientTLS settings

[howardjohn]

As part of https://github.com/istio/istio/issues/33210

Basically envoy made the defaults very restrictive, so if users want to to connect to legacy services the only current option is envoyfilter. I think adding this to DR opens up some usages

[istio-testing]

@howardjohn: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
release-notes_api	3c8b7a6b326702731ff1bd02ceb534ed557f8332	link	/test release-notes_api

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[costinm]

I'm not sure how much we broke. In-cluster traffic should be mostly fine - the other sidecars have the safe cyphers anyways.

This API is only relevant for legacy/special destinations outside of the cluster. AFAIK most web sites support the safe ciphers anyways, so no need for config.

For Gateway it is reasonable to be able to configure specific ciphers - you control the gateway and if you want to only support a specific cipher or keys it's fine. But the use case for egress is a bit more complicated, and the audience is different ( gateway admin may be expected to know the cipher names he wants to explicitly configure - but not if you talk with a remote site ).

And the cipher names are pretty horrible.

On Tue, Jun 1, 2021 at 2:55 PM John Howard @.***> wrote:

@.**** commented on this pull request.

In networking/v1alpha3/destination_rule.proto https://github.com/istio/api/pull/2014#discussion_r643505251:

@@ -964,6 +964,10 @@ message ClientTLSSettings {

// SNI string to present to the server during TLS handshake. string sni = 6; +

• // Optional: If specified, only support the specified cipher list.
• // Otherwise default to the default cipher list supported by Envoy.

I originally argued for that, but there were some concerns about reverting our security posture. I can see both arguments.. generally we favor backwards compat over security but in this case we already broke it so it seems a bit like a sunk cost to some extent.

Regarding your other comments - this does seem reasonable, I was mostly favoring similarities with Gateway api (literally copy+paste), but maybe its better to move it into mesh config (or append)...

@nrjpoddar https://github.com/nrjpoddar any thoughts?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/api/pull/2014#discussion_r643505251, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2TEH5MXJBJ2GLZ3LALTQVJM3ANCNFSM455NFYKA .

[nrjpoddar]

@costinm @howardjohn I don't think we broke a lot of users as evident from the fact that just 1 user has complained so far and the fact that most external servers support better ciphers. I think we are more or less agreeing that we should continue down this path of better secure defaults with some knob for users to still downgrade to less secure ciphers as needed.

My reasonings for adding in DR APIs are:

1. As most services will have support for stronger ciphers, I anticipate a user only requiring this option for few hosts at which point we shouldn't force them to lower the security posture globally by only providing a mesh config option.
2. If a user still wants to configure this globally, they can always enable DR merging, and add a DR with no host and a custom cipher list in config root namespace.

[costinm]

I am ok with adding it to DR, but as a 'allow insecure ciphers' or similar option - instead of asking users to list each of the weird names.

On Wed, Jun 2, 2021 at 2:35 AM Neeraj Poddar @.***> wrote:

@costinm https://github.com/costinm @howardjohn https://github.com/howardjohn I don't think we broke a lot of users as evident from the fact that just 1 user has complained so far and the fact that most external servers support better ciphers. I think we are more or less agreeing that we should continue down this path of better secure defaults with some knob for users to still downgrade to less secure ciphers as needed.

My reasonings for adding in DR APIs are:

1. As most services will have support for stronger ciphers, I anticipate a user only requiring this option for few hosts at which point we shouldn't force them to lower the security posture globally by only providing a mesh config option.
2. If a user still wants to configure this globally, they can always enable DR merging, and add a DR with no host and a custom cipher list in config root namespace.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/pull/2014#issuecomment-852872309, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2UWNX3WIYUP4O2YTMDTQX3PHANCNFSM455NFYKA .

[nrjpoddar]

I like it!

Basically setting it to insecure gets you back to old defaults. If cipher configurability is desired we can add this array field that @howardjohn has in future.

[costinm]

If low level cipher config is desired - we still have EnvoyFilter, and if we find a lot of users need this we can add the array field to DR in future as well.

One thing to keep in mind is that Istio and K8S APIs can have multiple implementations - we are working on promoting the proxyless gRPC from experimental. We should start asking the question 'is this an envoy specific behavior, or is it something that other implementations of Istio APIs can use'. In this case - I think it can be supported everywhere - but cipher names in Java are slightly different from envoy AFAIK.

On Wed, Jun 2, 2021 at 7:39 AM Neeraj Poddar @.***> wrote:

I like it!

Basically setting it to insecure gets you back to old defaults. If cipher configurability is desired we can add this array field that @howardjohn https://github.com/howardjohn has in future.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/istio/api/pull/2014#issuecomment-853085065, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2T35DT3IKZ44AOFEZTTQY7BVANCNFSM455NFYKA .

[nrjpoddar]

Cipher suites names come from openssl so it can be different in other runtimes not based on openssl derivatives.

[howardjohn]

@ramaraochavali WDYT of "allow insecure mode"? Do you have a need for specific ciphers?

[ramaraochavali]

No we do not have need for specific ciphers as of now. But we never know - our security might enforce it. I agree listing all ciphers for regular use is problematic. How about this?

Introduce a new cipher_validation_mode : DEFAULT|INSECURE|CONFIGURED DEFAULT - Current Default Cipher Suites (Default value that Envoy provides) INSECURE - Earlier cipher suites it used to Allow CONFIGURED- when this set, user has to specify list of ciphers?

[hzxuzhonghu]

I do think most users not want to config the ciphers, secure cipher suites should satisfy most users. If this is for the very little percentage of users, how about adding a global api, which allows only operator to set.

[istio-testing]

@howardjohn: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.