Limit picod request body size for prevent DoS attack#326
Conversation
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
|
@RainbowMango: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @RainbowMango! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR updates PicoD’s Gin server setup to enforce a maximum HTTP request body size globally (not only for authenticated routes), reducing DoS risk from oversized payloads.
Changes:
- Add a global Gin middleware that wraps
c.Request.Bodywithhttp.MaxBytesReader(..., MaxBodySize)and setengine.MaxMultipartMemory = MaxBodySize. - Remove the request body size enforcement from
AuthMiddleware, keeping auth focused on JWT verification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/picod/server.go | Adds global request body size limiting middleware and aligns multipart parsing memory cap with MaxBodySize. |
| pkg/picod/auth.go | Removes body-size enforcement from authentication middleware. |
| // Limit request body size to 32 MB to prevent DoS attacks | ||
| engine.Use(func(c *gin.Context) { | ||
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) | ||
| c.Next() | ||
| }) | ||
| engine.MaxMultipartMemory = MaxBodySize |
| // Global middleware | ||
| engine.Use(gin.Logger()) // Request logging | ||
| engine.Use(gin.Recovery()) // Crash recovery | ||
| // Limit request body size to 32 MB to prevent DoS attacks |
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) | ||
| c.Next() | ||
| }) | ||
| engine.MaxMultipartMemory = MaxBodySize |
|
|
||
| // Enforce maximum body size to prevent memory exhaustion | ||
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) | ||
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves the request body size limit enforcement from the authentication middleware to a global middleware in the server setup. Feedback includes a request to remove redundant blank lines in auth.go, a suggestion to generalize a comment in server.go to avoid hardcoded values, and a recommendation to lower MaxMultipartMemory to prevent high memory consumption during multipart uploads.
|
|
||
| // Enforce maximum body size to prevent memory exhaustion | ||
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) | ||
|
|
There was a problem hiding this comment.
There are multiple consecutive blank lines here. Please remove the extra blank line to maintain consistent code style.
| // Global middleware | ||
| engine.Use(gin.Logger()) // Request logging | ||
| engine.Use(gin.Recovery()) // Crash recovery | ||
| // Limit request body size to 32 MB to prevent DoS attacks |
There was a problem hiding this comment.
The comment explicitly mentions '32 MB', which might become outdated if the MaxBodySize constant is changed in the future. It is better to refer to the constant name or use a more generic description.
| // Limit request body size to 32 MB to prevent DoS attacks | |
| // Limit request body size to prevent DoS attacks |
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) | ||
| c.Next() | ||
| }) | ||
| engine.MaxMultipartMemory = MaxBodySize |
There was a problem hiding this comment.
Setting engine.MaxMultipartMemory to the same value as the hard body size limit (MaxBodySize) effectively disables Gin's ability to spill large multipart uploads to disk. If the goal is to prevent memory exhaustion, it is often better to keep MaxMultipartMemory at a lower value (e.g., 8 MiB) while using http.MaxBytesReader to enforce the absolute maximum request size. This allows processing files up to 32 MiB without consuming 32 MiB of RAM per concurrent request.
| engine.Use(gin.Recovery()) // Crash recovery | ||
| // Limit request body size to 32 MB to prevent DoS attacks | ||
| engine.Use(func(c *gin.Context) { | ||
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, MaxBodySize) |
There was a problem hiding this comment.
TestAuthMiddleware_MaxBodySize needs to be updated as well.
What type of PR is this?
/kind cleanup
/kind security
What this PR does / why we need it:
This pull request refactors how the maximum request body size is enforced in the application. The body size limit logic has been moved from the authentication middleware to a new global middleware, ensuring that all incoming requests are protected from large payloads, not just those passing through authentication.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This PR is split from #250 to address the comments of #250 (comment).
Does this PR introduce a user-facing change?: