From f22ca61f9f6c59d0a6f4341dc9d994dbb4431e94 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 13:29:17 -0400 Subject: [PATCH 01/29] add `account_id` to Credentials --- botocore/credentials.py | 117 +++++++++++++++++++++++++++++++++++----- botocore/session.py | 16 +++++- 2 files changed, 118 insertions(+), 15 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 6c2d72013c..5d8d767a64 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -44,9 +44,11 @@ ) from botocore.tokens import SSOTokenProvider from botocore.utils import ( + ArnParser, ContainerMetadataFetcher, FileWebIdentityTokenLoader, InstanceMetadataFetcher, + InvalidArnException, JSONFileCache, SSOTokenLoader, parse_key_val_file, @@ -55,7 +57,9 @@ logger = logging.getLogger(__name__) ReadOnlyCredentials = namedtuple( - 'ReadOnlyCredentials', ['access_key', 'secret_key', 'token'] + 'ReadOnlyCredentials', + ['access_key', 'secret_key', 'token', 'account_id'], + defaults=(None,), ) _DEFAULT_MANDATORY_REFRESH_TIMEOUT = 10 * 60 # 10 min @@ -307,9 +311,17 @@ class Credentials: :param str token: The security token, valid only for session credentials. :param str method: A string which identifies where the credentials were found. + :param str account_id: The account ID associated with the credentials. """ - def __init__(self, access_key, secret_key, token=None, method=None): + def __init__( + self, + access_key, + secret_key, + token=None, + method=None, + account_id=None, + ): self.access_key = access_key self.secret_key = secret_key self.token = token @@ -317,6 +329,7 @@ def __init__(self, access_key, secret_key, token=None, method=None): if method is None: method = 'explicit' self.method = method + self.account_id = account_id self._normalize() @@ -332,7 +345,7 @@ def _normalize(self): def get_frozen_credentials(self): return ReadOnlyCredentials( - self.access_key, self.secret_key, self.token + self.access_key, self.secret_key, self.token, self.account_id ) @@ -349,6 +362,7 @@ class RefreshableCredentials(Credentials): :param str method: A string which identifies where the credentials were found. :param function time_fetcher: Callback function to retrieve current time. + :param str account_id: The account ID associated with the credentials. """ # The time at which we'll attempt to refresh, but not @@ -367,6 +381,7 @@ def __init__( refresh_using, method, time_fetcher=_local_now, + account_id=None, ): self._refresh_using = refresh_using self._access_key = access_key @@ -377,9 +392,10 @@ def __init__( self._refresh_lock = threading.Lock() self.method = method self._frozen_credentials = ReadOnlyCredentials( - access_key, secret_key, token + access_key, secret_key, token, account_id ) self._normalize() + self._account_id = account_id def _normalize(self): self._access_key = botocore.compat.ensure_unicode(self._access_key) @@ -394,6 +410,7 @@ def create_from_metadata(cls, metadata, refresh_using, method): expiry_time=cls._expiry_datetime(metadata['expiry_time']), method=method, refresh_using=refresh_using, + account_id=metadata.get('account_id'), ) return instance @@ -436,6 +453,19 @@ def token(self): def token(self, value): self._token = value + @property + def account_id(self): + """Warning: Using this property can lead to race conditions if you + access another property subsequently along the refresh boundary. + Please use get_frozen_credentials instead. + """ + self._refresh() + return self._account_id + + @account_id.setter + def account_id(self, value): + self._account_id = value + def _seconds_remaining(self): delta = self._expiry_time - self._time_fetcher() return total_seconds(delta) @@ -532,7 +562,7 @@ def _protected_refresh(self, is_mandatory): return self._set_from_data(metadata) self._frozen_credentials = ReadOnlyCredentials( - self._access_key, self._secret_key, self._token + self._access_key, self._secret_key, self._token, self._account_id ) if self._is_expired(): # We successfully refreshed credentials but for whatever @@ -572,6 +602,7 @@ def _set_from_data(self, data): logger.debug( "Retrieved credentials will expire at: %s", self._expiry_time ) + self.account_id = data.get('account_id') self._normalize() def get_frozen_credentials(self): @@ -628,6 +659,7 @@ def __init__(self, refresh_using, method, time_fetcher=_local_now): self._refresh_lock = threading.Lock() self.method = method self._frozen_credentials = None + self._account_id = None def refresh_needed(self, refresh_in=None): if self._frozen_credentials is None: @@ -681,6 +713,7 @@ def _get_cached_credentials(self): 'secret_key': creds['SecretAccessKey'], 'token': creds['SessionToken'], 'expiry_time': expiration, + 'account_id': creds.get('AccountId'), } def _load_from_cache(self): @@ -728,6 +761,7 @@ def __init__( self._generate_assume_role_name() super().__init__(cache, expiry_window_seconds) + self._arn_parser = ArnParser() def _generate_assume_role_name(self): self._role_session_name = 'botocore-session-%s' % (int(time.time())) @@ -756,6 +790,18 @@ def _create_cache_key(self): argument_hash = sha1(args.encode('utf-8')).hexdigest() return self._make_file_safe(argument_hash) + def _resolve_account_id(self, response): + user_arn = response.get('AssumedRoleUser', {}).get('Arn') + if user_arn is not None: + try: + account_id = self._arn_parser.parse_arn(user_arn)['account'] + except InvalidArnException: + logger.debug( + 'Unable to parse account ID from ARN: %s', user_arn + ) + else: + response['Credentials']['AccountId'] = account_id + class AssumeRoleCredentialFetcher(BaseAssumeRoleCredentialFetcher): def __init__( @@ -816,7 +862,9 @@ def _get_credentials(self): """Get credentials by calling assume role.""" kwargs = self._assume_role_kwargs() client = self._create_client() - return client.assume_role(**kwargs) + response = client.assume_role(**kwargs) + self._resolve_account_id(response) + return response def _assume_role_kwargs(self): """Get the arguments for assume role based on current configuration.""" @@ -903,7 +951,9 @@ def _get_credentials(self): # the token, explicitly configure the client to not sign requests. config = Config(signature_version=UNSIGNED) client = self._client_creator('sts', config=config) - return client.assume_role_with_web_identity(**kwargs) + response = client.assume_role_with_web_identity(**kwargs) + self._resolve_account_id(response) + return response def _assume_role_kwargs(self): """Get the arguments for assume role based on current configuration.""" @@ -987,6 +1037,7 @@ def load(self): secret_key=creds_dict['secret_key'], token=creds_dict.get('token'), method=self.METHOD, + account_id=creds_dict['account_id'], ) def _retrieve_credentials_using(self, credential_process): @@ -1017,6 +1068,7 @@ def _retrieve_credentials_using(self, credential_process): 'secret_key': parsed['SecretAccessKey'], 'token': parsed.get('SessionToken'), 'expiry_time': parsed.get('Expiration'), + 'account_id': self._resolve_account_id(parsed), } except KeyError as e: raise CredentialRetrievalError( @@ -1024,14 +1076,22 @@ def _retrieve_credentials_using(self, credential_process): error_msg=f"Missing required key in response: {e}", ) + def _resolve_account_id(self, parsed_response): + return parsed_response.get('AccountId') or self.profile_config.get( + 'aws_account_id' + ) + @property def _credential_process(self): + return self.profile_config.get('credential_process') + + @property + def profile_config(self): if self._loaded_config is None: self._loaded_config = self._load_config() - profile_config = self._loaded_config.get('profiles', {}).get( + return self._loaded_config.get('profiles', {}).get( self._profile_name, {} ) - return profile_config.get('credential_process') class InstanceMetadataProvider(CredentialProvider): @@ -1072,6 +1132,7 @@ class EnvProvider(CredentialProvider): # AWS_SESSION_TOKEN is what other AWS SDKs have standardized on. TOKENS = ['AWS_SECURITY_TOKEN', 'AWS_SESSION_TOKEN'] EXPIRY_TIME = 'AWS_CREDENTIAL_EXPIRATION' + ACCOUNT_ID = 'AWS_ACCOUNT_ID' def __init__(self, environ=None, mapping=None): """ @@ -1081,8 +1142,8 @@ def __init__(self, environ=None, mapping=None): :param mapping: An optional mapping of variable names to environment variable names. Use this if you want to change the mapping of access_key->AWS_ACCESS_KEY_ID, etc. - The dict can have up to 3 keys: ``access_key``, ``secret_key``, - ``session_token``. + The dict can have up to 5 keys: ``access_key``, ``secret_key``, + ``token``, ``expiry_time``, and ``account_id``. """ if environ is None: environ = os.environ @@ -1098,6 +1159,7 @@ def _build_mapping(self, mapping): var_mapping['secret_key'] = self.SECRET_KEY var_mapping['token'] = self.TOKENS var_mapping['expiry_time'] = self.EXPIRY_TIME + var_mapping['account_id'] = self.ACCOUNT_ID else: var_mapping['access_key'] = mapping.get( 'access_key', self.ACCESS_KEY @@ -1111,6 +1173,9 @@ def _build_mapping(self, mapping): var_mapping['expiry_time'] = mapping.get( 'expiry_time', self.EXPIRY_TIME ) + var_mapping['account_id'] = mapping.get( + 'account_id', self.ACCOUNT_ID + ) return var_mapping def load(self): @@ -1135,6 +1200,7 @@ def load(self): expiry_time, refresh_using=fetcher, method=self.METHOD, + account_id=credentials['account_id'], ) return Credentials( @@ -1142,6 +1208,7 @@ def load(self): credentials['secret_key'], credentials['token'], method=self.METHOD, + account_id=credentials['account_id'], ) else: return None @@ -1184,6 +1251,11 @@ def fetch_credentials(require_expiry=True): provider=method, cred_var=mapping['expiry_time'] ) + credentials['account_id'] = None + account_id = environ.get(mapping['account_id'], '') + if account_id: + credentials['account_id'] = account_id + return credentials return fetch_credentials @@ -1234,6 +1306,7 @@ class SharedCredentialProvider(CredentialProvider): # aws_security_token, but the SDKs are standardizing on aws_session_token # so we support both. TOKENS = ['aws_security_token', 'aws_session_token'] + ACCOUNT_ID = 'aws_account_id' def __init__(self, creds_filename, profile_name=None, ini_parser=None): self._creds_filename = creds_filename @@ -1260,8 +1333,13 @@ def load(self): config, self.ACCESS_KEY, self.SECRET_KEY ) token = self._get_session_token(config) + account_id = self._get_account_id(config) return Credentials( - access_key, secret_key, token, method=self.METHOD + access_key, + secret_key, + token, + method=self.METHOD, + account_id=account_id, ) def _get_session_token(self, config): @@ -1269,6 +1347,9 @@ def _get_session_token(self, config): if token_envvar in config: return config[token_envvar] + def _get_account_id(self, config): + return config.get(self.ACCOUNT_ID) + class ConfigProvider(CredentialProvider): """INI based config provider with profile sections.""" @@ -1282,6 +1363,7 @@ class ConfigProvider(CredentialProvider): # aws_security_token, but the SDKs are standardizing on aws_session_token # so we support both. TOKENS = ['aws_security_token', 'aws_session_token'] + ACCOUNT_ID = 'aws_account_id' def __init__(self, config_filename, profile_name, config_parser=None): """ @@ -1318,8 +1400,13 @@ def load(self): profile_config, self.ACCESS_KEY, self.SECRET_KEY ) token = self._get_session_token(profile_config) + account_id = self._get_account_id(profile_config) return Credentials( - access_key, secret_key, token, method=self.METHOD + access_key, + secret_key, + token, + method=self.METHOD, + account_id=account_id, ) else: return None @@ -1329,6 +1416,9 @@ def _get_session_token(self, profile_config): if token_name in profile_config: return profile_config[token_name] + def _get_account_id(self, profile_config): + return profile_config.get(self.ACCOUNT_ID) + class BotoProvider(CredentialProvider): METHOD = 'boto-config' @@ -2134,6 +2224,7 @@ def _get_credentials(self): 'SecretAccessKey': credentials['secretAccessKey'], 'SessionToken': credentials['sessionToken'], 'Expiration': self._parse_timestamp(credentials['expiration']), + 'AccountId': self._account_id, }, } return credentials diff --git a/botocore/session.py b/botocore/session.py index 0739286ec6..581d1e47fa 100644 --- a/botocore/session.py +++ b/botocore/session.py @@ -479,7 +479,9 @@ def set_default_client_config(self, client_config): """ self._client_config = client_config - def set_credentials(self, access_key, secret_key, token=None): + def set_credentials( + self, access_key, secret_key, token=None, account_id=None + ): """ Manually create credentials for this session. If you would prefer to use botocore without a config file, environment variables, @@ -495,9 +497,13 @@ def set_credentials(self, access_key, secret_key, token=None): :type token: str :param token: An option session token used by STS session credentials. + + :type account_id: str + :param account_id: An optional account ID associated with the + credentials. """ self._credentials = botocore.credentials.Credentials( - access_key, secret_key, token + access_key, secret_key, token, account_id=account_id ) def get_credentials(self): @@ -841,6 +847,7 @@ def create_client( aws_secret_access_key=None, aws_session_token=None, config=None, + aws_account_id=None, ): """Create a botocore client. @@ -910,6 +917,10 @@ def create_client( :rtype: botocore.client.BaseClient :return: A botocore client instance + :type aws_account_id: string + :param aws_account_id: The AWS account ID to use when creating the client. + Same semantics as aws_access_key_id above. + """ default_client_config = self.get_default_client_config() # If a config is provided and a default config is set, then @@ -945,6 +956,7 @@ def create_client( access_key=aws_access_key_id, secret_key=aws_secret_access_key, token=aws_session_token, + account_id=aws_account_id, ) elif self._missing_cred_vars(aws_access_key_id, aws_secret_access_key): raise PartialCredentialsError( From bc166c00e9474504a1dc65b68968a750c0798e43 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 13:30:41 -0400 Subject: [PATCH 02/29] unit tests for `account_id` in credentials --- tests/functional/test_credentials.py | 3 +- tests/integration/test_credentials.py | 10 +- tests/unit/test_credentials.py | 251 ++++++++++++++++++++++++++ tests/unit/test_session.py | 17 ++ 4 files changed, 278 insertions(+), 3 deletions(-) diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index 073e97d07c..5981d1cedc 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -198,7 +198,7 @@ def create_assume_role_response(self, credentials, expiration=None): }, 'AssumedRoleUser': { 'AssumedRoleId': 'myroleid', - 'Arn': 'arn:aws:iam::1234567890:user/myuser', + 'Arn': f'arn:aws:iam::{credentials.account_id}:user/myuser', }, } @@ -209,6 +209,7 @@ def create_random_credentials(self): 'fake-%s' % random_chars(15), 'fake-%s' % random_chars(35), 'fake-%s' % random_chars(45), + account_id='fake-%s' % random_chars(12), ) def assert_creds_equal(self, c1, c2): diff --git a/tests/integration/test_credentials.py b/tests/integration/test_credentials.py index 239c206a70..3d0f12c366 100644 --- a/tests/integration/test_credentials.py +++ b/tests/integration/test_credentials.py @@ -72,7 +72,10 @@ def test_access_secret_vs_profile_code(self, credentials_cls): ) credentials_cls.assert_called_with( - access_key='code', secret_key='code-secret', token=mock.ANY + access_key='code', + secret_key='code-secret', + token=mock.ANY, + account_id=mock.ANY, ) def test_profile_env_vs_code(self): @@ -97,7 +100,10 @@ def test_access_secret_env_vs_code(self, credentials_cls): ) credentials_cls.assert_called_with( - access_key='code', secret_key='code-secret', token=mock.ANY + access_key='code', + secret_key='code-secret', + token=mock.ANY, + account_id=mock.ANY, ) def test_access_secret_env_vs_profile_code(self): diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 4301b6446f..06f891911c 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -190,6 +190,14 @@ def test_refresh_returns_partial_credentials(self): with self.assertRaises(botocore.exceptions.CredentialRetrievalError): self.creds.access_key + def test_account_id_refresh(self): + metadata = self.metadata.copy() + metadata['account_id'] = '123456789012' + self.refresher.return_value = metadata + self.mock_time.return_value = datetime.now(tzlocal()) + self.assertTrue(self.creds.refresh_needed()) + self.assertEqual(self.creds.account_id, '123456789012') + class TestDeferredRefreshableCredentials(unittest.TestCase): def setUp(self): @@ -257,6 +265,7 @@ def get_expected_creds_from_response(self, response): 'secret_key': response['Credentials']['SecretAccessKey'], 'token': response['Credentials']['SessionToken'], 'expiry_time': expiration, + 'account_id': None, } def some_future_time(self): @@ -688,6 +697,49 @@ def test_mfa_refresh_enabled(self): ] self.assertEqual(calls, expected_calls) + def test_account_id(self): + response = { + 'Credentials': { + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': self.some_future_time().isoformat(), + }, + 'AssumedRoleUser': { + 'AssumedRoleId': 'ARO123EXAMPLE123:myrole', + 'Arn': 'arn:aws:sts::123456789012:assumed-role/myrole', + }, + } + client_creator = self.create_client_creator(with_response=response) + refresher = credentials.AssumeRoleCredentialFetcher( + client_creator, self.source_creds, self.role_arn + ) + expected_response = self.get_expected_creds_from_response(response) + expected_response['account_id'] = '123456789012' + response = refresher.fetch_credentials() + self.assertEqual(response, expected_response) + + def test_account_id_invalid_arn(self): + response = { + 'Credentials': { + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': self.some_future_time().isoformat(), + }, + 'AssumedRoleUser': { + 'AssumedRoleId': 'ARO123EXAMPLE123:myrole', + 'Arn': 'foo', + }, + } + client_creator = self.create_client_creator(with_response=response) + refresher = credentials.AssumeRoleCredentialFetcher( + client_creator, self.source_creds, self.role_arn + ) + expected_response = self.get_expected_creds_from_response(response) + response = refresher.fetch_credentials() + self.assertEqual(response, expected_response) + class TestAssumeRoleWithWebIdentityCredentialFetcher(BaseEnvVar): def setUp(self): @@ -720,6 +772,7 @@ def get_expected_creds_from_response(self, response): 'secret_key': response['Credentials']['SecretAccessKey'], 'token': response['Credentials']['SessionToken'], 'expiry_time': expiration, + 'account_id': None, } def test_no_cache(self): @@ -795,6 +848,53 @@ def test_assume_role_in_cache_but_expired(self): self.assertEqual(response, expected) + def test_account_id(self): + response = { + 'Credentials': { + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': self.some_future_time().isoformat(), + }, + 'AssumedRoleUser': { + 'AssumedRoleId': 'ARO123EXAMPLE123:myrole', + 'Arn': 'arn:aws:sts::123456789012:assumed-role/myrole', + }, + } + client_creator = self.create_client_creator(with_response=response) + refresher = credentials.AssumeRoleWithWebIdentityCredentialFetcher( + client_creator, + self.load_token, + self.role_arn, + ) + expected_response = self.get_expected_creds_from_response(response) + expected_response['account_id'] = '123456789012' + response = refresher.fetch_credentials() + self.assertEqual(response, expected_response) + + def test_account_id_invalid_arn(self): + response = { + 'Credentials': { + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': self.some_future_time().isoformat(), + }, + 'AssumedRoleUser': { + 'AssumedRoleId': 'ARO123EXAMPLE123:myrole', + 'Arn': 'foo', + }, + } + client_creator = self.create_client_creator(with_response=response) + refresher = credentials.AssumeRoleWithWebIdentityCredentialFetcher( + client_creator, + self.load_token, + self.role_arn, + ) + expected_response = self.get_expected_creds_from_response(response) + response = refresher.fetch_credentials() + self.assertEqual(response, expected_response) + class TestAssumeRoleWithWebIdentityCredentialProvider(unittest.TestCase): def setUp(self): @@ -1012,6 +1112,22 @@ def test_envvars_found_with_session_token(self): self.assertEqual(creds.token, 'baz') self.assertEqual(creds.method, 'env') + def test_envvars_found_with_account_id(self): + environ = { + 'AWS_ACCESS_KEY_ID': 'foo', + 'AWS_SECRET_ACCESS_KEY': 'bar', + 'AWS_SESSION_TOKEN': 'baz', + 'AWS_ACCOUNT_ID': '123456789012', + } + provider = credentials.EnvProvider(environ) + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.account_id, '123456789012') + self.assertEqual(creds.method, 'env') + def test_envvars_not_found(self): provider = credentials.EnvProvider(environ={}) creds = provider.load() @@ -1119,6 +1235,22 @@ def test_can_override_expiry_env_var_mapping(self): with self.assertRaisesRegex(RuntimeError, error_message): creds.get_frozen_credentials() + def test_can_override_account_id_env_var_mapping(self): + environ = { + 'AWS_ACCESS_KEY_ID': 'foo', + 'AWS_SECRET_ACCESS_KEY': 'bar', + 'AWS_SESSION_TOKEN': 'baz', + 'FOO_ACCOUNT_ID': '123456789012', + } + provider = credentials.EnvProvider( + environ, {'account_id': 'FOO_ACCOUNT_ID'} + ) + creds = provider.load() + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.account_id, '123456789012') + def test_partial_creds_is_an_error(self): # If the user provides an access key, they must also # provide a secret key. Not doing so will generate an @@ -1390,6 +1522,28 @@ def test_credentials_file_does_not_exist_returns_none(self): creds = provider.load() self.assertIsNone(creds) + def test_credentials_file_exists_with_account_id(self): + self.ini_parser.return_value = { + 'default': { + 'aws_access_key_id': 'foo', + 'aws_secret_access_key': 'bar', + 'aws_session_token': 'baz', + 'aws_account_id': '123456789012', + } + } + provider = credentials.SharedCredentialProvider( + creds_filename='~/.aws/creds', + profile_name='default', + ini_parser=self.ini_parser, + ) + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.method, 'shared-credentials-file') + self.assertEqual(creds.account_id, '123456789012') + class TestConfigFileProvider(BaseEnvVar): def setUp(self): @@ -1452,6 +1606,24 @@ def test_partial_creds_is_error(self): with self.assertRaises(botocore.exceptions.PartialCredentialsError): provider.load() + def test_config_account_id(self): + profile_config = { + 'aws_access_key_id': 'a', + 'aws_secret_access_key': 'b', + 'aws_session_token': 'c', + 'aws_account_id': '123456789012', + } + parsed = {'profiles': {'default': profile_config}} + parser = mock.Mock() + parser.return_value = parsed + provider = credentials.ConfigProvider('cli.cfg', 'default', parser) + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'a') + self.assertEqual(creds.secret_key, 'b') + self.assertEqual(creds.token, 'c') + self.assertEqual(creds.account_id, '123456789012') + class TestBotoProvider(BaseEnvVar): def setUp(self): @@ -3407,6 +3579,82 @@ def test_missing_expiration_and_session_token(self): self.assertIsNone(creds.token) self.assertEqual(creds.method, 'custom-process') + def test_missing_account_id(self): + self.loaded_config['profiles'] = { + 'default': {'credential_process': 'my-process'} + } + self._set_process_return_value( + { + 'Version': 1, + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': '2999-01-01T00:00:00Z', + # Missing AccountId. + } + ) + + provider = self.create_process_provider() + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.method, 'custom-process') + self.assertIsNone(creds.account_id) + + def test_account_id_from_profile(self): + self.loaded_config['profiles'] = { + 'default': { + 'credential_process': 'my-process', + 'aws_account_id': '1234567890', + } + } + self._set_process_return_value( + { + 'Version': 1, + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': '2999-01-01T00:00:00Z', + # Missing AccountId. + } + ) + provider = self.create_process_provider() + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.method, 'custom-process') + self.assertEqual(creds.account_id, '1234567890') + + def test_account_id_from_process_takes_precedence(self): + self.loaded_config['profiles'] = { + 'default': { + 'credential_process': 'my-process', + 'aws_account_id': '1234567890', + } + } + self._set_process_return_value( + { + 'Version': 1, + 'AccessKeyId': 'foo', + 'SecretAccessKey': 'bar', + 'SessionToken': 'baz', + 'Expiration': '2999-01-01T00:00:00Z', + 'AccountId': '0987654321', + } + ) + provider = self.create_process_provider() + creds = provider.load() + self.assertIsNotNone(creds) + self.assertEqual(creds.access_key, 'foo') + self.assertEqual(creds.secret_key, 'bar') + self.assertEqual(creds.token, 'baz') + self.assertEqual(creds.method, 'custom-process') + self.assertEqual(creds.account_id, '0987654321') + class TestProfileProviderBuilder(unittest.TestCase): def setUp(self): @@ -3494,6 +3742,7 @@ def test_can_fetch_credentials(self): 'SecretAccessKey': 'bar', 'SessionToken': 'baz', 'Expiration': '2008-09-23T12:43:20Z', + 'AccountId': '1234567890', }, } self.assertEqual(self.cache[cache_key], expected_cached_credentials) @@ -3587,6 +3836,7 @@ def test_load_sso_credentials_without_cache(self): self.assertEqual(credentials.access_key, 'foo') self.assertEqual(credentials.secret_key, 'bar') self.assertEqual(credentials.token, 'baz') + self.assertEqual(credentials.account_id, '1234567890') def test_load_sso_credentials_with_cache(self): cached_creds = { @@ -3620,6 +3870,7 @@ def test_load_sso_credentials_with_cache_expired(self): self.assertEqual(credentials.access_key, 'foo') self.assertEqual(credentials.secret_key, 'bar') self.assertEqual(credentials.token, 'baz') + self.assertEqual(credentials.account_id, '1234567890') def test_required_config_not_set(self): del self.config['sso_start_url'] diff --git a/tests/unit/test_session.py b/tests/unit/test_session.py index ef2495ebcf..5f10283717 100644 --- a/tests/unit/test_session.py +++ b/tests/unit/test_session.py @@ -549,6 +549,23 @@ def test_cred_provider_called_when_partial_creds_provided(self): aws_secret_access_key='foo', ) + @mock.patch('botocore.client.ClientCreator') + def test_credential_params(self, client_creator): + self.session.create_client( + 'sts', + 'us-west-2', + aws_access_key_id='foo', + aws_secret_access_key='bar', + aws_session_token='baz', + aws_account_id='123456789012', + ) + call_args = client_creator.return_value.create_client.call_args[1] + credentials = call_args['credentials'] + self.assertEqual(credentials.access_key, 'foo') + self.assertEqual(credentials.secret_key, 'bar') + self.assertEqual(credentials.token, 'baz') + self.assertEqual(credentials.account_id, '123456789012') + def test_cred_provider_not_called_on_unsigned_client(self): cred_provider = mock.Mock() self.session.register_component('credential_provider', cred_provider) From 958dc8eb61ed971ce370fe503b8f3d511184c5ac Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 13:34:32 -0400 Subject: [PATCH 03/29] add `account_id_endpoint_mode` config setting to endpoint resolution --- botocore/args.py | 18 +++++++++ botocore/config.py | 4 ++ botocore/configprovider.py | 6 +++ botocore/exceptions.py | 4 ++ botocore/regions.py | 78 ++++++++++++++++++++++++++++++++++++++ botocore/session.py | 10 +---- 6 files changed, 112 insertions(+), 8 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index dbbcbe8a99..1941bfbeef 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -165,6 +165,7 @@ def get_client_args( is_secure, endpoint_bridge, event_emitter, + credentials, ) # Copy the session's user agent factory and adds client configuration. @@ -267,11 +268,15 @@ def compute_client_args( client_config.disable_request_compression ), client_context_params=client_config.client_context_params, + account_id_endpoint_mode=( + client_config.account_id_endpoint_mode + ), ) self._compute_retry_config(config_kwargs) self._compute_connect_timeout(config_kwargs) self._compute_user_agent_appid_config(config_kwargs) self._compute_request_compression_config(config_kwargs) + self._compute_account_id_endpoint_mode(config_kwargs) s3_config = self.compute_s3_config(client_config) is_s3_service = self._is_s3_service(service_name) @@ -598,6 +603,14 @@ def _validate_min_compression_size(self, min_size): return min_size + def _compute_account_id_endpoint_mode(self, config_kwargs): + ep_mode = config_kwargs.get('account_id_endpoint_mode') + if ep_mode is None: + ep_mode = self._config_store.get_config_variable( + 'account_id_endpoint_mode' + ) + config_kwargs['account_id_endpoint_mode'] = ep_mode + def _ensure_boolean(self, val): if isinstance(val, bool): return val @@ -617,6 +630,7 @@ def _build_endpoint_resolver( is_secure, endpoint_bridge, event_emitter, + credentials, ): if endpoints_ruleset_data is None: return None @@ -666,6 +680,7 @@ def _build_endpoint_resolver( event_emitter=event_emitter, use_ssl=is_secure, requested_auth_scheme=sig_version, + credentials=credentials, ) def compute_endpoint_resolver_builtin_defaults( @@ -750,6 +765,9 @@ def compute_endpoint_resolver_builtin_defaults( 's3_disable_multiregion_access_points', False ), EPRBuiltins.SDK_ENDPOINT: given_endpoint, + # account ID is calculated later if account based routing is + # enabled and configured for the service + EPRBuiltins.AWS_ACCOUNT_ID: None, } def _compute_user_agent_appid_config(self, config_kwargs): diff --git a/botocore/config.py b/botocore/config.py index 5ae7521530..96f868d723 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -227,6 +227,9 @@ class Config: the ``Client Context Parameters`` section of the service client's documentation. Invalid parameters or ones that are not used by the specified service will be ignored. + :type account_id_endpoint_mode: str + :param account_id_endpoint_mode: Enables or disables account ID based + endpoint routing for supported operations. Defaults to None. """ @@ -257,6 +260,7 @@ class Config: ('request_min_compression_size_bytes', None), ('disable_request_compression', None), ('client_context_params', None), + ('account_id_endpoint_mode', None), ] ) diff --git a/botocore/configprovider.py b/botocore/configprovider.py index f335f7e379..e33e5e063f 100644 --- a/botocore/configprovider.py +++ b/botocore/configprovider.py @@ -161,6 +161,12 @@ False, utils.ensure_boolean, ), + 'account_id_endpoint_mode': ( + 'account_id_endpoint_mode', + 'AWS_ACCOUNT_ID_ENDPOINT_MODE', + 'preferred', + None, + ), } # A mapping for the s3 specific configuration vars. These are the configuration # vars that typically go in the s3 section of the config file. This mapping diff --git a/botocore/exceptions.py b/botocore/exceptions.py index 1c480abbf8..e934433bc6 100644 --- a/botocore/exceptions.py +++ b/botocore/exceptions.py @@ -814,3 +814,7 @@ class EndpointResolutionError(EndpointProviderError): class UnknownEndpointResolutionBuiltInName(EndpointProviderError): fmt = 'Unknown builtin variable name: {name}' + + +class AccountIDNotFound(EndpointResolutionError): + fmt = '`account_id_endpoint_mode is set to `required` but no account ID was found.' diff --git a/botocore/regions.py b/botocore/regions.py index 0fe8f0ee0e..52ba3b3bac 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -26,8 +26,10 @@ from botocore.crt import CRT_SUPPORTED_AUTH_TYPES from botocore.endpoint_provider import EndpointProvider from botocore.exceptions import ( + AccountIDNotFound, EndpointProviderError, EndpointVariantError, + InvalidConfigError, InvalidEndpointConfigurationError, InvalidHostLabelError, MissingDependencyException, @@ -46,6 +48,12 @@ LOG = logging.getLogger(__name__) DEFAULT_URI_TEMPLATE = '{service}.{region}.{dnsSuffix}' # noqa DEFAULT_SERVICE_DATA = {'endpoints': {}} +# Allowed values for the ``account_id_endpoint_mode`` config field. +VALID_ACCOUNT_ID_ENDPOINT_MODES = [ + 'preferred', + 'disabled', + 'required', +] class BaseEndpointResolver: @@ -450,6 +458,8 @@ class EndpointResolverBuiltins(str, Enum): AWS_S3_DISABLE_MRAP = "AWS::S3::DisableMultiRegionAccessPoints" # Whether a custom endpoint has been configured (str) SDK_ENDPOINT = "SDK::Endpoint" + # An account ID sourced from the credential resolution process. + AWS_ACCOUNT_ID = "AWS::Auth::AccountId" class EndpointRulesetResolver: @@ -465,6 +475,7 @@ def __init__( event_emitter, use_ssl=True, requested_auth_scheme=None, + credentials=None, ): self._provider = EndpointProvider( ruleset_data=endpoint_ruleset_data, @@ -478,6 +489,7 @@ def __init__( self._use_ssl = use_ssl self._requested_auth_scheme = requested_auth_scheme self._instance_cache = {} + self._credentials = credentials def construct_endpoint( self, @@ -546,6 +558,7 @@ def _get_provider_params( customized_builtins = self._get_customized_builtins( operation_model, call_args, request_context ) + self._resolve_account_id_builtin(request_context, customized_builtins) for param_name, param_def in self._param_definitions.items(): param_val = self._resolve_param_from_context( param_name=param_name, @@ -562,6 +575,71 @@ def _get_provider_params( return provider_params + def _resolve_account_id_builtin(self, request_context, builtins): + """Resolve the ``AWS::Auth::AccountId`` builtin if configured in the + ruleset, account ID based routing is enabled and it has not already + been resolved in a custom handler. + """ + if 'AccountId' in self._param_definitions: + act_id_ep_mode = self._resolve_account_id_endpoint_mode( + request_context + ) + act_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID + if act_id_ep_mode == 'disabled': + # if account ID has been set with a custom handler, but the mode + # is disabled, we must unset it so it won't be passed to the + # endpoint provider. + builtins[act_id_builtin_key] = None + return + + act_id_builtin = builtins.get(act_id_builtin_key) + if act_id_builtin is None: + self._do_resolve_account_id_builtin(act_id_ep_mode, builtins) + + def _resolve_account_id_endpoint_mode(self, request_context): + """Resolve the account ID endpoint mode for the request. Account ID + based routing is always disabled for presigned and unsigned requests. + Otherwise, the mode is determined by the ``account_id_endpoint_mode`` + config setting. + """ + not_presign = not request_context.get('is_presign_request', False) + should_sign = self._requested_auth_scheme != UNSIGNED + creds_available = self._credentials is not None + if all((not_presign, should_sign, creds_available)): + config = request_context['client_config'] + act_id_ep_mode = config.account_id_endpoint_mode + return self._validate_account_id_endpoint_mode(act_id_ep_mode) + return 'disabled' + + def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): + if account_id_endpoint_mode not in VALID_ACCOUNT_ID_ENDPOINT_MODES: + valid_modes_str = ', '.join(VALID_ACCOUNT_ID_ENDPOINT_MODES) + error_msg = ( + f"Invalid value '{account_id_endpoint_mode}' for " + "account_id_endpoint_mode. Valid values are: " + f"{valid_modes_str}." + ) + raise InvalidConfigError(error_msg=error_msg) + return account_id_endpoint_mode + + def _do_resolve_account_id_builtin( + self, account_id_endpoint_mode, builtins + ): + # This will make a call to resolve credentials if they are not already + # or need to be refreshed. + frozen_creds = self._credentials.get_frozen_credentials() + account_id = frozen_creds.account_id + if account_id is None: + if account_id_endpoint_mode == 'preferred': + LOG.debug( + '`account_id_endpoint_mode` is set to `preferred`, but no ' + 'account ID was found.' + ) + elif account_id_endpoint_mode == 'required': + raise AccountIDNotFound() + else: + builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id + def _resolve_param_from_context( self, param_name, operation_model, call_args ): diff --git a/botocore/session.py b/botocore/session.py index 581d1e47fa..cdb5a3feaf 100644 --- a/botocore/session.py +++ b/botocore/session.py @@ -479,9 +479,7 @@ def set_default_client_config(self, client_config): """ self._client_config = client_config - def set_credentials( - self, access_key, secret_key, token=None, account_id=None - ): + def set_credentials(self, access_key, secret_key, token=None): """ Manually create credentials for this session. If you would prefer to use botocore without a config file, environment variables, @@ -497,13 +495,9 @@ def set_credentials( :type token: str :param token: An option session token used by STS session credentials. - - :type account_id: str - :param account_id: An optional account ID associated with the - credentials. """ self._credentials = botocore.credentials.Credentials( - access_key, secret_key, token, account_id=account_id + access_key, secret_key, token ) def get_credentials(self): From aa604a3c130f5ada4f237062a0dbb9ecb9f450dc Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 13:34:50 -0400 Subject: [PATCH 04/29] unit tests for `account_id_endpoint_mode` --- tests/functional/test_credentials.py | 3 +- .../endpoints/valid-rules/aws-account-id.json | 83 ++++++ tests/unit/test_args.py | 25 ++ tests/unit/test_endpoint_provider.py | 239 +++++++++++++++++- 4 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 tests/unit/data/endpoints/valid-rules/aws-account-id.json diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index 5981d1cedc..6c05d940b0 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -790,7 +790,8 @@ def assert_session_credentials(self, expected_params, **kwargs): expected_creds = self.create_random_credentials() response = self.create_assume_role_response(expected_creds) session = StubbedSession(**kwargs) - stubber = session.stub('sts') + config = Config(signature_version=UNSIGNED) + stubber = session.stub('sts', config=config) stubber.add_response( 'assume_role_with_web_identity', response, expected_params ) diff --git a/tests/unit/data/endpoints/valid-rules/aws-account-id.json b/tests/unit/data/endpoints/valid-rules/aws-account-id.json new file mode 100644 index 0000000000..debea100cd --- /dev/null +++ b/tests/unit/data/endpoints/valid-rules/aws-account-id.json @@ -0,0 +1,83 @@ +{ + "parameters": { + "Region": { + "type": "string", + "builtIn": "AWS::Region", + "documentation": "The region to dispatch this request, eg. `us-east-1`." + }, + "AccountId": { + "type": "string", + "builtIn": "AWS::Auth::AccountId", + "documentation": "The account ID to dispatch this request, eg. `123456789012`." + } + }, + "rules": [ + { + "documentation": "Template the account ID into the URI when account ID is set", + "conditions": [ + { + "fn": "isSet", + "argv": [ + { + "ref": "AccountId" + } + ] + }, + { + "fn": "isSet", + "argv": [ + { + "ref": "Region" + } + ] + } + ], + "endpoint": { + "url": "https://{AccountId}.amazonaws.com", + "properties": { + "authSchemes": [ + { + "name": "sigv4", + "signingName": "serviceName", + "signingRegion": "{Region}" + } + ] + } + }, + "type": "endpoint" + }, + { + "documentation": "Fallback when account ID isn't set", + "conditions": [ + { + "fn": "isSet", + "argv": [ + { + "ref": "Region" + } + ] + } + ], + "endpoint": { + "url": "https://amazonaws.com", + "properties": { + "authSchemes": [ + { + "name": "sigv4", + "signingName": "serviceName", + "signingRegion": "{Region}" + } + ] + } + }, + "type": "endpoint" + }, + { + "documentation": "fallback when region is unset", + "conditions": [], + "error": "Region must be set to resolve a valid endpoint", + "type": "error" + } + ], + "version": "1.3" +} diff --git a/tests/unit/test_args.py b/tests/unit/test_args.py index 8f1c992422..4d90c0d17d 100644 --- a/tests/unit/test_args.py +++ b/tests/unit/test_args.py @@ -614,6 +614,30 @@ def test_bad_value_disable_request_compression(self): config = client_args['client_config'] self.assertFalse(config.disable_request_compression) + def test_account_id_endpoint_mode_config_store(self): + self.config_store.set_config_variable( + 'account_id_endpoint_mode', 'preferred' + ) + config = self.call_get_client_args()['client_config'] + self.assertEqual(config.account_id_endpoint_mode, 'preferred') + + def test_account_id_endpoint_mode_client_config(self): + config = Config(account_id_endpoint_mode='preferred') + config = self.call_get_client_args(client_config=config) + client_config = config['client_config'] + self.assertEqual(client_config.account_id_endpoint_mode, 'preferred') + + def test_account_id_endpoint_mode_client_config_overrides_config_store( + self, + ): + self.config_store.set_config_variable( + 'account_id_endpoint_mode', 'preferred' + ) + config = Config(account_id_endpoint_mode='required') + config = self.call_get_client_args(client_config=config) + client_config = config['client_config'] + self.assertEqual(client_config.account_id_endpoint_mode, 'required') + class TestEndpointResolverBuiltins(unittest.TestCase): def setUp(self): @@ -679,6 +703,7 @@ def test_builtins_defaults(self): bins['AWS::S3::DisableMultiRegionAccessPoints'], False ) self.assertEqual(bins['SDK::Endpoint'], None) + self.assertEqual(bins['AWS::Auth::AccountId'], None) def test_aws_region(self): bins = self.call_compute_endpoint_resolver_builtin_defaults( diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 51a07079bc..0d8e7d8c27 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -18,6 +18,9 @@ import pytest +from botocore import UNSIGNED +from botocore.config import Config +from botocore.credentials import Credentials from botocore.endpoint_provider import ( EndpointProvider, EndpointRule, @@ -28,12 +31,14 @@ TreeRule, ) from botocore.exceptions import ( + AccountIDNotFound, EndpointResolutionError, + InvalidConfigError, MissingDependencyException, UnknownSignatureVersionError, ) from botocore.loaders import Loader -from botocore.regions import EndpointRulesetResolver +from botocore.regions import EndpointResolverBuiltins, EndpointRulesetResolver from tests import requires_crt REGION_TEMPLATE = "{Region}" @@ -98,8 +103,7 @@ def rule_lib(partitions): return RuleSetStandardLibrary(partitions) -@pytest.fixture(scope="module") -def ruleset_dict(): +def _ruleset_dict(): path = os.path.join( os.path.dirname(__file__), "data", @@ -111,6 +115,11 @@ def ruleset_dict(): return json.load(f) +@pytest.fixture(scope="module") +def ruleset_dict(): + return _ruleset_dict() + + @pytest.fixture(scope="module") def endpoint_provider(ruleset_dict, partitions): return EndpointProvider(ruleset_dict, partitions) @@ -511,3 +520,227 @@ def test_aws_is_virtual_hostable_s3_bucket_allow_subdomains( rule_lib.aws_is_virtual_hostable_s3_bucket(bucket, True) == expected_value ) + + +def _account_id_ruleset(): + rule_path = os.path.join( + os.path.dirname(__file__), + "data", + "endpoints", + "valid-rules", + "aws-account-id.json", + ) + with open(rule_path) as f: + return json.load(f) + + +@pytest.fixture +def operation_model_empty_context_params(): + operation_model = Mock() + operation_model.static_context_parameters = [] + operation_model.context_parameters = [] + return operation_model + + +ACCOUNT_ID_RULESET = _account_id_ruleset() +BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID = { + EndpointResolverBuiltins.AWS_REGION: "us-west-2", + EndpointResolverBuiltins.AWS_ACCOUNT_ID: None, +} +BUILTINS_WITH_RESOLVED_ACCOUNT_ID = { + EndpointResolverBuiltins.AWS_REGION: "us-west-2", + EndpointResolverBuiltins.AWS_ACCOUNT_ID: "0987654321", +} +STATIC_CREDENTIALS = Credentials( + access_key="access_key", + secret_key="secret_key", + token="token", + account_id="1234567890", +) + + +def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): + service_model = Mock() + service_model.client_context_parameters = [] + return EndpointRulesetResolver( + endpoint_ruleset_data=ruleset, + partition_data={}, + service_model=service_model, + builtins=bulitins, + client_context=None, + event_emitter=Mock(), + use_ssl=True, + credentials=credentials, + requested_auth_scheme=auth_scheme, + ) + + +ACT_ID_REQUIRED_CONTEXT = { + "client_config": Config(account_id_endpoint_mode="required") +} +ACT_ID_PREFERRED_CONTEXT = { + "client_config": Config(account_id_endpoint_mode="preferred") +} +ACT_ID_DISABLED_CONTEXT = { + "client_config": Config(account_id_endpoint_mode="disabled") +} + +URL_NO_ACCOUNT_ID = "https://amazonaws.com" +URL_WITH_ACCOUNT_ID = "https://1234567890.amazonaws.com" + + +@pytest.mark.parametrize( + "ruleset, builtins, credentials, auth_scheme, request_context, expected_url", + [ + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_REQUIRED_CONTEXT, + URL_WITH_ACCOUNT_ID, + ), + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_PREFERRED_CONTEXT, + URL_WITH_ACCOUNT_ID, + ), + # custom account ID takes precedence over credentials + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_RESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_REQUIRED_CONTEXT, + "https://0987654321.amazonaws.com", + ), + # no account ID builtin in ruleset + ( + _ruleset_dict(), + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_REQUIRED_CONTEXT, + "https://us-west-2.amazonaws.com", + ), + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_DISABLED_CONTEXT, + URL_NO_ACCOUNT_ID, + ), + # custom account ID removed if account ID mode is disabled + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_RESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + ACT_ID_DISABLED_CONTEXT, + URL_NO_ACCOUNT_ID, + ), + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + UNSIGNED, + ACT_ID_REQUIRED_CONTEXT, + URL_NO_ACCOUNT_ID, + ), + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + STATIC_CREDENTIALS, + None, + {**ACT_ID_REQUIRED_CONTEXT, "is_presign_request": True}, + URL_NO_ACCOUNT_ID, + ), + # no credentials + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + None, + None, + ACT_ID_PREFERRED_CONTEXT, + URL_NO_ACCOUNT_ID, + ), + # no account ID in credentials + ( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + Credentials(access_key="foo", secret_key="bar", token="baz"), + None, + ACT_ID_PREFERRED_CONTEXT, + URL_NO_ACCOUNT_ID, + ), + ], +) +def test_account_id_builtin( + operation_model_empty_context_params, + ruleset, + builtins, + credentials, + auth_scheme, + request_context, + expected_url, +): + resolver = create_ruleset_resolver( + ruleset, builtins, credentials, auth_scheme + ) + endpoint = resolver.construct_endpoint( + operation_model=operation_model_empty_context_params, + request_context=request_context, + call_args={}, + ) + assert endpoint.url == expected_url + + +@pytest.mark.parametrize( + "credentials, auth_scheme, request_context, expected_error", + [ + # invalid value for mode + ( + STATIC_CREDENTIALS, + None, + {"client_config": Config(account_id_endpoint_mode="foo")}, + InvalidConfigError, + ), + # mode is case sensitive + ( + STATIC_CREDENTIALS, + None, + {"client_config": Config(account_id_endpoint_mode="PREFERRED")}, + InvalidConfigError, + ), + # no account ID found but required + ( + Credentials(access_key="foo", secret_key="bar", token="baz"), + None, + ACT_ID_REQUIRED_CONTEXT, + AccountIDNotFound, + ), + ], +) +def test_account_id_error_cases( + operation_model_empty_context_params, + credentials, + auth_scheme, + request_context, + expected_error, +): + resolver = create_ruleset_resolver( + ACCOUNT_ID_RULESET, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + credentials, + auth_scheme, + ) + with pytest.raises(expected_error): + resolver.construct_endpoint( + operation_model=operation_model_empty_context_params, + request_context=request_context, + call_args={}, + ) From 78f4a37e69dd4675b45651b2875a9ef4d6debcd7 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 15:54:30 -0400 Subject: [PATCH 05/29] shorten test name --- tests/unit/test_args.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/test_args.py b/tests/unit/test_args.py index 4d90c0d17d..e80057db8a 100644 --- a/tests/unit/test_args.py +++ b/tests/unit/test_args.py @@ -627,9 +627,7 @@ def test_account_id_endpoint_mode_client_config(self): client_config = config['client_config'] self.assertEqual(client_config.account_id_endpoint_mode, 'preferred') - def test_account_id_endpoint_mode_client_config_overrides_config_store( - self, - ): + def test_acct_id_ep_mode_client_cfg_overrides_cfg_store(self): self.config_store.set_config_variable( 'account_id_endpoint_mode', 'preferred' ) From e4188baf19f75580dbe85c55385539fa4dcfba83 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 4 Oct 2023 17:20:27 -0400 Subject: [PATCH 06/29] add `account_id` to set_credentials --- botocore/session.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/botocore/session.py b/botocore/session.py index cdb5a3feaf..79a19aebe9 100644 --- a/botocore/session.py +++ b/botocore/session.py @@ -479,7 +479,9 @@ def set_default_client_config(self, client_config): """ self._client_config = client_config - def set_credentials(self, access_key, secret_key, token=None): + def set_credentials( + self, access_key, secret_key, token=None, account_id=None + ): """ Manually create credentials for this session. If you would prefer to use botocore without a config file, environment variables, @@ -495,9 +497,12 @@ def set_credentials(self, access_key, secret_key, token=None): :type token: str :param token: An option session token used by STS session credentials. + + :type account_id: str + :param account_id: The account ID part of the credentials. """ self._credentials = botocore.credentials.Credentials( - access_key, secret_key, token + access_key, secret_key, token, account_id=account_id ) def get_credentials(self): From c79dc035136afc0196378f5ac19f097da9872409 Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 5 Oct 2023 09:43:54 -0400 Subject: [PATCH 07/29] pr feedback --- botocore/config.py | 14 ++++++++++++-- botocore/exceptions.py | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/botocore/config.py b/botocore/config.py index 96f868d723..c2605fa471 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -230,8 +230,18 @@ class Config: :type account_id_endpoint_mode: str :param account_id_endpoint_mode: Enables or disables account ID based endpoint routing for supported operations. - - Defaults to None. + Valid options are: + + * ``preferred`` -- Attempt to resolve account ID during endpoint + resolution if supported by the service. If account ID cannot be + resolved, fallback to a different endpoint. + * ``required`` -- Require account ID to be resolved during endpoint + resolution. If account ID cannot be resolved, raises + ``AccountIDNotFound`` exception. + * ``disabled`` -- Disable account ID based endpoint routing. The SDK + will not attempt to resolve account ID during endpoint resolution. + + If not specified, the default behavior is ``preferred``. """ OPTION_DEFAULTS = OrderedDict( diff --git a/botocore/exceptions.py b/botocore/exceptions.py index e934433bc6..5fcd4a7e23 100644 --- a/botocore/exceptions.py +++ b/botocore/exceptions.py @@ -817,4 +817,4 @@ class UnknownEndpointResolutionBuiltInName(EndpointProviderError): class AccountIDNotFound(EndpointResolutionError): - fmt = '`account_id_endpoint_mode is set to `required` but no account ID was found.' + fmt = '"account_id_endpoint_mode" is set to "required" but no account ID was found.' From 9bc1a2e21bb3f7310f5ba2c6ba3da67f28cf142a Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 5 Oct 2023 15:52:33 -0400 Subject: [PATCH 08/29] clean up config docs --- botocore/config.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/botocore/config.py b/botocore/config.py index c2605fa471..7bd0d05b72 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -228,18 +228,14 @@ class Config: documentation. Invalid parameters or ones that are not used by the specified service will be ignored. :type account_id_endpoint_mode: str - :param account_id_endpoint_mode: Enables or disables account ID based - endpoint routing for supported operations. + :param account_id_endpoint_mode: Set account ID based endpoint behavior + for supported operations. Valid options are: - * ``preferred`` -- Attempt to resolve account ID during endpoint - resolution if supported by the service. If account ID cannot be - resolved, fallback to a different endpoint. - * ``required`` -- Require account ID to be resolved during endpoint - resolution. If account ID cannot be resolved, raises - ``AccountIDNotFound`` exception. - * ``disabled`` -- Disable account ID based endpoint routing. The SDK - will not attempt to resolve account ID during endpoint resolution. + * ``preferred`` -- Use account ID based endpoint routing if available. + * ``required`` -- Raise ``AccountIDNotFound`` exception if account ID + based endpoint routing is unavailable. + * ``disabled`` -- Disable account ID based endpoint routing. If not specified, the default behavior is ``preferred``. """ From dd4d7c39ebac921b4dd94f0760851c94c3bb011e Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 6 Oct 2023 13:29:22 -0400 Subject: [PATCH 09/29] pr feedback --- botocore/config.py | 2 +- botocore/credentials.py | 5 +-- botocore/exceptions.py | 2 +- botocore/regions.py | 52 +++++++++++----------------- botocore/session.py | 2 +- tests/unit/test_args.py | 22 ------------ tests/unit/test_endpoint_provider.py | 4 +-- 7 files changed, 27 insertions(+), 62 deletions(-) diff --git a/botocore/config.py b/botocore/config.py index 7bd0d05b72..892027e7a4 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -233,7 +233,7 @@ class Config: Valid options are: * ``preferred`` -- Use account ID based endpoint routing if available. - * ``required`` -- Raise ``AccountIDNotFound`` exception if account ID + * ``required`` -- Raise ``AccountIdNotFound`` exception if account ID based endpoint routing is unavailable. * ``disabled`` -- Disable account ID based endpoint routing. diff --git a/botocore/credentials.py b/botocore/credentials.py index 5d8d767a64..627a66c195 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -1251,10 +1251,7 @@ def fetch_credentials(require_expiry=True): provider=method, cred_var=mapping['expiry_time'] ) - credentials['account_id'] = None - account_id = environ.get(mapping['account_id'], '') - if account_id: - credentials['account_id'] = account_id + credentials['account_id'] = environ.get(mapping['account_id']) return credentials diff --git a/botocore/exceptions.py b/botocore/exceptions.py index 5fcd4a7e23..8506971c31 100644 --- a/botocore/exceptions.py +++ b/botocore/exceptions.py @@ -816,5 +816,5 @@ class UnknownEndpointResolutionBuiltInName(EndpointProviderError): fmt = 'Unknown builtin variable name: {name}' -class AccountIDNotFound(EndpointResolutionError): +class AccountIdNotFound(EndpointResolutionError): fmt = '"account_id_endpoint_mode" is set to "required" but no account ID was found.' diff --git a/botocore/regions.py b/botocore/regions.py index 52ba3b3bac..827853e0a7 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -26,7 +26,7 @@ from botocore.crt import CRT_SUPPORTED_AUTH_TYPES from botocore.endpoint_provider import EndpointProvider from botocore.exceptions import ( - AccountIDNotFound, + AccountIdNotFound, EndpointProviderError, EndpointVariantError, InvalidConfigError, @@ -49,11 +49,11 @@ DEFAULT_URI_TEMPLATE = '{service}.{region}.{dnsSuffix}' # noqa DEFAULT_SERVICE_DATA = {'endpoints': {}} # Allowed values for the ``account_id_endpoint_mode`` config field. -VALID_ACCOUNT_ID_ENDPOINT_MODES = [ +VALID_ACCOUNT_ID_ENDPOINT_MODES = ( 'preferred', 'disabled', 'required', -] +) class BaseEndpointResolver: @@ -580,44 +580,36 @@ def _resolve_account_id_builtin(self, request_context, builtins): ruleset, account ID based routing is enabled and it has not already been resolved in a custom handler. """ - if 'AccountId' in self._param_definitions: - act_id_ep_mode = self._resolve_account_id_endpoint_mode( - request_context - ) - act_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - if act_id_ep_mode == 'disabled': - # if account ID has been set with a custom handler, but the mode - # is disabled, we must unset it so it won't be passed to the - # endpoint provider. - builtins[act_id_builtin_key] = None - return - - act_id_builtin = builtins.get(act_id_builtin_key) - if act_id_builtin is None: - self._do_resolve_account_id_builtin(act_id_ep_mode, builtins) + if 'AccountId' not in self._param_definitions: + return + + acct_id_ep_mode = self._resolve_account_id_endpoint_mode( + request_context + ) + acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID + acct_id_builtin = builtins.get(acct_id_builtin_key) + if acct_id_ep_mode == 'disabled': + # Unset the account ID if endpoint mode is disabled. + builtins[acct_id_builtin_key] = None + elif acct_id_builtin is None: + self._do_resolve_account_id_builtin(acct_id_ep_mode, builtins) def _resolve_account_id_endpoint_mode(self, request_context): - """Resolve the account ID endpoint mode for the request. Account ID - based routing is always disabled for presigned and unsigned requests. - Otherwise, the mode is determined by the ``account_id_endpoint_mode`` - config setting. - """ + """Resolve the account ID endpoint mode for the request.""" not_presign = not request_context.get('is_presign_request', False) should_sign = self._requested_auth_scheme != UNSIGNED creds_available = self._credentials is not None if all((not_presign, should_sign, creds_available)): - config = request_context['client_config'] - act_id_ep_mode = config.account_id_endpoint_mode - return self._validate_account_id_endpoint_mode(act_id_ep_mode) + ep_mode = request_context['client_config'].account_id_endpoint_mode + return self._validate_account_id_endpoint_mode(ep_mode) return 'disabled' def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): if account_id_endpoint_mode not in VALID_ACCOUNT_ID_ENDPOINT_MODES: - valid_modes_str = ', '.join(VALID_ACCOUNT_ID_ENDPOINT_MODES) error_msg = ( f"Invalid value '{account_id_endpoint_mode}' for " "account_id_endpoint_mode. Valid values are: " - f"{valid_modes_str}." + f"{', '.join(VALID_ACCOUNT_ID_ENDPOINT_MODES)}." ) raise InvalidConfigError(error_msg=error_msg) return account_id_endpoint_mode @@ -625,8 +617,6 @@ def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): def _do_resolve_account_id_builtin( self, account_id_endpoint_mode, builtins ): - # This will make a call to resolve credentials if they are not already - # or need to be refreshed. frozen_creds = self._credentials.get_frozen_credentials() account_id = frozen_creds.account_id if account_id is None: @@ -636,7 +626,7 @@ def _do_resolve_account_id_builtin( 'account ID was found.' ) elif account_id_endpoint_mode == 'required': - raise AccountIDNotFound() + raise AccountIdNotFound() else: builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id diff --git a/botocore/session.py b/botocore/session.py index 79a19aebe9..13e116ca68 100644 --- a/botocore/session.py +++ b/botocore/session.py @@ -499,7 +499,7 @@ def set_credentials( credentials. :type account_id: str - :param account_id: The account ID part of the credentials. + :param account_id: The account ID for the credentials. """ self._credentials = botocore.credentials.Credentials( access_key, secret_key, token, account_id=account_id diff --git a/tests/unit/test_args.py b/tests/unit/test_args.py index e80057db8a..c84aa6ee2d 100644 --- a/tests/unit/test_args.py +++ b/tests/unit/test_args.py @@ -614,28 +614,6 @@ def test_bad_value_disable_request_compression(self): config = client_args['client_config'] self.assertFalse(config.disable_request_compression) - def test_account_id_endpoint_mode_config_store(self): - self.config_store.set_config_variable( - 'account_id_endpoint_mode', 'preferred' - ) - config = self.call_get_client_args()['client_config'] - self.assertEqual(config.account_id_endpoint_mode, 'preferred') - - def test_account_id_endpoint_mode_client_config(self): - config = Config(account_id_endpoint_mode='preferred') - config = self.call_get_client_args(client_config=config) - client_config = config['client_config'] - self.assertEqual(client_config.account_id_endpoint_mode, 'preferred') - - def test_acct_id_ep_mode_client_cfg_overrides_cfg_store(self): - self.config_store.set_config_variable( - 'account_id_endpoint_mode', 'preferred' - ) - config = Config(account_id_endpoint_mode='required') - config = self.call_get_client_args(client_config=config) - client_config = config['client_config'] - self.assertEqual(client_config.account_id_endpoint_mode, 'required') - class TestEndpointResolverBuiltins(unittest.TestCase): def setUp(self): diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 0d8e7d8c27..eeacba36bd 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -31,7 +31,7 @@ TreeRule, ) from botocore.exceptions import ( - AccountIDNotFound, + AccountIdNotFound, EndpointResolutionError, InvalidConfigError, MissingDependencyException, @@ -721,7 +721,7 @@ def test_account_id_builtin( Credentials(access_key="foo", secret_key="bar", token="baz"), None, ACT_ID_REQUIRED_CONTEXT, - AccountIDNotFound, + AccountIdNotFound, ), ], ) From 109dfbaeef9ebb48ce79cdb53a3e06c8d8dcca1f Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 6 Oct 2023 15:03:36 -0400 Subject: [PATCH 10/29] only include account_id in credential fetchers if successfully parsed --- botocore/credentials.py | 25 +++++++++++++++++-------- tests/functional/test_credentials.py | 3 +-- tests/unit/test_credentials.py | 3 --- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 627a66c195..04c88eef7a 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -708,13 +708,16 @@ def _get_cached_credentials(self): creds = response['Credentials'] expiration = _serialize_if_needed(creds['Expiration'], iso=True) - return { + creds_dict = { 'access_key': creds['AccessKeyId'], 'secret_key': creds['SecretAccessKey'], 'token': creds['SessionToken'], 'expiry_time': expiration, - 'account_id': creds.get('AccountId'), } + account_id = creds.get('AccountId') + if account_id is not None: + creds_dict['account_id'] = account_id + return creds_dict def _load_from_cache(self): if self._cache_key in self._cache: @@ -1037,7 +1040,7 @@ def load(self): secret_key=creds_dict['secret_key'], token=creds_dict.get('token'), method=self.METHOD, - account_id=creds_dict['account_id'], + account_id=creds_dict.get('account_id'), ) def _retrieve_credentials_using(self, credential_process): @@ -1063,12 +1066,11 @@ def _retrieve_credentials_using(self, credential_process): ), ) try: - return { + creds_dict = { 'access_key': parsed['AccessKeyId'], 'secret_key': parsed['SecretAccessKey'], 'token': parsed.get('SessionToken'), 'expiry_time': parsed.get('Expiration'), - 'account_id': self._resolve_account_id(parsed), } except KeyError as e: raise CredentialRetrievalError( @@ -1076,6 +1078,11 @@ def _retrieve_credentials_using(self, credential_process): error_msg=f"Missing required key in response: {e}", ) + account_id = self._resolve_account_id(parsed) + if account_id is not None: + creds_dict['account_id'] = account_id + return creds_dict + def _resolve_account_id(self, parsed_response): return parsed_response.get('AccountId') or self.profile_config.get( 'aws_account_id' @@ -1200,7 +1207,7 @@ def load(self): expiry_time, refresh_using=fetcher, method=self.METHOD, - account_id=credentials['account_id'], + account_id=credentials.get('account_id'), ) return Credentials( @@ -1208,7 +1215,7 @@ def load(self): credentials['secret_key'], credentials['token'], method=self.METHOD, - account_id=credentials['account_id'], + account_id=credentials.get('account_id'), ) else: return None @@ -1251,7 +1258,9 @@ def fetch_credentials(require_expiry=True): provider=method, cred_var=mapping['expiry_time'] ) - credentials['account_id'] = environ.get(mapping['account_id']) + account_id = environ.get(mapping['account_id']) + if account_id is not None: + credentials['account_id'] = account_id return credentials diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index 6c05d940b0..5981d1cedc 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -790,8 +790,7 @@ def assert_session_credentials(self, expected_params, **kwargs): expected_creds = self.create_random_credentials() response = self.create_assume_role_response(expected_creds) session = StubbedSession(**kwargs) - config = Config(signature_version=UNSIGNED) - stubber = session.stub('sts', config=config) + stubber = session.stub('sts') stubber.add_response( 'assume_role_with_web_identity', response, expected_params ) diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 06f891911c..696c1998f5 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -265,7 +265,6 @@ def get_expected_creds_from_response(self, response): 'secret_key': response['Credentials']['SecretAccessKey'], 'token': response['Credentials']['SessionToken'], 'expiry_time': expiration, - 'account_id': None, } def some_future_time(self): @@ -285,7 +284,6 @@ def test_no_cache(self): refresher = credentials.AssumeRoleCredentialFetcher( client_creator, self.source_creds, self.role_arn ) - expected_response = self.get_expected_creds_from_response(response) response = refresher.fetch_credentials() @@ -772,7 +770,6 @@ def get_expected_creds_from_response(self, response): 'secret_key': response['Credentials']['SecretAccessKey'], 'token': response['Credentials']['SessionToken'], 'expiry_time': expiration, - 'account_id': None, } def test_no_cache(self): From 24f6df23d7c69cf07f7cb8c71cc8ae0d7bd9d145 Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 6 Oct 2023 17:10:49 -0400 Subject: [PATCH 11/29] formatting fixes --- botocore/credentials.py | 10 ++++------ botocore/regions.py | 7 ++++--- tests/unit/test_credentials.py | 1 + 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 04c88eef7a..7567216fa5 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -1084,9 +1084,8 @@ def _retrieve_credentials_using(self, credential_process): return creds_dict def _resolve_account_id(self, parsed_response): - return parsed_response.get('AccountId') or self.profile_config.get( - 'aws_account_id' - ) + account_id = parsed_response.get('AccountId') + return account_id or self.profile_config.get('aws_account_id') @property def _credential_process(self): @@ -1096,9 +1095,8 @@ def _credential_process(self): def profile_config(self): if self._loaded_config is None: self._loaded_config = self._load_config() - return self._loaded_config.get('profiles', {}).get( - self._profile_name, {} - ) + profiles = self._loaded_config.get('profiles', {}) + return profiles.get(self._profile_name, {}) class InstanceMetadataProvider(CredentialProvider): diff --git a/botocore/regions.py b/botocore/regions.py index 827853e0a7..0cbb0732e1 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -587,11 +587,11 @@ def _resolve_account_id_builtin(self, request_context, builtins): request_context ) acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - acct_id_builtin = builtins.get(acct_id_builtin_key) if acct_id_ep_mode == 'disabled': # Unset the account ID if endpoint mode is disabled. builtins[acct_id_builtin_key] = None - elif acct_id_builtin is None: + + elif builtins.get(acct_id_builtin_key) is None: self._do_resolve_account_id_builtin(acct_id_ep_mode, builtins) def _resolve_account_id_endpoint_mode(self, request_context): @@ -599,7 +599,8 @@ def _resolve_account_id_endpoint_mode(self, request_context): not_presign = not request_context.get('is_presign_request', False) should_sign = self._requested_auth_scheme != UNSIGNED creds_available = self._credentials is not None - if all((not_presign, should_sign, creds_available)): + + if not_presign and should_sign and creds_available: ep_mode = request_context['client_config'].account_id_endpoint_mode return self._validate_account_id_endpoint_mode(ep_mode) return 'disabled' diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 696c1998f5..02c99aac89 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -284,6 +284,7 @@ def test_no_cache(self): refresher = credentials.AssumeRoleCredentialFetcher( client_creator, self.source_creds, self.role_arn ) + expected_response = self.get_expected_creds_from_response(response) response = refresher.fetch_credentials() From e321aebe79b1768c4af5d86436b35285aa9f635d Mon Sep 17 00:00:00 2001 From: davidlm Date: Tue, 10 Oct 2023 13:28:49 -0400 Subject: [PATCH 12/29] updates to test_endpoint_provider.py --- tests/unit/test_endpoint_provider.py | 97 +++++++++------------------- 1 file changed, 32 insertions(+), 65 deletions(-) diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index eeacba36bd..3dffbbbaae 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -103,7 +103,8 @@ def rule_lib(partitions): return RuleSetStandardLibrary(partitions) -def _ruleset_dict(): +@pytest.fixture(scope="module") +def ruleset_dict(): path = os.path.join( os.path.dirname(__file__), "data", @@ -115,11 +116,6 @@ def _ruleset_dict(): return json.load(f) -@pytest.fixture(scope="module") -def ruleset_dict(): - return _ruleset_dict() - - @pytest.fixture(scope="module") def endpoint_provider(ruleset_dict, partitions): return EndpointProvider(ruleset_dict, partitions) @@ -522,7 +518,8 @@ def test_aws_is_virtual_hostable_s3_bucket_allow_subdomains( ) -def _account_id_ruleset(): +@pytest.fixture +def account_id_ruleset(): rule_path = os.path.join( os.path.dirname(__file__), "data", @@ -542,7 +539,6 @@ def operation_model_empty_context_params(): return operation_model -ACCOUNT_ID_RULESET = _account_id_ruleset() BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID = { EndpointResolverBuiltins.AWS_REGION: "us-west-2", EndpointResolverBuiltins.AWS_ACCOUNT_ID: None, @@ -551,7 +547,7 @@ def operation_model_empty_context_params(): EndpointResolverBuiltins.AWS_REGION: "us-west-2", EndpointResolverBuiltins.AWS_ACCOUNT_ID: "0987654321", } -STATIC_CREDENTIALS = Credentials( +CREDENTIALS = Credentials( access_key="access_key", secret_key="secret_key", token="token", @@ -569,19 +565,18 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): builtins=bulitins, client_context=None, event_emitter=Mock(), - use_ssl=True, credentials=credentials, requested_auth_scheme=auth_scheme, ) -ACT_ID_REQUIRED_CONTEXT = { +ACCT_ID_REQUIRED_CONTEXT = { "client_config": Config(account_id_endpoint_mode="required") } -ACT_ID_PREFERRED_CONTEXT = { +ACCT_ID_PREFERRED_CONTEXT = { "client_config": Config(account_id_endpoint_mode="preferred") } -ACT_ID_DISABLED_CONTEXT = { +ACCT_ID_DISABLED_CONTEXT = { "client_config": Config(account_id_endpoint_mode="disabled") } @@ -590,98 +585,73 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): @pytest.mark.parametrize( - "ruleset, builtins, credentials, auth_scheme, request_context, expected_url", + "builtins, credentials, auth_scheme, request_context, expected_url", [ ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, None, - ACT_ID_REQUIRED_CONTEXT, - URL_WITH_ACCOUNT_ID, - ), - ( - ACCOUNT_ID_RULESET, - BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, - None, - ACT_ID_PREFERRED_CONTEXT, + ACCT_ID_REQUIRED_CONTEXT, URL_WITH_ACCOUNT_ID, ), # custom account ID takes precedence over credentials ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_RESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, None, - ACT_ID_REQUIRED_CONTEXT, + ACCT_ID_REQUIRED_CONTEXT, "https://0987654321.amazonaws.com", ), - # no account ID builtin in ruleset - ( - _ruleset_dict(), - BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, - None, - ACT_ID_REQUIRED_CONTEXT, - "https://us-west-2.amazonaws.com", - ), ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, None, - ACT_ID_DISABLED_CONTEXT, + ACCT_ID_DISABLED_CONTEXT, URL_NO_ACCOUNT_ID, ), # custom account ID removed if account ID mode is disabled ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_RESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, None, - ACT_ID_DISABLED_CONTEXT, + ACCT_ID_DISABLED_CONTEXT, URL_NO_ACCOUNT_ID, ), ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, UNSIGNED, - ACT_ID_REQUIRED_CONTEXT, + ACCT_ID_REQUIRED_CONTEXT, URL_NO_ACCOUNT_ID, ), ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - STATIC_CREDENTIALS, + CREDENTIALS, None, - {**ACT_ID_REQUIRED_CONTEXT, "is_presign_request": True}, + {**ACCT_ID_REQUIRED_CONTEXT, "is_presign_request": True}, URL_NO_ACCOUNT_ID, ), # no credentials ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, None, None, - ACT_ID_PREFERRED_CONTEXT, + ACCT_ID_PREFERRED_CONTEXT, URL_NO_ACCOUNT_ID, ), # no account ID in credentials ( - ACCOUNT_ID_RULESET, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, Credentials(access_key="foo", secret_key="bar", token="baz"), None, - ACT_ID_PREFERRED_CONTEXT, + ACCT_ID_PREFERRED_CONTEXT, URL_NO_ACCOUNT_ID, ), ], ) def test_account_id_builtin( operation_model_empty_context_params, - ruleset, + account_id_ruleset, builtins, credentials, auth_scheme, @@ -689,7 +659,7 @@ def test_account_id_builtin( expected_url, ): resolver = create_ruleset_resolver( - ruleset, builtins, credentials, auth_scheme + account_id_ruleset, builtins, credentials, auth_scheme ) endpoint = resolver.construct_endpoint( operation_model=operation_model_empty_context_params, @@ -700,43 +670,40 @@ def test_account_id_builtin( @pytest.mark.parametrize( - "credentials, auth_scheme, request_context, expected_error", + "credentials, request_context, expected_error", [ # invalid value for mode ( - STATIC_CREDENTIALS, - None, + CREDENTIALS, {"client_config": Config(account_id_endpoint_mode="foo")}, InvalidConfigError, ), # mode is case sensitive ( - STATIC_CREDENTIALS, - None, + CREDENTIALS, {"client_config": Config(account_id_endpoint_mode="PREFERRED")}, InvalidConfigError, ), # no account ID found but required ( Credentials(access_key="foo", secret_key="bar", token="baz"), - None, - ACT_ID_REQUIRED_CONTEXT, + ACCT_ID_REQUIRED_CONTEXT, AccountIdNotFound, ), ], ) def test_account_id_error_cases( operation_model_empty_context_params, + account_id_ruleset, credentials, - auth_scheme, request_context, expected_error, ): resolver = create_ruleset_resolver( - ACCOUNT_ID_RULESET, + account_id_ruleset, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, credentials, - auth_scheme, + None, ) with pytest.raises(expected_error): resolver.construct_endpoint( From f41ff9cfd8efe8fbb58d166a0908e4f9d1c19faf Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 11 Oct 2023 16:54:56 -0400 Subject: [PATCH 13/29] changelog --- .changes/next-release/feature-endpoints-57979.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/feature-endpoints-57979.json diff --git a/.changes/next-release/feature-endpoints-57979.json b/.changes/next-release/feature-endpoints-57979.json new file mode 100644 index 0000000000..f3a8f9799b --- /dev/null +++ b/.changes/next-release/feature-endpoints-57979.json @@ -0,0 +1,5 @@ +{ + "type": "feature", + "category": "endpoints", + "description": "Enables account ID endpoint routing" +} From a183e91ba0d81ae8cfdd64834641e3003266f700 Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 13 Oct 2023 11:06:14 -0400 Subject: [PATCH 14/29] pr feedback --- .../next-release/feature-endpoints-57979.json | 2 +- botocore/exceptions.py | 2 +- botocore/regions.py | 33 ++++++++++--------- botocore/session.py | 1 - 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/.changes/next-release/feature-endpoints-57979.json b/.changes/next-release/feature-endpoints-57979.json index f3a8f9799b..5af6c3747a 100644 --- a/.changes/next-release/feature-endpoints-57979.json +++ b/.changes/next-release/feature-endpoints-57979.json @@ -1,5 +1,5 @@ { "type": "feature", "category": "endpoints", - "description": "Enables account ID endpoint routing" + "description": "Adds support for account ID endpoint routing" } diff --git a/botocore/exceptions.py b/botocore/exceptions.py index 8506971c31..8fd052f30e 100644 --- a/botocore/exceptions.py +++ b/botocore/exceptions.py @@ -817,4 +817,4 @@ class UnknownEndpointResolutionBuiltInName(EndpointProviderError): class AccountIdNotFound(EndpointResolutionError): - fmt = '"account_id_endpoint_mode" is set to "required" but no account ID was found.' + fmt = '{msg}' diff --git a/botocore/regions.py b/botocore/regions.py index 0cbb0732e1..3f85b3d76a 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -48,12 +48,6 @@ LOG = logging.getLogger(__name__) DEFAULT_URI_TEMPLATE = '{service}.{region}.{dnsSuffix}' # noqa DEFAULT_SERVICE_DATA = {'endpoints': {}} -# Allowed values for the ``account_id_endpoint_mode`` config field. -VALID_ACCOUNT_ID_ENDPOINT_MODES = ( - 'preferred', - 'disabled', - 'required', -) class BaseEndpointResolver: @@ -465,6 +459,13 @@ class EndpointResolverBuiltins(str, Enum): class EndpointRulesetResolver: """Resolves endpoints using a service's endpoint ruleset""" + # Allowed values for the ``account_id_endpoint_mode`` config field. + VALID_ACCOUNT_ID_ENDPOINT_MODES = ( + 'preferred', + 'disabled', + 'required', + ) + def __init__( self, endpoint_ruleset_data, @@ -606,11 +607,14 @@ def _resolve_account_id_endpoint_mode(self, request_context): return 'disabled' def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): - if account_id_endpoint_mode not in VALID_ACCOUNT_ID_ENDPOINT_MODES: + if ( + account_id_endpoint_mode + not in self.VALID_ACCOUNT_ID_ENDPOINT_MODES + ): error_msg = ( f"Invalid value '{account_id_endpoint_mode}' for " "account_id_endpoint_mode. Valid values are: " - f"{', '.join(VALID_ACCOUNT_ID_ENDPOINT_MODES)}." + f"{', '.join(self.VALID_ACCOUNT_ID_ENDPOINT_MODES)}." ) raise InvalidConfigError(error_msg=error_msg) return account_id_endpoint_mode @@ -621,13 +625,12 @@ def _do_resolve_account_id_builtin( frozen_creds = self._credentials.get_frozen_credentials() account_id = frozen_creds.account_id if account_id is None: - if account_id_endpoint_mode == 'preferred': - LOG.debug( - '`account_id_endpoint_mode` is set to `preferred`, but no ' - 'account ID was found.' - ) - elif account_id_endpoint_mode == 'required': - raise AccountIdNotFound() + acct_id_ep_mode = account_id_endpoint_mode + msg = f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}' + if acct_id_ep_mode == 'preferred': + LOG.debug('%s, but account ID was not found.', msg) + elif acct_id_ep_mode == 'required': + raise AccountIdNotFound(msg=msg) else: builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id diff --git a/botocore/session.py b/botocore/session.py index 13e116ca68..721cf50f94 100644 --- a/botocore/session.py +++ b/botocore/session.py @@ -919,7 +919,6 @@ def create_client( :type aws_account_id: string :param aws_account_id: The AWS account ID to use when creating the client. Same semantics as aws_access_key_id above. - """ default_client_config = self.get_default_client_config() # If a config is provided and a default config is set, then From 6892215225f87f3e92bc1e3b527cca995c9b35df Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 13 Oct 2023 11:12:56 -0400 Subject: [PATCH 15/29] string formatting --- botocore/regions.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/botocore/regions.py b/botocore/regions.py index 3f85b3d76a..235160e8fc 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -607,14 +607,12 @@ def _resolve_account_id_endpoint_mode(self, request_context): return 'disabled' def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): - if ( - account_id_endpoint_mode - not in self.VALID_ACCOUNT_ID_ENDPOINT_MODES - ): + valid_modes = self.VALID_ACCOUNT_ID_ENDPOINT_MODES + if account_id_endpoint_mode not in valid_modes: error_msg = ( - f"Invalid value '{account_id_endpoint_mode}' for " - "account_id_endpoint_mode. Valid values are: " - f"{', '.join(self.VALID_ACCOUNT_ID_ENDPOINT_MODES)}." + f'Invalid value "{account_id_endpoint_mode}" for ' + 'account_id_endpoint_mode. Valid values are: ' + f'{", ".join(valid_modes)}.' ) raise InvalidConfigError(error_msg=error_msg) return account_id_endpoint_mode From c38ebf499b05df068b5e830d2affcdb1b2862895 Mon Sep 17 00:00:00 2001 From: davidlm Date: Tue, 17 Oct 2023 09:44:08 -0400 Subject: [PATCH 16/29] log message formatting --- botocore/credentials.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 7567216fa5..fe881d5fd6 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -799,9 +799,8 @@ def _resolve_account_id(self, response): try: account_id = self._arn_parser.parse_arn(user_arn)['account'] except InvalidArnException: - logger.debug( - 'Unable to parse account ID from ARN: %s', user_arn - ) + log_msg = 'Unable to parse account ID from ARN: %s' + logger.debug(log_msg, user_arn) else: response['Credentials']['AccountId'] = account_id From ef2397b3299e864722c71f28bea682d759508286 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 18 Oct 2023 17:17:33 -0400 Subject: [PATCH 17/29] pass `account_id_endpoint_mode` as an input param to resolver and simplify resolution branching logic --- botocore/args.py | 3 ++ botocore/regions.py | 60 +++++++++++------------- tests/unit/test_endpoint_provider.py | 70 ++++++++++++++-------------- 3 files changed, 64 insertions(+), 69 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index 1941bfbeef..446654c105 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -166,6 +166,7 @@ def get_client_args( endpoint_bridge, event_emitter, credentials, + new_config.account_id_endpoint_mode, ) # Copy the session's user agent factory and adds client configuration. @@ -631,6 +632,7 @@ def _build_endpoint_resolver( endpoint_bridge, event_emitter, credentials, + account_id_endpoint_mode, ): if endpoints_ruleset_data is None: return None @@ -681,6 +683,7 @@ def _build_endpoint_resolver( use_ssl=is_secure, requested_auth_scheme=sig_version, credentials=credentials, + account_id_endpoint_mode=account_id_endpoint_mode, ) def compute_endpoint_resolver_builtin_defaults( diff --git a/botocore/regions.py b/botocore/regions.py index 235160e8fc..062cce6ca4 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -459,9 +459,10 @@ class EndpointResolverBuiltins(str, Enum): class EndpointRulesetResolver: """Resolves endpoints using a service's endpoint ruleset""" + DEFAULT_ACCOUNT_ID_ENDPOINT_MODE = 'preferred' # Allowed values for the ``account_id_endpoint_mode`` config field. VALID_ACCOUNT_ID_ENDPOINT_MODES = ( - 'preferred', + DEFAULT_ACCOUNT_ID_ENDPOINT_MODE, 'disabled', 'required', ) @@ -477,6 +478,7 @@ def __init__( use_ssl=True, requested_auth_scheme=None, credentials=None, + account_id_endpoint_mode=None, ): self._provider = EndpointProvider( ruleset_data=endpoint_ruleset_data, @@ -491,6 +493,9 @@ def __init__( self._requested_auth_scheme = requested_auth_scheme self._instance_cache = {} self._credentials = credentials + if account_id_endpoint_mode is None: + account_id_endpoint_mode = self.DEFAULT_ACCOUNT_ID_ENDPOINT_MODE + self._account_id_endpoint_mode = account_id_endpoint_mode def construct_endpoint( self, @@ -559,7 +564,7 @@ def _get_provider_params( customized_builtins = self._get_customized_builtins( operation_model, call_args, request_context ) - self._resolve_account_id_builtin(request_context, customized_builtins) + self._resolve_account_id_builtin(customized_builtins) for param_name, param_def in self._param_definitions.items(): param_val = self._resolve_param_from_context( param_name=param_name, @@ -576,54 +581,43 @@ def _get_provider_params( return provider_params - def _resolve_account_id_builtin(self, request_context, builtins): - """Resolve the ``AWS::Auth::AccountId`` builtin if configured in the - ruleset, account ID based routing is enabled and it has not already - been resolved in a custom handler. - """ + def _resolve_account_id_builtin(self, builtins): + """Resolve the ``AWS::Auth::AccountId`` builtin.""" + # do this check separately so mode is only validated if it's being used if 'AccountId' not in self._param_definitions: return - acct_id_ep_mode = self._resolve_account_id_endpoint_mode( - request_context - ) + self._validate_account_id_endpoint_mode() acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - if acct_id_ep_mode == 'disabled': + if not self._should_resolve_account_id_builtin(): # Unset the account ID if endpoint mode is disabled. builtins[acct_id_builtin_key] = None + return - elif builtins.get(acct_id_builtin_key) is None: - self._do_resolve_account_id_builtin(acct_id_ep_mode, builtins) - - def _resolve_account_id_endpoint_mode(self, request_context): - """Resolve the account ID endpoint mode for the request.""" - not_presign = not request_context.get('is_presign_request', False) - should_sign = self._requested_auth_scheme != UNSIGNED - creds_available = self._credentials is not None - - if not_presign and should_sign and creds_available: - ep_mode = request_context['client_config'].account_id_endpoint_mode - return self._validate_account_id_endpoint_mode(ep_mode) - return 'disabled' + if builtins.get(acct_id_builtin_key) is None: + self._do_resolve_account_id_builtin(builtins) - def _validate_account_id_endpoint_mode(self, account_id_endpoint_mode): + def _validate_account_id_endpoint_mode(self): valid_modes = self.VALID_ACCOUNT_ID_ENDPOINT_MODES - if account_id_endpoint_mode not in valid_modes: + if self._account_id_endpoint_mode not in valid_modes: error_msg = ( - f'Invalid value "{account_id_endpoint_mode}" for ' - 'account_id_endpoint_mode. Valid values are: ' + f'Invalid value "{self._account_id_endpoint_mode}" ' + 'for account_id_endpoint_mode. Valid values are: ' f'{", ".join(valid_modes)}.' ) raise InvalidConfigError(error_msg=error_msg) - return account_id_endpoint_mode - def _do_resolve_account_id_builtin( - self, account_id_endpoint_mode, builtins - ): + def _should_resolve_account_id_builtin(self): + signing_enabled = self._requested_auth_scheme != UNSIGNED + creds_available = self._credentials is not None + mode_enabled = self._account_id_endpoint_mode != 'disabled' + return signing_enabled and creds_available and mode_enabled + + def _do_resolve_account_id_builtin(self, builtins): frozen_creds = self._credentials.get_frozen_credentials() account_id = frozen_creds.account_id if account_id is None: - acct_id_ep_mode = account_id_endpoint_mode + acct_id_ep_mode = self._account_id_endpoint_mode msg = f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}' if acct_id_ep_mode == 'preferred': LOG.debug('%s, but account ID was not found.', msg) diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 3dffbbbaae..30bbe5ccc2 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -19,7 +19,6 @@ import pytest from botocore import UNSIGNED -from botocore.config import Config from botocore.credentials import Credentials from botocore.endpoint_provider import ( EndpointProvider, @@ -553,9 +552,16 @@ def operation_model_empty_context_params(): token="token", account_id="1234567890", ) +REQUIRED = 'required' +PREFERRED = 'preferred' +DISABLED = 'disabled' +URL_NO_ACCOUNT_ID = "https://amazonaws.com" +URL_WITH_ACCOUNT_ID = "https://1234567890.amazonaws.com" -def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): +def create_ruleset_resolver( + ruleset, bulitins, credentials, auth_scheme, account_id_endpoint_mode +): service_model = Mock() service_model.client_context_parameters = [] return EndpointRulesetResolver( @@ -567,31 +573,18 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): event_emitter=Mock(), credentials=credentials, requested_auth_scheme=auth_scheme, + account_id_endpoint_mode=account_id_endpoint_mode, ) -ACCT_ID_REQUIRED_CONTEXT = { - "client_config": Config(account_id_endpoint_mode="required") -} -ACCT_ID_PREFERRED_CONTEXT = { - "client_config": Config(account_id_endpoint_mode="preferred") -} -ACCT_ID_DISABLED_CONTEXT = { - "client_config": Config(account_id_endpoint_mode="disabled") -} - -URL_NO_ACCOUNT_ID = "https://amazonaws.com" -URL_WITH_ACCOUNT_ID = "https://1234567890.amazonaws.com" - - @pytest.mark.parametrize( - "builtins, credentials, auth_scheme, request_context, expected_url", + "builtins, credentials, auth_scheme, account_id_endpoint_mode, expected_url", [ ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, CREDENTIALS, None, - ACCT_ID_REQUIRED_CONTEXT, + REQUIRED, URL_WITH_ACCOUNT_ID, ), # custom account ID takes precedence over credentials @@ -599,14 +592,14 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): BUILTINS_WITH_RESOLVED_ACCOUNT_ID, CREDENTIALS, None, - ACCT_ID_REQUIRED_CONTEXT, + REQUIRED, "https://0987654321.amazonaws.com", ), ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, CREDENTIALS, None, - ACCT_ID_DISABLED_CONTEXT, + DISABLED, URL_NO_ACCOUNT_ID, ), # custom account ID removed if account ID mode is disabled @@ -614,21 +607,21 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): BUILTINS_WITH_RESOLVED_ACCOUNT_ID, CREDENTIALS, None, - ACCT_ID_DISABLED_CONTEXT, + DISABLED, URL_NO_ACCOUNT_ID, ), ( - BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + BUILTINS_WITH_RESOLVED_ACCOUNT_ID, CREDENTIALS, UNSIGNED, - ACCT_ID_REQUIRED_CONTEXT, + REQUIRED, URL_NO_ACCOUNT_ID, ), ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, CREDENTIALS, - None, - {**ACCT_ID_REQUIRED_CONTEXT, "is_presign_request": True}, + UNSIGNED, + REQUIRED, URL_NO_ACCOUNT_ID, ), # no credentials @@ -636,7 +629,7 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, None, None, - ACCT_ID_PREFERRED_CONTEXT, + PREFERRED, URL_NO_ACCOUNT_ID, ), # no account ID in credentials @@ -644,7 +637,7 @@ def create_ruleset_resolver(ruleset, bulitins, credentials, auth_scheme): BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, Credentials(access_key="foo", secret_key="bar", token="baz"), None, - ACCT_ID_PREFERRED_CONTEXT, + PREFERRED, URL_NO_ACCOUNT_ID, ), ], @@ -655,39 +648,43 @@ def test_account_id_builtin( builtins, credentials, auth_scheme, - request_context, + account_id_endpoint_mode, expected_url, ): resolver = create_ruleset_resolver( - account_id_ruleset, builtins, credentials, auth_scheme + account_id_ruleset, + builtins, + credentials, + auth_scheme, + account_id_endpoint_mode, ) endpoint = resolver.construct_endpoint( operation_model=operation_model_empty_context_params, - request_context=request_context, + request_context={}, call_args={}, ) assert endpoint.url == expected_url @pytest.mark.parametrize( - "credentials, request_context, expected_error", + "credentials, account_id_endpoint_mode, expected_error", [ # invalid value for mode ( CREDENTIALS, - {"client_config": Config(account_id_endpoint_mode="foo")}, + "foo", InvalidConfigError, ), # mode is case sensitive ( CREDENTIALS, - {"client_config": Config(account_id_endpoint_mode="PREFERRED")}, + "PREFERRED", InvalidConfigError, ), # no account ID found but required ( Credentials(access_key="foo", secret_key="bar", token="baz"), - ACCT_ID_REQUIRED_CONTEXT, + REQUIRED, AccountIdNotFound, ), ], @@ -696,7 +693,7 @@ def test_account_id_error_cases( operation_model_empty_context_params, account_id_ruleset, credentials, - request_context, + account_id_endpoint_mode, expected_error, ): resolver = create_ruleset_resolver( @@ -704,10 +701,11 @@ def test_account_id_error_cases( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, credentials, None, + account_id_endpoint_mode, ) with pytest.raises(expected_error): resolver.construct_endpoint( operation_model=operation_model_empty_context_params, - request_context=request_context, + request_context={}, call_args={}, ) From 49e071ff49c5a30e76050de045df965b96d86a53 Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 19 Oct 2023 09:04:51 -0400 Subject: [PATCH 18/29] clean up _resolve_account_id_builtin --- botocore/regions.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/botocore/regions.py b/botocore/regions.py index 062cce6ca4..aec02355c9 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -583,7 +583,6 @@ def _get_provider_params( def _resolve_account_id_builtin(self, builtins): """Resolve the ``AWS::Auth::AccountId`` builtin.""" - # do this check separately so mode is only validated if it's being used if 'AccountId' not in self._param_definitions: return @@ -592,9 +591,8 @@ def _resolve_account_id_builtin(self, builtins): if not self._should_resolve_account_id_builtin(): # Unset the account ID if endpoint mode is disabled. builtins[acct_id_builtin_key] = None - return - if builtins.get(acct_id_builtin_key) is None: + elif builtins.get(acct_id_builtin_key) is None: self._do_resolve_account_id_builtin(builtins) def _validate_account_id_endpoint_mode(self): From dee1e9f39b97453c0e80434b06287b20eafb88f3 Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 26 Oct 2023 18:15:26 -0400 Subject: [PATCH 19/29] move account id resolution to separate class --- botocore/regions.py | 107 ++++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/botocore/regions.py b/botocore/regions.py index aec02355c9..ebc497c372 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -456,8 +456,8 @@ class EndpointResolverBuiltins(str, Enum): AWS_ACCOUNT_ID = "AWS::Auth::AccountId" -class EndpointRulesetResolver: - """Resolves endpoints using a service's endpoint ruleset""" +class CredentialBuiltinResolver: + """Resolves endpoint builtins sourced from credentials.""" DEFAULT_ACCOUNT_ID_ENDPOINT_MODE = 'preferred' # Allowed values for the ``account_id_endpoint_mode`` config field. @@ -467,6 +467,55 @@ class EndpointRulesetResolver: 'required', ) + def __init__(self, credentials, account_id_endpoint_mode): + self._credentials = credentials + if account_id_endpoint_mode is None: + account_id_endpoint_mode = self.DEFAULT_ACCOUNT_ID_ENDPOINT_MODE + self._account_id_endpoint_mode = account_id_endpoint_mode + + def resolve_account_id_builtin(self, builtins, signing_enabled): + """Resolve the ``AWS::Auth::AccountId`` builtin.""" + self._validate_account_id_endpoint_mode() + acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID + if not self._should_resolve_account_id_builtin(signing_enabled): + # Unset the account ID if endpoint mode is disabled. + builtins[acct_id_builtin_key] = None + + elif builtins.get(acct_id_builtin_key) is None: + self._do_resolve_account_id_builtin(builtins) + + def _validate_account_id_endpoint_mode(self): + valid_modes = self.VALID_ACCOUNT_ID_ENDPOINT_MODES + if self._account_id_endpoint_mode not in valid_modes: + error_msg = ( + f'Invalid value "{self._account_id_endpoint_mode}" ' + 'for account_id_endpoint_mode. Valid values are: ' + f'{", ".join(valid_modes)}.' + ) + raise InvalidConfigError(error_msg=error_msg) + + def _should_resolve_account_id_builtin(self, signing_enabled): + creds_available = self._credentials is not None + mode_enabled = self._account_id_endpoint_mode != 'disabled' + return signing_enabled and creds_available and mode_enabled + + def _do_resolve_account_id_builtin(self, builtins): + frozen_creds = self._credentials.get_frozen_credentials() + account_id = frozen_creds.account_id + if account_id is None: + acct_id_ep_mode = self._account_id_endpoint_mode + msg = f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}' + if acct_id_ep_mode == 'preferred': + LOG.debug('%s, but account ID was not found.', msg) + elif acct_id_ep_mode == 'required': + raise AccountIdNotFound(msg=msg) + else: + builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id + + +class EndpointRulesetResolver: + """Resolves endpoints using a service's endpoint ruleset""" + def __init__( self, endpoint_ruleset_data, @@ -492,10 +541,9 @@ def __init__( self._use_ssl = use_ssl self._requested_auth_scheme = requested_auth_scheme self._instance_cache = {} - self._credentials = credentials - if account_id_endpoint_mode is None: - account_id_endpoint_mode = self.DEFAULT_ACCOUNT_ID_ENDPOINT_MODE - self._account_id_endpoint_mode = account_id_endpoint_mode + self._credential_builtin_resolver = CredentialBuiltinResolver( + credentials, account_id_endpoint_mode + ) def construct_endpoint( self, @@ -564,7 +612,7 @@ def _get_provider_params( customized_builtins = self._get_customized_builtins( operation_model, call_args, request_context ) - self._resolve_account_id_builtin(customized_builtins) + self._resolve_credential_builtins(customized_builtins) for param_name, param_def in self._param_definitions.items(): param_val = self._resolve_param_from_context( param_name=param_name, @@ -581,48 +629,11 @@ def _get_provider_params( return provider_params - def _resolve_account_id_builtin(self, builtins): - """Resolve the ``AWS::Auth::AccountId`` builtin.""" - if 'AccountId' not in self._param_definitions: - return - - self._validate_account_id_endpoint_mode() - acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - if not self._should_resolve_account_id_builtin(): - # Unset the account ID if endpoint mode is disabled. - builtins[acct_id_builtin_key] = None - - elif builtins.get(acct_id_builtin_key) is None: - self._do_resolve_account_id_builtin(builtins) - - def _validate_account_id_endpoint_mode(self): - valid_modes = self.VALID_ACCOUNT_ID_ENDPOINT_MODES - if self._account_id_endpoint_mode not in valid_modes: - error_msg = ( - f'Invalid value "{self._account_id_endpoint_mode}" ' - 'for account_id_endpoint_mode. Valid values are: ' - f'{", ".join(valid_modes)}.' - ) - raise InvalidConfigError(error_msg=error_msg) - - def _should_resolve_account_id_builtin(self): + def _resolve_credential_builtins(self, builtins): + resolver = self._credential_builtin_resolver signing_enabled = self._requested_auth_scheme != UNSIGNED - creds_available = self._credentials is not None - mode_enabled = self._account_id_endpoint_mode != 'disabled' - return signing_enabled and creds_available and mode_enabled - - def _do_resolve_account_id_builtin(self, builtins): - frozen_creds = self._credentials.get_frozen_credentials() - account_id = frozen_creds.account_id - if account_id is None: - acct_id_ep_mode = self._account_id_endpoint_mode - msg = f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}' - if acct_id_ep_mode == 'preferred': - LOG.debug('%s, but account ID was not found.', msg) - elif acct_id_ep_mode == 'required': - raise AccountIdNotFound(msg=msg) - else: - builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id + if 'AccountId' in self._param_definitions: + resolver.resolve_account_id_builtin(builtins, signing_enabled) def _resolve_param_from_context( self, param_name, operation_model, call_args From 83017c5eea96344f983cc927e59da8935e906070 Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 27 Oct 2023 13:35:01 -0400 Subject: [PATCH 20/29] bug fix. account id may not always be a builtin when configured as a param on a ruleset --- botocore/regions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/botocore/regions.py b/botocore/regions.py index ebc497c372..15ed26e217 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -632,7 +632,8 @@ def _get_provider_params( def _resolve_credential_builtins(self, builtins): resolver = self._credential_builtin_resolver signing_enabled = self._requested_auth_scheme != UNSIGNED - if 'AccountId' in self._param_definitions: + param_def = self._param_definitions.get('AccountId') + if param_def is not None and param_def.builtin is not None: resolver.resolve_account_id_builtin(builtins, signing_enabled) def _resolve_param_from_context( From 9db7688bda475beefaaf1ba2391ff90e07fb91a3 Mon Sep 17 00:00:00 2001 From: davidlm Date: Mon, 30 Oct 2023 12:11:20 -0400 Subject: [PATCH 21/29] add back line lost in rebase --- botocore/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/botocore/config.py b/botocore/config.py index 892027e7a4..7ff40ca598 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -227,6 +227,7 @@ class Config: the ``Client Context Parameters`` section of the service client's documentation. Invalid parameters or ones that are not used by the specified service will be ignored. + :type account_id_endpoint_mode: str :param account_id_endpoint_mode: Set account ID based endpoint behavior for supported operations. From d4b00a63a05ae42d564da9a284b090ee5c71264b Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 1 Nov 2023 15:48:31 -0400 Subject: [PATCH 22/29] pr feedback. pass builtin resolver to ruleset resolver and refactor so only builtin resolver is checking conditions --- botocore/args.py | 15 ++-- botocore/regions.py | 111 +++++++++++++++++++-------- tests/unit/test_credentials.py | 10 ++- tests/unit/test_endpoint_provider.py | 63 +++++++-------- 4 files changed, 123 insertions(+), 76 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index 446654c105..ab3ad8beb7 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -25,6 +25,7 @@ import botocore.serialize from botocore.config import Config from botocore.endpoint import EndpointCreator +from botocore.regions import CredentialBuiltinResolver from botocore.regions import EndpointResolverBuiltins as EPRBuiltins from botocore.regions import EndpointRulesetResolver from botocore.signers import RequestSigner @@ -152,7 +153,10 @@ def get_client_args( protocol, parameter_validation ) response_parser = botocore.parsers.create_parser(protocol) - + credential_builtin_resolver = CredentialBuiltinResolver( + credentials=credentials, + account_id_endpoint_mode=new_config.account_id_endpoint_mode, + ) ruleset_resolver = self._build_endpoint_resolver( endpoints_ruleset_data, partition_data, @@ -165,8 +169,7 @@ def get_client_args( is_secure, endpoint_bridge, event_emitter, - credentials, - new_config.account_id_endpoint_mode, + credential_builtin_resolver, ) # Copy the session's user agent factory and adds client configuration. @@ -631,8 +634,7 @@ def _build_endpoint_resolver( is_secure, endpoint_bridge, event_emitter, - credentials, - account_id_endpoint_mode, + credential_builtin_resolver, ): if endpoints_ruleset_data is None: return None @@ -682,8 +684,7 @@ def _build_endpoint_resolver( event_emitter=event_emitter, use_ssl=is_secure, requested_auth_scheme=sig_version, - credentials=credentials, - account_id_endpoint_mode=account_id_endpoint_mode, + credential_builtin_resolver=credential_builtin_resolver, ) def compute_endpoint_resolver_builtin_defaults( diff --git a/botocore/regions.py b/botocore/regions.py index 15ed26e217..2e8b9ce518 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -466,23 +466,50 @@ class CredentialBuiltinResolver: 'disabled', 'required', ) + REFRESHABLE_CREDENTIAL_METHODS = ( + 'custom-process', + 'assume-role', + 'assume-role-with-web-identity', + 'sso', + ) + STATIC_CREDENTIAL_METHODS = ( + 'explicit', + 'env', + 'shared-credentials-file', + 'config-file', + ) + SUPPORTED_CREDENTIAL_METHODS = ( + REFRESHABLE_CREDENTIAL_METHODS + STATIC_CREDENTIAL_METHODS + ) def __init__(self, credentials, account_id_endpoint_mode): self._credentials = credentials if account_id_endpoint_mode is None: account_id_endpoint_mode = self.DEFAULT_ACCOUNT_ID_ENDPOINT_MODE self._account_id_endpoint_mode = account_id_endpoint_mode + self._validate_account_id_endpoint_mode() - def resolve_account_id_builtin(self, builtins, signing_enabled): + def resolve_account_id_builtin(self, builtin_configured, builtin_value): """Resolve the ``AWS::Auth::AccountId`` builtin.""" - self._validate_account_id_endpoint_mode() - acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - if not self._should_resolve_account_id_builtin(signing_enabled): - # Unset the account ID if endpoint mode is disabled. - builtins[acct_id_builtin_key] = None + acct_id_ep_mode = self._account_id_endpoint_mode + mode_disabled = acct_id_ep_mode == 'disabled' + no_credentials = self._credentials is None + if not builtin_configured or mode_disabled or no_credentials: + return None - elif builtins.get(acct_id_builtin_key) is None: - self._do_resolve_account_id_builtin(builtins) + if builtin_value is not None: + return builtin_value + + frozen_creds = self._credentials.get_frozen_credentials() + account_id = frozen_creds.account_id + if account_id is None: + msg = self._create_no_account_id_message() + if acct_id_ep_mode == 'preferred': + LOG.debug(msg) + elif acct_id_ep_mode == 'required': + raise AccountIdNotFound(msg=msg) + + return account_id def _validate_account_id_endpoint_mode(self): valid_modes = self.VALID_ACCOUNT_ID_ENDPOINT_MODES @@ -494,23 +521,31 @@ def _validate_account_id_endpoint_mode(self): ) raise InvalidConfigError(error_msg=error_msg) - def _should_resolve_account_id_builtin(self, signing_enabled): - creds_available = self._credentials is not None - mode_enabled = self._account_id_endpoint_mode != 'disabled' - return signing_enabled and creds_available and mode_enabled - - def _do_resolve_account_id_builtin(self, builtins): - frozen_creds = self._credentials.get_frozen_credentials() - account_id = frozen_creds.account_id - if account_id is None: - acct_id_ep_mode = self._account_id_endpoint_mode - msg = f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}' - if acct_id_ep_mode == 'preferred': - LOG.debug('%s, but account ID was not found.', msg) - elif acct_id_ep_mode == 'required': - raise AccountIdNotFound(msg=msg) + def _create_no_account_id_message(self): + acct_id_ep_mode = self._account_id_endpoint_mode + method = self._credentials.method + base_msg = ( + f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}, ' + f'but account ID could not be resolved from source "{method}". ' + ) + if method in self.STATIC_CREDENTIAL_METHODS: + msg = ( + f'{base_msg} Credential source "{method}" does not have ' + 'an account ID associated with it. Please check your ' + 'configuration and try again.' + ) + elif method in self.REFRESHABLE_CREDENTIAL_METHODS: + msg = ( + f'{base_msg} Please ensure that your credential ' + 'source is configured to return an account ID.' + ) else: - builtins[EndpointResolverBuiltins.AWS_ACCOUNT_ID] = account_id + supported = ", ".join(self.SUPPORTED_CREDENTIAL_METHODS) + msg = ( + f'{base_msg} Please change your credential source to one of: ' + f'{supported} or set "account_id_endpoint_mode" to "disabled".' + ) + return msg class EndpointRulesetResolver: @@ -526,8 +561,7 @@ def __init__( event_emitter, use_ssl=True, requested_auth_scheme=None, - credentials=None, - account_id_endpoint_mode=None, + credential_builtin_resolver=None, ): self._provider = EndpointProvider( ruleset_data=endpoint_ruleset_data, @@ -541,9 +575,7 @@ def __init__( self._use_ssl = use_ssl self._requested_auth_scheme = requested_auth_scheme self._instance_cache = {} - self._credential_builtin_resolver = CredentialBuiltinResolver( - credentials, account_id_endpoint_mode - ) + self._credential_builtin_resolver = credential_builtin_resolver def construct_endpoint( self, @@ -630,11 +662,24 @@ def _get_provider_params( return provider_params def _resolve_credential_builtins(self, builtins): + if self._credential_builtin_resolver is None: + return + + self._resolve_account_id_builtin(builtins) + + def _resolve_account_id_builtin(self, builtins): resolver = self._credential_builtin_resolver - signing_enabled = self._requested_auth_scheme != UNSIGNED - param_def = self._param_definitions.get('AccountId') - if param_def is not None and param_def.builtin is not None: - resolver.resolve_account_id_builtin(builtins, signing_enabled) + builtin_configured = self._builtin_configured('AccountId') + acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID + current_builtin_value = builtins.get(acct_id_builtin_key) + account_id = resolver.resolve_account_id_builtin( + builtin_configured, current_builtin_value + ) + builtins[acct_id_builtin_key] = account_id + + def _builtin_configured(self, param_name): + param_def = self._param_definitions.get(param_name) + return param_def is not None and param_def.builtin is not None def _resolve_param_from_context( self, param_name, operation_model, call_args diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 02c99aac89..a3c7730a91 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -191,10 +191,18 @@ def test_refresh_returns_partial_credentials(self): self.creds.access_key def test_account_id_refresh(self): + # set the expiry to now so the creds will need a refresh again + now = datetime.now(tzlocal()) + self.mock_time.return_value = now + metadata = self.metadata.copy() + metadata['expiry_time'] = now.isoformat() + self.refresher.return_value = metadata + self.assertTrue(self.creds.refresh_needed()) + self.assertIsNone(self.creds.account_id) + # set the account id in the mocked refresher metadata = self.metadata.copy() metadata['account_id'] = '123456789012' self.refresher.return_value = metadata - self.mock_time.return_value = datetime.now(tzlocal()) self.assertTrue(self.creds.refresh_needed()) self.assertEqual(self.creds.account_id, '123456789012') diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 30bbe5ccc2..bcf584cef2 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -18,7 +18,6 @@ import pytest -from botocore import UNSIGNED from botocore.credentials import Credentials from botocore.endpoint_provider import ( EndpointProvider, @@ -37,7 +36,11 @@ UnknownSignatureVersionError, ) from botocore.loaders import Loader -from botocore.regions import EndpointResolverBuiltins, EndpointRulesetResolver +from botocore.regions import ( + CredentialBuiltinResolver, + EndpointResolverBuiltins, + EndpointRulesetResolver, +) from tests import requires_crt REGION_TEMPLATE = "{Region}" @@ -560,10 +563,13 @@ def operation_model_empty_context_params(): def create_ruleset_resolver( - ruleset, bulitins, credentials, auth_scheme, account_id_endpoint_mode + ruleset, bulitins, credentials, account_id_endpoint_mode ): service_model = Mock() service_model.client_context_parameters = [] + credential_builtin_resolver = CredentialBuiltinResolver( + credentials, account_id_endpoint_mode + ) return EndpointRulesetResolver( endpoint_ruleset_data=ruleset, partition_data={}, @@ -571,19 +577,16 @@ def create_ruleset_resolver( builtins=bulitins, client_context=None, event_emitter=Mock(), - credentials=credentials, - requested_auth_scheme=auth_scheme, - account_id_endpoint_mode=account_id_endpoint_mode, + credential_builtin_resolver=credential_builtin_resolver, ) @pytest.mark.parametrize( - "builtins, credentials, auth_scheme, account_id_endpoint_mode, expected_url", + "builtins, credentials, account_id_endpoint_mode, expected_url", [ ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, CREDENTIALS, - None, REQUIRED, URL_WITH_ACCOUNT_ID, ), @@ -591,14 +594,12 @@ def create_ruleset_resolver( ( BUILTINS_WITH_RESOLVED_ACCOUNT_ID, CREDENTIALS, - None, REQUIRED, "https://0987654321.amazonaws.com", ), ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, CREDENTIALS, - None, DISABLED, URL_NO_ACCOUNT_ID, ), @@ -606,21 +607,12 @@ def create_ruleset_resolver( ( BUILTINS_WITH_RESOLVED_ACCOUNT_ID, CREDENTIALS, - None, DISABLED, URL_NO_ACCOUNT_ID, ), ( BUILTINS_WITH_RESOLVED_ACCOUNT_ID, - CREDENTIALS, - UNSIGNED, - REQUIRED, - URL_NO_ACCOUNT_ID, - ), - ( - BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - CREDENTIALS, - UNSIGNED, + None, REQUIRED, URL_NO_ACCOUNT_ID, ), @@ -628,7 +620,6 @@ def create_ruleset_resolver( ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, None, - None, PREFERRED, URL_NO_ACCOUNT_ID, ), @@ -636,7 +627,6 @@ def create_ruleset_resolver( ( BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, Credentials(access_key="foo", secret_key="bar", token="baz"), - None, PREFERRED, URL_NO_ACCOUNT_ID, ), @@ -647,7 +637,6 @@ def test_account_id_builtin( account_id_ruleset, builtins, credentials, - auth_scheme, account_id_endpoint_mode, expected_url, ): @@ -655,7 +644,6 @@ def test_account_id_builtin( account_id_ruleset, builtins, credentials, - auth_scheme, account_id_endpoint_mode, ) endpoint = resolver.construct_endpoint( @@ -681,29 +669,34 @@ def test_account_id_builtin( "PREFERRED", InvalidConfigError, ), - # no account ID found but required - ( - Credentials(access_key="foo", secret_key="bar", token="baz"), - REQUIRED, - AccountIdNotFound, - ), ], ) -def test_account_id_error_cases( - operation_model_empty_context_params, +def test_account_id_endpoint_mode_input_error_cases( account_id_ruleset, credentials, account_id_endpoint_mode, expected_error, ): + with pytest.raises(expected_error): + create_ruleset_resolver( + account_id_ruleset, + BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, + credentials, + account_id_endpoint_mode, + ) + + +def test_required_mode_no_account_id( + account_id_ruleset, operation_model_empty_context_params +): + credentials = Credentials(access_key="a", secret_key="b", token="c") resolver = create_ruleset_resolver( account_id_ruleset, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, credentials, - None, - account_id_endpoint_mode, + REQUIRED, ) - with pytest.raises(expected_error): + with pytest.raises(AccountIdNotFound): resolver.construct_endpoint( operation_model=operation_model_empty_context_params, request_context={}, From 747c59c3856c61be55fdc1f31bae5b645ab6e711 Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 1 Nov 2023 16:55:53 -0400 Subject: [PATCH 23/29] error tests --- tests/unit/test_endpoint_provider.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index bcf584cef2..093dfe153e 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -686,17 +686,32 @@ def test_account_id_endpoint_mode_input_error_cases( ) +@pytest.mark.parametrize( + "credential_method, expected_message", + [ # refreshable method + ("assume-role", "ensure that your credential source is configured"), + # static method + ("env", "check your configuration and try again"), + # unsupported method + ("foo", "change your credential source to one of"), + ], +) def test_required_mode_no_account_id( - account_id_ruleset, operation_model_empty_context_params + account_id_ruleset, + operation_model_empty_context_params, + credential_method, + expected_message, ): - credentials = Credentials(access_key="a", secret_key="b", token="c") + credentials = Credentials( + access_key="a", secret_key="b", token="c", method=credential_method + ) resolver = create_ruleset_resolver( account_id_ruleset, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, credentials, REQUIRED, ) - with pytest.raises(AccountIdNotFound): + with pytest.raises(AccountIdNotFound, match=expected_message): resolver.construct_endpoint( operation_model=operation_model_empty_context_params, request_context={}, From e94c43816cd0a9d2f4a1ae7f539a9419038fb8c6 Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 2 Nov 2023 17:31:05 -0400 Subject: [PATCH 24/29] add higher level builtin resolver --- botocore/args.py | 22 +++++-- botocore/regions.py | 98 +++++++++++----------------- tests/unit/test_endpoint_provider.py | 31 +++------ 3 files changed, 63 insertions(+), 88 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index ab3ad8beb7..55f7b9f79d 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -25,7 +25,7 @@ import botocore.serialize from botocore.config import Config from botocore.endpoint import EndpointCreator -from botocore.regions import CredentialBuiltinResolver +from botocore.regions import CredentialBuiltinResolver, EndpointBuiltinResolver from botocore.regions import EndpointResolverBuiltins as EPRBuiltins from botocore.regions import EndpointRulesetResolver from botocore.signers import RequestSigner @@ -153,9 +153,8 @@ def get_client_args( protocol, parameter_validation ) response_parser = botocore.parsers.create_parser(protocol) - credential_builtin_resolver = CredentialBuiltinResolver( - credentials=credentials, - account_id_endpoint_mode=new_config.account_id_endpoint_mode, + builtin_resolver = self._construct_builtin_resolver( + credentials, new_config ) ruleset_resolver = self._build_endpoint_resolver( endpoints_ruleset_data, @@ -169,7 +168,7 @@ def get_client_args( is_secure, endpoint_bridge, event_emitter, - credential_builtin_resolver, + builtin_resolver, ) # Copy the session's user agent factory and adds client configuration. @@ -621,6 +620,15 @@ def _ensure_boolean(self, val): else: return val.lower() == 'true' + def _construct_builtin_resolver(self, credentials, client_config): + credential_builtin_resolver = CredentialBuiltinResolver( + credentials, client_config.account_id_endpoint_mode + ) + # This only includes CredentialBuiltinResolver for now, but may grow + # as more endpoint builtins are added. + builtin_resolvers = {'credentials': credential_builtin_resolver} + return EndpointBuiltinResolver(builtin_resolvers) + def _build_endpoint_resolver( self, endpoints_ruleset_data, @@ -634,7 +642,7 @@ def _build_endpoint_resolver( is_secure, endpoint_bridge, event_emitter, - credential_builtin_resolver, + builtin_resolver, ): if endpoints_ruleset_data is None: return None @@ -684,7 +692,7 @@ def _build_endpoint_resolver( event_emitter=event_emitter, use_ssl=is_secure, requested_auth_scheme=sig_version, - credential_builtin_resolver=credential_builtin_resolver, + builtin_resolver=builtin_resolver, ) def compute_endpoint_resolver_builtin_defaults( diff --git a/botocore/regions.py b/botocore/regions.py index 2e8b9ce518..9857f0d642 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -466,21 +466,6 @@ class CredentialBuiltinResolver: 'disabled', 'required', ) - REFRESHABLE_CREDENTIAL_METHODS = ( - 'custom-process', - 'assume-role', - 'assume-role-with-web-identity', - 'sso', - ) - STATIC_CREDENTIAL_METHODS = ( - 'explicit', - 'env', - 'shared-credentials-file', - 'config-file', - ) - SUPPORTED_CREDENTIAL_METHODS = ( - REFRESHABLE_CREDENTIAL_METHODS + STATIC_CREDENTIAL_METHODS - ) def __init__(self, credentials, account_id_endpoint_mode): self._credentials = credentials @@ -503,7 +488,10 @@ def resolve_account_id_builtin(self, builtin_configured, builtin_value): frozen_creds = self._credentials.get_frozen_credentials() account_id = frozen_creds.account_id if account_id is None: - msg = self._create_no_account_id_message() + msg = ( + f'"account_id_endpoint_mode" is set to "{acct_id_ep_mode}", ' + 'but account ID could not be resolved.' + ) if acct_id_ep_mode == 'preferred': LOG.debug(msg) elif acct_id_ep_mode == 'required': @@ -521,31 +509,35 @@ def _validate_account_id_endpoint_mode(self): ) raise InvalidConfigError(error_msg=error_msg) - def _create_no_account_id_message(self): - acct_id_ep_mode = self._account_id_endpoint_mode - method = self._credentials.method - base_msg = ( - f'"account_id_endpoint_mode" is set to {acct_id_ep_mode}, ' - f'but account ID could not be resolved from source "{method}". ' + +class EndpointBuiltinResolver: + """Resolves endpoint builtins during endpoint construction""" + + def __init__(self, builtin_resolvers): + self._builtin_resolvers = builtin_resolvers + + def resolve(self, param_definitions, builtins): + """Resolve endpoint builtins""" + self._resolve_credential_builtins(param_definitions, builtins) + + def _resolve_credential_builtins(self, param_definitions, builtins): + self._resolve_account_id_builtin(param_definitions, builtins) + + def _resolve_account_id_builtin(self, param_definitions, builtins): + builtin_configured = self._builtin_configured( + param_definitions, 'AccountId' ) - if method in self.STATIC_CREDENTIAL_METHODS: - msg = ( - f'{base_msg} Credential source "{method}" does not have ' - 'an account ID associated with it. Please check your ' - 'configuration and try again.' - ) - elif method in self.REFRESHABLE_CREDENTIAL_METHODS: - msg = ( - f'{base_msg} Please ensure that your credential ' - 'source is configured to return an account ID.' - ) - else: - supported = ", ".join(self.SUPPORTED_CREDENTIAL_METHODS) - msg = ( - f'{base_msg} Please change your credential source to one of: ' - f'{supported} or set "account_id_endpoint_mode" to "disabled".' - ) - return msg + acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID + current_builtin_value = builtins.get(acct_id_builtin_key) + credential_resolver = self._builtin_resolvers['credentials'] + account_id = credential_resolver.resolve_account_id_builtin( + builtin_configured, current_builtin_value + ) + builtins[acct_id_builtin_key] = account_id + + def _builtin_configured(self, param_definitions, param_name): + param_def = param_definitions.get(param_name) + return param_def is not None and param_def.builtin is not None class EndpointRulesetResolver: @@ -561,7 +553,7 @@ def __init__( event_emitter, use_ssl=True, requested_auth_scheme=None, - credential_builtin_resolver=None, + builtin_resolver=None, ): self._provider = EndpointProvider( ruleset_data=endpoint_ruleset_data, @@ -575,7 +567,7 @@ def __init__( self._use_ssl = use_ssl self._requested_auth_scheme = requested_auth_scheme self._instance_cache = {} - self._credential_builtin_resolver = credential_builtin_resolver + self._builtin_resolver = builtin_resolver def construct_endpoint( self, @@ -644,7 +636,7 @@ def _get_provider_params( customized_builtins = self._get_customized_builtins( operation_model, call_args, request_context ) - self._resolve_credential_builtins(customized_builtins) + self._resolve_builtins(customized_builtins) for param_name, param_def in self._param_definitions.items(): param_val = self._resolve_param_from_context( param_name=param_name, @@ -661,25 +653,11 @@ def _get_provider_params( return provider_params - def _resolve_credential_builtins(self, builtins): - if self._credential_builtin_resolver is None: + def _resolve_builtins(self, builtins): + if self._builtin_resolver is None: return - self._resolve_account_id_builtin(builtins) - - def _resolve_account_id_builtin(self, builtins): - resolver = self._credential_builtin_resolver - builtin_configured = self._builtin_configured('AccountId') - acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID - current_builtin_value = builtins.get(acct_id_builtin_key) - account_id = resolver.resolve_account_id_builtin( - builtin_configured, current_builtin_value - ) - builtins[acct_id_builtin_key] = account_id - - def _builtin_configured(self, param_name): - param_def = self._param_definitions.get(param_name) - return param_def is not None and param_def.builtin is not None + self._builtin_resolver.resolve(self._param_definitions, builtins) def _resolve_param_from_context( self, param_name, operation_model, call_args diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 093dfe153e..fccbd44f4b 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -38,6 +38,7 @@ from botocore.loaders import Loader from botocore.regions import ( CredentialBuiltinResolver, + EndpointBuiltinResolver, EndpointResolverBuiltins, EndpointRulesetResolver, ) @@ -567,9 +568,12 @@ def create_ruleset_resolver( ): service_model = Mock() service_model.client_context_parameters = [] - credential_builtin_resolver = CredentialBuiltinResolver( - credentials, account_id_endpoint_mode - ) + builtin_resolvers = { + "credentials": CredentialBuiltinResolver( + credentials, account_id_endpoint_mode + ) + } + builtin_resolver = EndpointBuiltinResolver(builtin_resolvers) return EndpointRulesetResolver( endpoint_ruleset_data=ruleset, partition_data={}, @@ -577,7 +581,7 @@ def create_ruleset_resolver( builtins=bulitins, client_context=None, event_emitter=Mock(), - credential_builtin_resolver=credential_builtin_resolver, + builtin_resolver=builtin_resolver, ) @@ -686,32 +690,17 @@ def test_account_id_endpoint_mode_input_error_cases( ) -@pytest.mark.parametrize( - "credential_method, expected_message", - [ # refreshable method - ("assume-role", "ensure that your credential source is configured"), - # static method - ("env", "check your configuration and try again"), - # unsupported method - ("foo", "change your credential source to one of"), - ], -) def test_required_mode_no_account_id( account_id_ruleset, operation_model_empty_context_params, - credential_method, - expected_message, ): - credentials = Credentials( - access_key="a", secret_key="b", token="c", method=credential_method - ) resolver = create_ruleset_resolver( account_id_ruleset, BUILTINS_WITH_UNRESOLVED_ACCOUNT_ID, - credentials, + Credentials(access_key="a", secret_key="b", token="c"), REQUIRED, ) - with pytest.raises(AccountIdNotFound, match=expected_message): + with pytest.raises(AccountIdNotFound): resolver.construct_endpoint( operation_model=operation_model_empty_context_params, request_context={}, From b7088d62cafb0fa524441dcfcb12a105d1abf562 Mon Sep 17 00:00:00 2001 From: davidlm Date: Thu, 2 Nov 2023 20:15:18 -0400 Subject: [PATCH 25/29] fix quotes --- tests/unit/test_endpoint_provider.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index fccbd44f4b..2c8313537f 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -556,9 +556,9 @@ def operation_model_empty_context_params(): token="token", account_id="1234567890", ) -REQUIRED = 'required' -PREFERRED = 'preferred' -DISABLED = 'disabled' +REQUIRED = "required" +PREFERRED = "preferred" +DISABLED = "disabled" URL_NO_ACCOUNT_ID = "https://amazonaws.com" URL_WITH_ACCOUNT_ID = "https://1234567890.amazonaws.com" From 9d3deab2e2098b8260d0a2d624e357b22df3fa31 Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 3 Nov 2023 10:21:22 -0400 Subject: [PATCH 26/29] change variable name --- botocore/args.py | 4 ++-- botocore/regions.py | 6 +++--- tests/unit/test_endpoint_provider.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index 55f7b9f79d..50feac68ac 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -626,8 +626,8 @@ def _construct_builtin_resolver(self, credentials, client_config): ) # This only includes CredentialBuiltinResolver for now, but may grow # as more endpoint builtins are added. - builtin_resolvers = {'credentials': credential_builtin_resolver} - return EndpointBuiltinResolver(builtin_resolvers) + resolver_map = {'credentials': credential_builtin_resolver} + return EndpointBuiltinResolver(resolver_map) def _build_endpoint_resolver( self, diff --git a/botocore/regions.py b/botocore/regions.py index 9857f0d642..e66ff303f2 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -513,8 +513,8 @@ def _validate_account_id_endpoint_mode(self): class EndpointBuiltinResolver: """Resolves endpoint builtins during endpoint construction""" - def __init__(self, builtin_resolvers): - self._builtin_resolvers = builtin_resolvers + def __init__(self, resolver_map): + self._resolver_map = resolver_map def resolve(self, param_definitions, builtins): """Resolve endpoint builtins""" @@ -529,7 +529,7 @@ def _resolve_account_id_builtin(self, param_definitions, builtins): ) acct_id_builtin_key = EndpointResolverBuiltins.AWS_ACCOUNT_ID current_builtin_value = builtins.get(acct_id_builtin_key) - credential_resolver = self._builtin_resolvers['credentials'] + credential_resolver = self._resolver_map['credentials'] account_id = credential_resolver.resolve_account_id_builtin( builtin_configured, current_builtin_value ) diff --git a/tests/unit/test_endpoint_provider.py b/tests/unit/test_endpoint_provider.py index 2c8313537f..9d5c5eab91 100644 --- a/tests/unit/test_endpoint_provider.py +++ b/tests/unit/test_endpoint_provider.py @@ -568,12 +568,12 @@ def create_ruleset_resolver( ): service_model = Mock() service_model.client_context_parameters = [] - builtin_resolvers = { + resolver_map = { "credentials": CredentialBuiltinResolver( credentials, account_id_endpoint_mode ) } - builtin_resolver = EndpointBuiltinResolver(builtin_resolvers) + builtin_resolver = EndpointBuiltinResolver(resolver_map) return EndpointRulesetResolver( endpoint_ruleset_data=ruleset, partition_data={}, From c6b2d0f00d6f9d506565e20e8fce28253e40de1d Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 3 Nov 2023 10:31:07 -0400 Subject: [PATCH 27/29] split account ID refresh into two tests --- tests/unit/test_credentials.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index a3c7730a91..46e8990937 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -190,16 +190,13 @@ def test_refresh_returns_partial_credentials(self): with self.assertRaises(botocore.exceptions.CredentialRetrievalError): self.creds.access_key - def test_account_id_refresh(self): - # set the expiry to now so the creds will need a refresh again - now = datetime.now(tzlocal()) - self.mock_time.return_value = now - metadata = self.metadata.copy() - metadata['expiry_time'] = now.isoformat() - self.refresher.return_value = metadata + def test_account_id_unset(self): + self.mock_time.return_value = datetime.now(tzlocal()) self.assertTrue(self.creds.refresh_needed()) self.assertIsNone(self.creds.account_id) - # set the account id in the mocked refresher + + def test_account_id_set(self): + self.mock_time.return_value = datetime.now(tzlocal()) metadata = self.metadata.copy() metadata['account_id'] = '123456789012' self.refresher.return_value = metadata From 9febc3c6ae31b39acc896d3ef357ef79a5bf792a Mon Sep 17 00:00:00 2001 From: davidlm Date: Fri, 3 Nov 2023 12:34:12 -0400 Subject: [PATCH 28/29] add creds method to message --- botocore/regions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/botocore/regions.py b/botocore/regions.py index e66ff303f2..0c419ab4be 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -490,7 +490,8 @@ def resolve_account_id_builtin(self, builtin_configured, builtin_value): if account_id is None: msg = ( f'"account_id_endpoint_mode" is set to "{acct_id_ep_mode}", ' - 'but account ID could not be resolved.' + 'but account ID could not be resolved. Retrieved credentials ' + f'using: "{self._credentials.method}".' ) if acct_id_ep_mode == 'preferred': LOG.debug(msg) From eccd175d9ea6b7cb188204f110c639893b5668e5 Mon Sep 17 00:00:00 2001 From: davidlm Date: Mon, 6 Nov 2023 14:50:02 -0500 Subject: [PATCH 29/29] pr feedback --- botocore/args.py | 2 -- botocore/regions.py | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index 50feac68ac..94f6501b31 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -624,8 +624,6 @@ def _construct_builtin_resolver(self, credentials, client_config): credential_builtin_resolver = CredentialBuiltinResolver( credentials, client_config.account_id_endpoint_mode ) - # This only includes CredentialBuiltinResolver for now, but may grow - # as more endpoint builtins are added. resolver_map = {'credentials': credential_builtin_resolver} return EndpointBuiltinResolver(resolver_map) diff --git a/botocore/regions.py b/botocore/regions.py index 0c419ab4be..07c6c1f036 100644 --- a/botocore/regions.py +++ b/botocore/regions.py @@ -512,13 +512,13 @@ def _validate_account_id_endpoint_mode(self): class EndpointBuiltinResolver: - """Resolves endpoint builtins during endpoint construction""" + """Resolves endpoint builtins during endpoint construction.""" def __init__(self, resolver_map): self._resolver_map = resolver_map def resolve(self, param_definitions, builtins): - """Resolve endpoint builtins""" + """Resolve endpoint builtins.""" self._resolve_credential_builtins(param_definitions, builtins) def _resolve_credential_builtins(self, param_definitions, builtins):