fix: fixed logic to determine if long-term credentials is used #904

ponkio-o · 2023-10-31T09:37:54Z

*Issue #885

Description of changes:

The current logic looks to see if AWS_SESSION_TOKEN is present, which is causing unnecessary warnings when using a self-hosted runner with EC2 instance profile.
Since this warning is originally issued when only AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are set, I fixed the logic to determine.

Have you followed the guidelines in our Contributing guide?

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

ponkio-o · 2023-10-31T10:29:31Z

However, this fix doesn't cover all cases. For example, the warning is not displayed when using IAM User as shown below.

      - name: Setup aws credentials
        uses: aws-actions/[email protected]
        with:
          aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
          aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
          aws-region: ap-northeast-1

To cover these, it is necessary to make sure that no other inputs are set in more upstream functions. Like this

configure-aws-credentials/src/index.ts

Lines 73 to 93 in a4d9254

    
             if ( 
        
               !!roleToAssume && 
        
               !webIdentityTokenFile && 
        
               !AccessKeyId && 
        
               !process.env['ACTIONS_ID_TOKEN_REQUEST_TOKEN'] && 
        
               !roleChaining 
        
             ) { 
        
               core.info( 
        
                 'It looks like you might be trying to authenticate with OIDC. Did you mean to set the `id-token` permission? ' + 
        
                   'If you are not trying to authenticate with OIDC and the action is working successfully, you can ignore this message.' 
        
               ); 
        
             } 
        
             return ( 
        
               !!roleToAssume && 
        
               !!process.env['ACTIONS_ID_TOKEN_REQUEST_TOKEN'] && 
        
               !AccessKeyId && 
        
               !webIdentityTokenFile && 
        
               !roleChaining 
        
             ); 
        
           };

peterwoodworth · 2023-10-31T16:32:54Z

test/index.test.ts

@@ -874,6 +874,8 @@ describe('Configure AWS Credentials', () => {

  test('prints warning for access key usage and no session token', async () => {
    jest.spyOn(core, 'getInput').mockImplementation(mockGetInput(ASSUME_ROLE_INPUTS));
+    process.env['AWS_ACCESS_KEY_ID'] = FAKE_ACCESS_KEY_ID;


We should also have another test without these lines you've added to verify that the warning does NOT throw

Thank you for your review! I added test check doesn't show warning when using STS access key.
3e570a9

jplock · 2023-11-01T01:13:26Z

We could also validate the prefix of the access key per https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-unique-ids

ponkio-o · 2023-11-01T01:49:10Z

We could also validate the prefix of the access key per
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-unique-ids

+1
It's looks very good! It would be more reliable and simpler to look at prefixes to detect long-term credentials.

jplock · 2023-11-01T12:11:46Z

src/assumeRole.ts

+  // IAM access key starts with 'AKIA' prefix.
+  // https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-unique-ids
+  if (accessKey) {
+    if (accessKey.startsWith('AKIA')) {


Combine these into one condition separated by &&?

I’d also define AKIA as a constant up above with the link to the documentation.

@jplock Thank you for your suggestion!
I fixed using constant and to simplify if condition.
018873b

ref https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/prefer-optional-chain.md

jplock

Looks good to me

peterwoodworth · 2023-11-01T18:43:48Z

Brilliant @jplock, that's a fantastic solution. Thank you both very much for working on this and bringing a solution to the table 🙂

Not a requirement at all, feel free to disagree with this slight nit, I think I would redefine the const as an all caps const at the top of the file, just below the imports. Other than that I think this looks perfect.

I'm not a maintainer here anymore, so I cannot give approval, but I'm sure the current maintainers will have time to take a look relatively soon

jplock · 2023-11-01T22:00:57Z

+1 I agree with that (constants should be in all caps and defined near the top outside of the function).

ponkio-o · 2023-11-02T02:43:12Z

Thank you @jplock and @peterwoodworth!
Please check it out, as I fixed it!
314fe97

jplock · 2023-11-03T13:44:29Z

@tim-finnigan would you be able to review this PR please?

tim-finnigan · 2023-11-05T21:58:30Z

Thanks for creating this PR! I think we want to be careful about hard coding prefixes that could change, or may not apply to every intended scenario. But the changes here are probably fine. We are planning to discuss this PR further this week, and will likely also want to confirm our thinking with someone from the IAM team before approving this.

ponkio-o · 2023-11-21T02:18:34Z

Hi @tim-finnigan, Do you have any update?

tim-finnigan · 2023-11-21T23:55:27Z

Hi @ponkio-o thanks for your patience. We discussed this topic further as a team and decided to remove the warning. There are more details in this PR: #926.

Thanks again for working on this PR. While I think the changes here are an improvement to the existing warning conditions, we are hesitant to specify hard-coded IAM prefixes that could also change in the future.

ponkio-o · 2023-11-22T04:07:50Z

@tim-finnigan
Thank you for your reply! I agree your approach (to delete this warning). Also, I agree your talk about "Shared Responsibility Model".

So, I will close this PR.

ponkio-o added 2 commits October 31, 2023 18:28

fixes determination of whether long-term credentials are used or not

1d1179c

update dist

7bb2e4a

peterwoodworth reviewed Oct 31, 2023

View reviewed changes

ponkio-o added 4 commits November 1, 2023 11:45

fix: change logic to determine if using long-term credentials

a4d19f7

chore: fix FAKE_ACCESS_KEY_ID prefix

4832b39

add: added test when assume role with sts access key

3e570a9

chore: update dist

73f7d26

ponkio-o requested a review from peterwoodworth November 1, 2023 02:50

jplock reviewed Nov 1, 2023

View reviewed changes

fix: use optional chain expressions and defined prefix in const

018873b

ponkio-o requested a review from jplock November 1, 2023 13:53

jplock approved these changes Nov 1, 2023

View reviewed changes

fix: change constans to upper case

314fe97

ponkio-o requested a review from jplock November 2, 2023 02:43

jplock approved these changes Nov 2, 2023

View reviewed changes

jplock mentioned this pull request Nov 3, 2023

chore: replace shortened URL with full URL #905

Merged

ponkio-o closed this Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fixed logic to determine if long-term credentials is used #904

fix: fixed logic to determine if long-term credentials is used #904

ponkio-o commented Oct 31, 2023

ponkio-o commented Oct 31, 2023

peterwoodworth Oct 31, 2023

ponkio-o Nov 1, 2023

jplock commented Nov 1, 2023

ponkio-o commented Nov 1, 2023

jplock Nov 1, 2023

jplock Nov 1, 2023

ponkio-o Nov 1, 2023

ponkio-o Nov 1, 2023

jplock left a comment

peterwoodworth commented Nov 1, 2023

jplock commented Nov 1, 2023

ponkio-o commented Nov 2, 2023

jplock commented Nov 3, 2023

tim-finnigan commented Nov 5, 2023

ponkio-o commented Nov 21, 2023

tim-finnigan commented Nov 21, 2023

ponkio-o commented Nov 22, 2023

fix: fixed logic to determine if long-term credentials is used #904

fix: fixed logic to determine if long-term credentials is used #904

Conversation

ponkio-o commented Oct 31, 2023

ponkio-o commented Oct 31, 2023

peterwoodworth Oct 31, 2023

Choose a reason for hiding this comment

ponkio-o Nov 1, 2023

Choose a reason for hiding this comment

jplock commented Nov 1, 2023

ponkio-o commented Nov 1, 2023

jplock Nov 1, 2023

Choose a reason for hiding this comment

jplock Nov 1, 2023

Choose a reason for hiding this comment

ponkio-o Nov 1, 2023

Choose a reason for hiding this comment

ponkio-o Nov 1, 2023

Choose a reason for hiding this comment

jplock left a comment

Choose a reason for hiding this comment

peterwoodworth commented Nov 1, 2023

jplock commented Nov 1, 2023

ponkio-o commented Nov 2, 2023

jplock commented Nov 3, 2023

tim-finnigan commented Nov 5, 2023

ponkio-o commented Nov 21, 2023

tim-finnigan commented Nov 21, 2023

ponkio-o commented Nov 22, 2023