-
Notifications
You must be signed in to change notification settings - Fork 471
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
feat: add drop-current-credentials
and role-output-credentials
#325
Conversation
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 think there's a typo.
+1 for this. Exporting the env vars by default not only complicates work with multiple profile, but in some cases forces the users to manually clear the variables (by writing empty values to $GITHUB_ENV), because many tools give the env vars precedence over the configuration files. |
Howdy! Any progress with that PR? this seems to be breaking composable actions that use that library. |
@aksel What is the actual state of this PR? It will be merge in the near future? |
If anyone runs into this:
This PR is important to make this action usable multiple times per job. |
@byF Can u paste the code that does not work for you ? I am still setting up the variables to empty and it works for me
|
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.
Thanks a ton @blz-ea for this contribution, and many apologies for the long delay between your submission and this review.
Overall this is good work, I've left a few comments that I'd like you to address when you have the opportunity.
However before you address those, I have concern over adding two new and mostly unrelated features in the same PR. I'm going to request that you separate drop-current-credentials
functionality from this PR in a new PR and branch. I'll give both this PR and the new one my full attention upon hearing back from you 🙂
@@ -118,6 +118,28 @@ In this example, the Action will load the OIDC token from the GitHub-provided en | |||
``` | |||
In this example, the secret `AWS_ROLE_TO_ASSUME` contains a string like `arn:aws:iam::123456789100:role/my-github-actions-role`. To assume a role in the same account as the static credentials, you can simply specify the role name, like `role-to-assume: my-github-actions-role`. | |||
|
|||
```yaml |
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.
We don't need a full example, but I'd like for there to be at least a description of drop-current-credentials
in the README with a short explanation on when it would be useful to use
|
||
for (const e of envs) { | ||
core.exportVariable(e, ''); | ||
delete process.env[e] |
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 line delete process.env[e]
seems redundant to me. core.exportVariable
sets these values to empty, why would we need to delete them altogether in the second line?
For reference, here's how exportVariable is implemented under the hood https://github.com/actions/toolkit/blob/b00a9fd033f4b30f2355acd212f531ecbbb9b38f/packages/core/src/core.ts#L83-L93
@@ -228,14 +253,32 @@ async function validateCredentials(expectedAccessKeyId) { | |||
} | |||
} | |||
|
|||
function getStsClient(region) { | |||
function getStsClient(region, 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.
I'm unsure what problem this is exactly trying to solve. Why do we need to pass in credentials now?
There are a couple other instances where getStsClient
and exportAccountId
get called (lines 43 and 382 in your index.js) without being updated to have credentials passed in. Since I'm confused at the necessity for this in the first place, I'm not sure why credentials are only passed on line 418. Could you clear this up for me?
A reimplementation of @blz-ea's role-output-credentials from aws-actions#325 for the newer v1 aws-credentials provider. Implements output-credentials functionality to support use-cases that do not wish to persist credentials into GHA environment variables (useful when multiple accounts or roles need to be involved). Does NOT re-implement drop-current-credentials as I have no use-case for that. I hope that by a smaller more tactile patch I can get this merged upstream. All credit to @blz-ea, but direct admonitions at me.
A reimplementation of @blz-ea's role-output-credentials from aws-actions#325 for the newer v1 aws-credentials provider. Implements output-credentials functionality to support use-cases that do not wish to persist credentials into GHA environment variables (useful when multiple accounts or roles need to be involved). Does NOT re-implement drop-current-credentials as I have no use-case for that. I hope that by a smaller more tactile patch I can get this merged upstream. All credit to @blz-ea, but direct admonitions at me.
Apologies for the delay once more, but I'll be closing this due to staleness. Thanks for the contribution |
Hi @peterwoodworth |
@yporwal1 this had to get closed because the PR was abandoned, not that it's the submitters fault or anything. This action was in no-mans land for a while in terms of ownership, but the next version of this action should be coming in the near future, and it has this new feature implemented |
Fixes #236, fixes #379
Description:
Add
drop-current-credentials
input which when set totrue
removes AWS credentials prior to assuming the role.Useful when you need to assume multiple roles, based on the original role that was attached to EC2
Add
role-output-credentials
input which when set totrue
outputs credentials instead of adding them to environment. Credentials can be referenced in the later steps. This solves certain security problem, and assume multiple roles without adding them to environmentaws-account-id
instead of source rolesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.