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

fix S3 v3 Range handling #9082

Merged
merged 2 commits into from Sep 8, 2023
Merged

fix S3 v3 Range handling #9082

merged 2 commits into from Sep 8, 2023

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Sep 6, 2023

Motivation

As reported in #9076, there is an issue in the way Moto and LocalStack handles HTTP Range requests for S3. AWS is apparently not following the RFC for range requests and is very, very forgiving, as shown by the added test.

An int-range is invalid if the last-pos value is present and less than the first-pos.

As pointed out by @sjperkins who went through the RFC, the server can ignore the range header for some conditions:

A server that supports range requests MAY ignore or reject a Range header field that contains an invalid ranges-specifier (Section 14.1.1), a ranges-specifier with more than two overlapping ranges, or a set of many small ranges that are not listed in ascending order,

Basically, if the range request is badly formatted or invalid, S3 will just treat the request as a non-range request and will return the entire object with a 200 status code.
Only certain conditions will trigger an exception and 416, mainly if the beginning of the request is above the end of the object and the request is a valid Range request (meaning it can be parsed), or if the request is a "suffix" request and it ends with 0.

Changes

This fixes the behaviour only for the v3 provider for now, I'll need to open a PR upstream in moto to fix the behaviour there to fix the default current provider. Edit: the PR upstream is already merged, so I'd like to have the v3 behaviour in line.

I might remove all the skip for the new provider tests and selectively xfail the tests for the default provider in a follow up PR, to see the extent of the issue with the current provider, and try to have some tests such as this one valid for both.

I've launched a run with this fix in the forced v3 S3 branch here, and it's green: https://app.circleci.com/pipelines/github/localstack/localstack/17924/workflows/3026ab82-afaa-4b31-9b45-fbd7afbab1bd

This also fix the behaviour for UploadPartCopy which has a very strict handling of the CopySourceRange parameter.

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Sep 6, 2023
@bentsku bentsku self-assigned this Sep 6, 2023
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 27m 0s ⏱️
2 171 tests 1 671 ✔️ 500 💤 0
2 172 runs  1 671 ✔️ 501 💤 0

Results for commit 20c58e5.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review September 6, 2023 20:23
@sjperkins
Copy link

AWS is apparently not following the RFC for range requests and is very, very forgiving, as shown by the added test.

Just wanted to strengthen the above argument by drawing attention to #9076 (comment). The rfc gives servers the latitude to ignore an invalid range request:

A server that supports range requests MAY ignore or reject a Range header field that contains an invalid ranges-specifier (Section 14.1.1), a ranges-specifier with more than two overlapping ranges, or a set of many small ranges that are not listed in ascending order,

@bentsku
Copy link
Contributor Author

bentsku commented Sep 7, 2023

Thank you @sjperkins, I'll update the PR description with the quote. Very appreciated!

Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bentsku bentsku merged commit 1406450 into master Sep 8, 2023
28 checks passed
@bentsku bentsku deleted the fix-s3-range branch September 8, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants