HCM: guard Envoy from removal of critical response headers. #15658

mathetake · 2021-03-25T06:18:53Z

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Commit Message: HCM: guard Envoy from removal of critical response headers.
Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves #13756 and ref: #15487.
For the request path, Envoy is already guarded by HeaderUtility::checkRequiredHeaders and its error is gracefully handled in the onPoolReady.

Risk Level: high (change in the core feature's critical code path)
Testing: unittest && integration test

The following is the current crash path with uncaught exception:

envoy/source/common/http/conn_manager_impl.cc

Line 1463 in 25d5066

chargeStats(headers);
envoy/source/common/http/conn_manager_impl.cc

Line 771 in 25d5066

uint64_t response_code = Utility::getResponseStatus(headers);

envoy/source/common/http/utility.cc

Lines 442 to 445 in 25d5066

    
           const HeaderEntry* header = headers.Status(); 
        
           uint64_t response_code; 
        
           if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { 
        
             throw CodecClientException(":status must be specified and a valid unsigned long");

and we've never reached the assertion for this here:

envoy/source/common/http/http2/codec_impl.cc

Line 237 in 25d5066

ASSERT(headers.Status() != nullptr);

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

source/common/http/filter_manager.cc

mathetake · 2021-03-25T07:15:04Z

cc @asraa

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

source/common/http/filter_manager.cc

asraa

My 2 cents! Thank you so much for doing this!

source/common/http/header_utility.cc

source/common/http/filter_manager.cc

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-03-26T05:00:13Z

source/common/http/conn_manager_impl.cc

+    ASSERT(response_encoder_->encode100ContinueHeaders(continue_headers).ok());
+    chargeStats(continue_headers);


chargeStats assumes that the required headers are present, so we need to change the order here and elsewhere.

mathetake · 2021-03-26T05:02:49Z

still crashes when it is head request 😕

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-03-26T06:31:37Z

OK, now all the crashes disappeared. Will work on the unit / integration tests.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-03-26T14:40:57Z

source/common/http/filter_manager.cc

  filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);
+  state_.non_100_response_headers_encoded_ = true;


this change is necessary since there's a check of non_100_response_headers_encoded_ != false in the local reply and it makes sense to keep that assertion.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

asraa · 2021-03-31T14:50:51Z

I thikn you could probably add an ASSERT(checkRequiredResponseHeaders()) after Alyssa's code to reset.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-04-02T00:31:37Z

Our test added in this PR has passed for QUIC now, but

OverflowingResponseCode
MissingStatus

protocol tests have started failing for not only QUIC but also htt1,2. Will take a look.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-04-02T08:34:55Z

OK I hope I fixed the problem. Our approach here would change the behavior slightly different from the one in #15648. That is the upstream client will never receive the RESET even if there's invalid status headers coming, and instead just send the 502 with 'protocol error'-ish message.

mathetake · 2021-04-02T08:36:10Z

I hope the latest commit efdc243 explains well :)

mathetake · 2021-04-02T10:29:40Z

@asraa @alyssawilk PTAL! :)

asraa

The code changes by themselves look OK to me. I'm just not 100% sure if they should all be handled with reset like QUIC was.
@alyssawilk

alyssawilk

Thanks for tackling this! It'll be great to have Envoy even more robust.

include/envoy/http/codec.h

source/common/http/conn_manager_impl.cc

alyssawilk · 2021-04-06T20:56:45Z

source/common/http/filter_manager.cc

+  // filter_manager_callbacks_.streamEnded() returns True here when the codec failed to encode
+  // headers due to the lack of required response headers, and filter_manager_callbacks already sent
+  // the local reply and ended the stream by itself. So in that case this function must be no-op.
+  if (end_stream && !filter_manager_callbacks_.streamEnded()) {


where are we now calling maybeEndEncode after the stream already ended?
I'm not sure but my concern is we may have a problem we should be solving before we reach this point.

that is when the local reply is sent (due to the missing requires response headers) in the ConnectionManagerImpl::ActiveStream::encodeHeaders as filter_manager_callbacks.encodeHeaders, and the filter manager then reaches maybeEndEncode.

envoy/source/common/http/filter_manager.cc

Lines 1070 to 1072 in 174e51a

filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);

state_.non_100_response_headers_encoded_ = true;

maybeEndEncode(modified_end_stream);

But yeah I think this made the code a little bit more complex so maybe we should just return the status from filter_manager_callbacks and send the LocalReply just before maybeEndEncode in FilterManager..

Yeah, I think I'd rather have the HCM encodeHeaders return a status, so the filter manager knows something has gone awry.
I think it's useful to retain checks in maybeEndEncode happening once to catch other weird corner case bugs.
cc @snowp for a second opinion.

test/common/http/conn_manager_impl_test.cc

alyssawilk · 2021-04-06T21:01:04Z

test/integration/fake_upstream.cc

-    encoder_.encodeHeaders(*headers_copy, end_stream);
+    const auto status = encoder_.encodeHeaders(*headers_copy, end_stream);
+    // simulate the connection manager's behavior to handle encoding failures.
+    if (!status.ok()) {


I think this is a problem.
Is this added to fix the integration tests we had that Envoy successfully handed upstream responses with no status? I think it's now testing the upstream works, not that Envoy works, and we need some way to force the encoder to encode this for tests, even if it looks invalid.

I think this is why coverage is now failing, because we're no longer testing handling bad responses from upstream.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-04-07T03:06:51Z

I'm waiting for the reply to #15658 (comment) since it might affect the other parts

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake · 2021-04-08T07:13:59Z

will work on the fix of test after the comment https://github.com/envoyproxy/envoy/pull/15658/files#r608174940 has reached consensus since it also affects the other parts

github-actions · 2021-05-08T20:01:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-05-16T00:03:22Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

… chain. (#16745) This is the rework of the previous PR (#15658). Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves #13756 and ref: #15487. Testing: new integration tests Risk: low Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

… chain. (envoyproxy#16745) This is the rework of the previous PR (envoyproxy#15658). Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves envoyproxy#13756 and ref: envoyproxy#15487. Testing: new integration tests Risk: low Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

HCM: guard Envoy from removal of cirical response headers.

3b0cebf

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake changed the title ~~HCM: guard Envoy from removal of cirical response headers.~~ HCM: guard Envoy from removal of critical response headers. Mar 25, 2021

Add test for checkRequired{Request,Response}Headers

16a9c78

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake commented Mar 25, 2021

View reviewed changes

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

mathetake marked this pull request as ready for review March 25, 2021 07:14

mathetake requested review from alyssawilk, ggreenway and mattklein123 as code owners March 25, 2021 07:14

Fix typo

0571faa

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake commented Mar 25, 2021

View reviewed changes

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

asraa reviewed Mar 25, 2021

View reviewed changes

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

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

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

mattklein123 assigned asraa Mar 25, 2021

mathetake added 3 commits March 26, 2021 10:31

review

0fd4855

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Make codec return the status

3e0b74a

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Fix assertion

928717c

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake commented Mar 26, 2021

View reviewed changes

Fix head request crash

52f9319

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake added 3 commits March 26, 2021 23:06

Fix typo

9f4628d

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Merge remote-tracking branch 'origin/main' into status-header

f7d37b8

Add maybeEndDecode test

7fca42d

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake commented Mar 26, 2021

View reviewed changes

mathetake added 5 commits March 27, 2021 00:10

Simplify

0dafb70

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Add codec test

5c564ad

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Fix comment

59cc65d

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Add http2 integration test

719c1be

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Add http integration test

ce54947

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake requested a review from asraa March 28, 2021 07:17

mathetake added 2 commits April 1, 2021 07:24

Merge remote-tracking branch 'origin/main' into status-header

e51dbc9

Revert reverted change to QUIC

8434064

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Fix tests

efdc243

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

asraa reviewed Apr 6, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into status-header

7093416

alyssawilk reviewed Apr 6, 2021

View reviewed changes

mathetake added 5 commits April 7, 2021 11:06

Revert bad merge

b91f23b

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Tidy up messages

1034e00

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Add @return comments

a0b8b3a

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

RELEASE_ASSERT -> ASSERT

e8fd6d9

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Fix ASSERT

174e51a

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

mathetake added 2 commits April 8, 2021 05:51

Merge remote-tracking branch 'origin/main' into status-header

0c8c649

Split FilterRemovedRequiredHeaders to reseponse/request

c7eda3c

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 8, 2021

github-actions bot closed this May 16, 2021

mathetake mentioned this pull request Jun 1, 2021

HCM: protect against removal of critical response headers by a filter chain. #16745

Merged

mathetake deleted the status-header branch June 29, 2021 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCM: guard Envoy from removal of critical response headers. #15658

HCM: guard Envoy from removal of critical response headers. #15658

mathetake commented Mar 25, 2021 •

edited

Loading

mathetake commented Mar 25, 2021

asraa left a comment

mathetake Mar 26, 2021

mathetake commented Mar 26, 2021

mathetake commented Mar 26, 2021

mathetake Mar 26, 2021 •

edited

Loading

asraa commented Mar 31, 2021

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake commented Apr 2, 2021

asraa left a comment

alyssawilk left a comment

alyssawilk Apr 6, 2021

mathetake Apr 8, 2021 •

edited

Loading

mathetake Apr 8, 2021

alyssawilk Apr 8, 2021 •

edited

Loading

alyssawilk Apr 6, 2021

mathetake commented Apr 7, 2021

mathetake commented Apr 8, 2021

github-actions bot commented May 8, 2021

github-actions bot commented May 16, 2021

	const HeaderEntry* header = headers.Status();
	uint64_t response_code;
	if (!header \|\| !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
	throw CodecClientException(":status must be specified and a valid unsigned long");

		ASSERT(response_encoder_->encode100ContinueHeaders(continue_headers).ok());
		chargeStats(continue_headers);

		filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);
		state_.non_100_response_headers_encoded_ = true;

	filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);
	state_.non_100_response_headers_encoded_ = true;
	maybeEndEncode(modified_end_stream);

HCM: guard Envoy from removal of critical response headers. #15658

HCM: guard Envoy from removal of critical response headers. #15658

Conversation

mathetake commented Mar 25, 2021 • edited Loading

mathetake commented Mar 25, 2021

asraa left a comment

Choose a reason for hiding this comment

mathetake Mar 26, 2021

Choose a reason for hiding this comment

mathetake commented Mar 26, 2021

mathetake commented Mar 26, 2021

mathetake Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

asraa commented Mar 31, 2021

mathetake commented Apr 2, 2021 • edited Loading

mathetake commented Apr 2, 2021 • edited Loading

mathetake commented Apr 2, 2021 • edited Loading

mathetake commented Apr 2, 2021

asraa left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

mathetake Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

mathetake Apr 8, 2021

Choose a reason for hiding this comment

alyssawilk Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

mathetake commented Apr 7, 2021

mathetake commented Apr 8, 2021

github-actions bot commented May 8, 2021

github-actions bot commented May 16, 2021

mathetake commented Mar 25, 2021 •

edited

Loading

mathetake Mar 26, 2021 •

edited

Loading

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake commented Apr 2, 2021 •

edited

Loading

mathetake Apr 8, 2021 •

edited

Loading

alyssawilk Apr 8, 2021 •

edited

Loading