From d6d893c8c374ff78bb7704b4da6666d36939bf1b Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 9 Dec 2025 09:48:13 -0800 Subject: [PATCH] s3: add s3:ExistingObjectTag condition support for bucket policies (#7677) * s3: add s3:ExistingObjectTag condition support in policy engine Add support for s3:ExistingObjectTag/ condition keys in bucket policies, allowing access control based on object tags. Changes: - Add ObjectEntry field to PolicyEvaluationArgs (entry.Extended metadata) - Update EvaluateConditions to handle s3:ExistingObjectTag/ format - Extract tag value from entry metadata using X-Amz-Tagging- prefix This enables policies like: { "Condition": { "StringEquals": { "s3:ExistingObjectTag/status": ["public"] } } } Fixes: https://github.com/seaweedfs/seaweedfs/issues/7447 * s3: update EvaluatePolicy to accept object entry for tag conditions Update BucketPolicyEngine.EvaluatePolicy to accept objectEntry parameter (entry.Extended metadata) for evaluating tag-based policy conditions. Changes: - Add objectEntry parameter to EvaluatePolicy method - Update callers in auth_credentials.go and s3api_bucket_handlers.go - Pass nil for objectEntry in auth layer (entry fetched later in handlers) For tag-based conditions to work, handlers should call EvaluatePolicy with the object's entry.Extended after fetching the entry from filer. * s3: add tests for s3:ExistingObjectTag policy conditions Add comprehensive tests for object tag-based policy conditions: - TestExistingObjectTagCondition: Basic tag matching scenarios - Matching/non-matching tag values - Missing tags, no tags, empty tags - Multiple tags with one matching - TestExistingObjectTagConditionMultipleTags: Multiple tag conditions - Both tags match - Only one tag matches - TestExistingObjectTagDenyPolicy: Deny policies with tag conditions - Default allow without tag - Deny when specific tag present * s3: document s3:ExistingObjectTag support and feature status Update policy engine documentation: - Add s3:ExistingObjectTag/ to supported condition keys - Add 'Object Tag-Based Access Control' section with examples - Add 'Feature Status' section with implemented and planned features Planned features for future implementation: - s3:RequestObjectTag/ - s3:RequestObjectTagKeys - s3:x-amz-server-side-encryption - Cross-account access * Implement tag-based policy re-check in handlers - Add checkPolicyWithEntry helper to S3ApiServer for handlers to re-check policy after fetching object entry (for s3:ExistingObjectTag conditions) - Add HasPolicyForBucket method to policy engine for efficient check - Integrate policy re-check in GetObjectHandler after entry is fetched - Integrate policy re-check in HeadObjectHandler after entry is fetched - Update auth_credentials.go comments to explain two-phase evaluation - Update documentation with supported operations for tag-based conditions This implements 'Approach 1' where handlers re-check the policy with the object entry after fetching it, allowing tag-based conditions to be properly evaluated. * Add integration tests for s3:ExistingObjectTag conditions - Add TestCheckPolicyWithEntry: tests checkPolicyWithEntry helper with various tag scenarios (matching tags, non-matching tags, empty entry, nil entry) - Add TestCheckPolicyWithEntryNoPolicyForBucket: tests early return when no policy - Add TestCheckPolicyWithEntryNilPolicyEngine: tests nil engine handling - Add TestCheckPolicyWithEntryDenyPolicy: tests deny policies with tag conditions - Add TestHasPolicyForBucket: tests HasPolicyForBucket method These tests cover the Phase 2 policy evaluation with object entry metadata, ensuring tag-based conditions are properly evaluated. * Address code review nitpicks - Remove unused extractObjectTags placeholder function (engine.go) - Add clarifying comment about s3:ExistingObjectTag/ evaluation - Consolidate duplicate tag-based examples in README - Factor out tagsToEntry helper to package level in tests * Address code review feedback - Fix unsafe type assertions in GetObjectHandler and HeadObjectHandler when getting identity from context (properly handle type assertion failure) - Extract getConditionContextValue helper to eliminate duplicated logic between EvaluateConditions and EvaluateConditionsLegacy - Ensure consistent handling of missing condition keys (always return empty slice) * Fix GetObjectHandler to match HeadObjectHandler pattern Add safety check for nil objectEntryForSSE before tag-based policy evaluation, ensuring tag-based conditions are always evaluated rather than silently skipped if entry is unexpectedly nil. Addresses review comment from Copilot. * Fix HeadObject action name in docs for consistency Change 'HeadObject' to 's3:HeadObject' to match other action names. * Extract recheckPolicyWithObjectEntry helper to reduce duplication Move the repeated identity extraction and policy re-check logic from GetObjectHandler and HeadObjectHandler into a shared helper method. * Add validation for empty tag key in s3:ExistingObjectTag condition Prevent potential issues with malformed policies containing s3:ExistingObjectTag/ (empty tag key after slash). --- weed/s3api/auth_credentials.go | 6 +- .../policy_engine/README_POLICY_ENGINE.md | 105 ++++++- weed/s3api/policy_engine/conditions.go | 46 +++- weed/s3api/policy_engine/engine.go | 32 +-- weed/s3api/policy_engine/engine_test.go | 244 ++++++++++++++++ weed/s3api/policy_engine/types.go | 5 + weed/s3api/s3_existing_object_tag_test.go | 260 ++++++++++++++++++ weed/s3api/s3api_bucket_handlers.go | 4 +- weed/s3api/s3api_bucket_policy_engine.go | 90 ++---- weed/s3api/s3api_object_handlers.go | 19 ++ weed/s3api/s3api_server.go | 56 ++++ 11 files changed, 762 insertions(+), 105 deletions(-) create mode 100644 weed/s3api/s3_existing_object_tag_test.go diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index b5824f2d1..3f4670a7e 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -582,8 +582,10 @@ func (iam *IdentityAccessManagement) authRequest(r *http.Request, action Action) // - No policy or indeterminate → fall through to IAM checks if iam.policyEngine != nil && bucket != "" { principal := buildPrincipalARN(identity) - // Use context-aware policy evaluation to get the correct S3 action - allowed, evaluated, err := iam.policyEngine.EvaluatePolicyWithContext(bucket, object, string(action), principal, r) + // Phase 1: Evaluate bucket policy without object entry. + // Tag-based conditions (s3:ExistingObjectTag) are re-checked by handlers + // after fetching the entry, which is the Phase 2 check. + allowed, evaluated, err := iam.policyEngine.EvaluatePolicy(bucket, object, string(action), principal, r, nil) if err != nil { // SECURITY: Fail-close on policy evaluation errors diff --git a/weed/s3api/policy_engine/README_POLICY_ENGINE.md b/weed/s3api/policy_engine/README_POLICY_ENGINE.md index 70dbf37f1..9a5ab3b3c 100644 --- a/weed/s3api/policy_engine/README_POLICY_ENGINE.md +++ b/weed/s3api/policy_engine/README_POLICY_ENGINE.md @@ -135,8 +135,70 @@ Standard AWS condition keys are supported: - `aws:UserAgent` - Client user agent - `s3:x-amz-acl` - Requested ACL - `s3:VersionId` - Object version ID +- `s3:ExistingObjectTag/` - Value of an existing object tag (see example below) - And many more... +### 5. Object Tag-Based Access Control + +You can control access based on object tags using `s3:ExistingObjectTag/`: + +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::my-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/status": ["public"] + } + } + } + ] +} +``` + +This allows anonymous access only to objects that have a tag `status=public`. + +**Deny access to confidential objects:** + +```json +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::my-bucket/*" + }, + { + "Effect": "Deny", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::my-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/classification": ["confidential", "secret"] + } + } + } + ] +} +``` + +**Supported Operations for Tag-Based Conditions:** + +Tag-based conditions (`s3:ExistingObjectTag/`) are evaluated for the following operations: +- `s3:GetObject` (GET object) +- `s3:GetObjectVersion` (GET object with versionId) +- `s3:HeadObject` (HEAD object) + +Note: For these conditions to be evaluated, the object must exist and the policy engine re-checks access after fetching the object metadata. + ## Policy Evaluation ### Evaluation Order (AWS-Compatible) @@ -270,10 +332,39 @@ go test -v -run TestPolicyValidation ## Compatibility -- ✅ **Full backward compatibility** with existing `identities.json` -- ✅ **AWS S3 API compatibility** for bucket policies -- ✅ **Standard condition operators** and keys -- ✅ **Proper evaluation precedence** (Deny > Allow > Default Deny) -- ✅ **Performance optimized** with caching and compiled patterns - -The policy engine provides a seamless upgrade path from SeaweedFS's existing simple IAM system to full AWS S3-compatible policies, giving you the best of both worlds: simplicity for basic use cases and power for complex enterprise scenarios. \ No newline at end of file +- Full backward compatibility with existing `identities.json` +- AWS S3 API compatibility for bucket policies +- Standard condition operators and keys +- Proper evaluation precedence (Deny > Allow > Default Deny) +- Performance optimized with caching and compiled patterns + +The policy engine provides a seamless upgrade path from SeaweedFS's existing simple IAM system to full AWS S3-compatible policies, giving you the best of both worlds: simplicity for basic use cases and power for complex enterprise scenarios. + +## Feature Status + +### Implemented + +| Feature | Description | +|---------|-------------| +| Bucket Policies | Full AWS S3-compatible bucket policies | +| Condition Operators | StringEquals, IpAddress, Bool, DateGreaterThan, etc. | +| `aws:SourceIp` | IP-based access control with CIDR support | +| `aws:SecureTransport` | Require HTTPS | +| `aws:CurrentTime` | Time-based access control | +| `s3:ExistingObjectTag/` | Tag-based access control for existing objects | +| Wildcard Patterns | Support for `*` and `?` in actions and resources | +| Principal Matching | `*`, account IDs, and user ARNs | + +### Planned + +| Feature | GitHub Issue | +|---------|--------------| +| `s3:RequestObjectTag/` | 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 | +| `s3:x-amz-storage-class` | Storage class condition | +| Cross-account access | Access across different accounts | +| VPC Endpoint policies | Network-level policies | + +For feature requests or to track progress, see the [GitHub Issues](https://github.com/seaweedfs/seaweedfs/issues). \ No newline at end of file diff --git a/weed/s3api/policy_engine/conditions.go b/weed/s3api/policy_engine/conditions.go index fc8005fd0..4e310060a 100644 --- a/weed/s3api/policy_engine/conditions.go +++ b/weed/s3api/policy_engine/conditions.go @@ -10,6 +10,7 @@ import ( "time" "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" ) // LRUNode represents a node in the doubly-linked list for efficient LRU operations @@ -705,8 +706,36 @@ func GetConditionEvaluator(operator string) (ConditionEvaluator, error) { } } +// ExistingObjectTagPrefix is the prefix for S3 policy condition keys +const ExistingObjectTagPrefix = "s3:ExistingObjectTag/" + +// getConditionContextValue resolves the value(s) for a condition key. +// For s3:ExistingObjectTag/ conditions, it looks up the tag in objectEntry. +// For other condition keys, it looks up the value in contextValues. +func getConditionContextValue(key string, contextValues map[string][]string, objectEntry map[string][]byte) []string { + if strings.HasPrefix(key, ExistingObjectTagPrefix) { + tagKey := key[len(ExistingObjectTagPrefix):] + if tagKey == "" { + return []string{} // Invalid: empty tag key + } + metadataKey := s3_constants.AmzObjectTaggingPrefix + tagKey + if objectEntry != nil { + if tagValue, exists := objectEntry[metadataKey]; exists { + return []string{string(tagValue)} + } + } + return []string{} + } + + if vals, exists := contextValues[key]; exists { + return vals + } + return []string{} +} + // EvaluateConditions evaluates all conditions in a policy statement -func EvaluateConditions(conditions PolicyConditions, contextValues map[string][]string) bool { +// objectEntry is the object's metadata from entry.Extended (can be nil) +func EvaluateConditions(conditions PolicyConditions, contextValues map[string][]string, objectEntry map[string][]byte) bool { if len(conditions) == 0 { return true // No conditions means always true } @@ -719,11 +748,7 @@ func EvaluateConditions(conditions PolicyConditions, contextValues map[string][] } for key, value := range conditionMap { - contextVals, exists := contextValues[key] - if !exists { - contextVals = []string{} - } - + contextVals := getConditionContextValue(key, contextValues, objectEntry) if !conditionEvaluator.Evaluate(value.Strings(), contextVals) { return false // If any condition fails, the whole condition block fails } @@ -734,7 +759,8 @@ func EvaluateConditions(conditions PolicyConditions, contextValues map[string][] } // EvaluateConditionsLegacy evaluates conditions using the old interface{} format for backward compatibility -func EvaluateConditionsLegacy(conditions map[string]interface{}, contextValues map[string][]string) bool { +// objectEntry is the object's metadata from entry.Extended (can be nil) +func EvaluateConditionsLegacy(conditions map[string]interface{}, contextValues map[string][]string, objectEntry map[string][]byte) bool { if len(conditions) == 0 { return true // No conditions means always true } @@ -753,11 +779,7 @@ func EvaluateConditionsLegacy(conditions map[string]interface{}, contextValues m } for key, value := range conditionMapTyped { - contextVals, exists := contextValues[key] - if !exists { - contextVals = []string{} - } - + contextVals := getConditionContextValue(key, contextValues, objectEntry) if !conditionEvaluator.Evaluate(value, contextVals) { return false // If any condition fails, the whole condition block fails } diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index 01af3c240..62e375eff 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -91,6 +91,14 @@ func (engine *PolicyEngine) DeleteBucketPolicy(bucketName string) error { return nil } +// HasPolicyForBucket checks if a bucket has a policy configured +func (engine *PolicyEngine) HasPolicyForBucket(bucketName string) bool { + engine.mutex.RLock() + defer engine.mutex.RUnlock() + _, exists := engine.contexts[bucketName] + return exists +} + // EvaluatePolicy evaluates a policy for the given arguments func (engine *PolicyEngine) EvaluatePolicy(bucketName string, args *PolicyEvaluationArgs) PolicyEvaluationResult { engine.mutex.RLock() @@ -154,7 +162,7 @@ func (engine *PolicyEngine) evaluateStatement(stmt *CompiledStatement, args *Pol // Check conditions if len(stmt.Statement.Condition) > 0 { - if !EvaluateConditions(stmt.Statement.Condition, args.Conditions) { + if !EvaluateConditions(stmt.Statement.Condition, args.Conditions, args.ObjectEntry) { return false } } @@ -201,10 +209,8 @@ func ExtractConditionValuesFromRequest(r *http.Request) map[string][]string { values["aws:Referer"] = []string{referer} } - // S3 object-level conditions - if r.Method == "GET" || r.Method == "HEAD" { - values["s3:ExistingObjectTag"] = extractObjectTags(r) - } + // Note: s3:ExistingObjectTag/ conditions are evaluated using objectEntry + // passed to EvaluatePolicy, not extracted from the request. // S3 bucket-level conditions if delimiter := r.URL.Query().Get("delimiter"); delimiter != "" { @@ -243,13 +249,6 @@ func ExtractConditionValuesFromRequest(r *http.Request) map[string][]string { return values } -// extractObjectTags extracts object tags from request (placeholder implementation) -func extractObjectTags(r *http.Request) []string { - // This would need to be implemented based on how object tags are stored - // For now, return empty slice - return []string{} -} - // BuildResourceArn builds an ARN for the given bucket and object func BuildResourceArn(bucketName, objectName string) string { if objectName == "" { @@ -352,15 +351,6 @@ func GetObjectNameFromArn(arn string) string { return "" } -// HasPolicyForBucket checks if a bucket has a policy -func (engine *PolicyEngine) HasPolicyForBucket(bucketName string) bool { - engine.mutex.RLock() - defer engine.mutex.RUnlock() - - _, exists := engine.contexts[bucketName] - return exists -} - // GetPolicyStatements returns all policy statements for a bucket func (engine *PolicyEngine) GetPolicyStatements(bucketName string) []PolicyStatement { engine.mutex.RLock() diff --git a/weed/s3api/policy_engine/engine_test.go b/weed/s3api/policy_engine/engine_test.go index 1bb36dc4a..4de537ac1 100644 --- a/weed/s3api/policy_engine/engine_test.go +++ b/weed/s3api/policy_engine/engine_test.go @@ -5,9 +5,23 @@ import ( "net/url" "testing" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" ) +// tagsToEntry converts a map of tag key-value pairs to the entry.Extended format +// used for s3:ExistingObjectTag/ condition evaluation +func tagsToEntry(tags map[string]string) map[string][]byte { + if tags == nil { + return nil + } + entry := make(map[string][]byte) + for k, v := range tags { + entry[s3_constants.AmzObjectTaggingPrefix+k] = []byte(v) + } + return entry +} + func TestPolicyEngine(t *testing.T) { engine := NewPolicyEngine() @@ -714,3 +728,233 @@ type MockLegacyIAM struct{} func (m *MockLegacyIAM) authRequest(r *http.Request, action Action) (Identity, s3err.ErrorCode) { return nil, s3err.ErrNone } + +// TestExistingObjectTagCondition tests s3:ExistingObjectTag/ condition support +func TestExistingObjectTagCondition(t *testing.T) { + engine := NewPolicyEngine() + + // Policy that allows GetObject only for objects with specific tag + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/status": ["public"] + } + } + } + ] + }` + + err := engine.SetBucketPolicy("test-bucket", policyJSON) + if err != nil { + t.Fatalf("Failed to set bucket policy: %v", err) + } + + tests := []struct { + name string + objectTags map[string]string + expected PolicyEvaluationResult + }{ + { + name: "Matching tag value - should allow", + objectTags: map[string]string{"status": "public"}, + expected: PolicyResultAllow, + }, + { + name: "Non-matching tag value - should be indeterminate", + objectTags: map[string]string{"status": "private"}, + expected: PolicyResultIndeterminate, + }, + { + name: "Missing tag - should be indeterminate", + objectTags: map[string]string{"other": "value"}, + expected: PolicyResultIndeterminate, + }, + { + name: "No tags - should be indeterminate", + objectTags: nil, + expected: PolicyResultIndeterminate, + }, + { + name: "Empty tags - should be indeterminate", + objectTags: map[string]string{}, + expected: PolicyResultIndeterminate, + }, + { + name: "Multiple tags with matching one - should allow", + objectTags: map[string]string{"status": "public", "owner": "admin"}, + expected: PolicyResultAllow, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + args := &PolicyEvaluationArgs{ + Action: "s3:GetObject", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", + ObjectEntry: tagsToEntry(tt.objectTags), + } + + result := engine.EvaluatePolicy("test-bucket", args) + if 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() + + // Policy that requires multiple tag conditions + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/status": ["public"], + "s3:ExistingObjectTag/tier": ["free", "premium"] + } + } + } + ] + }` + + err := engine.SetBucketPolicy("test-bucket", policyJSON) + if err != nil { + t.Fatalf("Failed to set bucket policy: %v", err) + } + + tests := []struct { + name string + objectTags map[string]string + expected PolicyEvaluationResult + }{ + { + name: "Both tags match - should allow", + objectTags: map[string]string{"status": "public", "tier": "free"}, + expected: PolicyResultAllow, + }, + { + name: "Both tags match (premium tier) - should allow", + objectTags: map[string]string{"status": "public", "tier": "premium"}, + expected: PolicyResultAllow, + }, + { + name: "Only status matches - should be indeterminate", + objectTags: map[string]string{"status": "public"}, + expected: PolicyResultIndeterminate, + }, + { + name: "Only tier matches - should be indeterminate", + objectTags: map[string]string{"tier": "free"}, + expected: PolicyResultIndeterminate, + }, + { + name: "Neither tag matches - should be indeterminate", + objectTags: map[string]string{"status": "private", "tier": "basic"}, + expected: PolicyResultIndeterminate, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + args := &PolicyEvaluationArgs{ + Action: "s3:GetObject", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", + ObjectEntry: tagsToEntry(tt.objectTags), + } + + result := engine.EvaluatePolicy("test-bucket", args) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} + +// TestExistingObjectTagDenyPolicy tests deny policies with tag conditions +func TestExistingObjectTagDenyPolicy(t *testing.T) { + engine := NewPolicyEngine() + + // Policy that denies access to objects with confidential tag + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*" + }, + { + "Effect": "Deny", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/classification": ["confidential"] + } + } + } + ] + }` + + err := engine.SetBucketPolicy("test-bucket", policyJSON) + if err != nil { + t.Fatalf("Failed to set bucket policy: %v", err) + } + + tests := []struct { + name string + objectTags map[string]string + expected PolicyEvaluationResult + }{ + { + name: "No tags - allow by default statement", + objectTags: nil, + expected: PolicyResultAllow, + }, + { + name: "Non-confidential tag - allow", + objectTags: map[string]string{"classification": "public"}, + expected: PolicyResultAllow, + }, + { + name: "Confidential tag - deny", + objectTags: map[string]string{"classification": "confidential"}, + expected: PolicyResultDeny, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + args := &PolicyEvaluationArgs{ + Action: "s3:GetObject", + Resource: "arn:aws:s3:::test-bucket/test-object", + Principal: "*", + ObjectEntry: tagsToEntry(tt.objectTags), + } + + result := engine.EvaluatePolicy("test-bucket", args) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} diff --git a/weed/s3api/policy_engine/types.go b/weed/s3api/policy_engine/types.go index d68b1f297..c6c76b55f 100644 --- a/weed/s3api/policy_engine/types.go +++ b/weed/s3api/policy_engine/types.go @@ -106,6 +106,11 @@ type PolicyEvaluationArgs struct { Resource string Principal string Conditions map[string][]string + // ObjectEntry is the object's metadata from entry.Extended. + // Used for evaluating conditions like s3:ExistingObjectTag/. + // Tags are stored with s3_constants.AmzObjectTaggingPrefix (X-Amz-Tagging-) prefix. + // Can be nil for bucket-level operations or when object doesn't exist. + ObjectEntry map[string][]byte } // PolicyCache for caching compiled policies diff --git a/weed/s3api/s3_existing_object_tag_test.go b/weed/s3api/s3_existing_object_tag_test.go new file mode 100644 index 000000000..65cca9f1a --- /dev/null +++ b/weed/s3api/s3_existing_object_tag_test.go @@ -0,0 +1,260 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCheckPolicyWithEntry tests the checkPolicyWithEntry helper method +// This verifies that tag-based conditions are properly evaluated when +// the object entry is available (Phase 2 of policy evaluation) +func TestCheckPolicyWithEntry(t *testing.T) { + // Create a minimal S3ApiServer with policy engine + s3a := &S3ApiServer{ + policyEngine: NewBucketPolicyEngine(), + } + + bucket := "test-bucket" + + // Set up a policy that allows access only to objects with status=public tag + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/status": ["public"] + } + } + } + ] + }` + + err := s3a.policyEngine.engine.SetBucketPolicy(bucket, policyJSON) + require.NoError(t, err) + + tests := []struct { + name string + objectEntry map[string][]byte + wantErrCode s3err.ErrorCode + wantChecked bool + description string + }{ + { + name: "object with public tag - allow", + objectEntry: map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "status": []byte("public"), + }, + wantErrCode: s3err.ErrNone, + wantChecked: true, + description: "Object with status=public tag should be allowed", + }, + { + name: "object with private tag - indeterminate (no matching statement)", + objectEntry: map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "status": []byte("private"), + }, + wantErrCode: s3err.ErrNone, + wantChecked: false, // Indeterminate = no policy matched + description: "Object with status=private should return indeterminate (falls back to IAM)", + }, + { + name: "object with no tags - indeterminate (no matching statement)", + objectEntry: map[string][]byte{}, + wantErrCode: s3err.ErrNone, + wantChecked: false, // Indeterminate = no policy matched + description: "Object without tags should return indeterminate (falls back to IAM)", + }, + { + name: "nil entry - indeterminate (condition cannot be evaluated)", + objectEntry: nil, + wantErrCode: s3err.ErrNone, + wantChecked: false, // Indeterminate = no policy matched + description: "Nil entry should return indeterminate (falls back to IAM)", + }, + { + name: "object with multiple tags including public - allow", + objectEntry: map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "status": []byte("public"), + s3_constants.AmzObjectTaggingPrefix + "category": []byte("documents"), + }, + wantErrCode: s3err.ErrNone, + wantChecked: true, + description: "Object with status=public among other tags should be allowed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/test-bucket/test-object.txt", nil) + object := "/test-object.txt" + principal := "*" + action := string(s3_constants.ACTION_READ) + + errCode, checked := s3a.checkPolicyWithEntry(req, bucket, object, action, principal, tt.objectEntry) + + assert.Equal(t, tt.wantErrCode, errCode, tt.description) + assert.Equal(t, tt.wantChecked, checked, "Policy should be evaluated") + }) + } +} + +// TestCheckPolicyWithEntryNoPolicyForBucket tests that checkPolicyWithEntry +// returns early when there's no policy for the bucket +func TestCheckPolicyWithEntryNoPolicyForBucket(t *testing.T) { + s3a := &S3ApiServer{ + policyEngine: NewBucketPolicyEngine(), + } + + bucket := "bucket-without-policy" + req := httptest.NewRequest(http.MethodGet, "/bucket-without-policy/test.txt", nil) + + objectEntry := map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "status": []byte("private"), + } + + errCode, checked := s3a.checkPolicyWithEntry(req, bucket, "/test.txt", string(s3_constants.ACTION_READ), "*", objectEntry) + + assert.Equal(t, s3err.ErrNone, errCode, "No policy should mean no denial") + assert.False(t, checked, "Policy should not be evaluated when bucket has no policy") +} + +// TestCheckPolicyWithEntryNilPolicyEngine tests that checkPolicyWithEntry +// handles nil policy engine gracefully +func TestCheckPolicyWithEntryNilPolicyEngine(t *testing.T) { + s3a := &S3ApiServer{ + policyEngine: nil, + } + + req := httptest.NewRequest(http.MethodGet, "/test-bucket/test.txt", nil) + + errCode, checked := s3a.checkPolicyWithEntry(req, "test-bucket", "/test.txt", string(s3_constants.ACTION_READ), "*", nil) + + assert.Equal(t, s3err.ErrNone, errCode, "Nil policy engine should allow access") + assert.False(t, checked, "Policy should not be evaluated when engine is nil") +} + +// TestCheckPolicyWithEntryDenyPolicy tests deny policies with tag conditions +func TestCheckPolicyWithEntryDenyPolicy(t *testing.T) { + s3a := &S3ApiServer{ + policyEngine: NewBucketPolicyEngine(), + } + + bucket := "secure-bucket" + + // Policy that allows all access but denies access to confidential objects + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::secure-bucket/*" + }, + { + "Effect": "Deny", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::secure-bucket/*", + "Condition": { + "StringEquals": { + "s3:ExistingObjectTag/classification": ["confidential"] + } + } + } + ] + }` + + err := s3a.policyEngine.engine.SetBucketPolicy(bucket, policyJSON) + require.NoError(t, err) + + tests := []struct { + name string + objectEntry map[string][]byte + wantErrCode s3err.ErrorCode + description string + }{ + { + name: "public object - allow", + objectEntry: map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "classification": []byte("public"), + }, + wantErrCode: s3err.ErrNone, + description: "Object with classification=public should be allowed", + }, + { + name: "confidential object - deny", + objectEntry: map[string][]byte{ + s3_constants.AmzObjectTaggingPrefix + "classification": []byte("confidential"), + }, + wantErrCode: s3err.ErrAccessDenied, + description: "Object with classification=confidential should be denied by explicit deny", + }, + { + name: "no classification tag - allow", + objectEntry: map[string][]byte{}, + wantErrCode: s3err.ErrNone, + description: "Object without classification tag should be allowed (deny condition not met)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/secure-bucket/test.txt", nil) + + errCode, checked := s3a.checkPolicyWithEntry(req, bucket, "/test.txt", string(s3_constants.ACTION_READ), "*", tt.objectEntry) + + assert.Equal(t, tt.wantErrCode, errCode, tt.description) + assert.True(t, checked, "Policy should be evaluated") + }) + } +} + +// TestHasPolicyForBucket tests the HasPolicyForBucket method +func TestHasPolicyForBucket(t *testing.T) { + engine := policy_engine.NewPolicyEngine() + + // Initially no policy + assert.False(t, engine.HasPolicyForBucket("test-bucket")) + + // Set a policy + policyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*" + } + ] + }` + err := engine.SetBucketPolicy("test-bucket", policyJSON) + require.NoError(t, err) + + // Now has policy + assert.True(t, engine.HasPolicyForBucket("test-bucket")) + + // Other bucket still has no policy + assert.False(t, engine.HasPolicyForBucket("other-bucket")) + + // Delete the policy + err = engine.DeleteBucketPolicy("test-bucket") + require.NoError(t, err) + + // No longer has policy + assert.False(t, engine.HasPolicyForBucket("test-bucket")) +} + diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 09bea9aa8..2d67aa551 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -765,8 +765,8 @@ func (s3a *S3ApiServer) AuthWithPublicRead(handler http.HandlerFunc, action Acti // Check bucket policy for anonymous access using the policy engine principal := "*" // Anonymous principal - // Use context-aware policy evaluation to get the correct S3 action - allowed, evaluated, err := s3a.policyEngine.EvaluatePolicyWithContext(bucket, object, string(action), principal, r) + // Evaluate bucket policy (objectEntry nil - not yet fetched) + allowed, evaluated, err := s3a.policyEngine.EvaluatePolicy(bucket, object, string(action), principal, r, nil) if err != nil { // SECURITY: Fail-close on policy evaluation errors // If we can't evaluate the policy, deny access rather than falling through to IAM diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index fc674e12f..c8cd05344 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -87,56 +87,27 @@ func (bpe *BucketPolicyEngine) DeleteBucketPolicy(bucket string) error { return bpe.engine.DeleteBucketPolicy(bucket) } -// EvaluatePolicy evaluates whether an action is allowed by bucket policy -// Returns: (allowed bool, evaluated bool, error) -// - allowed: whether the policy allows the action -// - evaluated: whether a policy was found and evaluated (false = no policy exists) -// - error: any error during evaluation -func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal string) (allowed bool, evaluated bool, err error) { - // Validate required parameters - if bucket == "" { - return false, false, fmt.Errorf("bucket cannot be empty") - } - if action == "" { - return false, false, fmt.Errorf("action cannot be empty") - } - - // Convert action to S3 action format using base mapping (no HTTP context available) - s3Action := mapBaseActionToS3Format(action) - - // Build resource ARN - resource := buildResourceARN(bucket, object) - - glog.V(4).Infof("EvaluatePolicy: bucket=%s, resource=%s, action=%s, principal=%s", bucket, resource, s3Action, principal) - - // Evaluate using the policy engine - args := &policy_engine.PolicyEvaluationArgs{ - Action: s3Action, - Resource: resource, - Principal: principal, - } - - result := bpe.engine.EvaluatePolicy(bucket, args) - - switch result { - case policy_engine.PolicyResultAllow: - glog.V(3).Infof("EvaluatePolicy: ALLOW - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) - return true, true, nil - case policy_engine.PolicyResultDeny: - glog.V(3).Infof("EvaluatePolicy: DENY - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) - return false, true, nil - case policy_engine.PolicyResultIndeterminate: - // No policy exists for this bucket - glog.V(4).Infof("EvaluatePolicy: INDETERMINATE (no policy) - bucket=%s", bucket) - return false, false, nil - default: - return false, false, fmt.Errorf("unknown policy result: %v", result) - } +// HasPolicyForBucket checks if a bucket has a policy configured +func (bpe *BucketPolicyEngine) HasPolicyForBucket(bucket string) bool { + return bpe.engine.HasPolicyForBucket(bucket) } -// EvaluatePolicyWithContext evaluates whether an action is allowed by bucket policy using HTTP request context -// This version uses the HTTP request to determine the actual S3 action more accurately -func (bpe *BucketPolicyEngine) EvaluatePolicyWithContext(bucket, object, action, principal string, r *http.Request) (allowed bool, evaluated bool, err error) { +// EvaluatePolicy evaluates whether an action is allowed by bucket policy +// +// Parameters: +// - bucket: the bucket name +// - object: the object key (can be empty for bucket-level operations) +// - action: the action being performed (e.g., "Read", "Write") +// - principal: the principal ARN or identifier +// - r: the HTTP request (optional, used for condition evaluation and action resolution) +// - objectEntry: the object's metadata from entry.Extended (can be nil at auth time, +// should be passed when available for tag-based conditions like s3:ExistingObjectTag) +// +// Returns: +// - allowed: whether the policy allows the action +// - evaluated: whether a policy was found and evaluated (false = no policy exists) +// - error: any error during evaluation +func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal string, r *http.Request, objectEntry map[string][]byte) (allowed bool, evaluated bool, err error) { // Validate required parameters if bucket == "" { return false, false, fmt.Errorf("bucket cannot be empty") @@ -145,41 +116,38 @@ func (bpe *BucketPolicyEngine) EvaluatePolicyWithContext(bucket, object, action, return false, false, fmt.Errorf("action cannot be empty") } - // Convert action to S3 action format using request context + // Convert action to S3 action format // ResolveS3Action handles nil request internally (falls back to mapBaseActionToS3Format) s3Action := ResolveS3Action(r, action, bucket, object) // Build resource ARN resource := buildResourceARN(bucket, object) - glog.V(4).Infof("EvaluatePolicyWithContext: bucket=%s, resource=%s, action=%s (from %s), principal=%s", - bucket, resource, s3Action, action, principal) + glog.V(4).Infof("EvaluatePolicy: bucket=%s, resource=%s, action=%s, principal=%s", + bucket, resource, s3Action, principal) // Evaluate using the policy engine args := &policy_engine.PolicyEvaluationArgs{ - Action: s3Action, - Resource: resource, - Principal: principal, + Action: s3Action, + Resource: resource, + Principal: principal, + ObjectEntry: objectEntry, } result := bpe.engine.EvaluatePolicy(bucket, args) switch result { case policy_engine.PolicyResultAllow: - glog.V(3).Infof("EvaluatePolicyWithContext: ALLOW - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) + glog.V(3).Infof("EvaluatePolicy: ALLOW - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) return true, true, nil case policy_engine.PolicyResultDeny: - glog.V(3).Infof("EvaluatePolicyWithContext: DENY - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) + glog.V(3).Infof("EvaluatePolicy: DENY - bucket=%s, action=%s, principal=%s", bucket, s3Action, principal) return false, true, nil case policy_engine.PolicyResultIndeterminate: // No policy exists for this bucket - glog.V(4).Infof("EvaluatePolicyWithContext: INDETERMINATE (no policy) - bucket=%s", bucket) + glog.V(4).Infof("EvaluatePolicy: INDETERMINATE (no policy) - bucket=%s", bucket) return false, false, nil default: return false, false, fmt.Errorf("unknown policy result: %v", result) } } - -// NOTE: The convertActionToS3Format wrapper has been removed for simplicity. -// EvaluatePolicy and EvaluatePolicyWithContext now call ResolveS3Action or -// mapBaseActionToS3Format directly, making the control flow more explicit. diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 43cc4e5fc..bdd16195a 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -634,6 +634,19 @@ func (s3a *S3ApiServer) GetObjectHandler(w http.ResponseWriter, r *http.Request) } entryFetchTime = time.Since(tEntryFetch) + // Safety check: entry must be valid before tag-based policy evaluation + if objectEntryForSSE == nil { + glog.Errorf("GetObjectHandler: objectEntryForSSE is nil for %s/%s (should not happen)", bucket, object) + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) + return + } + + // Re-check bucket policy with object entry for tag-based conditions (e.g., s3:ExistingObjectTag) + if errCode := s3a.recheckPolicyWithObjectEntry(r, bucket, object, string(s3_constants.ACTION_READ), objectEntryForSSE.Extended, "GetObjectHandler"); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) + return + } + // Check if PartNumber query parameter is present (for multipart GET requests) partNumberStr := r.URL.Query().Get("partNumber") if partNumberStr == "" { @@ -2187,6 +2200,12 @@ func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request return } + // Re-check bucket policy with object entry for tag-based conditions (e.g., s3:ExistingObjectTag) + if errCode := s3a.recheckPolicyWithObjectEntry(r, bucket, object, string(s3_constants.ACTION_READ), objectEntryForSSE.Extended, "HeadObjectHandler"); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) + return + } + // Implicit Directory Handling for s3fs Compatibility // ==================================================== // diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 939c891c3..94321814e 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -252,6 +252,62 @@ func (s3a *S3ApiServer) syncBucketPolicyToEngine(bucket string, policyDoc *polic } } +// checkPolicyWithEntry re-evaluates bucket policy with the object entry metadata. +// This is used by handlers after fetching the entry to enforce tag-based conditions +// like s3:ExistingObjectTag/. +// +// Returns: +// - s3err.ErrCode: ErrNone if allowed, ErrAccessDenied if denied +// - bool: true if policy was evaluated (has policy for bucket), false if no policy +func (s3a *S3ApiServer) checkPolicyWithEntry(r *http.Request, bucket, object, action, principal string, objectEntry map[string][]byte) (s3err.ErrorCode, bool) { + if s3a.policyEngine == nil { + return s3err.ErrNone, false + } + + // Skip if no policy for this bucket + if !s3a.policyEngine.HasPolicyForBucket(bucket) { + return s3err.ErrNone, false + } + + allowed, evaluated, err := s3a.policyEngine.EvaluatePolicy(bucket, object, action, principal, r, objectEntry) + if err != nil { + glog.Errorf("checkPolicyWithEntry: error evaluating policy for %s/%s: %v", bucket, object, err) + return s3err.ErrInternalError, true + } + + if !evaluated { + return s3err.ErrNone, false + } + + if !allowed { + glog.V(3).Infof("checkPolicyWithEntry: policy denied access to %s/%s for principal %s", bucket, object, principal) + return s3err.ErrAccessDenied, true + } + + return s3err.ErrNone, true +} + +// recheckPolicyWithObjectEntry performs the second phase of policy evaluation after +// an object's entry is fetched. It extracts identity from context and checks for +// tag-based conditions like s3:ExistingObjectTag/. +// +// Returns s3err.ErrNone if allowed, or an error code if denied or on error. +func (s3a *S3ApiServer) recheckPolicyWithObjectEntry(r *http.Request, bucket, object, action string, objectEntry map[string][]byte, handlerName string) s3err.ErrorCode { + identityRaw := GetIdentityFromContext(r) + var identity *Identity + if identityRaw != nil { + var ok bool + identity, ok = identityRaw.(*Identity) + if !ok { + glog.Errorf("%s: unexpected identity type in context for %s/%s", handlerName, bucket, object) + return s3err.ErrInternalError + } + } + principal := buildPrincipalARN(identity) + errCode, _ := s3a.checkPolicyWithEntry(r, bucket, object, action, principal, objectEntry) + return errCode +} + // classifyDomainNames classifies domains into path-style and virtual-host style domains. // A domain is considered path-style if: // 1. It contains a dot (has subdomains)