aws-cli icon indicating copy to clipboard operation
aws-cli copied to clipboard

Add support for generating a pre-signed S3 PUT url

Open G-Rath opened this issue 3 years ago • 12 comments

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 avatar May 27 '22 01:05 G-Rath

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)

G-Rath avatar Jun 03 '22 00:06 G-Rath

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.

justin-kidman-axomic avatar Jul 30 '22 12:07 justin-kidman-axomic

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%.

:exclamation: 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%) :arrow_up:
awscli/customizations/s3/subcommands.py 95.56% <40.00%> (+95.56%) :arrow_up:

... and 204 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 31 '22 19:07 codecov-commenter

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

G-Rath avatar Jul 31 '22 19:07 G-Rath

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 avatar Oct 23 '23 19:10 fczuardi

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 :(

fczuardi avatar Oct 24 '23 13:10 fczuardi

@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 avatar Oct 24 '23 18:10 G-Rath

@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?

G-Rath avatar Oct 24 '23 18:10 G-Rath