Skip to content
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

Merged
merged 8 commits into from Feb 13, 2020

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 12, 2020

Fixes #12

W/ exception of query string parameter quoting bits being addressed in PR #48.

Copy link
Member

@frankyn frankyn left a comment

I have one nit overall LGTM, thanks @tseaver.


if "x-goog-content-sha256" in lowercased_headers:
payload = lowercased_headers["x-goog-content-sha256"]
elif "x-amz-content-sha256" in lowercased_headers:
Copy link
Member

@frankyn frankyn Feb 12, 2020

Only support x-goog- and ignore x-amz.

Copy link
Member

@frankyn frankyn left a comment

One question, but overall LGTM. Thanks @tseaver

@@ -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))
Copy link
Member

@frankyn frankyn Feb 12, 2020

Does this encode '/' as well? It should not.

Copy link
Contributor Author

@tseaver tseaver Feb 12, 2020

It does not: it is actually designed to quote the path part.

Copy link
Member

@frankyn frankyn Feb 12, 2020

This might hit an issue with conformance tests when that's added but approving for now.

Copy link
Contributor Author

@tseaver tseaver Feb 13, 2020

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.

Copy link
Member

@frankyn frankyn left a comment

LGTM.

tseaver added 2 commits Feb 12, 2020
The caller already URL-encodes the 'resource' path. and so we
don't want to do it here.

Reverts commit 6cc5f8a.
@tseaver tseaver merged commit 8712da8 into master Feb 13, 2020
3 checks passed
@tseaver tseaver deleted the 12-fix-v4-signing-formatting branch Aug 24, 2021
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* Rename 'canonicalize' to show V2 only.
* Refactor / simplify header whitespace normalization.
* Sign user-supplied payload hash.
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* Rename 'canonicalize' to show V2 only.
* Refactor / simplify header whitespace normalization.
* Sign user-supplied payload hash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants