aioaws icon indicating copy to clipboard operation
aioaws copied to clipboard

Fix handling of bucket names

Open br3ndonland opened this issue 3 years ago • 2 comments

Description

This PR will correct two interrelated issues with how aioaws handles bucket names.

Bucket names are not correctly parsed from bucket domains when adding to upload policy conditions

S3 buckets can be provided either as the bucket name alone, like aioaws-testing, or as a "virtual hosted-style" domain, like aioaws-testing.s3.us-east-1.amazonaws.com.

When a bucket domain is provided, aioaws.s3.S3Client.signed_upload_url is incorrectly adding the entire domain as an upload policy condition:

https://github.com/samuelcolvin/aioaws/blob/9e938435da88bb0be26b21b834f37e6cd759e15e/aioaws/s3.py#L176-L181

Attempts to upload will raise AccessDenied errors, like Invalid according to Policy: Policy Condition failed: ["eq", "$bucket", "aioaws-testing.s3.us-east-1.amazonaws.com"].

Tests were not catching this error because they were only testing with bucket names, not bucket domains.

This PR will correct aioaws.s3.S3Client.signed_upload_url to help ensure that bucket names, not bucket domains, are used as policy conditions.

Bucket names with dots cannot be used

As explained in #22, bucket names can also have . in them, like my_bucket_name.com or my_bucket_name.com.s3.us-east-1.amazonaws.com.

aioaws currently assumes any bucket name with a . is a domain:

https://github.com/samuelcolvin/aioaws/blob/9e938435da88bb0be26b21b834f37e6cd759e15e/aioaws/core.py#L42-L47

This PR will update the check for bucket domains to use a more specific regex, rather than simply ..

Changes

Clients

Parse bucket names by checking for ([^.]+).s3.[^.]+.amazonaws.com, as suggested in https://github.com/samuelcolvin/aioaws/pull/24#discussion_r789789613, and take this approach consistently in both aioaws.core.AwsClient and aioaws.s3.S3Client.signed_upload_url.

https://github.com/samuelcolvin/aioaws/blob/505414e036b5274676795e4c6d5f2822c69be9a8/aioaws/core.py#L25

https://github.com/samuelcolvin/aioaws/blob/505414e036b5274676795e4c6d5f2822c69be9a8/aioaws/core.py#L43-L49

https://github.com/samuelcolvin/aioaws/blob/505414e036b5274676795e4c6d5f2822c69be9a8/aioaws/s3.py#L176-L180

As explained in https://github.com/samuelcolvin/aioaws/pull/24#discussion_r843335603, this parsing approach introduces certain limitations. For example, aioaws can only be used on AWS S3 specifically, and not other S3-compatible platforms, like Backblaze B2, Cloudflare R2, DigitalOcean Spaces, Linode Object Storage, etc.

Tests

  • Parametrize tests/test_s3.py::test_upload_url and tests/test_s3.py::test_real_upload to run with both a bucket name and a bucket domain
  • Update the hard-coded signature in tests/test_s3.py::test_upload_url for the updated test parameters

br3ndonland avatar Dec 05 '21 17:12 br3ndonland

Codecov Report

Merging #24 (505414e) into main (bd8c0a5) will decrease coverage by 0.72%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   97.31%   96.59%   -0.73%     
==========================================
  Files           9        9              
  Lines         522      528       +6     
  Branches       95       96       +1     
==========================================
+ Hits          508      510       +2     
- Misses          9       11       +2     
- Partials        5        7       +2     
Impacted Files Coverage Δ
aioaws/core.py 98.00% <100.00%> (-2.00%) :arrow_down:
aioaws/s3.py 97.47% <100.00%> (-1.66%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bd8c0a5...505414e. Read the comment docs.

codecov[bot] avatar Jan 22 '22 20:01 codecov[bot]

Tests appear to be running now (?).

I'll try to boost the coverage back up.

br3ndonland avatar Jan 22 '22 20:01 br3ndonland