From 44d575100a3bba0c0e97af96cf625a27b7f8dec8 Mon Sep 17 00:00:00 2001 From: 7y-9 Date: Tue, 23 Jun 2026 13:19:24 +0800 Subject: [PATCH] fix(s3api): preserve requested AES256 copy encryption (#10049) * fix(s3api): preserve requested AES256 copy encryption Problem CopyObject metadata processing ignored an explicit x-amz-server-side-encryption: AES256 request header. A destination copy could lose the requested SSE-S3 metadata even though KMS requests were handled. Root cause processMetadataBytes only wrote the destination SSE header when the requested algorithm was aws:kms. Any other explicit SSE algorithm fell through to the source-preservation branch. Fix Write the requested SSE algorithm whenever x-amz-server-side-encryption is present, and keep KMS-specific metadata handling limited to aws:kms. Co-authored-by: Codex * fix(s3api): reject unsupported copy encryption algorithms A mistyped or unsupported x-amz-server-side-encryption value on a copy request slipped past validation and got persisted as the destination's algorithm header, advertising encryption that was never applied. Reject anything other than AES256 or aws:kms up front. * fix(s3api): write SSE key metadata for empty encrypted copies A zero-byte source copied with an explicit SSE request took the no-content branch and never ran the encryption path, leaving the object with a bare algorithm header but no key. HEAD then advertised SSE while the encryption-state machine saw the header as orphaned. Run the inline encryption path when the destination requests encryption so the key metadata is written too. * s3api: use SSEAlgorithmKMS constant in copy metadata handling * test(s3api): cover source SSE preservation on copy * test(iam): allow the local client's real source IP in SourceIp tests The aws:SourceIp allow policies hardcoded the loopback CIDRs, but a CI runner reaching the server over localhost can be observed with one of the host's RFC1918 addresses (the S3 endpoint is advertised on a 10.x interface), so the positive-condition PutObject was denied and the allow assertion flaked while the deny path passed trivially. Broaden the allow list to loopback plus private ranges via a shared helper, and log the denial on each failed attempt so any residual failure is diagnosable. --------- Co-authored-by: Codex Co-authored-by: Chris Lu --- .../s3_iam_inline_policy_condition_test.go | 104 +++++++++--------- weed/s3api/s3api_copy_validation.go | 14 ++- weed/s3api/s3api_object_handlers_copy.go | 36 +++--- .../s3api_object_handlers_copy_mime_test.go | 29 +++++ 4 files changed, 113 insertions(+), 70 deletions(-) diff --git a/test/s3/iam/s3_iam_inline_policy_condition_test.go b/test/s3/iam/s3_iam_inline_policy_condition_test.go index a09007b7d..ba54ef159 100644 --- a/test/s3/iam/s3_iam_inline_policy_condition_test.go +++ b/test/s3/iam/s3_iam_inline_policy_condition_test.go @@ -37,10 +37,43 @@ func isAccessDenied(err error) bool { return ok && awsErr.Code() == "AccessDenied" } +// localSourceAllowCIDRs lists every address a loopback-targeted test client may +// present as aws:SourceIp. The SDK reaches the server over localhost, but +// depending on resolver order and host routing the source the server observes +// can be IPv4 loopback, IPv6 loopback, or one of the host's RFC1918 addresses +// (CI runners advertise the S3 endpoint on a 10.x interface). An allow policy +// meant to match "this local client" must cover all of them or the +// positive-condition assertion flakes. +var localSourceAllowCIDRs = []string{ + "127.0.0.0/8", + "::1/128", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", +} + +// sourceIpAllowPolicy builds an Allow policy for s3:* on bucketName gated by an +// aws:SourceIp IpAddress condition over the given CIDRs. +func sourceIpAllowPolicy(bucketName string, cidrs ...string) string { + quoted := make([]string, len(cidrs)) + for i, c := range cidrs { + quoted[i] = `"` + c + `"` + } + return `{ + "Version":"2012-10-17", + "Statement":[{ + "Effect":"Allow", + "Action":"s3:*", + "Resource":["arn:aws:s3:::` + bucketName + `","arn:aws:s3:::` + bucketName + `/*"], + "Condition":{"IpAddress":{"aws:SourceIp":[` + strings.Join(quoted, ",") + `]}} + }] + }` +} + // TestIAMUserInlinePolicySourceIpCondition verifies that an aws:SourceIp condition -// on a user inline policy is honored. Tests run from localhost (127.0.0.1), so a -// policy that only allows access from a non-loopback CIDR must deny the request, -// and a policy that allows access from 127.0.0.0/8 must allow it. +// on a user inline policy is honored. Tests run against a local server, so a +// policy that only allows a non-local CIDR must deny the request, and a policy +// that allows the local client's address (see localSourceAllowCIDRs) must allow it. func TestIAMUserInlinePolicySourceIpCondition(t *testing.T) { framework := NewS3IAMTestFramework(t) defer framework.Cleanup() @@ -87,30 +120,13 @@ func TestIAMUserInlinePolicySourceIpCondition(t *testing.T) { } }) - policyDoc := func(cidrs ...string) string { - quoted := make([]string, len(cidrs)) - for i, c := range cidrs { - quoted[i] = `"` + c + `"` - } - return `{ - "Version":"2012-10-17", - "Statement":[{ - "Effect":"Allow", - "Action":"s3:*", - "Resource":["arn:aws:s3:::` + bucketName + `","arn:aws:s3:::` + bucketName + `/*"], - "Condition":{"IpAddress":{"aws:SourceIp":[` + strings.Join(quoted, ",") + `]}} - }] - }` - } - t.Run("denies_when_source_ip_does_not_match", func(t *testing.T) { // SourceIp 198.51.100.0/24 is RFC5737 TEST-NET-2; the test client is on - // loopback (127.0.0.1 or ::1 depending on resolver), so the condition - // must fail and the action must be denied. + // the local host, so the condition must fail and the action be denied. _, err = iamClient.PutUserPolicy(&iam.PutUserPolicyInput{ UserName: aws.String(userName), PolicyName: aws.String(policyName), - PolicyDocument: aws.String(policyDoc("198.51.100.0/24")), + PolicyDocument: aws.String(sourceIpAllowPolicy(bucketName, "198.51.100.0/24")), }) require.NoError(t, err) @@ -127,25 +143,25 @@ func TestIAMUserInlinePolicySourceIpCondition(t *testing.T) { }) t.Run("allows_when_source_ip_matches", func(t *testing.T) { - // Cover both IPv4 and IPv6 loopback: on CI runners `localhost` may - // resolve to ::1 first, in which case a 127.0.0.0/8-only allow would - // silently never match and the test would hang. _, err = iamClient.PutUserPolicy(&iam.PutUserPolicyInput{ UserName: aws.String(userName), PolicyName: aws.String(policyName), - PolicyDocument: aws.String(policyDoc("127.0.0.0/8", "::1/128")), + PolicyDocument: aws.String(sourceIpAllowPolicy(bucketName, localSourceAllowCIDRs...)), }) require.NoError(t, err) require.Eventually(t, func() bool { - _, err := userS3.PutObject(&s3.PutObjectInput{ + _, putErr := userS3.PutObject(&s3.PutObjectInput{ Bucket: aws.String(bucketName), Key: aws.String("allowed.txt"), Body: aws.ReadSeekCloser(strings.NewReader("ok")), }) - return err == nil + if putErr != nil { + t.Logf("allow attempt denied (source IP not in %v?): %v", localSourceAllowCIDRs, putErr) + } + return putErr == nil }, 10*time.Second, 500*time.Millisecond, - "PutObject must succeed when aws:SourceIp condition matches the loopback range") + "PutObject must succeed when aws:SourceIp condition matches the local client address") }) } @@ -215,27 +231,8 @@ func TestIAMGroupInlinePolicyEnforcement(t *testing.T) { } }) - // Cover both IPv4 and IPv6 loopback in the allow CIDR list: on CI runners - // `localhost` may resolve to ::1 first, in which case a 127.0.0.0/8-only - // allow would silently never match and the test would hang. - allowDoc := `{ - "Version":"2012-10-17", - "Statement":[{ - "Effect":"Allow", - "Action":"s3:*", - "Resource":["arn:aws:s3:::` + bucketName + `","arn:aws:s3:::` + bucketName + `/*"], - "Condition":{"IpAddress":{"aws:SourceIp":["127.0.0.0/8","::1/128"]}} - }] - }` - denyDoc := `{ - "Version":"2012-10-17", - "Statement":[{ - "Effect":"Allow", - "Action":"s3:*", - "Resource":["arn:aws:s3:::` + bucketName + `","arn:aws:s3:::` + bucketName + `/*"], - "Condition":{"IpAddress":{"aws:SourceIp":"198.51.100.0/24"}} - }] - }` + allowDoc := sourceIpAllowPolicy(bucketName, localSourceAllowCIDRs...) + denyDoc := sourceIpAllowPolicy(bucketName, "198.51.100.0/24") t.Run("crud_round_trip", func(t *testing.T) { _, err := iamClient.PutGroupPolicy(&iam.PutGroupPolicyInput{ @@ -277,12 +274,15 @@ func TestIAMGroupInlinePolicyEnforcement(t *testing.T) { require.NoError(t, err) require.Eventually(t, func() bool { - _, err := userS3.PutObject(&s3.PutObjectInput{ + _, putErr := userS3.PutObject(&s3.PutObjectInput{ Bucket: aws.String(bucketName), Key: aws.String("group-allowed.txt"), Body: aws.ReadSeekCloser(strings.NewReader("ok")), }) - return err == nil + if putErr != nil { + t.Logf("allow attempt denied (source IP not in %v?): %v", localSourceAllowCIDRs, putErr) + } + return putErr == nil }, 10*time.Second, 500*time.Millisecond, "group member must be allowed when the group policy condition matches") }) diff --git a/weed/s3api/s3api_copy_validation.go b/weed/s3api/s3api_copy_validation.go index ca4413b42..9298787e0 100644 --- a/weed/s3api/s3api_copy_validation.go +++ b/weed/s3api/s3api_copy_validation.go @@ -115,9 +115,19 @@ func validateSSEKMSCopyRequirements(srcMetadata map[string][]byte, headers http. // validateEncryptionCompatibility validates that encryption methods are not conflicting func validateEncryptionCompatibility(headers http.Header) error { + sseAlgorithm := headers.Get(s3_constants.AmzServerSideEncryption) hasSSEC := hasSSECHeaders(headers) - hasSSEKMS := headers.Get(s3_constants.AmzServerSideEncryption) == "aws:kms" - hasSSES3 := headers.Get(s3_constants.AmzServerSideEncryption) == "AES256" + hasSSEKMS := sseAlgorithm == s3_constants.SSEAlgorithmKMS + hasSSES3 := sseAlgorithm == s3_constants.SSEAlgorithmAES256 + + // Reject unsupported algorithms so they are never persisted as a bogus + // destination header advertising encryption that was never applied. + if sseAlgorithm != "" && !hasSSEKMS && !hasSSES3 { + return &CopyValidationError{ + Code: s3err.ErrInvalidEncryptionAlgorithm, + Message: fmt.Sprintf("Unsupported server-side encryption algorithm: %s", sseAlgorithm), + } + } // Count how many encryption methods are specified encryptionCount := 0 diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 77651e7f7..d76f0a95c 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -350,8 +350,10 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request if entry.Attributes.FileSize == 0 || len(entry.GetChunks()) == 0 { dstEntry.Chunks = nil - // Handle inline encrypted content - fixes GitHub #7562 - if len(entry.Content) > 0 { + // Handle inline encrypted content - fixes GitHub #7562. + // Also run when the destination requests encryption with no content so + // empty objects get real key metadata, not just a bare algorithm header. + if len(entry.Content) > 0 || dstWantsSSEC || dstWantsSSEKMS || dstWantsSSES3 { inlineContent, inlineMetadata, inlineErr := s3a.processInlineContentForCopy( entry, r, dstBucket, dstObject, srcHasSSEC, srcHasSSEKMS, srcHasSSES3, @@ -1029,26 +1031,28 @@ func processMetadataBytes(reqHeader http.Header, existing map[string][]byte, rep metadata[s3_constants.AmzStorageClass] = []byte(sc) } - // Handle SSE-KMS headers - these are always processed from request headers if present - if sseAlgorithm := reqHeader.Get(s3_constants.AmzServerSideEncryption); sseAlgorithm == "aws:kms" { + // Handle destination SSE headers from the request when present. + if sseAlgorithm := reqHeader.Get(s3_constants.AmzServerSideEncryption); sseAlgorithm != "" { metadata[s3_constants.AmzServerSideEncryption] = []byte(sseAlgorithm) - // KMS Key ID (optional - can use default key) - if kmsKeyID := reqHeader.Get(s3_constants.AmzServerSideEncryptionAwsKmsKeyId); kmsKeyID != "" { - metadata[s3_constants.AmzServerSideEncryptionAwsKmsKeyId] = []byte(kmsKeyID) - } + if sseAlgorithm == s3_constants.SSEAlgorithmKMS { + // KMS Key ID (optional - can use default key) + if kmsKeyID := reqHeader.Get(s3_constants.AmzServerSideEncryptionAwsKmsKeyId); kmsKeyID != "" { + metadata[s3_constants.AmzServerSideEncryptionAwsKmsKeyId] = []byte(kmsKeyID) + } - // Encryption Context (optional) - if encryptionContext := reqHeader.Get(s3_constants.AmzServerSideEncryptionContext); encryptionContext != "" { - metadata[s3_constants.AmzServerSideEncryptionContext] = []byte(encryptionContext) - } + // Encryption Context (optional) + if encryptionContext := reqHeader.Get(s3_constants.AmzServerSideEncryptionContext); encryptionContext != "" { + metadata[s3_constants.AmzServerSideEncryptionContext] = []byte(encryptionContext) + } - // Bucket Key Enabled (optional) - if bucketKeyEnabled := reqHeader.Get(s3_constants.AmzServerSideEncryptionBucketKeyEnabled); bucketKeyEnabled != "" { - metadata[s3_constants.AmzServerSideEncryptionBucketKeyEnabled] = []byte(bucketKeyEnabled) + // Bucket Key Enabled (optional) + if bucketKeyEnabled := reqHeader.Get(s3_constants.AmzServerSideEncryptionBucketKeyEnabled); bucketKeyEnabled != "" { + metadata[s3_constants.AmzServerSideEncryptionBucketKeyEnabled] = []byte(bucketKeyEnabled) + } } } else { - // If not explicitly setting SSE-KMS, preserve existing SSE headers from source + // If not explicitly setting SSE, preserve existing SSE headers from source for _, sseHeader := range []string{ s3_constants.AmzServerSideEncryption, s3_constants.AmzServerSideEncryptionAwsKmsKeyId, diff --git a/weed/s3api/s3api_object_handlers_copy_mime_test.go b/weed/s3api/s3api_object_handlers_copy_mime_test.go index 38ffc2643..23e1915f5 100644 --- a/weed/s3api/s3api_object_handlers_copy_mime_test.go +++ b/weed/s3api/s3api_object_handlers_copy_mime_test.go @@ -3,6 +3,8 @@ package s3api import ( "net/http" "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) func TestResolveDestinationMime(t *testing.T) { @@ -279,3 +281,30 @@ func TestProcessMetadataBytes_CopyInheritsSystemHeaders(t *testing.T) { t.Errorf("Content-Encoding = %q, want %q", got, "gzip") } } + +func TestProcessMetadataBytes_CopyAppliesRequestedSSES3Header(t *testing.T) { + req := http.Header{} + req.Set(s3_constants.AmzServerSideEncryption, s3_constants.SSEAlgorithmAES256) + + out, err := processMetadataBytes(req, nil, false, false) + if err != nil { + t.Fatalf("processMetadataBytes returned error: %v", err) + } + if got := string(out[s3_constants.AmzServerSideEncryption]); got != s3_constants.SSEAlgorithmAES256 { + t.Fatalf("%s = %q, want %q", s3_constants.AmzServerSideEncryption, got, s3_constants.SSEAlgorithmAES256) + } +} + +func TestProcessMetadataBytes_CopyPreservesSourceSSEWhenRequestOmitsHeader(t *testing.T) { + existing := map[string][]byte{ + s3_constants.AmzServerSideEncryption: []byte(s3_constants.SSEAlgorithmKMS), + } + + out, err := processMetadataBytes(http.Header{}, existing, false, false) + if err != nil { + t.Fatalf("processMetadataBytes returned error: %v", err) + } + if got := string(out[s3_constants.AmzServerSideEncryption]); got != s3_constants.SSEAlgorithmKMS { + t.Fatalf("%s = %q, want %q", s3_constants.AmzServerSideEncryption, got, s3_constants.SSEAlgorithmKMS) + } +}