From 79994b69afaa7f2967a312a414c3bbbaaf80762c Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 11 Sep 2026 22:17:11 -0700 Subject: [PATCH] s3: fail closed on unsupported bucket-policy condition operators (#11283) * s3: support StringEqualsIgnoreCase and related condition operators The S3 bucket-policy condition engine rejected StringEqualsIgnoreCase (and StringNotEqualsIgnoreCase, StringLikeIgnoreCase, StringNotLikeIgnoreCase), which AWS and the IAM policy engine both accept. Add evaluators and register them in GetConditionEvaluator so valid policies using these operators evaluate correctly instead of being skipped. * s3: reject bucket policies with unsupported condition operators validateStatement did not check Condition operators, so a policy with an unknown operator (e.g. a typo or unsupported key) was accepted at upload time and only surfaced at evaluation, where it was silently skipped. Reuse GetConditionEvaluator to reject unknown operators when a policy is parsed or stored, failing closed at the entry point instead of relying on evaluation-time handling. * s3: fail closed on unsupported condition operators at evaluation EvaluateConditions skipped statements whose condition operator was unsupported, logging a warning and continuing. With no remaining conditions to fail, the function returned true, so an Allow statement conditioned on an unrecognized operator became unconditional and granted access to private objects. Return false instead so an unrecognized operator fails the condition block and the statement does not match, matching the fail-closed behavior of the IAM policy engine. * s3: validate condition operators at upload time only, not load time Validating condition operators in validateStatement rejected the whole policy document from ParsePolicy, which SetBucketPolicy uses when loading stored bucket policies. A legacy policy saved before this change could contain an unsupported operator, and rejecting it at load time dropped the entire policy - including unrelated explicit Deny statements - so the bucket lost its protections. Move the operator check into ValidateBucketPolicy, which only the PutBucketPolicy handler and admin UI run at upload time, so legacy policies still load and EvaluateConditions fails the unsupported statement closed instead. * s3: drop non-AWS StringLikeIgnoreCase and StringNotLikeIgnoreCase operators AWS defines StringEqualsIgnoreCase and StringNotEqualsIgnoreCase but not StringLikeIgnoreCase or StringNotLikeIgnoreCase (StringLike and StringNotLike are case-sensitive only). Registering the wildcard IgnoreCase variants made the engine accept operators AWS rejects. Keep only the two AWS-defined IgnoreCase operators and add a test asserting the wildcard IgnoreCase names are unsupported. --- weed/s3api/policy_engine/bucket_policy.go | 6 ++ weed/s3api/policy_engine/conditions.go | 36 ++++++++- .../conditions_failclosed_test.go | 18 +++++ .../conditions_ignorecase_test.go | 41 ++++++++++ .../conditions_validation_test.go | 74 +++++++++++++++++++ 5 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 weed/s3api/policy_engine/conditions_failclosed_test.go create mode 100644 weed/s3api/policy_engine/conditions_ignorecase_test.go create mode 100644 weed/s3api/policy_engine/conditions_validation_test.go diff --git a/weed/s3api/policy_engine/bucket_policy.go b/weed/s3api/policy_engine/bucket_policy.go index d909489fd..aac1f4bd5 100644 --- a/weed/s3api/policy_engine/bucket_policy.go +++ b/weed/s3api/policy_engine/bucket_policy.go @@ -49,6 +49,12 @@ func ValidateBucketPolicy(policyDoc *PolicyDocument, bucket string) error { return fmt.Errorf("statement %d: bucket policies only support S3 actions, got %s", i, action) } } + + for operator := range statement.Condition { + if _, err := GetConditionEvaluator(operator); err != nil { + return fmt.Errorf("statement %d: unsupported condition operator %q: %v", i, operator, err) + } + } } return nil diff --git a/weed/s3api/policy_engine/conditions.go b/weed/s3api/policy_engine/conditions.go index af55b06c2..c5ac36fd6 100644 --- a/weed/s3api/policy_engine/conditions.go +++ b/weed/s3api/policy_engine/conditions.go @@ -218,6 +218,36 @@ func (e *StringNotLikeEvaluator) Evaluate(conditionValue interface{}, contextVal return true } +// StringEqualsIgnoreCaseEvaluator evaluates StringEqualsIgnoreCase conditions +type StringEqualsIgnoreCaseEvaluator struct{} + +func (e *StringEqualsIgnoreCaseEvaluator) Evaluate(conditionValue interface{}, contextValues []string) bool { + expectedValues := getCachedNormalizedValues(conditionValue) + for _, expected := range expectedValues { + for _, contextValue := range contextValues { + if strings.EqualFold(expected, contextValue) { + return true + } + } + } + return false +} + +// StringNotEqualsIgnoreCaseEvaluator evaluates StringNotEqualsIgnoreCase conditions +type StringNotEqualsIgnoreCaseEvaluator struct{} + +func (e *StringNotEqualsIgnoreCaseEvaluator) Evaluate(conditionValue interface{}, contextValues []string) bool { + expectedValues := getCachedNormalizedValues(conditionValue) + for _, expected := range expectedValues { + for _, contextValue := range contextValues { + if strings.EqualFold(expected, contextValue) { + return false + } + } + } + return true +} + // NumericEqualsEvaluator evaluates NumericEquals conditions type NumericEqualsEvaluator struct{} @@ -650,6 +680,10 @@ func GetConditionEvaluator(operator string) (ConditionEvaluator, error) { return &StringLikeEvaluator{}, nil case "StringNotLike": return &StringNotLikeEvaluator{}, nil + case "StringEqualsIgnoreCase": + return &StringEqualsIgnoreCaseEvaluator{}, nil + case "StringNotEqualsIgnoreCase": + return &StringNotEqualsIgnoreCaseEvaluator{}, nil case "NumericEquals": return &NumericEqualsEvaluator{}, nil case "NumericNotEquals": @@ -730,7 +764,7 @@ func EvaluateConditions(conditions PolicyConditions, contextValues map[string][] conditionEvaluator, err := GetConditionEvaluator(operator) if err != nil { glog.Warningf("Unsupported condition operator: %s", operator) - continue + return false } for key, value := range conditionMap { diff --git a/weed/s3api/policy_engine/conditions_failclosed_test.go b/weed/s3api/policy_engine/conditions_failclosed_test.go new file mode 100644 index 000000000..f4b0a531c --- /dev/null +++ b/weed/s3api/policy_engine/conditions_failclosed_test.go @@ -0,0 +1,18 @@ +package policy_engine + +import ( + "testing" +) + +func TestEvaluateConditionsFailsClosedOnUnsupportedOperator(t *testing.T) { + conditions := PolicyConditions{ + "StringEqualsBogus": { + "aws:username": NewStringOrStringSlice("alice"), + }, + } + contextValues := map[string][]string{} + got := EvaluateConditions(conditions, contextValues, nil, nil) + if got { + t.Fatalf("EvaluateConditions returned true for unsupported operator; expected false (fail closed)") + } +} diff --git a/weed/s3api/policy_engine/conditions_ignorecase_test.go b/weed/s3api/policy_engine/conditions_ignorecase_test.go new file mode 100644 index 000000000..50ce78a48 --- /dev/null +++ b/weed/s3api/policy_engine/conditions_ignorecase_test.go @@ -0,0 +1,41 @@ +package policy_engine + +import ( + "testing" +) + +func TestConditionEvaluatorsIgnoreCase(t *testing.T) { + tests := []struct { + name string + operator string + conditionValue interface{} + contextValues []string + expected bool + }{ + {"StringEqualsIgnoreCase - match", "StringEqualsIgnoreCase", "ALICE", []string{"alice"}, true}, + {"StringEqualsIgnoreCase - no match", "StringEqualsIgnoreCase", "alice", []string{"bob"}, false}, + {"StringNotEqualsIgnoreCase - match", "StringNotEqualsIgnoreCase", "alice", []string{"bob"}, true}, + {"StringNotEqualsIgnoreCase - no match", "StringNotEqualsIgnoreCase", "ALICE", []string{"alice"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evaluator, err := GetConditionEvaluator(tt.operator) + if err != nil { + t.Fatalf("Failed to get condition evaluator: %v", err) + } + result := evaluator.Evaluate(tt.conditionValue, tt.contextValues) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} + +func TestGetConditionEvaluatorRejectsNonAWSIgnoreCaseOperators(t *testing.T) { + for _, op := range []string{"StringLikeIgnoreCase", "StringNotLikeIgnoreCase"} { + if _, err := GetConditionEvaluator(op); err == nil { + t.Fatalf("GetConditionEvaluator accepted non-AWS operator %q; expected error", op) + } + } +} diff --git a/weed/s3api/policy_engine/conditions_validation_test.go b/weed/s3api/policy_engine/conditions_validation_test.go new file mode 100644 index 000000000..ec61feb76 --- /dev/null +++ b/weed/s3api/policy_engine/conditions_validation_test.go @@ -0,0 +1,74 @@ +package policy_engine + +import ( + "testing" +) + +func TestValidateBucketPolicyRejectsUnsupportedConditionOperator(t *testing.T) { + unsupported := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": {"StringEqualsBogus": {"aws:username": "alice"}} + }] + }` + supportedIgnoreCase := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": {"StringEqualsIgnoreCase": {"aws:username": "alice"}} + }] + }` + supportedStringEquals := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": "*", + "Action": "s3:GetObject", + "Resource": "arn:aws:s3:::test-bucket/*", + "Condition": {"StringEquals": {"aws:username": "alice"}} + }] + }` + + t.Run("upload rejects unsupported operator", func(t *testing.T) { + policy, err := ParsePolicy(unsupported) + if err != nil { + t.Fatalf("ParsePolicy failed: %v", err) + } + if err := ValidateBucketPolicy(policy, "test-bucket"); err == nil { + t.Fatalf("ValidateBucketPolicy expected error for unsupported operator, got nil") + } + }) + + t.Run("upload accepts supported IgnoreCase operator", func(t *testing.T) { + policy, err := ParsePolicy(supportedIgnoreCase) + if err != nil { + t.Fatalf("ParsePolicy failed: %v", err) + } + if err := ValidateBucketPolicy(policy, "test-bucket"); err != nil { + t.Fatalf("ValidateBucketPolicy unexpected error: %v", err) + } + }) + + t.Run("upload accepts supported StringEquals operator", func(t *testing.T) { + policy, err := ParsePolicy(supportedStringEquals) + if err != nil { + t.Fatalf("ParsePolicy failed: %v", err) + } + if err := ValidateBucketPolicy(policy, "test-bucket"); err != nil { + t.Fatalf("ValidateBucketPolicy unexpected error: %v", err) + } + }) + + t.Run("load tolerates legacy unsupported operator", func(t *testing.T) { + if _, err := ParsePolicy(unsupported); err != nil { + t.Fatalf("ParsePolicy must not reject legacy unsupported operator at load time: %v", err) + } + }) +}