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: make v4 signing formatting consistent w/ spec #56
Conversation
Prep for review / update of *V4* canonicalization.
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.
I have one nit overall LGTM, thanks @tseaver.
google/cloud/storage/_signing.py
Outdated
|
||
if "x-goog-content-sha256" in lowercased_headers: | ||
payload = lowercased_headers["x-goog-content-sha256"] | ||
elif "x-amz-content-sha256" in lowercased_headers: |
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.
Only support x-goog- and ignore x-amz.
Per review from @frankyn
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.
One question, but overall LGTM. Thanks @tseaver
tests/unit/test__signing.py
Outdated
@@ -577,7 +577,7 @@ def _generate_helper( | |||
) | |||
self.assertEqual(scheme, expected_scheme) | |||
self.assertEqual(netloc, expected_netloc) | |||
self.assertEqual(path, resource) | |||
self.assertEqual(path, six.moves.urllib.parse.quote(resource)) |
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.
Does this encode '/' as well? It should not.
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 does not: it is actually designed to quote the path part.
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.
This might hit an issue with conformance tests when that's added but approving for now.
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.
As it happens, I backed it out here, because the bucket / blob already do the urlencoding of the resource before calling _sigining.generate_signed_url_v4
.
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.
The caller already URL-encodes the 'resource' path. and so we don't want to do it here. Reverts commit 6cc5f8a.
* Rename 'canonicalize' to show V2 only. * Refactor / simplify header whitespace normalization. * Sign user-supplied payload hash.
* Rename 'canonicalize' to show V2 only. * Refactor / simplify header whitespace normalization. * Sign user-supplied payload hash.
Fixes #12
W/ exception of query string parameter quoting bits being addressed in PR #48.