[http] handle upgrade responses with no body#8797
[http] handle upgrade responses with no body#8797asraa wants to merge 2 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
|
/review @alyssawilk |
| EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); | ||
| Buffer::OwnedImpl response( | ||
| "HTTP/1.1 200 OK\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); | ||
| codec_->dispatch(response); |
There was a problem hiding this comment.
Should we allow upgrade with HEAD or just reset the connection? I'm inclined to think that's pretty shady and we may want to send an error response
@mattklein123 @PiotrSikora
If we do allow HEAD and upgrade, I'm not sure if we'd want to perform the upgrade (which I think this change does) since HEAD is basically supposed to inform one of the response headers one would get if one did a GET. I think we'd really need to special case it. I'd definitely want an integration test in that case.
There was a problem hiding this comment.
IMO probably better to just 400 it, but will defer to what you and @PiotrSikora think.
There was a problem hiding this comment.
HEAD is legit, e.g. HTTP/1.1-to-HTTP/2 upgrade where the first request is HEAD, i.e.:
HEAD / HTTP/1.1
Host: example.com
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: ...
I'm aware that Envoy doesn't support HTTP/1.1-to-HTTP/2 upgrade just yet, but we're supposed to send regular HTTP/1.1 response to such request.
There was a problem hiding this comment.
In response to the upgrade: h2c case, Envoy won't crash like it does here, since we'll bypass upgrade mode since we don't support it. (And only in upgrade mode do we assert that there's no deferred end stream headers).
but we're supposed to send regular HTTP/1.1 response to such request.
I take this to mean that we should allow HEAD and upgrade, but skip the upgrade?
There was a problem hiding this comment.
Correct, upgrade is always optional (unless enforced by the client with OPTIONS method).
There was a problem hiding this comment.
Hmmm. We also have websocket upgrades, though. So the current codepath does a check for if we have upgrade headers. And if we haveh2c, we strip the relevant headers. Otherwise, we set handling_upgrade_ to true and upgrade for websocket.
There was a problem hiding this comment.
Bump on this? The upgrade behavior is:
- if
h2c, strip upgrade headers and ignore the upgrade (already implemented, unchanged) - if upgrade headers are present with anything else, handle as a
websocketupgrade - skip the upgrade if it's a response to a
HEADand remove upgrade headers
There's an integration test for this. Piotr, does that make sense, or should I just 400 it?
There was a problem hiding this comment.
Yeah, if we're stripping upgrades for h2c I would mildly prefer disallowing over allowing-and-altering. Again I'm Ok going with either, especially as we're not catching this on the request path so it'd be 5xx response rather than an early 4xx. Happy to defer to Piotr and Matt on it though
There was a problem hiding this comment.
Erroring out HEAD requests with upgrade headers seems like an OK way to avoid this crash condition.
What upgrade types does Envoy support? I assume only websocket.
There was a problem hiding this comment.
That's correct, websocket is the only one supported. h2c upgrades are stripped.
I'll re-open and make the 400 fix on Monday :)
Signed-off-by: Asra Ali <asraa@google.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 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! |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 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! |
|
This pull request has been automatically closed because it has not had activity in the last 14 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! |
This avoids using the
deferred_end_stream_headers_optimization when handling upgrade responses with no body.In this particular case, we send a HEAD request and respond with an upgrade. Because the request has HEAD type, we opt to add headers to
deferred_end_stream_headers_since the response can't have a body. This causes anASSERTfailure since we shouldn't havedeferred_end_stream_headers_when handling an upgrade.This PR changes that behavior to add headers when we can't have a body and we are not handling an upgrade.
Fixes OSS-Fuzz issue:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18483
Testing: Add corpus and unit test case.
Signed-off-by: Asra Ali asraa@google.com