Skip to content
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

Added ProgrammaticAssumeRoleProvider and Session.assume_role() for #761 #2096

Closed
wants to merge 1 commit into from

Conversation

benkehoe
Copy link

@benkehoe benkehoe commented Jul 3, 2020

Issue #761 asks for first-class support for sts:AssumeRole, where there is a Session.assume_role() method that produces another session (boto3.Session would then also get an assume_role() method). To implement this, the existing credentials.AssumeRoleProvider seemed too intimately tied to loading credentials from config, in particular there did not seem to be a good way of injecting an arbitrary Credentials object, so I added a new, simpler ProgrammaticAssumeRoleProvider class that mainly just wraps the AssumeRoleCredentialFetcher. I've replicated the unit tests for AssumeRoleProvider except those that relate to configuration files.

I'm hoping to get feedback on whether this is a contribution that can feasibly be accepted before I add the functional tests and go to the effort of figuring out how the integration testing works.

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2020

Codecov Report

Merging #2096 into develop will decrease coverage by 0.01%.
The diff coverage is 85.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2096      +/-   ##
===========================================
- Coverage    93.10%   93.09%   -0.02%     
===========================================
  Files           60       60              
  Lines        11101    11128      +27     
===========================================
+ Hits         10336    10360      +24     
- Misses         765      768       +3     
Impacted Files Coverage Δ
botocore/session.py 97.10% <20.00%> (-1.03%) ⬇️
botocore/credentials.py 98.67% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b515ea8...b07933e. Read the comment docs.

@benkehoe
Copy link
Author

Pinging on this. Would love to make my first contribution to the Python SDK!

@tim-finnigan
Copy link
Contributor

Hi @benkehoe, thank you for the PR and sorry for the wait. I discussed this with the team and will share some points that came up.

Currently we don’t provide public interfaces for any CredentialProviders in botocore. To support that would require a substantial refactoring of the codebase.

The implementation here doesn’t follow our existing CredentialProvider workflows and involves replacing the existing credential chain with an overwritten one. That makes the session inoperable for any credentials except the ARN used to assume the role. The current change also doesn't account for the use of profiles, which means the new session will be broken for common configuration cases.

For those reasons we think that currently it makes more sense for this to exist as a third-party plugin. But we may revisit this in a future refactoring or major version.

@benkehoe
Copy link
Author

benkehoe commented Apr 15, 2022

Thanks for following up. It's true that botocore doesn't today provide programmatic credential definitions, but this is generally not true of other SDKs, e.g., JavaScript (even in v2). It's unfortunate that Python doesn't get the same capability.

For the record, though, I wanted to respond to this part:

The implementation here doesn’t follow our existing CredentialProvider workflows and involves replacing the existing credential chain with an overwritten one. That makes the session inoperable for any credentials except the ARN used to assume the role. The current change also doesn't account for the use of profiles, which means the new session will be broken for common configuration cases.

This is intentional. The explicit purpose of this credential provider is to provide a programmatic way to get a new session that uses an assumed role based on a parent session. This new session should not be able to pick up any other credentials. Such a session can only be created through Python code, not through configuration (there's already a credential provider for that). For that reason it uses a CredentialResolver that only uses the programmatic assumed role credential provider. It is similar to how a boto3 Session created with explicit credentials will not ever use the credential resolver. Would you be more amenable to this change if Session.assume_role() created the DeferredRefreshableCredentials directly and set them on the newly-created child session's Session._credentials field, like boto3.Session does with explicit credentials?

@tim-finnigan
Copy link
Contributor

Hi @benkehoe thanks for sharing those points. Unfortunately the team is still not considering the proposed changes at this time. We could make changes to more tightly integrate this with the existing interfaces, but this would still be a special case feature that we’d need to maintain outside of the normal credential flow.

Some SDKs may have some form of this feature as you mentioned, but generally speaking we are trying to move toward more standardization of feature implementation across SDKs. Adding another different approach here in botocore would take us further from alignment.

But we will continue recommending your aws-assume-role-lib library to those who could benefit from using it. Also this PR prompted a broader team discussion on how boto3 could better support/incorporate third-party libraries and plugins such as that one. If you have any thoughts to share regarding that please let us know.

@benkehoe
Copy link
Author

benkehoe commented Apr 26, 2022

we are trying to move toward more standardization of feature implementation across SDKs.

As part of that, please, please include programmatic configuration as a standardized feature.

I have one last-ditch proposal. What if we added a new CredentialProvider subclass to the default chain, one that read from the session config and relied on the configuration values to present, e.g., role_arn, but instead of source_profile, it had to be source_credentials_object and be a Credentials object? So creating an assumed role session would be like this:

parent_session = botocore.Session()
config = {
  "role_arn": "arn:aws:...",
  "source_credentials_object": parent_session.get_credentials()
}
assumed_role_session = botocore.Session(session_vars=config)

This credential provider would never get activated by existing config, and this config would never activate existing credential providers. Then that session creation would get wrapped up in a Session.assume_role() method.

My concern with AWS recommending 3rd party libraries like aws-assume-role-lib and requests-aws4auth (because botocore doesn't expose request signing, see #1784) is that it encourages broader software supply chains for code that directly touches credentials. Like, in the absence of botocore stepping up to the plate, I guess it's the next best thing, but I'd much rather be able to recommend that people limit their dependencies for SDK credential handling to code coming directly from AWS. But in general, an AWS-curated list of plugin/extensions/etc. (with a healthy warning label) would be a good thing, I think.

@benkehoe
Copy link
Author

benkehoe commented May 3, 2022

I've been a fool. This implementation is needlessly overcomplicated, because I missed how static credentials are used in a botocore Session. If a boto3.Session is allowed to set the _credentials field of a botocore.Session, I don't need any changes in botocore at all. If not, all I need is a change to the set_credentials() method to allow a Credentials object directly. I am going to close this PR, implement it that way on boto3, and open a PR there for further discussion.

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.

None yet

3 participants