envoyproxy / envoyproxy/envoy

Ext-Proc: Data truncation with two ext_proc filters configured and body modes are STREAMED

Open
#47,449 1 comment 0 reactions 3 assignees Claimed by @yanavlasov View on GitHub
triage
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 20h
Merged PRs (30d)
437

Description

*Title*: Data truncation with two ext_proc filters configured and body modes are STREAMED

*Description*:

If there are two ext_proc filters configured, and the body processing modes of both of the filters are STREAMED, there is a data truncation issue happens if:

1) Client request contains trailers.
2) Client body external processing is going on in the 1st ext_proc filter.
3) Client trailers arrive Envoy.
4) Header response is received by the Envoy 2nd ext_proc filter.

The root cause is that the 2nd ext_proc filter header response processing call continueIfNecessary(), which evetually call filter_manager commonContinue(). As trailers is already received, it will call doTrailers(): https://github.com/envoyproxy/envoy/blob/26a8314c4e787bb606c30a51976679c816a8ca30/source/common/http/filter_manager.cc#L142. This will terminate the decoding process early while the data is still being processed by the 1st filter. Then when the 1st ext_proc filter finished body processing, re-inject the data to filter chain will cause ASSERT() failure in debug build. In prod build, the data will be lost.

Such issue happens for FULL_DUPLEX_STREAMED mode as well.

The root cause is calling commonContinue() during Envoy ext_proc header response processing incorrectly intertwines the filter manager states with ext_proc filter states. This should be avoided during the header response processing.

The original fix is proposed by: https://github.com/envoyproxy/envoy/pull/45355. However, that's causing an issue for websocket: https://github.com/envoyproxy/envoy/pull/47060. So, https://github.com/envoyproxy/envoy/pull/45355 is reverted for STREAMED mode. However, the data truncation issue resurface after that revert.

The new fix is proposed: https://github.com/envoyproxy/envoy/pull/47353. This PR basically keeps the original behavior, i.e, send the headers down the filter chain once the header response arrives Envoy. It adds new injectDecodeHeadersToFilterChain()/injectEncodeHeadersToFilterChain APIs to achieve this. As this approach is sending the headers to the filter chain without call the filter manager: commonContinue(), thus the filter_manager state machine won't cause problem with it.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.