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

Storage: add support for V4 signed URLs #7460

Merged
merged 37 commits into from
Apr 17, 2019
Merged

Storage: add support for V4 signed URLs #7460

merged 37 commits into from
Apr 17, 2019

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 26, 2019

Partial support for the new feature: adds a version argument to Blob.generate_signed_url, and uses it to switch between the two signing algorithms.

Remaining work:

  • Improve assertions for how variations in calling are handled (different methods, headers, etc.) See the data-driven tests in Data-driven V4 URL signing tests, and implementation changes google-cloud-dotnet#2882 for examples (even better, work out how to share them like the Firestore conformance tests).
  • Add system tests for V4 blob signed URLs.
  • Add Bucket.generate_signed_url, to permit listing bucket contents.
  • Add system test for resumable upload using V4 blob signed URL.

Deferred work:

No longer in scope:

  • Add Client.generate_signed_url, to permit listing buckets.

@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Feb 26, 2019
@tseaver tseaver requested a review from frankyn February 26, 2019 22:50
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 26, 2019
@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 26, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Mar 4, 2019

The docs build failure on CI is unrelated to this PR: it was fixed in #7458.

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.

Doing a first pass.

.. note::

If you are on Google Compute Engine, you can't generate a signed URL.
Follow `Issue 922`_ for updates on this. If you'd like to be able to
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankyn googleapis/google-auth-library-python#50 is still open: AFAIK, the only way to get signing done on GCE is to use the IAM-based workaround. I plan to add that here (following your Ruby PoC) once we're OK with the surface.

storage/google/cloud/storage/_signing.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/_signing.py Show resolved Hide resolved
storage/google/cloud/storage/_signing.py Show resolved Hide resolved
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 5, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Mar 11, 2019

@frankyn A question: the current Python V2 version allows for passing method="RESUMABLE", and converts that to "POST" and mutates the resource per https://cloud.google.com/storage/docs/access-control/signed-urls#signing-resumable. Should V4 follow suit?

@frankyn
Copy link
Member

frankyn commented Mar 12, 2019

At a blob level, yes. The resumable header is also supported when using V4 signed URLs.

One request I have is to state in the documentation that the HeaderName:HeaderValue is expected in a follow-up request. At the moment only the name is documented.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 12, 2019

Flaky spanner systest reported in #7504.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 12, 2019

@crwilcox Can you check what is hanging the Kokoro builds here?

@frankyn frankyn added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Mar 12, 2019
@frankyn
Copy link
Member

frankyn commented Mar 12, 2019

@tseaver please do not support signed URLs at a client level.

@crwilcox
Copy link
Contributor

@tseaver do you need anything from me to move this along?

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.

Python doesn't have the timestamp offset bug mentioned in email.

@tseaver
Copy link
Contributor Author

tseaver commented Apr 1, 2019

@frankyn As of 0d54faa, this PR is passing all the cross-language conformance tests from the dotnet repository. I have one system test passing as well (simple Blob GET). 734a9b3 adds V4 systests paralleling the existing V2 ones.

@tseaver tseaver removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love. labels Apr 1, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Apr 1, 2019

@frankyn 68742be adds Bucket.generate_signed_url (with support for both V2 and V4). 6c5cfc7 adds systests for that feature.

@tseaver
Copy link
Contributor Author

tseaver commented Apr 1, 2019

@frankyn I believe the only remaining work is to add support for IAM / access token signing: note that the feature does not exist yet for V2 in Python.

@tseaver tseaver changed the title [WIP] Storage: add support for V4 signed URLs Storage: add support for V4 signed URLs Apr 1, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 1, 2019
@@ -387,16 +432,25 @@ def generate_signed_url(
client = self._require_client(client)
credentials = client._credentials

return generate_signed_url(
if version == "v2":
Copy link
Member

Choose a reason for hiding this comment

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

@tseaver could you add a warning as log output that V2 will change to V4 in the future:

You have generated a signed URL using the default v2 signing implementation. In the future, this will default to v4. You may experience breaking changes if you use longer than 7 day expiration times with v4. To opt-in to the behavior specify version="v2".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tseaver
Copy link
Contributor Author

tseaver commented Apr 1, 2019

@frankyn 2a956e5 adds the systest for signed resumable upload URLs (both V2 and V4), including round-tripping.

@tseaver
Copy link
Contributor Author

tseaver commented Apr 17, 2019

@frankyn PTAL.

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.

Two nits and LGTM.
Thanks @tseaver!

storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/bucket.py Outdated Show resolved Hide resolved
storage/tests/system.py Outdated Show resolved Hide resolved
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.

LGTM, pending tests. Thanks @tseaver!

@tseaver tseaver merged commit 7527784 into googleapis:master Apr 17, 2019
@tseaver tseaver deleted the storage-v4_signing-wip branch April 17, 2019 22:34
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 Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants