Skip to content

feat: option to assume role to get credentials#6

Open
elventear wants to merge 1 commit into
jmkeyes:mainfrom
elventear:assume_role
Open

feat: option to assume role to get credentials#6
elventear wants to merge 1 commit into
jmkeyes:mainfrom
elventear:assume_role

Conversation

@elventear

Copy link
Copy Markdown

When fetching credentials after obtaining the boto3 session, we check if the configuration has an optional assume_role field, which indicates that role needs to be assumed to obtain the credentials.

If found, the role is assumed and used to obtain the credentails. If not the original boto3 session is used to obtain the credentials.

@jmkeyes

jmkeyes commented Apr 17, 2025

Copy link
Copy Markdown
Owner

Thanks for making this contribution! I agree that having the option to assume a role would be a good addition to the accepted configuration keys: using temporary credentials is a much better option than static access/secret keys.

@jmkeyes jmkeyes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

/lgtm

The whitespace changes in the diff of the test code made things a bit harder to read, but I can run black later if needed.

Comment thread keyrings/codeartifact.py Outdated
Comment thread keyrings/codeartifact.py Outdated
Comment thread tests/test_backend.py Outdated
Comment thread tests/test_backend.py Outdated
@jmkeyes

jmkeyes commented Apr 17, 2025

Copy link
Copy Markdown
Owner

Not strictly relevant to the PR, but the AWS SDK can also assume a role if configured to do so.

While it is nice to explicitly configure it ourselves here, it begs the question of how much responsibility this library should take in replicating that logic.

This also ties into #8: where do we draw the line between the behaviour of this library (via configuration) and the implicit behaviour of the boto3 library and the AWS SDK?

@elventear

elventear commented Apr 17, 2025

Copy link
Copy Markdown
Author

Not strictly relevant to the PR, but the AWS SDK can also assume a role if configured to do so.

While it is nice to explicitly configure it ourselves here, it begs the question of how much responsibility this library should take in replicating that logic.

This also ties into #8: where do we draw the line between the behaviour of this library (via configuration) and the implicit behaviour of the boto3 library and the AWS SDK?

We have a considerable number of profiles and AWS child accounts that require cross-account authentication. Tightly coupling IAM configuration between identity roles and resource access can become difficult (one reason being IAM policy size limits) and keeping our identity roles loosely coupled from some resource access roles makes maintenance easier.

We could have additional configuration settings for the AWS SDK to assume the roles, but all the combinations we have would create an explosion of configuration entries. Being able to have loose coupling between the identity roles we use and resource roles in the configuration for this library makes management of all these things much simpler for our users.

@elventear elventear requested a review from jmkeyes April 17, 2025 22:03
@jmkeyes

jmkeyes commented Apr 17, 2025

Copy link
Copy Markdown
Owner

Not strictly relevant to the PR, but the AWS SDK can also assume a role if configured to do so.

While it is nice to explicitly configure it ourselves here, it begs the question of how much responsibility this library should take in replicating that logic.

This also ties into #8: where do we draw the line between the behaviour of this library (via configuration) and the implicit behaviour of the boto3 library and the AWS SDK?

We have a considerable number of profiles and AWS child accounts that require cross-account authentication. Tightly coupling IAM configuration between identity roles and resource access can become difficult (one reason being IAM policy size limits) and keeping our identity roles loosely coupled from some resource access roles makes maintenance easier.

We could have additional configuration settings for the AWS SDK to assume the roles, but all the combinations we have would create an explosion of configuration entries. Being able to have loose coupling between the identity roles we use and resource roles in the configuration for this library makes management of all these things much simpler for our users.

I agree.

The first use case for this keyring backend had very little need for configuration at all.

While it is convenient to define access/secret keys in the keyringrc.cfg for a trivial use case doing so is still a bad practice: this library isn't responsible for managing the credentials to access AWS but it should use the existing ones.

For version 3.x of this backend, I'm tempted to drop support for specifying access/secret keys in configuration and explicitly require users to migrate them to ~/.aws/credentials.

This would substantially limit the configuration options that this library needs to be aware of.

@elventear

Copy link
Copy Markdown
Author

@jmkeyes does this look good to merge?

Comment thread keyrings/codeartifact.py Outdated
@jmkeyes

jmkeyes commented May 21, 2025

Copy link
Copy Markdown
Owner

@jmkeyes does this look good to merge?

Almost! Here are some thoughts:

  1. I'd like to rename the assume_role configuration option to assume_role_arn to be explicit about the content of the string.
  2. I've finally had some time to rework the unit tests to use botocore.stub.Stubber to stub out the AWS interactions so I can avoid the drawbacks of monkey-patching boto3 internals. As of 537fe6f, CodeArtifactBackend has a session keyword argument that defaults to boto3.session.Session() if not provided. There's a StubbingSession in the tests directory that allows for injecting expected responses from AWS API calls.
  3. Finally -- while not required in this PR -- since the role session name is mandatory for the AssumeRole call, I'd prefer to make the default something usable for auditing in CloudTrail rather than a just fixed string. I want to include information about the identity of the user invoking AssumeRole at minimum. (I can add this in a followup commit.)

Let me know what you think!

Comment thread keyrings/codeartifact.py Outdated
Comment thread keyrings/codeartifact.py Outdated
Comment thread requirements-test.txt Outdated
Comment thread tests/test_backend.py
Comment thread README.md Outdated
When fetching credentials after obtaining the boto3 session, we check if
the configuration has an optional `assume_role` field, which indicates
that role needs to be assumed to obtain the credentials.

If found, the role is assumed and used to obtain the credentails. If not
the original boto3 session is used to obtain the credentials.
@elventear

Copy link
Copy Markdown
Author

@jmkeyes coming back to this finally. I have addressed your comments, and rebased on top of the latest changes. If a session name is not provided we will use the UserId returned by the sts.get_current_caller_identity.

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.

2 participants