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.

Sign up for GitHub

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

Default to virtual style addressing #1387

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a671f1b
Decouple signature version of S3 addressing style
jamesls Feb 16, 2018
cfeaae6
Remove hardcoded 's3.amazonaws.com' for virtual hosted addressing
jamesls Feb 16, 2018
dc552fd
Remove special case govcloud handling
jamesls Feb 16, 2018
0ef645c
Account for 400 response from HeadBucket/HeadObject
jamesls Feb 16, 2018
ad05e88
Use global endpoint for s3 presigning
jamesls Feb 20, 2018
7d92ed6
Use regional endpoint for dualstack endpoints
jamesls Feb 23, 2018
61dacf4
Add changelog entry for #1387
jamesls Feb 23, 2018
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
9 changes: 7 additions & 2 deletions botocore/signers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,8 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600,
expires_in = ExpiresIn
http_method = HttpMethod
context = {
'is_presign_request': True
'is_presign_request': True,
'partition': self.meta.partition,
}

request_signer = self._request_signer
Expand Down Expand Up @@ -586,6 +587,8 @@ def generate_presigned_url(self, ClientMethod, Params=None, ExpiresIn=3600,
prepare_request_dict(
request_dict, endpoint_url=self.meta.endpoint_url, context=context)

request_dict['context']['partition'] = self.meta.partition
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crap, I meant to take this out. We don't need partition in the request context.


# Generate the presigned url.
return request_signer.generate_presigned_url(
request_dict=request_dict, expires_in=expires_in,
Expand Down Expand Up @@ -686,7 +689,9 @@ def generate_presigned_post(self, Bucket, Key, Fields=None, Conditions=None,

# 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={'is_presign_request': True, 'partition': self.meta.partition},
)

# Append that the bucket name to the list of conditions.
conditions.append({'bucket': bucket})
Expand Down
3 changes: 3 additions & 0 deletions botocore/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,9 @@ def fix_s3_host(request, signature_version, region_name,
addressing.

"""
if request.context.get('is_presign_request', False):
if request.context.get('partition', '') == 'aws':
default_endpoint_url = 's3.amazonaws.com'
try:
switch_to_virtual_host_style(
request, signature_version, default_endpoint_url)
Expand Down
116 changes: 114 additions & 2 deletions tests/functional/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import botocore.session
from botocore.config import Config
from botocore.compat import urlsplit
from botocore.exceptions import ParamValidationError
from botocore import UNSIGNED

Expand Down Expand Up @@ -349,15 +350,15 @@ def test_generate_unauthed_url(self):
'Bucket': 'foo',
'Key': 'bar'
})
self.assertEqual(url, 'https://foo.s3.us-west-2.amazonaws.com/bar')
self.assertEqual(url, 'https://foo.s3.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.us-west-2.amazonaws.com/'
'url': 'https://foo.s3.amazonaws.com/'
}
self.assertEqual(parts, expected)

Expand Down Expand Up @@ -775,6 +776,7 @@ def _verify_expected_endpoint_url(region, bucket, key, s3_config,
environ['AWS_ACCESS_KEY_ID'] = 'access_key'
environ['AWS_SECRET_ACCESS_KEY'] = 'secret_key'
environ['AWS_CONFIG_FILE'] = 'no-exist-foo'
environ['AWS_SHARED_CREDENTIALS_FILE'] = 'no-exist-foo'
session = create_session()
session.config_filename = 'no-exist-foo'
config = Config(
Expand All @@ -790,3 +792,113 @@ def _verify_expected_endpoint_url(region, bucket, key, s3_config,
Key=key, Body=b'bar')
request_sent = mock_send.call_args[0][0]
assert_equal(request_sent.url, expected_url)


def _create_s3_client(region, is_secure, endpoint_url, s3_config,
signature_version):
environ = {}
with mock.patch('os.environ', environ):
environ['AWS_ACCESS_KEY_ID'] = 'access_key'
environ['AWS_SECRET_ACCESS_KEY'] = 'secret_key'
environ['AWS_CONFIG_FILE'] = 'no-exist-foo'
environ['AWS_SHARED_CREDENTIALS_FILE'] = 'no-exist-foo'
session = create_session()
session.config_filename = 'no-exist-foo'
config = Config(
signature_version=signature_version,
s3=s3_config
)
s3 = session.create_client('s3', region_name=region, use_ssl=is_secure,
config=config,
endpoint_url=endpoint_url)
return s3


def test_addressing_for_presigned_urls():
# See TestGeneratePresigned for more detailed test cases
# on presigned URLs. Here's we're just focusing on the
# adddressing mode used for presigned URLs.
# We special case presigned URLs due to backwards
# compatibility.
t = S3AddressingCases(_verify_presigned_url_addressing)

# us-east-1, or the "global" endpoint. A signature version of
# None means the user doesn't have signature version configured.
yield t.case(region='us-east-1', bucket='bucket', key='key',
signature_version=None,
expected_url='https://bucket.s3.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-east-1', bucket='bucket', key='key',
signature_version='s3v4',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-east-1', bucket='bucket', key='key',
signature_version='s3v4',
s3_config={'addressing_style': 'path'},
expected_url='https://s3.amazonaws.com/bucket/key')

# A region that supports both 's3' and 's3v4'.
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version=None,
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version='s3v4',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version='s3v4',
s3_config={'addressing_style': 'path'},
expected_url='https://s3.us-west-2.amazonaws.com/bucket/key')

# An 's3v4' only region.
yield t.case(region='us-east-2', bucket='bucket', key='key',
signature_version=None,
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-east-2', bucket='bucket', key='key',
signature_version='s3',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-east-2', bucket='bucket', key='key',
signature_version='s3v4',
expected_url='https://bucket.s3.amazonaws.com/key')
yield t.case(region='us-east-2', bucket='bucket', key='key',
signature_version='s3v4',
s3_config={'addressing_style': 'path'},
expected_url='https://s3.us-east-2.amazonaws.com/bucket/key')

# Dualstack endpoints
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version=None,
s3_config={'use_dualstack_endpoint': True},
expected_url='https://bucket.s3.amazonaws.com/key')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm the expected url does not look like a dualstack endpoint. Should that be the case for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it looks like the existing behavior is buggy for presigned URLs and dualstack. On the develop branch this is what I'm seeing:

Region Signature URL
us-east-1 (default) https://bucket.s3.amazonaws.com/foo
us-east-1 s3 https://bucket.s3.dualstack.us-east-1.amazonaws.com/foo
us-east-1 s3v4 https://s3.dualstack.us-east-1.amazonaws.com/bucket/foo

which is clearly wrong. In testing this out, it doesn't appear that you can use bucket.s3.dualstack.amazonaws.com (i.e a regionless endpoint for dualstack) so I think I'll update this test case to just use bucket.s3.dualstack.us-east-1.amazonaws.com. Let me know if you see any issues with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah seems fine with me.


# Accelerate
yield t.case(region='us-west-2', bucket='bucket', key='key',
signature_version=None,
s3_config={'use_accelerate_endpoint': True},
expected_url='https://bucket.s3-accelerate.amazonaws.com/key')

# A region that we don't know about.
yield t.case(region='us-west-50', bucket='bucket', key='key',
signature_version=None,
expected_url='https://bucket.s3.amazonaws.com/key')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should add a test case for a user provided url?


def _verify_presigned_url_addressing(region, bucket, key, s3_config,
is_secure=True,
customer_provided_endpoint=None,
expected_url=None,
signature_version=None):
s3 = _create_s3_client(region=region, is_secure=is_secure,
endpoint_url=customer_provided_endpoint,
s3_config=s3_config,
signature_version=signature_version)
url = s3.generate_presigned_url(
'get_object', {'Bucket': bucket, 'Key': key})
# We're not trying to verify the params for URL presigning,
# those are tested elsewhere. We just care about the hostname/path.
parts = urlsplit(url)
actual = '%s://%s%s' % parts[:3]
assert_equal(actual, expected_url)
17 changes: 12 additions & 5 deletions tests/integration/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,23 +671,30 @@ def test_presign_sigv2(self):
'get_object', Params={'Bucket': self.bucket_name, 'Key': self.key})
self.assertTrue(
presigned_url.startswith(
'https://%s.s3.%s.amazonaws.com/%s' % (
self.bucket_name, self.region, self.key)),
'https://%s.s3.amazonaws.com/%s' % (
self.bucket_name, self.key)),
"Host was suppose to use DNS style, instead "
"got: %s" % presigned_url)
# Try to retrieve the object using the presigned url.
self.assertEqual(requests.get(presigned_url).content, b'foo')

def test_presign_sigv4(self):
# For a newly created bucket, you can't use virtualhosted
# addressing and 's3v4' due to the backwards compat behavior
# using '.s3.amazonaws.com' for anything in the AWS partition.
# Instead you either have to use the older 's3' signature version
# of you have to use path style addressing. The latter is being
# done here.
self.client_config.signature_version = 's3v4'
self.client_config.s3 = {'addressing_style': 'path'}
self.client = self.session.create_client(
's3', config=self.client_config)
presigned_url = self.client.generate_presigned_url(
'get_object', Params={'Bucket': self.bucket_name, 'Key': self.key})

self.assertTrue(
presigned_url.startswith(
'https://%s.s3.us-west-2.amazonaws.com/%s' % (
'https://s3.us-west-2.amazonaws.com/%s/%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 +722,7 @@ def test_presign_post_sigv2(self):
# Make sure the correct endpoint is being used
self.assertTrue(
post_args['url'].startswith(
'https://%s.s3.us-west-2.amazonaws.com' % self.bucket_name),
'https://%s.s3.amazonaws.com' % self.bucket_name),
"Host was suppose to use DNS style, instead "
"got: %s" % post_args['url'])

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

Expand Down