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

awsutil: ensure GenerateCredentialChain checks envs #80

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Jul 13, 2023

#57 caused a regression in Vault. In that PR the AWS environment variable checks in GenerateCredentialChain were moved to NewCredentialsConfig for AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE, and AWS_ROLE_SESSION_NAME. This caused users of the awsutil library that call GenerateCredentialConfig to fail to set the above values if they are depending on env variables to be read.

Another solution could be to find all users of go-secure-stdlib’s awsutil and ensure env vars are properly read and set in the config before calling GenerateCredentialConfig like in hashicorp/vault#21930. However, I think a this PR is a better resolution. With this change we shouldn't have to make any changes anywhere else.

This change was tested with the following steps:

Additionally, this has been tested against the Boundary E2E tests by @ddebko. Thanks!

Closes #71

@fairclothjm fairclothjm marked this pull request as ready for review July 19, 2023 15:34
@fairclothjm fairclothjm requested review from a team and ddebko July 19, 2023 15:34
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

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

LGTM -- might be nice to add some unit tests though so we don't get another regression? This is a pretty long function with lots of edge cases.

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM, just one query

awsutil/generate_credentials.go Show resolved Hide resolved
@fairclothjm
Copy link
Contributor Author

might be nice to add some unit tests though so we don't get another regression? This is a pretty long function with lots of edge cases.

@swenson Agreed! I will address adding some tests in another PR if you are ok with that?

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.

Following v0.2.1, applications using Vault cannot login via AWS Method on EKS
6 participants