diff --git a/go.mod b/go.mod index 696cb0ce7c4..4f6eada5c38 100644 --- a/go.mod +++ b/go.mod @@ -31,8 +31,8 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.8.1 github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.6.1 - github.com/aws/aws-sdk-go-v2 v1.30.3 github.com/aws/aws-sdk-go-v2/config v1.26.3 + github.com/aws/aws-sdk-go-v2/credentials v1.17.26 github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.15.11 github.com/aws/aws-sdk-go-v2/service/s3 v1.48.0 github.com/deckarep/golang-set/v2 v2.3.0 @@ -60,8 +60,8 @@ require ( github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.27.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.51.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.51.0 // indirect + github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.5.4 // indirect - github.com/aws/aws-sdk-go-v2/credentials v1.17.26 // indirect github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect diff --git a/pkg/common/common.go b/pkg/common/common.go index c42734fd3f5..a2f1523b979 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -336,6 +336,28 @@ func UpdateBackupStorageLocation(bsl *velerov1.BackupStorageLocation, bslSpec ve if _, ok := bslSpec.Config["checksumAlgorithm"]; !ok { bslSpec.Config["checksumAlgorithm"] = "" } + + // Auto-detect region for actual AWS S3 buckets (not S3-compatible storage) + // This only applies when: + // 1. No custom s3Url is configured (meaning it's real AWS S3) + // 2. No region is already specified in the config + // 3. A bucket name is provided in ObjectStorage + if _, hasS3Url := bslSpec.Config["s3Url"]; !hasS3Url { + if _, hasRegion := bslSpec.Config["region"]; !hasRegion { + if bslSpec.ObjectStorage != nil && bslSpec.ObjectStorage.Bucket != "" { + // Attempt to auto-detect the bucket's region + // AWS Security confirmed that GetBucketRegion works with anonymous credentials + // for both public and private buckets (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg) + if detectedRegion, err := aws.GetBucketRegion(bslSpec.ObjectStorage.Bucket); err == nil && detectedRegion != "" { + bslSpec.Config["region"] = detectedRegion + // Note: We successfully auto-detected the region. This is logged at a higher level + // to avoid importing logging dependencies here. + } + // If auto-detection fails, we continue without setting the region. + // The user can still manually specify it if needed. + } + } + } } } diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index f1f0053fb5f..ed533d501da 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -460,6 +460,106 @@ func TestUpdateBackupStorageLocation(t *testing.T) { }, }, }, + { + name: "AWS region auto-detection - region already specified", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-west-2", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-west-2", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + }, + }, + { + name: "AWS region auto-detection - skipped for S3-compatible storage", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "https://minio.example.com", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "s3Url": "https://minio.example.com", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "test-bucket", + }, + }, + }, + }, + }, + { + name: "AWS region auto-detection - real bucket (openshift-velero-plugin-s3-auto-region-test-1)", + bsl: &velerov1.BackupStorageLocation{}, + bslSpec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{}, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "openshift-velero-plugin-s3-auto-region-test-1", + }, + }, + }, + expectedBsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + RegistryDeploymentLabel: "True", + }, + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: "aws", + Config: map[string]string{ + "region": "us-east-1", + "checksumAlgorithm": "", + }, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "openshift-velero-plugin-s3-auto-region-test-1", + }, + }, + }, + }, + }, } for _, tt := range tests { diff --git a/pkg/storage/aws/s3.go b/pkg/storage/aws/s3.go index 3a7d25b4170..166619b71eb 100644 --- a/pkg/storage/aws/s3.go +++ b/pkg/storage/aws/s3.go @@ -6,8 +6,8 @@ import ( "net/http" "net/url" - "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go/aws/request" @@ -29,15 +29,23 @@ func GetBucketRegion(bucket string) (string, error) { // Client therefore needs to be configured with region. // In local dev environments, you might have ~/.aws/config that could be loaded and set with default region. // In cluster/CI environment, ~/.aws/config may not be configured, so set hinting region server explicitly. - // Also set to use anonymous credentials. If the bucket is private, this function would not work unless we modify it to take credentials. + // Also set to use anonymous credentials. This works for both public and private buckets as AWS Security + // confirmed that HeadBucket API (used by GetBucketRegion) doesn't enforce s3:ListBucket permissions + // for region retrieval - this is expected AWS behavior. cfg, err := config.LoadDefaultConfig(context.Background(), config.WithRegion("us-east-1"), // This is not default region being used, this is to specify a region hinting server that we will use to get region from. - config.WithCredentialsProvider(aws.AnonymousCredentials{}), ) if err != nil { return "", err } - region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket) + region, err = manager.GetBucketRegion(context.Background(), s3.NewFromConfig(cfg), bucket, func(o *s3.Options) { + // AWS Security confirmed that anonymous credentials can be used here for GetBucketRegion. + // The HeadBucket API endpoint used internally by GetBucketRegion does not enforce + // s3:ListBucket permissions for retrieving bucket region information. + // Reference: AWS Security response (Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg) + // This is expected AWS behavior, not a security vulnerability. + o.Credentials = credentials.NewStaticCredentialsProvider("anon-credentials", "anon-secret", "") + }) if region != "" { return region, nil } diff --git a/pkg/storage/aws/s3_test.go b/pkg/storage/aws/s3_test.go index 4843808034c..3df9527a0a9 100644 --- a/pkg/storage/aws/s3_test.go +++ b/pkg/storage/aws/s3_test.go @@ -14,12 +14,38 @@ func TestGetBucketRegion(t *testing.T) { wantErr bool }{ { + // Public bucket with s3:ListBucket permission works anonymously + // Note: While the bucket policy includes s3:ListBucket permission (shown below), + // AWS Security confirmed that HeadBucket API doesn't actually enforce this + // permission for region retrieval. The GetBucketRegion operation works with + // anonymous credentials on both public and private buckets. + // { + // "Version": "2012-10-17", + // "Statement": [ + // { + // "Sid": "publicList", + // "Effect": "Allow", + // "Principal": "*", + // "Action": "s3:ListBucket", + // "Resource": "arn:aws:s3:::openshift-velero-plugin-s3-auto-region-test-1" + // } + // ] + // } + // ❯ aws s3api head-bucket --bucket openshift-velero-plugin-s3-auto-region-test-1 --no-sign-request + // { + // "BucketRegion": "us-east-1", + // "AccessPointAlias": false + // } name: "openshift-velero-plugin-s3-auto-region-test-1", bucket: "openshift-velero-plugin-s3-auto-region-test-1", region: "us-east-1", wantErr: false, }, { + // Private bucket - AWS Security confirmed that HeadBucket API (used by GetBucketRegion) + // does NOT require credentials with s3:ListBucket permission for region retrieval. + // This is expected AWS behavior, not a security vulnerability. + // Reference: AWS Security Engagement ID: CACenGS4Mha_KeJ=e3jBSLD6rPZ2iNtfuJUv9QJViaCOt7GVNDg name: "openshift-velero-plugin-s3-auto-region-test-2", bucket: "openshift-velero-plugin-s3-auto-region-test-2", region: "us-west-1", diff --git a/tests/e2e/lib/dpa_helpers.go b/tests/e2e/lib/dpa_helpers.go index 1ddee4f7916..1ce4b45f023 100644 --- a/tests/e2e/lib/dpa_helpers.go +++ b/tests/e2e/lib/dpa_helpers.go @@ -324,6 +324,20 @@ func (v *DpaCustomResource) DoesBSLSpecMatchesDpa(dpaBSLSpec velero.BackupStorag configWithChecksumAlgorithm["checksumAlgorithm"] = "" dpaBSLSpec.Config = configWithChecksumAlgorithm } + + // Handle auto-detected region for AWS (real AWS S3, not S3-compatible storage) + _, hasS3Url := dpaBSLSpec.Config["s3Url"] + _, dpaHasRegion := dpaBSLSpec.Config["region"] + bslRegion, bslHasRegion := bslReal.Spec.Config["region"] + + // If this is real AWS (no s3Url), DPA has no region, but BSL has one (auto-detected) + if !hasS3Url && !dpaHasRegion && bslHasRegion { + // Accept the auto-detected region by adding it to the expected spec + if dpaBSLSpec.Config == nil { + dpaBSLSpec.Config = make(map[string]string) + } + dpaBSLSpec.Config["region"] = bslRegion + } } if !reflect.DeepEqual(dpaBSLSpec, bslReal.Spec) { log.Println(cmp.Diff(dpaBSLSpec, bslReal.Spec))