From 78f245ce25905becee813f6bcfbaf737a697f076 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Mon, 18 Dec 2017 13:38:57 -0800 Subject: [PATCH 1/4] Hook more events into presign functionality Specifically ensure the following: * The emission of provide-client-params event * The emission of before-parameter-build event * Registering s3 accelerate handler to before-sign so s3 accelerate presigned url's can be created --- botocore/client.py | 20 ++++++++++------- botocore/signers.py | 12 +++++++--- tests/functional/test_s3.py | 43 ++++++++++++++++++++++++++++++++++++ tests/functional/test_sts.py | 3 --- tests/unit/test_signers.py | 28 ++++++++++++++++++----- 5 files changed, 87 insertions(+), 19 deletions(-) diff --git a/botocore/client.py b/botocore/client.py index f23d0a3cee..f1fca3fdb0 100644 --- a/botocore/client.py +++ b/botocore/client.py @@ -139,7 +139,7 @@ def _register_s3_events(self, client, endpoint_bridge, endpoint_url, # Also make sure that the hostname gets switched to # s3-accelerate.amazonaws.com client.meta.events.register_first( - 'request-created.s3', switch_host_s3_accelerate) + 'before-sign.s3', switch_host_s3_accelerate) self._set_s3_presign_signature_version( client.meta, client_config, scoped_config) @@ -618,6 +618,16 @@ def _make_api_call(self, operation_name, api_params): def _convert_to_request_dict(self, api_params, operation_model, context=None): + api_params = self._emit_api_params( + api_params, operation_model, context) + request_dict = self._serializer.serialize_to_request( + api_params, operation_model) + prepare_request_dict(request_dict, endpoint_url=self._endpoint.host, + user_agent=self._client_config.user_agent, + context=context) + return request_dict + + def _emit_api_params(self, api_params, operation_model, context): # Given the API params provided by the user and the operation_model # we can serialize the request to a request_dict. operation_name = operation_model.name @@ -639,13 +649,7 @@ def _convert_to_request_dict(self, api_params, operation_model, endpoint_prefix=self._service_model.endpoint_prefix, operation_name=operation_name), params=api_params, model=operation_model, context=context) - - request_dict = self._serializer.serialize_to_request( - api_params, operation_model) - prepare_request_dict(request_dict, endpoint_url=self._endpoint.host, - user_agent=self._client_config.user_agent, - context=context) - return request_dict + return api_params def get_paginator(self, operation_name): """Create a paginator for an operation. diff --git a/botocore/signers.py b/botocore/signers.py index 64a9411d28..7b6f5afb4e 100644 --- a/botocore/signers.py +++ b/botocore/signers.py @@ -131,7 +131,9 @@ def sign(self, operation_name, request, region_name=None, 'before-sign.{0}.{1}'.format(self._service_name, operation_name), request=request, signing_name=signing_name, region_name=self._region_name, - signature_version=signature_version, request_signer=self) + signature_version=signature_version, request_signer=self, + operation_name=operation_name + ) if signature_version != botocore.UNSIGNED: kwargs = { @@ -551,8 +553,11 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600, """ client_method = ClientMethod params = Params + if params is None: + params = {} expires_in = ExpiresIn http_method = HttpMethod + context = {} request_signer = self._request_signer serializer = self._serializer @@ -565,6 +570,8 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600, operation_model = self.meta.service_model.operation_model( operation_name) + params = self._emit_api_params(params, operation_model, context) + # Create a request dict based on the params to serialize. request_dict = serializer.serialize_to_request( params, operation_model) @@ -575,8 +582,7 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600, # Prepare the request dict by including the client's endpoint url. prepare_request_dict( - request_dict, endpoint_url=self.meta.endpoint_url - ) + request_dict, endpoint_url=self.meta.endpoint_url, context=context) # Generate the presigned url. return request_signer.generate_presigned_url( diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index d0b2e2b218..84ac25b976 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -350,6 +350,49 @@ def test_presign_unsigned(self): self.assertEqual( 'https://s3.us-east-2.amazonaws.com/', url) + def test_presign_url_with_ssec(self): + config = Config(signature_version='s3') + client = self.session.create_client('s3', 'us-east-1', config=config) + url = client.generate_presigned_url( + ClientMethod='get_object', + Params={ + 'Bucket': 'mybucket', + 'Key': 'mykey', + 'SSECustomerKey': 'a' * 32, + 'SSECustomerAlgorithm': 'AES256' + } + ) + # The md5 of the sse-c key will be injected when parameters are + # built so it should show up in the presigned url as well. + self.assertIn( + 'x-amz-server-side-encryption-customer-key-md5=', url + ) + + def test_presign_s3_accelerate(self): + config = Config(signature_version=botocore.UNSIGNED, + s3={'use_accelerate_endpoint': True}) + client = self.session.create_client('s3', 'us-east-1', config=config) + url = client.generate_presigned_url( + ClientMethod='get_object', + Params={'Bucket': 'mybucket', 'Key': 'mykey'} + ) + # The url should be the accelerate endpoint + self.assertEqual( + 'https://mybucket.s3-accelerate.amazonaws.com/mykey', url) + + def test_presign_post_s3_accelerate(self): + config = Config(signature_version=botocore.UNSIGNED, + s3={'use_accelerate_endpoint': True}) + client = self.session.create_client('s3', 'us-east-1', config=config) + parts = client.generate_presigned_post( + Bucket='mybucket', Key='mykey') + # The url should be the accelerate endpoint + expected = { + 'fields': {'key': 'mykey'}, + 'url': 'https://mybucket.s3-accelerate.amazonaws.com/' + } + self.assertEqual(parts, expected) + def test_correct_url_used_for_s3(): # Test that given various sets of config options and bucket names, diff --git a/tests/functional/test_sts.py b/tests/functional/test_sts.py index d2a67b7d03..2c2ee22056 100644 --- a/tests/functional/test_sts.py +++ b/tests/functional/test_sts.py @@ -23,9 +23,6 @@ class TestSTSPresignedUrl(BaseSessionTest): def setUp(self): super(TestSTSPresignedUrl, self).setUp() self.client = self.session.create_client('sts', 'us-west-2') - # Makes sure that no requests will go through - self.stubber = Stubber(self.client) - self.stubber.activate() def test_presigned_url_contains_no_content_type(self): timestamp = datetime(2017, 3, 22, 0, 0) diff --git a/tests/unit/test_signers.py b/tests/unit/test_signers.py index 3a4db0dc68..24c3851972 100644 --- a/tests/unit/test_signers.py +++ b/tests/unit/test_signers.py @@ -22,6 +22,7 @@ from botocore.config import Config from botocore.credentials import Credentials from botocore.credentials import ReadOnlyCredentials +from botocore.hooks import HierarchicalEmitter from botocore.exceptions import NoRegionError, UnknownSignatureVersionError from botocore.exceptions import UnknownClientMethodError, ParamValidationError from botocore.exceptions import UnsupportedSignatureVersionError @@ -128,7 +129,7 @@ def test_emits_before_sign(self): 'before-sign.service_name.operation_name', request=mock.ANY, signing_name='signing_name', region_name='region_name', signature_version='v4', - request_signer=self.signer) + request_signer=self.signer, operation_name='operation_name') def test_disable_signing(self): # Returning botocore.UNSIGNED from choose-signer disables signing! @@ -727,7 +728,7 @@ def test_generate_presigned_url(self): 'query_string': {}, 'url_path': u'/mybucket/mykey', 'method': u'GET', - 'context': {}} + 'context': mock.ANY} self.generate_url_mock.assert_called_with( request_dict=ref_request_dict, expires_in=3600, operation_name='GetObject') @@ -748,7 +749,7 @@ def test_generate_presigned_url_with_query_string(self): 'query_string': {u'response-content-disposition': disposition}, 'url_path': u'/mybucket/mykey', 'method': u'GET', - 'context': {}} + 'context': mock.ANY} self.generate_url_mock.assert_called_with( request_dict=ref_request_dict, expires_in=3600, operation_name='GetObject') @@ -772,7 +773,7 @@ def test_generate_presigned_url_expires(self): 'query_string': {}, 'url_path': u'/mybucket/mykey', 'method': u'GET', - 'context': {}} + 'context': mock.ANY} self.generate_url_mock.assert_called_with( request_dict=ref_request_dict, expires_in=20, operation_name='GetObject') @@ -788,11 +789,28 @@ def test_generate_presigned_url_override_http_method(self): 'query_string': {}, 'url_path': u'/mybucket/mykey', 'method': u'PUT', - 'context': {}} + 'context': mock.ANY} self.generate_url_mock.assert_called_with( request_dict=ref_request_dict, expires_in=3600, operation_name='GetObject') + def test_generate_presigned_url_emits_param_events(self): + emitter = mock.Mock(HierarchicalEmitter) + emitter.emit.return_value = [] + self.client.meta.events = emitter + self.client.generate_presigned_url( + 'get_object', Params={'Bucket': self.bucket, 'Key': self.key}) + events_emitted = [ + emit_call[0][0] for emit_call in emitter.emit.call_args_list + ] + self.assertEqual( + events_emitted, + [ + 'provide-client-params.s3.GetObject', + 'before-parameter-build.s3.GetObject' + ] + ) + class TestGeneratePresignedPost(unittest.TestCase): def setUp(self): From 24c9024f93265ff511edf38b861b40b09fcb2058 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Mon, 18 Dec 2017 15:08:25 -0800 Subject: [PATCH 2/4] Add changelog entry --- .changes/next-release/bugfix-Presign-62691.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/bugfix-Presign-62691.json diff --git a/.changes/next-release/bugfix-Presign-62691.json b/.changes/next-release/bugfix-Presign-62691.json new file mode 100644 index 0000000000..e0930048ab --- /dev/null +++ b/.changes/next-release/bugfix-Presign-62691.json @@ -0,0 +1,5 @@ +{ + "category": "Presign", + "type": "bugfix", + "description": "Fix issue where some events were not fired during the presigning of a request thus not including a variety of customizations (`#1340 `__)" +} From 26b555d907207d8362a4f978e76f92a02a8f7e09 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Tue, 16 Jan 2018 13:19:20 -0800 Subject: [PATCH 3/4] Convert a comment from PR to code --- tests/unit/test_signers.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/test_signers.py b/tests/unit/test_signers.py index 24c3851972..367e9c3efc 100644 --- a/tests/unit/test_signers.py +++ b/tests/unit/test_signers.py @@ -728,6 +728,10 @@ def test_generate_presigned_url(self): 'query_string': {}, 'url_path': u'/mybucket/mykey', 'method': u'GET', + # mock.ANY is used because client parameter related events + # inject values into the context. So using the context's exact + # value for these tests will be a maintenance burden if + # anymore customizations are added that inject into the context. 'context': mock.ANY} self.generate_url_mock.assert_called_with( request_dict=ref_request_dict, expires_in=3600, From 27bc626d894675dc154da1d8d6cb341cbe81c69d Mon Sep 17 00:00:00 2001 From: kyleknap Date: Tue, 16 Jan 2018 15:06:37 -0800 Subject: [PATCH 4/4] Ensure presignings do not get processed by stubber --- botocore/signers.py | 4 ++- botocore/stub.py | 13 ++++++++-- tests/functional/test_sts.py | 3 +++ tests/functional/test_stub.py | 49 ++++++++++++++++++++++++++++++++--- tests/unit/test_signers.py | 16 ++++++++++++ 5 files changed, 79 insertions(+), 6 deletions(-) diff --git a/botocore/signers.py b/botocore/signers.py index 7b6f5afb4e..79ac731557 100644 --- a/botocore/signers.py +++ b/botocore/signers.py @@ -557,7 +557,9 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600, params = {} expires_in = ExpiresIn http_method = HttpMethod - context = {} + context = { + 'is_presign_request': True + } request_signer = self._request_signer serializer = self._serializer diff --git a/botocore/stub.py b/botocore/stub.py index 7b7b386a54..68f5a16756 100644 --- a/botocore/stub.py +++ b/botocore/stub.py @@ -331,12 +331,14 @@ def _assert_expected_call_order(self, model, params): operation_name=model.name, reason='Operation mismatch: found response for %s.' % name) - def _get_response_handler(self, model, params, **kwargs): + def _get_response_handler(self, model, params, context, **kwargs): self._assert_expected_call_order(model, params) # Pop off the entire response once everything has been validated return self._queue.popleft()['response'] - def _assert_expected_params(self, model, params, **kwargs): + def _assert_expected_params(self, model, params, context, **kwargs): + if self._should_not_stub(context): + return self._assert_expected_call_order(model, params) expected_params = self._queue[0]['expected_params'] if expected_params is None: @@ -357,6 +359,13 @@ def _assert_expected_params(self, model, params, **kwargs): reason='Expected parameters:\n%s,\nbut received:\n%s' % ( pformat(expected_params), pformat(params))) + def _should_not_stub(self, context): + # Do not include presign requests when processing stubbed client calls + # as a presign request will never have an HTTP request sent over the + # wire for it and therefore not receive a response back. + if context and context.get('is_presign_request'): + return True + def _validate_response(self, operation_name, service_response): service_model = self.client.meta.service_model operation_model = service_model.operation_model(operation_name) diff --git a/tests/functional/test_sts.py b/tests/functional/test_sts.py index 2c2ee22056..d2a67b7d03 100644 --- a/tests/functional/test_sts.py +++ b/tests/functional/test_sts.py @@ -23,6 +23,9 @@ class TestSTSPresignedUrl(BaseSessionTest): def setUp(self): super(TestSTSPresignedUrl, self).setUp() self.client = self.session.create_client('sts', 'us-west-2') + # Makes sure that no requests will go through + self.stubber = Stubber(self.client) + self.stubber.activate() def test_presigned_url_contains_no_content_type(self): timestamp = datetime(2017, 3, 22, 0, 0) diff --git a/tests/functional/test_stub.py b/tests/functional/test_stub.py index 48a93ac19e..f44b722b44 100644 --- a/tests/functional/test_stub.py +++ b/tests/functional/test_stub.py @@ -28,9 +28,12 @@ class TestStubber(unittest.TestCase): def setUp(self): session = botocore.session.get_session() - config = botocore.config.Config(signature_version=botocore.UNSIGNED) - self.client = session.create_client('s3', config=config) - + config = botocore.config.Config( + signature_version=botocore.UNSIGNED, + s3={'addressing_style': 'path'} + ) + self.client = session.create_client( + 's3', region_name='us-east-1', config=config) self.stubber = Stubber(self.client) def test_stubber_returns_response(self): @@ -270,3 +273,43 @@ def test_many_expected_params(self): except StubAssertionError: self.fail( "Stubber inappropriately raised error for same parameters.") + + def test_no_stub_for_presign_url(self): + try: + with self.stubber: + url = self.client.generate_presigned_url( + ClientMethod='get_object', + Params={ + 'Bucket': 'mybucket', + 'Key': 'mykey' + } + ) + self.assertEqual( + url, 'https://s3.amazonaws.com/mybucket/mykey') + except StubResponseError: + self.fail( + 'Stubbed responses should not be required for generating ' + 'presigned requests' + ) + + def test_can_stub_with_presign_url_mixed_in(self): + desired_response = {} + expected_params = { + 'Bucket': 'mybucket', + 'Prefix': 'myprefix', + } + self.stubber.add_response( + 'list_objects', desired_response, expected_params) + with self.stubber: + url = self.client.generate_presigned_url( + ClientMethod='get_object', + Params={ + 'Bucket': 'myotherbucket', + 'Key': 'myotherkey' + } + ) + self.assertEqual( + url, 'https://s3.amazonaws.com/myotherbucket/myotherkey') + actual_response = self.client.list_objects(**expected_params) + self.assertEqual(desired_response, actual_response) + self.stubber.assert_no_pending_responses() diff --git a/tests/unit/test_signers.py b/tests/unit/test_signers.py index 367e9c3efc..c0ad4d9abb 100644 --- a/tests/unit/test_signers.py +++ b/tests/unit/test_signers.py @@ -815,6 +815,22 @@ def test_generate_presigned_url_emits_param_events(self): ] ) + def test_generate_presign_url_emits_is_presign_in_context(self): + emitter = mock.Mock(HierarchicalEmitter) + emitter.emit.return_value = [] + self.client.meta.events = emitter + self.client.generate_presigned_url( + 'get_object', Params={'Bucket': self.bucket, 'Key': self.key}) + kwargs_emitted = [ + emit_call[1] for emit_call in emitter.emit.call_args_list + ] + for kwargs in kwargs_emitted: + self.assertTrue( + kwargs.get('context', {}).get('is_presign_request'), + 'The context did not have is_presign_request set to True for ' + 'the following kwargs emitted: %s' % kwargs + ) + class TestGeneratePresignedPost(unittest.TestCase): def setUp(self):