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

Request body streams should use chunked encoding #966

Closed
ddragana opened this issue Nov 15, 2019 · 75 comments
Closed

Request body streams should use chunked encoding #966

ddragana opened this issue Nov 15, 2019 · 75 comments

Comments

@ddragana
Copy link

Chunked encoding in the client-server direction is not widely used on the web. Stand-alone apps use it, but browser(s?), at least Firefox, do not support it.

the chunked encoding from server to client is broken on some server (they are probably old once, I do not have any data about this).

I am wondering what kind of bugs we will get if we start using chunked encoding in the client-server direction.

should we limit request body streams only to h2? We definitely should restrict it to https.

@annevk
Copy link
Member

annevk commented Nov 15, 2019

cc @yoichio @yutakahirano

@yoichio
Copy link
Contributor

yoichio commented Nov 15, 2019

To protect the legacy servers, the fetch spec states UA doesn’t allow no-cors requests to have a streaming body, and UA forces CORS preflight for any cross-origin request with a streaming body. [1]
Thus client and server are owned by single author.

[1] (https://fetch.spec.whatwg.org/#dom-request)

@annevk
Copy link
Member

annevk commented Nov 18, 2019

I think same-origin-policy-wise we indeed got it covered, but I think we should strongly consider not requiring clients to implement chunked encoding for uploads and require H2 as a minimum for upload streams.

If we do decide to implement chunked encoding for uploads requiring HTTPS would help avoid problematic middleware.

cc @whatwg/http

@MattMenke2
Copy link
Contributor

Sending chunked uploads over the web without some sort of server advertisement of support also makes me pretty nervous about middle-box/server compat issues, though requiring a server advertisement first addresses the server issue.

I don't think we want to make chunked uploads an optional client feature, just from a cross-browser compatibility standpoint. If one browser supports it and others do not, seems like that would bite a lot of web developers.

Requiring H2 instead presents its own set of problems with SSL-decrypting middleboxes, but I, at least, am less concerned about breaking just those.

@ricea
Copy link
Collaborator

ricea commented Nov 20, 2019

I think it would be a pity to introduce an H2 dependency as H2 won't work in all environments, which could push the need to carry a perpetual fallback code path onto applications.

However, the same middleboxes that are breaking H2 may also break H1 chunked uploads, so there may be no good choices.

I'd like to at least experiment with permitting it over H1. We can compare success rates for H1 and H2 and see if there is a significant difference.

@yutakahirano
Copy link
Member

Requiring https sounds reasonable. I'm not sure about requiring H2...

We could instead add another header to the CORS preflight response (note that we already require preflight for all cross-origin requests).

@yutakahirano
Copy link
Member

Anyway, I'd like to hear opinions from server side developers.

@ioquatix
Copy link

ioquatix commented Nov 20, 2019

I am the engineer behind Ruby's async-http HTTP/1.x and HTTP/2 client & server and falcon which is a zero-configuration HTTP/1 & HTTP/2 server for Ruby web applications.

When I first started working on async-http it was not clear to me that bi-directional streaming was possible via HTTP/1. But there are several ways it can work:

a/ For HTTP/1.0 (and 1.1) you can leverage shutdown(WR) and use a request body without a content-length (and obviously connection: close). This is in violation of the spec but only slightly and in practice it does work in certain situations. We don't expose this by default since it violates the spec, but at least I wanted to comment on it since I played around with it and it allows bi-directional streaming without chunked encoding.

b/ For HTTP/1.1, using chunked encoding for the request body works perfectly and is completely standards compliant.

c/ For HTTP/2, obviously this is a non-issue.

Falcon supports (b) and (c) by default, out of the box.

I wanted to try this out and provide a way for others to try it out too, so I quickly threw together a remote echo server.

Firstly, you can get the client and server here: https://github.com/socketry/utopia-falcon-heroku/

The application is hosted on Heroku right now: https://utopia-falcon-heroku.herokuapp.com/

Response Streaming

http://utopia-falcon-heroku.herokuapp.com/beer/index?count=100

It is actually going via an HTTP/1.1 middle box and working perfectly.

Request Streaming

Locally (working)

Using the code above:

$ bundle install
$ falcon serve

To start the client:

$ rake echo URL=https://localhost:9292/echo/index

Type some lines and they will be echoed back in real time.

Heroku (not working?)

Run this on the client:

$ rake echo URL=https://utopia-falcon-heroku.herokuapp.com/echo/index

You should be able to type lines and get an interactive response, but it seems like the request might be getting buffered. I can add more logging on the server to see what is going on, and will report back.

Conclusion

I wish that we would focus on the standards more than the existing implementations. The point of the standard is to say how it should work. And the point of the engineer is to implement it. The standard can say "This should work on HTTP/1.1" and it's within the existing design parameters. The fact it doesn't work is a bug. By encoding this into the standard, you are essentially encoding buggy designs into the standard. If you do this repeatedly, I can imagine that we end up with a huge mess based on crappy implementations, and standards that are getting repeatedly more messy.

@ioquatix
Copy link

I did some more testing.

It seems that while Heroku supports streaming uploads, and streaming downloads, it doesn't support bi-directional streaming correctly.

The server log indicates it's receiving the individual lines I send it, but the response appears to be buffered until the request body is completed. I'm going to create an issue on the appropriate Heroku repo.

That being said, this should work. And if the standard can mandate that it should be supported by H1, then it requires all middle boxes to be upgraded to support it.

Given that the vast majority of existing infrastructure still runs on H1 - I think this is going to be necessary. This isn't just about client -> server, but client -> heroku -> server etc.

@bradisbell
Copy link

Are there more details about the reasoning behind the proposed restrictions?

Streamed HTTP PUT requests are made all the time. Many of these likely violate spec by not using chunked encoding but also not specifying content length (in the case of indefinite lengths, like live streaming media). Some do successfully use chunked encoding, but I don't have a sense for how widespread this is. In any case, it seems strange to limit what the browser can do out of a sort of protection for a server. I don't understand the reason for a server to declare that it supports chunked encoding from the client.

Is there a particular potential side effect that is the source for concern?

@MattMenke2
Copy link
Contributor

MattMenke2 commented Nov 20, 2019

From a practical standpoint, if a significant enough number of users are on browsers that don't implement a "standard", it's not really a standard, and developers will generally either not use it, or if they do use it, ensure they have some sort of fallback logic, unless they can compel their users to use a particular browser.

Switching a half-duplex API with hundreds of bits dangling off of it with a full-duplex API, things can fall apart badly, so we're talking a large investment of time in working through all that. It also adds a lot of new opportunities for implementation divergence, as it touches so many HTTP-layer things.

There are things like auth (you get a challenge in the middle of a streaming upload), to socket pool limits, to what to do if we get a 4xx response while still uploading a stream, to keeping streaming uploads with completed response bodies alive, to what these look like to the WebRequest extension API, request events will no longer be in the order that consumers expect them to be in, etc. Alternatively, a different set of internal APIs could be introduced for just this case, but then they'd need to plug in very differently (below the layer that hooks up auth, cookies, etc), which would present its own set of issues around duplicating behavior in two very different codepaths.

Of course, restricting it to H2, you still have all those problems. So I'm not actually sure restricting things to H2 makes things much simpler. The code unique to H1 isn't exactly all that complicated, so even having a second copy of it just for this use case shouldn't be too bad - it's just dealing with everything else between the lower HTTP layer and the web page that would be a major investment.

Anyhow, I don't want to give the impression I'm speaking for anyone who has the power to determine whether this gets done in Chrome or not (I'm not even on the network stack team any more), but it would be a pretty major investment in terms of engineering time. I think it would be good to see if there's buy-in from someone who says, and has the power to say, "yes, I'd be willing to devote 3 to 18 months of engineering time to be sure this gets done", if folks on the Chrome team think this is an API worth supporting.

Edit: Removed confusing, redundant bonus clauses.

@annevk
Copy link
Member

annevk commented Nov 20, 2019

To be clear, we already more or less agreed that full-duplex is out-of-scope for this initial round of upload streams as no browser has the architecture for that and nobody is willing to invest in it at this point. It's an open question to what extent Fetch should call that out.

Not using chunked encoding for uploads and also not specifying a Content-Length is not something anyone has considered to date for H/1 I think. I'm not sure we can get agreement on that however as it's non-compliant and a bit of a hack.

Chunked encoding is being considered, but is also new code for all clients involved. For a protocol on its way out it's not clear it's worth investing in. (As a reminder, security-wise we are fine as there's a CORS preflight for it in all cases.) Also, if most deployments end up using H/2 we might end up with these subtle bugs in certain scenarios that never get fixed.

@ioquatix
Copy link

ioquatix commented Nov 20, 2019

To be clear, we already more or less agreed that full-duplex is out-of-scope for this initial round of upload streams as no browser has the architecture for that and nobody is willing to invest in it at this point. It's an open question to what extent Fetch should call that out.

I can respect that.

However, you should be mindful about building the interfaces to support the fetch functionality. If you don't consider the needs of bi-directional streaming, it might be tricky to retrofit it in the future. There are nice symmetries to be had. The way I'd define the spec, is to assume bi-directional streaming is viable, build the interface around that, and then limit it to uni-directional streaming. Nothing is lost, but when browsers are ready to go with bi-directional streaming, the interfaces are ready to go and are nice to work with.

Bearing in mind that even if browsers don't support it, there is nothing to stop Node.js implementing the same fetch style APIs, and that's much more likely to support bi-directional streaming and have valid use cases for it.

@ioquatix
Copy link

ioquatix commented Nov 20, 2019

For a protocol on its way out it's not clear it's worth investing in.

I tried to address this specifically above - maybe it's not needed but a lot of application servers use HTTP/1 proxies. So, maybe browsers don't need it (i.e. they make HTTP/2 connections to load balancer) but the load balancer -> application server which is still often HTTP/1 definitely needs to support it.

@MattMenke2
Copy link
Contributor

Apologies, I completely lost context on what the issue was here in my last comment.

Anyhow, chunked encoding wouldn't be new code for Chromium (unless we have to sniff for HTTP/1.1 server support before using it, at least - which I'm not sure is necessary, given the preflight?) - it would just be exposing an obscure but already existing code path to the web.

@wenbozhu
Copy link

For HTTP/1.1, not allowing chunked-encoding and not specifying C-L will force the U-A to half-close the connection which may be interpreted by servers as a client-initiated cancellation, and we even have a http-wg ticket to standardize this behavior.

I highly doubt any proxies will break with chunk-encoded request bodies. Server implementations might but this is a design-time decision, i.e. knowing which http-server or API you app is talking to.

===

Agreed with @ricea ... It will be very hard to use the streamed fetch upload only with H2 if streamed upload will not always work (as decided by intermediaries) and the client application has to decide in the runtime which server-side end-point (URL) to target e.g. one with upload streaming and one without.

===

It's ok to make https a requirement although it might complicate local testing. With HTTP, chrome will always speak http/1.1 (?).

@mnot
Copy link
Member

mnot commented Nov 25, 2019

I don't think requiring H2 or TLS will do much good here; many (most?) CDNs talk H1 on the back end, so if there are interop problems, they still can happen (although I think by far the most common behaviour will be generating a 411 Length Required; many/most servers do this if Content-Length isn't present, as they want to know how many bytes they're about to handle).

Don't use half-close to delimit a request; you'll have a bad time.

@yutakahirano
Copy link
Member

I am (again) proposing the following:

  • Restrict the feature for secure contexts.
    We have no checks for same-origin requests, which means we want some reasons to trust the same-originness. Without TLS an attacker can use this feature to attack servers not ready for it.
  • Have a new HTTP header attached to CORS preflight.
    With the header we'll be sure that both the client and the origin server are ready for this feature. We don't need to rely on the use of H2 to exchange the signal.

These don't cover the proxy problem @mnot mentioned, but with TLS the server side developer should have contacts to proxies - because at least they share the certificate. Hence asking server side developers to ensure that the streaming upload feature can be used for their service sounds reasonable to me.

What do you think?

@yutakahirano
Copy link
Member

I talked with @annevk at IRC.

  • We agreed that the secure context restriction is a must-have.
  • I misunderstood the motivation for the H2 restriction. I thought the main motivation was for security and that was why I proposed the new header on CORS preflight.
  • @annevk pointed out that for some user agents don't want to implement chunked encoding from implementation cost point of view.
  • @annevk and I agreed that it's not clear whether we need the new headers from security point of view.

@ddragana, can you tell us Firefox's implementation status of chunked encoding? If it's not there is it very hard to implement it?

I also would like to know, If some user agents don't want to support H1.1 with streaming upload do we actually need H2 restriction? Alternatively we can expose the support status and/or error status to web developers so that they can fall back.

@annevk
Copy link
Member

annevk commented Dec 5, 2019

I chatted with @ddragana in parallel and adding chunked encoding is not a lot of work for Firefox.

I'd be opposed to leaving the decision whether to support chunked encoding up to implementers.

Now that you make the H/1.0 vs H/1.1 distinction, what if the server replied to the CORS preflight using H/1.0? Would we still try talking to it using H/1.1 or refuse to do upload streams for that particular URL?

@yutakahirano
Copy link
Member

yutakahirano commented Dec 11, 2019

I think blocking H/1.0 preflight response will bring us some safety and we won't lose much coverage. @wenbozhu @ioquatix Do you agree with the idea?

@sleevi
Copy link

sleevi commented Dec 11, 2019

@yutakahirano I appreciate the desire to mitigate intermediaries, but I don't think preflights really addresses that holistically. While unencrypted HTTP may have intermediaries present anywhere along the network path, even encrypted (TLS) connections can still have intermediaries. They generally fall into one of two places - intermediaries near the server (which @mnot mentioned) and intermediaries near the client (e.g. local security policy, antivirus, etc).

A preflight doesn't really address intermediaries near the client, and it seems unlikely in practice that it'd successfully consistently address intermediaries near the server, since in both cases, they would generally transmit the header unmodified.

It might be tempting to say "intermediaries near the server need to have the server's private key, so that's the server's problem", and that "intermediaries near the client won't be able to use publicly trusted certificates, so we can require them" - but now that exposes a side-channel to servers that we presently and intentionally don't expose to the Web.

I'm sympathetic that "the ecosystem may be (... is probably) ossified" is not a reason not to do it, but I think this gets back to @MattMenke2 remark in #966 (comment)

This same conceptual challenge is the same underlying problem the ecosystem faced with TLS 1.3. This took years to successfully do, and generally required a variety of 'hacks' and workarounds in order to eventually get something successfully through. This worked reasonably well for something evolving and that had flexibility to have those workarounds, but I don't think we'd have the same leverage here.

That's why this still represents a major investment to "do it right", even if we could potentially "do it quick", and have it cause a host of long-tail issues. My only worry is that we then punt those long-tail issues to web servers, and we've tried to avoid that (see also the WebSockets handshake).

@annevk
Copy link
Member

annevk commented Dec 11, 2019

What is the side channel? That the user is behind an intermediary that does not support upload streams? Isn't that already revealed if an intermediary does any kind of meddling with the traffic (e.g., remove certain headers)?

@sleevi
Copy link

sleevi commented Dec 11, 2019 via email

@jakearchibald
Copy link
Collaborator

jakearchibald commented Jun 24, 2020

I'm not sure about 'most', but Node & ExpressJS support it https://github.com/jakearchibald/chunked-encoding-request-test/.

@ioquatix
Copy link

Falcon is a Ruby web server which supports HTTPS and HTTP/2 for development, so it certainly is possible.

@jakearchibald
Copy link
Collaborator

Node also supports HTTPS and HTTP/2, but I don't see it used often in dev. To clarify, I'm not saying that running HTTPS and HTTP/2 locally is impossible. I'm saying it isn't often used.

@jakearchibald

This comment has been minimized.

@annevk

This comment has been minimized.

@jakearchibald

This comment has been minimized.

@annevk annevk closed this as completed in 7149b15 Jun 16, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 4, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 5, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 5, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 5, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 8, 2022
…=testonly

Automatic update from web-platform-tests
Disallow streaming upload on HTTP/1.1

We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}

--

wpt-commits: 86181156e33f7951394257ce678e998dd71a1b94
wpt-pr: 34687
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jul 11, 2022
…=testonly

Automatic update from web-platform-tests
Disallow streaming upload on HTTP/1.1

We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}

--

wpt-commits: 86181156e33f7951394257ce678e998dd71a1b94
wpt-pr: 34687
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL enables origin trial flag FetchUploadStreaming.
You can try the feature with
"Chrome --enable-features=FetchUploadStreaming" or an OT token.
Given whatwg/fetch#966 (comment),
this CL also introduces the temporal AllowHTTP1ForStreamingUpload property
to measure upload streaming capability over each protocol.
API explaner: https://bit.ly/2SVvKbR

Bug: 688906
Change-Id: I46ccf37b2268371bb98af50c16b032f2d5fd470a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2174099
Commit-Queue: Yoichi Osato <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Yutaka Hirano <[email protected]>
Reviewed-by: Matt Menke <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#781762}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 4c75c0c9f730589ad8d6c33af919d6b105be1462
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
We decided not to allow the streaming upload feature on HTTP/1.1. Remove
the feature, and move tests accordingly.

See also: whatwg/fetch#966

Bug: 688906
Change-Id: I4e616469aad2378495ad81ba9034ca034f8ab1b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734308
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Sam McNally <[email protected]>
Reviewed-by: Yoichi Osato <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1020706}
NOKEYCHECK=True
GitOrigin-RevId: ec63eb95dc85a6ba0de9043090f63307e84ddf46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

18 participants