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

proposal: x/net/http2: distinguish no body from empty body #53894

Open
shreyas44 opened this issue Jul 15, 2022 · 10 comments
Open

proposal: x/net/http2: distinguish no body from empty body #53894

shreyas44 opened this issue Jul 15, 2022 · 10 comments
Labels
Projects
Milestone

Comments

@shreyas44
Copy link

@shreyas44 shreyas44 commented Jul 15, 2022

Issue

A server cannot differentiate between the below two scenarios:

  1. No body was sent - The final header frame of the h2 request contains the END_STREAM and END_HEADERS flag and no data frame was sent after that
  2. An empty body was sent - The final header frame of the h2 request only contains the END_HEADERS flag. Subsequently, an empty data frame with an END_STREAM flag is sent.

When reading the body on http.Request, both immediately return io.EOF and 0 bytes are read.

Proposal

Create http2.NoBody and http2.EmptyBody to represent the two scenarios and set the req.Body to them. http2.NoBody can equal http.NoBody as well.

Use Case

Some servers differentiate between the two kinds of requests. We're running a MITM proxy that needs to read which kind of request a client is making and forward the same to the target server.

Alternatives Considered

Add a method to http.Request (named ReadDataFrame?) that returns whether or not a client sent a data frame.

Personally, I prefer adding http2.NoBody and http2.EmptyBody.

Additional Notes

  • When req.Body is set to nil or http.NoBody, no data frame is sent by http2.Transport. If it's set to another reader that returns 0 bytes (ex: ioutil.NopCloser(strings.NewReader(""))), then an empty data frame is sent. This gives a way to differentiate between the two scenarios when sending requests but not when accepting requests through http.Server or http2.Server
@gopherbot gopherbot added this to the Proposal milestone Jul 15, 2022
@neild
Copy link
Contributor

@neild neild commented Jul 15, 2022

HTTP, so far as I know, doesn't make any distinction between "no body" and "empty body". The choice to send the END_STREAM flag in a header frame or in an empty data frame is an implementation detail. Adding API surface to expose this detail implies that users should consider and/or take advantage of the distinction.

If we were to support this, I think there needs to be compelling evidence that this is of general value.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 20, 2022
@rsc rsc moved this from Incoming to Active in Proposals Jul 20, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jul 20, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Jul 27, 2022

Some servers differentiate between the two kinds of requests. We're running a MITM proxy that needs to read which kind of request a client is making and forward the same to the target server.

@shreyas44, can you please give more detail about this? Which servers? What kinds of differentiation?

@neild's comment suggests that we probably shouldn't do this without a compelling reason.

@rsc rsc changed the title proposal: x/net/http2: Differentiate between no body and empty body proposal: x/net/http2: distinguish no body from empty body Jul 27, 2022
@shreyas44
Copy link
Author

@shreyas44 shreyas44 commented Jul 27, 2022

@neild @rsc apologies for the inactivity.

HTTP, so far as I know, doesn't make any distinction between "no body" and "empty body". The choice to send the END_STREAM flag in a header frame or in an empty data frame is an implementation detail. Adding API surface to expose this detail implies that users should consider and/or take advantage of the distinction.

If we were to support this, I think there needs to be compelling evidence that this is of general value.

You're right that for the majority of the cases this doesn't matter and should remain an implementation detail. The only use case I can think of right now is ours - trying to read and replicate the behaviour of a client that does differentiate between the two (Go being one of them as mentioned previously, although I'm not sure whether that's intentional).

We could also use http.NoBody to indicate no body and an instance of the default reader that immediately returns EOF for an empty body. While this still exposes the distinction, it doesn't create any new variables/constants. This is also inline with the behaviour of http2.Transport where no body is sent if req.Body is http.NoBody and an empty body is sent if the body is any other reader that immediately returns EOF.

can you please give more detail about this? Which servers? What kinds of differentiation?

What caused issues for us was an AkamaiGhost server. When sending an empty body, we received a 403 forbidden response but when sending no body we received a successful response.

Cloudflare also seems to differentiate between the two. The crux of the linked post is that cloudflare doesn't add a Content-Length header when forwarding a h2 request with no body to a cloudflare worker but it does when there's an empty body.

@neild
Copy link
Contributor

@neild neild commented Jul 27, 2022

I think the terms "no body" and "empty body" are adding confusion here. There is no such distinction in HTTP. All HTTP requests and responses have a payload, and the payload may have a length of zero. There aren't different types of zero-length payloads.

What you're requesting is a way to distinguish between HTTP/2 streams with a zero-length payload that send the END_STREAM flag in the last headers frame vs. the first (empty) data frame.

This is a distinguishable property of HTTP/2 streams, just as the division of the payload between data frames is, but HTTP/2 doesn't draw a semantic distinction in either case and I don't think we should be exposing either in the net/http API.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 3, 2022

Is there any indication from Akamai or Cloudflare that this behavior is intentional, rather than just a bug in their implementations? If this is just a server-side bug in those two systems, it would be better for Go not to expose the difference and open the door to more such bugs.

@shreyas44
Copy link
Author

@shreyas44 shreyas44 commented Aug 4, 2022

As far as I know, neither Akamai nor Cloudflare have mentioned this issue anywhere. You might be right in that it's a bug and not intentional.

This is a distinguishable property of HTTP/2 streams, just as the division of the payload between data frames is, but HTTP/2 doesn't draw a semantic distinction in either case and I don't think we should be exposing either in the net/http API.

it would be better for Go not to expose the difference and open the door to more such bugs.

Fair. On the note of preventing bugs, I think x/net/http2 should set req.Body to http.NoBody in both cases instead of the current behavior where it's set to a reader that immediately returns EOF. This is a different issue however.

@shreyas44
Copy link
Author

@shreyas44 shreyas44 commented Aug 4, 2022

What you're requesting is a way to distinguish between HTTP/2 streams with a zero-length payload that send the END_STREAM flag in the last headers frame vs. the first (empty) data frame.

Do you have any suggestions on how we could do this without having to fork x/net/http2?

@neild
Copy link
Contributor

@neild neild commented Aug 5, 2022

Do you have any suggestions on how we could do this without having to fork x/net/http2?

I'm afraid not. The net/http and x/net/http2 packages don't try to be all things to all people--there are, for example, legitimate reasons under some circumstances to care about the order of received headers, but there is no way that I know of to get this information with net/http.

Perhaps there's a case to be made for this feature (if it's something that other HTTP/2 implementations commonly expose, that would be an argument that net/http should perhaps expose it as well), but a good dividing line is that net/http should expose the RFC-defined semantic properties of HTTP requests and responses, and not expose incidental implementation details.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 5, 2022

@neild and I are both mildly but not strongly opposed to this. We sympathize with having to interop with broken servers.

If we do this, it should be locked in with a test, have similar behavior between HTTP/1 and HTTP/2, and be documented.

I also want to see first a summary table of the current and proposed states of the server behavior for all combinations of:

  • http/1 vs http/2
  • content-length: 0 vs no content-length (or chunked for http/1)
  • both END bits set in http2 vs two frames

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

No branches or pull requests

5 participants