Extensions: Network filter to forward SNI from the downstream TLS connection to the upstream TLS connection by vadimeisenbergibm · Pull Request #4334 · envoyproxy/envoy

vadimeisenbergibm · 2018-09-04T14:49:54Z

Signed-off-by: Vadim Eisenberg vadime@il.ibm.com

Description: Add forward_original_sni field to the upstream TLS Context. It will allow forwarding SNI from the downstream connection (extracted by the TLS inspector filter) to the upstream. One of the use cases described in #4076. This flag should be mutually exclusive with the sni filed of UpstreamTlsContext.

Risk Level:Low

Testing: unit tests

Docs Changes: yes

Release Notes: yes

Related to #4076

Depends on #4973

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-09-04T14:50:28Z

^ @lizan @rshriram I started to work on it.

api/envoy/api/v2/auth/cert.proto

cmluciano · 2018-09-04T15:41:23Z

api/envoy/api/v2/auth/cert.proto

  //
  //   TLS renegotiation is considered insecure and shouldn't be used unless absolutely necessary.
-  bool allow_renegotiation = 3;
+  bool allow_renegotiation = 4;


Is changing the option number for an existing command a breaking change? Can the new option be 4 instead?

Right, please don't change field number, use 4 in new field.

rshriram · 2018-09-04T15:51:17Z

api/envoy/api/v2/auth/cert.proto

+  // If true, the SNI from the downstream connection is forwarded to the upstream connection.
+  //
+  // TODO: handle oneof with the sni
+  bool forward_original_sni = 3;


why do you need an option for this? why can't you always forward the downstream SNI if there is no SNI value specified in the cluster?

This is what @lizan wanted #4076 (comment)

Add explicit field forward_original_sni would be better than defaulting to forward.

I proposed:

Change tcp_proxy to set the SNI of the upstream connection in case the cluster's tls context does not have sni field.

@rshriram @lizan Could you please agree on the API ?

Let's be explicit between forward downstream SNI vs no SNI. You should be able to use oneof without breaking wire format.

lizan · 2018-09-04T22:34:24Z

api/envoy/api/v2/auth/cert.proto

+  // If true, the SNI from the downstream connection is forwarded to the upstream connection.
+  //
+  // TODO: handle oneof with the sni
+  bool forward_original_sni = 3;


Let's be explicit between forward downstream SNI vs no SNI. You should be able to use oneof without breaking wire format.

lizan · 2018-09-04T22:35:30Z

api/envoy/api/v2/auth/cert.proto

  //
  //   TLS renegotiation is considered insecure and shouldn't be used unless absolutely necessary.
-  bool allow_renegotiation = 3;
+  bool allow_renegotiation = 4;


Right, please don't change field number, use 4 in new field.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-09-05T13:31:15Z

@lizan I have fixed the API (this is just the beginning of the PR), do you know how should I specify that only one of the options, sni or forward_original_sni, may be used?

rshriram

So here is my rationale:
no incoming SNI and cluster has SNI - send cluster SNI
Has incoming SNI and no cluster SNI - send incoming SNI
Has incoming SNI and has cluster SNI (set through destinationRule for eg) : we have choices - ignore incoming SNI and use cluster SNI or ignore cluster SNI and use incoming SNI

Both of these scenarios are useful. When we setup Envoy as a plain pass through proxy like egress gateway, we want the inbound SNI to be forwarded if present, else use the SNI set in the cluster
So that the envoy on the other cluster that’s receiving traffic will have some SNI to route on.

From that point of view, this field should not be a one of. It should be independent and during upstream connection, we evaluate the cases above.

vadimeisenbergibm · 2018-09-05T14:42:47Z

@rshriram

no incoming SNI and cluster has SNI - send cluster SNI
Has incoming SNI and no cluster SNI - send incoming SNI
Has incoming SNI and has cluster SNI (set through destinationRule for eg) : we have choices - ignore incoming SNI and use cluster SNI or ignore cluster SNI and use incoming SNI

"No incoming SNI" - do you mean the SNI was not received from the downstream connection? So the decision which SNI to send must be determined dynamically, correct?

So it should depend on three variables, sni, forward_original_sni (statically set in the configuration of the cluster) and let's call it sni_received _dynamically_from_the_downstream.

So the algorithm is:

if !sni_received_dynamically_from_the_downstream && sni {
    send sni
}

if sni_received_dynamically_from_the_downstream && !sni {
   send sni_received_dynamically_from_the_downstream
}

if sni_received_dynamically_from_the_downstream && sni {
   if forward_original_sni {
     send sni_received_dynamically_from_the_downstream
   } else {
     send sni
   } 
}

// if !sni_received _dynamically_from_the_downstream && !sni {
//      do not send any sni
// }

Did I understand you correct?

ggreenway · 2018-09-05T16:23:35Z

What configurations do we expect this to work in? I think this only makes sense with tcp_proxy, because it has 1:1 downstream-connection:upstream-connection relationship. For http, if you had multiple requests from different downstream connections, with different SNI values, going over the same upstream connection, it isn't clear what the behavior should be.

If it only works with tcp_proxy, I'm not sure this is the right place for the configuration to live.

vadimeisenbergibm · 2018-09-05T16:26:48Z

@ggreenway Good point in
#4334 (comment).

So @lizan, what do you think? Should I add this flag as a parameter to the tcp_proxy?

rshriram · 2018-09-05T20:11:50Z

@ggreenway why isn't this valid for http as well? The SNI is outside of the HTTP semantics. It doesn't matter if we have a Mx1 or 1x1 right? the contract here is that if there is an SNI for the inbound connection that caused this cluster to be used, then use that SNI (if allowed).

I am assuming that you can get a handle over the connection on which a request arrived, for HTTP and TCP alike.

ggreenway · 2018-09-05T20:17:22Z

I think it doesn't make sense because for HTTP we're doing request proxying not connection proxying, and the SNI value is a connection property. So you're taking a value from the first downstream request, and applying it to N upstream requests.

lizan · 2018-09-05T21:59:36Z

In HTTP (or any other supported Nx1 proxy L7 protocol) the connection pool can be based on upstream SNI. I think it makes sense to send all request from downstream connections with same SNI to same upstream connection. This doesn't have to be implemented initially, but please leave a comment.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…ansportSocket() Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

rshriram · 2018-09-07T20:07:05Z

I think it doesn't make sense because for HTTP we're doing request proxying not connection proxying, and the SNI value is a connection property. So you're taking a value from the first downstream request, and applying it to N upstream requests.

Agree. But the SNI here is not for routing. Its for policy enforcement, where the policy system might want to look at the SNI header and the HTTP authority to make a decision.

ggreenway · 2018-09-07T20:09:38Z

But the SNI here is not for routing. Its for policy enforcement

That may be true in this use-case, but you don't know how it will be used elsewhere.

I think if we want to do this for HTTP, @lizan's suggestion is the right way to do it.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

stale · 2018-09-16T02:26:18Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-09-23T03:14:02Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: ronenschafferibm <ronen.schaffer@ibm.com>

This reverts commit 3cc57db. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…createTransportSocket()" This reverts commit 2f39451. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-10-10T07:21:38Z

@rshriram @lizan Could you please reopen this PR? I am back from a vacation and will continue working on it. Please disregard the implementation meanwhile, I am going to change it.

This reverts commit f90f771. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…test Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…ning if statement since it should be dependant on context or downstream connection Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-11-06T09:17:34Z

@lizan Could you please review this PR? I wrote several unit tests, please let me know if more tests are required.

lizan

Looks great and in this is in a good direction.

Can you pull out the non-extension part to a separate PR? That will help the review process.

We might want to wrap override_server_name in some class (perhaps named TransportSocketOptions, and that can be embedded directly in FilterState, but I'd like to iterate that on the separate PR.

lizan · 2018-11-06T09:29:15Z

source/common/stream_info/BUILD

    ],
 )

+envoy_cc_library(


this should be under tcp_proxy, see what is done for PerConnectionCluster, also this should be named override_server_name, "forward" what the filter does and this key only intended for override.

My intention was as follows:
The filter is not related directly to the TCP proxy. The filter specifies the intention that the SNI must be forwarded. Other filters, such as TCP proxy, can extract that information and forward the SNI.

So there are three different names here:

Forward Requested Server Name in the Stream Info, means that there is some requested server name, not necessarrily SNI, maybe requested server name from some other protocol, that must be forwarded to the upstream, not necessarily by tcp_proxy.

Forward Original SNI - the name of the filter, specifies the intention to forward the SNI, a specific kind of a server name

Override Server Name in tcp_proxy, cluster manager etc. it means that the server name of the upstream cluster must be overridden by that value. It does not specify that it is SNI and does not specify where this server name comes from.

This is why I put Forward Requested Server Name in the stream info and not in TCP proxy, to make this artifact independent from the TCP proxy.

lizan · 2018-11-06T09:33:00Z

CODEOWNERS

 # sni_cluster extension
 /*/extensions/filters/network/sni_cluster @rshriram @lizan
+#  extension
+/*/extensions/filters/network/forward_original_sni @lizan


name yourself?

@lizan Sure, let me extract the non-extension part into a separate PR, I am in favor of separate PRs.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

rshriram · 2018-11-06T15:43:30Z

docs/root/configuration/network_filters/forward_original_sni.rst

+Forward Original SNI
+=========================
+
+The `forward_original_sni` is a network filter that instructs other filters,


given this explanation, I think this should be called use_downstream_sni .
The explanation also needs to be tweaked to state that when an inbound connection is using TLS, and the upstream cluster is using mTLS, this option will allow user to automatically set the SNI of upstream cluster to be the same as the downstream SNI.

@rshriram Agreed, used_downstream_sni is more clear.

Regarding:

The explanation also needs to be tweaked to state that when an inbound connection is using TLS, and the upstream cluster is using mTLS, this option will allow user to automatically set the SNI of upstream cluster to be the same as the downstream SNI.

So why does the explanation not cover the case you are describing? If we can consider mTLS being a kind of TLS: a network filter that instructs other filters, such as tcp_proxy, to forward the SNI value from the downstream connection to the upstream connection

rshriram · 2018-11-06T15:44:11Z

@lizan Since this has become an extension and this is very specific to Istio, why can't we move this to istio/proxy, just like the way we moved the tcp_cluster_rewrite to istio/proxy?

mattklein123 · 2018-11-07T02:09:44Z

@lizan Since this has become an extension and this is very specific to Istio, why can't we move this to istio/proxy, just like the way we moved the tcp_cluster_rewrite to istio/proxy?

+1, if this is Istio specific please move it.

lizan · 2018-11-07T05:33:10Z

Yes I'm thinking of that too. the non-extension part (#4973) have to be done before that anyway.

stale · 2018-11-14T06:30:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-11-21T06:42:13Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

vadimeisenbergibm · 2018-11-22T07:43:41Z

@lizan Could you please reopen this PR? I would like to complete it for reference, and then I will close it and move it to istio/proxy.

lizan · 2018-11-23T10:44:09Z

@vadimeisenbergibm feel free to open another PR, this one is already too heavy to open...

add forward_original_sni to upstream TLS Context

7166095

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

cmluciano reviewed Sep 4, 2018

View reviewed changes

rshriram reviewed Sep 4, 2018

View reviewed changes

dnoe assigned lizan Sep 4, 2018

lizan suggested changes Sep 4, 2018

View reviewed changes

reorder the fields in UpstreamTlsContext, put forward_original_sni last

8e298cc

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

rshriram reviewed Sep 5, 2018

View reviewed changes

vadimeisenbergibm added 2 commits September 6, 2018 04:55

remove TODO related to oneof and clarified the configuration

fea6a13

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add Network::ConnectionSocket::OptionsSharedPtr parameter to createTr…

2f39451

…ansportSocket() Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix format

3cc57db

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 16, 2018

stale bot closed this Sep 23, 2018

ronensc and others added 3 commits October 8, 2018 18:45

Add forward-original-sni to context_config

f90f771

Signed-off-by: ronenschafferibm <ronen.schaffer@ibm.com>

Revert "fix format"

1d8578a

This reverts commit 3cc57db. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "add Network::ConnectionSocket::OptionsSharedPtr parameter to …

5e7bed7

…createTransportSocket()" This reverts commit 2f39451. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

ymesika mentioned this pull request Oct 10, 2018

Split Horizon EDS istio/istio#9222

Closed

Revert "Add forward-original-sni to context_config"

f8b4f60

This reverts commit f90f771. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm added 10 commits November 5, 2018 15:27

add OverrideRequestedServerNameWithoutSniInUpstreamTlsContext test

7b28148

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix format

0dc8998

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add passing nonempty value of override_server_name in UnknownCluster …

46dd604

…test Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add DynamicHostRemoveWithTls test

89ce5fc

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

tlsContext -> ssl_context according to the json schema

d44e6f2

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

remove ssl_context from DynamicHostRemoveWithTls

c2fada8

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

move setting hash key by the override_server_name field out of contai…

25c1aa6

…ning if statement since it should be dependant on context or downstream connection Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix a counter value due to changes

7bb1793

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add drained cb for example.com and ibm.com connection pools

ca99513

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix the number of drained connection pools

6b8ea04

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm changed the title ~~[WIP] Extensions: Network filter to forward SNI from the downstream TLS connection to the upstream TLS connection~~ Extensions: Network filter to forward SNI from the downstream TLS connection to the upstream TLS connection Nov 6, 2018

lizan suggested changes Nov 6, 2018

View reviewed changes

vadimeisenbergibm added 3 commits November 6, 2018 13:10

add vadimeisenbergibm to code owners of forward_original_sni extension

08602ac

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

extract override_server_name functionality into a separate PR

7ccaf1d

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

extract override_server_name functionality into a separate PR, leftovers

0f13892

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm mentioned this pull request Nov 6, 2018

tls: add functionality to override requested server name in the upstream cluster #4973

Merged

rshriram reviewed Nov 6, 2018

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 14, 2018

vadimeisenbergibm mentioned this pull request Nov 20, 2018

TLS origination doesn't seem to work with wildcard domains istio/istio#10041

Closed

stale bot closed this Nov 21, 2018

vadimeisenbergibm mentioned this pull request Nov 28, 2018

Forward Downstream SNI filter istio/proxy#2045

Merged

DanStough mentioned this pull request Jun 29, 2022

forward_downstream_sni Filter for Forward Proxy Egress #21953

Closed

Conversation

vadimeisenbergibm commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadimeisenbergibm commented Sep 4, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadimeisenbergibm commented Sep 5, 2018

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

vadimeisenbergibm commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Sep 5, 2018

Uh oh!

vadimeisenbergibm commented Sep 5, 2018

Uh oh!

rshriram commented Sep 5, 2018

Uh oh!

ggreenway commented Sep 5, 2018

Uh oh!

lizan commented Sep 5, 2018

Uh oh!

rshriram commented Sep 7, 2018

Uh oh!

ggreenway commented Sep 7, 2018

Uh oh!

stale bot commented Sep 16, 2018

Uh oh!

stale bot commented Sep 23, 2018

Uh oh!

vadimeisenbergibm commented Oct 10, 2018

Uh oh!

vadimeisenbergibm commented Nov 6, 2018

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Nov 6, 2018

Uh oh!

mattklein123 commented Nov 7, 2018

Uh oh!

lizan commented Nov 7, 2018

Uh oh!

stale bot commented Nov 14, 2018

Uh oh!

stale bot commented Nov 21, 2018

Uh oh!

vadimeisenbergibm commented Nov 22, 2018

Uh oh!

lizan commented Nov 23, 2018

Uh oh!

vadimeisenbergibm commented Sep 4, 2018 •

edited

Loading

vadimeisenbergibm commented Sep 5, 2018 •

edited

Loading