[filters] original source listener filter beginnings

[klarose]

Description: This adds an original_src listener filter to provide IP source transparency on upstream connections. This is an alternative implementation to PR #5255. It uses a socket option to set the local address on the upstream connection to the downstream remote address, as well as to ensure that it all connections from the same source IP go to the same connection pool. Since socket options are copied between downstream and upstream connections, we can be sure that the options will be applied to the upstream connection.

By setting the local address on the upstream connection, we can force it to bind to the original downstream remote address. We need to ensure that connection pools only handle traffic for a given source IP because they have no concept of the mapping between a downstream request and an upstream connection. The socketOption hashKey() handles this for us.

Currently this solution only works when there is no NAT/proxy in front of the downstream, or a PROXY protocol filter is run prior to the original_src filter. I want to add an http filter to run before the envoy.router filter so we can use http headers to get the address.

One possible issue with this solution is that we could leak connection pools, or the map we use to track them could grow too large; I'm not aware of any resource constraints on the number of connection pools created by the hash key mechanism).

TODO:

• Add the option to make the port a criteria of the connection as well.
• Set the MARK and IP_TRANSPARENT socket options.
• Add some documentation
• Add an HTTP version of the filter to handle XFF as a source of truth for the downstream remote address.
• Look into restricting the number of connection pools which could be created this way/add cleanup logic, etc.

Risk Level: Medium. By default this shouldn't do much, though I have made a small change to the connection code. The majority of this is in a filter that won't be brought in unless configured.

Testing: Lots of UT. Ran a simple client/server + envoy in docker to show that the transparency worked.
Docs Changes: TODO

[klarose]

@lizan @jrajahalme Here's an example of the alternative implementation, as discussed.

[jrajahalme]

On Dec 17, 2018, at 3:25 PM, Kyle Larose ***@***.***> wrote: @klarose commented on this pull request. In include/envoy/network/listen_socket.h <#5337 (comment)>: > @@ -25,6 +25,21 @@ class Socket { */ virtual const Address::InstanceConstSharedPtr& localAddress() const PURE; + /** + * Set the local address of the socket. On accepted sockets the local address defaults to the + * one at which the connection was received at, which is the same as the listener's address, if + * the listener is bound to a specific address. + * + * @param local_address the new local address. + * @param restored a flag marking the local address as being restored to a value that is + * different from the one the socket was initially accepted at. This should only be set + * to 'true' when restoring the original destination address of a connection redirected + * by iptables REDIRECT. The caller is responsible for making sure the new address is + * actually different when passing restored as 'true'. + */ + virtual void setLocalAddress(const Address::InstanceConstSharedPtr& local_address, + bool restored) PURE; + As discussed in #5255 <#5255>, I've moved this into the base Socket class.

Not sure if the “restored” semantics is needed for `Socket`. If not could also leave this in `ConnectionSocket` and add a method without the 2nd parameter to `Socket`. Jarno

[lizan]

At a first glance I prefer this than #5255, this seems cleaner and provides more extensibility to {listener,network,http} filters. Didn't look at details yet though. @jrajahalme @mattklein123 WDYT?

[mattklein123]

@jrajahalme @mattklein123 WDYT?

Sorry I promise I will look through all of this tomorrow morning.

[klarose]

/retest

[repokitteh-read-only]

🙀 Error while processing event:

evaluation error
error: finished: function github_get_statuses error: GET https://api.github.com/repos/envoyproxy/envoy/commits/c194e149cb99746edaaa66eab8e4b668dd6cd5b4/status: 502 Server Error []

🐱

Caused by: a #5337 (comment) was created by @klarose.

see: more, trace.

[klarose]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5337 (comment) was created by @klarose.

see: more, trace.

[lizan]

We have a list of follow up items but this PR LGTM as they are tracked. @mattklein123?

[mattklein123]

LGTM with one question. Thank you!

[mattklein123]

Do you plan on doing the full doc flow in a follow up? We should have complete config docs for this filter and it would be great to have some more general overview of all of the features we have that help with transparency?

[klarose]

Yes, I do. I already have it ready for review -- I was waiting for this to get merged before submitting a PR. Here's what it looks like right now: klarose@48e56e8

[klarose]

That said, I haven't covered the last bit -- an architectural overview of the feature(s). I can certainly do that as part of that PR.

[mattklein123]

Sounds good on a follow up. Yes please on some type of arch overview!

[vadimeisenbergibm]

@klarose @lizan @mattklein123 So is it possible now to set the upstream SNI by a downstream HOST header? I would like to use Header-to-Metadata filter to instruct Envoy to use the Host header for grouping connections in the connection pool and for setting SNI for the upstream connection, when the upstream connection performs TLS origination.

If it is not possible, what is missing?

[klarose]

@vadimeisenbergibm This doesn't add that feature. All I've done here is cause an upstream connection to use the same source IP as the downstream. Setting the SNI is at another level. I think @mattklein123 was considering the best way of communicating the SNI to the connection pool in that other thread. Here I have used a socket option to do so, leveraging the existing socket options pass to it. However, soon I will need to add yet another set of socket options. I thin Matt was wondering whether we could kill two birds with one stone and somehow provide the ability to pass both the SNI override and the new socket options forward... I haven't quite thought that far ahead yet.

As for your use-case, you want to set the upstream SNI based on some form of downstream information (related to istio MTLS, right)? I have a similar use-case: I want to set the upstream SNI based on some information I have gleaned dynamically from another downstream filter, similar to the original dst filter. So, I would certainly like to see something like this worked out if possible.

[vadimeisenbergibm]

@klarose My use case: Envoy performs TLS origination for HTTP traffic and sets upstream SNI according to the HOST header from the downstream.

@klarose @mattklein123 @lizan So how the feature above can be implemented? How should we proceed?

[klarose]

@vadimeisenbergibm I'm about to hack something up for an internal prototype on this. Let me try that, and then push it to my github repo. Maybe that can serve as a starting point? I don't plan on it being pretty enough to merge in as it as, and I won't have time to drive it to completion until I've finished my original_src work, but it should serve as an example somebody else could take over.

[vadimeisenbergibm]

@klarose Sounds like a plan! Note this PR that I implemented for forwarding SNI for TCP #4973.

[klarose]

@vadimeisenbergibm @mattklein123 @lizan See https://github.com/klarose/envoy/tree/upstream_sni_setter/source/extensions/filters/http/upstream_sni_from_header

It's pretty rough. Needs a lot of cleanup and a bit more design, but it works. I used a hacked up version of the header to metadata filter. We'll want to clean up the data model. I had to make some changes to the core conn pool creation code. Not sure how I feel about that, but I couldn't think of a nice alternative. If there are better suggestions, I'm all ears. :)

Note: I didn't finish the http2 implementation.

[vadimeisenbergibm]

@klarose Your filter sets TransportSocketOptions, but I think you still have to add code to pass TransportSocketOptions to TransportSocketFactory so SSLTransportSocketFactory will set the SNI. This is what I did in #5415, that PR also contains the code in Router that set TransportSocketOptions.

[klarose]

@vadimeisenbergibm one of my changes passes a list of transport socket options to the connection pool on creation. It then passes that to the new connection created when the active client is instantisted. That takes care of setting the upstream ani, since the transport socket option will be applied, doesn't it? I mean, it certainly worked for me. I proved it both functionally and with captures.

[vadimeisenbergibm]

one of my changes passes a list of transport socket options to the connection pool on creation. It then passes that to the new connection created when the active client is instantisted.

@klarose So where is this code?

[klarose]

@vadimeisenbergibm

https://github.com/klarose/envoy/commits/upstream_sni_setter

It's the "random cleanup" commit, coupled with the stream info one.
klarose@eb3faf9

klarose@d800e8b

Tl;dr: we add the option to the stream info. The lb context exposes the stream info. The connection manager uses that to pass the options to the connection pool.
Sorry for the bad naming. I was in a bit of a hurry to get it up this morning.

[vadimeisenbergibm]

@klarose I see. I was confused by "Random cleanup" and did not look into it. I would propose to extract that commit into a separate PR and call it "support passing TransportSocketOptions in HTTP filters", or "Pass TransportSockets to HTTP connection pool and to a new connection". I think that PR will be valuable on its own.

Then we can discuss how to implement the upstream_sni_setter filter.