http1 204 response with body fails codec assertion

[adisuissa]

Removing the codec assert checking that a 204 response must be called with end_stream set to true.

The spec states that a 204 response cannot contain a body, and Envoy removes the body from the response.
Also added an integration test, and fuzz test examples that used to fail the assertion.

Risk Level: Low
Testing: new integration test, and 2 fuzz test examples added
Docs Changes: None
Release Notes: None
Fixes oss-fuzz bug: 23853

Signed-off-by: Adi Suissa-Peleg adip@google.com

[adisuissa]

/cc @asraa @yanavlasov

This should be ok as long as upstreams are trusted.

[lizan]

upstream are not trusted

[alyssawilk]

Well upstreams are mostly trusted, but we want to move in a direction they aren't. I think it'd be good to fuzz invalid 204-with-body if we can. Thoughts @asraa ?

[asraa]

Agreed, we've been having offline discussion about this.

Adi tested what happens to 204-with-bodies, and it looks like in integration tests (opt mode, so no assert failure) their bodies are rightfully dropped. The direction I suggest leaning towards is avoiding the ASSERT failure, which seems like the wrong thing. It's not an invariant -- and 204s with bodies should maybe just end their encode after encodeHeaders anyway.

[yanavlasov]

Yes, we should not ASSERT on wire data. And yes, we should fuzz it. Is it possible to modify fuzz test to specify presence of the body in corpus? In this way the extra if will not be needed.

[adisuissa]

Thanks for all the comments.
Removed the assert and added an integration test that verifies that the body is removed from a 204 response.

Just to recap:
The assertion that was added in #11458 has an assumption that when a 204 response is sent, end_stream will be true.
This should be ok, as long as the upstreams are behaving according to the spec.
As its not really an invariant the assertion should be removed.
If the 204 response has a body, the assertion will fail. Without the assertion Envoy removes the body and returns the 204 response.

[antoniovicente]

Modifying the codec fuzzer to set end_stream to true in case of a 204 response.

A 204 response must not have a body according to the spec (https://tools.ietf.org/html/rfc7230#section-3.3.3).
This should be valid as long as upstreams are trusted.

Risk Level: Low (tests only)
Testing: Updated fuzzer
Fixes oss-fuzz bug: 23853

Signed-off-by: Adi Suissa-Peleg adip@google.com

The PR description seems to be out of date. It also does not follow the PR creation template which calls for:

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

[adisuissa]

Thanks @antoniovicente!
I've updated the PR description.

[asraa]

Thanks!

[asraa]

LGTM from my end

[adisuissa]

@lizan @alyssawilk @yanavlasov - other comments?