Repository: boto/botocore
Pull request #958 (2016-06-21 22:52:00)

Use SigV4 by default for S3
All S3 requests will now use SigV4 by default so that we can take advantage of stronger security algorithms and improved access key isolation.

cc @kyleknap @jamesls

jamesls: Flip in incorporate feedback pending failing test being fixed.
codecov-io: ## [Current coverage][cc-pull] is **97.29%**
> Merging [#958][cc-pull] into [develop][cc-base-branch] will decrease coverage by **0.02%**

```diff
@@            develop       #958   diff @@
==========================================
  Files            43         43          
  Lines          7014       7058    +44   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6826       6867    +41   
- Misses          188        191     +3   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [ab22547...fa575e8][cc-compare]
[cc-base-branch]: https://codecov.io/gh/boto/botocore/branch/develop?src=pr
[cc-compare]: https://codecov.io/gh/boto/botocore/compare/ab2254734f35b14252a7b8c3f7ddc9f18ecbdac4...fa575e8a76a38ee0afe33e250aadf9515f1f418a
[cc-pull]: https://codecov.io/gh/boto/botocore/pull/958?src=pr
JordonPhillips: Updated
JordonPhillips: To capture some offline discussion: we decided to use an event strategy to set the signer. To simplify things here, I opened a new PR to address that: #964 

I'll rebase on top of that once it gets through.
JordonPhillips: rebased on top of #964 to get the ball rolling
jamesls: Flipping to incorporating-feedback pending travis build investigation.
JordonPhillips: I'm unable to reproduce that error locally...
JordonPhillips: I got it. See #969 for more details.

Patch:
diff --git a/.changes/1.4.32.json b/.changes/1.4.32.json
deleted file mode 100644
index 5afc9d2ba..000000000
--- a/.changes/1.4.32.json
+++ /dev/null
@@ -1,12 +0,0 @@
-[
-  {
-    "category": "S3", 
-    "description": "Fixed a bug where the S3 region redirector was potentially causing a memory leak on python 2.6.", 
-    "type": "bugfix"
-  }, 
-  {
-    "category": "``s3``", 
-    "description": "Update s3 client to latest version", 
-    "type": "feature"
-  }
-]
\ No newline at end of file
diff --git a/.changes/next-release/bugfix-S3-4368.json b/.changes/next-release/bugfix-S3-4368.json
new file mode 100644
index 000000000..c501a1b9a
--- /dev/null
+++ b/.changes/next-release/bugfix-S3-4368.json
@@ -0,0 +1,5 @@
+{
+  "category": "S3",
+  "description": "Fixed a bug where the S3 region redirector was potentially causing a memory leak on python 2.6.",
+  "type": "bugfix"
+}
diff --git a/.changes/next-release/feature-s3-24638.json b/.changes/next-release/feature-s3-24638.json
new file mode 100644
index 000000000..c1da9a8a9
--- /dev/null
+++ b/.changes/next-release/feature-s3-24638.json
@@ -0,0 +1,5 @@
+{
+  "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/CHANGELOG.rst b/CHANGELOG.rst
index 1193d3162..6a734096b 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -2,13 +2,6 @@
 CHANGELOG
 =========
 
-1.4.32
-======
-
-* bugfix:S3: Fixed a bug where the S3 region redirector was potentially causing a memory leak on python 2.6.
-* feature:``s3``: Update s3 client to latest version
-
-
 1.4.31
 ======
 
diff --git a/botocore/__init__.py b/botocore/__init__.py
index b84c1b005..17fafa110 100644
--- a/botocore/__init__.py
+++ b/botocore/__init__.py
@@ -16,7 +16,7 @@ import os
 import re
 import logging
 
-__version__ = '1.4.32'
+__version__ = '1.4.31'
 
 
 class NullHandler(logging.Handler):
diff --git a/botocore/client.py b/botocore/client.py
index b2febfb16..6317d1392 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -34,6 +34,7 @@ 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
 
@@ -178,6 +179,15 @@ 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):
@@ -206,8 +216,14 @@ 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, endpoint_config['signing_region'],
+            service_name, signing_region,
             endpoint_config['signing_name'],
             endpoint_config['signature_version'],
             credentials, event_emitter)
@@ -216,7 +232,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_config['region_name'],
+            region_name=endpoint_region_name,
             signature_version=endpoint_config['signature_version'],
             user_agent=user_agent)
         if client_config is not None:
@@ -228,11 +244,15 @@ 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_config['region_name'],
+            service_model, region_name=endpoint_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))
@@ -248,6 +268,37 @@ 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:
@@ -424,32 +475,17 @@ class ClientEndpointBridge(object):
         return region_name, signing_region
 
     def _resolve_signature_version(self, service_name, resolved):
-        # 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
+        configured_version = _get_configured_signature_version(
+            service_name, self.client_config, self.scoped_config)
+        if configured_version is not None:
+            return configured_version
+
         # Pick a signature version from the endpoint metadata if present.
         if 'signatureVersions' in resolved:
             potential_versions = resolved['signatureVersions']
             if service_name == 's3':
-                # 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 's3v4'
+            elif '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.
@@ -460,6 +496,36 @@ 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/data/endpoints.json b/botocore/data/endpoints.json
index 754f7aaaa..4f21fefe2 100644
--- a/botocore/data/endpoints.json
+++ b/botocore/data/endpoints.json
@@ -34,9 +34,6 @@
         "ap-northeast-2": {
           "description": "Asia Pacific (Seoul)"
         },
-        "ap-south-1": {
-          "description": "Asia Pacific (Bombay)"
-        },
         "ap-southeast-1": {
           "description": "Asia Pacific (Singapore)"
         },
@@ -56,17 +53,7 @@
       "services": {
         "acm": {
           "endpoints": {
-            "us-east-1": {},
-            "us-west-1": {},
-            "us-west-2": {},
-            "eu-west-1": {},
-            "eu-central-1": {},
-            "ap-northeast-1": {},
-            "ap-northeast-2": {},
-            "ap-south-1": {},
-            "ap-southeast-1": {},
-            "ap-southeast-2": {},
-            "sa-east-1": {}
+            "us-east-1": {}
           }
         },
         "apigateway": {
@@ -129,7 +116,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -185,7 +171,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -205,7 +190,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "eu-west-1": {},
@@ -247,7 +231,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -294,7 +277,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -342,7 +324,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -372,7 +353,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -406,7 +386,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -421,7 +400,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -447,7 +425,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -471,7 +448,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -505,7 +481,6 @@
             "us-west-2": {},
             "eu-west-1": {},
             "eu-central-1": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "ap-northeast-1": {},
@@ -516,15 +491,10 @@
         "events": {
           "endpoints": {
             "us-east-1": {},
-            "us-west-1": {},
             "us-west-2": {},
-            "eu-west-1": {},
-            "eu-central-1": {},
-            "ap-southeast-1": {},
-            "ap-southeast-2": {},
             "ap-northeast-1": {},
-            "ap-northeast-2": {},
-            "sa-east-1": {}
+            "eu-west-1": {},
+            "eu-central-1": {}
           }
         },
         "firehose": {
@@ -555,7 +525,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-2": {},
             "eu-west-1": {},
             "eu-central-1": {}
@@ -620,7 +589,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "eu-west-1": {},
@@ -635,7 +603,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -659,7 +626,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "eu-west-1": {},
@@ -702,7 +668,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -738,7 +703,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "eu-west-1": {},
@@ -799,12 +763,6 @@
                 "s3v4"
               ]
             },
-            "ap-south-1": {
-              "hostname": "s3-ap-south-1.amazonaws.com",
-              "signatureVersions": [
-                "s3v4"
-              ]
-            },
             "ap-southeast-1": {
               "hostname": "s3-ap-southeast-1.amazonaws.com"
             },
@@ -860,7 +818,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -884,7 +841,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
@@ -980,7 +936,6 @@
             "us-west-2": {},
             "ap-northeast-1": {},
             "ap-northeast-2": {},
-            "ap-south-1": {},
             "ap-southeast-1": {},
             "ap-southeast-2": {},
             "sa-east-1": {},
diff --git a/botocore/data/s3/2006-03-01/service-2.json b/botocore/data/s3/2006-03-01/service-2.json
index c30e8bcf5..161cc02cf 100644
--- a/botocore/data/s3/2006-03-01/service-2.json
+++ b/botocore/data/s3/2006-03-01/service-2.json
@@ -827,7 +827,6 @@
         "eu-west-1",
         "us-west-1",
         "us-west-2",
-        "ap-south-1",
         "ap-southeast-1",
         "ap-southeast-2",
         "ap-northeast-1",
@@ -3594,7 +3593,7 @@
       "members":{
         "NoncurrentDays":{
           "shape":"Days",
-          "documentation":"Specifies the number of days an object is noncurrent before Amazon S3 can perform the associated action. For information about the noncurrent days calculations, see <a href=\"http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-access-control.html\">How Amazon S3 Calculates When an Object Became Noncurrent</a> in the Amazon Simple Storage Service Developer Guide."
+          "documentation":"Specifies the number of days an object is noncurrent before Amazon S3 can perform the associated action. For information about the noncurrent days calculations, see <a href=\"/AmazonS3/latest/dev/s3-access-control.html\">How Amazon S3 Calculates When an Object Became Noncurrent</a> in the Amazon Simple Storage Service Developer Guide."
         }
       },
       "documentation":"Specifies when noncurrent object versions expire. Upon expiration, Amazon S3 permanently deletes the noncurrent object versions. You set this lifecycle configuration action on a bucket that has versioning enabled (or suspended) to request that Amazon S3 delete noncurrent object versions at a specific period in the object's lifetime."
@@ -3604,7 +3603,7 @@
       "members":{
         "NoncurrentDays":{
           "shape":"Days",
-          "documentation":"Specifies the number of days an object is noncurrent before Amazon S3 can perform the associated action. For information about the noncurrent days calculations, see <a href=\"http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-access-control.html\">How Amazon S3 Calculates When an Object Became Noncurrent</a> in the Amazon Simple Storage Service Developer Guide."
+          "documentation":"Specifies the number of days an object is noncurrent before Amazon S3 can perform the associated action. For information about the noncurrent days calculations, see <a href=\"/AmazonS3/latest/dev/s3-access-control.html\">How Amazon S3 Calculates When an Object Became Noncurrent</a> in the Amazon Simple Storage Service Developer Guide."
         },
         "StorageClass":{
           "shape":"TransitionStorageClass",
diff --git a/botocore/utils.py b/botocore/utils.py
index d39836283..7f1f54ff5 100644
--- a/botocore/utils.py
+++ b/botocore/utils.py
@@ -632,6 +632,29 @@ 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.
diff --git a/docs/source/conf.py b/docs/source/conf.py
index fafe600d0..ef88e8333 100644
--- a/docs/source/conf.py
+++ b/docs/source/conf.py
@@ -54,7 +54,7 @@ copyright = u'2013, Mitch Garnaat'
 # The short X.Y version.
 version = '1.4.'
 # The full version, including alpha/beta/rc tags.
-release = '1.4.32'
+release = '1.4.31'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.


Test patch:
diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py
index 890487f25..0837f4d4c 100644
--- a/tests/functional/test_s3.py
+++ b/tests/functional/test_s3.py
@@ -446,3 +446,19 @@ 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 dce688822..8ea4194a2 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -27,11 +27,12 @@ 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 TestAutoGeneratedClient(unittest.TestCase):
+class BaseClientTest(unittest.TestCase):
     def setUp(self):
         self.service_description = {
             'metadata': {
@@ -141,6 +142,8 @@ class TestAutoGeneratedClient(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(
@@ -1322,6 +1325,85 @@ class TestAutoGeneratedClient(unittest.TestCase):
         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):
@@ -1583,14 +1665,14 @@ class TestClientEndpointBridge(unittest.TestCase):
         resolved = bridge.resolve('test', 'us-west-2')
         self.assertEqual('s3', resolved['signature_version'])
 
-    def test_uses_s3_over_s3v4_for_s3(self):
+    def test_uses_s3v4_over_s3_for_s3_client(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('s3', resolved['signature_version'])
+        self.assertEqual('s3v4', 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 a999093c1..cd2cd04d3 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -18,6 +18,7 @@ import six
 
 import mock
 
+import botocore
 from botocore import xform_name
 from botocore.compat import OrderedDict
 from botocore.awsrequest import AWSRequest
@@ -46,6 +47,7 @@ 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
 
@@ -591,6 +593,46 @@ 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(