Add AWS Process Credential Resolver - #658
Conversation
0e39eb8 to
246f957
Compare
Replace ProcessCredentialsConfig with a keyword-only timeout argument on ProcessCredentialsResolver. Default to no timeout to match the SEP and existing AWS SDK behavior.
Wrap non-UTF-8 stdout and malformed Expiration in SmithyIdentityError, drop the redundant version None check, and fall back to the profile's aws_account_id when the process output omits AccountId.
e341b1c to
0c48d90
Compare
jonathan343
left a comment
There was a problem hiding this comment.
I'm bringing this PR back to life now that we have a foundation for the default credentials chain and config resolution. I've adapted this to the pattern introduced in #749.
| return shlex.split(command) | ||
|
|
||
|
|
||
| def _split_windows_command(command: str) -> list[str]: |
There was a problem hiding this comment.
Note for reviewer: This is inspired by botocore's _windows_shell_split utility function.
| from .conftest import OtherIdentity | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Note for reviewer:
Most cases are ported from botocore: https://github.com/boto/botocore/blob/develop/tests/unit/test_compat.py#L111-L134
I also added two realistic Windows path cases
arandito
left a comment
There was a problem hiding this comment.
Thanks @jonathan343! This PR looks mostly good to me.
Just had a couple comments related to naming and error handling.
Let me know if you have any questions!
| argument_started = True | ||
|
|
||
| if in_quotes: | ||
| raise ValueError(f"No closing quotation in string: {command}") |
There was a problem hiding this comment.
Some of the in-flight network providers introduce the concept of <ResolverName>ConfigurationError to make these errors surface more descriptively. Can we have a ProcessConfigurationError or something similar?
| return arguments | ||
|
|
||
|
|
||
| class ProfileProcessCredentialsProvider: |
There was a problem hiding this comment.
The naming convention for providers is currently <CanonicalName>Provider since they directly map to a standard slot in the chain.
Can we rename this to ProfileCredentialProcessProvider?
| # credentials. Suppress chaining to avoid exposing it in tracebacks. | ||
| try: | ||
| decoded = stdout.decode("utf-8") | ||
| creds = json.loads(decoded) |
There was a problem hiding this comment.
nit: json.loads can succeed for non-dict outputs like a list or string.
Can we add an isinstance(creds, dict) guard to raise a cleaner error? This would prevent a vague AttributeError: 'list' object has no attribute 'get' error from propagating up the chain.
| "AccessKeyId and SecretAccessKey are required for process credentials" | ||
| ) | ||
|
|
||
| self._credentials = AWSCredentialsIdentity( |
There was a problem hiding this comment.
nit: Should we also validate that the fields are strings before creating the identity object? Without it, the resolver sets non-string values that would raise at request-signing time.
Overview
This PR adds AWS process-based credential support to
smithy-aws-core. It introduces aProcessCredentialsResolverthat sources credentials from an external command, and wires it into the default AWS identity chain so that a profile'scredential_processsetting is automatically honored. It also handles the common failure modes and caching behavior expected for both long-lived and temporary credentials.Testing
Note
This was tested with internal AWS process tools for sourcing credentials. TODO: Test with popular external tools.
The identity chain is currently opt-in. We'll construct one and set it as the config's credentials resolver. Once set, credentials sourced from a
credential_processprofile are resolved through the chain:Example Script:
The resolver can also be used directly, bypassing the chain and profile configuration entirely:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.