Fixing HCM fuzzer ContinueAndEndStream with end_stream set #11497
Conversation
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
|
@asraa please take a look, and let me know if you think something else should've been done |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
There was a problem hiding this comment.
Thanks Adi! Interesting, I think this is correct based on matching HCM behavior in the fuzzer since the ASSERT is enforcing this "should-be" behavior.
I'm not sure if this ASSERT is really enforcing a true invariant, though. Maybe add this to the docstring for this status in include/envoy/http/filter.h so filter devs don't violate?
|
Giving this another thought, and I'm not sure if we should keep the assert in cases where a dynamically loaded filter will return |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…20200604 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
| Is there a contract my HTTP filter must adhere to? | ||
| ================================================== | ||
|
|
||
| * A filter's ``decodeData`` implementation must not return ``FilterHeadersStatus::ContinueAndEndStream`` when called with ``end_stream`` set. |
There was a problem hiding this comment.
There are a bunch more in https://github.com/envoyproxy/envoy/blob/master/include/envoy/http/filter.h that might be worth re-iterating. E.g. the StopIteration contract, which headers you can safely expect to exist in decodeHeaders() (e.g. Path? Host?).
@envoyproxy/maintainers what other contracts exist here?
There was a problem hiding this comment.
Added notes about:
- headers encoding/decoding
- data encoding/decoding
- trailers encoding/decoding
- known headers: Host and Path (optional for CONNECT requests).
Anything else that comes to mind?
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…20200604 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…20200604 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
| ---------------------------------------------------------------------------------------------- | ||
|
|
||
| Yes. The map must contain the following headers: ``Host``, ``Path`` (this might be omitted for | ||
| CONNECT requests). |
There was a problem hiding this comment.
@alyssawilk does this cover all relevant aspects of the contract here?
There was a problem hiding this comment.
@mattklein123 @antoniovicente as well.
There was a problem hiding this comment.
Currently the Envoy constraints are that the first filter receiving decodeHeaders will have a Host and if non-CONNECT it will have a Path. That said there's nothing stopping a user-defined filter from removing either (though the codec may crash or throw when trying to serialize incomplete headers if a later filter doesn't add it back) as the HCM will not validate before passing to the next filter.
There was a problem hiding this comment.
Thanks for the clarification.
I've updated the text as follows:
The first filter of the decoding filter chain will have the following headers in the map:
HostPath(this might be omitted for CONNECT requests).
Although these headers may be omitted by one of the filters on the decoding filter chain,
they should be reinserted before the terminal filter is triggered.
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…y#11497) Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in envoyproxy#4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…y#11497) Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in envoyproxy#4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
Fixing the HCM fuzzer to avoid cases where its
decodeHeadersmethod returnsFilterHeadersStatus::ContinueAndEndStreamwhen called withend_stream=true.This assert was added in #4885, to avoid misuse of the
FilterHeadersStatus::ContinueAndEndStreamvalue.Risk Level: Low - tests only
Testing: Changed HCM fuzzer test code
Fixes oss fuzz issue 21971
Signed-off-by: Adi Suissa-Peleg adip@google.com