gsh-digital-services/seaweedFS
5
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: copy to bucket with default SSE-S3 encryption fails (#7562) (#7568)

Browse Source 
* 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.
This commit is contained in:
Chris Lu
2025-11-28 13:28:17 -08:00
committed by GitHub
parent 7e4bab8032
commit bd419fda51
5 changed files with 364 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
weed/s3api/s3_sse_s3.go
View File

@@ -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

19
weed/s3api/s3_sse_s3_test.go
View File

@@ -630,11 +630,19 @@ 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: true,
expected: false, // Should not be considered encrypted without the key
},
{
name: "SSE-KMS metadata",
@@ -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 {

31
weed/s3api/s3api_object_handlers_copy.go
View File

@@ -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.

88
weed/s3api/s3api_object_handlers_copy_test.go
View File

@@ -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)
}
})
}
}