changes: modify ngx.read_body api limitation in HTTP/2 and HTTP/3 #2237
+75
−15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Appla
reviewed
Oct 17, 2023
| if (r->http_version == NGX_HTTP_VERSION_30 | ||
| && r->headers_in.content_length_n < 0) | ||
| { | ||
| ngx_log_error(NGX_LOG_WARN, r->connection->log, 0, |
There was a problem hiding this comment.
Yes, HTTP/3 has same problem.
oowl
approved these changes
Oct 17, 2023
| @@ -2724,7 +2724,7 @@ lua_need_request_body | |||
|
|
|||
| **phase:** *depends on usage* | |||
|
|
|||
| Due to the stream processing feature of HTTP2, it does not support HTTP2 connection. | |||
| Due to the stream processing feature of HTTP/2 or HTTP/3, this configuration could potentially block the entire request. Therefore, this configuration is effective only when HTTP/2 or HTTP/3 requests send content-length header. For requests with versions lower than HTTP/2, this configuration can still be used without any problems. | |||
There was a problem hiding this comment.
We have discussed this problem and reached an agreement that enables read_body API in the HTTP2 request contain the content-length header
And why chunk-transfer not enabled? from RFC
In https://greenbytes.de/tech/webdav/rfc7540.html#rfc.section.8.1.p.4:
HTTP/2 uses DATA frames to carry message payloads. The chunked transfer encoding defined in Section 4.1 of [RFC7230] MUST NOT be used in HTTP/2.
There was a problem hiding this comment.
@zhuizhuhaomeng Hi, Can you help us take a look at this PR?
swananan
changed the title
zhuizhuhaomeng
approved these changes
Oct 26, 2023
swananan
force-pushed
the
modify_read_body_api_limitation
branch
from
432bed9
to
4af043a
Compare
ogercyprien
added a commit
to opendatasoft/lua-resty-waf
that referenced
this pull request
Jan 31, 2024
…ength header This check ensures that the underlying lua-nginx-module will not crash when trying to fetch the body of a POST request made with HTTP2 or HTTP3 with an unspecified Content-Length header. See openresty/lua-nginx-module#2237
ogercyprien
added a commit
to opendatasoft/lua-resty-waf
that referenced
this pull request
Feb 1, 2024
This check ensures that the underlying lua-nginx-module will not crash when trying to fetch the body of a POST request made with HTTP2 or HTTP3 with an unspecified Content-Length header. See openresty/lua-nginx-module#2237
5 tasks
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.
#2174 mentioned an issue where there is a chance of blocking the ngx.read_body API when using HTTP/2. As a result, #2174 disabled the ngx.read_body API in HTTP/2. However, #2211 also suggested that perhaps we should not completely disable the ngx.read_body API in HTTP/2, as many HTTP/2 requests could still benefit from this.
To address this, I attempted to define the minimum limitations for using ngx.read_body in HTTP/2. From what I understand, ngx.read_body is based on the Nginx core to handle the client's body, and the Nginx core typically reads the entire client body before calling the callback registered by the lua-nginx-module. There are two scenarios in which HTTP/2 can trigger this callback. First, when the Content-Length header is used, Nginx core can determine the exact number of bytes to read from the client side. The second scenario is when an HTTP/2 request sends the entire body and immediately closes the HTTP/2 stream. Additionally, HTTP/2 does not support the chunked transfer encoding.
The first scenario is relatively straightforward to detect in the lua-nginx-module. However, the second scenario is challenging to identify because Nginx core takes over the remaining part, and the lua-nginx-module can only wait to be called by the Nginx core. Therefore, in this pull request, I propose to only allow HTTP/2 requests that carry the Content-Length header. So in the second scenario, If someone want to use ngx.read_body, need to add the Content-Length header.
I also considered adding a timeout option for the read_body API. However, I decided against it for two reasons:
Nginx already has a timeout option for reading the client's body, which is configured using client_body_timeout. This configuration terminates the HTTP request when a timeout occurs. Introducing another timeout option, which would keep the rest of the Lua thread logic running, seemed redundant.
Secondly, I believe that scenarios causing the blocking of the ngx.read_body API should not use ngx.read_body. Adding a timeout option may not immediately expose the real problem.
It's worth noting that HTTP/3 faces a similar problem, and this pr implemented the same solution for it.