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.

Sign up for GitHub

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

Jump to bottom

Respond with an HTTP 4xx than only a RST_STREAM to malformed requests #747

Open
franfastly opened this issue Feb 16, 2024 · 2 comments · May be fixed by #748
Open

Respond with an HTTP 4xx than only a RST_STREAM to malformed requests #747

franfastly opened this issue Feb 16, 2024 · 2 comments · May be fixed by #748

Comments

@franfastly
Copy link

Issue

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.

Proposal

There was a conversation in the mailing list of the IETF-HTTP-WG about whether a server can send a response to malformed requests instead of treating them as a stream error. From Steffan Eising's last response to that thread:

  • Sending a HTTP response is preferable, since this gives much better error reporting on the client side.
  • Violations of HTTP/2 framing layer should error on the h2 layer. Basically, the server no longer trusts the client.
  • Sending a RESPONSE with subsequent RST is a good pattern

Although there was no consensus about specifying a single correct behaviour, I think it was clear that a 400 response was acceptable.

I could create a PR to make h2 reply a HEADERS+400+END_STREAM frame followed by a RST_STREAM+PROTOCOL_ERROR frame to all the malformed!() macro invocations in

h2/src/server.rs

Line 1506 in 0077d3d

fn convert_poll_message(

@seanmonstar
Copy link
Member

I remember that conversation. I think that's probably generally fine.

cc @acfoltzer @nox @Noah-Kennedy, thoughts?

@acfoltzer
Copy link
Member

@seanmonstar I've been working with @franfastly on this investigation and an internal patch we've been cooking up to address the issue. I think overall I would prefer that h2 just return HEADERS+END_STREAM in these cases where the framing of h2 itself is not at risk, but at the moment that behavior would break h2spec tests. Pending movement on that issue, Fran and I believe this is the best solution.

franfastly added a commit to franfastly/h2 that referenced this issue Feb 20, 2024
@franfastly
Respond 400 + Reset to malformed request (hyperium#747)
f529011 
`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 be sent to the client before the `RST_STREAM`
frame is sent.

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
a `HEADERS`+`400`+`END_STREAM` frame is now received before the
`RST_STREAM + PROTOCOL_ERROR` frame.

In addition to validating this change against the `h2` tests and the
`h2spec` test suite, I have also tested it with other clients like `curl`
a custom ad-hoc client written with `python3+httpx` and `varnishtest`.
franfastly added a commit to franfastly/h2 that referenced this issue Feb 20, 2024
@franfastly
Respond 400 + Reset to malformed request (hyperium#747)
7b41f65 
`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`.
@franfastly franfastly linked a pull request Feb 20, 2024 that will close this issue
Open
franfastly added a commit to franfastly/h2 that referenced this issue Feb 22, 2024
@franfastly
Respond 400 + Reset to malformed request (hyperium#747)
f98522a 
`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`.
franfastly added a commit to franfastly/h2 that referenced this issue Feb 22, 2024
@franfastly
Respond 400 + Reset to malformed request (hyperium#747)
7481e41 
`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`.
@ctz ctz mentioned this issue Sep 5, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Respond 400 + Reset to malformed request franfastly/h2
3 participants
@seanmonstar @acfoltzer @franfastly