Enforce size limit on POST /api/fleet/uploads#6159
Enforce size limit on POST /api/fleet/uploads#6159ycombinator merged 10 commits intoelastic:mainfrom
POST /api/fleet/uploads#6159Conversation
|
This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
|
pzl
left a comment
There was a problem hiding this comment.
looks good
pointed out a few places where names or messages were unclear if it was describing large files (.i.e. a file itself being like 3GB with config set lower, and either rejecting file body bytes above that; or when reading this json file-description payload, looking at file.size property and rejecting for that limit). Whereas this is the limit placed on the total JSON payload body of file metadata / header.
The diff in isolation is clear which is being addressed, but down the road I can see this being an easy mixup, which thing is "too large": the file contents itself or the start / header payload.
Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com>
michel-laterman
left a comment
There was a problem hiding this comment.
Do we also want to use the config vars for upload finailize and chunks?
Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com>
Sure, I think this makes sense. Will update PR. |
Implemented request body size limit for upload finalize API in 3fbd7c7. For the upload chunk API, I'm seeing this check already: fleet-server/internal/pkg/api/handleUpload.go Lines 130 to 131 in acc7110 Do we need/want to do more? |
|
@pzl, should we allow for users to specify a custom value for file chunk size? |
Probably not a good idea without specific reason. This size was chosen quite specifically. A non-exhaustive list of concerns:
|
|
@Mergifyio backport 8.19 9.2 9.3 |
✅ Backports have been createdDetails
Cherry-pick of b91dc36 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of b91dc36 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
|
* Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Rename test case * Update internal/pkg/api/handleUpload.go Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> (cherry picked from commit b91dc36) # Conflicts: # internal/pkg/api/handleUpload_test.go
* Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Rename test case * Update internal/pkg/api/handleUpload.go Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> (cherry picked from commit b91dc36) # Conflicts: # internal/pkg/api/handleUpload_test.go
* Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Rename test case * Update internal/pkg/api/handleUpload.go Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> (cherry picked from commit b91dc36)
* Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml * Rename test case * Update internal/pkg/api/handleUpload.go * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- (cherry picked from commit b91dc36) Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com>
#6234) * Enforce size limit on `POST /api/fleet/uploads` (#6159) * Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Rename test case * Update internal/pkg/api/handleUpload.go Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> (cherry picked from commit b91dc36) # Conflicts: # internal/pkg/api/handleUpload_test.go * Fix conflicts --------- Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
…#6235) * Enforce size limit on `POST /api/fleet/uploads` (#6159) * Write unit test for max payload size * Enforce max payload size * Adding CHANGELOG fragment * Update changelog/fragments/1767789965-upload-limit.yaml Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Rename test case * Update internal/pkg/api/handleUpload.go Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> * Add uploader.ErrPayloadSizeTooLarge * Implement payload size limit for upload complete API * Return error instead of writing header directly --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> (cherry picked from commit b91dc36) # Conflicts: # internal/pkg/api/handleUpload_test.go * Fixing conflicts --------- Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
What is the problem this PR solves?
// Please do not just reference an issue. Explain WHAT the problem this PR solves here.
This PR prevents request bodies of arbitrarily large size to be sent to the
POST /api/fleet/uploadsAPI.How does this PR solve the problem?
// Explain HOW you solved the problem in your code. It is possible that during PR reviews this changes and then this section should be updated.
This PR checks the size of the request body sent to the
POST /api/fleet/uploadsAPI and, if it exceeds the configured limit, rejects the request, responding with an HTTP 413 Request Entity Too Large status code. By default, the limit is configured to 5MiB but can be overridden in the Fleet Server input configuration via theserver.limits.upload_start_limit.max_body_byte_sizesetting.How to test this PR locally
Design Checklist
Checklist
./changelog/fragmentsusing the changelog toolRelated issues