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

Prioritize ssm creds for linux #4155

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

sidlovskyy
Copy link

@sidlovskyy sidlovskyy commented Apr 24, 2024

Summary

In Linux environment credentials priority is set to:

  1. Env vars (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY).
  2. Shared credentials file
  3. EC2 role credentials.
  4. Rotating shared credentials file located at /rotatingcreds/credentials

When ECS Agent (in case of ECS Anywhere) is installed on EC2 instance of other AWS account with IAM Role attached, EC2 Role credentials are prioritized over rotating credentials which means SSM Agent credentials won't be used.
As far as there is no way to change credentials providers priority it's not possible to make ECS Agent use SSM Agent rotating credentials. It makes usage of ECS Anywhere hardly possible when external instance is part of other AWS Account with EC2 IAM Role credentials attached. It may not be possible to modify EC2 Role credentials due to policies in external AWS Account.

It makes sense to prioritize rotating credentials over EC2 Role credentials in case ECS Agent is installed on external instance. Please note, similar behavior is implemented for Windows:

credProviders = append(credProviders[:1], append([]credentials.Provider{providers.NewRotatingSharedCredentialsProvider()}, credProviders[1:]...)...)

Alternatively, it would be great if credentials providers priority could be re-configured by additional config variable.

Implementation details

In Linux environment when ECS_EXTERNAL variable is set to true RotatingSharedCredentialsProvider is prioritized over EC2RoleProvider so that SSM Agent credentials are prioritized over EC2 Role credentials.

Testing

New tests cover the changes: no

Description for the changelog

Prioritize RotatingSharedCredentialsProvider over EC2RoleProvider instance credentials provider for external instances in Linux

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sidlovskyy sidlovskyy marked this pull request as ready for review April 24, 2024 15:14
@sidlovskyy sidlovskyy requested a review from a team as a code owner April 24, 2024 15:14
@amogh09
Copy link
Contributor

amogh09 commented May 6, 2024

Thanks for the contribution @sidlovskyy . I see that this change was made for Windows in #3184. I am trying to find out why Linux was not considered in-scope at that time.

//
// In the case of external instance `RotatingSharedCredentialsProvider` provider should be prioritized over
// `EC2RoleProvider` one as SSM credentials are going to be ignored in case agent is deployed to EC2
// with instance role which doesn't have permissions needed to run ECS Agent
func GetCredentials(isExternal bool) *credentials.Credentials {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this change makes GetCredentials implementation for Linux identical to that of Windows. I think we should merge the two into a single instancecreds/instancecreds.go file.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the meanwhile I will reproduce this issue on my end. Once the PR is ready I can verify that the change works by testing it in the same environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to reproduce the issue on my end.

@amogh09
Copy link
Contributor

amogh09 commented May 6, 2024

Thanks for the contribution @sidlovskyy . I see that this change was made for Windows in #3184. I am trying to find out why Linux was not considered in-scope at that time.

I talked to some folks about this. We don't have a good reason as to why #3184 did not make the same change for Linux. It makes sense for the instance credentials fetch logic to be platform agnostic.

@sidlovskyy
Copy link
Author

Thanks for the contribution @sidlovskyy . I see that this change was made for Windows in #3184. I am trying to find out why Linux was not considered in-scope at that time.

I talked to some folks about this. We don't have a good reason as to why #3184 did not make the same change for Linux. It makes sense for the instance credentials fetch logic to be platform agnostic.

Thank you, should I update PR to include platform agnostic instancecreds.go that is the same as existing windows implementation?

@jhoward-ts
Copy link

Hey @amogh09 and all guys - @sidlovskyy has sadly moved on from this project, but my team is still interested in this. Any updates?

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.

5 participants