From 720f48411284cd5b45297a704235b554aa2d25e0 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 13:10:44 -0500 Subject: [PATCH 1/6] refactor: rename 'canonicalize' to show V2 only Prep for review / update of *V4* canonicalization. --- google/cloud/storage/_signing.py | 6 +++--- tests/unit/test__signing.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index e7c8e3328..cde81212b 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -206,8 +206,8 @@ def get_canonical_headers(headers): ) -def canonicalize(method, resource, query_parameters, headers): - """Canonicalize method, resource +def canonicalize_v2(method, resource, query_parameters, headers): + """Canonicalize method, resource per the V2 spec. :type method: str :param method: The HTTP verb that will be used when requesting the URL. @@ -368,7 +368,7 @@ def generate_signed_url_v2( """ expiration_stamp = get_expiration_seconds_v2(expiration) - canonical = canonicalize(method, resource, query_parameters, headers) + canonical = canonicalize_v2(method, resource, query_parameters, headers) # Generate the string to sign. elements_to_sign = [ diff --git a/tests/unit/test__signing.py b/tests/unit/test__signing.py index ebd7f9c17..1eb0d4ebe 100644 --- a/tests/unit/test__signing.py +++ b/tests/unit/test__signing.py @@ -309,12 +309,12 @@ def test_w_embedded_ws(self): self.assertEqual(ordered, expected_ordered) -class Test_canonicalize(unittest.TestCase): +class Test_canonicalize_v2(unittest.TestCase): @staticmethod def _call_fut(*args, **kwargs): - from google.cloud.storage._signing import canonicalize + from google.cloud.storage._signing import canonicalize_v2 - return canonicalize(*args, **kwargs) + return canonicalize_v2(*args, **kwargs) def test_wo_headers_or_query_parameters(self): method = "GET" From 799bb01b0ceb4f478c63add91afa14e409765b67 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 13:52:32 -0500 Subject: [PATCH 2/6] refactor: simplify header ws normalization --- google/cloud/storage/_signing.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index cde81212b..7b3ba2180 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -18,7 +18,6 @@ import collections import datetime import hashlib -import re import json import six @@ -31,8 +30,6 @@ NOW = datetime.datetime.utcnow # To be replaced by tests. -MULTIPLE_SPACES_RE = r"\s+" -MULTIPLE_SPACES = re.compile(MULTIPLE_SPACES_RE) SERVICE_ACCOUNT_URL = ( "https://googleapis.dev/python/google-api-core/latest/" @@ -192,7 +189,7 @@ def get_canonical_headers(headers): normalized = collections.defaultdict(list) for key, val in headers: key = key.lower().strip() - val = MULTIPLE_SPACES.sub(" ", val.strip()) + val = " ".join(val.split()) normalized[key].append(val) ordered_headers = sorted((key, ",".join(val)) for key, val in normalized.items()) From 6cc5f8ab72a1fba57a5ead2036f7d0c819950c2d Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 14:20:51 -0500 Subject: [PATCH 3/6] fix: ensure URI quoting of resource path --- google/cloud/storage/_signing.py | 6 ++++-- tests/unit/test__signing.py | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index 7b3ba2180..f575cf682 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -586,9 +586,11 @@ def generate_signed_url_v4( ordered_query_parameters = sorted(query_parameters.items()) canonical_query_string = six.moves.urllib.parse.urlencode(ordered_query_parameters) + quoted_resource = six.moves.urllib.parse.quote(resource) + canonical_elements = [ method, - resource, + quoted_resource, canonical_query_string, canonical_header_string, signed_headers, @@ -617,7 +619,7 @@ def generate_signed_url_v4( signature = binascii.hexlify(signature_bytes).decode("ascii") return "{}{}?{}&X-Goog-Signature={}".format( - api_access_endpoint, resource, canonical_query_string, signature + api_access_endpoint, quoted_resource, canonical_query_string, signature ) diff --git a/tests/unit/test__signing.py b/tests/unit/test__signing.py index 1eb0d4ebe..2ba6fb67b 100644 --- a/tests/unit/test__signing.py +++ b/tests/unit/test__signing.py @@ -544,9 +544,9 @@ def _generate_helper( generation=None, headers=None, query_parameters=None, + resource="/name/path", ): now = datetime.datetime(2019, 2, 26, 19, 53, 27) - resource = "/name/path" signer_email = "service@example.com" credentials = _make_credentials(signer_email=signer_email) credentials.sign_bytes.return_value = b"DEADBEEF" @@ -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)) self.assertEqual(frag, "") # Check the URL parameters. @@ -656,6 +656,9 @@ def test_w_custom_query_parameters_w_string_value(self): def test_w_custom_query_parameters_w_none_value(self): self._generate_helper(query_parameters={"qux": None}) + def test_w_non_ascii_resource(self): + self._generate_helper(resource="/name/p\xe5th") + def test_with_access_token(self): resource = "/name/path" signer_email = "service@example.com" From 97a26c7274ecf92175c35103e37c3363cb35f129 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 14:42:32 -0500 Subject: [PATCH 4/6] fix: sign user-supplied payload hash --- google/cloud/storage/_signing.py | 11 ++++++++++- tests/unit/test__signing.py | 6 ++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index f575cf682..fef2f6163 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -588,13 +588,22 @@ def generate_signed_url_v4( quoted_resource = six.moves.urllib.parse.quote(resource) + lowercased_headers = dict(ordered_headers) + + if "x-goog-content-sha256" in lowercased_headers: + payload = lowercased_headers["x-goog-content-sha256"] + elif "x-amz-content-sha256" in lowercased_headers: + payload = lowercased_headers["x-amz-content-sha256"] + else: + payload = "UNSIGNED-PAYLOAD" + canonical_elements = [ method, quoted_resource, canonical_query_string, canonical_header_string, signed_headers, - "UNSIGNED-PAYLOAD", + payload, ] canonical_request = "\n".join(canonical_elements) diff --git a/tests/unit/test__signing.py b/tests/unit/test__signing.py index 2ba6fb67b..e0b642725 100644 --- a/tests/unit/test__signing.py +++ b/tests/unit/test__signing.py @@ -650,6 +650,12 @@ def test_w_custom_host_header(self): def test_w_custom_headers(self): self._generate_helper(headers={"x-goog-foo": "bar"}) + def test_w_custom_payload_hash_goog(self): + self._generate_helper(headers={"x-goog-content-sha256": "DEADBEEF"}) + + def test_w_custom_payload_hash_amz(self): + self._generate_helper(headers={"x-amz-content-sha256": "DEADBEEF"}) + def test_w_custom_query_parameters_w_string_value(self): self._generate_helper(query_parameters={"bar": "/"}) From a16c8cc8ecdb44b71b462f9b003436b873e6accd Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 15:23:47 -0500 Subject: [PATCH 5/6] fix: drop support for 'x-amz-content-sha256' Per review from @frankyn --- google/cloud/storage/_signing.py | 2 -- tests/unit/test__signing.py | 3 --- 2 files changed, 5 deletions(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index fef2f6163..cbf4e8d4d 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -592,8 +592,6 @@ def generate_signed_url_v4( if "x-goog-content-sha256" in lowercased_headers: payload = lowercased_headers["x-goog-content-sha256"] - elif "x-amz-content-sha256" in lowercased_headers: - payload = lowercased_headers["x-amz-content-sha256"] else: payload = "UNSIGNED-PAYLOAD" diff --git a/tests/unit/test__signing.py b/tests/unit/test__signing.py index e0b642725..8ac4a5190 100644 --- a/tests/unit/test__signing.py +++ b/tests/unit/test__signing.py @@ -653,9 +653,6 @@ def test_w_custom_headers(self): def test_w_custom_payload_hash_goog(self): self._generate_helper(headers={"x-goog-content-sha256": "DEADBEEF"}) - def test_w_custom_payload_hash_amz(self): - self._generate_helper(headers={"x-amz-content-sha256": "DEADBEEF"}) - def test_w_custom_query_parameters_w_string_value(self): self._generate_helper(query_parameters={"bar": "/"}) From eba4239f7f067bff09f1d39769b30a22658f4fa5 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 12 Feb 2020 16:50:35 -0500 Subject: [PATCH 6/6] fix: revert "fix: ensure URI quoting of resource path" The caller already URL-encodes the 'resource' path. and so we don't want to do it here. Reverts commit 6cc5f8ab72a1fba57a5ead2036f7d0c819950c2d. --- google/cloud/storage/_signing.py | 8 ++++---- tests/unit/test__signing.py | 7 ++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/google/cloud/storage/_signing.py b/google/cloud/storage/_signing.py index cbf4e8d4d..0da075ddb 100644 --- a/google/cloud/storage/_signing.py +++ b/google/cloud/storage/_signing.py @@ -298,6 +298,7 @@ def generate_signed_url_v2( :type resource: str :param resource: A pointer to a specific resource (typically, ``/bucket-name/path/to/blob.txt``). + Caller should have already URL-encoded the value. :type expiration: Union[Integer, datetime.datetime, datetime.timedelta] :param expiration: Point in time when the signed URL should expire. @@ -459,6 +460,7 @@ def generate_signed_url_v4( :type resource: str :param resource: A pointer to a specific resource (typically, ``/bucket-name/path/to/blob.txt``). + Caller should have already URL-encoded the value. :type expiration: Union[Integer, datetime.datetime, datetime.timedelta] :param expiration: Point in time when the signed URL should expire. @@ -586,8 +588,6 @@ def generate_signed_url_v4( ordered_query_parameters = sorted(query_parameters.items()) canonical_query_string = six.moves.urllib.parse.urlencode(ordered_query_parameters) - quoted_resource = six.moves.urllib.parse.quote(resource) - lowercased_headers = dict(ordered_headers) if "x-goog-content-sha256" in lowercased_headers: @@ -597,7 +597,7 @@ def generate_signed_url_v4( canonical_elements = [ method, - quoted_resource, + resource, canonical_query_string, canonical_header_string, signed_headers, @@ -626,7 +626,7 @@ def generate_signed_url_v4( signature = binascii.hexlify(signature_bytes).decode("ascii") return "{}{}?{}&X-Goog-Signature={}".format( - api_access_endpoint, quoted_resource, canonical_query_string, signature + api_access_endpoint, resource, canonical_query_string, signature ) diff --git a/tests/unit/test__signing.py b/tests/unit/test__signing.py index 8ac4a5190..c7231b56b 100644 --- a/tests/unit/test__signing.py +++ b/tests/unit/test__signing.py @@ -544,9 +544,9 @@ def _generate_helper( generation=None, headers=None, query_parameters=None, - resource="/name/path", ): now = datetime.datetime(2019, 2, 26, 19, 53, 27) + resource = "/name/path" signer_email = "service@example.com" credentials = _make_credentials(signer_email=signer_email) credentials.sign_bytes.return_value = b"DEADBEEF" @@ -577,7 +577,7 @@ def _generate_helper( ) self.assertEqual(scheme, expected_scheme) self.assertEqual(netloc, expected_netloc) - self.assertEqual(path, six.moves.urllib.parse.quote(resource)) + self.assertEqual(path, resource) self.assertEqual(frag, "") # Check the URL parameters. @@ -659,9 +659,6 @@ def test_w_custom_query_parameters_w_string_value(self): def test_w_custom_query_parameters_w_none_value(self): self._generate_helper(query_parameters={"qux": None}) - def test_w_non_ascii_resource(self): - self._generate_helper(resource="/name/p\xe5th") - def test_with_access_token(self): resource = "/name/path" signer_email = "service@example.com"