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

implement S3 ASF pre-signed POST support #6980

Merged
merged 3 commits into from Oct 6, 2022
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Oct 5, 2022

This PR implements S3 pre-signed POST support
Currently, the provider does not implement signature validation, but it is not a lot of work to implement, as only the policy is signed (a JSON string encoded in base64). It can be done in a follow up PR.

To implement support for this operation, we needed to create an operation not defined in the specs, as the clients (like Boto) would never call directly to this URL. But it is implement server side, and documented. See https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPOST.html

Also, I had to make a small change to the serializer, but I'm not sure it should be generalised. I think 3XX status code are not supposed to return a body. So I skipped serializing the body in case the status code was >= 300 and < 400.

It also came to light that the botocore parser does not like 3XX responses from moto, as it tries to parse them as exception. I need to add a special try...except clause to catch the exception and act in consequence.

It also implements small tests fixes for pre-signed URL in general, and wrong logic in skipping them/checking conditions based on Legacy/ASF.

@bentsku bentsku temporarily deployed to localstack-ext-tests October 5, 2022 21:15 Inactive
@bentsku bentsku temporarily deployed to localstack-ext-tests October 5, 2022 22:18 Inactive
@coveralls
Copy link

coveralls commented Oct 5, 2022

Coverage Status

Coverage decreased (-0.2%) to 78.478% when pulling fa75068 on s3-asf-presigned-post into 8192c19 on master.

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

LocalStack integration with Pro

       1 files   -     2         1 suites   - 2   58m 5s ⏱️ - 23m 57s
1 344 tests  -     6  1 196 ✔️  -   24  148 💤 +  18  0 ±0 
1 344 runs   - 518  1 196 ✔️  - 392  148 💤  - 126  0 ±0 

Results for commit fa75068. ± Comparison against base commit 8192c19.

♻️ This comment has been updated with latest results.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

nice! well done!

localstack/services/s3/provider.py Outdated Show resolved Hide resolved
@bentsku bentsku temporarily deployed to localstack-ext-tests October 6, 2022 14:40 Inactive
@bentsku bentsku merged commit 8ba04c2 into master Oct 6, 2022
@bentsku bentsku deleted the s3-asf-presigned-post branch October 6, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants