Skip to content

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

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

alextwoods
Copy link
Contributor

@alextwoods alextwoods commented Mar 18, 2025

Description of changes:
Implements a configurable credentials resolver chain and sets it as the default identity resolver in generated Config.

The credential chain is configured by providing tuples of callables for each credential source, the first returns true if credentials are available from that source, the second is a constructor for that source.

Individual credential resolvers can be constructed from multiple sources (eg: web identity token from environment or profile) - having the sources be defined using callable's allows us to de-couple the different sources for a given credential resolver from the implementation of the resolver itself.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alextwoods alextwoods marked this pull request as ready for review March 20, 2025 16:30
@alextwoods alextwoods requested a review from a team as a code owner March 20, 2025 16:31
Comment on lines 46 to 47
if source[0]():
self._credentials_resolver = source[1]()
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@alextwoods alextwoods Mar 24, 2025

Choose a reason for hiding this comment

The 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:

  1. static credentials from ENV
  2. Web identity from ENV
  3. static credentials from profile
  4. web identity from profile.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combining threads here - I'm not convinced that this Source concept is worth it. I see this as adding complexity unnecessarily. What I want to see is something dead simple, like:

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

  1. Availability and resolution are separate concerns - as an example - if a credential_process is configured, the process resolver should be considered "available" and should be used by the chain even if executing that processes fails/doesn't return credentials. (ie, we wouldn't move on to the next resolver if a configured credential process fails).
  2. Sources don't map 1:1 to resolvers - resolvers like Assume role or web identity can be configured multiple different ways at different levels of precedence - ie, they are different sources.
  3. Separation of concerns - configuration sources vs resolver properties. How and where we source configuration generally shouldn't be a concern for an individual resolver - that is, I'd argue that something like a web identity resolver shouldn't need to be concerned with where/how the token file is specified (env, shared config, ect) - it should just know how to use that.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. That's news to me. Why not? And there's ways around that which aren't as complicated, such as having different exception types.
  2. If they don't, they need to be refactored until they do. If you have a resolver that checks three different areas that can't be run sequentially then it's dangerous to run at any point.
  3. I don't agree. The purpose of a credential resolver is to source credentials from some method of configuration. Constructor arguments can be used to narrow their search if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With something like a configured credential_process- it was explicitly configured and I'd argue that it should be treated as authoritative if present. However, looking at existing SDK behavior, this seems to be underspecified and likely inconsistent across SDKs - both the Ruby and Java SDKs will resolve process credentials and fail during a request, surfacing the command failure in that case, but I'm guessing there are other SDKs that might move on in the chain....

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.

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor docstring change, otherwise I think this looks good. @jonathan343 did you have anything else you wanted to see?

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the two wording issues and otherwise code looks good to me!

alextwoods and others added 2 commits March 27, 2025 09:20
…vers/imds.py

Co-authored-by: Nate Prewitt <nate.prewitt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants