Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respond 400 + Reset to malformed request #748

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

franfastly
Copy link

@franfastly franfastly commented Feb 20, 2024

h2 returns RST_STREAM frames with the PROTOCOL_ERROR bit set as a response to many types of errors in client requests. Many of those cases, when handled by an HTTP/1 server such as the one used in hyper, would result in an HTTP 400 Bad Request response returned to the client rather than a TCP reset (the HTTP/1 equivalent of a RST_STREAM). As a consequence, a client will observe differences in behaviour between HTTP/1 and HTTP/2: a malformed request will result in a 400 response when HTTP/1 is used whereas the same request will result in a reset stream with h2.

This PR makes h2 reply a HEADERS+400+END_STREAM frame followed by a RST_STREAM+PROTOCOL_ERROR frame to all the malformed!() macro invocations in Peer::convert_poll_message() in src/server.rs.

The Reset variant in the h2::proto::error::Error Enum now contains an Option<http::StatusCode> value that is set by the malformed!() macro with a Some(400). That value is propagated all the way until h2::proto::streams::send::Send::send_reset() where, if not None, a h2::frame::headers::Headers frame with the status code and the END_STREAM flag set will now be sent to the client before the RST_STREAM frame.

Some of the parameters passed to send_reset() have been grouped into a new SendResetContext Struct in order to avoid a clippy::too_many_arguments lint.

Tests where malformed requests were sent have been updated to check that an additional HEADERS+400+END_STREAM frame is now received before the RST_STREAM + PROTOCOL_ERROR frame.

This change has been validated with other clients like curl, a custom ad-hoc client written with python3+httpx and varnishtest.

Fixes #747

@franfastly franfastly marked this pull request as ready for review February 20, 2024 14:16
@franfastly franfastly changed the title Respond 400 + Reset to malformed request (#747) Respond 400 + Reset to malformed request Feb 20, 2024
@franfastly
Copy link
Author

I'm not sure why clippy started complaining about this. I could amend this PR to appease clippy or push a fix in separate PR.

@seanmonstar
Copy link
Member

Could be a newer clippy being a jerk.

`h2` returns `RST_STREAM` frames with the `PROTOCOL_ERROR` bit set as a
response to many types of errors in client requests. Many of those cases,
when handled by an HTTP/1 server such as the one used in `hyper`, would
result in an HTTP 400 Bad Request response returned to the client rather
than a TCP reset (the HTTP/1 equivalent of a `RST_STREAM`). As a
consequence, a client will observe differences in behaviour between
HTTP/1 and HTTP/2: a malformed request will result in a 400 response when
HTTP/1 is used whereas the same request will result in a reset stream
with `h2`.

Make `h2` reply a `HEADERS`+`400`+`END_STREAM` frame followed by a
`RST_STREAM`+`PROTOCOL_ERROR` frame to all the `malformed!()` macro
invocations in `Peer::convert_poll_message()` in `src/server.rs`.

The `Reset` variant in the `h2::proto::error::Error` Enum now contains an
`Option<http::StatusCode>` value that is set by the `malformed!()` macro
with a `Some(400)`. That value is propagated all the way until
`h2::proto::streams::send::Send::send_reset()` where, if not `None`, a
`h2::frame::headers::Headers` frame with the status code and the
`END_STREAM` flag set will now be sent to the client before the
`RST_STREAM` frame.

Some of the parameters passed to `send_reset()` have been grouped into a
new `SendResetContext` Struct in order to avoid a
`clippy::too_many_arguments` lint.

Tests where malformed requests were sent have been updated to check that
an additional  `HEADERS`+`400`+`END_STREAM` frame is now received before
the `RST_STREAM + PROTOCOL_ERROR` frame.

This change has been validated with  other clients like `curl`, a custom
ad-hoc client written with `python3+httpx` and `varnishtest`.
@acfoltzer
Copy link
Member

@seanmonstar I'm taking over this one from Fran while he's on vacation for the next few weeks. I would offer to do the review myself, but I'm the coauthor of the internal predecessor to this patch; it probably would be good to have a third party perspective before landing this change.

@franfastly
Copy link
Author

Hi! Is there anything I can do to help with the review of this PR? Thank you in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respond with an HTTP 4xx than only a RST_STREAM to malformed requests
3 participants