feat: Allow audience to be explicitly specified #362

alblue · 2022-01-17T15:21:29Z

Allow the audience to be configured instead of defaulting to 'sts.amazonaws.com'.

It's desirable to be able to use different audiences to ensure that changes aren't enabled on the wrong location -- for example, you can use an audience of 'prod' in the production accounts and 'dev' in the development accounts.

By adding audience as an argument to the job, you can set a specific parameter in the with block, and the example has been updated to reflect this.

By the way, it's possible to configure both the role and the OICD provider with an 'either' clause, so you can use one of several audiences -- but that somewhat defeats the point of using a non-default value :)

"Action": "sts:AssumeRoleWithWebIdentity",
"Condition": {
"StringLike": {
"token.actions.githubusercontent.com:sub": "repo:alblue/*",
"token.actions.githubusercontent.com:aud": ["sts.amazonaws.com","alblue"]
}

Furthermore this approach allows you to use the audience to allow for different sets roles to be enabled; you could have a dev audience that is only trusted by the GitHubDev role, and a prod audience that is only trusted by the GitHubProd role. The OICD audience could have both, but you'd then guarantee that the dev audience couldn't assume the GitHubProd role (and vice versa).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

fixes: #458, #457

alblue · 2022-01-18T11:44:11Z

This is an update of #285 after the refactoring to use the core module for token acquisition. Tested from forked branch by manipulation of dist/index.js (not done for this commit; presumably there's a build process to do it?) /cc @richardhboyd

alblue · 2022-01-28T22:12:17Z

Is there anything else needed for this patch to be accepted?

index.js

action.yml

alblue · 2022-02-20T21:31:22Z

What else needs to be done for this patch to be accepted?

alblue · 2022-03-06T13:04:40Z

Is there any way to merge this please? There hasn't been any other feedback and we are currently maintaining a long lived fork of this project in the interim. There has been no issues found using this feature and it adds zero change out of the box since the default remains the same as before. However we are using the different audience to ensure that dev cannot accidentally deploy to prod (and vice versa) by using different audiences between the two.

The default audience for the GitHub OIDC uses sts.amazonaws.com, but there are situations when it would be desirable to allow different audience names to be used instead. Allow this to be specified as an argument to the action.

alblue · 2022-03-24T11:25:54Z

Sorry, I realised that I marked the conversations as complete but then completely failed to push that change to the right branch, so it never made it to the PR for merging :-(

@bryantbiggs could I ask you to review please?

bryantbiggs

looks correct to me - lets see if the maintainers will merge 👍🏽

alblue · 2022-03-24T11:57:45Z

Thanks @bryantbiggs -- sorry I left this unfixed so long as I thought I'd pushed to this with your recommendations in place back in Jan

alblue · 2022-04-07T13:42:59Z

Any one able to review/merge this simple request?

alblue · 2022-04-27T20:18:49Z

Is any maintainer in this repo able to review and merge this change? Alternatively if there are any things that need to be completed prior to this please let me know.

alblue · 2022-05-24T13:41:48Z

Is there anyone who can review/merge this request please?

Justin-Schmitz · 2022-06-20T14:33:51Z

@clareliguori - Please review :) We have a need for this feature

alblue · 2022-06-23T12:23:17Z

@aragbhingre could you approve and merge this please? It's adding a simple field to the defaults and provides real value for those that need to specify a different audience

shearn89 · 2022-07-01T07:43:03Z

@paragbhingre - any chance this can get reviewed and merged? We have customer use cases that would benefit and it looks like a few other people would too! Or @richardhboyd?

peterwoodworth

Sorry for the long delay here @alblue,

This PR looks great! I think the Readme here could be improved a bit more to bring more visibility to this feature. Could you provide a short example in Assuming a Role -> Examples that shows when a user would want to use this feature (such as in the PR the other customer made for CN regions) and add a small blurb? Since the default value is already sts.amazon.com I don't think we need to add this to our existing examples

alblue · 2022-07-07T00:03:15Z

Ok great, I'll work on this and submit a new PR. Would you prefer me to rebase this on latest, or add a second commit to this with the updated readme information?

peterwoodworth · 2022-07-07T15:03:05Z

Just push a second commit to the branch that this PR is based off @alblue!

sagi-shimoni · 2022-07-14T11:59:47Z

great feature! this is very useful, ! hope it will be merged soon..

shearn89 · 2022-07-19T08:30:59Z

Hi - any chance we can get the change resolved and merge this? Are there any further changes to be made?

Justin-Schmitz · 2022-07-19T12:42:07Z

Hi - any chance we can get the change resolved and merge this? Are there any further changes to be made?

@peterwoodworth - Is this ready for merge? Thank you

peterwoodworth

Thank you all so much for your patience here! I am hoping to have the next release out next week

peterwoodworth · 2022-07-20T18:46:30Z

index.js

@@ -19,6 +19,7 @@ async function assumeRole(params) {
  const isDefined = i => !!i;

  const {
+    audience,


This line is causing our testing to fail since it's not being used - the webIdentityToken should have already covered the audience, right? Do you remember why you added this line here?

Hi Peter,

I'm sorry to let you know that @alblue passed away last week (https://www.infoq.com/news/2022/07/alex-blewitt/).

I've been working with him on this (we work on the same company). If you let me know which would be the best way to take over his fork in this issue I can make any required changes on my side.

If you would prefer to take it over on your side that would be OK too, as you probably know your code better than anyone.

Best regards,

Thank you @enriquesantosblanco-sanuk - I worked with @alblue. It was devastating news.
Although I cannot help with the specific code, I am happy to assist in ensuing this gets completed if needs be.

This is devastating news... I'm so sorry, my condolences 😞

Thank you on behalf of his entire Cloud Platform Evolution team. Knowing that his genius lives on through code is comforting to all that knew him. All the best to you and yours!

outdated

peterwoodworth

Thanks @alblue, this submission and your patience is very much appreciated

bryantbiggs reviewed Feb 3, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

bryantbiggs reviewed Feb 3, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

bryantbiggs reviewed Feb 3, 2022

View reviewed changes

action.yml Outdated Show resolved Hide resolved

feat: Allow audience to be explicitly specified

2f8dfd0

The default audience for the GitHub OIDC uses sts.amazonaws.com, but there are situations when it would be desirable to allow different audience names to be used instead. Allow this to be specified as an argument to the action.

alblue force-pushed the allow-audience branch from a86f57e to 2f8dfd0 Compare March 24, 2022 11:24

bryantbiggs approved these changes Mar 24, 2022

View reviewed changes

alblue mentioned this pull request Jun 23, 2022

Fix AWS China Region STS incorrect token audience #458

Closed

peterwoodworth suggested changes Jul 6, 2022

View reviewed changes

peterwoodworth mentioned this pull request Jul 6, 2022

Incorrect token audience with AWS China region #457

Closed

Updated README.md for information about using alternate audiences

d1edd20

peterwoodworth approved these changes Jul 20, 2022

View reviewed changes

peterwoodworth previously requested changes Jul 20, 2022

View reviewed changes

remove unused variable

79fafe3

peterwoodworth approved these changes Jul 21, 2022

View reviewed changes

peterwoodworth merged commit bf7f81f into aws-actions:master Jul 21, 2022

austinvalle mentioned this pull request Jul 26, 2022

Unexpected input(s) 'audience' #475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow audience to be explicitly specified #362

feat: Allow audience to be explicitly specified #362

alblue commented Jan 17, 2022 •

edited by peterwoodworth

Loading

alblue commented Jan 18, 2022 •

edited

Loading

alblue commented Jan 28, 2022

alblue commented Feb 20, 2022

alblue commented Mar 6, 2022

alblue commented Mar 24, 2022

bryantbiggs left a comment

alblue commented Mar 24, 2022

alblue commented Apr 7, 2022

alblue commented Apr 27, 2022

alblue commented May 24, 2022

Justin-Schmitz commented Jun 20, 2022

alblue commented Jun 23, 2022

shearn89 commented Jul 1, 2022

peterwoodworth left a comment

alblue commented Jul 7, 2022

peterwoodworth commented Jul 7, 2022

sagi-shimoni commented Jul 14, 2022

shearn89 commented Jul 19, 2022

Justin-Schmitz commented Jul 19, 2022

peterwoodworth left a comment

peterwoodworth Jul 20, 2022

enriquesantosblanco-sanuk Jul 21, 2022

Justin-Schmitz Jul 21, 2022

peterwoodworth Jul 21, 2022

Justin-Schmitz Jul 21, 2022

peterwoodworth left a comment

feat: Allow audience to be explicitly specified #362

feat: Allow audience to be explicitly specified #362

Conversation

alblue commented Jan 17, 2022 • edited by peterwoodworth Loading

alblue commented Jan 18, 2022 • edited Loading

alblue commented Jan 28, 2022

alblue commented Feb 20, 2022

alblue commented Mar 6, 2022

alblue commented Mar 24, 2022

bryantbiggs left a comment

Choose a reason for hiding this comment

alblue commented Mar 24, 2022

alblue commented Apr 7, 2022

alblue commented Apr 27, 2022

alblue commented May 24, 2022

Justin-Schmitz commented Jun 20, 2022

alblue commented Jun 23, 2022

shearn89 commented Jul 1, 2022

peterwoodworth left a comment

Choose a reason for hiding this comment

alblue commented Jul 7, 2022

peterwoodworth commented Jul 7, 2022

sagi-shimoni commented Jul 14, 2022

shearn89 commented Jul 19, 2022

Justin-Schmitz commented Jul 19, 2022

peterwoodworth left a comment

Choose a reason for hiding this comment

peterwoodworth Jul 20, 2022

Choose a reason for hiding this comment

enriquesantosblanco-sanuk Jul 21, 2022

Choose a reason for hiding this comment

Justin-Schmitz Jul 21, 2022

Choose a reason for hiding this comment

peterwoodworth Jul 21, 2022

Choose a reason for hiding this comment

Justin-Schmitz Jul 21, 2022

Choose a reason for hiding this comment

peterwoodworth left a comment

Choose a reason for hiding this comment

alblue commented Jan 17, 2022 •

edited by peterwoodworth

Loading

alblue commented Jan 18, 2022 •

edited

Loading