tls: add options to validate SANs and send SNI for upstream hostname#36903
tls: add options to validate SANs and send SNI for upstream hostname#36903ggreenway merged 11 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
/assign-from @envoyproxy/senior-maintainers |
|
@envoyproxy/senior-maintainers assignee is @alyssawilk |
Signed-off-by: Greg Greenway <ggreenway@apple.com>
RyanTheOptimist
left a comment
There was a problem hiding this comment.
I think I might be a bit unclear about the use case here. Do we have an issue which explains the motivation? Or maybe we could just flesh out the PR description?
| // If true, replace any Subject Alternative Name validations with a validation for a DNS SAN matching | ||
| // the SNI value sent. Note that the validation will be against the actual requested SNI, regardless of how it | ||
| // is configured. | ||
| bool auto_sni_san_validation = 7; |
There was a problem hiding this comment.
I don't think I quite understand the semantics here. Perhaps we could have an example?
| // the hostname is known due to either a DNS cluster type or the | ||
| // :ref:`hostname <envoy_v3_api_field_config.endpoint.v3.Endpoint.hostname>` is set on | ||
| // the host. | ||
| bool auto_host_sni = 6; |
There was a problem hiding this comment.
How is this intended to relate to the auto_sni here:
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/protocol.proto#config-core-v3-upstreamhttpprotocoloptions
And ditto for auto_san_validation vs auto_sni_san_validation?
|
I updated the PR description with a lot more details. That text should probably make it into the docs somewhere; do you have a good idea of the appropriate place for them? |
Also, could add to the protocol buffer changes for more elucidation. |
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
I took a stab at finding a common place to describe the various related options and their interactions, and linked to that from the proto fields. I also made some minor cleanups and corrections. |
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext | ||
| sni: www.envoyproxy.io | ||
| common_tls_context: |
There was a problem hiding this comment.
I added this because I didn't want an example to not have a validation_context. That makes it a dangerously-insecure example.
There was a problem hiding this comment.
probably we can use 1 config file and just highlight different parts - i can follow up with this
| name: envoy.transport_sockets.tls | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext | ||
| sni: proxy-postgres-backend.example.com |
There was a problem hiding this comment.
I added this so that it's a more complete example for how someone should actually configure this
There was a problem hiding this comment.
im not sure i agree with this change - this is an example not using sni - also this breaks the highlighting
There was a problem hiding this comment.
actually - it doesnt break it - its ~similar in current docs - wondering what should be highlighed here
There was a problem hiding this comment.
Want me to revert this one? No SNI with validation is fine. Anything with no validation is probably not-fine.
There was a problem hiding this comment.
yeah - understood - on balance i would say keep the status quo on this
There was a problem hiding this comment.
ok, reverted that change
|
|
||
| .. note:: | ||
|
|
||
| If the "Subject Alternative Names" for a certificate are for a wildcard domain, eg ``*.example.com``, |
There was a problem hiding this comment.
This note didn't make sense; Envoy will correctly validate an expected SAN of test.example.com against a certificate with *.example.com (Envoy considers that a match)
There was a problem hiding this comment.
it was quite a while ago when i added this - but i thought i added it after testing it out
There was a problem hiding this comment.
It's documented in match_typed_subject_alt_names; this definitely works
There was a problem hiding this comment.
hmm - not sure - but im wondering if there are some circumstances where it doesnt work
There was a problem hiding this comment.
If there are, it's a bug and we should fix it.
There was a problem hiding this comment.
if i get some time ill try and retry the steps i might have taken
|
/docs |
|
Docs for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/36903/docs/index.html The docs are (re-)rendered each time the CI |
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Thanks for the new comments and docs. LGTM, though obviously please wait for phlax's review too.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
headsup that CI is currently down 8( |
|
/retest |
These options are related, but do not have to be used together.
The existing
auto_sniandauto_san_validationmake Envoy set upstream TLS SNI and validate SANs based on the downstream requests's:authority(or override header).The new
auto_host_snioption causes Envoy to set the upstream TLS SNI to the hostname in the cluster for the host being connected to. For example, in a DNS cluster with server1.example.com and server2.example as the cluster members, connections would have SNI of server1.example.com and server2.example.com, respectively.The new
auto_sni_san_validationoption causes Envoy to validate that the peer's server certificate has a DNS SAN that matches whatever SNI value was sent (regardless of how it's configured). This is appropriate to set in many/most configurations for normally-behaving upstream servers without special certificate validation requirements.Risk Level: Low
Testing: New tests
Release Notes: Added