Skip to content

Commit

Permalink
move warning to legacy resolver
Browse files Browse the repository at this point in the history
  • Loading branch information
davidlm committed Nov 14, 2023
1 parent 9e5c01f commit 6f50a5b
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 89 deletions.
9 changes: 3 additions & 6 deletions botocore/args.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,8 @@ def get_client_args(
protocol, parameter_validation
)
response_parser = botocore.parsers.create_parser(protocol)
uses_builtin_data = endpoint_bridge.resolver_uses_builtin_data()
builtin_resolver = self._construct_builtin_resolver(
credentials, new_config, uses_builtin_data
credentials, new_config
)
ruleset_resolver = self._build_endpoint_resolver(
endpoints_ruleset_data,
Expand Down Expand Up @@ -621,15 +620,13 @@ def _ensure_boolean(self, val):
else:
return val.lower() == 'true'

def _construct_builtin_resolver(
self, credentials, client_config, uses_builtin_data
):
def _construct_builtin_resolver(self, credentials, client_config):
credential_builtin_resolver = CredentialBuiltinResolver(
credentials,
client_config.account_id_endpoint_mode,
)
resolver_map = {'credentials': credential_builtin_resolver}
return EndpointBuiltinResolver(resolver_map, uses_builtin_data)
return EndpointBuiltinResolver(resolver_map)

def _build_endpoint_resolver(
self,
Expand Down
30 changes: 9 additions & 21 deletions botocore/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -789,8 +789,17 @@ def _pick_region_values(self, resolved, region_name, endpoint_url):
and 'region' in resolved['credentialScope']
):
signing_region = resolved['credentialScope']['region']
self._maybe_warn_custom_signing_region(signing_region)
return region_name, signing_region

def _maybe_warn_custom_signing_region(self, signing_region):
if not self.resolver_uses_builtin_data():
warnings.warn(
"Detected a signing region resolved from a custom "
f"endpoints.json file: {signing_region}. This value "
"may not be honored during request signing."
)

def _resolve_signature_version(self, service_name, resolved):
configured_version = _get_configured_signature_version(
service_name, self.client_config, self.scoped_config
Expand Down Expand Up @@ -1092,34 +1101,13 @@ def _resolve_endpoint_ruleset(
request_context['auth_type'] = auth_type
if 'region' in signing_context and ignore_signing_region:
del signing_context['region']
self._maybe_warn_signing_region_mismatch(signing_context)
if 'signing' in request_context:
request_context['signing'].update(signing_context)
else:
request_context['signing'] = signing_context

return endpoint_url, additional_headers

def _maybe_warn_signing_region_mismatch(self, signing_context):
legacy_signing_region = self._request_signer.region_name
signing_region = signing_context.get('region')
if (
signing_region is not None
and legacy_signing_region != signing_region
and not self._ruleset_resolver.uses_builtin_data_path
and self._ruleset_resolver.credential_scope_set
):
warnings.warn(
"Detected an endpoint resolved from a custom endpoints.json"
"file and credentials scoped to a single region: "
f"'{signing_region}'. The signing region this file has "
"resolved does not match the signing region of the client. "
"This may cause issues with request signing.\n"
f"legacy signing region: '{legacy_signing_region}'\n"
f"current signing region: '{signing_region}'\n "
f"Using '{signing_region}' to sign the request."
)

def get_paginator(self, operation_name):
"""Create a paginator for an operation.
Expand Down
30 changes: 1 addition & 29 deletions botocore/regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -530,18 +530,8 @@ def resolve_credential_scope_builtin(
class EndpointBuiltinResolver:
"""Resolves endpoint builtins during endpoint construction."""

def __init__(self, resolver_map, uses_builtin_data_path=True):
def __init__(self, resolver_map):
self._resolver_map = resolver_map
self._uses_builtin_data_path = uses_builtin_data_path
self._credential_scope_set = False

@property
def uses_builtin_data_path(self):
return self._uses_builtin_data_path

@property
def credential_scope_set(self):
return self._credential_scope_set

def resolve(self, param_definitions, builtins):
"""Resolve endpoint builtins."""
Expand Down Expand Up @@ -573,10 +563,6 @@ def _resolve_credential_scope_builtin(self, param_definitions, builtins):
scope = credential_resolver.resolve_credential_scope_builtin(
builtin_configured, current_builtin_value
)
if scope is not None:
self._credential_scope_set = True
else:
self._credential_scope_set = False
builtins[scope_builtin_key] = scope

def _builtin_configured(self, param_definitions, param_name):
Expand Down Expand Up @@ -969,17 +955,3 @@ def ruleset_error_to_botocore_exception(self, ruleset_exception, params):
if msg == 'EndpointId must be a valid host label.':
return InvalidEndpointConfigurationError(msg=msg)
return None

@property
def uses_builtin_data_path(self):
if self._builtin_resolver is None:
return True

return self._builtin_resolver.uses_builtin_data_path

@property
def credential_scope_set(self):
if self._builtin_resolver is None:
return False

return self._builtin_resolver.credential_scope_set
26 changes: 0 additions & 26 deletions tests/functional/test_endpoint_rulesets.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import json
from functools import lru_cache
from pathlib import Path
from unittest import mock

import pytest

Expand Down Expand Up @@ -306,28 +305,3 @@ def builtin_overwriter_handler(builtins, **kwargs):
except (ClientError, ResponseParserError):
pass
assert len(http_stubber.requests) == 0


@mock.patch('botocore.signers.RequestSigner.region_name', 'us-east-1')
@mock.patch(
'botocore.regions.EndpointRulesetResolver.auth_schemes_to_signing_ctx',
return_value=('v4', {'region': 'us-west-2'}),
)
@mock.patch(
'botocore.regions.EndpointRulesetResolver.uses_builtin_data_path', False
)
@mock.patch(
'botocore.regions.EndpointRulesetResolver.credential_scope_set', True
)
def test_signing_region_mismatch_warns(auth_schemes_mock, patched_session):
patched_session.set_credentials(
access_key='foo', secret_key='bar', token='baz', scope='us-west-2'
)
client = patched_session.create_client('iam', region_name='us-west-2')
body = b'<ListRolesResponse><ListRolesResult></ListRolesResult></ListRolesResponse>'
with ClientHTTPStubber(client) as http_stubber:
http_stubber.add_response(status=200, body=body)
with pytest.warns(
UserWarning, match='does not match the signing region'
):
client.list_roles()
28 changes: 28 additions & 0 deletions tests/functional/test_regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -545,3 +545,31 @@ def test_endpoint_resolver_knows_its_datasource(patched_session, is_builtin):
with mock.patch.object(loader, 'is_builtin_path', return_value=is_builtin):
resolver = session._get_internal_component('endpoint_resolver')
assert resolver.uses_builtin_data == is_builtin


@pytest.fixture
def mock_resolved_endpoint():
return {
'credentialScope': {'region': 'us-east-2'},
'endpointName': 'us-east-2',
'signatureVersions': ['v4'],
}


@mock.patch('botocore.regions.EndpointResolver.construct_endpoint')
@pytest.mark.parametrize(
'is_builtin, num_expected_warnings', [(True, 0), (False, 1)]
)
def test_custom_endpoints_file_signing_region_warns(
construct_endpoint_mock,
is_builtin,
num_expected_warnings,
mock_resolved_endpoint,
patched_session,
recwarn,
):
construct_endpoint_mock.return_value = mock_resolved_endpoint
loader = patched_session.get_component('data_loader')
with mock.patch.object(loader, 'is_builtin_path', return_value=is_builtin):
patched_session.create_client('iam')
assert len(recwarn) == num_expected_warnings
8 changes: 1 addition & 7 deletions tests/unit/test_endpoint_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -746,34 +746,30 @@ def test_required_mode_no_account_id(


@pytest.mark.parametrize(
"builtins, credentials, scope_set, expected_url",
"builtins, credentials, expected_url",
[
# scope matches region
(
BUILTINS_WITH_UNRESOLVED_CREDENTIAL_SCOPE,
CREDENTIALS_WITH_SCOPE,
True,
URL_WITH_CREDENTIAL_SCOPE,
),
# pre-resolved scope
(
BUILTINS_WITH_RESOLVED_CREDENTIAL_SCOPE,
CREDENTIALS_WITH_SCOPE,
True,
URL_WITH_OTHER_CREDENTIAL_SCOPE,
),
# no scope in credentials
(
BUILTINS_WITH_UNRESOLVED_CREDENTIAL_SCOPE,
CREDENTIALS_NO_SCOPE,
False,
URL_NO_SCOPE,
),
# no credentials
(
BUILTINS_WITH_UNRESOLVED_CREDENTIAL_SCOPE,
None,
False,
URL_NO_SCOPE,
),
],
Expand All @@ -783,7 +779,6 @@ def test_credential_scope_builtin(
credential_scope_ruleset,
builtins,
credentials,
scope_set,
expected_url,
):
resolver = create_ruleset_resolver(
Expand All @@ -794,5 +789,4 @@ def test_credential_scope_builtin(
request_context={},
call_args={},
)
assert resolver.credential_scope_set == scope_set
assert endpoint.url == expected_url

0 comments on commit 6f50a5b

Please sign in to comment.