-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sign v4 breakage in Python 3 #427
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -198,7 +198,7 @@ def sign_v4(method, url, region, headers=None, access_key=None, | |||
|
|||
date = datetime.utcnow() | |||
headers['X-Amz-Date'] = date.strftime("%Y%m%dT%H%M%SZ") | |||
headers['X-Amz-Content-Sha256'] = content_sha256 | |||
headers['X-Amz-Content-Sha256'] = content_sha256.decode('ascii') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content_sha256 can be "UNSIGNED_PAYLOAD" ".decode will fail in that scenario. Do it like this
if content_sha256 == _UNSIGNED_PAYLOAD:
headers['X-Amz-Content-Sha256'] = content_sha256
else:
headers['X-Amz-Content-Sha256'] = content_sha256.decode('ascii')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems pretty clumsy to keep content_sha256
as bytes
in one case and as str
in the other. I am making a slightly better change to handle this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is but it should be handled in a way UNSIGNED_PAYLOAD should still be a string, keeping it as byte causes presigned signature requests to fail.
@@ -209,7 +209,7 @@ def sign_v4(method, url, region, headers=None, access_key=None, | |||
canonical_req = generate_canonical_request(method, | |||
parsed_url, | |||
headers_to_sign, | |||
content_sha256) | |||
content_sha256.decode('ascii')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
It was introduced in commit id 31e8a35 _get_bucket_region() was failing in Python 3 with signature mismatch error.
69de3ce
to
2f5c359
Compare
It was introduced in commit id 31e8a35 - once this was found (with git bisect), the fix was easy to figure out.
_get_bucket_region()
was failing in Python 3 with signature mismatcherror.
Short program that demonstrates the issue:
Generates: