Skip to content

Commit 613b60a

Browse files
fix: replace custom AWS credential parser with SDK's built-in parser
Replace our hand-rolled ParseAWSCredentials() and loadAWSCredentialsFromFile() with the AWS SDK's native INI credential loading via config.WithSharedCredentialsFiles / config.WithSharedConfigFiles. This matches the pattern used by Velero (velero/pkg/repository/config/aws.go) and fixes: - Quoted credential values (e.g. aws_access_key_id = "AKID") that our custom parser could not strip correctly - Named profile support ([profile backup] sections) - Session token (aws_session_token) passthrough - Role assumption and other advanced SDK credential features Changes: - objectstore.go: Remove ParseAWSCredentials() and loadAWSCredentialsFromFile(). Change NewS3ObjectStore() to accept a credentialsFile path instead of raw []byte. Init() now delegates to the AWS SDK via WithSharedCredentialsFiles. Add writeCredentialsToTempFile() helper for the in-memory credential path. InitObjectStore() resolves CredentialsData to a temp file or uses CredentialsFile directly. - objectstore_test.go: Replace TestParseAWSCredentials and TestLoadAWSCredentialsFromFile with TestInitWithSDKCredentials (covers unquoted, quoted, named profile, session token), TestInitWithCredentialsData, TestInitWithMissingCredentialsFile, and TestWriteCredentialsToTempFile. - go.mod: Move aws-sdk-go-v2/credentials from direct to indirect dependency (no longer imported directly). - config/manager/manager.yaml: Mount /tmp as emptyDir so the controller can write temporary credential files when readOnlyRootFilesystem is set. Closes #22
1 parent cb0d4a8 commit 613b60a

4 files changed

Lines changed: 154 additions & 125 deletions

File tree

config/manager/manager.yaml

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,11 @@ spec:
9696
requests:
9797
cpu: 10m
9898
memory: 64Mi
99-
volumeMounts: []
100-
volumes: []
99+
volumeMounts:
100+
- name: tmp
101+
mountPath: /tmp
102+
volumes:
103+
- name: tmp
104+
emptyDir: {}
101105
serviceAccountName: controller-manager
102106
terminationGracePeriodSeconds: 10

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ toolchain go1.25.8
77
require (
88
github.com/aws/aws-sdk-go-v2 v1.41.1
99
github.com/aws/aws-sdk-go-v2/config v1.32.7
10-
github.com/aws/aws-sdk-go-v2/credentials v1.19.7
1110
github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.21.1
1211
github.com/aws/aws-sdk-go-v2/service/s3 v1.96.0
1312
github.com/evanphx/json-patch/v5 v5.9.11
@@ -31,6 +30,7 @@ require (
3130
github.com/Masterminds/semver/v3 v3.4.0 // indirect
3231
github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
3332
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.4 // indirect
33+
github.com/aws/aws-sdk-go-v2/credentials v1.19.7 // indirect
3434
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.17 // indirect
3535
github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.17 // indirect
3636
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.17 // indirect

pkg/uploader/objectstore.go

Lines changed: 59 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"github.com/aws/aws-sdk-go-v2/aws"
3030
"github.com/aws/aws-sdk-go-v2/aws/retry"
3131
"github.com/aws/aws-sdk-go-v2/config"
32-
"github.com/aws/aws-sdk-go-v2/credentials"
3332
"github.com/aws/aws-sdk-go-v2/feature/s3/manager"
3433
"github.com/aws/aws-sdk-go-v2/service/s3"
3534
s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"
@@ -48,10 +47,11 @@ type S3ObjectStore struct {
4847
}
4948

5049
// NewS3ObjectStore creates a new S3ObjectStore.
51-
// credentialsData contains the raw credential file content (INI-style with
52-
// aws_access_key_id and aws_secret_access_key). Pass nil to use default
53-
// AWS credential chain (env vars, instance profile, etc.).
54-
func NewS3ObjectStore(bucket, prefix, region string, credentialsData []byte) (*S3ObjectStore, error) {
50+
// credentialsFile is the path to an AWS credentials file (INI-style).
51+
// Pass empty string to use the default AWS credential chain (env vars,
52+
// instance profile, etc.). The file is parsed by the AWS SDK which handles
53+
// quoted values, profiles, session tokens, and role assumption.
54+
func NewS3ObjectStore(bucket, prefix, region, credentialsFile string) (*S3ObjectStore, error) {
5555
store := &S3ObjectStore{
5656
bucket: bucket,
5757
prefix: prefix,
@@ -62,8 +62,8 @@ func NewS3ObjectStore(bucket, prefix, region string, credentialsData []byte) (*S
6262
"prefix": prefix,
6363
"region": region,
6464
}
65-
if len(credentialsData) > 0 {
66-
configMap["credentialsData"] = string(credentialsData)
65+
if credentialsFile != "" {
66+
configMap["credentialsFile"] = credentialsFile
6767
}
6868

6969
if err := store.Init(configMap); err != nil {
@@ -75,8 +75,10 @@ func NewS3ObjectStore(bucket, prefix, region string, credentialsData []byte) (*S
7575

7676
// Init initializes the ObjectStore with the provided config.
7777
// This implements velero.ObjectStore.Init().
78-
// Expected config keys: bucket, prefix, region, and either credentialsData
79-
// (raw credential content) or credentialsFile (path to credentials file).
78+
// Expected config keys: bucket, prefix, region, and optionally credentialsFile
79+
// (path to credentials file) or credentialsData (raw credential content that
80+
// will be written to a temp file). Credential parsing is delegated to the AWS
81+
// SDK via config.WithSharedCredentialsFiles, matching Velero's approach.
8082
func (s *S3ObjectStore) Init(configMap map[string]string) error {
8183
bucket := configMap["bucket"]
8284
prefix := configMap["prefix"]
@@ -98,19 +100,25 @@ func (s *S3ObjectStore) Init(configMap map[string]string) error {
98100
opts = append(opts, config.WithRegion(region))
99101
}
100102

101-
// Load credentials: prefer in-memory data, fall back to file path
102-
if credData := configMap["credentialsData"]; credData != "" {
103-
creds, err := ParseAWSCredentials([]byte(credData))
104-
if err != nil {
105-
return fmt.Errorf("failed to parse credentials: %w", err)
106-
}
107-
opts = append(opts, config.WithCredentialsProvider(creds))
108-
} else if credFile := configMap["credentialsFile"]; credFile != "" {
109-
creds, err := loadAWSCredentialsFromFile(credFile)
103+
// Load credentials using the AWS SDK's built-in parser, matching Velero's
104+
// pattern (velero/pkg/repository/config/aws.go). This correctly handles
105+
// quoted values, named profiles, session tokens, and role assumption.
106+
if credFile := configMap["credentialsFile"]; credFile != "" {
107+
opts = append(opts,
108+
config.WithSharedCredentialsFiles([]string{credFile}),
109+
config.WithSharedConfigFiles([]string{credFile}),
110+
)
111+
} else if credData := configMap["credentialsData"]; credData != "" {
112+
// Write in-memory credentials to a temp file for the AWS SDK to parse.
113+
tmpFile, err := writeCredentialsToTempFile([]byte(credData))
110114
if err != nil {
111-
return fmt.Errorf("failed to load credentials: %w", err)
115+
return fmt.Errorf("failed to write credentials to temp file: %w", err)
112116
}
113-
opts = append(opts, config.WithCredentialsProvider(creds))
117+
defer func() { _ = os.Remove(tmpFile) }()
118+
opts = append(opts,
119+
config.WithSharedCredentialsFiles([]string{tmpFile}),
120+
config.WithSharedConfigFiles([]string{tmpFile}),
121+
)
114122
}
115123

116124
// Add retry configuration for transient errors
@@ -298,71 +306,56 @@ func (s *S3ObjectStore) GetObjectBytes(key string) ([]byte, error) {
298306
return io.ReadAll(reader)
299307
}
300308

301-
// ParseAWSCredentials parses AWS credentials from raw INI-style content.
302-
// The expected format contains aws_access_key_id and aws_secret_access_key
303-
// key=value pairs (with optional [default] section header).
304-
func ParseAWSCredentials(data []byte) (aws.CredentialsProvider, error) {
305-
var accessKeyID, secretAccessKey string
306-
307-
for line := range strings.SplitSeq(string(data), "\n") {
308-
line = strings.TrimSpace(line)
309-
if strings.HasPrefix(line, "aws_access_key_id") {
310-
parts := strings.SplitN(line, "=", 2)
311-
if len(parts) == 2 {
312-
accessKeyID = strings.TrimSpace(parts[1])
313-
}
314-
} else if strings.HasPrefix(line, "aws_secret_access_key") {
315-
parts := strings.SplitN(line, "=", 2)
316-
if len(parts) == 2 {
317-
secretAccessKey = strings.TrimSpace(parts[1])
318-
}
319-
}
309+
// writeCredentialsToTempFile writes credential data to a temporary file and returns
310+
// the file path. The caller is responsible for removing the file when done.
311+
// The file is created with restrictive permissions (0600) to protect credentials.
312+
func writeCredentialsToTempFile(data []byte) (string, error) {
313+
tmpFile, err := os.CreateTemp("", "aws-credentials-*")
314+
if err != nil {
315+
return "", fmt.Errorf("failed to create temp credentials file: %w", err)
320316
}
321317

322-
if accessKeyID == "" || secretAccessKey == "" {
323-
return nil, fmt.Errorf(
324-
"credentials data missing aws_access_key_id or aws_secret_access_key",
325-
)
318+
if _, err := tmpFile.Write(data); err != nil {
319+
_ = tmpFile.Close()
320+
_ = os.Remove(tmpFile.Name())
321+
return "", fmt.Errorf("failed to write temp credentials file: %w", err)
326322
}
327323

328-
return credentials.NewStaticCredentialsProvider(
329-
accessKeyID, secretAccessKey, "",
330-
), nil
331-
}
332-
333-
// loadAWSCredentialsFromFile loads AWS credentials from a file path.
334-
// Used by the datamover pod where credentials are mounted as a volume.
335-
func loadAWSCredentialsFromFile(path string) (aws.CredentialsProvider, error) {
336-
data, err := os.ReadFile(path)
337-
if err != nil {
338-
return nil, fmt.Errorf("failed to read credentials file: %w", err)
324+
if err := tmpFile.Close(); err != nil {
325+
_ = os.Remove(tmpFile.Name())
326+
return "", fmt.Errorf("failed to close temp credentials file: %w", err)
339327
}
340-
return ParseAWSCredentials(data)
328+
329+
return tmpFile.Name(), nil
341330
}
342331

343332
// InitObjectStore creates an ObjectStore based on the provider type.
344-
// If CredentialsData is set, it is used directly. Otherwise, CredentialsFile
345-
// is read from disk (used by the datamover pod where credentials are volume-mounted).
333+
// Credentials are resolved to a file path for the AWS SDK's built-in parser:
334+
// - If CredentialsData is set, it is written to a temp file.
335+
// - If CredentialsFile is set, the path is used directly.
346336
func InitObjectStore(cfg *UploaderConfig) (velero.ObjectStore, error) {
347337
if cfg.BSLProvider == "" {
348338
return nil, fmt.Errorf("BSL provider is required but was empty")
349339
}
350340

351-
// Resolve credentials: prefer in-memory data, fall back to reading file
352-
credData := cfg.CredentialsData
353-
if len(credData) == 0 && cfg.CredentialsFile != "" {
354-
var err error
355-
credData, err = os.ReadFile(cfg.CredentialsFile)
341+
// Resolve credentials to a file path for the AWS SDK.
342+
// Prefer in-memory data (controller path), fall back to file (uploader pod path).
343+
credentialsFile := ""
344+
if len(cfg.CredentialsData) > 0 {
345+
tmpFile, err := writeCredentialsToTempFile(cfg.CredentialsData)
356346
if err != nil {
357-
return nil, fmt.Errorf("failed to read credentials file %s: %w",
358-
cfg.CredentialsFile, err)
347+
return nil, fmt.Errorf("failed to write credentials: %w", err)
359348
}
349+
defer func() { _ = os.Remove(tmpFile) }()
350+
credentialsFile = tmpFile
351+
} else if cfg.CredentialsFile != "" {
352+
credentialsFile = cfg.CredentialsFile
360353
}
361354

362355
switch strings.ToLower(cfg.BSLProvider) {
363356
case "aws":
364357
return NewS3ObjectStore(
365-
cfg.BSLBucket, cfg.BSLPrefix, cfg.BSLRegion, credData,
358+
cfg.BSLBucket, cfg.BSLPrefix, cfg.BSLRegion, credentialsFile,
366359
)
367360
case "gcp":
368361
// TODO: Implement GCP Cloud Storage support (issue #11)
@@ -373,7 +366,7 @@ func InitObjectStore(cfg *UploaderConfig) (velero.ObjectStore, error) {
373366
default:
374367
// Try S3-compatible for unknown providers
375368
return NewS3ObjectStore(
376-
cfg.BSLBucket, cfg.BSLPrefix, cfg.BSLRegion, credData,
369+
cfg.BSLBucket, cfg.BSLPrefix, cfg.BSLRegion, credentialsFile,
377370
)
378371
}
379372
}

0 commit comments

Comments
 (0)