HCM: guard Envoy from removal of critical response headers. by mathetake · Pull Request #15658 · envoyproxy/envoy

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

  const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end());
-  state_.non_100_response_headers_encoded_ = true;
  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

	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;
	maybeEndEncode(modified_end_stream);

Conversation

mathetake commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mathetake commented Mar 25, 2021

Uh oh!

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathetake Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

mathetake commented Mar 26, 2021

Uh oh!

mathetake commented Mar 26, 2021

Uh oh!

mathetake Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asraa commented Mar 31, 2021

Uh oh!

mathetake commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Apr 2, 2021

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

mathetake Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathetake Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

mathetake commented Apr 7, 2021

Uh oh!

mathetake commented Apr 8, 2021

Uh oh!

github-actions bot commented May 8, 2021

Uh oh!

github-actions bot commented May 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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