connection: Set the SOL_IP,IP_TRANSPARENT option on listen sockets #2719

rlenglet · 2018-03-05T18:57:52Z

connection: Set the SOL_IP,IP_TRANSPARENT option on listen sockets

Implement the "transparent" option in the LDS Listener to set the SOL_IP/IP_TRANSPARENT option on listen sockets, which allows using Envoy with the iptables TPROXY target. Unlike the iptables REDIRECT target, TPROXY allows preserving both the source and destination IP addresses and ports of accepted connections.

Update the OriginalDst filter to restore the original destination IP address of TPROXY connections in addition to REDIRECT connections.

Signed-off-by: Romain Lenglet romain@covalent.io
Signed-off-by: Jarno Rajahalme jarno@covalent.io
Risk Level: Low

ggreenway · 2018-03-05T19:05:52Z

source/common/network/listen_socket_impl.cc

+  // instead of REDIRECT to get both the original source and destination addresses of the
+  // redirected socket.
+  on = 1;
+  rc = setsockopt(fd_, SOL_IP, IP_TRANSPARENT, &on, sizeof(on));


I don't think this should be set unconditionally; this should only be done based on some configuration.

dnoe · 2018-03-05T19:10:00Z

This seems like it should be configurable per listener. Can you create a PR in https://github.com/envoyproxy/data-plane-api with a proposed modification to LDS configuration?

rlenglet · 2018-03-05T19:12:00Z

Will do. Thanks!

rlenglet · 2018-03-05T20:37:04Z

Submitted this PR to data-plane-api: envoyproxy/data-plane-api#522.
Will work on implementing the flag.

PiotrSikora

See: #2659.

PiotrSikora · 2018-03-06T00:06:47Z

source/common/network/listen_socket_impl.cc

+  // instead of REDIRECT to get both the original source and destination addresses of the
+  // redirected socket.
+  on = 1;
+  rc = setsockopt(fd_, SOL_IP, IP_TRANSPARENT, &on, sizeof(on));


AFAIK, this should be IP_TRANSPARENT for IPv4 and IPV6_TRANSPARENT for IPv6.

Fixed in upcoming commit.

Note that this won't work for IPv6 when building using Envoy's Docker image (Ubuntu 16.04.3 with libc6-dev 2.23-0ubuntu10) because IPV6_TRANSPARENT is not defined in that image:

$ ./ci/run_envoy_docker.sh 'grep IPV6_TRANSPARENT /usr/include/x86_64-linux-gnu/bits/in.h'

returns nothing.

On Ubuntu 17.10 with libc6-dev 2.26-0ubuntu2.1, this macro is defined as expected:

$ grep IPV6_TRANSPARENT /usr/include/x86_64-linux-gnu/bits/in.h #define IPV6_TRANSPARENT 75

PiotrSikora · 2018-03-06T00:07:32Z

source/common/network/listen_socket_impl.cc

+  rc = setsockopt(fd_, SOL_IP, IP_TRANSPARENT, &on, sizeof(on));
+  if (rc == -1) {
+    if (errno == EPERM) {
+      // Ignore. Not running with CAP_NET_ADMIN capability.


This is still a real error, if configuration requires IP_TRANSPARENT.

Fixed in upcoming commit.

PiotrSikora · 2018-03-06T00:09:24Z

source/common/network/listen_socket_impl.cc

@@ -37,6 +38,22 @@ TcpListenSocket::TcpListenSocket(const Address::InstanceConstSharedPtr& address,
  int rc = setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
  RELEASE_ASSERT(rc != -1);

+#if defined SOL_IP && defined IP_TRANSPARENT


I believe this can be moved into if (bind_to_port) { ... }, since as far as I recall, it's useless without bind().

One could make the same argument about the existing setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, ...) above. Should I move all setsockopt calls into if (bind_to_port) { ... }?

PiotrSikora · 2018-03-06T00:09:32Z

source/common/network/listen_socket_impl.cc

+  // instead of REDIRECT to get both the original source and destination addresses of the
+  // redirected socket.
+  on = 1;
+  rc = setsockopt(fd_, SOL_IP, IP_TRANSPARENT, &on, sizeof(on));


rlenglet · 2018-03-06T02:21:28Z

Re: #2659, this is a different, complementary issue. You need first to implement IP_TRANSPARENT in the inbound listen socket anyway, like I do here, so that you get the original source address. Without that, you won't know what address to bind to.

rlenglet · 2018-03-06T16:55:35Z

The mac tests timed out. It doesn't seem to be a functional failure. Is that something I should look into?

…#522) Add a "transparent" option to Listener to set the SOL_IP/IP_TRANSPARENT option on listen sockets, which allows using Envoy with the iptables TPROXY target. Unlike the iptables REDIRECT target, TPROXY allows preserving both the source and destination IP addresses and ports of accepted connections. API changes for: envoyproxy/envoy#2719 Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch · 2018-03-06T20:31:38Z

@rlenglet probably #2428, let's see if that is fixed here.

Pull a quick-and-dirty implementation of the SOL_IP,IP_TRANSPARENT option setup on listen sockets: https://github.com/rlenglet/envoy/tree/quick-and-dirty-transparent This will be reverted when the proper, clean change is merged upstream into Envoy: envoyproxy/envoy#2719 Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch · 2018-03-08T18:05:29Z

@rlenglet can you merge to resolve conflict?
@PiotrSikora @dnoe @ggreenway any further comments?

mattklein123 · 2018-03-08T18:09:09Z

include/envoy/server/listener_manager.h

   * @param bind_to_port supplies whether to actually bind the socket.
   * @return Network::SocketSharedPtr an initialized and potentially bound socket.
   */
  virtual Network::SocketSharedPtr
-  createListenSocket(Network::Address::InstanceConstSharedPtr address, bool bind_to_port) PURE;
+  createListenSocket(Network::Address::InstanceConstSharedPtr address, bool transparent,


Q: Do we foresee having more options like this in the future? If so should we switch to some type of options struct that gets passed around? That would a avoid having to change every individual callsite and test again in the future potentially. Just throwing it out there.

I personally don't foresee more options like this. But if we think that could happen, I could use #2734 to implement this internally.

If we don't foresee more options like this than I wouldn't worry about it.

I have one more option coming that is similar/related: enabling tcp-fastopen on listening sockets.

@ggreenway So what is your preference? Should we implement this with #2734?

question.. how does IP_FREEBIND differ from IP_TRANSPARENT?

@ggreenway Would it work for you better if I generalized the current option setting code (i.e., #2734 ) to accept multiple option setters, maintain a list of set options and have the call sites call all of them (unless one of them fails)?

@dnoe OK SGTM if you want to do the struct stuff.

@jrajahalme yes that sounds reasonable

FREEBIND allows binding to an address that isn't configured on any interface on the machine. IP_TRANSPARENT would also allow this, but from inside Envoy should otherwise work the same as a regular listener (whereas transparent is designed around a different use case where the traffic has been redirected to Envoy and has some original destination that is meaningful).

dnoe

Looks good to me.

PiotrSikora

Since there are no tests (due to iptables), did anybody other than the author tested that it really works?

Another questions is, how does this interact with SO_ORIGINAL_DST and PROXY protocol?

PiotrSikora · 2018-03-08T23:04:54Z

test/server/listener_manager_impl_test.cc

+  EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromJson(listener_foo_json), true));
+  checkStats(1, 0, 0, 0, 1, 0);
+
+  // Update foo listener, but with a different transparent option. Should throw.


Is there a reason why we cannot disable IP_TRANSPARENT for the new listener?

@PiotrSikora I was thinking that we have an atomicity issue. How do we atomically hand off the socket to the new listener and set/unset the option? That would require that there is a period of time when no thread is accepting on the socket during the handoff. I don't think we have that (?). And that's not desirable, since that would cause connections to be rejected during that period.

@rlenglet there is always a period of time when no thread is accepting new connections (i.e. when all threads are busy processing active requests), and no connections are rejected because of kernel-maintained backlog queue on the listening socket, so I don't think lack of atomicity is the problem.

Having said that, I'm fine if you want to leave it as-is.

I'm removing that verification. We know from an accepted socket's option whether it has been TPROXYed or not, so we can safely have a mix of connections with and without TPROXY in the same queue.

rlenglet · 2018-03-08T23:47:28Z

@PiotrSikora It should have no interaction with SO_ORIGINAL_DST or PROXY protocol. It basically has no effect unless one uses iptables TPROXY to redirect connections to the socket.

PiotrSikora · 2018-03-08T23:55:00Z

@rlenglet my point was what happens if you use IP_TRANSPARENT with SO_ORIGINAL_DST and/or PROXY protocol? I believe that PROXY protocol will just work fine (and take priority), but the behavior of SO_ORIGINAL_DST is unclear, and if the two are incompatible, then perhaps config guard is in order?

htuch · 2018-03-28T18:58:16Z

@rlenglet can you take a look at #2922? I started with some of the pieces in this PR, but there was a bunch of cleanup I did to make it (1) avoid the #ifdef proliferation, (2) add support for upstream as well as listener and (3) work with additional options without boiletplate. I think it might make sense to merge that one first and then this present PR should be much simpler.

rlenglet · 2018-03-28T21:24:12Z

@htuch If you can get #2922 merged soon, I'm fine with it getting merged first.

htuch · 2018-03-29T19:54:20Z

@rlenglet #2922 has merged.

rlenglet · 2018-03-29T20:19:10Z

@htuch Great! I'll merge from master today.

…nsparent Signed-off-by: Romain Lenglet <romain@covalent.io>

rlenglet · 2018-03-30T02:03:19Z

@htuch Please review. I merged from master.
I fixed the calls to the SocketOptionImpl constructor to pass empty optionals when the options were not set.

Pull a quick-and-dirty implementation of the SOL_IP,IP_TRANSPARENT option setup on listen sockets: https://github.com/rlenglet/envoy/tree/quick-and-dirty-transparent This will be reverted when the proper, clean change is merged upstream into Envoy: envoyproxy/envoy#2719 Signed-off-by: Romain Lenglet <romain@covalent.io>

Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch

This looks great, the implementation is ready to ship. Can you add a listener_manager_impl_test similar to what is done for freebind and was done in earlier revisions?

mattklein123 · 2018-03-30T16:50:14Z

Love, love, love how simple this all became. Great work @ggreenway @PiotrSikora @htuch @rlenglet!

@rlenglet can we get the doc/release note PR redone before we merge this? Thank you.

Signed-off-by: Romain Lenglet <romain@covalent.io>

rlenglet · 2018-03-30T17:28:14Z

@htuch Added tests. Looks clean.

rlenglet · 2018-03-30T17:37:02Z

@mattklein123 Here is the release note PR: envoyproxy/data-plane-api#593
Thanks!

rlenglet · 2018-03-30T17:38:40Z

The ipv6_tests failure is due to the test environment:

ERROR: /source/bazel/repositories.bzl:203:5: no such package '@boringssl//': Traceback (most recent call last):
	File "/build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external/bazel_tools/tools/build_defs/repo/git.bzl", line 69
		_clone_or_update(ctx)
	File "/build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external/bazel_tools/tools/build_defs/repo/git.bzl", line 44, in _clone_or_update
		fail(("error cloning %s:\n%s" % (ctx....)))
error cloning boringssl:
+ cd /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external
+ rm -rf /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external/boringssl
+ git clone https://boringssl.googlesource.com/boringssl /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external/boringssl
Cloning into '/build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/external/boringssl'...
fatal: unable to access 'https://boringssl.googlesource.com/boringssl/': Failed to connect to boringssl.googlesource.com port 443: Network is unreachable
 and referenced by '//external:ssl'

htuch · 2018-03-30T17:39:15Z

test/common/network/socket_option_impl_test.cc

@@ -82,6 +126,12 @@ TEST_F(SocketOptionImplTest, SetOptionFreeebindSuccessFalse) {
  }
 }

+// Transparent settings have no effect on post-bind behavior.


I thought transparent does have an effect on both prebind and postbind?

Right. I will relax that.

Added a commit to support setting IP_TRANSPARENT both pre-bind and post-bind.

Signed-off-by: Romain Lenglet <romain@covalent.io>

ggreenway

LGTM; just questioning whether we can remove the v1 support.

ggreenway · 2018-03-30T18:45:53Z

source/common/json/config_schemas.cc

@@ -185,6 +185,7 @@ const std::string Json::Schema::LISTENER_SCHEMA(R"EOF(
       },
       "drain_type": {"type" : "string", "enum" : ["default", "modify_only"]},
       "ssl_context" : {"$ref" : "#/definitions/ssl_context"},
+       "transparent" : {"type": "boolean"},


Is the v1 API absolutely required?

We'll need it for Istio. Cf. istio/istio#4654 (review)

htuch · 2018-03-30T20:43:15Z

Thanks @rlenglet for all your patience here.

rlenglet · 2018-03-30T20:57:05Z

@htuch Thanks a lot!

ggreenway requested changes Mar 5, 2018

View reviewed changes

htuch assigned dnoe and ggreenway Mar 5, 2018

PiotrSikora suggested changes Mar 6, 2018

View reviewed changes

rlenglet mentioned this pull request Mar 6, 2018

lds: Add "transparent" option to support iptables TPROXY in listeners envoyproxy/data-plane-api#522

Merged

rlenglet mentioned this pull request Mar 6, 2018

listen_socket: Support multiple options and allow socket options to be set before bind(). #2734

Merged

rlenglet mentioned this pull request Mar 6, 2018

[WIP] Build Envoy proxy with Istio filters and TPROXY support cilium/cilium#3035

Closed

htuch assigned PiotrSikora and htuch Mar 8, 2018

rlenglet mentioned this pull request Mar 8, 2018

Filter HTTP in Istio sidecar proxy cilium/cilium#3065

Closed

30 tasks

mattklein123 reviewed Mar 8, 2018

View reviewed changes

dnoe previously approved these changes Mar 8, 2018

View reviewed changes

PiotrSikora reviewed Mar 8, 2018

View reviewed changes

htuch mentioned this pull request Mar 28, 2018

socket: IP_FREEBIND support for listeners and upstream connections. #2922

Merged

Merge remote-tracking branch 'upstream/master' into listen-socket-tra…

b6ede70

…nsparent Signed-off-by: Romain Lenglet <romain@covalent.io>

This was referenced Mar 30, 2018

Update Envoy dependency to support transparent listeners istio/proxy#1338

Merged

Use iptables TPROXY instead of REDIRECT for inbound traffic istio/istio#4654

Merged

Fix tests

b4d7134

Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch suggested changes Mar 30, 2018

View reviewed changes

rlenglet added 2 commits March 30, 2018 17:05

Clean up Listener config mock

fd77c94

Signed-off-by: Romain Lenglet <romain@covalent.io>

Add tests for transparent option

899de9e

Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch reviewed Mar 30, 2018

View reviewed changes

rlenglet added 2 commits March 30, 2018 17:57

Support setting the IP_TRANSPARENT socket option post-bind

7f7eba8

Signed-off-by: Romain Lenglet <romain@covalent.io>

Fix tests for setting transparent option post-bind

4dd204a

Signed-off-by: Romain Lenglet <romain@covalent.io>

htuch approved these changes Mar 30, 2018

View reviewed changes

ggreenway requested changes Mar 30, 2018

View reviewed changes

ggreenway approved these changes Mar 30, 2018

View reviewed changes

htuch merged commit 3b5fb0e into envoyproxy:master Mar 30, 2018

rlenglet deleted the listen-socket-transparent branch March 30, 2018 20:56

connection: Set the SOL_IP,IP_TRANSPARENT option on listen sockets #2719

connection: Set the SOL_IP,IP_TRANSPARENT option on listen sockets #2719

Conversation

rlenglet commented Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe commented Mar 5, 2018

rlenglet commented Mar 5, 2018

rlenglet commented Mar 5, 2018

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlenglet commented Mar 6, 2018

rlenglet commented Mar 6, 2018

htuch commented Mar 6, 2018

htuch commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe left a comment

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlenglet commented Mar 8, 2018 • edited Loading

PiotrSikora commented Mar 8, 2018

htuch commented Mar 28, 2018

rlenglet commented Mar 28, 2018

htuch commented Mar 29, 2018

rlenglet commented Mar 29, 2018

rlenglet commented Mar 30, 2018

htuch left a comment

Choose a reason for hiding this comment

mattklein123 commented Mar 30, 2018

rlenglet commented Mar 30, 2018

rlenglet commented Mar 30, 2018

rlenglet commented Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

rlenglet Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Mar 30, 2018

rlenglet commented Mar 30, 2018

rlenglet commented Mar 5, 2018 •

edited

Loading

PiotrSikora Mar 10, 2018 •

edited

Loading

rlenglet commented Mar 8, 2018 •

edited

Loading

rlenglet commented Mar 30, 2018 •

edited

Loading

rlenglet Mar 30, 2018 •

edited

Loading