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

fix: Updated token retrieval to use new API #270

Merged
merged 5 commits into from
Sep 29, 2021
Merged

fix: Updated token retrieval to use new API #270

merged 5 commits into from
Sep 29, 2021

Conversation

richardhboyd
Copy link

Issue #, if available:

Description of changes:

  • Updated the OIDC JWT fetching process to use the new API
  • replaced sigstore with sts.amazon.aws as the audience to reflect OIDC best practices for the audience to reflect the party that will be receiving the JWT.

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

@richardhboyd richardhboyd changed the title Updated token retrieval to use new API fix: Updated token retrieval to use new API Sep 29, 2021
@@ -571,7 +570,6 @@ describe('Configure AWS Credentials', () => {
test('only role arn and region provided to use GH OIDC Token', async () => {
process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'test-token';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these environment variables anymore?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. These being set is how we determine whether or not we are in a self-hosted runner or in a "proper" GitHub Action. If we remove them, the test will behave as if it were in a self-hosted runner (where OIDC isn't supported)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then where are we using it's test-token value in the test?

Copy link
Author

Choose a reason for hiding this comment

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

We don't use it in the test, we use it in the action.

https://github.com/aws-actions/configure-aws-credentials/blob/master/index.js#L284

Copy link
Author

Choose a reason for hiding this comment

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

We're less concerned with the actual value of the ENV variable, we just need it set to something so that the action sees that it has been set and behaves as if the test were running in a 'real' GitHub Action.

Copy link
Contributor

@paragbhingre paragbhingre left a comment

Choose a reason for hiding this comment

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

Lgtm. 🎉

@mergify mergify bot merged commit 20ce4e5 into aws-actions:master Sep 29, 2021
mergify bot pushed a commit that referenced this pull request Sep 30, 2021
* Revert "chore: Update dist"

This reverts commit 9815921.

* Revert "fix: Updated token retrieval to use new API (#270)"

This reverts commit 20ce4e5.
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