feat: added OIDC #262

richardhboyd · 2021-09-20T18:28:39Z

Description of changes:

Added the ability to use GitHub provided OIDC token. If a user supplies a Role Arn and doesn't supply either an Access Key Id or a WebIdentityTokenFile we will attempt to load the OIDC Token from the environment variable provided by GitHub. If the Environment variable isn't set (because this could be running in a self-hosted runner) then we will fall back to the default credential provider as previously outlined in the README.md.

Updated the README.md to remove most mentions of using IAM Users as we expect future users to rely on the OIDC implementation, but the IAM User use-case is still defined (it's just not highlighted any more in the documentation).

Thanks to @aidansteele for the reference implmentation.

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

efekarakus

Looks great @richardhboyd ! Mostly just some some recommendations for troubleshooting 🙏

efekarakus · 2021-09-23T16:47:41Z

README.md

+| Field                   | Provided | Provided                               | Provided                                             | Provided                                        |
+| -----------             | :------: | :------------------------------------: | :------------------------------------:               | :------------------------------------:          |
+| aws-access-key-id       | Y        | Y                                      | N                                                    | N                                               |
+| role-to-assume          | N        | Y                                      | Y                                                    | Y                                               |
+| web-identity-token-file | N        | N                                      | Y                                                    | N                                               |
+| **Identity Used**       | IAM User | Assume Role using IAM User credentials | Assume Role using WebIdentity Token File credentials | Assume Role directly using GitHub OIDC provider |


nit: I find this table a bit hard to parse, but it might be just me! For some reason my brain took a while to register that i should be looking at each column separately instead of the rows 😅

What do you think of the following format?

| **Identity Used** | `aws-access-key-id` | `role-to-assume` | `web-identity-token-file` | |-----------------------------------------------------------------|---------------------|------------------|---------------------------| | IAM User | ✔ | | | | Assume Role using IAM User credentials | ✔ | ✔ | | | Assume Role using WebIdentity Token File credentials | | ✔ | ✔ | | [✅ Recommended] Assume Role directly using GitHub OIDC provider | | ✔ | |

I like it. I'll change it I'm moving the recommended approach to the top row.

README.md

efekarakus · 2021-09-23T16:52:04Z

README.md

+### Sample IAM Role CloudFormation Template 
+```yaml


🤩

What do you think of adding a "Launch stack" button to make this easy to create for folks? (maybe something like this: https://github.com/buildkite/cloudformation-launch-stack-button-svg)

I would love to. I need to get an S3 bucket approved for storing this and that'll take longer than the slack we have in the schedule right now. I will create an Issue for adding that.

efekarakus · 2021-09-23T17:17:04Z

index.js

@@ -43,6 +45,10 @@ async function assumeRole(params) {
  let roleArn = roleToAssume;
  if (!roleArn.startsWith('arn:aws')) {
    // Supports only 'aws' partition. Customers in other partitions ('aws-cn') will need to provide full ARN
+  assert(
+      isDefined(sourceAccountId),
+      "Source Account ID is needed if the Role Name is provided and not the Role Arn."


Can the user provide this information? I wonder if we can provide a suggested action here if we end up in this error scenario 🤔

This is what I was struggling with. It seems like we're trying to help the customer by saying "if you only provide a Role Name, we'll try our best to figure out which account you're talking about". Customers will encounter this exception if they supply a Role Name for use with the OIDC. Our current logic has the STS client call "get-caller-identity" and parse the response to get an account ID. I feel like this is a bit too much magic for customers who may be running this in a self-hosted runner with some limited permissions in AccountA and we might try to accidentally assume another role in accountA. I'd prefer that for the OIDC users we ask them to explicitly provide the Arn and we can relax it later.

index.js

richardhboyd · 2021-09-23T18:42:08Z

I think I squashed the commits correctly so we don't spam the commit history.

efekarakus

!

bshelton229 · 2021-09-26T20:13:14Z

README.md

-The default session duration is 6 hours, but if you would like to adjust this you can pass a duration to `role-duration-seconds`.
+We recommend using GitHub's OIDC provider to get short-lived credentials needed for your actions. 
+Specifying `role-to-assume` without providing an `aws-access-key-id` or a `web-identity-token-file` will signal to the action that you wish to use the OIDC provider.
+The default session duration is 1 hour when using the OIDC provider to directly assume an IAM Role.


I may be misunderstanding this documentation, but it doesn't look like this logic was implemented in the PR and the default is still using MAX_ACTION_RUNTIME regardless of method. I think 3600 seconds is the default on new roles and that would mean we'd need to set role-duration-seconds on every use of the plugin or change the maximum session duration on our roles. I like the 1 hour default if using OIDC.

I'm really excited for this to land and it's been working great in testing.

This was a really good catch! Thank you! I'm fixing it now. It'll default to 3600 for GH OIDC provider Roles and remain 6hrs (for backwards compatibility reasons) for the rest.

That sounds perfect, thanks!

Fixed, and I updated the test to validate it.

paragbhingre · 2021-09-27T18:08:20Z

README.md

@@ -99,48 +119,52 @@ Example:
 ```
 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`.

-### Permissions for assuming a role


Can we also remove permissions to assuming a role from the Table of Contents

paragbhingre · 2021-09-27T18:11:23Z

README.md

-        }
-    ]
-}
+### Sample IAM Role CloudFormation Template 


Can we add this to Table of Contents and link them here

paragbhingre · 2021-09-27T18:54:03Z

index.js

+    let sourceAccountId;
+    let webIdentityToken;
+    if(useGitHubOIDCProvider()) {
+      webIdentityToken = await getWebIdentityToken(maskAccountId, region);


Do we need to send maskAccountId and region to getWebIdentityToken for pulling OIDC credentials?

paragbhingre

HI @richardhboyd , Thanks for this cool feature implementation. I have couple of questions and suggestions for you otherwise it looks fine to me 🎉

bshelton229

One minor suggestion around reading role-duration-seconds and a general question I can't seem to answer regarding the id-token permission for the workflow.

bshelton229 · 2021-09-27T19:39:39Z

index.js

-  if(isDefined(webIdentityTokenFile)) {
-    core.debug("webIdentityTokenFile provided. Will call sts:AssumeRoleWithWebIdentity and take session tags from token contents.")
+    assumeRoleRequest.WebIdentityToken = webIdentityToken;
+    assumeRoleRequest.DurationSeconds = 3600;


We would still want the ability to set role-duration-seconds using the input. There are a few roles for us that I specifically would want to intentionally shorten this value. I think a default of 3600 for OIDC to match the role default in AWS makes a lot of sense, so the normal path is to not have to worry about setting anything. I think this should probably just be toggling the default value based on useGitHubOIDCProvider() type logic near the definition. Conditional defaults may be a little odd, but I think you're right about wanting to maintain backwards compatibility, and I think this lines up with the new documentation.

Maybe something like const roleDurationSeconds = core.getInput('role-duration-seconds', {required: false}) || getDefaultRoleDurationSeconds(???);?

Again, another good catch. I shouldn't have tried to write this code so early on a Monday.

What are your thoughts on

if(useGitHubOIDCProvider()) { webIdentityToken = await getWebIdentityToken(maskAccountId, region); roleDurationSeconds = core.getInput('role-duration-seconds', {required: false}) || DEFAULT_ROLE_DURATION_FOR_OIDC_ROLES;

Then I removed the manual change I just made. My only challenge with the code you provided is that I'd have to pass a bunch of env variables into the getDefaultRoleSDurationSeconds() which may not have been set yet so the behavior of the program might change based on where the call was made from, which seemed a bit hard to maintain.

That looks great to me.

bshelton229 · 2021-09-27T19:54:50Z

README.md

+
+### Examples
+
+```yaml


I have a general question I've been struggling to find documentation on from Github. What I'm seeing in my test pipelines is we need to set the id-token: write permission in the job permissions field in order to be provided the token for OIDC. I can't find this documented by Github anywhere. I'm also finding that when I set that, and thus go down the road of defining the permissions block at all, it undoes any other default permissions. This means I need to do something like:

permissions: id-token: write contents: read

In order to both get the OIDC token and continue to be able to use the token for normal actions/checkout@v2 checkouts.

The google auth plugin did seem to allude to this setting when they updated for OIDC I noticed - https://github.com/google-github-actions/auth#usage

I can't find any Github documentation around this, which I assume is because this actual feature is still beta? If AWS or anyone understands how this thing works, maybe it could be documented here :)

google-github-actions/auth@f8bb88e#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

we're also still waiting on official documenation

Awesome, thanks for verifying I'm not just missing something.

bshelton229

Looks good to me for the role-duration-seconds

Bumps [jest](https://github.com/facebook/jest) from 27.2.1 to 27.2.2. - [Release notes](https://github.com/facebook/jest/releases) - [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md) - [Commits](jestjs/jest@v27.2.1...v27.2.2) --- updated-dependencies: - dependency-name: jest dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [ansi-regex](https://github.com/chalk/ansi-regex) from 5.0.0 to 5.0.1. - [Release notes](https://github.com/chalk/ansi-regex/releases) - [Commits](chalk/ansi-regex@v5.0.0...v5.0.1) --- updated-dependencies: - dependency-name: ansi-regex dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [aws-sdk](https://github.com/aws/aws-sdk-js) from 2.991.0 to 2.996.0. - [Release notes](https://github.com/aws/aws-sdk-js/releases) - [Changelog](https://github.com/aws/aws-sdk-js/blob/master/CHANGELOG.md) - [Commits](aws/aws-sdk-js@v2.991.0...v2.996.0) --- updated-dependencies: - dependency-name: aws-sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

paragbhingre

Looks good to me. 💯 🎉

the Action supports ID token issued by GitHub OIDC and GitHub Acitons can assume the specified role using IAM identity provider. See aws-actions/configure-aws-credentials#262 for more details.

richardhboyd changed the title ~~added OIDC, maybe~~ feat: added OIDC Sep 20, 2021

efekarakus reviewed Sep 23, 2021

View reviewed changes

richardhboyd marked this pull request as ready for review September 23, 2021 18:48

efekarakus approved these changes Sep 23, 2021

View reviewed changes

bshelton229 reviewed Sep 26, 2021

View reviewed changes

paragbhingre reviewed Sep 27, 2021

View reviewed changes

bshelton229 reviewed Sep 27, 2021

View reviewed changes

feat: OIDC provider (with PR comments)

5e4f686

bshelton229 approved these changes Sep 27, 2021

View reviewed changes

dependabot bot and others added 6 commits September 28, 2021 03:01

chore: Update dist

ef6880f

feat: OIDC provider (with PR comments)

8d77d75

Merge remote-tracking branch 'origin/master'

8c9da02

paragbhingre approved these changes Sep 28, 2021

View reviewed changes

mergify bot merged commit b8c74de into aws-actions:master Sep 28, 2021

takanabe mentioned this pull request Oct 1, 2021

Use configure-aws-credentials Actions takanabe/github-actions-oidc-test#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added OIDC #262

feat: added OIDC #262

richardhboyd commented Sep 20, 2021

efekarakus left a comment

efekarakus Sep 23, 2021

richardhboyd Sep 23, 2021

efekarakus Sep 23, 2021

richardhboyd Sep 23, 2021

efekarakus Sep 23, 2021

richardhboyd Sep 23, 2021

richardhboyd commented Sep 23, 2021

efekarakus left a comment

bshelton229 Sep 26, 2021

richardhboyd Sep 27, 2021

bshelton229 Sep 27, 2021

richardhboyd Sep 27, 2021

paragbhingre Sep 27, 2021

paragbhingre Sep 27, 2021 •

edited

Loading

paragbhingre Sep 27, 2021

richardhboyd Sep 27, 2021

paragbhingre left a comment

bshelton229 left a comment

bshelton229 Sep 27, 2021

richardhboyd Sep 27, 2021

richardhboyd Sep 27, 2021

bshelton229 Sep 27, 2021

bshelton229 Sep 27, 2021 •

edited

Loading

richardhboyd Sep 27, 2021

bshelton229 Sep 27, 2021

bshelton229 left a comment

paragbhingre left a comment


		### Examples

		```yaml

feat: added OIDC #262

feat: added OIDC #262

Conversation

richardhboyd commented Sep 20, 2021

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardhboyd commented Sep 23, 2021

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paragbhingre Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paragbhingre left a comment

Choose a reason for hiding this comment

bshelton229 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshelton229 Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshelton229 left a comment

Choose a reason for hiding this comment

paragbhingre left a comment

Choose a reason for hiding this comment

paragbhingre Sep 27, 2021 •

edited

Loading

bshelton229 Sep 27, 2021 •

edited

Loading