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

[mathetake]

This is the rework of the previous PR (#15658).

Commit Message: HCM: protect against removal of critical response headers by a filter chain.
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.

Notes

• For the request path, Envoy is already guarded by HeaderUtility::checkRequiredHeaders and its error is gracefully handled in the onPoolReady.

• The following is the current crash path with uncaught exception caused by removed :status header after filter chains:

envoy/source/common/http/conn_manager_impl.cc

Line 1481 in 5c51bf1

chargeStats(headers);

envoy/source/common/http/conn_manager_impl.cc

Line 777 in 5c51bf1

uint64_t response_code = Utility::getResponseStatus(headers);

envoy/source/common/http/utility.cc

Line 455 in 5c51bf1

throw CodecClientException(":status must be specified and a valid unsigned long");

• In the previous PR, I implemented this guard in the codec and make the codec return the status, but it's going to make the implementation much more complex than I expected (You can see in the size of the diff in HCM: guard Envoy from removal of critical response headers. #15658), plus introduce a lot of complexity in the code base (there were even unresolved comment about its direction). So I came back to the first solution there as opposed to the comment by @asraa HCM: guard Envoy from removal of critical response headers. #15658 (comment)).

Signed-off-by: Takeshi Yoneda [email protected]

[asraa]

i think my vote for making encodeHeaders (although larger change) return a status is that we won't need to check elsewhere in case some other extension point besides filters break status (i think that was the comment on my request side PR). but maybe that should change when that will be a problem

[mathetake]

we won't need to check elsewhere in case some other extension point besides filters break status

Thanks, that makes sense.. but yeah we could expand this check when actually we see other extension points break the status header.

[asraa]

The code otherwise looks good to me, deferring to a senior maintainer for any opinions on the approach
@envoyproxy/senior-maintainers

[alyssawilk]

thanks for fixing this.

LGTM modulo comment request.

Non-blocking comments, fixing this crash is good, but if we want to allow semi-trusted wasm, I think we should do the full suite of header checks that we do in the codec after wasm runs, ensure all headers only have valid characters, we don't allow smuggling by having transfer encoding and content length, relative urls don't start with ../ etc? Is there a plan and/or tracking issue for that?

[alyssawilk]

I hear your point about calling this from the codec being really messy.
optional!: Could we at least as part of this PR make the codecs not throw/crash if they get bad data a well? I think we'd prefer to serialize 0 if status code is not present to crashing though Matt may disagree =P

[mathetake]

In order to do so, we should eliminate all the usage of getResponseStatus and use getResponseStatusNoThrow instead? e.g.

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

  Line 336 in 5c51bf1

  const uint64_t status = Http::Utility::getResponseStatus(*headers);

• envoy/source/common/http/http1/codec_impl.cc

  Lines 373 to 375 in 5c51bf1

  	// The contract is that client codecs must ensure that :status is present.
  	ASSERT(headers.Status() != nullptr);
  	uint64_t numeric_status = Utility::getResponseStatus(headers);

I think fixing this is worth a separate PR and discussion, so can I open an issue for now and discuss there?

[alyssawilk]

SGTM, and as it's optional don't feel obliged to tackle it (though it would be nice!)
I think it's largely a part of removing exceptions from the data plane, which I think @chaoqin-li1123 is looking at

[mathetake]

but if we want to allow semi-trusted wasm, I think we should do the full suite of header checks that we do in the codec after wasm runs, ensure all headers only have valid characters, we don't allow smuggling by having transfer encoding and content length, relative urls don't start with ../ etc? Is there a plan and/or tracking issue for that?

yeah that's true and this is somewhat related to the comment by @PiotrSikora #15487 (comment), and yes, we should do Wasm specific check in addition to this Envoy-wide protection. I will open an issue for tracking this. Thanks!