From a13a09a11df5c18f968b113cb72d26b32f7c4753 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Tue, 28 Jul 2026 19:03:27 +0200 Subject: [PATCH 1/4] fix(aws): reject incomplete static credential configuration Signed-off-by: Minh Vu --- catalog/glue/glue.go | 5 ++++- catalog/glue/glue_test.go | 17 +++++++++++++++++ io/gocloud/s3.go | 7 +++++-- io/gocloud/s3_test.go | 17 +++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/catalog/glue/glue.go b/catalog/glue/glue.go index 171b62b9e..ccb55f251 100644 --- a/catalog/glue/glue.go +++ b/catalog/glue/glue.go @@ -123,7 +123,10 @@ func toAwsConfig(ctx context.Context, p iceberg.Properties) (aws.Config, error) } key, secret, token := p[AccessKeyID], p[SecretAccessKey], p[SessionToken] - if key != "" || secret != "" || token != "" { + if (key == "") != (secret == "") || (token != "" && key == "") { + return aws.Config{}, errors.New("glue.access-key-id and glue.secret-access-key must be configured together") + } + if key != "" { opts = append(opts, config.WithCredentialsProvider( credentials.NewStaticCredentialsProvider(key, secret, token))) } diff --git a/catalog/glue/glue_test.go b/catalog/glue/glue_test.go index 3fd0fa237..d76417ed7 100644 --- a/catalog/glue/glue_test.go +++ b/catalog/glue/glue_test.go @@ -50,6 +50,23 @@ import ( var errGluePurgeRemove = errors.New("glue purge remove failed") +func TestLoadAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { + t.Parallel() + + tests := []iceberg.Properties{ + {AccessKeyID: "access"}, + {SecretAccessKey: "secret"}, + {SessionToken: "token"}, + {AccessKeyID: "access", SessionToken: "token"}, + {SecretAccessKey: "secret", SessionToken: "token"}, + } + + for _, props := range tests { + _, err := toAwsConfig(context.Background(), props) + require.ErrorContains(t, err, "glue.access-key-id and glue.secret-access-key must be configured together") + } +} + type mockGlueClient struct { mock.Mock } diff --git a/io/gocloud/s3.go b/io/gocloud/s3.go index eaa190a9f..38dbad6da 100644 --- a/io/gocloud/s3.go +++ b/io/gocloud/s3.go @@ -68,9 +68,12 @@ func ParseAWSConfig(ctx context.Context, props map[string]string) (*aws.Config, accessKey, secretAccessKey := props[io.S3AccessKeyID], props[io.S3SecretAccessKey] token := props[io.S3SessionToken] - if accessKey != "" || secretAccessKey != "" || token != "" { + if (accessKey == "") != (secretAccessKey == "") || (token != "" && accessKey == "") { + return nil, errors.New("s3.access-key-id and s3.secret-access-key must be configured together") + } + if accessKey != "" { opts = append(opts, config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider( - props[io.S3AccessKeyID], props[io.S3SecretAccessKey], props[io.S3SessionToken], + accessKey, secretAccessKey, token, ))) } diff --git a/io/gocloud/s3_test.go b/io/gocloud/s3_test.go index 1f352c9b2..58bdb7460 100644 --- a/io/gocloud/s3_test.go +++ b/io/gocloud/s3_test.go @@ -114,6 +114,23 @@ func TestResolveS3AWSConfigCredentialPrecedence(t *testing.T) { }) } +func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { + t.Parallel() + + tests := []map[string]string{ + {io.S3AccessKeyID: "access"}, + {io.S3SecretAccessKey: "secret"}, + {io.S3SessionToken: "token"}, + {io.S3AccessKeyID: "access", io.S3SessionToken: "token"}, + {io.S3SecretAccessKey: "secret", io.S3SessionToken: "token"}, + } + + for _, props := range tests { + _, err := ParseAWSConfig(context.Background(), props) + require.ErrorContains(t, err, "s3.access-key-id and s3.secret-access-key must be configured together") + } +} + func TestParseAWSConfigRemoteSigningEnabled(t *testing.T) { t.Parallel() From 1e34f6b6f11fc79e7b18e4e1f8743c50e0f6e62d Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Tue, 28 Jul 2026 19:08:15 +0200 Subject: [PATCH 2/4] refactor(aws): clarify static credential validation Signed-off-by: Minh Vu --- catalog/glue/glue.go | 5 +++-- catalog/glue/glue_test.go | 10 ++++++++++ io/gocloud/s3.go | 5 +++-- io/gocloud/s3_test.go | 10 ++++++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/catalog/glue/glue.go b/catalog/glue/glue.go index ccb55f251..eaa928c33 100644 --- a/catalog/glue/glue.go +++ b/catalog/glue/glue.go @@ -123,10 +123,11 @@ func toAwsConfig(ctx context.Context, p iceberg.Properties) (aws.Config, error) } key, secret, token := p[AccessKeyID], p[SecretAccessKey], p[SessionToken] - if (key == "") != (secret == "") || (token != "" && key == "") { + hasKey, hasSecret := key != "", secret != "" + if hasKey != hasSecret || (token != "" && !hasKey) { return aws.Config{}, errors.New("glue.access-key-id and glue.secret-access-key must be configured together") } - if key != "" { + if hasKey { opts = append(opts, config.WithCredentialsProvider( credentials.NewStaticCredentialsProvider(key, secret, token))) } diff --git a/catalog/glue/glue_test.go b/catalog/glue/glue_test.go index d76417ed7..c01c01dcb 100644 --- a/catalog/glue/glue_test.go +++ b/catalog/glue/glue_test.go @@ -65,6 +65,16 @@ func TestLoadAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { _, err := toAwsConfig(context.Background(), props) require.ErrorContains(t, err, "glue.access-key-id and glue.secret-access-key must be configured together") } + + cfg, err := toAwsConfig(context.Background(), iceberg.Properties{ + AccessKeyID: "access", SecretAccessKey: "secret", SessionToken: "token", + }) + require.NoError(t, err) + creds, err := cfg.Credentials.Retrieve(context.Background()) + require.NoError(t, err) + require.Equal(t, "access", creds.AccessKeyID) + require.Equal(t, "secret", creds.SecretAccessKey) + require.Equal(t, "token", creds.SessionToken) } type mockGlueClient struct { diff --git a/io/gocloud/s3.go b/io/gocloud/s3.go index 38dbad6da..0ddaa2cb6 100644 --- a/io/gocloud/s3.go +++ b/io/gocloud/s3.go @@ -68,10 +68,11 @@ func ParseAWSConfig(ctx context.Context, props map[string]string) (*aws.Config, accessKey, secretAccessKey := props[io.S3AccessKeyID], props[io.S3SecretAccessKey] token := props[io.S3SessionToken] - if (accessKey == "") != (secretAccessKey == "") || (token != "" && accessKey == "") { + hasAccessKey, hasSecretAccessKey := accessKey != "", secretAccessKey != "" + if hasAccessKey != hasSecretAccessKey || (token != "" && !hasAccessKey) { return nil, errors.New("s3.access-key-id and s3.secret-access-key must be configured together") } - if accessKey != "" { + if hasAccessKey { opts = append(opts, config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider( accessKey, secretAccessKey, token, ))) diff --git a/io/gocloud/s3_test.go b/io/gocloud/s3_test.go index 58bdb7460..d72d8bff4 100644 --- a/io/gocloud/s3_test.go +++ b/io/gocloud/s3_test.go @@ -129,6 +129,16 @@ func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { _, err := ParseAWSConfig(context.Background(), props) require.ErrorContains(t, err, "s3.access-key-id and s3.secret-access-key must be configured together") } + + cfg, err := ParseAWSConfig(context.Background(), map[string]string{ + io.S3AccessKeyID: "access", io.S3SecretAccessKey: "secret", io.S3SessionToken: "token", + }) + require.NoError(t, err) + creds, err := cfg.Credentials.Retrieve(context.Background()) + require.NoError(t, err) + require.Equal(t, "access", creds.AccessKeyID) + require.Equal(t, "secret", creds.SecretAccessKey) + require.Equal(t, "token", creds.SessionToken) } func TestParseAWSConfigRemoteSigningEnabled(t *testing.T) { From 2f3c33eaee8d8c52f265a3db309406063fd255e7 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 1 Aug 2026 17:59:56 +0200 Subject: [PATCH 3/4] fix(aws): apply credential validation consistently --- catalog/glue/glue.go | 8 ++--- catalog/glue/glue_test.go | 25 +++++++++------ internal/awsconfig/credentials.go | 38 ++++++++++++++++++++++ io/gocloud/s3.go | 19 +++++++---- io/gocloud/s3_test.go | 53 +++++++++++++++++++++++-------- 5 files changed, 109 insertions(+), 34 deletions(-) create mode 100644 internal/awsconfig/credentials.go diff --git a/catalog/glue/glue.go b/catalog/glue/glue.go index eaa928c33..02615f0a0 100644 --- a/catalog/glue/glue.go +++ b/catalog/glue/glue.go @@ -32,6 +32,7 @@ import ( "github.com/apache/iceberg-go" "github.com/apache/iceberg-go/catalog" "github.com/apache/iceberg-go/catalog/internal" + internalaws "github.com/apache/iceberg-go/internal/awsconfig" "github.com/apache/iceberg-go/io" "github.com/apache/iceberg-go/metrics" "github.com/apache/iceberg-go/table" @@ -123,11 +124,10 @@ func toAwsConfig(ctx context.Context, p iceberg.Properties) (aws.Config, error) } key, secret, token := p[AccessKeyID], p[SecretAccessKey], p[SessionToken] - hasKey, hasSecret := key != "", secret != "" - if hasKey != hasSecret || (token != "" && !hasKey) { - return aws.Config{}, errors.New("glue.access-key-id and glue.secret-access-key must be configured together") + if err := internalaws.ValidateStaticCredentials(AccessKeyID, SecretAccessKey, SessionToken, key, secret, token); err != nil { + return aws.Config{}, err } - if hasKey { + if key != "" { opts = append(opts, config.WithCredentialsProvider( credentials.NewStaticCredentialsProvider(key, secret, token))) } diff --git a/catalog/glue/glue_test.go b/catalog/glue/glue_test.go index c01c01dcb..c0a09a24c 100644 --- a/catalog/glue/glue_test.go +++ b/catalog/glue/glue_test.go @@ -53,17 +53,24 @@ var errGluePurgeRemove = errors.New("glue purge remove failed") func TestLoadAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { t.Parallel() - tests := []iceberg.Properties{ - {AccessKeyID: "access"}, - {SecretAccessKey: "secret"}, - {SessionToken: "token"}, - {AccessKeyID: "access", SessionToken: "token"}, - {SecretAccessKey: "secret", SessionToken: "token"}, + tests := []struct { + name string + props iceberg.Properties + err string + }{ + {"access key only", iceberg.Properties{AccessKeyID: "access"}, "glue.access-key-id and glue.secret-access-key must be configured together"}, + {"secret key only", iceberg.Properties{SecretAccessKey: "secret"}, "glue.access-key-id and glue.secret-access-key must be configured together"}, + {"session token only", iceberg.Properties{SessionToken: "token"}, "glue.session-token requires glue.access-key-id and glue.secret-access-key"}, + {"access key and token", iceberg.Properties{AccessKeyID: "access", SessionToken: "token"}, "glue.access-key-id and glue.secret-access-key must be configured together"}, + {"secret key and token", iceberg.Properties{SecretAccessKey: "secret", SessionToken: "token"}, "glue.access-key-id and glue.secret-access-key must be configured together"}, } - for _, props := range tests { - _, err := toAwsConfig(context.Background(), props) - require.ErrorContains(t, err, "glue.access-key-id and glue.secret-access-key must be configured together") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := toAwsConfig(context.Background(), tt.props) + require.ErrorContains(t, err, tt.err) + }) } cfg, err := toAwsConfig(context.Background(), iceberg.Properties{ diff --git a/internal/awsconfig/credentials.go b/internal/awsconfig/credentials.go new file mode 100644 index 000000000..513ef9840 --- /dev/null +++ b/internal/awsconfig/credentials.go @@ -0,0 +1,38 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package awsconfig + +import ( + "errors" + "fmt" +) + +var ErrIncompleteStaticCredentials = errors.New("incomplete static AWS credentials") + +// ValidateStaticCredentials ensures a session token is only configured with +// the complete key pair required by AWS static credential providers. +func ValidateStaticCredentials(keyName, secretName, tokenName, key, secret, token string) error { + if key == "" && secret == "" && token != "" { + return fmt.Errorf("%w: %s requires %s and %s", ErrIncompleteStaticCredentials, tokenName, keyName, secretName) + } + if (key == "") != (secret == "") { + return fmt.Errorf("%w: %s and %s must be configured together", ErrIncompleteStaticCredentials, keyName, secretName) + } + + return nil +} diff --git a/io/gocloud/s3.go b/io/gocloud/s3.go index 0ddaa2cb6..39e978e1e 100644 --- a/io/gocloud/s3.go +++ b/io/gocloud/s3.go @@ -28,6 +28,7 @@ import ( "strconv" "time" + internalaws "github.com/apache/iceberg-go/internal/awsconfig" "github.com/apache/iceberg-go/io" "github.com/apache/iceberg-go/utils" "github.com/aws/aws-sdk-go-v2/aws" @@ -68,11 +69,10 @@ func ParseAWSConfig(ctx context.Context, props map[string]string) (*aws.Config, accessKey, secretAccessKey := props[io.S3AccessKeyID], props[io.S3SecretAccessKey] token := props[io.S3SessionToken] - hasAccessKey, hasSecretAccessKey := accessKey != "", secretAccessKey != "" - if hasAccessKey != hasSecretAccessKey || (token != "" && !hasAccessKey) { - return nil, errors.New("s3.access-key-id and s3.secret-access-key must be configured together") + if err := internalaws.ValidateStaticCredentials(io.S3AccessKeyID, io.S3SecretAccessKey, io.S3SessionToken, accessKey, secretAccessKey, token); err != nil { + return nil, err } - if hasAccessKey { + if accessKey != "" { opts = append(opts, config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider( accessKey, secretAccessKey, token, ))) @@ -178,6 +178,13 @@ func resolveUsePathStyle(endpoint string, props map[string]string) bool { // resolveS3AWSConfig returns the AWS config for the S3 FileIO, preferring an // ambient context config but letting explicit s3.* credentials override it. func resolveS3AWSConfig(ctx context.Context, props map[string]string) (*aws.Config, error) { + if err := internalaws.ValidateStaticCredentials( + io.S3AccessKeyID, io.S3SecretAccessKey, io.S3SessionToken, + props[io.S3AccessKeyID], props[io.S3SecretAccessKey], props[io.S3SessionToken], + ); err != nil { + return nil, err + } + var ( base *aws.Config err error @@ -199,9 +206,7 @@ func resolveS3AWSConfig(ctx context.Context, props map[string]string) (*aws.Conf cfg.Region = r } - // A complete explicit key pair overrides the credentials. A partial set - // (missing the access key or secret) falls through to the context/default - // chain rather than installing a provider with blank fields. + // A complete explicit key pair overrides the credentials. if props[io.S3AccessKeyID] != "" && props[io.S3SecretAccessKey] != "" { cfg.Credentials = credentials.NewStaticCredentialsProvider( props[io.S3AccessKeyID], props[io.S3SecretAccessKey], props[io.S3SessionToken], diff --git a/io/gocloud/s3_test.go b/io/gocloud/s3_test.go index d72d8bff4..2f6dafa05 100644 --- a/io/gocloud/s3_test.go +++ b/io/gocloud/s3_test.go @@ -95,12 +95,11 @@ func TestResolveS3AWSConfigCredentialPrecedence(t *testing.T) { assert.Nil(t, shared.HTTPClient, "shared ctx config must stay unmutated") }) - // A partial key set (missing the secret) must not clobber the context creds. - t.Run("partial props creds fall through", func(t *testing.T) { + // A partial key set must fail consistently, even with ambient credentials. + t.Run("partial props creds are rejected", func(t *testing.T) { t.Parallel() - cfg, err := resolveS3AWSConfig(ctxWith, map[string]string{io.S3AccessKeyID: "PARTIAL"}) - require.NoError(t, err) - assert.Equal(t, "CTX", retrieve(t, cfg), "incomplete override must keep ctx creds") + _, err := resolveS3AWSConfig(ctxWith, map[string]string{io.S3AccessKeyID: "PARTIAL"}) + require.ErrorContains(t, err, "s3.access-key-id and s3.secret-access-key must be configured together") }) // Explicit region overrides the context config's region, without mutating it. @@ -117,19 +116,45 @@ func TestResolveS3AWSConfigCredentialPrecedence(t *testing.T) { func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { t.Parallel() - tests := []map[string]string{ - {io.S3AccessKeyID: "access"}, - {io.S3SecretAccessKey: "secret"}, - {io.S3SessionToken: "token"}, - {io.S3AccessKeyID: "access", io.S3SessionToken: "token"}, - {io.S3SecretAccessKey: "secret", io.S3SessionToken: "token"}, + tests := []struct { + name string + props map[string]string + err string + }{ + {"access key only", map[string]string{io.S3AccessKeyID: "access"}, "s3.access-key-id and s3.secret-access-key must be configured together"}, + {"secret key only", map[string]string{io.S3SecretAccessKey: "secret"}, "s3.access-key-id and s3.secret-access-key must be configured together"}, + {"session token only", map[string]string{io.S3SessionToken: "token"}, "s3.session-token requires s3.access-key-id and s3.secret-access-key"}, + {"access key and token", map[string]string{io.S3AccessKeyID: "access", io.S3SessionToken: "token"}, "s3.access-key-id and s3.secret-access-key must be configured together"}, + {"secret key and token", map[string]string{io.S3SecretAccessKey: "secret", io.S3SessionToken: "token"}, "s3.access-key-id and s3.secret-access-key must be configured together"}, } - for _, props := range tests { - _, err := ParseAWSConfig(context.Background(), props) - require.ErrorContains(t, err, "s3.access-key-id and s3.secret-access-key must be configured together") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := ParseAWSConfig(context.Background(), tt.props) + require.ErrorContains(t, err, tt.err) + }) } + t.Run("empty props use the default chain", func(t *testing.T) { + t.Parallel() + cfg, err := ParseAWSConfig(context.Background(), map[string]string{}) + require.NoError(t, err) + _, static := cfg.Credentials.(credentials.StaticCredentialsProvider) + assert.False(t, static) + }) + + t.Run("key pair without token is accepted", func(t *testing.T) { + t.Parallel() + cfg, err := ParseAWSConfig(context.Background(), map[string]string{ + io.S3AccessKeyID: "access", io.S3SecretAccessKey: "secret", + }) + require.NoError(t, err) + creds, err := cfg.Credentials.Retrieve(context.Background()) + require.NoError(t, err) + assert.Empty(t, creds.SessionToken) + }) + cfg, err := ParseAWSConfig(context.Background(), map[string]string{ io.S3AccessKeyID: "access", io.S3SecretAccessKey: "secret", io.S3SessionToken: "token", }) From 9109f0893f8fc66ea18becc027375517f701883a Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sun, 2 Aug 2026 15:13:45 +0200 Subject: [PATCH 4/4] test(aws): assert incomplete credential sentinel --- io/gocloud/s3_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/io/gocloud/s3_test.go b/io/gocloud/s3_test.go index 2f6dafa05..61a5abd54 100644 --- a/io/gocloud/s3_test.go +++ b/io/gocloud/s3_test.go @@ -26,6 +26,7 @@ import ( "testing" "time" + internalaws "github.com/apache/iceberg-go/internal/awsconfig" "github.com/apache/iceberg-go/io" "github.com/apache/iceberg-go/utils" "github.com/aws/aws-sdk-go-v2/aws" @@ -99,6 +100,7 @@ func TestResolveS3AWSConfigCredentialPrecedence(t *testing.T) { t.Run("partial props creds are rejected", func(t *testing.T) { t.Parallel() _, err := resolveS3AWSConfig(ctxWith, map[string]string{io.S3AccessKeyID: "PARTIAL"}) + require.ErrorIs(t, err, internalaws.ErrIncompleteStaticCredentials) require.ErrorContains(t, err, "s3.access-key-id and s3.secret-access-key must be configured together") }) @@ -132,6 +134,7 @@ func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() _, err := ParseAWSConfig(context.Background(), tt.props) + require.ErrorIs(t, err, internalaws.ErrIncompleteStaticCredentials) require.ErrorContains(t, err, tt.err) }) }