-
Notifications
You must be signed in to change notification settings - Fork 28
Add AWS Process Credential Resolver #658
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 all commits
d88aee5
de84579
287f3ae
5d7f607
246f957
e341b1c
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 | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,115 @@ | ||||||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||
| import asyncio | ||||||
| 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: | ||||||
|
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 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.
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. 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], | ||||||
|
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. The PR description says we will accept a 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: | ||||||
|
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. 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() | ||||||
|
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. Did you intend to have this indented? It's fine either way, but if |
||||||
| 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')}" | ||||||
|
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. Should we include the returncode in the exception message in case it is useful in debugging?
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. 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}" | ||||||
|
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. [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 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: | ||||||
|
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.
Suggested change
Nit: the |
||||||
| 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) | ||||||
|
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 does this look like when we have an invalid string (for instance That will raise |
||||||
| 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 | ||||||
There was a problem hiding this comment.
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?