s3api: support s3:RequestObjectTag/<key> in bucket policies (#11910)

* s3api: support s3:RequestObjectTag/<key> in bucket policies

The tags come from X-Amz-Tagging only where the handler stores them from
there: not on PutObjectTagging, which reads the body, and not on a copy
that keeps the source's tags. For a repeated key the last value counts,
since that is what CopyObject stores.

* s3api: extract RequestObjectTag only where the handler stores it

The header was honored on any request carrying it, so a tag attached to
a GET, DELETE, part upload, multipart completion, directory marker or a
PUT subresource satisfied a tag-conditioned policy the object never
receives. Extraction now follows the handlers: PutObject, a copy under
X-Amz-Tagging-Directive: REPLACE, and CreateMultipartUpload store the
header; a plain object PUT is identified by carrying only X-Amz-*
signing parameters.

* s3api: exclude multipart part PUTs before the copy-source check

A PUT carrying X-Amz-Copy-Source plus a numeric partNumber and uploadId
routes to CopyObjectPartHandler, which stores a part and never reads
X-Amz-Tagging. Checking the copy source first let such a request satisfy
an s3:RequestObjectTag condition under REPLACE with tags that are never
stored. Mirror the router: the part handlers match only when both
partNumber (digits only) and uploadId are present, so a lone or
non-numeric partNumber still falls through to the storing handlers.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* s3api: gate tag extraction on the routed subresource, not query key shape

Rejecting every PUT whose query held a key outside X-Amz- dropped
X-Amz-Tagging from ordinary object PUTs that carry arbitrary parameters —
Signature V2 presigns (AWSAccessKeyId/Signature/Expires) among them — so a
tag-conditioned Deny could be bypassed by appending an unrelated query
parameter while putToFiler still stored the tags. Exclude the object PUT
subresources the router actually dispatches (tagging, renameObject, acl,
retention, legal-hold) ahead of the copy-source check instead.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

* s3api: cover the routed-subresource and part-copy tag extraction cases

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

---------

Co-authored-by: Chris Lu <chris.lu@gmail.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

weed/s3api/policy_engine/README_POLICY_ENGINE.md

@@ -356,6 +356,7 @@ The policy engine provides a seamless upgrade path from SeaweedFS's existing sim
 | `aws:SecureTransport` | Require HTTPS |
 | `aws:CurrentTime` | Time-based access control |
 | `s3:ExistingObjectTag/<key>` | Tag-based access control for existing objects |
+| `s3:RequestObjectTag/<key>` | Tag conditions on the tags a PUT request sets |
 | Wildcard Patterns | Support for `*` and `?` in actions and resources |
 | Principal Matching | `*`, account IDs, and user ARNs |
 
@@ -363,7 +364,6 @@ The policy engine provides a seamless upgrade path from SeaweedFS's existing sim
 
 | Feature | GitHub Issue |
 |---------|--------------|
-| `s3:RequestObjectTag/<key>` | For tag conditions on PUT requests |
 | `s3:RequestObjectTagKeys` | Check which tag keys are in request |
 | `s3:x-amz-content-sha256` | Content hash condition |
 | `s3:x-amz-server-side-encryption` | SSE condition |

weed/s3api/policy_engine/conditions.go

@@ -728,6 +728,10 @@ func GetConditionEvaluator(operator string) (ConditionEvaluator, error) {
 // ExistingObjectTagPrefix is the prefix for S3 policy condition keys
 const ExistingObjectTagPrefix = "s3:ExistingObjectTag/"
 
+// RequestObjectTagPrefix is the prefix for condition keys on the tags a
+// request sets through the X-Amz-Tagging header.
+const RequestObjectTagPrefix = "s3:RequestObjectTag/"
+
 // getConditionContextValue resolves the value(s) for a condition key.
 // For s3:ExistingObjectTag/<key> conditions, it looks up the tag in objectEntry.
 // For other condition keys, it looks up the value in contextValues.

weed/s3api/policy_engine/engine.go

@@ -3,6 +3,7 @@ package policy_engine
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"regexp"
 	"strings"
 	"sync"
@@ -10,6 +11,7 @@ import (
 	"time"
 
 	"github.com/seaweedfs/seaweedfs/weed/glog"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
 )
 
 // PolicyEvaluationResult represents the result of policy evaluation
@@ -439,6 +441,10 @@ func (engine *PolicyEngine) ExtractConditionValuesFromRequest(r *http.Request) m
 	// Note: s3:ExistingObjectTag/<key> conditions are evaluated using objectEntry
 	// passed to EvaluatePolicy, not extracted from the request.
 
+	for key, value := range requestObjectTags(r) {
+		values[RequestObjectTagPrefix+key] = []string{value}
+	}
+
 	// S3 bucket-level conditions
 	if delimiter := r.URL.Query().Get("delimiter"); delimiter != "" {
 		values["s3:delimiter"] = []string{delimiter}
@@ -496,6 +502,86 @@ func (engine *PolicyEngine) ExtractConditionValuesFromRequest(r *http.Request) m
 	return values
 }
 
+// requestObjectTags returns the tags that X-Amz-Tagging will store on the
+// object, or nil when the handler ignores the header — otherwise a tag
+// attached to a read or to a write that never stores it would satisfy a
+// tag-conditioned policy the object cannot meet.
+func requestObjectTags(r *http.Request) map[string]string {
+	tagging := r.Header.Get(s3_constants.AmzObjectTagging)
+	if tagging == "" {
+		return nil
+	}
+	if !storesRequestObjectTags(r) {
+		return nil
+	}
+	// A malformed header is rejected by the handler with InvalidTag.
+	parsed, err := url.ParseQuery(tagging)
+	if err != nil {
+		return nil
+	}
+	// putToFiler rejects a repeated key, but CopyObject stores its last
+	// value, so that is the one the policy has to see.
+	tags := make(map[string]string, len(parsed))
+	for key, vals := range parsed {
+		tags[key] = vals[len(vals)-1]
+	}
+	return tags
+}
+
+// storesRequestObjectTags reports whether the request's handler saves the
+// X-Amz-Tagging header on the object: PutObject, a copy under
+// X-Amz-Tagging-Directive: REPLACE, and CreateMultipartUpload do; every
+// other operation — reads, deletes, part uploads, completion, tagging
+// subresources — does not.
+func storesRequestObjectTags(r *http.Request) bool {
+	query := r.URL.Query()
+	if r.Method == http.MethodPost {
+		return query.Has("uploads") && !query.Has("uploadId")
+	}
+	if r.Method != http.MethodPut {
+		return false
+	}
+	// A numeric partNumber together with uploadId routes to the part
+	// handlers — with or without a copy source — which never read
+	// X-Amz-Tagging.
+	if query.Has("partNumber") && query.Has("uploadId") && isDigits(query.Get("partNumber")) {
+		return false
+	}
+	// The PUT subresources route before CopyObject and never read
+	// X-Amz-Tagging; any other query key — presign parameters included —
+	// still reaches the object PUT handlers.
+	for _, subresource := range []string{"tagging", "renameObject", "acl", "retention", "legal-hold"} {
+		if query.Has(subresource) {
+			return false
+		}
+	}
+	if r.Header.Get("X-Amz-Copy-Source") != "" {
+		return r.Header.Get(s3_constants.AmzObjectTaggingDirective) == "REPLACE"
+	}
+	if query.Has("partNumber") || query.Has("uploadId") {
+		return false
+	}
+	// An explicit directory-marker PUT stores no tags.
+	if strings.HasSuffix(r.URL.Path, "/") && r.ContentLength <= 1024 {
+		return false
+	}
+	return true
+}
+
+// isDigits reports whether s is non-empty and all ASCII digits, matching the
+// partNumber route constraint.
+func isDigits(s string) bool {
+	if s == "" {
+		return false
+	}
+	for _, c := range s {
+		if c < '0' || c > '9' {
+			return false
+		}
+	}
+	return true
+}
+
 // IsMultipartContinuationAction returns true for actions that do not carry
 // their own SSE header because SSE is inherited from CreateMultipartUpload.
 func IsMultipartContinuationAction(action string) bool {

weed/s3api/policy_engine/engine_test.go

@@ -896,6 +896,161 @@ func TestExistingObjectTagCondition(t *testing.T) {
 	}
 }
 
+func TestRequestObjectTagCondition(t *testing.T) {
+	engine := NewPolicyEngine()
+
+	policyJSON := `{
+		"Version": "2012-10-17",
+		"Statement": [
+			{
+				"Effect": "Allow",
+				"Principal": "*",
+				"Action": "s3:PutObject",
+				"Resource": "arn:aws:s3:::test-bucket/*",
+				"Condition": {
+					"StringEquals": {
+						"s3:RequestObjectTag/department": ["engineering"]
+					}
+				}
+			}
+		]
+	}`
+
+	if err := engine.SetBucketPolicy("test-bucket", policyJSON); err != nil {
+		t.Fatalf("Failed to set bucket policy: %v", err)
+	}
+
+	copyHeaders := map[string]string{"X-Amz-Copy-Source": "/src-bucket/src-object"}
+	replaceHeaders := map[string]string{
+		"X-Amz-Copy-Source":                    "/src-bucket/src-object",
+		s3_constants.AmzObjectTaggingDirective: "REPLACE",
+	}
+
+	tests := []struct {
+		name     string
+		method   string
+		target   string
+		headers  map[string]string
+		tagging  string
+		expected PolicyEvaluationResult
+	}{
+		{"matching tag allows", "", "", nil, "department=engineering", PolicyResultAllow},
+		{"matching tag among others allows", "", "", nil, "project=alpha&department=engineering", PolicyResultAllow},
+		{"url-encoded value is decoded", "", "", nil, "department=engin%65ering", PolicyResultAllow},
+		{"other value is not allowed", "", "", nil, "department=sales", PolicyResultIndeterminate},
+		{"missing tag is not allowed", "", "", nil, "project=alpha", PolicyResultIndeterminate},
+		{"no tagging header is not allowed", "", "", nil, "", PolicyResultIndeterminate},
+		{"malformed header is not allowed", "", "", nil, "department=engineering&%zz", PolicyResultIndeterminate},
+		{"last value of a repeated key counts", "", "", nil, "department=sales&department=engineering", PolicyResultAllow},
+		{"earlier value of a repeated key is ignored", "", "", nil, "department=engineering&department=sales", PolicyResultIndeterminate},
+		{"header is ignored on PutObjectTagging", "", "?tagging", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header counts with an unrelated query parameter", "", "?foo=bar", nil, "department=engineering", PolicyResultAllow},
+		{"header counts on a Signature V2 presigned PUT", "", "?AWSAccessKeyId=k&Signature=s&Expires=1", nil, "department=engineering", PolicyResultAllow},
+		{"header is ignored on a renameObject subresource", "", "?renameObject", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on a copy that keeps source tags", "", "", copyHeaders, "department=engineering", PolicyResultIndeterminate},
+		{"header counts on a copy that replaces tags", "", "", replaceHeaders, "department=engineering", PolicyResultAllow},
+		{"header is ignored on a multipart copy that replaces tags", "", "?partNumber=1&uploadId=x", replaceHeaders, "department=engineering", PolicyResultIndeterminate},
+		{"header counts on a copy with a lone partNumber", "", "?partNumber=1", replaceHeaders, "department=engineering", PolicyResultAllow},
+		{"header counts on a multipart create", "POST", "?uploads", nil, "department=engineering", PolicyResultAllow},
+		{"header is ignored on a part upload", "", "?partNumber=1&uploadId=x", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on multipart completion", "POST", "?uploadId=x", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on a directory marker", "", "/", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on an acl subresource", "", "?acl", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on a GET", "GET", "", nil, "department=engineering", PolicyResultIndeterminate},
+		{"header is ignored on a DELETE", "DELETE", "", nil, "department=engineering", PolicyResultIndeterminate},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			method := tt.method
+			if method == "" {
+				method = http.MethodPut
+			}
+			req := httptest.NewRequest(method, "/test-bucket/test-object"+tt.target, nil)
+			for k, v := range tt.headers {
+				req.Header.Set(k, v)
+			}
+			if tt.tagging != "" {
+				req.Header.Set(s3_constants.AmzObjectTagging, tt.tagging)
+			}
+
+			args := &PolicyEvaluationArgs{
+				Action:     "s3:PutObject",
+				Resource:   "arn:aws:s3:::test-bucket/test-object",
+				Principal:  "*",
+				Conditions: engine.ExtractConditionValuesFromRequest(req),
+			}
+
+			if result := engine.EvaluatePolicy("test-bucket", args); result != tt.expected {
+				t.Errorf("Expected %v, got %v", tt.expected, result)
+			}
+		})
+	}
+}
+
+func TestRequestObjectTagConditionDeny(t *testing.T) {
+	engine := NewPolicyEngine()
+
+	policyJSON := `{
+		"Version": "2012-10-17",
+		"Statement": [
+			{
+				"Effect": "Allow",
+				"Principal": "*",
+				"Action": "s3:PutObject",
+				"Resource": "arn:aws:s3:::test-bucket/*"
+			},
+			{
+				"Effect": "Deny",
+				"Principal": "*",
+				"Action": "s3:PutObject",
+				"Resource": "arn:aws:s3:::test-bucket/*",
+				"Condition": {
+					"StringNotEquals": {
+						"s3:RequestObjectTag/department": ["engineering"]
+					}
+				}
+			}
+		]
+	}`
+
+	if err := engine.SetBucketPolicy("test-bucket", policyJSON); err != nil {
+		t.Fatalf("Failed to set bucket policy: %v", err)
+	}
+
+	tests := []struct {
+		name     string
+		target   string
+		tagging  string
+		expected PolicyEvaluationResult
+	}{
+		{"required tag passes", "", "department=engineering", PolicyResultAllow},
+		{"other value is denied", "", "department=sales", PolicyResultDeny},
+		{"missing tag is denied", "", "project=alpha", PolicyResultDeny},
+		{"repeated key ending in another value is denied", "", "department=engineering&department=sales", PolicyResultDeny},
+		{"malformed header is denied", "", "department=engineering&%zz", PolicyResultDeny},
+		{"unrelated query parameter does not hide the tag", "?foo=bar", "department=sales", PolicyResultDeny},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodPut, "/test-bucket/test-object"+tt.target, nil)
+			req.Header.Set(s3_constants.AmzObjectTagging, tt.tagging)
+
+			args := &PolicyEvaluationArgs{
+				Action:     "s3:PutObject",
+				Resource:   "arn:aws:s3:::test-bucket/test-object",
+				Principal:  "*",
+				Conditions: engine.ExtractConditionValuesFromRequest(req),
+			}
+
+			if result := engine.EvaluatePolicy("test-bucket", args); result != tt.expected {
+				t.Errorf("Expected %v, got %v", tt.expected, result)
+			}
+		})
+	}
+}
+
 // TestExistingObjectTagConditionMultipleTags tests policies with multiple tag conditions
 func TestExistingObjectTagConditionMultipleTags(t *testing.T) {
 	engine := NewPolicyEngine()