aioaws
aioaws copied to clipboard
Fix handling of bucket names
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
andtests/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
Codecov Report
Merging #24 (505414e) into main (bd8c0a5) will decrease coverage by
0.72%
. The diff coverage is100.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.
Tests appear to be running now (?).
I'll try to boost the coverage back up.