Skip to content

Add support for generating a pre-signed S3 PUT url#6993

Open
G-Rath wants to merge 1 commit intoaws:developfrom
G-Rath:s3-support-more-presigned-links
Open

Add support for generating a pre-signed S3 PUT url#6993
G-Rath wants to merge 1 commit intoaws:developfrom
G-Rath:s3-support-more-presigned-links

Conversation

@G-Rath
Copy link
Copy Markdown

@G-Rath G-Rath commented May 27, 2022

Issue #, if available: #3050

Description of changes:

This adds support for aws s3 presign-put per the desired design.

The design mentioned having presign become an alias for presign-get - I've left this out for now because it's not dependent on this PR so should be doable in a follow up PR (I don't mind doing it in this one, but for now am focusing on the core function).

The design also mentioned having presign-post which I've also left out as from what I can tell that requires significantly more work? I would be interested in exploring supporting that too as it sounds more powerful because you can pass a policy, but I'd prefer landing it separately given how PR should be ready to go and be usable without it.

Finally, I've got a test drafted up but need some guidance on how to handle doing a PUT request as I can't find any examples elsewhere in the codebase and am not versed in Python enough to feel confident in picking an approach (I'm hoping that six has a method similar to it's urlopen but haven't been able to find anything so far).

This is the draft of the test:

class TestPresignPutCommand(BaseS3IntegrationTest):

    def test_can_upload_presigned_url(self):
        bucket_name = _SHARED_BUCKET
        file_contents = b'this is foo.txt'
        p = aws('s3 presign-put s3://%s/foo.txt' % (bucket_name,))
        self.assert_no_errors(p)

        url = p.stdout.strip()

        # todo: make PUT request

        # Make sure object is in bucket.
        self.assertTrue(self.key_exists(bucket_name, key_name='foo.txt'))
        self.assertEqual(
                self.get_key_contents(bucket_name, key_name='foo.txt'),
                file_contents)

        self.assertEqual(
                self.content_type_for_key(bucket_name, key_name='foo.txt'),
                'text/plain')

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@G-Rath
Copy link
Copy Markdown
Author

G-Rath commented Jun 3, 2022

The CI failures seem unrelated to my change

ERROR: Package 'botocore' requires a different Python: 3.6.15 not in '>=3.7'

& (on Windows)

'pytest' is not recognized as an internal or external command

Let me know if there's anything I need to do (e.g. rebase to pull in a fix)

@justin-kidman-axomic
Copy link
Copy Markdown

The CI failures seem unrelated to my change

ERROR: Package 'botocore' requires a different Python: 3.6.15 not in '>=3.7'

& (on Windows)

'pytest' is not recognized as an internal or external command

Let me know if there's anything I need to do (e.g. rebase to pull in a fix)

Should pass if you update your branch to latest base path. They removed Python 3.6 from the testing matrix now.

@G-Rath G-Rath force-pushed the s3-support-more-presigned-links branch from 927303f to 8db2324 Compare July 31, 2022 19:15
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 31, 2022

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (6e0239d) 0.08% compared to head (927303f) 92.82%.

❗ Current head 927303f differs from pull request most recent head 4111106. Consider uploading reports for the commit 4111106 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #6993       +/-   ##
============================================
+ Coverage     0.08%   92.82%   +92.74%     
============================================
  Files          208      204        -4     
  Lines        16629    16344      -285     
============================================
+ Hits            14    15172    +15158     
+ Misses       16615     1172    -15443     
Files Coverage Δ
awscli/customizations/s3/s3.py 100.00% <ø> (+100.00%) ⬆️
awscli/customizations/s3/subcommands.py 95.56% <40.00%> (+95.56%) ⬆️

... and 204 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@G-Rath
Copy link
Copy Markdown
Author

G-Rath commented Jul 31, 2022

@justin-kidman-axomic thanks for the heads up - I've rebased my branch, so now someone just needs to approve running the workflow

@fczuardi
Copy link
Copy Markdown

fczuardi commented Oct 23, 2023

I want to use your fork and test this new command. what is the best course of action?

[Update]: Oh, I got my answer, the new binary is on ./bin/aws

@fczuardi
Copy link
Copy Markdown

I noticed that this patch is for an older version of the CLI, that dont yet uses Signature Version 4, so maybe this is the main reason that the patch didnt got merged :(

@G-Rath
Copy link
Copy Markdown
Author

G-Rath commented Oct 24, 2023

@fczuardi this patch hasn't been merged as its still waiting to be reviewed; sigv4 has been out for quite a while and even mentioned in the description for this command so my expectation is it does support it, but if that was a blocker it would be requested in a review (which just has not happened yet).

@G-Rath G-Rath force-pushed the s3-support-more-presigned-links branch from 20ee029 to 4111106 Compare October 24, 2023 18:11
@G-Rath
Copy link
Copy Markdown
Author

G-Rath commented Oct 24, 2023

@fczuardi if it helps, I've rebased this off latest develop so it should be more up to date

@kdaily @tim-finnigan it would be good if we could move this PR along - could I at least get the workflows approved to know if CI is passing?

@yasamoka
Copy link
Copy Markdown

Hello, are there any blockers remaining for this PR? I would like to help move this along if possible. Thanks!

@G-Rath
Copy link
Copy Markdown
Author

G-Rath commented Jun 17, 2024

@yasamoka no blockers, just waiting for review from the AWS team

@coltfred
Copy link
Copy Markdown

It would be great if they could get this merged so the CLI could be used for both PUT and GET. I ran into this issue today.

@stepancheg
Copy link
Copy Markdown

I checked the PR, it works when rebased on master. Merge it please?

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.

Please support generation of pre-signed URLs for S3 other than GET

8 participants