From 16b822bf08e238c722636bc3f4d6810d64a1a1e3 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 28 Nov 2025 12:41:51 -0800 Subject: [PATCH] fix: copy to bucket with default SSE-S3 encryption fails (#7562) When copying an object from an encrypted bucket to a temporary unencrypted bucket, then to another bucket with default SSE-S3 encryption, the operation fails with 'invalid SSE-S3 source key type' error. Root cause: When objects are copied from an SSE-S3 encrypted bucket to an unencrypted bucket, the 'X-Amz-Server-Side-Encryption: AES256' header is preserved but the actual encryption key (SeaweedFSSSES3Key) is stripped. This creates an 'orphaned' SSE-S3 header that causes IsSSES3EncryptedInternal() to return true, triggering decryption logic with a nil key. Fix: 1. Modified IsSSES3EncryptedInternal() to require BOTH the AES256 header AND the SeaweedFSSSES3Key to be present before returning true 2. Added isOrphanedSSES3Header() to detect orphaned SSE-S3 headers 3. Updated copy handler to strip orphaned headers during copy operations Fixes #7562 --- test/s3/sse/s3_sse_integration_test.go | 228 ++++++++++++++++++ weed/s3api/s3_sse_s3.go | 16 +- weed/s3api/s3_sse_s3_test.go | 17 +- weed/s3api/s3api_object_handlers_copy.go | 30 ++- weed/s3api/s3api_object_handlers_copy_test.go | 65 +++++ 5 files changed, 351 insertions(+), 5 deletions(-) diff --git a/test/s3/sse/s3_sse_integration_test.go b/test/s3/sse/s3_sse_integration_test.go index 0b3ff8f04..7b939ea76 100644 --- a/test/s3/sse/s3_sse_integration_test.go +++ b/test/s3/sse/s3_sse_integration_test.go @@ -1856,6 +1856,234 @@ func TestCrossSSECopy(t *testing.T) { }) } +// TestCopyToBucketDefaultEncryptedRegression tests copying objects to buckets with default +// encryption enabled. This is a regression test for GitHub issue #7562 where copying from +// an unencrypted bucket to a bucket with SSE-S3 default encryption fails with error +// "invalid SSE-S3 source key type". +// +// The scenario is: +// 1. Create source bucket with SSE-S3 encryption +// 2. Upload encrypted object +// 3. Copy to temp bucket (unencrypted) - data is decrypted +// 4. Copy from temp bucket to dest bucket with SSE-S3 default encryption - this should re-encrypt +// +// The bug occurs because the source detection incorrectly identifies the temp object as +// SSE-S3 encrypted (based on leftover metadata) when it's actually unencrypted. +func TestCopyToBucketDefaultEncryptedRegression(t *testing.T) { + ctx := context.Background() + client, err := createS3Client(ctx, defaultConfig) + require.NoError(t, err, "Failed to create S3 client") + + // Create three buckets for the test scenario + srcBucket, err := createTestBucket(ctx, client, defaultConfig.BucketPrefix+"copy-src-") + require.NoError(t, err, "Failed to create source bucket") + defer cleanupTestBucket(ctx, client, srcBucket) + + tempBucket, err := createTestBucket(ctx, client, defaultConfig.BucketPrefix+"copy-temp-") + require.NoError(t, err, "Failed to create temp bucket") + defer cleanupTestBucket(ctx, client, tempBucket) + + dstBucket, err := createTestBucket(ctx, client, defaultConfig.BucketPrefix+"copy-dst-") + require.NoError(t, err, "Failed to create destination bucket") + defer cleanupTestBucket(ctx, client, dstBucket) + + // Enable SSE-S3 default encryption on source bucket + _, err = client.PutBucketEncryption(ctx, &s3.PutBucketEncryptionInput{ + Bucket: aws.String(srcBucket), + ServerSideEncryptionConfiguration: &types.ServerSideEncryptionConfiguration{ + Rules: []types.ServerSideEncryptionRule{ + { + ApplyServerSideEncryptionByDefault: &types.ServerSideEncryptionByDefault{ + SSEAlgorithm: types.ServerSideEncryptionAes256, + }, + }, + }, + }, + }) + require.NoError(t, err, "Failed to set source bucket encryption") + + // Enable SSE-S3 default encryption on destination bucket + _, err = client.PutBucketEncryption(ctx, &s3.PutBucketEncryptionInput{ + Bucket: aws.String(dstBucket), + ServerSideEncryptionConfiguration: &types.ServerSideEncryptionConfiguration{ + Rules: []types.ServerSideEncryptionRule{ + { + ApplyServerSideEncryptionByDefault: &types.ServerSideEncryptionByDefault{ + SSEAlgorithm: types.ServerSideEncryptionAes256, + }, + }, + }, + }, + }) + require.NoError(t, err, "Failed to set destination bucket encryption") + + // Test data + testData := []byte("Test data for copy-to-default-encrypted bucket regression test - GitHub issue #7562") + objectKey := "test-object.txt" + + t.Run("CopyEncrypted_ToTemp_ToEncrypted", func(t *testing.T) { + // Step 1: Upload object to source bucket (will be automatically encrypted) + _, err = client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: aws.String(srcBucket), + Key: aws.String(objectKey), + Body: bytes.NewReader(testData), + // No encryption header - bucket default applies + }) + require.NoError(t, err, "Failed to upload to source bucket") + + // Verify source object is encrypted + srcHead, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(srcBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to HEAD source object") + assert.Equal(t, types.ServerSideEncryptionAes256, srcHead.ServerSideEncryption, + "Source object should be SSE-S3 encrypted") + + // Step 2: Copy to temp bucket (unencrypted) - this should decrypt + _, err = client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey), + CopySource: aws.String(fmt.Sprintf("%s/%s", srcBucket, objectKey)), + // No encryption - data should be stored unencrypted + }) + require.NoError(t, err, "Failed to copy to temp bucket") + + // Verify temp object is unencrypted + tempHead, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to HEAD temp object") + assert.Empty(t, tempHead.ServerSideEncryption, + "Temp object should be unencrypted") + + // Verify temp object content is correct + tempGet, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to GET temp object") + tempData, err := io.ReadAll(tempGet.Body) + tempGet.Body.Close() + require.NoError(t, err, "Failed to read temp object") + assertDataEqual(t, testData, tempData, "Temp object data mismatch") + + // Step 3: Copy from temp bucket to dest bucket (with default encryption) + // THIS IS THE BUG: This copy fails with "invalid SSE-S3 source key type" + _, err = client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + CopySource: aws.String(fmt.Sprintf("%s/%s", tempBucket, objectKey)), + // No encryption header - bucket default should apply + }) + require.NoError(t, err, "Failed to copy to destination bucket - GitHub issue #7562") + + // Verify destination object is encrypted + dstHead, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to HEAD destination object") + assert.Equal(t, types.ServerSideEncryptionAes256, dstHead.ServerSideEncryption, + "Destination object should be SSE-S3 encrypted via bucket default") + + // Verify destination object content is correct + dstGet, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to GET destination object") + dstData, err := io.ReadAll(dstGet.Body) + dstGet.Body.Close() + require.NoError(t, err, "Failed to read destination object") + assertDataEqual(t, testData, dstData, "Destination object data mismatch after re-encryption") + }) + + t.Run("DirectCopyUnencrypted_ToEncrypted", func(t *testing.T) { + // Simpler test case: copy from unencrypted bucket directly to encrypted bucket + objectKey := "direct-copy-test.txt" + + // Upload to temp bucket (no default encryption) + _, err = client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey), + Body: bytes.NewReader(testData), + }) + require.NoError(t, err, "Failed to upload to temp bucket") + + // Copy to destination bucket with default encryption + _, err = client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + CopySource: aws.String(fmt.Sprintf("%s/%s", tempBucket, objectKey)), + }) + require.NoError(t, err, "Failed direct copy unencrypted to default-encrypted bucket") + + // Verify destination is encrypted + dstHead, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to HEAD destination object") + assert.Equal(t, types.ServerSideEncryptionAes256, dstHead.ServerSideEncryption, + "Object should be encrypted via bucket default") + + // Verify content + dstGet, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(dstBucket), + Key: aws.String(objectKey), + }) + require.NoError(t, err, "Failed to GET destination object") + dstData, err := io.ReadAll(dstGet.Body) + dstGet.Body.Close() + require.NoError(t, err, "Failed to read destination object") + assertDataEqual(t, testData, dstData, "Data mismatch after encryption") + }) + + t.Run("CopyWithExplicitSSES3Header", func(t *testing.T) { + // Test explicit SSE-S3 header during copy (should work even without bucket default) + objectKey := "explicit-sse-copy-test.txt" + + // Upload to temp bucket (unencrypted) + _, err = client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey), + Body: bytes.NewReader(testData), + }) + require.NoError(t, err, "Failed to upload to temp bucket") + + // Copy with explicit SSE-S3 header + _, err = client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: aws.String(tempBucket), // Same bucket, but with encryption + Key: aws.String(objectKey + "-encrypted"), + CopySource: aws.String(fmt.Sprintf("%s/%s", tempBucket, objectKey)), + ServerSideEncryption: types.ServerSideEncryptionAes256, + }) + require.NoError(t, err, "Failed copy with explicit SSE-S3 header") + + // Verify encrypted + head, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey + "-encrypted"), + }) + require.NoError(t, err, "Failed to HEAD object") + assert.Equal(t, types.ServerSideEncryptionAes256, head.ServerSideEncryption, + "Object should be SSE-S3 encrypted") + + // Verify content + getResp, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(tempBucket), + Key: aws.String(objectKey + "-encrypted"), + }) + require.NoError(t, err, "Failed to GET object") + data, err := io.ReadAll(getResp.Body) + getResp.Body.Close() + require.NoError(t, err, "Failed to read object") + assertDataEqual(t, testData, data, "Data mismatch") + }) +} + // REGRESSION TESTS FOR CRITICAL BUGS FIXED // These tests specifically target the IV storage bugs that were fixed diff --git a/weed/s3api/s3_sse_s3.go b/weed/s3api/s3_sse_s3.go index 22292bb9b..f7cbfa991 100644 --- a/weed/s3api/s3_sse_s3.go +++ b/weed/s3api/s3_sse_s3.go @@ -51,11 +51,21 @@ func IsSSES3RequestInternal(r *http.Request) bool { } // IsSSES3EncryptedInternal checks if the object metadata indicates SSE-S3 encryption +// An object is considered SSE-S3 encrypted only if it has BOTH the encryption header +// AND the actual encryption key metadata. This prevents false positives when an object +// has leftover headers from a previous encryption state (e.g., after being decrypted +// during a copy operation). Fixes GitHub issue #7562. func IsSSES3EncryptedInternal(metadata map[string][]byte) bool { - if sseAlgorithm, exists := metadata[s3_constants.AmzServerSideEncryption]; exists { - return string(sseAlgorithm) == SSES3Algorithm + // Check for SSE-S3 algorithm header + sseAlgorithm, hasHeader := metadata[s3_constants.AmzServerSideEncryption] + if !hasHeader || string(sseAlgorithm) != SSES3Algorithm { + return false } - return false + + // Must also have the actual encryption key to be considered encrypted + // Without the key, the object cannot be decrypted and should be treated as unencrypted + _, hasKey := metadata[s3_constants.SeaweedFSSSES3Key] + return hasKey } // GenerateSSES3Key generates a new SSE-S3 encryption key diff --git a/weed/s3api/s3_sse_s3_test.go b/weed/s3api/s3_sse_s3_test.go index 391692921..dc3378ae3 100644 --- a/weed/s3api/s3_sse_s3_test.go +++ b/weed/s3api/s3_sse_s3_test.go @@ -630,12 +630,20 @@ func TestSSES3IsEncryptedInternal(t *testing.T) { expected: false, }, { - name: "Valid SSE-S3 metadata", + name: "Valid SSE-S3 metadata with key", metadata: map[string][]byte{ s3_constants.AmzServerSideEncryption: []byte("AES256"), + s3_constants.SeaweedFSSSES3Key: []byte("test-key-data"), }, expected: true, }, + { + name: "SSE-S3 header without key (orphaned header - GitHub #7562)", + metadata: map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte("AES256"), + }, + expected: false, // Should not be considered encrypted without the key + }, { name: "SSE-KMS metadata", metadata: map[string][]byte{ @@ -650,6 +658,13 @@ func TestSSES3IsEncryptedInternal(t *testing.T) { }, expected: false, }, + { + name: "Key without header", + metadata: map[string][]byte{ + s3_constants.SeaweedFSSSES3Key: []byte("test-key-data"), + }, + expected: false, // Need both header and key + }, } for _, tc := range testCases { diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 4dd31c8ce..84351c22c 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -171,8 +171,14 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request // Skip encryption-specific headers that might conflict with destination encryption type skipHeader := false + // Skip orphaned SSE-S3 headers (header exists but key is missing) + // This prevents confusion about the object's actual encryption state + if isOrphanedSSES3Header(k, entry.Extended) { + skipHeader = true + } + // If we're doing cross-encryption, skip conflicting headers - if len(entry.GetChunks()) > 0 { + if !skipHeader && len(entry.GetChunks()) > 0 { // Detect source and destination encryption types srcHasSSEC := IsSSECEncrypted(entry.Extended) srcHasSSEKMS := IsSSEKMSEncrypted(entry.Extended) @@ -2350,6 +2356,28 @@ func shouldCreateVersionForCopy(versioningState string) bool { return versioningState == s3_constants.VersioningEnabled } +// isOrphanedSSES3Header checks if a header is an orphaned SSE-S3 encryption header. +// An orphaned header is one where the encryption indicator exists but the actual key is missing. +// This can happen when an object was previously encrypted but then copied without encryption, +// leaving behind the header but removing the key. These orphaned headers should be stripped +// during copy operations to prevent confusion about the object's actual encryption state. +// Fixes GitHub issue #7562. +func isOrphanedSSES3Header(headerKey string, metadata map[string][]byte) bool { + if headerKey != s3_constants.AmzServerSideEncryption { + return false + } + + // Check if this is an AES256 (SSE-S3) header + if val, exists := metadata[s3_constants.AmzServerSideEncryption]; exists { + if string(val) == "AES256" { + // It's orphaned if the actual key is missing + _, hasKey := metadata[s3_constants.SeaweedFSSSES3Key] + return !hasKey + } + } + return false +} + // shouldSkipEncryptionHeader determines if a header should be skipped when copying extended attributes // based on the source and destination encryption types. This consolidates the repetitive logic for // filtering encryption-related headers during copy operations. diff --git a/weed/s3api/s3api_object_handlers_copy_test.go b/weed/s3api/s3api_object_handlers_copy_test.go index 018c9f270..99e995600 100644 --- a/weed/s3api/s3api_object_handlers_copy_test.go +++ b/weed/s3api/s3api_object_handlers_copy_test.go @@ -639,3 +639,68 @@ func TestCopyVersioningIntegration(t *testing.T) { }) } } + +// TestIsOrphanedSSES3Header tests detection of orphaned SSE-S3 headers. +// This is a regression test for GitHub issue #7562 where copying from an +// encrypted bucket to an unencrypted bucket left behind the encryption header +// without the actual key, causing subsequent copy operations to fail. +func TestIsOrphanedSSES3Header(t *testing.T) { + testCases := []struct { + name string + headerKey string + metadata map[string][]byte + expected bool + }{ + { + name: "Not an encryption header", + headerKey: "X-Amz-Meta-Custom", + metadata: map[string][]byte{ + "X-Amz-Meta-Custom": []byte("value"), + }, + expected: false, + }, + { + name: "SSE-S3 header with key present (valid)", + headerKey: s3_constants.AmzServerSideEncryption, + metadata: map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte("AES256"), + s3_constants.SeaweedFSSSES3Key: []byte("key-data"), + }, + expected: false, + }, + { + name: "SSE-S3 header without key (orphaned - GitHub #7562)", + headerKey: s3_constants.AmzServerSideEncryption, + metadata: map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte("AES256"), + }, + expected: true, + }, + { + name: "SSE-KMS header (not SSE-S3)", + headerKey: s3_constants.AmzServerSideEncryption, + metadata: map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte("aws:kms"), + }, + expected: false, + }, + { + name: "Different header key entirely", + headerKey: s3_constants.SeaweedFSSSES3Key, + metadata: map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte("AES256"), + }, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := isOrphanedSSES3Header(tc.headerKey, tc.metadata) + if result != tc.expected { + t.Errorf("isOrphanedSSES3Header(%q, metadata) = %v, expected %v", + tc.headerKey, result, tc.expected) + } + }) + } +}