-
Notifications
You must be signed in to change notification settings - Fork 74
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
Assume Role support improvements #20
Conversation
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
Please delete the |
Sorry for that, was my assumption this was required to get this on the radar of maintainers (based on other PRs). Fixed |
f837bf9
to
3c15fac
Compare
Now rebased on-top of 1.20 release of this plugin |
Sorry for not including this one in the upcoming release, I'll try to get someone more appropriate than me to review it. |
If no access key and secret is set, also attempting to fetch credentials via instance profile
3c15fac
to
48985bc
Compare
What's the ETA on this? We'd like to get this feature included as well. |
@HontoNoRoger Even though not ideal - you may want to try using the pre-built version I've linked at the end of my PR. We've been using that version on our Jenkins servers since the creation of this PR and it has been working nicely for us. |
@andresrc any updates? |
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.
The change looks good to me, but I am not an expert in this API. 👍 for pushing it anyway!
Hey guys, would it be possible to merge this so it comes in the next release? |
this is really necessary for the role assumption thing to work at all. Without this change, it you can't even use the role beasue the token insn't set! I don't really understand how the role assumption thing was half implemented in the first place; apparently it wasn't tested at all. |
@andresrc ping |
I'm ok with the change, but I would like a review from @Vlatombe |
LGTM |
@andresrc thanks for merging. Would it be possible to push a release so we don't have to use my custom build? |
this was released? |
Still no release. This was merged post 1.21 release so should come in 1.22 is my guess. |
Hi, sorry for the delay in the answer, I was out. Version 1.22 has been released. |
this page still show the 1.21 :( https://wiki.jenkins.io/display/JENKINS/CloudBees+AWS+Credentials+Plugin |
@cpanato While the wiki is out of data, Jenkins servers do see the new version |
@@ -113,6 +114,11 @@ public AWSCredentials getCredentials() { | |||
if (StringUtils.isBlank(iamRoleArn)) { | |||
return initialCredentials; | |||
} else { | |||
// Handle the case of delegation to instance profile | |||
if (StringUtils.isBlank(accessKey) && StringUtils.isBlank(secretKey.getPlainText()) ) { | |||
initialCredentials = (new InstanceProfileCredentialsProvider()).getCredentials(); |
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.
I know this has already been merged and released, but isn't the manually-specified IAM role in the credentials entry (iamRoleArn
) redundant in this case? The instance profile already contains an assigned IAM role, which is what should be used in this case, no?
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.
The use-case for this feature is to specifically switch to a different account's role.
Useful in cross-account integrations.
Instance Profile is needed for this (unless creds are used) to authenticate that you are allowed to do this switch, and role is needed as a target.
How to use the feature in a pipeline script? Where to find the documentation? UP: Found #15 (comment) |
Two changes in this PR.
1. Session Token variable
To use e.g AWS CLI with AssumeRole support, an
AWS_SESSION_TOKEN
variable must be set, or AWS would complain that no access key is found on record. This variable is only bound if credentials returned by the credentials provider are session-based.Fixes #19
2. Instance Profile credentials
To further support moving away from hard-set AWS Credentials and allow role switching (e.g cross-account access), this PR also adds following behavior:
If no access & secret keys are set as part of credentials, but IAM role is set - attempting to fetch credentials via instance profile.
Fixes #18 #15
Motivation for both changes is to simplify management of multi-AWS account environments and to remove useage of Access/Secret keys which should be rotated quite often according to security best-practices. Relying on instance profile and assume-role allows us to completely forget about provisioning IAM users in different accounts.
I've also added compile version of the HPI file here: https://github.com/AlexejK/aws-credentials-plugin/releases/tag/aws-credentials-1.21-pr20 (for those who need to try this out but can't build themselves)