implement sigv4 signing for s3 downloads #21956
Conversation
chris-smith-zocdoc
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
chris-smith-zocdoc
changed the title
chris-smith-zocdoc
force-pushed
the
cs_fix_aws_sigv4
branch
from
f75e8eb to
6e5b53c
Compare
| # and fallback to us-east-1 | ||
| signing_region = request.region or aws_credentials.default_region or "us-east-1" | ||
|
|
||
| signer = auth.SigV4Auth(aws_credentials.creds, "s3", signing_region) |
There was a problem hiding this comment.
Is it worth supporting the old codepath under a flag (HmacV1Auth)? Not sure risky you view this change as
There was a problem hiding this comment.
Good idea. If on the remote chance a user does see an issue, they can just configure the old behavior. Even if S3 might be perfectly fine with it, I can imagine a user using some S3 API-compatible service which we have never heard of and having an issue. It may never happen but I can't discount the possibility. Feature flags are cheap insurance.
You can set a removal_version and removal_hint on the transition option so that we maintainers know to remove the option at an appropriate point in the future (or reevaluate its necessity at least, maybe document that in the removal_hint).
There was a problem hiding this comment.
Is there a particular options subsystem I should add to? I don't see one for url handlers/s3. Or I could make a new one
There was a problem hiding this comment.
Yeah I don't see one either. Maybe add a new one then?
There was a problem hiding this comment.
Maybe aws-s3-download-handler or a better name?
There was a problem hiding this comment.
@tdyas I've added the hmacv1 back along with a new option subsystem to enable it (defaulting to sigv4) I wasn't sure deprecation is really necessary so I omitted it for now. If you'd like me to add it I can though, how many versions out should I target?
chris-smith-zocdoc
force-pushed
the
cs_fix_aws_sigv4
branch
3 times, most recently
from
9a2032c to
e6fa17d
Compare
chris-smith-zocdoc
force-pushed
the
cs_fix_aws_sigv4
branch
from
e6fa17d to
152eaa3
Compare
| botocore = SimpleNamespace() | ||
| botocore.exceptions = SimpleNamespace(NoCredentialsError=Exception) |
There was a problem hiding this comment.
Use of Exception here was causing some tests to pass (through this catch
) that should have been failing. I've updated the cases.| def add_auth(request): | ||
| request.url == expected_url | ||
| assert request.url == expected_auth_url |
There was a problem hiding this comment.
missing assert
| @pytest.fixture | ||
| def monkeypatch_botocore(monkeypatch): | ||
| def do_patching(expected_url): |
There was a problem hiding this comment.
renamed to match the parameter in the tests cases below
| [ | ||
| ( | ||
| "s3://bucket/keypart1/keypart2/file.txt", | ||
| "https://s3.amazonaws.com/bucket/keypart1/keypart2/file.txt", |
There was a problem hiding this comment.
under hmacv1 the url thats signed is the s3 path style (which can be different that what the request is actually made with)
| "https://bucket.s3.amazonaws.com/keypart1/keypart2/file.txt", | ||
| "https://bucket.s3.amazonaws.com/keypart1/keypart2/file.txt", |
There was a problem hiding this comment.
for sigv4 is the same url we make the request with (virtual host style)
|
Thanks for the contribution. We've just branched for 2.26, so merging this pull request now will come out in 2.27, please move the release notes updates to |
Fixes #21955