-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Prevent override of region when using endpoint_url #464
Conversation
Coveralls is wrong. Coverage should have went up. |
LGTM 🚢-it! |
region_name_override = endpoint['properties'].get( | ||
'credentialScope', {}).get('region') | ||
# provided region name if an endpoint url was not manually set. | ||
# If a endpoint url was specified, the user must specify a region. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Region is only required for sigv4, so you could previously specify an endpoint-url without a region provided you weren't using sigv4. For example, this is a breaking change for commands such as this:
$ aws configure list | grep region
region <not set> None None
$ aws s3api list-buckets --endpoint-url https://s3.amazonaws.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point so how about instead?
if endpoint_url is None and region_name is None:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that will fix the issue. In the example above endpoint_url is not None so we don't pull in the region override from the heuristics file.
d52ac03
to
68a4c9d
Compare
@jamesls |
Is there another change that's needed before the calls to us-west-2 work? I'm seeing a signing error with the latest changes:
|
2028552
to
5755430
Compare
Also touched up endpoint code.
5755430
to
57a3240
Compare
To make it clear what I did in the latest commits, here is a summary:
|
region_name, endpoint_url, verify, user_agent, | ||
event_emitter) | ||
|
||
|
||
def get_endpoint_complex(service_name, endpoint_prefix, signature_version, | ||
def get_endpoint_complex(service_name, endpoint_prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a backward-incompatible change of a public function. It's possible we might break someone using Botocore.
I think this looks good. We just need to decide if we really want to make that breaking change. 🚢-it. |
def test_regionalized_endpoints(self): | ||
|
||
sts = self.session.create_client('sts', region_name='ap-southeast-1') | ||
response = sts.get_session_token() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't call sts when you're using session credentials (say on a CI box using an IAM role), so we should put a check where we only run these tests if we have long term credentials.
@jamesls |
Prevent override of region when using endpoint_url
This ensures that user's region is not overwritten by the credentialScope if the user supplies an endpoint url. This is needed to be able to make signed calls to STS regionalized endpoints, without getting auth errors due to incorrect region . So for example, in the CLI, to make a GetSessionToken call to us-west-2 you would do:
Before, this would cause a signing error if signed with sigv4 because the credentialScope was set to
us-east-1
and that overrode the region that you provided.cc @jamesls @danielgtaylor