diff options
| author | Chris Lu <chrislusf@users.noreply.github.com> | 2025-11-25 09:56:20 -0800 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2025-11-25 09:56:20 -0800 |
| commit | f6a604c538c0afa3caec55f8f717a784878e064b (patch) | |
| tree | 35c2226e98e9eb721a0b8c95d2fd9deab3059b1a | |
| parent | a24c31de06093337cd31e42ed94de67ce1d5c95b (diff) | |
| download | seaweedfs-f6a604c538c0afa3caec55f8f717a784878e064b.tar.xz seaweedfs-f6a604c538c0afa3caec55f8f717a784878e064b.zip | |
S3: Fix encrypted file copy with multiple chunks (#7530) (#7546)
* S3: Fix encrypted file copy with multiple chunks (#7530)
When copying encrypted files with multiple chunks (encrypted volumes via
-filer.encryptVolumeData), the copied file could not be read. This was
caused by the chunk copy operation not preserving the IsCompressed flag,
which led to improper handling of compressed/encrypted data during upload.
The fix:
1. Modified uploadChunkData to accept an isCompressed parameter
2. Updated copySingleChunk to pass the source chunk's IsCompressed flag
3. Updated copySingleChunkForRange for partial copy operations
4. Updated all other callers to pass the appropriate compression flag
5. Added comprehensive tests for encrypted volume copy scenarios
This ensures that when copying chunks:
- The IsCompressed flag from the source chunk is passed to the upload
- Compressed data is marked as compressed, preventing double-compression
- Already-encrypted data is not re-encrypted (Cipher: false is correct)
- All chunk metadata (CipherKey, IsCompressed, ETag) is preserved
Tests added:
- TestCreateDestinationChunkPreservesEncryption: Verifies metadata preservation
- TestCopySingleChunkWithEncryption: Tests various encryption/compression scenarios
- TestCopyChunksPreservesMetadata: Tests multi-chunk metadata preservation
- TestEncryptedVolumeScenario: Documents and tests the exact issue #7530 scenario
Fixes #7530
* Address PR review feedback: simplify tests and improve clarity
- Removed TestUploadChunkDataCompressionFlag (panic-based test)
- Removed TestCopySingleChunkWithEncryption (duplicate coverage)
- Removed TestCopyChunksPreservesMetadata (duplicate coverage)
- Added ETag verification to TestEncryptedVolumeCopyScenario
- Renamed to TestEncryptedVolumeCopyScenario for better clarity
- All test coverage now in TestCreateDestinationChunkPreservesEncryption
and TestEncryptedVolumeCopyScenario which focus on the actual behavior
| -rw-r--r-- | weed/s3api/s3api_encrypted_volume_copy_test.go | 176 | ||||
| -rw-r--r-- | weed/s3api/s3api_key_rotation.go | 4 | ||||
| -rw-r--r-- | weed/s3api/s3api_object_handlers_copy.go | 21 | ||||
| -rw-r--r-- | weed/s3api/s3api_streaming_copy.go | 2 |
4 files changed, 190 insertions, 13 deletions
diff --git a/weed/s3api/s3api_encrypted_volume_copy_test.go b/weed/s3api/s3api_encrypted_volume_copy_test.go new file mode 100644 index 000000000..7e84384fb --- /dev/null +++ b/weed/s3api/s3api_encrypted_volume_copy_test.go @@ -0,0 +1,176 @@ +package s3api + +import ( + "bytes" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// TestCreateDestinationChunkPreservesEncryption tests that createDestinationChunk preserves CipherKey and IsCompressed +func TestCreateDestinationChunkPreservesEncryption(t *testing.T) { + s3a := &S3ApiServer{} + + testCases := []struct { + name string + sourceChunk *filer_pb.FileChunk + expectedOffset int64 + expectedSize uint64 + shouldPreserveCK bool + shouldPreserveIC bool + }{ + { + name: "Encrypted and compressed chunk", + sourceChunk: &filer_pb.FileChunk{ + Offset: 0, + Size: 1024, + CipherKey: []byte("test-cipher-key-1234567890123456"), + IsCompressed: true, + ETag: "test-etag", + }, + expectedOffset: 0, + expectedSize: 1024, + shouldPreserveCK: true, + shouldPreserveIC: true, + }, + { + name: "Only encrypted chunk", + sourceChunk: &filer_pb.FileChunk{ + Offset: 1024, + Size: 2048, + CipherKey: []byte("test-cipher-key-1234567890123456"), + IsCompressed: false, + ETag: "test-etag-2", + }, + expectedOffset: 1024, + expectedSize: 2048, + shouldPreserveCK: true, + shouldPreserveIC: false, + }, + { + name: "Only compressed chunk", + sourceChunk: &filer_pb.FileChunk{ + Offset: 2048, + Size: 512, + CipherKey: nil, + IsCompressed: true, + ETag: "test-etag-3", + }, + expectedOffset: 2048, + expectedSize: 512, + shouldPreserveCK: false, + shouldPreserveIC: true, + }, + { + name: "Unencrypted and uncompressed chunk", + sourceChunk: &filer_pb.FileChunk{ + Offset: 4096, + Size: 1024, + CipherKey: nil, + IsCompressed: false, + ETag: "test-etag-4", + }, + expectedOffset: 4096, + expectedSize: 1024, + shouldPreserveCK: false, + shouldPreserveIC: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + dstChunk := s3a.createDestinationChunk(tc.sourceChunk, tc.expectedOffset, tc.expectedSize) + + // Verify offset and size + if dstChunk.Offset != tc.expectedOffset { + t.Errorf("Expected offset %d, got %d", tc.expectedOffset, dstChunk.Offset) + } + if dstChunk.Size != tc.expectedSize { + t.Errorf("Expected size %d, got %d", tc.expectedSize, dstChunk.Size) + } + + // Verify CipherKey preservation + if tc.shouldPreserveCK { + if !bytes.Equal(dstChunk.CipherKey, tc.sourceChunk.CipherKey) { + t.Errorf("CipherKey not preserved: expected %v, got %v", tc.sourceChunk.CipherKey, dstChunk.CipherKey) + } + } else { + if len(dstChunk.CipherKey) > 0 { + t.Errorf("Expected no CipherKey, got %v", dstChunk.CipherKey) + } + } + + // Verify IsCompressed preservation + if dstChunk.IsCompressed != tc.shouldPreserveIC { + t.Errorf("IsCompressed not preserved: expected %v, got %v", tc.shouldPreserveIC, dstChunk.IsCompressed) + } + + // Verify ETag preservation + if dstChunk.ETag != tc.sourceChunk.ETag { + t.Errorf("ETag not preserved: expected %s, got %s", tc.sourceChunk.ETag, dstChunk.ETag) + } + }) + } +} + +// TestEncryptedVolumeCopyScenario documents the expected behavior for encrypted volumes (issue #7530) +func TestEncryptedVolumeCopyScenario(t *testing.T) { + t.Run("Scenario: Copy file on encrypted volume with multiple chunks", func(t *testing.T) { + // Scenario description for issue #7530: + // 1. Volume is started with -filer.encryptVolumeData + // 2. File is uploaded via S3 (automatically encrypted, multiple chunks) + // 3. File is copied/renamed via S3 CopyObject + // 4. Copied file should be readable + // + // The bug was that IsCompressed flag was not preserved during copy, + // causing the upload logic to potentially double-compress the data, + // making the copied file unreadable. + + sourceChunks := []*filer_pb.FileChunk{ + { + FileId: "1,abc123", + Offset: 0, + Size: 4194304, + CipherKey: util.GenCipherKey(), // Simulates encrypted volume + IsCompressed: true, // Simulates compression + ETag: "etag1", + }, + { + FileId: "2,def456", + Offset: 4194304, + Size: 4194304, + CipherKey: util.GenCipherKey(), + IsCompressed: true, + ETag: "etag2", + }, + } + + s3a := &S3ApiServer{} + + // Verify that createDestinationChunk preserves all necessary metadata + for i, srcChunk := range sourceChunks { + dstChunk := s3a.createDestinationChunk(srcChunk, srcChunk.Offset, srcChunk.Size) + + // Critical checks for issue #7530 + if !dstChunk.IsCompressed { + t.Errorf("Chunk %d: IsCompressed flag MUST be preserved to prevent double-compression", i) + } + if !bytes.Equal(dstChunk.CipherKey, srcChunk.CipherKey) { + t.Errorf("Chunk %d: CipherKey MUST be preserved for encrypted volumes", i) + } + if dstChunk.Offset != srcChunk.Offset { + t.Errorf("Chunk %d: Offset must be preserved", i) + } + if dstChunk.Size != srcChunk.Size { + t.Errorf("Chunk %d: Size must be preserved", i) + } + if dstChunk.ETag != srcChunk.ETag { + t.Errorf("Chunk %d: ETag must be preserved", i) + } + } + + t.Log("✓ All chunk metadata properly preserved for encrypted volume copy scenario") + }) +} + diff --git a/weed/s3api/s3api_key_rotation.go b/weed/s3api/s3api_key_rotation.go index 050a2826c..f2d406fb7 100644 --- a/weed/s3api/s3api_key_rotation.go +++ b/weed/s3api/s3api_key_rotation.go @@ -215,7 +215,7 @@ func (s3a *S3ApiServer) rotateSSECChunk(chunk *filer_pb.FileChunk, sourceKey, de newChunk.Size = uint64(len(reencryptedData)) // Upload re-encrypted data - if err := s3a.uploadChunkData(reencryptedData, assignResult); err != nil { + if err := s3a.uploadChunkData(reencryptedData, assignResult, false); err != nil { return nil, fmt.Errorf("upload re-encrypted data: %w", err) } @@ -263,7 +263,7 @@ func (s3a *S3ApiServer) rotateSSEKMSChunk(chunk *filer_pb.FileChunk, srcKeyID, d // 3. Update metadata accordingly // Upload data with new key (placeholder implementation) - if err := s3a.uploadChunkData(chunkData, assignResult); err != nil { + if err := s3a.uploadChunkData(chunkData, assignResult, false); err != nil { return nil, fmt.Errorf("upload rotated data: %w", err) } diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 86a7bc74b..91da98a0e 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -817,7 +817,7 @@ func (s3a *S3ApiServer) copySingleChunk(chunk *filer_pb.FileChunk, dstPath strin return nil, fmt.Errorf("download chunk data: %w", err) } - if err := s3a.uploadChunkData(chunkData, assignResult); err != nil { + if err := s3a.uploadChunkData(chunkData, assignResult, chunk.IsCompressed); err != nil { return nil, fmt.Errorf("upload chunk data: %w", err) } @@ -852,7 +852,7 @@ func (s3a *S3ApiServer) copySingleChunkForRange(originalChunk, rangeChunk *filer return nil, fmt.Errorf("download chunk range data: %w", err) } - if err := s3a.uploadChunkData(chunkData, assignResult); err != nil { + if err := s3a.uploadChunkData(chunkData, assignResult, originalChunk.IsCompressed); err != nil { return nil, fmt.Errorf("upload chunk range data: %w", err) } @@ -1140,13 +1140,14 @@ func (s3a *S3ApiServer) prepareChunkCopy(sourceFileId, dstPath string) (*filer_p } // uploadChunkData uploads chunk data to the destination using common upload logic -func (s3a *S3ApiServer) uploadChunkData(chunkData []byte, assignResult *filer_pb.AssignVolumeResponse) error { +// isCompressed indicates if the data is already compressed and should not be compressed again +func (s3a *S3ApiServer) uploadChunkData(chunkData []byte, assignResult *filer_pb.AssignVolumeResponse, isCompressed bool) error { dstUrl := fmt.Sprintf("http://%s/%s", assignResult.Location.Url, assignResult.FileId) uploadOption := &operation.UploadOption{ UploadUrl: dstUrl, - Cipher: false, - IsInputCompressed: false, + Cipher: false, // Data is already encrypted if source had CipherKey; don't re-encrypt + IsInputCompressed: isCompressed, MimeType: "", PairMap: nil, Jwt: security.EncodedJwt(assignResult.Auth), @@ -1367,7 +1368,7 @@ func (s3a *S3ApiServer) copyMultipartSSEKMSChunk(chunk *filer_pb.FileChunk, dest } // Upload the final data - if err := s3a.uploadChunkData(finalData, assignResult); err != nil { + if err := s3a.uploadChunkData(finalData, assignResult, false); err != nil { return nil, fmt.Errorf("upload chunk data: %w", err) } @@ -1497,7 +1498,7 @@ func (s3a *S3ApiServer) copyMultipartSSECChunk(chunk *filer_pb.FileChunk, copySo } // Upload the final data - if err := s3a.uploadChunkData(finalData, assignResult); err != nil { + if err := s3a.uploadChunkData(finalData, assignResult, false); err != nil { return nil, nil, fmt.Errorf("upload chunk data: %w", err) } @@ -1780,7 +1781,7 @@ func (s3a *S3ApiServer) copyCrossEncryptionChunk(chunk *filer_pb.FileChunk, sour // For unencrypted destination, finalData remains as decrypted plaintext // Upload the final data - if err := s3a.uploadChunkData(finalData, assignResult); err != nil { + if err := s3a.uploadChunkData(finalData, assignResult, false); err != nil { return nil, fmt.Errorf("upload chunk data: %w", err) } @@ -1991,7 +1992,7 @@ func (s3a *S3ApiServer) copyChunkWithReencryption(chunk *filer_pb.FileChunk, cop } // Upload the processed data - if err := s3a.uploadChunkData(finalData, assignResult); err != nil { + if err := s3a.uploadChunkData(finalData, assignResult, false); err != nil { return nil, fmt.Errorf("upload processed chunk data: %w", err) } @@ -2214,7 +2215,7 @@ func (s3a *S3ApiServer) copyChunkWithSSEKMSReencryption(chunk *filer_pb.FileChun } // Upload the processed data - if err := s3a.uploadChunkData(finalData, assignResult); err != nil { + if err := s3a.uploadChunkData(finalData, assignResult, false); err != nil { return nil, fmt.Errorf("upload processed chunk data: %w", err) } diff --git a/weed/s3api/s3api_streaming_copy.go b/weed/s3api/s3api_streaming_copy.go index 49480b6ea..457986858 100644 --- a/weed/s3api/s3api_streaming_copy.go +++ b/weed/s3api/s3api_streaming_copy.go @@ -493,7 +493,7 @@ func (scm *StreamingCopyManager) createChunkFromData(data []byte, offset int64, } // Upload data - if err := scm.s3a.uploadChunkData(data, assignResult); err != nil { + if err := scm.s3a.uploadChunkData(data, assignResult, false); err != nil { return nil, fmt.Errorf("upload chunk data: %w", err) } |
