[filters] Prevent a filter from sending local reply and continue #14416

asraa · 2020-12-15T14:05:26Z

Signed-off-by: Asra Ali asraa@google.com

Commit Message: Prevent filter iteration on headers after sending a local reply and returning FilterHeadersStatus::Continue.
Additional Description:
Before, the test hits this crash:

[ RUN      ] Protocols/ProtocolIntegrationTest.ContinueAfterLocalReply/IPv4_Http2Downstream_Http2Upstream
[2020-12-14 16:51:35.110][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:104] Caught Segmentation fault, suspect faulting address 0x0
[2020-12-14 16:51:35.110][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2020-12-14 16:51:35.110][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:92] Envoy version: 0/1.17.0-dev/test/RELEASE/BoringSSL
[2020-12-14 16:51:35.111][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #0: __restore_rt [0x7fc14cdc68a0]
[2020-12-14 16:51:35.115][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #1: Envoy::ConnectionPool::ConnPoolImplBase::attachStreamToClient() [0xc378d6]
[2020-12-14 16:51:35.120][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #2: Envoy::ConnectionPool::ConnPoolImplBase::onUpstreamReady() [0xc3824d]
[2020-12-14 16:51:35.124][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #3: Envoy::ConnectionPool::ConnPoolImplBase::onConnectionEvent() [0xc39118]
[2020-12-14 16:51:35.128][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #4: Envoy::Network::ConnectionImplBase::raiseConnectionEvent() [0x10eda1b]
[2020-12-14 16:51:35.132][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #5: Envoy::Network::ConnectionImpl::raiseEvent() [0x10e4889]
[2020-12-14 16:51:35.136][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #6: Envoy::Network::ConnectionImpl::onWriteReady() [0x10e744e]
[2020-12-14 16:51:35.140][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #7: Envoy::Network::ConnectionImpl::onFileEvent() [0x10e64e9]
[2020-12-14 16:51:35.144][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #8: std::__1::__function::__func<>::operator()() [0x10d0971]
[2020-12-14 16:51:35.148][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #9: Envoy::Event::FileEventImpl::assignEvents()::$_1::__invoke() [0x10d174c]
[2020-12-14 16:51:35.152][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #10: event_process_active_single_queue [0x17c33f8]
[2020-12-14 16:51:35.156][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #11: event_base_loop [0x17c1dce]
[2020-12-14 16:51:35.160][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #12: Envoy::Server::WorkerImpl::threadRoutine() [0xac9688]
[2020-12-14 16:51:35.164][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #13: Envoy::Thread::ThreadImplPosix::ThreadImplPosix()::{lambda()#1}::__invoke() [0x17bb2a3]
[2020-12-14 16:51:35.164][29][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #14: start_thread [0x7fc14cdbb6db]

The downside here is iteration_state_ isn't modified to stop iteration. I don't think this matters since it's changed to StopSingleIteration in maybeContinueDecoding right after.

Risk Level: Low/medium in case this changes expected filter iteration?
Testing: Added test that shows message logs and behavior passes.
Fixes part of #13737

Signed-off-by: Asra Ali <asraa@google.com>

alyssawilk · 2020-12-15T16:43:04Z

Thanks for the fix! mind checking out CI and pinging back when it's sorted?

Signed-off-by: Asra Ali <asraa@google.com>

source/common/http/filter_manager.cc

test/integration/protocol_integration_test.cc

asraa · 2020-12-16T16:16:56Z

Just to double check, oauth2 filter sends a local reply and then sends StopAllIterationAndBuffer (the test failure). Is it possible that people can send a local reply and then continue header iteration?

The most minimal constraint would be changing the condition to "don't return continue after local reply" rather than "must return stopiteration after local reply"

alyssawilk · 2020-12-16T16:23:04Z

I wonder if it's an attempt to drain the connection so it can be reused (for HTTP/1.1 etc.)?
cc @rgs1 @derekargueta @snowp

Signed-off-by: Asra Ali <asraa@google.com>

rgs1 · 2020-12-16T17:52:11Z

Just to double check, oauth2 filter sends a local reply and then sends StopAllIterationAndBuffer (the test failure). Is it possible that people can send a local reply and then continue header iteration?

The most minimal constraint would be changing the condition to "don't return continue after local reply" rather than "must return stopiteration after local reply"

Thanks for this fix! I somehow forgot to follow-up with a fix here when we first bumped into this internally... this should be StopIteration. Our internal copy of this filter -- and similar filters -- have already being fixed... And that made the crash go away.

rgs1 · 2020-12-16T17:57:17Z

I wonder if it's an attempt to drain the connection so it can be reused (for HTTP/1.1 etc.)?
cc @rgs1 @derekargueta @snowp

Which part? The Stop and Buffer as opposed to full Stop? Or continuing header iteration post local reply...? Afaik this was just a bug/omission on our side, not something to reset the connection state...

alyssawilk · 2020-12-16T17:58:27Z

Ah cool. If it's just broken I'd be inclined to restore your last push and fix oath upstream too, WDYT?

This reverts commit b47f26c. Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com>

rgs1

Oauth bits look good.. Thank you!

source/extensions/filters/http/oauth2/filter.cc

This reverts commit 2df4777. Signed-off-by: Asra Ali <asraa@google.com>

This reverts commit a7b95d2. Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-12-17T14:23:50Z

Sorry about the back and forth! All set with just preventing continue after a direct response. No changes to oauth2 (which maybe can be a TODO)

Related to envoyproxy#14416. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…cal-reply Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2020-12-21T18:41:53Z

Not sure if the wait is to update filter_manager to restrict back to stopiteration after local reply (rather than just "not continue"). Tests pass with @rgs1's Oauth PR merged.

If it wasn't let me know!

Signed-off-by: Asra Ali <asraa@google.com>

mattklein123

Thanks!

* master: (30 commits) Deflaked: Guarddog_impl_test (envoyproxy#14475) [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315) [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416) oauth2: improving coverage (envoyproxy#14479) owners: Change dio email address (envoyproxy#14498) macos build: Fix ninja install (envoyproxy#14495) http: use OptRef helper to reduce some boilerplate (envoyproxy#14361) doc: update test/integration/README.md (envoyproxy#14485) server: wait workers to start before draining parent. (envoyproxy#14319) api: relax inline_string length limitation in DataSource (envoyproxy#14461) oauth: properly stop filter chain when a response was sent (envoyproxy#14476) listener: deprecate use_proxy_proto (envoyproxy#14406) deps: update cel and remove a patch (envoyproxy#14473) preconnect: rename: (envoyproxy#14474) coverage: ratcheting limits (envoyproxy#14472) grpc mux: fix sending node again after stream is reset (envoyproxy#14080) [test] Replace printers_include with printers_lib. (envoyproxy#14442) tcp: nodelay in the new pool (envoyproxy#14453) test: replace mock_methodn macros with mock_method (envoyproxy#14450) tcp: extending tcp integration test (envoyproxy#14451) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

asraa added 2 commits December 15, 2020 08:53

handle local reply and continue

2ddc5f5

Signed-off-by: Asra Ali <asraa@google.com>

different fix

276ebb7

Signed-off-by: Asra Ali <asraa@google.com>

alyssawilk self-assigned this Dec 15, 2020

fix complete

609a6d9

Signed-off-by: Asra Ali <asraa@google.com>

alyssawilk assigned snowp Dec 15, 2020

asraa added 2 commits December 15, 2020 16:17

fix flakiness in test and log msg

8c6eb78

Signed-off-by: Asra Ali <asraa@google.com>

fix hcm debug tests

90a1d29

Signed-off-by: Asra Ali <asraa@google.com>

snowp added the waiting label Dec 16, 2020

repokitteh-read-only bot removed the waiting label Dec 16, 2020

alyssawilk previously approved these changes Dec 16, 2020

View reviewed changes

source/common/http/filter_manager.cc Outdated Show resolved Hide resolved

test/integration/protocol_integration_test.cc Show resolved Hide resolved

just stop continue after sendlocal

b47f26c

Signed-off-by: Asra Ali <asraa@google.com>

asraa dismissed alyssawilk’s stale review via b47f26c December 16, 2020 16:43

asraa added 2 commits December 16, 2020 13:03

Revert "just stop continue after sendlocal"

a7b95d2

This reverts commit b47f26c. Signed-off-by: Asra Ali <asraa@google.com>

fix oath2

2df4777

Signed-off-by: Asra Ali <asraa@google.com>

rgs1 previously approved these changes Dec 16, 2020

View reviewed changes

alyssawilk reviewed Dec 16, 2020

View reviewed changes

source/extensions/filters/http/oauth2/filter.cc Outdated Show resolved Hide resolved

asraa added 2 commits December 16, 2020 14:23

Revert "fix oath2"

b788903

This reverts commit 2df4777. Signed-off-by: Asra Ali <asraa@google.com>

Revert "Revert "just stop continue after sendlocal""

1e1e8a8

This reverts commit a7b95d2. Signed-off-by: Asra Ali <asraa@google.com>

asraa dismissed rgs1’s stale review via 1e1e8a8 December 16, 2020 19:24

alyssawilk previously approved these changes Dec 17, 2020

View reviewed changes

rgs1 pushed a commit to rgs1/envoy that referenced this pull request Dec 17, 2020

oauth: properly stop filter chain when a response was sent

c837d6e

Related to envoyproxy#14416. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 mentioned this pull request Dec 17, 2020

oauth: properly stop filter chain when a response was sent #14476

Merged

repokitteh-read-only bot added the waiting label Dec 18, 2020

mattklein123 self-assigned this Dec 18, 2020

Merge remote-tracking branch 'upstream/master' into continue-after-lo…

32e4872

…cal-reply Signed-off-by: Asra Ali <asraa@google.com>

update now that oauth fixed

3cb7ceb

Signed-off-by: Asra Ali <asraa@google.com>

asraa dismissed alyssawilk’s stale review via 3cb7ceb December 21, 2020 18:42

repokitteh-read-only bot removed the waiting label Dec 21, 2020

asraa added 2 commits December 21, 2020 15:40

add debug death test

62208ba

Signed-off-by: Asra Ali <asraa@google.com>

fix format

763cde8

Signed-off-by: Asra Ali <asraa@google.com>

mattklein123 approved these changes Dec 21, 2020

View reviewed changes

mattklein123 merged commit 4ba2827 into envoyproxy:master Dec 21, 2020

asraa mentioned this pull request Feb 5, 2021

Always process HTTP headers before processing HTTP body. proxy-wasm/proxy-wasm-cpp-host#95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[filters] Prevent a filter from sending local reply and continue #14416

[filters] Prevent a filter from sending local reply and continue #14416

asraa commented Dec 15, 2020 •

edited

Loading

alyssawilk commented Dec 15, 2020

asraa commented Dec 16, 2020

alyssawilk commented Dec 16, 2020

rgs1 commented Dec 16, 2020

rgs1 commented Dec 16, 2020

alyssawilk commented Dec 16, 2020

rgs1 left a comment

asraa commented Dec 17, 2020

asraa commented Dec 21, 2020 •

edited

Loading

mattklein123 left a comment

[filters] Prevent a filter from sending local reply and continue #14416

[filters] Prevent a filter from sending local reply and continue #14416

Conversation

asraa commented Dec 15, 2020 • edited Loading

alyssawilk commented Dec 15, 2020

asraa commented Dec 16, 2020

alyssawilk commented Dec 16, 2020

rgs1 commented Dec 16, 2020

rgs1 commented Dec 16, 2020

alyssawilk commented Dec 16, 2020

rgs1 left a comment

Choose a reason for hiding this comment

asraa commented Dec 17, 2020

asraa commented Dec 21, 2020 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

asraa commented Dec 15, 2020 •

edited

Loading

asraa commented Dec 21, 2020 •

edited

Loading