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.
This commit is contained in:
Chris Lu
2026-09-11 22:17:11 -07:00
committed by GitHub
parent 42b0ca7850
commit 79994b69af
5 changed files with 174 additions and 1 deletions
@@ -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
+35 -1
View File
@@ -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 {
@@ -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)")
}
}
@@ -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)
}
}
}
@@ -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)
}
})
}