Repository: boto/botocore
Pull request #972 (2016-06-29 00:03:13)

Revert "Merge pull request #958 from JordonPhillips/sigv4-default"
This reverts commit ef02b9920f9ced42a4a917cdc673b834c28d8fe8, reversing
changes made to 4a2181d6ca635465243b8a9754c4aaf4c873f28f.

The reason for this revert is that S3 has an issue handling unicode characters
in headers when doing sigv4.

cc @jamesls @kyleknap


Patch:
diff --git a/.changes/next-release/feature-s3-24638.json b/.changes/next-release/feature-s3-24638.json
deleted file mode 100644
index c1da9a8a9..000000000
--- a/.changes/next-release/feature-s3-24638.json
+++ /dev/null
@@ -1,5 +0,0 @@
-{
-  "description": "All S3 requests will now use SigV4 by default so that we can take advantage of stronger security algorithms and improved access key isolation.",
-  "type": "feature",
-  "category": "s3"
-}
diff --git a/botocore/client.py b/botocore/client.py
index 6317d1392..b2febfb16 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -34,7 +34,6 @@ from botocore.utils import fix_s3_host
 from botocore.utils import get_service_module_name
 from botocore.utils import switch_to_virtual_host_style
 from botocore.utils import switch_host_s3_accelerate
-from botocore.utils import get_sigv2_if_presigning_s3_request
 from botocore.utils import S3_ACCELERATE_ENDPOINT
 from botocore.utils import S3RegionRedirector
 
@@ -179,15 +178,6 @@ class ClientCreator(object):
             return
         S3RegionRedirector(endpoint_bridge, client).register()
 
-    def _get_default_s3_region(self, service_name, endpoint_bridge):
-        # If a user is providing a custom URL, the endpoint resolver will
-        # refuse to infer a signing region. If we want to default to s3v4,
-        # we have to account for this.
-        if service_name == 's3':
-            endpoint = endpoint_bridge.resolve('s3')
-            return endpoint['signing_region'], endpoint['region_name']
-        return None, None
-
     def _get_client_args(self, service_model, region_name, is_secure,
                          endpoint_url, verify, credentials,
                          scoped_config, client_config, endpoint_bridge):
@@ -216,14 +206,8 @@ class ClientCreator(object):
             if client_config.user_agent_extra is not None:
                 user_agent += ' %s' % client_config.user_agent_extra
 
-        signing_region = endpoint_config['signing_region']
-        endpoint_region_name = endpoint_config['region_name']
-        if signing_region is None and endpoint_region_name is None:
-            signing_region, endpoint_region_name = \
-                self._get_default_s3_region(service_name, endpoint_bridge)
-
         signer = RequestSigner(
-            service_name, signing_region,
+            service_name, endpoint_config['signing_region'],
             endpoint_config['signing_name'],
             endpoint_config['signature_version'],
             credentials, event_emitter)
@@ -232,7 +216,7 @@ class ClientCreator(object):
         # on the final values. We do not want the user to be able
         # to try to modify an existing client with a client config.
         config_kwargs = dict(
-            region_name=endpoint_region_name,
+            region_name=endpoint_config['region_name'],
             signature_version=endpoint_config['signature_version'],
             user_agent=user_agent)
         if client_config is not None:
@@ -244,15 +228,11 @@ class ClientCreator(object):
         self._inject_s3_configuration(
             config_kwargs, scoped_config, client_config)
         self._conditionally_unregister_fix_s3_host(endpoint_url, event_emitter)
-        self._set_s3_presign_signature_verion(
-            service_name, endpoint_bridge.endpoint_resolver,
-            client_config, scoped_config, event_emitter, signing_region,
-            endpoint_config['metadata'])
 
         new_config = Config(**config_kwargs)
         endpoint_creator = EndpointCreator(event_emitter)
         endpoint = endpoint_creator.create_endpoint(
-            service_model, region_name=endpoint_region_name,
+            service_model, region_name=endpoint_config['region_name'],
             endpoint_url=endpoint_config['endpoint_url'], verify=verify,
             response_parser_factory=self._response_parser_factory,
             timeout=(new_config.connect_timeout, new_config.read_timeout))
@@ -268,37 +248,6 @@ class ClientCreator(object):
             'client_config': new_config
         }
 
-    def _set_s3_presign_signature_verion(
-            self, service_name, endpoint_resolver, client_config,
-            scoped_config, event_emitter, region_name, endpoint_metadata):
-        if service_name != 's3':
-            return
-
-        # This will return the manually configured signature version, or None
-        # if none was manually set. If a customer manually sets the signature
-        # version, we always want to use what they set.
-        provided_signature_version = _get_configured_signature_version(
-            service_name, client_config, scoped_config)
-        if provided_signature_version is not None:
-            return
-
-        # Check to see if the region is a region that we know about. If we
-        # don't know about a region, then we can safely assume it's a new
-        # region that is sigv4 only, since all new S3 regions only allow sigv4.
-        regions = endpoint_resolver.get_available_endpoints(
-            service_name, endpoint_metadata.get('partition'))
-        if region_name not in regions:
-            return
-
-        # If it is a region we know about, we want to default to sigv2, so here
-        # we check to see if it is available.
-        signature_versions = endpoint_metadata.get('signatureVersions')
-        if 's3' not in signature_versions:
-            return
-
-        event_emitter.register(
-            'choose-signer.s3', get_sigv2_if_presigning_s3_request)
-
     def _create_methods(self, service_model):
         op_dict = {}
         for operation_name in service_model.operation_names:
@@ -475,17 +424,32 @@ class ClientEndpointBridge(object):
         return region_name, signing_region
 
     def _resolve_signature_version(self, service_name, resolved):
-        configured_version = _get_configured_signature_version(
-            service_name, self.client_config, self.scoped_config)
-        if configured_version is not None:
-            return configured_version
-
+        # Client config overrides everything.
+        client = self.client_config
+        if client and client.signature_version is not None:
+            return client.signature_version
+        # Scoped config overrides picking from the endpoint metadata.
+        scoped = self.scoped_config
+        if scoped is not None:
+            service_config = scoped.get(service_name)
+            if service_config is not None and isinstance(service_config, dict):
+                version = service_config.get('signature_version')
+                if version:
+                    logger.debug(
+                        "Switching signature version for service %s "
+                        "to version %s based on config file override.",
+                        service_name, version)
+                    return version
         # Pick a signature version from the endpoint metadata if present.
         if 'signatureVersions' in resolved:
             potential_versions = resolved['signatureVersions']
             if service_name == 's3':
-                return 's3v4'
-            elif 'v4' in potential_versions:
+                # We currently prefer s3 over s3v4.
+                if 's3' in potential_versions:
+                    return 's3'
+                elif 's3v4' in potential_versions:
+                    return 's3v4'
+            if 'v4' in potential_versions:
                 return 'v4'
             # Now just iterate over the signature versions in order until we
             # find the first one that is known to Botocore.
@@ -496,36 +460,6 @@ class ClientEndpointBridge(object):
             signature_version=resolved.get('signatureVersions'))
 
 
-def _get_configured_signature_version(service_name, client_config,
-                                      scoped_config):
-    """
-    Gets the manually configured signature version.
-
-    :returns: the customer configured signature version, or None if no
-        signature version was configured.
-    """
-    # Client config overrides everything.
-    client = client_config
-    if client and client.signature_version is not None:
-        return client.signature_version
-
-    # Scoped config overrides picking from the endpoint metadata.
-    scoped = scoped_config
-    if scoped is not None:
-        # A given service may have service specific configuration in the
-        # config file, so we need to check there as well.
-        service_config = scoped.get(service_name)
-        if service_config is not None and isinstance(service_config, dict):
-            version = service_config.get('signature_version')
-            if version:
-                logger.debug(
-                    "Switching signature version for service %s "
-                    "to version %s based on config file override.",
-                    service_name, version)
-                return version
-    return None
-
-
 class BaseClient(object):
 
     # This is actually reassigned with the py->op_name mapping
diff --git a/botocore/utils.py b/botocore/utils.py
index 7f1f54ff5..d39836283 100644
--- a/botocore/utils.py
+++ b/botocore/utils.py
@@ -632,29 +632,6 @@ def check_dns_name(bucket_name):
     return True
 
 
-def get_sigv2_if_presigning_s3_request(signature_version, signing_name,
-                                       **kwargs):
-    """
-    Returns the 's3' (sigv2) signer if presigning an s3 request. This is
-    intended to be used to set the default signature version for the signer
-    to sigv2.
-
-    :type signature_version: str
-    :param signature_version: The current client signature version.
-
-    :type signing_name: str
-    :param signing_name: The signing name of the service.
-
-    :return: 's3' if the request is an s3 presign request, None otherwise
-    """
-    if signing_name != 's3' or signature_version is botocore.UNSIGNED:
-        return
-
-    for suffix in ['-query', '-presign-post']:
-        if signature_version.endswith(suffix):
-            return 's3' + suffix
-
-
 def fix_s3_host(request, signature_version, region_name, **kwargs):
     """
     This handler looks at S3 requests just before they are signed.


Test patch:
diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py
index 0837f4d4c..890487f25 100644
--- a/tests/functional/test_s3.py
+++ b/tests/functional/test_s3.py
@@ -446,19 +446,3 @@ class TestGeneratePresigned(BaseS3OperationTest):
             'url': 'https://foo.s3.amazonaws.com/'
         }
         self.assertEqual(parts, expected)
-
-    def test_default_presign_uses_sigv2(self):
-        url = self.client.generate_presigned_url(ClientMethod='list_buckets')
-        self.assertNotIn('Algorithm=AWS4-HMAC-SHA256', url)
-
-    def test_sigv4_presign(self):
-        config = Config(signature_version='s3v4')
-        client = self.session.create_client('s3', self.region, config=config)
-        url = client.generate_presigned_url(ClientMethod='list_buckets')
-        self.assertIn('Algorithm=AWS4-HMAC-SHA256', url)
-
-    def test_sigv2_presign(self):
-        config = Config(signature_version='s3')
-        client = self.session.create_client('s3', self.region, config=config)
-        url = client.generate_presigned_url(ClientMethod='list_buckets')
-        self.assertNotIn('Algorithm=AWS4-HMAC-SHA256', url)
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index 8ea4194a2..dce688822 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -27,12 +27,11 @@ from botocore.exceptions import InvalidS3AddressingStyleError
 from botocore.exceptions import NoRegionError
 from botocore.exceptions import UnknownEndpointError
 from botocore.exceptions import UnknownSignatureVersionError
-from botocore.config import Config
 from botocore import exceptions
 from botocore.compat import six
 
 
-class BaseClientTest(unittest.TestCase):
+class TestAutoGeneratedClient(unittest.TestCase):
     def setUp(self):
         self.service_description = {
             'metadata': {
@@ -142,8 +141,6 @@ class BaseClientTest(unittest.TestCase):
         # Missing required 'Foo' param.
         service_client.test_operation(Bar='two')
 
-
-class TestAutoGeneratedClient(BaseClientTest):
     def test_client_name(self):
         creator = self.create_client_creator()
         service_client = creator.create_client(
@@ -1325,85 +1322,6 @@ class TestAutoGeneratedClient(BaseClientTest):
         service_client.test_operation(Foo='one')
         self.assertFalse(self.endpoint.make_request.called)
 
-    def test_region_does_not_resolve_if_not_s3_and_endpoint_url_provided(self):
-        creator = self.create_client_creator()
-        with mock.patch('botocore.client.RequestSigner'):
-            service_client = creator.create_client(
-                service_name='myservice', region_name=None,
-                credentials=self.credentials, endpoint_url='https://foo')
-            self.assertIsNone(service_client.meta.region_name)
-
-
-class TestS3Client(BaseClientTest):
-    def setUp(self):
-        super(TestS3Client, self).setUp()
-        self.service_description['metadata'] = {
-            "apiVersion": "2006-03-01",
-            "endpointPrefix": "s3",
-            "globalEndpoint": "s3.amazonaws.com",
-            "protocol": "rest-xml",
-            "serviceAbbreviation": "Amazon S3",
-            "serviceFullName": "Amazon Simple Storage Service",
-            "signatureVersion": "s3"
-        }
-        self.resolver.get_available_endpoints.return_value = ['us-east-1']
-        self.endpoint = {
-            'partition': 'aws',
-            'endpointName': 'us-east-1',
-            'hostname': 's3.amazonaws.com',
-            'signatureVersions': ['s3', 's3v4'],
-            'credentialScope': {'region': 'us-east-1'}
-        }
-        self.resolver.construct_endpoint.return_value = self.endpoint
-        self.event_emitter = mock.Mock()
-
-    def assert_choose_signer_not_registered(self):
-        calls = self.event_emitter.register.call_args_list
-        for call in calls:
-            self.assertFalse(
-                call[0] == 'choose-signer' and
-                call[1] == utils.get_sigv2_if_presigning_s3_request)
-
-    def test_endpoint_url_still_resolves_region(self):
-        creator = self.create_client_creator()
-        service_client = creator.create_client(
-            service_name='s3', region_name=None,
-            credentials=self.credentials, endpoint_url='https://foo')
-        self.assertEqual(service_client.meta.region_name, 'us-east-1')
-
-    def test_presign_event_registered(self):
-        creator = self.create_client_creator(event_emitter=self.event_emitter)
-        creator.create_client(
-            service_name='s3', region_name=None,
-            credentials=self.credentials)
-        self.event_emitter.register.assert_any_call(
-            'choose-signer.s3', utils.get_sigv2_if_presigning_s3_request)
-
-    def test_presign_default_not_set_if_signature_version_configured(self):
-        creator = self.create_client_creator(event_emitter=self.event_emitter)
-        config = Config(signature_version='s3v4')
-        creator.create_client(
-            service_name='s3', region_name=None,
-            credentials=self.credentials, client_config=config)
-        self.assert_choose_signer_not_registered()
-
-    def test_presign_default_not_set_if_region_is_unknown(self):
-        self.endpoint['endpointName'] = 'eu-central-5'
-        self.endpoint['credentialScope']['region'] = 'eu-central-5'
-        creator = self.create_client_creator(event_emitter=self.event_emitter)
-        creator.create_client(
-            service_name='s3', region_name='eu-central-5',
-            credentials=self.credentials)
-        self.assert_choose_signer_not_registered()
-
-    def test_presign_default_not_set_if_region_is_sigv4_only(self):
-        self.endpoint['signatureVersions'] = ['s3v4']
-        creator = self.create_client_creator(event_emitter=self.event_emitter)
-        creator.create_client(
-            service_name='s3', region_name='eu-central-1',
-            credentials=self.credentials)
-        self.assert_choose_signer_not_registered()
-
 
 class TestConfig(unittest.TestCase):
     def test_can_use_args_to_construct(self):
@@ -1665,14 +1583,14 @@ class TestClientEndpointBridge(unittest.TestCase):
         resolved = bridge.resolve('test', 'us-west-2')
         self.assertEqual('s3', resolved['signature_version'])
 
-    def test_uses_s3v4_over_s3_for_s3_client(self):
+    def test_uses_s3_over_s3v4_for_s3(self):
         resolver = mock.Mock()
         resolver.construct_endpoint.return_value = {
             'partition': 'aws', 'hostname': 'test.com',
             'endpointName': 'us-west-2', 'signatureVersions': ['s3v4', 's3']}
         bridge = ClientEndpointBridge(resolver)
         resolved = bridge.resolve('s3', 'us-west-2')
-        self.assertEqual('s3v4', resolved['signature_version'])
+        self.assertEqual('s3', resolved['signature_version'])
 
     def test_uses_s3v4_over_others_for_s3(self):
         resolver = mock.Mock()
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index cd2cd04d3..a999093c1 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -18,7 +18,6 @@ import six
 
 import mock
 
-import botocore
 from botocore import xform_name
 from botocore.compat import OrderedDict
 from botocore.awsrequest import AWSRequest
@@ -47,7 +46,6 @@ from botocore.utils import get_service_module_name
 from botocore.utils import percent_encode_sequence
 from botocore.utils import switch_host_s3_accelerate
 from botocore.utils import S3RegionRedirector
-from botocore.utils import get_sigv2_if_presigning_s3_request
 from botocore.model import DenormalizedStructureBuilder
 from botocore.model import ShapeResolver
 
@@ -593,46 +591,6 @@ class TestFixS3Host(unittest.TestCase):
         self.assertEqual(request.url, original_url)
 
 
-class TestSwitchToS3SigV2Presigner(unittest.TestCase):
-    def test_switch_query(self):
-        signer = get_sigv2_if_presigning_s3_request('s3v4-query', 's3')
-        self.assertEqual(signer, 's3-query')
-
-        signer = get_sigv2_if_presigning_s3_request('s3-query', 's3')
-        self.assertEqual(signer, 's3-query')
-
-    def test_switch_presign_post(self):
-        signer = get_sigv2_if_presigning_s3_request('s3v4-presign-post', 's3')
-        self.assertEqual(signer, 's3-presign-post')
-
-        signer = get_sigv2_if_presigning_s3_request('s3-presign-post', 's3')
-        self.assertEqual(signer, 's3-presign-post')
-
-    def test_does_not_switch_if_not_s3(self):
-        signer = get_sigv2_if_presigning_s3_request('s3v4-query', 'sqs')
-        self.assertIsNone(signer)
-
-        signer = get_sigv2_if_presigning_s3_request('s3-presign-post', 'rds')
-        self.assertIsNone(signer)
-
-    def test_does_not_switch_if_unsigned(self):
-        signer = get_sigv2_if_presigning_s3_request(botocore.UNSIGNED, 's3')
-        self.assertIsNone(signer)
-
-    def test_does_not_switch_if_not_presign(self):
-        signer = get_sigv2_if_presigning_s3_request('s3', 's3')
-        self.assertIsNone(signer)
-
-        signer = get_sigv2_if_presigning_s3_request('s3v4', 's3')
-        self.assertIsNone(signer)
-
-        signer = get_sigv2_if_presigning_s3_request('v4', 's3')
-        self.assertIsNone(signer)
-
-        signer = get_sigv2_if_presigning_s3_request('v2', 's3')
-        self.assertIsNone(signer)
-
-
 class TestSwitchToVirtualHostStyle(unittest.TestCase):
     def test_switch_to_virtual_host_style(self):
         request = AWSRequest(