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: correctly encode bytes for V2 signature #382

Merged
merged 3 commits into from Feb 17, 2021

Conversation

tritone
Copy link
Contributor

@tritone tritone commented Feb 17, 2021

V2 signature was passing a string to the sign_bytes function
instead of bytes. This works fine for most credentials (since
their sign_bytes implementations accept strings) but not for
impersonated credentials. V4 signature encodes the string before
calling sign_bytes, so I do the same here.

We should also look into clarifying the contract for the
sign_bytes interface in the auth library.

Fixes #373

V2 signature was passing a string to the sign_bytes function
instead of bytes. This works fine for most credentials (since
their sign_bytes implementations accept strings) but not for
impersonated credentials. V4 signature encodes the string before
calling sign_bytes, so I do the same here.

We should also look into clarifying the contract for the
sign_bytes interface in the auth library.

Fixes googleapis#373
@tritone tritone requested a review from a team February 17, 2021 03:30
@tritone tritone requested a review from a team as a code owner February 17, 2021 03:30
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Feb 17, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 17, 2021
@@ -77,7 +77,7 @@ def get_signed_query_params_v2(credentials, expiration, string_to_sign):
signed payload.
"""
ensure_signed_credentials(credentials)
signature_bytes = credentials.sign_bytes(string_to_sign)
signature_bytes = credentials.sign_bytes(string_to_sign.encode("ascii"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM.

Opnion: Issue here is that we have multiple sign versions and we focused on v4 more recently.

@tritone tritone merged commit f44212b into googleapis:master Feb 17, 2021
@tritone tritone deleted the signed-url-fix branch February 17, 2021 19:01
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* fix: correctly encode bytes for V2 signature

V2 signature was passing a string to the sign_bytes function
instead of bytes. This works fine for most credentials (since
their sign_bytes implementations accept strings) but not for
impersonated credentials. V4 signature encodes the string before
calling sign_bytes, so I do the same here.

We should also look into clarifying the contract for the
sign_bytes interface in the auth library.

Fixes googleapis#373

* fix py2 failure
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* fix: correctly encode bytes for V2 signature

V2 signature was passing a string to the sign_bytes function
instead of bytes. This works fine for most credentials (since
their sign_bytes implementations accept strings) but not for
impersonated credentials. V4 signature encodes the string before
calling sign_bytes, so I do the same here.

We should also look into clarifying the contract for the
sign_bytes interface in the auth library.

Fixes googleapis#373

* fix py2 failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It's impossible to generate a signed url (v2) with impersonated Credentials
2 participants