From 7d5cbfd54707629499724864f51590e5c974b0d7 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 27 Mar 2026 23:15:01 -0700 Subject: [PATCH] s3: support s3:x-amz-server-side-encryption policy condition (#8806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * s3: support s3:x-amz-server-side-encryption policy condition (#7680) - Normalize x-amz-server-side-encryption header values to canonical form (aes256 → AES256, aws:kms mixed-case → aws:kms) so StringEquals conditions work regardless of client capitalisation - Exempt UploadPart and UploadPartCopy from SSE Null conditions: these actions inherit SSE from the initial CreateMultipartUpload request and do not re-send the header, so Deny/Null("true") should not block them - Add sse_condition_test.go covering StringEquals, Null, case-insensitive normalisation, and multipart continuation action exemption * s3: address review comments on SSE condition support - Replace "inherited" sentinel in injectSSEForMultipart with "AES256" so that StringEquals/Null conditions evaluate against a meaningful value; add TODO noting that KMS multipart uploads need the actual algorithm looked up from the upload state - Rewrite TestSSECaseInsensitiveNormalization to drive normalisation through EvaluatePolicyForRequest with a real *http.Request so regressions in the production code path are caught; split into AES256 and aws:kms variants to cover both normalisation branches * s3: plumb real inherited SSE from multipart upload state into policy eval Instead of injecting a static "AES256" sentinel for UploadPart/UploadPartCopy, look up the actual SSE algorithm from the stored CreateMultipartUpload entry and pass it through the evaluation chain. Changes: - PolicyEvaluationArgs gains InheritedSSEAlgorithm string; set by the BucketPolicyEngine wrapper for multipart continuation actions - injectSSEForMultipart(conditions, inheritedSSE) now accepts the real algorithm; empty string means no SSE → Null("true") fires correctly - IsMultipartContinuationAction exported so the s3api wrapper can use it - BucketPolicyEngine gets a MultipartSSELookup callback (set by S3ApiServer) that fetches the upload entry and reads SeaweedFSSSEKMSKeyID / SeaweedFSSSES3Encryption to determine the algorithm - S3ApiServer.getMultipartSSEAlgorithm implements the lookup via getEntry - Tests updated: three multipart cases (AES256, aws:kms, no-SSE-must-deny) plus UploadPartCopy coverage --- weed/s3api/policy_engine/engine.go | 63 ++++- .../s3api/policy_engine/sse_condition_test.go | 249 ++++++++++++++++++ weed/s3api/policy_engine/types.go | 4 + weed/s3api/s3api_bucket_policy_engine.go | 15 ++ weed/s3api/s3api_object_handlers_multipart.go | 19 ++ weed/s3api/s3api_server.go | 5 + 6 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 weed/s3api/policy_engine/sse_condition_test.go diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index 2432b695c..bf66ebfd2 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -210,7 +210,15 @@ func (engine *PolicyEngine) evaluateStatement(stmt *CompiledStatement, args *Pol // Check conditions if len(stmt.Statement.Condition) > 0 { - match := EvaluateConditions(stmt.Statement.Condition, args.Conditions, args.ObjectEntry, args.Claims) + condCtx := args.Conditions + // Multipart continuation actions (UploadPart, UploadPartCopy) inherit SSE + // from CreateMultipartUpload and do not carry their own SSE header. + // Inject the real inherited algorithm so Null/StringEquals conditions + // evaluate against the value that was set at upload initiation. + if IsMultipartContinuationAction(args.Action) { + condCtx = injectSSEForMultipart(args.Conditions, args.InheritedSSEAlgorithm) + } + match := EvaluateConditions(stmt.Statement.Condition, condCtx, args.ObjectEntry, args.Claims) if !match { return false } @@ -435,9 +443,62 @@ func ExtractConditionValuesFromRequest(r *http.Request) map[string][]string { } } + // Normalize s3:x-amz-server-side-encryption value to canonical form. + // AWS accepts "AES256" case-insensitively; normalise so that policy + // StringEquals conditions work regardless of client capitalisation. + const sseKey = "s3:x-amz-server-side-encryption" + if sseVals, ok := values[sseKey]; ok { + normalized := make([]string, len(sseVals)) + for i, v := range sseVals { + switch strings.ToUpper(v) { + case "AES256": + normalized[i] = "AES256" + case "AWS:KMS": + normalized[i] = "aws:kms" + default: + normalized[i] = v + } + } + values[sseKey] = normalized + } + return values } +// IsMultipartContinuationAction returns true for actions that do not carry +// their own SSE header because SSE is inherited from CreateMultipartUpload. +func IsMultipartContinuationAction(action string) bool { + return action == "s3:UploadPart" || action == "s3:UploadPartCopy" +} + +// injectSSEForMultipart returns a condition context augmented with the +// inherited SSE algorithm for multipart continuation actions. +// +// UploadPart and UploadPartCopy do not re-send the SSE header because +// encryption is set once at CreateMultipartUpload. The caller supplies +// inheritedSSE (the canonical algorithm, e.g. "AES256" or "aws:kms") so +// that Null/StringEquals conditions on s3:x-amz-server-side-encryption +// evaluate against the real value. +// +// If inheritedSSE is empty (no SSE was requested at initiation), the +// conditions map is returned unchanged so Null("true") will correctly +// match and deny the request. +func injectSSEForMultipart(conditions map[string][]string, inheritedSSE string) map[string][]string { + const sseKey = "s3:x-amz-server-side-encryption" + if inheritedSSE == "" { + return conditions // no SSE at upload initiation; let Null("true") fire + } + if _, exists := conditions[sseKey]; exists { + return conditions // SSE header was actually sent on this request + } + modified := make(map[string][]string, len(conditions)+1) + for k, v := range conditions { + modified[k] = v + } + modified[sseKey] = []string{inheritedSSE} + return modified +} + // extractSourceIP returns the best-effort client IP address for condition evaluation. // Preference order: X-Forwarded-For (first valid IP), X-Real-Ip, then RemoteAddr. // IMPORTANT: X-Forwarded-For and X-Real-Ip are trusted without validation. diff --git a/weed/s3api/policy_engine/sse_condition_test.go b/weed/s3api/policy_engine/sse_condition_test.go new file mode 100644 index 000000000..a03447046 --- /dev/null +++ b/weed/s3api/policy_engine/sse_condition_test.go @@ -0,0 +1,249 @@ +package policy_engine + +import ( + "net/http" + "testing" +) + +// requiresSSEPolicy is a bucket policy that denies PutObject when the +// x-amz-server-side-encryption header is absent (Null == true). +const requiresSSEPolicy = `{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "DenyUnencryptedUploads", + "Effect": "Deny", + "Principal": "*", + "Action": "s3:PutObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "Null": { + "s3:x-amz-server-side-encryption": "true" + } + } + } + ] +}` + +// requiresAES256Policy denies PutObject unless AES256 is explicitly requested. +const requiresAES256Policy = `{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "AllowAES256Only", + "Effect": "Allow", + "Principal": "*", + "Action": "s3:PutObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:x-amz-server-side-encryption": "AES256" + } + } + } + ] +}` + +// requiresKMSPolicy allows PutObject only when aws:kms encryption is requested. +const requiresKMSPolicy = `{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "AllowKMSOnly", + "Effect": "Allow", + "Principal": "*", + "Action": "s3:PutObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:x-amz-server-side-encryption": "aws:kms" + } + } + } + ] +}` + +// multipartPolicy denies PutObject when SSE is absent but should NOT block UploadPart. +const multipartPolicy = `{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "DenyUnencryptedUploads", + "Effect": "Deny", + "Principal": "*", + "Action": ["s3:PutObject", "s3:UploadPart", "s3:UploadPartCopy"], + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "Null": { + "s3:x-amz-server-side-encryption": "true" + } + } + } + ] +}` + +func newEngineWithPolicy(t *testing.T, policy string) *PolicyEngine { + t.Helper() + engine := NewPolicyEngine() + if err := engine.SetBucketPolicy("test-bucket", policy); err != nil { + t.Fatalf("SetBucketPolicy: %v", err) + } + return engine +} + +func evalArgs(action string, conditions map[string][]string) *PolicyEvaluationArgs { + return &PolicyEvaluationArgs{ + Action: action, + Resource: "arn:aws:s3:::test-bucket/object.txt", + Principal: "*", + Conditions: conditions, + } +} + +func evalArgsWithSSE(action, inheritedSSE string) *PolicyEvaluationArgs { + return &PolicyEvaluationArgs{ + Action: action, + Resource: "arn:aws:s3:::test-bucket/object.txt", + Principal: "*", + Conditions: map[string][]string{}, + InheritedSSEAlgorithm: inheritedSSE, + } +} + +// TestSSEStringEqualsPresent – StringEquals with AES256 header present should Allow. +func TestSSEStringEqualsPresent(t *testing.T) { + engine := newEngineWithPolicy(t, requiresAES256Policy) + + conditions := map[string][]string{ + "s3:x-amz-server-side-encryption": {"AES256"}, + } + result := engine.EvaluatePolicy("test-bucket", evalArgs("s3:PutObject", conditions)) + if result != PolicyResultAllow { + t.Errorf("expected Allow, got %v", result) + } +} + +// TestSSEStringEqualsWrongValue – StringEquals with wrong SSE value should not Allow. +func TestSSEStringEqualsWrongValue(t *testing.T) { + engine := newEngineWithPolicy(t, requiresAES256Policy) + + conditions := map[string][]string{ + "s3:x-amz-server-side-encryption": {"aws:kms"}, + } + result := engine.EvaluatePolicy("test-bucket", evalArgs("s3:PutObject", conditions)) + if result == PolicyResultAllow { + t.Errorf("expected non-Allow, got %v", result) + } +} + +// TestSSENullConditionAbsent – Null("true") matches when header is absent → Deny. +func TestSSENullConditionAbsent(t *testing.T) { + engine := newEngineWithPolicy(t, requiresSSEPolicy) + + // No SSE header → condition "Null == true" matches → Deny statement fires + result := engine.EvaluatePolicy("test-bucket", evalArgs("s3:PutObject", map[string][]string{})) + if result != PolicyResultDeny { + t.Errorf("expected Deny (no SSE header), got %v", result) + } +} + +// TestSSENullConditionPresent – Null("true") does NOT match when header is present → not Deny. +func TestSSENullConditionPresent(t *testing.T) { + engine := newEngineWithPolicy(t, requiresSSEPolicy) + + conditions := map[string][]string{ + "s3:x-amz-server-side-encryption": {"AES256"}, + } + result := engine.EvaluatePolicy("test-bucket", evalArgs("s3:PutObject", conditions)) + // Deny condition not matched; no explicit Allow → Indeterminate + if result == PolicyResultDeny { + t.Errorf("expected non-Deny when SSE header present, got Deny") + } +} + +// TestSSECaseInsensitiveNormalizationAES256 drives the AES256 normalisation +// through EvaluatePolicyForRequest so that a regression in the production +// code path would be caught. The request carries the header in lowercase +// ("aes256"); after normalisation it must match the policy's "AES256" value. +func TestSSECaseInsensitiveNormalizationAES256(t *testing.T) { + engine := newEngineWithPolicy(t, requiresAES256Policy) + + req, _ := http.NewRequest(http.MethodPut, "/", nil) + req.RemoteAddr = "1.2.3.4:1234" + req.Header.Set("X-Amz-Server-Side-Encryption", "aes256") // lowercase + + result := engine.EvaluatePolicyForRequest("test-bucket", "object.txt", "PutObject", "*", req) + if result != PolicyResultAllow { + t.Errorf("expected Allow after AES256 case normalisation, got %v", result) + } +} + +// TestSSECaseInsensitiveNormalizationKMS drives the aws:kms branch of the +// normalisation through the production code path. The request carries +// "AWS:KMS" (mixed case); after normalisation it must match "aws:kms". +func TestSSECaseInsensitiveNormalizationKMS(t *testing.T) { + engine := newEngineWithPolicy(t, requiresKMSPolicy) + + req, _ := http.NewRequest(http.MethodPut, "/", nil) + req.RemoteAddr = "1.2.3.4:1234" + req.Header.Set("X-Amz-Server-Side-Encryption", "AWS:KMS") // mixed case + + result := engine.EvaluatePolicyForRequest("test-bucket", "object.txt", "PutObject", "*", req) + if result != PolicyResultAllow { + t.Errorf("expected Allow after aws:kms case normalisation, got %v", result) + } +} + +// TestSSEMultipartAES256Exempt – UploadPart with AES256 inherited from +// CreateMultipartUpload is not blocked by the Null("true") deny condition. +func TestSSEMultipartAES256Exempt(t *testing.T) { + engine := newEngineWithPolicy(t, multipartPolicy) + + result := engine.EvaluatePolicy("test-bucket", evalArgsWithSSE("s3:UploadPart", "AES256")) + if result == PolicyResultDeny { + t.Errorf("UploadPart with inherited AES256 should not be Deny, got Deny") + } +} + +// TestSSEMultipartKMSExempt – UploadPart with aws:kms inherited from +// CreateMultipartUpload is not blocked by the Null("true") deny condition. +func TestSSEMultipartKMSExempt(t *testing.T) { + engine := newEngineWithPolicy(t, multipartPolicy) + + result := engine.EvaluatePolicy("test-bucket", evalArgsWithSSE("s3:UploadPart", "aws:kms")) + if result == PolicyResultDeny { + t.Errorf("UploadPart with inherited aws:kms should not be Deny, got Deny") + } +} + +// TestSSEMultipartNoSSEDenied – UploadPart for an upload that had no SSE +// must still be denied by the Null("true") deny condition. +func TestSSEMultipartNoSSEDenied(t *testing.T) { + engine := newEngineWithPolicy(t, multipartPolicy) + + // inheritedSSE="" means CreateMultipartUpload was sent without SSE + result := engine.EvaluatePolicy("test-bucket", evalArgsWithSSE("s3:UploadPart", "")) + if result != PolicyResultDeny { + t.Errorf("UploadPart with no inherited SSE should be Deny, got %v", result) + } +} + +// TestSSEUploadPartCopyKMSExempt – UploadPartCopy with aws:kms is also exempt. +func TestSSEUploadPartCopyKMSExempt(t *testing.T) { + engine := newEngineWithPolicy(t, multipartPolicy) + + result := engine.EvaluatePolicy("test-bucket", evalArgsWithSSE("s3:UploadPartCopy", "aws:kms")) + if result == PolicyResultDeny { + t.Errorf("UploadPartCopy with inherited aws:kms should not be Deny, got Deny") + } +} + +// TestSSEPutObjectStillBlockedWithoutHeader – regular PutObject still denied without SSE. +func TestSSEPutObjectStillBlockedWithoutHeader(t *testing.T) { + engine := newEngineWithPolicy(t, multipartPolicy) + + result := engine.EvaluatePolicy("test-bucket", evalArgs("s3:PutObject", map[string][]string{})) + if result != PolicyResultDeny { + t.Errorf("PutObject without SSE should be Deny, got %v", result) + } +} diff --git a/weed/s3api/policy_engine/types.go b/weed/s3api/policy_engine/types.go index f6d35e6c6..862023b34 100644 --- a/weed/s3api/policy_engine/types.go +++ b/weed/s3api/policy_engine/types.go @@ -177,6 +177,10 @@ type PolicyEvaluationArgs struct { ObjectEntry map[string][]byte // Claims are JWT claims for jwt:* policy variables (can be nil) Claims map[string]interface{} + // InheritedSSEAlgorithm is the canonical SSE algorithm ("AES256" or "aws:kms") + // inherited from the CreateMultipartUpload request for UploadPart and + // UploadPartCopy actions. The empty string means no SSE was used. + InheritedSSEAlgorithm string } // PolicyCache for caching compiled policies diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index 1a014a919..28e4b90e1 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -13,6 +13,11 @@ import ( // BucketPolicyEngine wraps the policy_engine to provide bucket policy evaluation type BucketPolicyEngine struct { engine *policy_engine.PolicyEngine + // MultipartSSELookup retrieves the canonical SSE algorithm ("AES256" or + // "aws:kms") that was stored when a multipart upload was initiated. + // Returns "" when the upload had no SSE or when the entry cannot be found. + // Set by S3ApiServer after construction to give the engine filer access. + MultipartSSELookup func(bucket, uploadID string) string } // NewBucketPolicyEngine creates a new bucket policy engine @@ -156,6 +161,16 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal // But the caller is responsible for passing them. // Falling back to empty claims if not provided. } + + // For multipart continuation actions look up the SSE algorithm that was + // set at CreateMultipartUpload time. UploadPart/UploadPartCopy do not + // re-send the SSE header, so we inject the stored value so that bucket + // policy conditions on s3:x-amz-server-side-encryption evaluate correctly. + if policy_engine.IsMultipartContinuationAction(s3Action) && bpe.MultipartSSELookup != nil { + if uploadID := r.URL.Query().Get("uploadId"); uploadID != "" { + args.InheritedSSEAlgorithm = bpe.MultipartSSELookup(bucket, uploadID) + } + } } result := bpe.engine.EvaluatePolicy(bucket, args) diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index 92745cdcc..c84a009d5 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -471,6 +471,25 @@ func (s3a *S3ApiServer) genUploadsFolder(bucket string) string { return fmt.Sprintf("%s/%s", s3a.bucketDir(bucket), s3_constants.MultipartUploadsFolder) } +// getMultipartSSEAlgorithm returns the canonical SSE algorithm ("AES256" or +// "aws:kms") that was stored when the multipart upload was initiated, or "" +// if the upload entry is not found or had no SSE. It is used by the bucket +// policy engine to evaluate s3:x-amz-server-side-encryption conditions for +// UploadPart and UploadPartCopy, which do not re-send the SSE header. +func (s3a *S3ApiServer) getMultipartSSEAlgorithm(bucket, uploadID string) string { + entry, err := s3a.getEntry(s3a.genUploadsFolder(bucket), uploadID) + if err != nil || entry == nil || entry.Extended == nil { + return "" + } + if _, ok := entry.Extended[s3_constants.SeaweedFSSSEKMSKeyID]; ok { + return "aws:kms" + } + if _, ok := entry.Extended[s3_constants.SeaweedFSSSES3Encryption]; ok { + return "AES256" + } + return "" +} + func (s3a *S3ApiServer) genPartUploadPath(bucket, uploadID string, partID int) string { // Returns just the file path - no filer address needed // Upload traffic goes directly to volume servers, not through filer diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 42d9e77c4..08e376aec 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -213,6 +213,11 @@ func NewS3ApiServerWithStore(router *mux.Router, option *S3ApiServerOption, expl // This avoids circular dependency by not passing the entire S3ApiServer iam.policyEngine = policyEngine + // Give the policy engine a way to look up the SSE algorithm that was + // stored at CreateMultipartUpload time, so that UploadPart/UploadPartCopy + // policy conditions on s3:x-amz-server-side-encryption evaluate correctly. + policyEngine.MultipartSSELookup = s3ApiServer.getMultipartSSEAlgorithm + // Initialize advanced IAM system if config is provided or explicitly enabled if option.IamConfig != "" || option.EnableIam { configSource := "defaults"