diff options
| author | Chris Lu <chrislusf@users.noreply.github.com> | 2025-11-28 13:28:17 -0800 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-11-28 13:28:17 -0800 |
| commit | bd419fda5119ae7f7225cfca0fcb98bce04f4980 (patch) | |
| tree | 16e1f6d020daf38663c44c423f935d3a0546f7f8 /weed/s3api | |
| parent | 7e4bab80326866b1ade17bf3ebddb96999507a25 (diff) | |
| download | seaweedfs-bd419fda5119ae7f7225cfca0fcb98bce04f4980.tar.xz seaweedfs-bd419fda5119ae7f7225cfca0fcb98bce04f4980.zip | |
fix: copy to bucket with default SSE-S3 encryption fails (#7562) (#7568)
* filer use context without cancellation
* pass along context
* 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
* fmt
* refactor: simplify isOrphanedSSES3Header function logic
Remove redundant existence check since the caller iterates through
metadata map, making the check unnecessary. Improves readability
while maintaining the same functionality.
Diffstat (limited to 'weed/s3api')
| -rw-r--r-- | weed/s3api/s3_sse_s3.go | 16 | ||||
| -rw-r--r-- | weed/s3api/s3_sse_s3_test.go | 17 | ||||
| -rw-r--r-- | weed/s3api/s3api_object_handlers_copy.go | 31 | ||||
| -rw-r--r-- | weed/s3api/s3api_object_handlers_copy_test.go | 88 |
4 files changed, 135 insertions, 17 deletions
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,13 +630,21 @@ 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{ s3_constants.AmzServerSideEncryption: []byte("aws:kms"), @@ -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..0c465d3db 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) @@ -297,7 +303,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request // For non-versioned destination, use regular copy // Remove any versioning-related metadata from source that shouldn't carry over cleanupVersioningMetadata(dstEntry.Extended) - + dstPath := util.FullPath(fmt.Sprintf("%s/%s%s", s3a.option.BucketsPath, dstBucket, dstObject)) dstDir, dstName := dstPath.DirAndName() @@ -2350,6 +2356,27 @@ 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 + } + + // The header is AmzServerSideEncryption. Check if its value indicates SSE-S3. + if string(metadata[headerKey]) == "AES256" { + // It's an SSE-S3 header. It's orphaned if the actual encryption 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..369d2aac9 100644 --- a/weed/s3api/s3api_object_handlers_copy_test.go +++ b/weed/s3api/s3api_object_handlers_copy_test.go @@ -2,12 +2,13 @@ package s3api import ( "fmt" - "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "net/http" "reflect" "sort" "strings" "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) type H map[string]string @@ -442,10 +443,10 @@ func transferHeaderToH(data map[string][]string) H { // This addresses issue #7505 where copies were incorrectly creating versions for non-versioned buckets. func TestShouldCreateVersionForCopy(t *testing.T) { testCases := []struct { - name string - versioningState string - expectedResult bool - description string + name string + versioningState string + expectedResult bool + description string }{ { name: "VersioningEnabled", @@ -503,7 +504,7 @@ func TestCleanupVersioningMetadata(t *testing.T) { removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtDeleteMarkerKey, s3_constants.ExtIsLatestKey, s3_constants.ExtETagKey}, }, { - name: "HandlesEmptyMetadata", + name: "HandlesEmptyMetadata", sourceMetadata: map[string][]byte{}, expectedKeys: []string{}, removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtDeleteMarkerKey, s3_constants.ExtIsLatestKey, s3_constants.ExtETagKey}, @@ -511,11 +512,11 @@ func TestCleanupVersioningMetadata(t *testing.T) { { name: "PreservesNonVersioningMetadata", sourceMetadata: map[string][]byte{ - s3_constants.ExtVersionIdKey: []byte("version-456"), - s3_constants.ExtETagKey: []byte("\"def456\""), - "X-Amz-Meta-Custom": []byte("value1"), - "X-Amz-Meta-Another": []byte("value2"), - s3_constants.ExtIsLatestKey: []byte("true"), + s3_constants.ExtVersionIdKey: []byte("version-456"), + s3_constants.ExtETagKey: []byte("\"def456\""), + "X-Amz-Meta-Custom": []byte("value1"), + "X-Amz-Meta-Another": []byte("value2"), + s3_constants.ExtIsLatestKey: []byte("true"), }, expectedKeys: []string{"X-Amz-Meta-Custom", "X-Amz-Meta-Another"}, removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtETagKey, s3_constants.ExtIsLatestKey}, @@ -639,3 +640,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) + } + }) + } +} |
