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

Default to virtual style addressing #1387

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions botocore/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,16 +189,6 @@ def _get_s3_addressing_handler(self, endpoint_url, s3_config,
logger.debug("Defaulting to S3 virtual host style addressing with "
"path style addressing fallback.")

# For dual stack mode, we need to clear the default endpoint url in
# order to use the existing netloc if the bucket is dns compatible.
# Also, the default_endpoint_url of 's3.amazonaws.com' only works
# if we're in the 'aws' partition. Anywhere else we should
# just use the existing netloc.
if s3_config.get('use_dualstack_endpoint', False) or \
partition != 'aws':
return functools.partial(
fix_s3_host, default_endpoint_url=None)

# By default, try to use virtual style with path fallback.
return fix_s3_host

Expand Down
44 changes: 18 additions & 26 deletions botocore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@
# Based on rfc2986, section 2.3
SAFE_CHARS = '-._~'
LABEL_RE = re.compile(r'[a-z0-9][a-z0-9\-]*[a-z0-9]')
RESTRICTED_REGIONS = [
'us-gov-west-1',
'fips-us-gov-west-1',
]
RETRYABLE_HTTP_ERRORS = (requests.Timeout, requests.ConnectionError)
S3_ACCELERATE_WHITELIST = ['dualstack']

Expand Down Expand Up @@ -672,23 +668,16 @@ def check_dns_name(bucket_name):


def fix_s3_host(request, signature_version, region_name,
default_endpoint_url='s3.amazonaws.com', **kwargs):
default_endpoint_url=None, **kwargs):
"""
This handler looks at S3 requests just before they are signed.
If there is a bucket name on the path (true for everything except
ListAllBuckets) it checks to see if that bucket name conforms to
the DNS naming conventions. If it does, it alters the request to
use ``virtual hosting`` style addressing rather than ``path-style``
addressing. This allows us to avoid 301 redirects for all
bucket names that can be CNAME'd.
addressing.

"""
# By default we do not use virtual hosted style addressing when
# signed with signature version 4.
if signature_version is not botocore.UNSIGNED and \
's3v4' in signature_version:
return
elif not _allowed_region(region_name):
return
try:
switch_to_virtual_host_style(
request, signature_version, default_endpoint_url)
Expand Down Expand Up @@ -765,10 +754,6 @@ def _is_get_bucket_location_request(request):
return request.url.endswith('?location')


def _allowed_region(region_name):
return region_name not in RESTRICTED_REGIONS


def instance_cache(func):
"""Method decorator for caching method calls to a single instance.

Expand Down Expand Up @@ -904,14 +889,21 @@ def redirect_from_error(self, request_dict, response, operation, **kwargs):
error = response[1].get('Error', {})
error_code = error.get('Code')

if error_code == '301':
# A raw 301 error might be returned for several reasons, but we
# only want to try to redirect it if it's a HeadObject or
# HeadBucket because all other operations will return
# PermanentRedirect if region is incorrect.
if operation.name not in ['HeadObject', 'HeadBucket']:
return
elif error_code != 'PermanentRedirect':
# We have to account for 400 responses because
# if we sign a Head* request with the wrong region,
# we'll get a 400 Bad Request but we won't get a
# body saying it's an "AuthorizationHeaderMalformed".
is_special_head_object = (
error_code in ['301', '400'] and
operation.name in ['HeadObject', 'HeadBucket']
)
is_wrong_signing_region = (
error_code == 'AuthorizationHeaderMalformed' and
'Region' in error
)
is_permanent_redirect = error_code == 'PermanentRedirect'
if not any([is_special_head_object, is_wrong_signing_region,
is_permanent_redirect]):
return

bucket = request_dict['context']['signing']['bucket']
Expand Down
81 changes: 61 additions & 20 deletions tests/functional/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,10 @@ class TestRegionRedirect(BaseS3OperationTest):
def setUp(self):
super(TestRegionRedirect, self).setUp()
self.client = self.session.create_client(
's3', 'us-west-2', config=Config(signature_version='s3v4'))
's3', 'us-west-2', config=Config(
signature_version='s3v4',
s3={'addressing_style': 'path'},
))

self.redirect_response = mock.Mock()
self.redirect_response.headers = {
Expand All @@ -242,6 +245,23 @@ def setUp(self):
b' <Endpoint>foo.s3.eu-central-1.amazonaws.com</Endpoint>'
b'</Error>')

self.bad_signing_region_response = mock.Mock()
self.bad_signing_region_response.headers = {
'x-amz-bucket-region': 'eu-central-1'
}
self.bad_signing_region_response.status_code = 400
self.bad_signing_region_response.content = (
b'<?xml version="1.0" encoding="UTF-8"?>'
b'<Error>'
b' <Code>AuthorizationHeaderMalformed</Code>'
b' <Message>the region us-west-2 is wrong; '
b'expecting eu-central-1</Message>'
b' <Region>eu-central-1</Region>'
b' <RequestId>BD9AA1730D454E39</RequestId>'
b' <HostId></HostId>'
b'</Error>'
)

self.success_response = mock.Mock()
self.success_response.headers = {}
self.success_response.status_code = 200
Expand Down Expand Up @@ -295,6 +315,29 @@ def test_region_redirect_cache(self):
self.assertEqual(calls[1].url, fixed_url)
self.assertEqual(calls[2].url, fixed_url)

def test_resign_request_with_region_when_needed(self):
self.http_session_send_mock.side_effect = [
self.bad_signing_region_response, self.success_response,
]

# Create a client with no explicit configuration so we can
# verify the default behavior.
client = self.session.create_client(
's3', 'us-west-2')
first_response = client.list_objects(Bucket='foo')
self.assertEqual(
first_response['ResponseMetadata']['HTTPStatusCode'], 200)

self.assertEqual(self.http_session_send_mock.call_count, 2)
calls = [c[0][0] for c in self.http_session_send_mock.call_args_list]
initial_url = ('https://foo.s3.us-west-2.amazonaws.com/'
'?encoding-type=url')
self.assertEqual(calls[0].url, initial_url)

fixed_url = ('https://foo.s3.eu-central-1.amazonaws.com/'
'?encoding-type=url')
self.assertEqual(calls[1].url, fixed_url)


class TestGeneratePresigned(BaseS3OperationTest):
def test_generate_unauthed_url(self):
Expand All @@ -306,15 +349,15 @@ def test_generate_unauthed_url(self):
'Bucket': 'foo',
'Key': 'bar'
})
self.assertEqual(url, 'https://foo.s3.amazonaws.com/bar')
self.assertEqual(url, 'https://foo.s3.us-west-2.amazonaws.com/bar')

def test_generate_unauthed_post(self):
config = Config(signature_version=botocore.UNSIGNED)
client = self.session.create_client('s3', self.region, config=config)
parts = client.generate_presigned_post(Bucket='foo', Key='bar')
expected = {
'fields': {'key': 'bar'},
'url': 'https://foo.s3.amazonaws.com/'
'url': 'https://foo.s3.us-west-2.amazonaws.com/'
}
self.assertEqual(parts, expected)

Expand Down Expand Up @@ -403,42 +446,41 @@ def test_correct_url_used_for_s3():
# The default behavior for sigv2. DNS compatible buckets
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://bucket.s3.amazonaws.com/key')
expected_url='https://bucket.s3.us-west-2.amazonaws.com/key')
yield t.case(region='us-east-1', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-west-1', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://bucket.s3.amazonaws.com/key')
expected_url='https://bucket.s3.us-west-1.amazonaws.com/key')
yield t.case(region='us-west-1', bucket='bucket', key='key',
signature_version='s3', is_secure=False,
expected_url='http://bucket.s3.amazonaws.com/key')
expected_url='http://bucket.s3.us-west-1.amazonaws.com/key')

# The default behavior for sigv4. DNS compatible buckets still get path
# style addresses.
# Virtual host addressing is independent of signature version.
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version='s3v4',
expected_url=(
'https://s3.us-west-2.amazonaws.com/bucket/key'))
'https://bucket.s3.us-west-2.amazonaws.com/key'))
yield t.case(region='us-east-1', bucket='bucket', key='key',
signature_version='s3v4',
expected_url='https://s3.amazonaws.com/bucket/key')
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-west-1', bucket='bucket', key='key',
signature_version='s3v4',
expected_url=(
'https://s3.us-west-1.amazonaws.com/bucket/key'))
'https://bucket.s3.us-west-1.amazonaws.com/key'))
yield t.case(region='us-west-1', bucket='bucket', key='key',
signature_version='s3v4', is_secure=False,
expected_url=(
'http://s3.us-west-1.amazonaws.com/bucket/key'))
'http://bucket.s3.us-west-1.amazonaws.com/key'))

# Regions outside of the 'aws' partition.
# We're expecting path style because this is the default with
# 's3v4'.
# These should still default to virtual hosted addressing
# unless explicitly configured otherwise.
yield t.case(region='cn-north-1', bucket='bucket', key='key',
signature_version='s3v4',
expected_url=(
'https://s3.cn-north-1.amazonaws.com.cn/bucket/key'))
'https://bucket.s3.cn-north-1.amazonaws.com.cn/key'))
# This isn't actually supported because cn-north-1 is sigv4 only,
# but we'll still double check that our internal logic is correct
# when building the expected url.
Expand Down Expand Up @@ -522,15 +564,14 @@ def test_correct_url_used_for_s3():
s3_config=virtual_hosting,
expected_url='https://bucket.s3.us-gov-west-1.amazonaws.com/key')

# Test restricted regions not do virtual host by default
yield t.case(
region='us-gov-west-1', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://s3.us-gov-west-1.amazonaws.com/bucket/key')
expected_url='https://bucket.s3.us-gov-west-1.amazonaws.com/key')
yield t.case(
region='fips-us-gov-west-1', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://s3-fips-us-gov-west-1.amazonaws.com/bucket/key')
expected_url='https://bucket.s3-fips-us-gov-west-1.amazonaws.com/key')


# Test path style addressing.
Expand Down Expand Up @@ -608,11 +649,11 @@ def test_correct_url_used_for_s3():
yield t.case(
region='us-east-1', bucket='bucket', key='key',
s3_config=use_dualstack, signature_version='s3v4',
expected_url='https://s3.dualstack.us-east-1.amazonaws.com/bucket/key')
expected_url='https://bucket.s3.dualstack.us-east-1.amazonaws.com/key')
yield t.case(
region='us-west-2', bucket='bucket', key='key',
s3_config=use_dualstack, signature_version='s3v4',
expected_url='https://s3.dualstack.us-west-2.amazonaws.com/bucket/key')
expected_url='https://bucket.s3.dualstack.us-west-2.amazonaws.com/key')
# Non DNS compatible buckets use path style for dual stack.
yield t.case(
region='us-west-2', bucket='bucket.dot', key='key',
Expand Down
14 changes: 7 additions & 7 deletions tests/integration/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ def test_presign_sigv4(self):
'get_object', Params={'Bucket': self.bucket_name, 'Key': self.key})
self.assertTrue(
presigned_url.startswith(
'https://s3.amazonaws.com/%s/%s' % (
'https://%s.s3.amazonaws.com/%s' % (
self.bucket_name, self.key)),
"Host was suppose to be the us-east-1 endpoint, instead "
"got: %s" % presigned_url)
Expand Down Expand Up @@ -647,7 +647,7 @@ def test_presign_post_sigv4(self):
# Make sure the correct endpoint is being used
self.assertTrue(
post_args['url'].startswith(
'https://s3.amazonaws.com/%s' % self.bucket_name),
'https://%s.s3.amazonaws.com/' % self.bucket_name),
"Host was suppose to use us-east-1 endpoint, instead "
"got: %s" % post_args['url'])

Expand All @@ -671,8 +671,8 @@ def test_presign_sigv2(self):
'get_object', Params={'Bucket': self.bucket_name, 'Key': self.key})
self.assertTrue(
presigned_url.startswith(
'https://%s.s3.amazonaws.com/%s' % (
self.bucket_name, self.key)),
'https://%s.s3.%s.amazonaws.com/%s' % (
self.bucket_name, self.region, self.key)),
"Host was suppose to use DNS style, instead "
"got: %s" % presigned_url)
# Try to retrieve the object using the presigned url.
Expand All @@ -687,7 +687,7 @@ def test_presign_sigv4(self):

self.assertTrue(
presigned_url.startswith(
'https://s3.us-west-2.amazonaws.com/%s/%s' % (
'https://%s.s3.us-west-2.amazonaws.com/%s' % (
self.bucket_name, self.key)),
"Host was suppose to be the us-west-2 endpoint, instead "
"got: %s" % presigned_url)
Expand Down Expand Up @@ -715,7 +715,7 @@ def test_presign_post_sigv2(self):
# Make sure the correct endpoint is being used
self.assertTrue(
post_args['url'].startswith(
'https://%s.s3.amazonaws.com' % self.bucket_name),
'https://%s.s3.us-west-2.amazonaws.com' % self.bucket_name),
"Host was suppose to use DNS style, instead "
"got: %s" % post_args['url'])

Expand Down Expand Up @@ -748,7 +748,7 @@ def test_presign_post_sigv4(self):
# Make sure the correct endpoint is being used
self.assertTrue(
post_args['url'].startswith(
'https://s3.us-west-2.amazonaws.com/%s' % self.bucket_name),
'https://%s.s3.us-west-2.amazonaws.com/' % self.bucket_name),
"Host was suppose to use DNS style, instead "
"got: %s" % post_args['url'])

Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test_s3_addressing.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def test_list_objects_dns_name_non_classic(self):
prepared_request = self.get_prepared_request('list_objects', params,
force_hmacv1=True)
self.assertEqual(prepared_request.url,
'https://safename.s3.amazonaws.com/')
'https://safename.s3.us-west-2.amazonaws.com/')

def test_list_objects_unicode_query_string_eu_central_1(self):
self.region_name = 'eu-central-1'
Expand All @@ -81,7 +81,7 @@ def test_list_objects_unicode_query_string_eu_central_1(self):
prepared_request = self.get_prepared_request('list_objects', params)
self.assertEqual(
prepared_request.url,
('https://s3.eu-central-1.amazonaws.com/safename'
('https://safename.s3.eu-central-1.amazonaws.com/'
'?marker=%C3%A4%C3%B6%C3%BC-01.txt')
)

Expand All @@ -91,7 +91,7 @@ def test_list_objects_in_restricted_regions(self):
prepared_request = self.get_prepared_request('list_objects', params)
# Note how we keep the region specific endpoint here.
self.assertEqual(prepared_request.url,
'https://s3.us-gov-west-1.amazonaws.com/safename')
'https://safename.s3.us-gov-west-1.amazonaws.com/')

def test_list_objects_in_fips(self):
self.region_name = 'fips-us-gov-west-1'
Expand All @@ -100,7 +100,7 @@ def test_list_objects_in_fips(self):
# Note how we keep the region specific endpoint here.
self.assertEqual(
prepared_request.url,
'https://s3-fips-us-gov-west-1.amazonaws.com/safename')
'https://safename.s3-fips-us-gov-west-1.amazonaws.com/')

def test_list_objects_non_dns_name_non_classic(self):
self.region_name = 'us-west-2'
Expand Down
Loading