Skip to content

fix(s3deploy): use default credentials chain when access key / secret are not provided#512

Closed
HunterLarco wants to merge 2 commits intobep:masterfrom
HunterLarco:hunter/fix-credentials-2
Closed

fix(s3deploy): use default credentials chain when access key / secret are not provided#512
HunterLarco wants to merge 2 commits intobep:masterfrom
HunterLarco:hunter/fix-credentials-2

Conversation

@HunterLarco
Copy link
Contributor

@HunterLarco HunterLarco commented Nov 30, 2025

PR #510 was reverted due to failing integration tests on master which are now available at PR-time (998f750). This PR is a copy of #510 with additional changes to address the broken integration tests. The original PR description is copied below:

Fixes #509

TL;DR the current credentials loading logic does not use aws's default credential provider chain, preventing users from using the myriad of aws options (config files, sso, environment variables, etc...). This PR preserves the existing behavior of -key and -secret options while expanding how aws finds credentials when those overrides are not defined.

See https://docs.aws.amazon.com/sdk-for-go/v2/developer-guide/configure-gosdk.html

Backwards Compatibility

Tests pass, to my knowledge this should only impact users who don't set -key and -secret and does not remove existing authorization options, it just adds new ones. So it should be safe.

Version Updates

We need a new dependency for the default config factory so I ran:

go get github.com/aws/aws-sdk-go-v2/config

This in turn pulled in several transitive dependencies which are all related to the default credential provider change (such as sso dependencies) so this look as expected.

Testing

Tests pass and I have tested this PR locally with my own repo and can confirm that it works for the following setups:

  • multiple aws profile configured with AWS_PROFILE
  • sso credentials
  • access + secret credentials


// The region may be possible for the AWS SDK to figure out from the context.

if cfg.AccessKey == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: these are no longer required because the aws credential chain will automatically inspect environment variables

@HunterLarco
Copy link
Contributor Author

@bep I reopened this as you requested on #510. I have made no changes besides the one comment I left above and tests are passing. I'm concerned the integration tests either aren't running (though they look like they are) or that the original error was transient.

@HunterLarco HunterLarco marked this pull request as ready for review November 30, 2025 22:02
@HunterLarco HunterLarco force-pushed the hunter/fix-credentials-2 branch from d9d04e7 to 5ca1b74 Compare November 30, 2025 22:10
@bep
Copy link
Owner

bep commented Nov 30, 2025

@HunterLarco I re-ran your previous PR with same test failure (it basically fails in the common case with a secret in the env), which is obviously not somehting that I can release.

I will look into this later (as in a day or two).

Are you sure you rebased this against master?

@bep
Copy link
Owner

bep commented Nov 30, 2025

Thinking about it, I suspect the tests do not trigger on reopen. Please create a brand new PR to remove any confusion.

@bep bep closed this Nov 30, 2025
@HunterLarco
Copy link
Contributor Author

HunterLarco commented Dec 1, 2025

@bep I misspoke when I said it was "reopened".. this was a brand new PR (rebased from your recent changes to the github workflow on master) 😅 I can open a 3rd one if you'd prefer. I also just confirmed that all of the following setups work using this PR

# env variable
AWS_ACCESS_KEY_ID='<redacted>' AWS_SECRET_ACCESS_KEY='<redacted>' s3deploy ...

# arguments
s3deploy -key '<redacted>' -secret '<redacted>' ...

# config file
s3deploy ...

I also validated just now against the reverted PR and was unable to replicate the integration test failure. Something must be different in the test setup but I'm not clearly seeing what it might be. I'd appreciate if you'd take a look when you have a moment

@HunterLarco
Copy link
Contributor Author

^ bump~ @bep would you like me to open a duplicate or can we reopen this one?

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.

aws credential loading does not support profiles nor sso

2 participants