[WIP] Network-Level SNI reader filter

[ronenschafferibm]

Description:
This PR adds a network filter that reads the network-level SNI. The filter is based on TLS inspector
This PR solves #4076.
It is not ready yet. TODOs must be fixed before merge.

Risk Level:
Medium

Testing:
I've created a filter (GetSNIs) that emits to the log the connection SNI and the network-level SNI.
It can be found here: https://github.com/ronenschafferibm/envoy-get-snis-filter

Fixes #Issue:
#4076

[vadimeisenbergibm]

cc @cmluciano

[cmluciano]

semi-related xref: #4144

[cmluciano]

I'm wondering if the requestinfo for requestedServerName might be leveraged here

cc @zuercher since he is reviewing my linked PR

[ggreenway]

I just added a comment in issue #4076. This seems like a very specialized/corner use case, and I'm wondering if there's a better way to deal with it.

[vadimeisenbergibm]

An example usage is in https://github.com/ronenschafferibm/envoy-get-snis-filter/blob/master/envoy_config2.json. See the listeners below.

Note the filters:

1. The envoy.listener.tls_inspector listener filter , reads the outer SNI from the mTLS, envoy2.local and sets connection's socket's requestedServerName value.
2. The envoy.network_level_sni_reader filter (this PR), reads the inner SNI from the original TLS, after the mTLS is terminated and sets network_level_requested_server_name_ into an implementation class of ReadFilterCallbacks.
3. The GetSNIsFilter filter - an example custom filter, reads the values of both inner and outer SNIs and prints them into the log.

"listeners": [
      {
        "address": {
          "socket_address": {
            "address": "0.0.0.0",
            "port_value": 15002
          }
        },
        "listener_filters": [
          {
            "name": "envoy.listener.tls_inspector",
            "config": {
            }
          }
        ],
        "filter_chains": [
          {
            "filter_chain_match": {
              "server_names": "envoy2.local"
            },
            "tlsContext": {
              "commonTlsContext": {
                "tlsCertificates": [
                  {
                    "certificateChain": {
                      "filename": "/etc/my-envoy-certs/envoy2.crt"
                    },
                    "privateKey": {
                      "filename": "/etc/my-envoy-certs/envoy2.key"
                    }
                  }
                ],
                "validationContext": {
                  "trustedCa": {
                    "filename": "/etc/my-envoy-certs/envoy1.crt"
                  }
                },
                "alpnProtocols": [
                  "h2",
                  "http/1.1"
                ]
              },
              "requireClientCertificate": true
            },
            "filters": [
              {
                "name": "envoy.network_level_sni_reader",
                "config": {}
              },
              {
                "name": "GetSNIsFilter",
                "config": {}
              },
              {
                "name": "envoy.tcp_proxy",
                "config": {
                  "stat_prefix": "envoy2.local",
                  "access_log": [
                    {
                      "config": {
                        "path": "/dev/stdout"
                      },
                      "name": "envoy.file_access_log"
                    }
                  ],
                  "cluster": "cnn"
                }
              }
            ]
          }
        ]
      }
    ]

[ggreenway]

I have some concerns about this approach:

• This has substantial code duplication from tls-inspector
• This adds very filter-specific methods to a general-purpose interface (ReadFilterCallbacks). I think we need to come up with a better way for filters to attach metadata to a connection than this, if we pursue this approach.
• How will this filter be used in the end? I see in your example it's just logged in a later filter. Will you eventually want to route based on this? How will that work?

[vadimeisenbergibm]

An example usage without envoy.listener.tls_inspector, for a tcp proxy. Here there is no filter chain selection, just a plain tcp proxy on port 443.

See the listeners below.

Note the filters:

1. The envoy.network_level_sni_reader filter (this PR), reads the SNI from the TLS and sets in the ReadFilterCallbacks.
2. The GetSNIsFilter filter - an example custom filter, reads the value of the SNI and prints it into the log.

"listeners": [
      {
        "address": {
          "socket_address": {
            "address": "0.0.0.0",
            "port_value": 443
          }
        },
        "filter_chains": [
          {
            "filters": [
              {
                "name": "envoy.network_level_sni_reader",
                "config": {}
              },
              {
                "name": "GetSNIsFilter",
                "config": null
              },
              {
                "name": "envoy.tcp_proxy",
                "config": {
                  "stat_prefix": "tcp_proxy",
                  "access_log": [
                    {
                      "config": {
                        "path": "/dev/stdout"
                      },
                      "name": "envoy.file_access_log"
                    }
                  ],
                  "cluster": "original_dst"
                }
              }
            ]
          }
        ]
      },

[vadimeisenbergibm]

@PiotrSikora Could you please review the proposed filter's usage and the initial implementation?

[vadimeisenbergibm]

@ggreenway Thank you for your feedback.

This has substantial code duplication from tls-inspector

We can refactor the implementation to remove code duplication, to share code with tis-inspector.

This adds very filter-specific methods to a general-purpose interface (ReadFilterCallbacks). I think we need to come up with a better way for filters to attach metadata to a connection than this, if we pursue this approach.

So where this metadata should be attached?

How will this filter be used in the end?

The filter will be used by other filters that need the value of SNI.
Use cases:

1. to report SNI to the control plane
2. for the TCP access logger to log SNI
3. use the value of SNI in the RBAC filter, to specify who can access with site by the value of SNI

I guess there are more use cases, controlling TLS and monitoring the SNI values should be a pretty important topic.

[ggreenway]

If we start adding more things depending on this, all of them need to keep track of if they want to look at the outer or the inner SNI value, so the complication will spread.

If we're going to keep going with the route of a filter like this, I think we'd be better off overriding the existing SNI value from this filter somehow, ie if another filter after this one asks for the SNI value, it gets the value this filter discovered.

I think a more correct solution to this problem is a more general notion of tunneling. The no-work implementation, as I noted in the linked issue, is just to have envoy connect back to itself, although that has an effect on performance. If the performance of that is of substantial impact, we could look into adding a more direct loopback into envoy.

The advantage of a general tunneling approach is that it is clear at each layer what data is being operated on. You'd have two separate filter chains, one for the outer and one for the inner. Any logging, routing, etc would only use the data available in that layer.

[vadimeisenbergibm]

@ggreenway

I think a more correct solution to this problem is a more general notion of tunneling.

This is what we want to work in the long term (cc @elevran @rshriram). Can this filter still be useful as a short term solution?

@PiotrSikora @mattklein123 @rshriram Could you please provide your comments on the ideas of @ggreenway in
#4236 (comment) ?

Could you please instruct me and @ronenschafferibm on the short term vs. long term solutions and possible course of action?

[htuch]

If we do go with this PR, can you factor out as much common stuff with the TLS inspector as possible? It might only be a small amount of code, but I would rather we have a single place to fix/maintain/audit. Thanks.

[vadimeisenbergibm]

Sure, we will refactor the code to create as less code duplication as possible, and will follow all the code review remarks and the requirements of Envoy.

[rshriram]

I feel that the approach this PR takes is not a useful long term solution for the problem it is tackling. (see my comment in the related issue).

If you really want to go with trusting the SNI sent by the app, then I suggest you experiment with this filter for the internal watson use case before pushing it into the community code base. From that point of view, the refactoring done in the SSL code base is fine, as it allows filters like these to be built for internal use.

[rshriram]

Are you going to parse the client Hello for every on data call?

[vadimeisenbergibm]

No, this code is still WIP. We will parse the client hello in the first calls to onData, and then set a flag that we are done and return Network::FilterStatus::Continue immediately.

[vadimeisenbergibm]

@rshriram We need the ability to stick the inner SNI somewhere, for example in the ReadFilterCallbacks. Additional nice thing is to extract parseClientHello() from tls inspector to ssl/utilities.cc, so our custom filter will be able to use parseClientHello, and to stick it into ReadFilterCallbacks.

Having said that, I still think that the requirement is a generic one and is very simple - to report the inner SNI in case of mTLS between Envoys. The solution is also very simple - the refactoring, the additional field, and a simple filter.

[ronenschafferibm]

Closing this PR for the sake of a new PR #4331 that includes only the changes needed to tls_inspector so its logic could be shared with custom filters.

[vadimeisenbergibm]

@rshriram @lizan @PiotrSikora

We implemented the use case as required by our customers for Secure Egress Control, the design document is here - https://goo.gl/cQ29rX. I insist that the requirements are generic. The scenario that demonstrates the requirements is here - istio/istio.io#2608.

The only missing part to implement all the requirements in the open source is this PR and a PR in istio/proxy istio/proxy#1971. This PR adds a small filter, 71 lines of code for the .cc file of the filter. The filter provides missing functionality described in the issue #4076.

The change in istio-proxy is 6 lines of code (istio/proxy#1971). These are all the changes we need to implement the requirements of the Secure Egress Control in Istio. While there are other solutions that are long-term, I think the current PR and the PR in istio-proxy are simple enough to be introduced for the short term, and to be reimplemented later as part of other long-term solutions.

I would like to ask you to reconsider this PR and to consider to accept it as a short term solution, to be refactored/reimplemented later.