-
Notifications
You must be signed in to change notification settings - Fork 29
Implement a configurable credentials resolver chain #452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 3 commits
2cd5625
368fd21
87f73ba
c33518b
e5b55cc
4351de1
b325e70
d14e85e
086d9ce
ef5e169
b7412fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,12 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from .environment import EnvironmentCredentialsResolver | ||
| from .static import StaticCredentialsResolver | ||
| from .credentials_resolver_chain import CredentialsResolverChain | ||
|
|
||
| __all__ = ("EnvironmentCredentialsResolver", "StaticCredentialsResolver") | ||
| __all__ = ( | ||
| "EnvironmentCredentialsResolver", | ||
| "StaticCredentialsResolver", | ||
| "CredentialsResolverChain", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| from typing import Callable, List | ||
|
|
||
| from smithy_aws_core.credentials_resolvers import EnvironmentCredentialsResolver | ||
| from smithy_aws_core.identity import AWSCredentialsIdentity, AWSCredentialsResolver | ||
| from smithy_core.aio.interfaces.identity import IdentityResolver | ||
| from smithy_core.exceptions import SmithyIdentityException | ||
| from smithy_core.interfaces.identity import IdentityProperties | ||
|
|
||
| import os | ||
|
|
||
|
|
||
| def _env_creds_available() -> bool: | ||
| return bool(os.getenv("AWS_ACCESS_KEY_ID")) and bool( | ||
| os.getenv("AWS_SECRET_ACCESS_KEY") | ||
| ) | ||
|
alextwoods marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| def _build_env_creds() -> AWSCredentialsResolver: | ||
| return EnvironmentCredentialsResolver() | ||
|
|
||
|
|
||
| type CredentialSource = tuple[Callable[[], bool], Callable[[], AWSCredentialsResolver]] | ||
| _DEFAULT_SOURCES: list[CredentialSource] = [(_env_creds_available, _build_env_creds)] | ||
|
|
||
|
|
||
| class CredentialsResolverChain( | ||
| IdentityResolver[AWSCredentialsIdentity, IdentityProperties] | ||
| ): | ||
| """Resolves AWS Credentials from system environment variables.""" | ||
|
|
||
| def __init__(self, *, sources: List[CredentialSource] | None = None): | ||
|
alextwoods marked this conversation as resolved.
Outdated
|
||
| if sources is None: | ||
| sources = _DEFAULT_SOURCES | ||
| self._sources: List[CredentialSource] = sources | ||
| self._credentials_resolver: AWSCredentialsResolver | None = None | ||
|
|
||
| async def get_identity( | ||
| self, *, identity_properties: IdentityProperties | ||
| ) -> AWSCredentialsIdentity: | ||
| if self._credentials_resolver is not None: | ||
| return await self._credentials_resolver.get_identity( | ||
| identity_properties=identity_properties | ||
| ) | ||
|
|
||
| for source in self._sources: | ||
| if source[0](): | ||
| self._credentials_resolver = source[1]() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are we actually calling here 😅 A state check and then a builder for the resolver? I think this works, I'm wondering if we want to actually make a class for the abstraction rather than composing lists of functions. This is probably minor at this point, but may be worth spending some time on ergonomics later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - your right that this is a little confusing - I was hoping the typing itself would be sufficient... but its not. This is separating "is available" from construction for a given credential source. We first call an "is available" callable to see if we should construct credentials from a given source - think cases like SSO or assume role or process credentials, ect where we can cheaply identify when the credentials are available (by checking the presence of env/config values), but actually constructing a resolver might be more expensive.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're concerned about perf for instantiation, you can always have a class method on the class reference. That can be used to check availability and then you have the class there to instantiate if we're ready. It's essentially the same abstraction just wrapped into a single input.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other consideration in this is that construction of a given resolver is based on the source - and its not always a 1:1 mapping and the order of precedence is important. As an example: assuming a role from a web identity. Assuming a role from a web identity token file can be specified either through the environment or through the shared config file and the order of precedence (limited just to static credentials and web identity) is:
In addition RE: construction, the resolver chain may need to construct certain resolvers with different options than we'd typically specify by default - an example of this is IMDS credentials. Since IMDS is the last option and is always checked, we usually use lower retries and timeouts to have it fail faster. Edit: I do think a class/protocol interface makes a lot more sense. I'm working on an update to that, but I think the above answer still applies to why we might not just want to add an is_available method to AWS Credentials Resolvers.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See commit for defining source as a class/Protocol - definitely easier to understand/use I think.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combining threads here - I'm not convinced that this class ChainedIdentityResolver[I](IdentityResolver[I]):
def __init__(self, resolvers: Sequence[IdentityResolver[I]]) -> None:
self._resolvers = resolvers
async def get_identity(self, *, properties: _TypedProperties) -> I:
for resolver in self._resolvers:
try:
return await resolver.get_identity(properties=properties)
except SmithyIdentityException as e:
logger.debug(
"Failed to resolve identity from %s: %s", type(resolver), e
)
raise SmithyIdentityException("Failed to resolve identity from resolver chain.")I'm currently working on changing the interfaces here, so maybe we should just punt on this for now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the resolvers we currently have (env and IMDS) that approach is fine. I think theres three reasons it doesn't work as simply as that for the full aws credentials resolver chain
Again though - I think these are longer term concerns, a simple sequence of resolvers is sufficient for what is available now so I'm happy to drop this PR and leave it to future design work to describe how this should work.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With something like a configured We could potentially use different exception types to signal whether credentials are applicable/configured vs invalid, but generally I'd prefer not to use exceptions for control flow. On separation of concerns, I see resolvers as consumers of configuration rather than being responsible for doing the resolution of configuration - I think making an identity resolver responsible both for resolving configuration and for then resolving an identity from it increases the complexity and mixes two separate concerns. That said again, all of those are longer term concerns I think and with the ENV/IMDS resolvers only, sources are unnecessary and I'm happy to drop this PR for a simple sequence of resolvers. |
||
| return await self._credentials_resolver.get_identity( | ||
| identity_properties=identity_properties | ||
| ) | ||
|
|
||
| raise SmithyIdentityException( | ||
| "None of the configured credentials sources were able to resolve credentials." | ||
| ) | ||
|
jonathan343 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import pytest | ||
|
|
||
| from smithy_aws_core.credentials_resolvers import ( | ||
| CredentialsResolverChain, | ||
| StaticCredentialsResolver, | ||
| ) | ||
| from smithy_aws_core.identity import AWSCredentialsIdentity | ||
| from smithy_core.exceptions import SmithyIdentityException | ||
| from smithy_core.interfaces.identity import IdentityProperties | ||
|
|
||
|
|
||
| async def test_no_sources_resolve(): | ||
| resolver_chain = CredentialsResolverChain(sources=[]) | ||
| with pytest.raises(SmithyIdentityException): | ||
| await resolver_chain.get_identity(identity_properties=IdentityProperties()) | ||
|
|
||
|
|
||
| async def test_env_credentials_resolver_not_set(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.delenv("AWS_ACCESS_KEY_ID", raising=False) | ||
| monkeypatch.delenv("AWS_SECRET_ACCESS_KEY", raising=False) | ||
| resolver_chain = CredentialsResolverChain() | ||
|
|
||
| with pytest.raises(SmithyIdentityException): | ||
| await resolver_chain.get_identity(identity_properties=IdentityProperties()) | ||
|
|
||
|
|
||
| async def test_env_credentials_resolver_partial(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setenv("AWS_ACCESS_KEY_ID", "akid") | ||
| monkeypatch.delenv("AWS_SECRET_ACCESS_KEY", raising=False) | ||
| resolver_chain = CredentialsResolverChain() | ||
|
|
||
| with pytest.raises(SmithyIdentityException): | ||
| await resolver_chain.get_identity(identity_properties=IdentityProperties()) | ||
|
|
||
|
|
||
| async def test_env_credentials_resolver_success(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setenv("AWS_ACCESS_KEY_ID", "akid") | ||
| monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "secret") | ||
| resolver_chain = CredentialsResolverChain() | ||
|
|
||
| credentials = await resolver_chain.get_identity( | ||
| identity_properties=IdentityProperties() | ||
| ) | ||
| assert credentials.access_key_id == "akid" | ||
| assert credentials.secret_access_key == "secret" | ||
|
|
||
|
|
||
| async def test_custom_sources_with_static_credentials(): | ||
| static_credentials = AWSCredentialsIdentity( | ||
| access_key_id="static_akid", | ||
| secret_access_key="static_secret", | ||
| ) | ||
| static_resolver = StaticCredentialsResolver(credentials=static_credentials) | ||
| resolver_chain = CredentialsResolverChain( | ||
| sources=[(lambda: False, lambda: None), (lambda: True, lambda: static_resolver)] # type: ignore | ||
| ) | ||
|
|
||
| credentials = await resolver_chain.get_identity( | ||
| identity_properties=IdentityProperties() | ||
| ) | ||
| assert credentials.access_key_id == "static_akid" | ||
| assert credentials.secret_access_key == "static_secret" |
Uh oh!
There was an error while loading. Please reload this page.