add auto_sni and auto_san field in ClientTLSSettings #1773
Conversation
ZhiHanZ
requested review from
dcberg,
duderino,
linsun,
louiscryan,
nrjpoddar,
rshriram and
smawson
as code owners
istio-testing
added
the
size/M
google-cla
bot
added
the
cla: yes
|
|
||
| // SNI string to present to the server during TLS handshake. | ||
| string sni = 6; | ||
| // auto_sni and auto_san could automatically set the outbound SNI |
There was a problem hiding this comment.
Document this only works with HTTPS traffic, and when auto_sni is true it will override the sni set above.
|
@ZhiHanZ: The following test failed, say
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. |
| // Additional context: | ||
| // https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/protocol.proto#envoy-v3-api-field-config-core-v3-upstreamhttpprotocoloptions-auto-sni | ||
| // https://github.com/istio/istio/issues/27847 | ||
| bool auto_sni = 8; |
There was a problem hiding this comment.
Unless I missed something, the RFC does require that a HTTPs client sets the SNI this way. It is a bug to not do so - I don't think we need an API to indicate we should be compatible with the RFC.
I'm not sure what auto_san does in this context - but if it's about following the RFC it is also redundant.
A bigger issue is that the default value of bool in proto is false - meaning that if user doesn't include the new fields, the default behavior would be non-standard.
I don't think the API need a 'non-standard' mode - and for upgrade issues we can use an temporary internal env variable like we did in the past, not a long-term supported API change.
There was a problem hiding this comment.
@costinm RFC does not mandate HTTPS client to set SNI (it can be not set at all) / verify SAN in this way, RFC 2818 says
If the client has external information as to the expected identity of
the server, the hostname check MAY be omitted.
In most of Istio service-to-service communication in mTLS, we don't set SNI or verify SAN in this way, but we use SVID to perform the identity verification. This only need to be set for certain egress cases.
There was a problem hiding this comment.
@lizan RFC 7540 said:
The TLS implementation MUST support the Server Name Indication (SNI) [TLS-EXT] extension to TLS. HTTP/2 clients MUST indicate the target domain name when negotiating TLS.
RFC 2818 is too old and no longer reflects reality.
There was a problem hiding this comment.
Also I would note we DO always set an SNI. We just set our weird internal format (I think)
There was a problem hiding this comment.
In general: please include extensive docs for each field, including links to references ( RFC ),
default values - and if the default behavior changes some info about versions.
But for this change: I think the larger issue is what the RFC says and if we even want to have a setting that breaks standards ( and what the default should be )
istio-testing
added
the
needs-rebase
|
@ZhiHanZ: 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. |
|
Is there already an implementation of the auto_san and auto_sni property in the ClientTLSSetting or someone working or continuing on this PR? Did someone find the possibility to configure them via a custom EnvoyFilter? Thanks! |
|
cc @ZhiHanZ |
|
+1 |
|
@ZhiHanZ : If you are not planning to pursue this(I assume so, as there is no activity for quite sometime now), is it okay if I pick this up for further development? |
|
FYI: I have created the below RFC to resume work on this task : https://docs.google.com/document/d/1pTUl-Ng3nXAWJb7UGJtalftznpxQEfID/edit |
|
As per the latest discussions in the networking working group, an API change is not needed for supporting the same. The details are updated in the RFC, I think if you are okay, you can close this PR. I will be working on the implementation as per the RFC @ZhiHanZ |
11 participants
Introducing the required fields to add auto_sni and auto_san feature for istio destination rule sni settings
related issue:
istio/istio#27847