Credential Scope

[dlm6693]

This PR adds a scope to credentials that is used for endpoint resolution through the AWS::Auth::CredentialScope builtin parameter. Similar to account ID, it is treated as completely optional and only set on credential fetchers if the value is present. A couple of properties are added to the endpoint builtin resolver and piped through to the ruleset resolver in order to raise a configuration warning for customers that still rely on legacy behavior.

The base branch is https://github.com/boto/botocore/tree/account-id-endpoints since this PR is largely built on top of that branch and both will be merged at the same time. Once this is approved, account ID will be merged, this branch will be rebased with those changes and the base branch will be updated to https://github.com/boto/botocore/tree/develop/

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eccd175) 93.50% compared to head (78a0505) 93.53%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           account-id-endpoints    #3048      +/-   ##
========================================================
+ Coverage                 93.50%   93.53%   +0.02%     
========================================================
  Files                        66       66              
  Lines                     14129    14176      +47     
========================================================
+ Hits                      13212    13259      +47     
  Misses                      917      917              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nateprewitt]

We should avoid instantiating an actual Session and use BaseSessionTest or something similar. We can't easily control the state of the Session in diverse environments which will lead to unexpected failures (or worse successes).

[dlm6693]

We need to use something similar. Unfortunately both patched_session and BaseSessionTest rely on a cached Loader instance which cannot be used for this specific case

[nateprewitt]

Could we just do this to avoid redoing what's happening in the super() call already?

Suggested change

	super().setUp()
	self.tempdir = tempfile.TemporaryDirectory()
	self.environ['AWS_DATA_PATH'] = self.tempdir.name
	self.session = Session()
	self.session.set_config_variable(
	'credentials_file', 'noexist/foo/botocore'
	)
	self.session.config_filename = 'no-exist-foo'
	self.tempdir = tempfile.TemporaryDirectory()
	super().setUp(
	environ={'AWS_DATA_PATH': self.tempdir.name}
	)

Why did we need to override the credentials_file in this case? We shouldn't be using a profile and are passing hardcoded credentials here.

[dlm6693]

Discussed offline. The point of this was to mimic everything we're doing in BaseSessionTest and create_session. We needed to create our own session because the method uses a cached Loader instance which does not have the custom data path attached to it. However, since there are other tests that check that the env vars work as expected, we're going to modify create_session instead to pass a custom loader with the data path attached to it.

[nateprewitt]

Is there something interchangeable about this test that allows for mixed results, or is it testing that we use process credentials over a supplied profile? Ideally, we'd have the latter behavior tested once.

[dlm6693]

The latter. However, I think this is maybe the only case where credential values can be retrieved from more than one source. I don't know if it's a guarantee that all future credential values will share this behavior though so we may want to keep the individual checks. Here is the wording in the spec for account ID. Behavior for scope is expected to be the same. Note that it does not say to fall back to all credentials in the config/credentials file, just the individual value.

If the process credentials provider is provisioned through the config file, and successful 
credential response from the process as described above does not contain a value for account ID, 
SDKs SHOULD attempt to resolve the value of the profile property aws_account_id and store it.

[SamRemis]

So maybe this is a pythonic thing that I don't really understand; it seems odd to allow a method that we pass in to be able to set arbitrary attributes.

[dlm6693]

I wouldn't call it Pythonic, maybe Botonic™️? We have precedence for iterating over a list of resolvers that arbitrarily resolve credentials. However, in that case the first one that is successful terminates the loop whereas this one will always complete the loop.

Only endpoint builtins that are defined in the EndpointResolverBuiltins enum can be used as an input parameter to the the endpoint resolver. So if somebody decided to inject a custom resolver that set some arbitrary key value pair {'foo': 'bar'} in the builtins dict at runtime, an error will be raised.

[nateprewitt]

Previously, we'd discussed simplifying this a bit. Since _builtin_configured will always be a required check, can we not make that part of the default loop instead of requiring calling in each of these sub-functions. It also seems like we created this abstraction to work around the _account_id_endpoint_mode being disabled. Should we not handle that separately instead of having all future options require this overhead?

It may be premature optimization to break this up so much. Was there a reason this approach was kept?

[dlm6693]

We can. I think I opted to keep it because it is similar syntactically to the _validate_builtin logic and avoids the use of conditionals. Additionally we will need to update the mapping to use the pascal case AccountId and CredentialScope. I'll have something up soon.

[nateprewitt]

Resolving for inclusion at a later date.