Support AWS default credential chain for S3 by BOPOHA · Pull Request #433 · dutchcoders/transfer.sh

BOPOHA · 2021-11-01T18:04:50Z

No description provided.

francescayeye

see my comment, thanks

stefanbenten · 2026-09-23T08:00:38Z

@BOPOHA Do you intend to address the comment from @francescayeye ?

BOPOHA · 2026-09-24T18:05:22Z

@stefanbenten thanks for following up.

I revisited this PR against the current codebase and updated the implementation locally. Since the original proposal, transfer.sh has migrated to AWS SDK for Go v2, which provides a comprehensive default credential chain.

Based on the original review feedback, I agree that a separate s3-ec2role provider is no longer the right abstraction. The storage provider remains s3; only credential discovery changes.

The updated approach is:

Keep the existing s3 provider.
Preserve static credentials when both aws-access-key and aws-secret-key are configured.
Use the AWS SDK default credential chain when neither is configured.
Reject configurations where only one static credential is supplied.
Add tests and documentation covering static and role-based credentials.

This enables EC2 instance profiles, ECS task roles, EKS IRSA, standard AWS environment variables, and shared AWS configuration while remaining backward-compatible.

I have also updated #431 with the current motivation and acceptance criteria. I’m preparing to replace the old implementation in this PR with the revised version.

cc @francescayeye

BOPOHA · 2026-09-24T21:03:17Z

Thanks for the feedback. I implemented the requested explicit credential mode:

Added --s3-credentials-type and S3_CREDENTIALS_TYPE.
The default value is legacy, preserving the existing requirement for both aws-access-key and aws-secret-key.
Setting the value to default-sdk-credential-chain explicitly enables the AWS SDK credential chain for environment credentials, shared configuration, ECS task roles, EC2 instance profiles, and EKS IRSA.
Unsupported credential types now return a clear error.
The Helm chart exposes transfersh.s3.credentialsType with legacy as its default and only creates or mounts static credential Secrets in legacy mode.
Added tests for the default, legacy validation, SDK-chain selection, unsupported values, and AWS session-token handling.

Validation completed successfully with go test, go vet, Helm lint, and rendered manifests for both credential modes.

francescayeye · 2026-09-25T17:57:13Z

 	}
 }
+
+func resolveS3Credentials(credentialsType, accessKey, secretKey string) (string, string, error) {


can you move this function to server/storage/s3.go and make it part of the storage package?
thanks :)

francescayeye · 2026-09-25T18:00:33Z

-			} else if secretKey := c.String("aws-secret-key"); secretKey == "" {
-				return errors.New("secret-key not set")
-			} else if bucket := c.String("bucket"); bucket == "" {
+			accessKey, secretKey, err := resolveS3Credentials(c.String("s3-credentials-type"), c.String("aws-access-key"), c.String("aws-secret-key"))


also please keep the if ..., err } else if { structure of this part of cmd.go.
for extending the scope of acessKey and secretKey you can initialize them as empty variables before the first if branch.
thank you very much again

francescayeye · 2026-09-25T18:17:16Z

 func getAwsConfig(ctx context.Context, accessKey, secretKey string) (aws.Config, error) {
-	return config.LoadDefaultConfig(ctx,
-		config.WithCredentialsProvider(credentials.StaticCredentialsProvider{
+	if (accessKey == "") != (secretKey == "") {


it should be fine as not breaking change behaviour, but i think there's a little hidden logic here where we hide explicit information here that the credentials type implied into the s3 storage is the legacy one. i'd pass down c.String("s3-credentials-type") to storage.NewS3Storage along with c.String("aws-access-key") and c.String("aws-secret-key"), and then deal with the logic that's in resolveS3Credentials (that will be moved to storage package anyway) directly here in getAwsConfig checking on the credentials type to be passed as new parameter and the eventual error will be bubbled up to the storage.NewS3Strorage returned error

francescayeye

i apologize for splitting the view in multiple sub-reviews

francescayeye · 2026-09-25T18:45:16Z

+		t.Fatalf("retrieve credentials: %v", err)
+	}
+	if credentials.AccessKeyID != "configured-access-key" || credentials.SecretAccessKey != "configured-secret-key" || credentials.SessionToken != "" {
+		t.Fatalf("got credentials %q/%q, want configured static credentials", credentials.AccessKeyID, credentials.SecretAccessKey)


can you please reword as ""got credentials %q/%q, want configured static credentials and empty session token"? ;)

francescayeye · 2026-09-25T18:45:58Z

+		t.Fatalf("retrieve credentials: %v", err)
+	}
+	if credentials.AccessKeyID != "environment-access-key" || credentials.SecretAccessKey != "environment-secret-key" || credentials.SessionToken != "environment-session-token" {
+		t.Fatalf("got credentials %q/%q, want credentials from the default chain", credentials.AccessKeyID, credentials.SecretAccessKey)


the same here regarding session token

francescayeye

i apologize for splitting the view in multiple sub-reviews

BOPOHA · 2026-09-25T21:29:16Z

no problem

moved S3 credential handling to the storage package
kept the existing if / else if structure
moved the related tests to storage
updated the session-token messages
reverted the unrelated comment change
removed the duplicate credential-type constants

@francescayeye please review it and feel free to suggest any improvements

francescayeye reviewed Nov 8, 2021

View reviewed changes

Comment thread cmd/cmd.go Outdated

francescayeye requested changes Nov 8, 2021

View reviewed changes

Support the AWS default credential chain for S3

1d2ce4a

BOPOHA force-pushed the main branch from ba80471 to 1d2ce4a Compare September 24, 2026 17:57

BOPOHA mentioned this pull request Sep 24, 2026

Feature: Support AWS default credential chain for S3 #431

Open

BOPOHA changed the title ~~added s3-ec2role provider; fix #431~~ Support AWS default credential chain for S3 Sep 24, 2026

francescayeye reviewed Sep 24, 2026

View reviewed changes

Comment thread k8s/transfer.sh/values.yaml Outdated

Comment thread README.md Outdated

Add explicit S3 credential mode

e3700f3

francescayeye requested changes Sep 25, 2026

View reviewed changes

BOPOHA added 3 commits September 25, 2026 23:08

Address S3 credential review feedback

fe9084e

Remove unnecessary S3 comment change

edcd2fe

Centralize S3 credential type constants

cc2472f

Remove unrelated cmd formatting change

389126a

francescayeye approved these changes Sep 27, 2026

View reviewed changes

francescayeye merged commit f040f28 into dutchcoders:main Sep 28, 2026
6 checks passed

stefanbenten mentioned this pull request Sep 28, 2026

update aws credentials build #659

Closed

Conversation

BOPOHA commented Nov 1, 2021

Uh oh!

Uh oh!

francescayeye left a comment

Choose a reason for hiding this comment

Uh oh!

stefanbenten commented Sep 23, 2026

Uh oh!

BOPOHA commented Sep 24, 2026

Uh oh!

Uh oh!

Uh oh!

BOPOHA commented Sep 24, 2026

Uh oh!

francescayeye Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

francescayeye Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

francescayeye Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francescayeye left a comment

Choose a reason for hiding this comment

Uh oh!

francescayeye Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

francescayeye Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

francescayeye left a comment

Choose a reason for hiding this comment

Uh oh!

BOPOHA commented Sep 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

francescayeye Sep 25, 2026 •

edited

Loading