Listener: reset the file event when destroying listener filters #16952

soulxu · 2021-06-12T13:37:44Z

Commit Message: Listener: reset the file event before initializing new one
Additional Description:
The listener filter may add event on the new socket, but it won't cleanup the event.
If continue_on_listener_filters_timeout is set, the new connection may add same event
to the socket. So reset the file event before initialize new one.
Risk Level: low
Testing: integration test added
Docs Changes: n/a
Release Notes: n/a
Fixes #16951

Signed-off-by: He Jie Xu hejie.xu@intel.com

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2021-06-13T00:54:55Z

source/common/network/io_socket_handle_impl.cc

+  // the same file descriptor. This is not allowed.
+  if (file_event_ != nullptr) {
+    file_event_.reset();
+  }


Another fix will be to reset the event in the destructor of the listener filter, but that would require every listener filter to take care of it. That is why I change here.

Also, this fix doesn't prevent initiate two instances of FileEventImpl directly, it is how the bug istio/istio#18229 was trigger initially, but I searched the code, we don't have that case anymore. Since we create FileEventImpl only through the dispatcher. So maybe we can move the constructor of FileEventImpl as protected, and only dispatcher can instantiate it.

I have concerns about file_event_ referring to a Event::FileReadyCb cb that points to a deleted listener filter. Given that, I think it is important that we find a way to reset the file event when the listen filter is destroyed, rather than hack it this way. Who owns the listener IoSocketHandleImpl?

I think that the 2 listener filters that seem to be calling initializeFileEvent are tls_inspector and http_inspector. Is the issue here that those filters need events during some parts of the connection lifetime, but eventually need to remove themselves from accepting events directly?

It would be useful to know more about when this ASSERT fails, and possibly change this from ASSERT to RELEASE_ASSERT. Do the added tests cause this ASSERT to be hit?

One option that could be considered for cases like this one is to add a RAII object that can be used for filters like this one to hold the file_event registration in cases where the filter does own a the file event registration.

Also, I think that falling back to the default filter chain in cases where tls inspector times out may not be the right thing. If the client is not sending a handshake within a reasonable time, the proxy should just close the connection.

If this happens only on continue_on_listener_filters_timeout_, would resetting the file_event in ActiveTcpSocket::onTimeout() solve the issue?

No, because ActiveTcpSocket doesn't know who owns the relevant connection socket. The filters need to properly manage the file events they own.

I looked at the code of TLS inspector and I think I understand the issue better. I have concerns about the TLS inspector filter effectively removing itself from the filter chain on timeout. This behavior allows for trivial bypass of the TLS inspector and HTTP inspector functionality by waiting for a few seconds before sending the rest of the client hello. Are these inspectors purely debug hooks or are they meant to provide specific functionality or security properties?

Thanks for confirming that your new tests repo the ASSERT failure. I suggest we try to fix this issue by calling resetFileEvent in TLS inspector and HTTP inspector destructors if the filters haven't invoked their respective done callbacks.

I looked at the code of TLS inspector and I think I understand the issue better. I have concerns about the TLS inspector filter effectively removing itself from the filter chain on timeout. This behavior allows for trivial bypass of the TLS inspector and HTTP inspector functionality by waiting for a few seconds before sending the rest of the client hello. Are these inspectors purely debug hooks or are they meant to provide specific functionality or security properties?

Those inspectors are meant to provider specific functionality, for example, tls inspect get the SNI for later the filter chain matching.

Thanks for confirming that your new tests repo the ASSERT failure. I suggest we try to fix this issue by calling resetFileEvent in TLS inspector and HTTP inspector destructors if the filters haven't invoked their respective done callbacks.

got it, thanks for the review!

I looked at the code of TLS inspector and I think I understand the issue better. I have concerns about the TLS inspector filter effectively removing itself from the filter chain on timeout. This behavior allows for trivial bypass of the TLS inspector and HTTP inspector functionality by waiting for a few seconds before sending the rest of the client hello. Are these inspectors purely debug hooks or are they meant to provide specific functionality or security properties?

Those inspectors are meant to provider specific functionality, for example, tls inspect get the SNI for later the filter chain matching.

This seems problematic since the SNI extraction logic of the filter is trivially bypass-able by waiting for a timeout condition before sening the SNI. Is there potential for that to cause any security concerns?

Thanks for confirming that your new tests repo the ASSERT failure. I suggest we try to fix this issue by calling resetFileEvent in TLS inspector and HTTP inspector destructors if the filters haven't invoked their respective done callbacks.

got it, thanks for the review!

Those inspectors are meant to provider specific functionality, for example, tls inspect get the SNI for later the filter chain matching.

This seems problematic since the SNI extraction logic of the filter is trivially bypass-able by waiting for a timeout condition before sening the SNI. Is there potential for that to cause any security concerns?

I searched that, the Istio is using that https://github.com/istio/istio/blob/f38465f8f5c1edd944ed7776c1253ffa2d9768b2/pilot/pkg/networking/core/v1alpha3/listener_builder.go#L213

the protocolDetectionTimeout's doc probably explains the reason https://istio.io/latest/docs/reference/config/istio.mesh.v1alpha1/

so it should be for a listener accepts both tls and server-first protocol connection.

I do appreciate the desire to handle all kinds of things transparently, but adding significant delays for all connections to server-talks-first protocols seems broken.

soulxu · 2021-06-14T15:02:44Z

/retest

repokitteh-read-only · 2021-06-14T15:02:48Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16952 (comment) was created by @soulxu.

see: more, trace.

antoniovicente

Thanks for the fix. It would be useful to know more about what is going on here in order to evaluate if ignoring the file event re-registration is the right fix.

antoniovicente · 2021-06-14T21:20:22Z

source/common/network/io_socket_handle_impl.cc

+  // the same file descriptor. This is not allowed.
+  if (file_event_ != nullptr) {
+    file_event_.reset();
+  }


I have concerns about file_event_ referring to a Event::FileReadyCb cb that points to a deleted listener filter. Given that, I think it is important that we find a way to reset the file event when the listen filter is destroyed, rather than hack it this way. Who owns the listener IoSocketHandleImpl?

I think that the 2 listener filters that seem to be calling initializeFileEvent are tls_inspector and http_inspector. Is the issue here that those filters need events during some parts of the connection lifetime, but eventually need to remove themselves from accepting events directly?

It would be useful to know more about when this ASSERT fails, and possibly change this from ASSERT to RELEASE_ASSERT. Do the added tests cause this ASSERT to be hit?

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

test/extensions/filters/listener/http_inspector/http_inspector_test.cc

antoniovicente · 2021-06-18T18:58:14Z

source/extensions/filters/listener/http_inspector/http_inspector.h

@@ -66,6 +66,11 @@ using ConfigSharedPtr = std::shared_ptr<Config>;
 class Filter : public Network::ListenerFilter, Logger::Loggable<Logger::Id::filter> {
 public:
  Filter(const ConfigSharedPtr config);
+  ~Filter() override {
+    if (cb_) {
+      cb_->socket().ioHandle().resetFileEvents();


It this filter guaranteed to be the owner of the file event on the ioHandle at this point, or is it possible that some other filter owns the file event?

cb_ remains set after calls to resetFileEvents() in http_inspector.cc

Yes, it could be owned by other filter. Like we enable tls inspect and http inspect at same time, tls inspect successed, then http inspect timeout, both filter will reset the file event in the destruction. But it should be fine since the reset can be execute multiple times.

The question is wherever a filter that is not the TLS inspector nor HTTP inspector could own the file event. For example, the network::Connection owned by the http connection manager.

I think there is no other filter own the file event. Since this is on the stage of accepting the connection, so the l3/l4 filter doesn't instance yet.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente

Looks good. Just a few nits and requests to tighten test expectations.

/wait

source/extensions/filters/listener/proxy_protocol/proxy_protocol.h

source/extensions/filters/listener/tls_inspector/tls_inspector.h

source/server/active_tcp_listener.cc

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc

antoniovicente · 2021-06-21T18:49:51Z

test/integration/listener_filter_integration_test.cc

+  Buffer::OwnedImpl buffer("fake data");
+  client_->write(buffer, false);
+  // the timeout is set as one seconds, sleep 5 to trigger the timeout.
+  absl::SleepFor(absl::Seconds(5));


Would it be possible to use simulated time to avoid such a long sleep?

Also, see comments above regarding additional expectations we could add in order to ensure that this test case goes down the expected code paths.

let me see how to simulate time in integration test. If there is way, I should be able fix the proxy filter unittest also.

I changed the proxy filter unitest to use SimulatedTimeSystemHelper. But for the integration test, I saw the BaseIntegrationTest is using the real time-system.

envoy/test/integration/base_integration_test.h

Line 259 in f8d8023

Event::GlobalTimeSystem time_system_;

so I thought the integration is preferred to run at real timesystem?

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente

Many thanks for this awesome fix with comprehensive testing. You rock!

soulxu · 2021-06-23T02:03:02Z

@antoniovicente thanks for your review!

…bridge-stream * upstream/main: (268 commits) tools: adding dio,better comments (envoyproxy#17104) doc: fix misplaced #[extension-category] for Wasm runtimes (envoyproxy#17078) ci: Speedup deps precheck (envoyproxy#17102) doc: fix wrong link on wasm network filter. (envoyproxy#17079) docs: Added v3 API reference. (envoyproxy#17095) docs: Update include paths in repo (envoyproxy#17098) exception: make Ipv6Instance and Ipv4Instance not throw and remove some try catch pattern (envoyproxy#16122) tools: adding reminders for API shephards (envoyproxy#17081) ci: Fix wasm verify example (envoyproxy#17086) [fuzz]: fix oss fuzz bug 34515, limit maglev table size (envoyproxy#16671) test: silencing flaky test (envoyproxy#17084) Set `validate` flag when the SAN(SubjectAltName) matching is performed (envoyproxy#16816) Listener: reset the file event when destroying listener filters (envoyproxy#16952) docs: link additional filters that emit dynamic metadata (envoyproxy#17059) rds: add config reload time stat for rds (envoyproxy#17033) bazel: Use color by default for build and run commands (envoyproxy#17077) ci: Add timing for docker pull (envoyproxy#17074) [Windows] Adding note section in Original Source HTTP Filter (envoyproxy#17058) quic: add quic version counters in http3 codec stats. (envoyproxy#16943) quiche: change crypto stream factory interfaces (envoyproxy#17046) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

mattklein123 · 2021-06-28T03:54:33Z

@soulxu @antoniovicente I'm late to the discussion and I see quite a bit of back and forth, but it seems sub-optimal that each listener filter has to include this new stanza to release the file event. Are we going to follow up with some RAII wrapper? Or even better is there some way in which the code that is running the filters (IIRC ActiveTcpConncetion or something like that) could keep track of dealing with this if needed? Apologize if this has already been discussed at length, but a summary might be nice. Thank you.

soulxu · 2021-06-28T12:48:51Z

@soulxu @antoniovicente I'm late to the discussion and I see quite a bit of back and forth, but it seems sub-optimal that each listener filter has to include this new stanza to release the file event. Are we going to follow up with some RAII wrapper? Or even better is there some way in which the code that is running the filters (IIRC ActiveTcpConncetion or something like that) could keep track of dealing with this if needed? Apologize if this has already been discussed at length, but a summary might be nice. Thank you.

Initially, I reset the file event before initializing a new one inside ioHandle::initializeFileEvent(), then @antoniovicente has great point, that is file_event reset late after the accept filter destructed, which means file_event's callback still have reference to the accept filter at that time. So I changed back to reset file_event inside filter's destruction. But I think I did miss that we can reset the file event just before we clear the accept filters.

envoy/source/server/active_tcp_listener.cc

Line 204 in 5a8f89a

accept_filters_.clear();

so can invoke ioHandle().resetFileEvents() before accept_filters.clear(). and 'resetFileEvents' works fine even with the filter doesn't create any file event.

@antoniovicente correct me if I forget some points you pointed out.

mattklein123 · 2021-06-28T15:18:09Z

But I think I did miss that we can reset the file event just before we clear the accept filters.

Yeah this is how would have thought we would do it, but there is probably something I am missing about that. @antoniovicente WDYT?

antoniovicente · 2021-06-30T23:53:08Z

Use of an RAII wrapper to track ownership of the file event in the network filters that need it seems like a potential improvement. The framework itself resetting the file event as it clears and re-creates the filter chain seems like a potential option also.

That said, re-creation of the filter chain on timeout seems like broken behavior to me, see #16952 (comment)

antoniovicente · 2021-07-01T00:05:41Z

Also, I wonder if there's an alternate way to provide the functionality provided by this filters, like an observer callback that Network::ConnectionImpl can provide to peek at the bytes read early in the connection in order to allow these inspector filters to look at the bytes that are being processed without requiring them to peek at the socket buffer every time the client sends additional bytes.

mattklein123 · 2021-07-01T16:16:03Z

The framework itself resetting the file event as it clears and re-creates the filter chain seems like a potential option also.

This is the approach I would take personally.

Also, I wonder if there's an alternate way to provide the functionality provided by this filters, like an observer callback that Network::ConnectionImpl can provide to peek at the bytes read early in the connection in order to allow these inspector filters to look at the bytes that are being processed without requiring them to peek at the socket buffer every time the client sends additional bytes.

+1 this seems like a better option, though a much larger change to think about in the future.

ggreenway · 2021-07-01T16:21:55Z

That's something I've thought about for awhile as well. For a long time, BoringSSL was running in owns-the-fd mode, but I think it doesn't anymore (uses a memory BIO), so this could now be implemented. I'm a strong +1 on no more PEEK calls in the listener filters.

antoniovicente · 2021-07-01T16:59:34Z

A possible complexity is that the proxy protocol filter may do a real read after the PEEK in order to consume bytes that represent the proxy protocol header when it is present. The API that would replace the PEEKs would need to allow stop-and-buffer to support the proxy protocol case in addition to pure PEEK functionality that I think other filters depend on.

ggreenway · 2021-07-01T17:42:42Z

Maybe the way to do it is have the listener filters read what they want into a Buffer::Instance, and then pass the buffer with any data they didn't consume to the next step?

antoniovicente · 2021-07-01T17:45:01Z

Yes, moving the actual read from the socket to base infrastructure and passing the buffer through the various filters seems like the most likely implementation path.

soulxu · 2021-07-01T22:13:41Z

The framework itself resetting the file event as it clears and re-creates the filter chain seems like a potential option also.

This is the approach I would take personally.

Let me submit another PR fix it.

Also, I wonder if there's an alternate way to provide the functionality provided by this filters, like an observer callback that Network::ConnectionImpl can provide to peek at the bytes read early in the connection in order to allow these inspector filters to look at the bytes that are being processed without requiring them to peek at the socket buffer every time the client sends additional bytes.

+1 this seems like a better option, though a much larger change to think about in the future.

Let me file an issue for this.

…yproxy#16952) The listener filter may add event on the new socket, but it won't cleanup the event. If continue_on_listener_filters_timeout is set, the new connection may add same event to the socket. So reset the file event before initialize new one. Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

…yproxy#16952) The listener filter may add event on the new socket, but it won't cleanup the event. If continue_on_listener_filters_timeout is set, the new connection may add same event to the socket. So reset the file event before initialize new one. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Reset the file event before initialize new one

fe02e36

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente self-assigned this Jun 12, 2021

fix test

ba48b32

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu commented Jun 13, 2021

View reviewed changes

antoniovicente reviewed Jun 14, 2021

View reviewed changes

antoniovicente assigned florincoras and lambdai Jun 14, 2021

antoniovicente added the waiting:any label Jun 14, 2021

repokitteh-read-only bot removed the waiting:any label Jun 14, 2021

antoniovicente added the waiting label Jun 14, 2021

soulxu added 5 commits June 16, 2021 06:12

Revert the change

07404c8

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Reset file event in destructor of listener filter

6887a0e

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Reset file event in listener destructor

d88a451

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

add comment

a1f6653

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

a0c2f88

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested review from asraa, ggreenway, lambdai and lizan as code owners June 18, 2021 14:43

repokitteh-read-only bot removed the waiting label Jun 18, 2021

antoniovicente reviewed Jun 18, 2021

View reviewed changes

antoniovicente added the waiting label Jun 18, 2021

soulxu added 3 commits June 20, 2021 05:46

address comment

1f2216d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

revert unrelated change

5b96762

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

155b8cd

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 20, 2021

soulxu added 2 commits June 20, 2021 07:19

revert

1efba1f

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix fuzz test

770097a

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested a review from snowp as a code owner June 21, 2021 09:16

revert unrelated format fix

12e4fec

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente reviewed Jun 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 21, 2021

antoniovicente mentioned this pull request Jun 21, 2021

Protecting envoy from stacking events on the same fd #8925

Open

address comments

ab10edb

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 22, 2021

antoniovicente approved these changes Jun 22, 2021

View reviewed changes

antoniovicente changed the title ~~Listener: reset the file event before initialize new one~~ Listener: reset the file event when destroying listener filters Jun 22, 2021

antoniovicente merged commit 0929a71 into envoyproxy:main Jun 22, 2021

This was referenced Jul 3, 2021

listener: reset the file event in framework instead of listener filter doing itself #17227

Merged

envoy should read the data from the buffer Instead of listener filter to peek the data from socket buffer directly #17229

Closed

Listener: reset the file event when destroying listener filters #16952

Listener: reset the file event when destroying listener filters #16952

Conversation

soulxu commented Jun 12, 2021

soulxu Jun 13, 2021 • edited Loading

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

soulxu commented Jun 14, 2021

repokitteh-read-only bot commented Jun 14, 2021

antoniovicente 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

antoniovicente 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

antoniovicente left a comment

Choose a reason for hiding this comment

soulxu commented Jun 23, 2021

mattklein123 commented Jun 28, 2021

soulxu commented Jun 28, 2021

mattklein123 commented Jun 28, 2021

antoniovicente commented Jun 30, 2021

antoniovicente commented Jul 1, 2021

mattklein123 commented Jul 1, 2021

ggreenway commented Jul 1, 2021

antoniovicente commented Jul 1, 2021

ggreenway commented Jul 1, 2021

antoniovicente commented Jul 1, 2021

soulxu commented Jul 1, 2021

soulxu Jun 13, 2021 •

edited

Loading