Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from .container import ContainerCredentialsResolver
from .environment import EnvironmentCredentialsResolver
from .imds import IMDSCredentialsResolver
from .process import ProcessCredentialsConfig, ProcessCredentialsResolver
from .static import StaticCredentialsResolver

__all__ = (
Expand All @@ -20,6 +21,8 @@
"ContainerCredentialsResolver",
"EnvironmentCredentialsResolver",
"IMDSCredentialsResolver",
"ProcessCredentialsConfig",
"ProcessCredentialsResolver",
"StaticCredentialsResolver",
)

Expand Down
115 changes: 115 additions & 0 deletions packages/smithy-aws-core/src/smithy_aws_core/identity/process.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: Apache-2.0
import asyncio
Copy link
Contributor

Choose a reason for hiding this comment

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

High level: I'm starting to get a little concerned with all of the things we are missing since we don't have support for them yet and how we will track them. In this case, it's feature id tracking and adding this to the default chain (which will need to look in the config file).

Should we add some TODOs here?

import json
from dataclasses import dataclass
from datetime import UTC, datetime

from smithy_core.aio.interfaces.identity import IdentityResolver
from smithy_core.exceptions import SmithyIdentityError

from smithy_aws_core.identity.components import (
AWSCredentialsIdentity,
AWSIdentityProperties,
)

_DEFAULT_TIMEOUT = 30


@dataclass
class ProcessCredentialsConfig:
Copy link
Contributor

@SamRemis SamRemis Mar 18, 2026

Choose a reason for hiding this comment

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

What value does this class add?

Open to discussion, but IMO this should either be a default on the resolver's init method, or we should move the command into this config as well. I'd lean towards putting them both in the init of the resolver.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. It adds another layer of indirection for only one config option. Are the resolver config options likely to grow? I think it may be useful if we expect more options

"""Configuration for process credential retrieval operations."""

timeout: int = _DEFAULT_TIMEOUT


class ProcessCredentialsResolver(
IdentityResolver[AWSCredentialsIdentity, AWSIdentityProperties]
):
"""Resolves AWS Credentials from a process."""

def __init__(
self,
command: list[str],
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR description says we will accept a command string:

Improves command handling so callers can provide either explicit argument lists or a shell-style command string.

Looking at the commit history, it looks like you intentionally removed this capability. I'm alright with this; we can always add support later. What was the reason for the revert?

config: ProcessCredentialsConfig | None = None,
):
if not command:
raise ValueError("command must be a non-empty list")
self._command = list(command)
self._config = config or ProcessCredentialsConfig()
self._credentials = None

async def get_identity(
self, *, properties: AWSIdentityProperties
) -> AWSCredentialsIdentity:
if self._credentials is not None:
# Long-term credentials (no expiration) should always be reused
if self._credentials.expiration is None:
return self._credentials
# Temporary credentials should be reused if not expired
if datetime.now(UTC) < self._credentials.expiration:
Copy link
Contributor

@SamRemis SamRemis Mar 18, 2026

Choose a reason for hiding this comment

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

By default, botocore will force a refresh before 10-15 minutes before expiration; 15 minutes before is optional refresh, 10 minutes before is mandatory.

We don't have the concept of RefreshableCredentials here in this SDK - maybe we should? But for now, does it make sense for us to enforce this?

For instance, if request reaches this within 1 second of expiry, but the process takes 3 seconds, they will be guaranteed to be outdated before we can make the call.

return self._credentials

try:
process = await asyncio.create_subprocess_exec(
*self._command,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
except OSError as e:
raise SmithyIdentityError(f"Credential process failed to start: {e}") from e

try:
stdout, stderr = await asyncio.wait_for(
process.communicate(), timeout=self._config.timeout
)
except TimeoutError as e:
if process.returncode is None:
try:
process.kill()
except ProcessLookupError:
pass
await process.wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to have this indented? It's fine either way, but if process.returncode is not None, then wait will do nothing - the wait is there to make sure the killed process gets reaped but that's already happened if it has a return code, right?

raise SmithyIdentityError(
f"Credential process timed out after {self._config.timeout} seconds"
) from e

if process.returncode != 0:
raise SmithyIdentityError(
f"Credential process failed with non-zero exit code: {stderr.decode('utf-8')}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include the returncode in the exception message in case it is useful in debugging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Second comment here - we are assuming that this will always be parse-able as valid UTF-8.

While any other response encoding would be considered malformed output, a user would see a UnicodeDecodeError instead of the SmithyIdentityError. We should be guarding against other encodings or binary garbage being returned by the unknown process.

)
try:
creds = json.loads(stdout.decode("utf-8"))
except json.JSONDecodeError as e:
raise SmithyIdentityError(
f"Failed to parse credential process output: {e}"
Copy link
Contributor

Choose a reason for hiding this comment

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

[Non-blocking]

JSONDecodeError contains the json document being parsed. The string representation of this generally does not contain the JSON document, but any downstream error parser may wind up leaking any secrets that may have been contained in this json error.

The alternative here is to not raise this error using from e, but just raise it alone.

Since this scenario seems far-fetched, and SDK users should not be sharing raw error info with untrusted users, I'd say this approach is sufficiently secure and helpful for users looking to debug their credentials process. Just wanted to flag the potential concern, but I'm not requesting changes.

) from e

version = creds.get("Version")
if version is None or version != 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if version is None or version != 1:
if version != 1:

Nit: the None check is redundant here since None != 1 returns True.

raise SmithyIdentityError(
f"Unsupported version '{version}' for credential process provider, supported versions: 1"
)
access_key_id = creds.get("AccessKeyId")
secret_access_key = creds.get("SecretAccessKey")
session_token = creds.get("SessionToken")
expiration = creds.get("Expiration")
account_id = creds.get("AccountId")

if isinstance(expiration, str):
dt = datetime.fromisoformat(expiration)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this look like when we have an invalid string (for instance foo)?

That will raise ValueError: Invalid isoformat string, so maybe we need another try/except here.

expiration = dt.astimezone(UTC) if dt.tzinfo else dt.replace(tzinfo=UTC)

if access_key_id is None or secret_access_key is None:
raise SmithyIdentityError(
"AccessKeyId and SecretAccessKey are required for process credentials"
)

self._credentials = AWSCredentialsIdentity(
access_key_id=access_key_id,
secret_access_key=secret_access_key,
session_token=session_token,
expiration=expiration,
account_id=account_id,
)
return self._credentials
Loading
Loading