diff --git a/test/s3tables/table-buckets/s3tables_integration_test.go b/test/s3tables/table-buckets/s3tables_integration_test.go index 70d7979df1..2bc8105047 100644 --- a/test/s3tables/table-buckets/s3tables_integration_test.go +++ b/test/s3tables/table-buckets/s3tables_integration_test.go @@ -85,6 +85,10 @@ func TestS3TablesIntegration(t *testing.T) { testTablePolicy(t, client) }) + t.Run("PolicyConditionValidation", func(t *testing.T) { + testPolicyConditionValidation(t, client) + }) + t.Run("Tagging", func(t *testing.T) { testTagging(t, client) }) @@ -425,6 +429,46 @@ func testTablePolicy(t *testing.T, client *S3TablesClient) { t.Logf("Verified policy deletion") } +// A null condition value is refused with InvalidRequest instead of being +// stored and later panicking the evaluator, and the bucket stays reachable. +// A well-formed conditional policy is still stored as written. +func testPolicyConditionValidation(t *testing.T, client *S3TablesClient) { + bucketName := "test-policy-cond-" + randomString(8) + createBucketResp, err := client.CreateTableBucket(bucketName, nil) + require.NoError(t, err, "Failed to create table bucket") + defer client.DeleteTableBucket(createBucketResp.ARN) + bucketARN := createBucketResp.ARN + + nullCondition := `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":null}}}]}` + err = client.PutTableBucketPolicy(bucketARN, nullCondition) + require.Error(t, err, "a null condition value must be rejected") + assert.Contains(t, err.Error(), s3tables.ErrCodeInvalidRequest) + + _, err = client.GetTableBucket(bucketARN) + require.NoError(t, err, "the bucket must stay reachable after the rejected policy") + _, err = client.GetTableBucketPolicy(bucketARN) + assert.Error(t, err, "the rejected policy must not be stored") + + validCondition := `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:DeleteTableBucket","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}]}` + require.NoError(t, client.PutTableBucketPolicy(bucketARN, validCondition), "a valid conditional policy must be accepted") + getPolicyResp, err := client.GetTableBucketPolicy(bucketARN) + require.NoError(t, err) + assert.Equal(t, validCondition, getPolicyResp.ResourcePolicy) + + // A null Condition reads as none, so this Allow would grant everyone. + allowNull := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":null}]}` + err = client.PutTableBucketPolicy(bucketARN, allowNull) + require.Error(t, err, "an Allow with a null Condition must be rejected") + assert.Contains(t, err.Error(), s3tables.ErrCodeInvalidRequest) + getPolicyResp, err = client.GetTableBucketPolicy(bucketARN) + require.NoError(t, err) + assert.Equal(t, validCondition, getPolicyResp.ResourcePolicy, "the stored policy must be unchanged") + + _, err = client.GetTableBucket(bucketARN) + require.NoError(t, err, "the bucket must stay reachable under a valid conditional policy") + require.NoError(t, client.DeleteTableBucketPolicy(bucketARN)) +} + func testTagging(t *testing.T, client *S3TablesClient) { bucketName := "test-tag-bucket-" + randomString(8) diff --git a/weed/s3api/policy_engine/conditions.go b/weed/s3api/policy_engine/conditions.go index bfb7bf0b94..883a3490ec 100644 --- a/weed/s3api/policy_engine/conditions.go +++ b/weed/s3api/policy_engine/conditions.go @@ -130,6 +130,9 @@ var normalizedValueCache = NewNormalizedValueCache(1000) // getCachedNormalizedValues returns cached normalized values or caches new ones func getCachedNormalizedValues(value interface{}) []string { + if value == nil { + return nil // no expected values; never reach reflect.TypeOf(nil) + } // Create a string key for caching - more efficient than fmt.Sprintf typeStr := reflect.TypeOf(value).String() cacheKey := typeStr + ":" + fmt.Sprint(value) diff --git a/weed/s3api/policy_engine/conditions_failclosed_test.go b/weed/s3api/policy_engine/conditions_failclosed_test.go index f4b0a531c9..41f0d976ca 100644 --- a/weed/s3api/policy_engine/conditions_failclosed_test.go +++ b/weed/s3api/policy_engine/conditions_failclosed_test.go @@ -16,3 +16,35 @@ func TestEvaluateConditionsFailsClosedOnUnsupportedOperator(t *testing.T) { t.Fatalf("EvaluateConditions returned true for unsupported operator; expected false (fail closed)") } } + +// A policy parsed into interface{} can hand an evaluator a null condition +// value; no operator, set-qualified or not, may panic on it. +func TestConditionEvaluatorsTolerateNilValue(t *testing.T) { + operators := []string{ + "StringEquals", "StringNotEquals", "StringLike", "StringNotLike", + "StringEqualsIgnoreCase", "StringNotEqualsIgnoreCase", + "NumericEquals", "NumericNotEquals", "NumericLessThan", "NumericLessThanEquals", + "NumericGreaterThan", "NumericGreaterThanEquals", + "DateEquals", "DateNotEquals", "DateLessThan", "DateLessThanEquals", + "DateGreaterThan", "DateGreaterThanEquals", + "Bool", "IpAddress", "NotIpAddress", "ArnEquals", "ArnLike", "Null", + } + for _, base := range operators { + for _, operator := range []string{base, "ForAnyValue:" + base, "ForAllValues:" + base} { + evaluator, err := GetConditionEvaluator(operator) + if err != nil { + t.Fatalf("%s: %v", operator, err) + } + for _, contextValues := range [][]string{nil, {"x"}} { + func() { + defer func() { + if r := recover(); r != nil { + t.Errorf("%s with a nil value and context %v panicked: %v", operator, contextValues, r) + } + }() + evaluator.Evaluate(nil, contextValues) + }() + } + } + } +} diff --git a/weed/s3api/s3tables/handler_policy_condition_test.go b/weed/s3api/s3tables/handler_policy_condition_test.go new file mode 100644 index 0000000000..739b9a0686 --- /dev/null +++ b/weed/s3api/s3tables/handler_policy_condition_test.go @@ -0,0 +1,149 @@ +package s3tables + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const nullConditionDeny = `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":null}}}]}` + +// startConditionManager seeds startPolicyDenyManager's bucket, namespace and +// table with no table policy and bucketPolicy, if any, stored on the bucket +// the way a release without write-time condition checks could have left it. +func startConditionManager(t *testing.T, bucketPolicy string) (*s3tablestest.MemFiler, *Manager) { + t.Helper() + fs, m := startPolicyDenyManager(t) + bucket := fs.Get(TablesPath, policyDenyBucket) + require.NotNil(t, bucket) + delete(bucket.Extended, ExtendedKeyPolicy) + if bucketPolicy != "" { + bucket.Extended[ExtendedKeyPolicy] = []byte(bucketPolicy) + } + table := fs.Get(GetNamespacePath(policyDenyBucket, "ns"), "t") + require.NotNil(t, table) + delete(table.Extended, ExtendedKeyPolicy) + return fs, m +} + +func requireS3TablesError(t *testing.T, err error, code string) *S3TablesError { + t.Helper() + var s3err *S3TablesError + require.ErrorAs(t, err, &s3err) + assert.Equal(t, code, s3err.Type, s3err.Message) + return s3err +} + +// A condition the evaluator cannot compare is refused at write time, on the +// bucket and on the table, and nothing is stored. +func TestPutPolicyRejectsUnusableCondition(t *testing.T) { + fs, m := startConditionManager(t, "") + bucketARN, ns, name := deniedTableReq(t) + + err := runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{ + TableBucketARN: bucketARN, ResourcePolicy: nullConditionDeny, + }, DefaultAccountID) + s3err := requireS3TablesError(t, err, ErrCodeInvalidRequest) + assert.Contains(t, s3err.Message, `"aws:ResourceTag/env"`) + _, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the bucket policy must not be written") + + err = runPolicyOp(t, m, fs, "PutTablePolicy", &PutTablePolicyRequest{ + TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, ResourcePolicy: nullConditionDeny, + }, DefaultAccountID) + s3err = requireS3TablesError(t, err, ErrCodeInvalidRequest) + assert.Contains(t, s3err.Message, `"aws:ResourceTag/env"`) + _, stored = fs.Get(GetNamespacePath(policyDenyBucket, ns), name).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the table policy must not be written") + + // A string the numeric evaluator would skip is refused like a null one. + unreadable := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"NumericLessThan":{"aws:RequestTag/priority":"low"}}}]}` + err = runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{ + TableBucketARN: bucketARN, ResourcePolicy: unreadable, + }, DefaultAccountID) + s3err = requireS3TablesError(t, err, ErrCodeInvalidRequest) + assert.Contains(t, s3err.Message, "NumericLessThan") + _, stored = fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the bucket policy must not be written") + + valid := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}]}` + require.NoError(t, runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{ + TableBucketARN: bucketARN, ResourcePolicy: valid, + }, DefaultAccountID)) + assert.Equal(t, valid, string(fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy])) +} + +// The typed decode reads a null Condition as none, so an Allow carrying one is +// refused on the bucket and on the table, and nothing is stored. +func TestPutPolicyRejectsNullAllowCondition(t *testing.T) { + fs, m := startConditionManager(t, "") + bucketARN, ns, name := deniedTableReq(t) + allowNull := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":null}]}` + + requireS3TablesError(t, runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{ + TableBucketARN: bucketARN, ResourcePolicy: allowNull, + }, DefaultAccountID), ErrCodeInvalidRequest) + _, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the bucket policy must not be written") + + requireS3TablesError(t, runPolicyOp(t, m, fs, "PutTablePolicy", &PutTablePolicyRequest{ + TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, ResourcePolicy: allowNull, + }, DefaultAccountID), ErrCodeInvalidRequest) + _, stored = fs.Get(GetNamespacePath(policyDenyBucket, ns), name).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the table policy must not be written") +} + +// A keyless operator stored before write-time checks made its Allow grant +// everyone; it is now unusable, so a non-owner is turned away. GetTableBucket +// hides a bucket the caller may not read, so the refusal is NoSuchBucket, +// while GetTableBucketPolicy answers AccessDenied. +func TestStoredKeylessAllowDoesNotGrant(t *testing.T) { + keylessAllow := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{}}}]}` + fs, m := startConditionManager(t, keylessAllow) + bucketARN := policyDenyBucketARN(t) + + requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{ + TableBucketARN: bucketARN, + }, "reader"), ErrCodeNoSuchBucket) + requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucketPolicy", &GetTableBucketPolicyRequest{ + TableBucketARN: bucketARN, + }, "reader"), ErrCodeAccessDenied) + require.NoError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{ + TableBucketARN: bucketARN, + }, DefaultAccountID), "the owner keeps access") +} + +// A bucket policy stored before conditions were validated must not cut the +// connection: the owner gets an authorization error, the admin can still +// remove the policy, and the owner is let back in once it is gone. +func TestStoredUnusableConditionPolicyIsRecoverable(t *testing.T) { + fs, m := startConditionManager(t, nullConditionDeny) + bucketARN, ns, name := deniedTableReq(t) + + requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{ + TableBucketARN: bucketARN, + }, DefaultAccountID), ErrCodeNoSuchBucket) + require.Error(t, runPolicyOp(t, m, fs, "GetTable", &GetTableRequest{ + TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, + }, DefaultAccountID), "the broken bucket Deny must veto table reads") + requireS3TablesError(t, runPolicyOp(t, m, fs, "DeleteTableBucketPolicy", &DeleteTableBucketPolicyRequest{ + TableBucketARN: bucketARN, + }, DefaultAccountID), ErrCodeAccessDenied) + assert.Equal(t, nullConditionDeny, string(fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy])) + + require.NoError(t, runPolicyOp(t, m, fs, "DeleteTableBucketPolicy", &DeleteTableBucketPolicyRequest{ + TableBucketARN: bucketARN, + }, s3_constants.AccountAdminId), "the admin must be able to remove the broken policy") + _, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy] + assert.False(t, stored, "the broken policy must be gone") + + require.NoError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{ + TableBucketARN: bucketARN, + }, DefaultAccountID)) + require.NoError(t, runPolicyOp(t, m, fs, "GetTable", &GetTableRequest{ + TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, + }, DefaultAccountID)) +} diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index 2184668776..efd8a32593 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -1,9 +1,14 @@ package s3tables import ( + "bytes" "encoding/json" "fmt" + "math" + "net" + "strconv" "strings" + "time" "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/iam/sts" @@ -26,7 +31,7 @@ type PolicyDocument struct { func (pd *PolicyDocument) UnmarshalJSON(data []byte) error { type Alias PolicyDocument aux := &struct { - Statement interface{} `json:"Statement"` + Statement json.RawMessage `json:"Statement"` *Alias }{ Alias: (*Alias)(pd), @@ -35,9 +40,16 @@ func (pd *PolicyDocument) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &aux); err != nil { return err } + var statement interface{} + if len(aux.Statement) > 0 { + if err := json.Unmarshal(aux.Statement, &statement); err != nil { + return err + } + } // Handle Statement as either a single object or array - switch s := aux.Statement.(type) { + raws := []json.RawMessage{aux.Statement} + switch s := statement.(type) { case map[string]interface{}: // Single statement object - unmarshal to one Statement stmtData, err := json.Marshal(s) @@ -58,13 +70,23 @@ func (pd *PolicyDocument) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(stmtData, &pd.Statement); err != nil { return fmt.Errorf("failed to unmarshal statement array: %w", err) } + if err := json.Unmarshal(aux.Statement, &raws); err != nil { + return err + } case nil: // No statements pd.Statement = []Statement{} default: - return fmt.Errorf("Statement must be an object or array, got %T", aux.Statement) + return fmt.Errorf("Statement must be an object or array, got %T", statement) } + // The re-marshal above keeps only the last of a repeated key, so the + // Condition keys are counted from the original bytes. + for i := range pd.Statement { + if i < len(raws) { + pd.Statement[i].nullCondition, pd.Statement[i].splitCondition = conditionKeys(raws[i]) + } + } return nil } @@ -74,6 +96,44 @@ type Statement struct { Action interface{} `json:"Action"` // Can be string or []string Resource interface{} `json:"Resource"` // Can be string or []string Condition map[string]map[string]interface{} `json:"Condition,omitempty"` + // nullCondition records a literal "Condition": null, which decodes the + // same as no Condition. + nullCondition bool + // splitCondition records more than one Condition key, in any spelling. + // Those objects decode into one map in which an operator they share keeps + // only the last one's keys. + splitCondition bool +} + +// conditionKeys walks a statement object's keys, which a map would collapse, +// and reports a null Condition and a Condition under more than one key. +func conditionKeys(data []byte) (null, split bool) { + dec := json.NewDecoder(bytes.NewReader(data)) + if tok, err := dec.Token(); err != nil || tok != json.Delim('{') { + return false, false + } + conditions := 0 + for dec.More() { + key, err := dec.Token() + if err != nil { + break + } + var raw json.RawMessage + if err := dec.Decode(&raw); err != nil { + break + } + if name, _ := key.(string); strings.EqualFold(name, "Condition") { + conditions++ + null = null || string(raw) == "null" + } + } + return null, conditions > 1 +} + +// SplitCondition reports a statement with more than one Condition key, which +// the evaluator never grants on and always vetoes with. +func (s *Statement) SplitCondition() bool { + return s.splitCondition } type PolicyContext struct { @@ -142,15 +202,42 @@ func CheckTableOrBucketPermission(operation, principal, resourceOwner, resourceP } // validatePolicyDocument rejects a policy the evaluator could not honor: one -// that does not parse, or whose actions and resources name the vector service. -// Either stored here would be a dead rule the writer believes is enforced — -// vector requests authorize their s3tables action on a s3tables ARN. +// that does not parse, whose actions and resources name the vector service, +// whose conditions the evaluator cannot compare, or whose Allow carries a null +// Condition. Any of them stored here would be a rule the writer believes is +// enforced — vector requests authorize their s3tables action on a s3tables +// ARN, an unusable condition is skipped on an Allow and vetoes everything on a +// Deny, and a null Condition reads as none, so the Allow grants unconditionally. func validatePolicyDocument(raw string) error { var doc PolicyDocument if err := json.Unmarshal([]byte(raw), &doc); err != nil { return fmt.Errorf("invalid policy document: %w", err) } for _, stmt := range doc.Statement { + // A null Condition on a Deny only widens the veto, so it stays allowed. + if stmt.Effect == "Allow" && stmt.nullCondition { + return fmt.Errorf("invalid policy document: an Allow statement's Condition must not be null") + } + if stmt.splitCondition { + return fmt.Errorf("invalid policy document: a statement's Condition must be under one key, spelled Condition") + } + for operator, keys := range stmt.Condition { + if _, err := policy_engine.GetConditionEvaluator(operator); err != nil { + return fmt.Errorf("invalid policy document: condition operator %q is not supported", operator) + } + // An operator with no keys holds for every request. + if len(keys) == 0 { + return fmt.Errorf("invalid policy document: condition %s needs at least one key", operator) + } + for key, value := range keys { + if !tableConditionKey(key) { + return fmt.Errorf("invalid policy document: condition key %q is not supported in a table policy", key) + } + if !ConditionValue(operator, value) { + return fmt.Errorf("invalid policy document: condition %s on %q needs values %s can read (a number for Numeric*, an RFC 3339 time for Date*, true or false for Bool and Null, an IP or CIDR for IpAddress)", operator, key, operator) + } + } + } for _, action := range statementActions(stmt.Action) { if strings.HasPrefix(strings.ToLower(action), "s3vectors:") { return fmt.Errorf("action %q is not enforceable in a resource policy", action) @@ -165,6 +252,73 @@ func validatePolicyDocument(raw string) error { return nil } +// ConditionValue reports whether operator's evaluator can read v: a JSON +// type it can compare, and a value it parses rather than skips. A null +// value would panic it, an empty list never matches, and an unreadable +// value is skipped — dropping a Deny or matching a negated Allow. +func ConditionValue(operator string, v interface{}) bool { + list, ok := v.([]interface{}) + if !ok { + list = []interface{}{v} + } + base, qualified := strings.CutPrefix(operator, "ForAllValues:") + if !qualified { + base, _ = strings.CutPrefix(operator, "ForAnyValue:") + } + for _, item := range list { + switch item.(type) { + case string, float64, bool: + default: + return false + } + if !conditionValueReadable(base, item) { + return false + } + } + return len(list) > 0 +} + +// conditionValueReadable reports whether the evaluator for base parses item +// the way it reads it. An operator without a case here is refused, so one +// added to the policy engine cannot be accepted before it is checked. +func conditionValueReadable(base string, item interface{}) bool { + switch base { + case "StringEquals", "StringNotEquals", "StringLike", "StringNotLike", + "StringEqualsIgnoreCase", "StringNotEqualsIgnoreCase", "ArnEquals", "ArnLike": + return true + case "NumericEquals", "NumericNotEquals", "NumericLessThan", "NumericLessThanEquals", + "NumericGreaterThan", "NumericGreaterThanEquals": + switch v := item.(type) { + case float64: + return true + case string: + f, err := strconv.ParseFloat(v, 64) + return err == nil && !math.IsNaN(f) && !math.IsInf(f, 0) + } + case "DateEquals", "DateNotEquals", "DateLessThan", "DateLessThanEquals", + "DateGreaterThan", "DateGreaterThanEquals": + if v, ok := item.(string); ok { + _, err := time.Parse(time.RFC3339, v) + return err == nil + } + case "Bool", "Null": + switch v := item.(type) { + case bool: + return true + case string: + return strings.EqualFold(v, "true") || strings.EqualFold(v, "false") + } + case "IpAddress", "NotIpAddress": + if v, ok := item.(string); ok { + if _, _, err := net.ParseCIDR(v); err == nil { + return true + } + return net.ParseIP(v) != nil + } + } + return false +} + // arnService returns the service field of an ARN in any partition, so a // resource naming s3vectors is caught for aws-cn, aws-us-gov, and friends — // not just the commercial partition. @@ -210,7 +364,9 @@ func adminCaller(operation string, ctx *PolicyContext) bool { // matchesDenyStatement reports whether the JSON policy carries a statement // matching this request with Effect Deny. Malformed policies return false; -// the caller's normal evaluation still denies them. +// the caller's normal evaluation still denies them. A Deny whose condition +// cannot be evaluated applies: it was written to protect the resource, and +// dropping it would widen access. func matchesDenyStatement(operation, principal, resourcePolicy, resourceARN string, ctx *PolicyContext) bool { if resourcePolicy == "" { return false @@ -227,11 +383,17 @@ func matchesDenyStatement(operation, principal, resourcePolicy, resourceARN stri if stmt.Effect != "Deny" || !matchesDenyCaller(stmt.Principal, principal, ctx) || !matchesAction(stmt.Action, fullAction) || - (resourceARN != "" && !matchesResource(stmt.Resource, resourceARN)) || - !matchesConditions(stmt.Condition, ctx) { + (resourceARN != "" && !matchesResource(stmt.Resource, resourceARN)) { continue } - return true + matched, usable := matchesConditions(stmt.Condition, ctx) + if !usable || stmt.splitCondition { + glog.Warningf("S3Tables: policy on %q has a Deny condition that cannot be evaluated; denying %s for %s until the policy is replaced", resourceARN, fullAction, principal) + return true + } + if matched { + return true + } } return false } @@ -294,7 +456,12 @@ func checkPermission(operation, principal, owner, resourcePolicy, resourceARN st continue } - if !matchesConditions(stmt.Condition, ctx) { + // A condition that cannot be evaluated never grants, nor does a null + // or split one stored before validatePolicyDocument refused it. + if stmt.Effect == "Allow" && (stmt.nullCondition || stmt.splitCondition) { + continue + } + if matched, usable := matchesConditions(stmt.Condition, ctx); !usable || !matched { continue } @@ -527,34 +694,67 @@ func matchesActionPattern(pattern, action string) bool { return wildcard.MatchesWildcard(pattern, action) } -func matchesConditions(conditions map[string]map[string]interface{}, ctx *PolicyContext) bool { +// matchesConditions reports whether every condition holds for ctx, and +// whether the block can be evaluated at all: one unsupported operator, keyless +// operator or uncomparable value leaves it unusable, and the caller decides +// what that means for the statement's Effect. Every operator is checked +// before answering, so the verdict does not depend on map iteration order. +func matchesConditions(conditions map[string]map[string]interface{}, ctx *PolicyContext) (matched, usable bool) { if len(conditions) == 0 { - return true - } - if ctx == nil { - return false + return true, true } + matched = true for operator, conditionValues := range conditions { - if !matchesConditionOperator(operator, conditionValues, ctx) { - return false + ok, valid := matchesConditionOperator(operator, conditionValues, ctx) + if !valid { + return false, false } + matched = matched && ok } - return true + return matched, true } -func matchesConditionOperator(operator string, conditionValues map[string]interface{}, ctx *PolicyContext) bool { +func matchesConditionOperator(operator string, conditionValues map[string]interface{}, ctx *PolicyContext) (matched, usable bool) { evaluator, err := policy_engine.GetConditionEvaluator(operator) if err != nil { - return false + return false, false + } + // With no keys the loop below would hold for every request; a stored + // keyless operator is unusable, so it never lets an Allow grant. + if len(conditionValues) == 0 { + return false, false + } + for _, value := range conditionValues { + if !ConditionValue(operator, value) { + return false, false + } + } + if ctx == nil { + return false, true } for key, value := range conditionValues { contextVals := getConditionContextValues(key, ctx) if !evaluator.Evaluate(value, contextVals) { - return false + return false, true } } - return true + return true, true +} + +// tableConditionKey reports whether getConditionContextValues supplies key. +func tableConditionKey(key string) bool { + switch key { + case "s3tables:namespace", "s3tables:tableName", "s3tables:tableBucketName", + "s3tables:SSEAlgorithm", "s3tables:KMSKeyArn", "s3tables:StorageClass", "aws:TagKeys": + return true + } + for _, prefix := range []string{"aws:RequestTag/", "aws:ResourceTag/", "s3tables:TableBucketTag/", "s3vectors:VectorBucketTag/"} { + if tagKey, ok := strings.CutPrefix(key, prefix); ok { + return tagKey != "" + } + } + return false } func getConditionContextValues(key string, ctx *PolicyContext) []string { diff --git a/weed/s3api/s3tables/permissions_test.go b/weed/s3api/s3tables/permissions_test.go index 037cdbb2ce..f3c583ecd8 100644 --- a/weed/s3api/s3tables/permissions_test.go +++ b/weed/s3api/s3tables/permissions_test.go @@ -2,9 +2,17 @@ package s3tables import ( "encoding/json" + "fmt" + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "strconv" + "strings" "testing" "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" ) func TestMatchesActionPattern(t *testing.T) { @@ -127,6 +135,9 @@ func TestEvaluatePolicyWithConditions(t *testing.T) { } policyBytes, _ := json.Marshal(policy) policyStr := string(policyBytes) + if err := validatePolicyDocument(policyStr); err != nil { + t.Fatalf("conditional policy rejected by the validator: %v", err) + } tests := []struct { name string @@ -492,6 +503,391 @@ func TestValidatePolicyDocument(t *testing.T) { if err := validatePolicyDocument(deadMixed); err == nil { t.Fatal("resource list containing s3vectors should be rejected") } + + withCondition := func(condition string) string { + return `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":` + condition + `}]}` + } + for _, tc := range []struct { + condition string + mentions []string // the error must name these; empty means accepted + }{ + {`{"StringEquals":{"aws:ResourceTag/env":"prod"}}`, nil}, + {`{"StringEquals":{"aws:ResourceTag/env":["a","b"]}}`, nil}, + {`{"NumericLessThan":{"aws:RequestTag/priority":10}}`, nil}, + {`{"Bool":{"aws:ResourceTag/is_public":true}}`, nil}, + {`{"ForAnyValue:StringEquals":{"aws:TagKeys":["team","env"]}}`, nil}, + {`{"Null":{"aws:ResourceTag/owner":"true"}}`, nil}, + {`{"StringEquals":{"s3tables:namespace":"default"}}`, nil}, + {`{"StringEquals":{"aws:ResourceTag/env":null}}`, []string{"StringEquals", `"aws:ResourceTag/env"`}}, + {`{"StringEquals":{"aws:ResourceTag/env":[null]}}`, []string{"StringEquals", `"aws:ResourceTag/env"`}}, + {`{"StringNotEquals":{"aws:ResourceTag/env":[]}}`, []string{"StringNotEquals", `"aws:ResourceTag/env"`}}, + {`{"StringEquals":{"aws:ResourceTag/env":{}}}`, []string{"StringEquals", `"aws:ResourceTag/env"`}}, + {`{"StringEquals":{"aws:ResourceTag/env":{"a":1}}}`, []string{"StringEquals", `"aws:ResourceTag/env"`}}, + {`{"StringEquals":{"aws:ResourceTag/env":[["a"]]}}`, []string{"StringEquals", `"aws:ResourceTag/env"`}}, + {`{"StringEquals":null}`, []string{"StringEquals", "at least one key"}}, + {`{"StringEquals":{}}`, []string{"StringEquals", "at least one key"}}, + {`{"StringEqualsBogus":{"aws:ResourceTag/env":"prod"}}`, []string{`"StringEqualsBogus"`, "not supported"}}, + {`{"NumericLessThan":{"aws:RequestTag/priority":"10"}}`, nil}, + {`{"DateLessThan":{"aws:ResourceTag/created":"2026-01-01T00:00:00Z"}}`, nil}, + {`{"Bool":{"aws:ResourceTag/is_public":"TRUE"}}`, nil}, + {`{"IpAddress":{"aws:ResourceTag/net":"10.0.0.0/8"}}`, nil}, + {`{"StringEquals":{"aws:ResourceTag/env":10}}`, nil}, + {`{"NumericLessThan":{"aws:RequestTag/priority":"low"}}`, []string{"NumericLessThan", `"aws:RequestTag/priority"`}}, + {`{"NumericLessThan":{"aws:RequestTag/priority":true}}`, []string{"NumericLessThan", `"aws:RequestTag/priority"`}}, + {`{"NumericLessThan":{"aws:RequestTag/priority":["low","10"]}}`, []string{"NumericLessThan", `"aws:RequestTag/priority"`}}, + {`{"ForAnyValue:NumericLessThan":{"aws:RequestTag/priority":"low"}}`, []string{"ForAnyValue:NumericLessThan", `"aws:RequestTag/priority"`}}, + {`{"DateLessThan":{"aws:ResourceTag/created":"2026-01-01"}}`, []string{"DateLessThan", `"aws:ResourceTag/created"`}}, + {`{"Bool":{"aws:ResourceTag/is_public":"yes"}}`, []string{"Bool", `"aws:ResourceTag/is_public"`}}, + {`{"Null":{"aws:ResourceTag/owner":"maybe"}}`, []string{"Null", `"aws:ResourceTag/owner"`}}, + {`{"IpAddress":{"aws:ResourceTag/net":"not-an-ip"}}`, []string{"IpAddress", `"aws:ResourceTag/net"`}}, + {`{"StringEquals":{"s3tables:tableName":"t","s3tables:tableBucketName":"b","s3tables:StorageClass":"STANDARD"}}`, nil}, + {`{"StringEquals":{"s3tables:SSEAlgorithm":"AES256","s3tables:KMSKeyArn":"arn:k","s3tables:TableBucketTag/env":"prod"}}`, nil}, + {`{"IpAddress":{"aws:SourceIp":"10.0.0.0/8"}}`, []string{`"aws:SourceIp"`, "not supported"}}, + {`{"StringEquals":{"aws:resourcetag/env":"prod"}}`, []string{`"aws:resourcetag/env"`, "not supported"}}, + {`{"StringEquals":{"aws:ResourceTag/":"prod"}}`, []string{`"aws:ResourceTag/"`, "not supported"}}, + {`{"Null":{"aws:PrincipalArn":"true"}}`, []string{`"aws:PrincipalArn"`, "not supported"}}, + } { + err := validatePolicyDocument(withCondition(tc.condition)) + if len(tc.mentions) == 0 { + if err != nil { + t.Errorf("%s rejected: %v", tc.condition, err) + } + continue + } + if err == nil { + t.Errorf("%s accepted", tc.condition) + continue + } + for _, want := range tc.mentions { + if !strings.Contains(err.Error(), want) { + t.Errorf("%s: error %q does not mention %s", tc.condition, err, want) + } + } + } + + // The typed decode reads a null Condition as none, so an Allow carrying + // one would grant unconditionally; a Deny carrying one only vetoes more. + const ( + allowNull = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null}` + allowNullLower = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","condition":null}` + allowValid = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}` + allowNone = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*"}` + denyNull = `{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null}` + denyNone = `{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*"}` + // Duplicate keys reach the evaluator in sorted order, not document + // order: the lower-case null wins here, the object wins in the mirror, + // and both are refused whichever spelling is null. + allowNullWins = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","condition":null,"Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}` + allowObjectWins = `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null,"condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}` + ) + for _, tc := range []struct { + statement string + rejected bool + }{ + {`[` + allowNull + `]`, true}, + {`[` + allowNullLower + `]`, true}, + {allowNull, true}, + {`[` + denyNone + `,` + allowNull + `]`, true}, + {`[` + allowNullWins + `]`, true}, + {`[` + denyNull + `]`, false}, + {`[` + denyNull + `,` + allowValid + `]`, false}, + {`[` + allowNone + `]`, false}, + {`[` + allowObjectWins + `]`, true}, + } { + raw := `{"Version":"2012-10-17","Statement":` + tc.statement + `}` + err := validatePolicyDocument(raw) + switch { + case tc.rejected && err == nil: + t.Errorf("%s accepted", tc.statement) + case tc.rejected && !strings.Contains(err.Error(), "Condition must not be null"): + t.Errorf("%s: unexpected error %v", tc.statement, err) + case !tc.rejected && err != nil: + t.Errorf("%s rejected: %v", tc.statement, err) + } + } +} + +func TestConditionValue(t *testing.T) { + for _, tc := range []struct { + operator string + raw string + want bool + }{ + {"StringEquals", `"prod"`, true}, + {"StringEquals", `["a","b"]`, true}, + {"StringEquals", `10`, true}, + {"StringEquals", `true`, true}, + {"StringEquals", `null`, false}, + {"StringEquals", `[null]`, false}, + {"StringEquals", `["a",null]`, false}, + {"StringEquals", `[]`, false}, + {"StringEquals", `{}`, false}, + {"StringEquals", `{"a":1}`, false}, + {"StringEquals", `[["a"]]`, false}, + + {"NumericLessThan", `10`, true}, + {"NumericLessThan", `"10"`, true}, + {"NumericLessThan", `"low"`, false}, + {"NumericLessThan", `true`, false}, + {"NumericLessThan", `["low","10"]`, false}, + {"NumericLessThan", `"NaN"`, false}, + {"NumericNotEquals", `"nan"`, false}, + {"NumericNotEquals", `"Inf"`, false}, + {"NumericGreaterThan", `"-Infinity"`, false}, + {"ForAnyValue:NumericLessThan", `"low"`, false}, + {"ForAllValues:NumericLessThan", `"10"`, true}, + + {"DateLessThan", `"2026-01-01T00:00:00Z"`, true}, + {"DateLessThan", `"2026-01-01"`, false}, + + {"Bool", `true`, true}, + {"Bool", `"TRUE"`, true}, + {"Bool", `"yes"`, false}, + {"Null", `"false"`, true}, + {"Null", `"maybe"`, false}, + + {"IpAddress", `"10.0.0.0/8"`, true}, + {"IpAddress", `"10.0.0.1"`, true}, + {"IpAddress", `"not-an-ip"`, false}, + + {"StringEqualsBogus", `"prod"`, false}, + } { + var v interface{} + if err := json.Unmarshal([]byte(tc.raw), &v); err != nil { + t.Fatalf("%s: %v", tc.raw, err) + } + if got := ConditionValue(tc.operator, v); got != tc.want { + t.Errorf("ConditionValue(%s, %s) = %v, want %v", tc.operator, tc.raw, got, tc.want) + } + } +} + +// Every operator the policy engine evaluates must have its own case in +// ConditionValue: one that falls through to the default refuses every value, +// so a newly added operator turns this test red instead of being accepted +// unchecked or refused unnoticed. +func TestConditionValueCoversEveryOperator(t *testing.T) { + file, err := parser.ParseFile(token.NewFileSet(), filepath.Join("..", "policy_engine", "conditions.go"), nil, 0) + if err != nil { + t.Fatal(err) + } + var operators []string + ast.Inspect(file, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Name.Name != "baseConditionEvaluator" { + return true + } + ast.Inspect(fn.Body, func(n ast.Node) bool { + if clause, ok := n.(*ast.CaseClause); ok { + for _, expr := range clause.List { + if lit, ok := expr.(*ast.BasicLit); ok && lit.Kind == token.STRING { + operator, _ := strconv.Unquote(lit.Value) + operators = append(operators, operator) + } + } + } + return true + }) + return false + }) + if len(operators) < 20 { + t.Fatalf("found only %d operators in baseConditionEvaluator", len(operators)) // positive control + } + samples := []interface{}{"x", "10", "2026-01-01T00:00:00Z", "true", "10.0.0.0/8"} + for _, operator := range operators { + if _, err := policy_engine.GetConditionEvaluator(operator); err != nil { + t.Fatalf("%s: %v", operator, err) + } + readable := false + for _, sample := range samples { + readable = readable || ConditionValue(operator, sample) + } + if !readable { + t.Errorf("ConditionValue has no case for %s", operator) + } + } +} + +// A value the operator cannot read is skipped by its evaluator, so before +// write-time checks such a Deny never matched and the owner kept access. A +// stored one is now unusable: the Deny applies, and the admin can still +// remove the policy. +func TestUnreadableConditionDenyApplies(t *testing.T) { + arn := "arn:aws:s3tables:us-east-1:acct:bucket/b" + ctx := &PolicyContext{ + RequestTags: map[string]string{"priority": "5"}, + ResourceTags: map[string]string{"is_public": "true", "created": "2025-06-01T00:00:00Z"}, + } + admin := &PolicyContext{IdentityActions: []string{"Admin"}, RequestTags: ctx.RequestTags, ResourceTags: ctx.ResourceTags} + for _, condition := range []string{ + `{"NumericLessThan":{"aws:RequestTag/priority":"low"}}`, + `{"DateLessThan":{"aws:ResourceTag/created":"2026-01-01"}}`, + `{"Bool":{"aws:ResourceTag/is_public":"yes"}}`, + `{"NumericLessThan":{"aws:RequestTag/priority":"NaN"}}`, + } { + var conditions map[string]map[string]interface{} + if err := json.Unmarshal([]byte(condition), &conditions); err != nil { + t.Fatalf("%s: %v", condition, err) + } + if _, usable := matchesConditions(conditions, ctx); usable { + t.Errorf("%s reported usable", condition) + } + deny := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":` + condition + `}]}` + if checkPermission("s3tables:GetTableBucket", "owner", "owner", deny, arn, ctx) { + t.Errorf("%s: the Deny must apply to the owner", condition) + } + if !CheckPermissionWithContext("s3tables:DeleteTableBucketPolicy", "admin", "owner", deny, arn, admin) { + t.Errorf("%s: the admin must keep the bypass", condition) + } + } +} + +// A policy stored before conditions were validated may still carry a null +// value. Evaluating it must not panic: the Deny it sits on applies to the +// owner too, while the admin keeps the bypass needed to remove the policy. +func TestNullConditionDoesNotPanic(t *testing.T) { + arn := "arn:aws:s3tables:us-east-1:acct:bucket/b" + denyNull := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":null}}}]}` + ctx := &PolicyContext{ResourceTags: map[string]string{"env": "prod"}} + + if checkPermission("s3tables:GetTableBucket", "owner", "owner", denyNull, arn, ctx) { + t.Error("a Deny whose condition cannot be evaluated must still deny the owner") + } + if CheckTableOrBucketPermission("s3tables:GetTable", "owner", "owner", "", arn+"/table/ns/t", "owner", denyNull, arn, ctx) { + t.Error("a broken bucket Deny must veto the table request too") + } + admin := &PolicyContext{IdentityActions: []string{"Admin"}, ResourceTags: ctx.ResourceTags} + if !CheckPermissionWithContext("s3tables:DeleteTableBucketPolicy", "admin", "owner", denyNull, arn, admin) { + t.Error("the admin must keep the bypass to remove a broken policy") + } +} + +// An Allow whose condition cannot be evaluated grants nothing. A [null] value +// used to normalize to "", so StringNotEquals matched every request. +func TestUnusableConditionAllowNeverGrants(t *testing.T) { + ctx := &PolicyContext{ResourceTags: map[string]string{"env": "prod"}} + for _, condition := range []string{ + `{"StringNotEquals":{"aws:ResourceTag/env":[null]}}`, + `{"StringNotEquals":{"aws:ResourceTag/env":[]}}`, + `{"StringNotEquals":{"aws:ResourceTag/env":{"a":1}}}`, + `{"StringEquals":{"aws:ResourceTag/env":null}}`, + `{"StringEqualsBogus":{"aws:ResourceTag/env":"prod"}}`, + `{"NumericNotEquals":{"aws:ResourceTag/env":"low"}}`, + `{"NotIpAddress":{"aws:ResourceTag/env":"not-an-ip"}}`, + `{"NumericNotEquals":{"aws:ResourceTag/env":"NaN"}}`, + `{"StringEquals":{}}`, + `{"StringEquals":null}`, + } { + allow := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":` + condition + `}]}` + if CheckPermissionWithContext("s3tables:GetTable", "reader", "owner", allow, "", ctx) { + t.Errorf("%s granted a non-owner", condition) + } + } + // A keyless operator is unusable whether or not there is a context. + if _, usable := matchesConditions(map[string]map[string]interface{}{"StringEquals": {}}, nil); usable { + t.Error("a keyless operator without a context reported usable") + } +} + +// A stored Allow whose Condition is null never grants; a Deny with one still +// applies. +func TestStoredNullConditionAllowNeverGrants(t *testing.T) { + ctx := &PolicyContext{} + for _, tc := range []struct{ action, policy string }{ + {"s3tables:GetTable", `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null}]}`}, + {"s3tables:GetTable", `{"Statement":{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null}}`}, + {"s3tables:GetTable", `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","condition":null}]}`}, + {"s3tables:GetTable", `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"condition":null}]}`}, + {"s3vectors:GetVectorBucket", `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3vectors:GetVectorBucket","Resource":"*","Condition":null}]}`}, + } { + if CheckPermissionWithContext(tc.action, "reader", "owner", tc.policy, "", ctx) { + t.Errorf("%s granted a non-owner", tc.policy) + } + } + allow := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*"}]}` + if !CheckPermissionWithContext("s3tables:GetTable", "reader", "owner", allow, "", ctx) { + t.Error("an Allow without a Condition must still grant") + } + deny := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":null}]}` + if CheckPermissionWithContext("s3tables:GetTable", "owner", "owner", deny, "", ctx) { + t.Error("a Deny with a null Condition must apply") + } +} + +// Condition objects under two keys, repeated or respelled, decode into one map +// in which an operator they share keeps only the last one's keys. Such a +// statement is refused, and one stored before that never grants and always +// vetoes. +func TestConditionSplitAcrossSpellings(t *testing.T) { + for _, split := range []string{ + `"Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"condition":{"StringEquals":{"aws:ResourceTag/team":"x"}}`, + `"Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"condition":{}`, + `"Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"Condition":{"StringEquals":{"aws:ResourceTag/team":"x"}}`, + `"Condition":null,"Condition":{}`, + } { + for _, wrap := range []string{`{"Statement":[%s]}`, `{"Statement":%s}`} { + allow := fmt.Sprintf(wrap, `{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*",`+split+`}`) + if err := validatePolicyDocument(allow); err == nil { + t.Errorf("%s: validatePolicyDocument accepted it", allow) + } + if CheckPermissionWithContext("s3tables:GetTable", "reader", "owner", allow, "", &PolicyContext{ResourceTags: map[string]string{"env": "dev", "team": "x"}}) { + t.Errorf("%s: the Allow granted", allow) + } + deny := fmt.Sprintf(wrap, `{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*",`+split+`}`) + if err := validatePolicyDocument(deny); err == nil { + t.Errorf("%s: validatePolicyDocument accepted it", deny) + } + if CheckPermissionWithContext("s3tables:GetTable", "owner", "owner", deny, "", &PolicyContext{ResourceTags: map[string]string{"env": "dev", "team": "x"}}) { + t.Errorf("%s: the Deny was skipped", deny) + } + } + } + single := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:GetTable","Resource":"*","condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}]}` + if err := validatePolicyDocument(single); err != nil { + t.Errorf("one lowercase spelling: %v", err) + } + if !CheckPermissionWithContext("s3tables:GetTable", "reader", "owner", single, "", &PolicyContext{ResourceTags: map[string]string{"env": "prod"}}) { + t.Error("one lowercase spelling no longer grants") + } +} + +// Only the Condition keys are read from the original bytes. Every other field +// keeps the spelling the re-marshaled decode always picked, so a stored Deny +// written twice stays a Deny. +func TestStatementFieldSpellingsDecodeAsBefore(t *testing.T) { + stmt := `{"effect":"Deny","Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*"}` + for _, policy := range []string{`{"Statement":[` + stmt + `]}`, `{"Statement":` + stmt + `}`} { + if CheckPermissionWithContext("s3tables:GetTable", "owner", "owner", policy, "", nil) { + t.Errorf("%s: the Deny no longer applies", policy) + } + } +} + +// A Deny whose operator the evaluator does not know applies instead of being +// skipped: the writer meant it to protect the resource. +func TestUnsupportedOperatorDenyApplies(t *testing.T) { + deny := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":{"StringEqualsBogus":{"aws:ResourceTag/env":"prod"}}}]}` + ctx := &PolicyContext{ResourceTags: map[string]string{"env": "dev"}} + if CheckPermissionWithContext("s3tables:GetTable", "owner", "owner", deny, "", ctx) { + t.Error("a Deny with an unsupported operator must apply") + } +} + +// One unusable entry makes the whole block unusable whichever entry the map +// yields first, so a non-matching sibling cannot hide it. +func TestUnusableConditionIgnoresMapOrder(t *testing.T) { + ctx := &PolicyContext{ResourceTags: map[string]string{"env": "prod"}} + for _, condition := range []string{ + `{"StringEquals":{"aws:ResourceTag/env":"dev","aws:ResourceTag/team":null}}`, + `{"StringEquals":{"aws:ResourceTag/env":"dev"},"StringNotEquals":{"aws:ResourceTag/team":[null]}}`, + } { + deny := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:GetTable","Resource":"*","Condition":` + condition + `}]}` + for i := 0; i < 32; i++ { + if CheckPermissionWithContext("s3tables:GetTable", "owner", "owner", deny, "", ctx) { + t.Fatalf("%s: the Deny was skipped on attempt %d", condition, i) + } + } + } } // An ARN-scoped statement reaches a caller recorded under the ARN's trailing diff --git a/weed/s3api/s3vectors/handlers.go b/weed/s3api/s3vectors/handlers.go index edd6609711..56a48f6828 100644 --- a/weed/s3api/s3vectors/handlers.go +++ b/weed/s3api/s3vectors/handlers.go @@ -500,6 +500,11 @@ func validateVectorPolicy(raw string) error { if err := json.Unmarshal([]byte(raw), &doc); err != nil { return fmt.Errorf("invalid policy document: %w", err) } + for key := range doc { + if key != "Statement" && strings.EqualFold(key, "Statement") { + return fmt.Errorf("invalid policy document: %s must be spelled Statement", key) + } + } for _, stmt := range policyStatements(doc) { for key := range stmt { for _, field := range []string{"NotAction", "NotResource", "NotPrincipal"} { @@ -507,22 +512,33 @@ func validateVectorPolicy(raw string) error { return fmt.Errorf("invalid policy document: %s is not supported", field) } } - if key != "Condition" && strings.EqualFold(key, "Condition") { - return fmt.Errorf("invalid policy document: %s must be spelled Condition", key) + for _, field := range []string{"Condition", "Effect"} { + if key != field && strings.EqualFold(key, field) { + return fmt.Errorf("invalid policy document: %s must be spelled %s", key, field) + } } } + if condition, ok := stmt["Condition"]; ok && condition == nil && stmt["Effect"] == "Allow" { + return fmt.Errorf("invalid policy document: an Allow statement's Condition must not be null") + } } for _, stmt := range typed.Statement { + if stmt.SplitCondition() { + return fmt.Errorf("invalid policy document: a statement's Condition must be under one key") + } for operator, keys := range stmt.Condition { if _, err := policy_engine.GetConditionEvaluator(operator); err != nil { return fmt.Errorf("invalid policy document: condition operator %q is not supported", operator) } + if len(keys) == 0 { + return fmt.Errorf("invalid policy document: condition %s needs at least one key", operator) + } for key, value := range keys { if !vectorConditionKey(key) { return fmt.Errorf("invalid policy document: condition key %q is not supported in a vector bucket policy", key) } - if !conditionValue(value) { - return fmt.Errorf("invalid policy document: condition %s on %q needs a string, number or boolean, or a non-empty list of them", operator, key) + if !s3tables.ConditionValue(operator, value) { + return fmt.Errorf("invalid policy document: condition %s on %q needs values %s can read (a number for Numeric*, an RFC 3339 time for Date*, true or false for Bool and Null, an IP or CIDR for IpAddress)", operator, key, operator) } } } @@ -540,23 +556,6 @@ func validateVectorPolicy(raw string) error { return nil } -// conditionValue reports whether the evaluator can compare v: a null value -// would panic it, and an empty list never matches. -func conditionValue(v interface{}) bool { - list, ok := v.([]interface{}) - if !ok { - list = []interface{}{v} - } - for _, item := range list { - switch item.(type) { - case string, float64, bool: - default: - return false - } - } - return len(list) > 0 -} - // vectorConditionKey reports whether the vector evaluation supplies key. func vectorConditionKey(key string) bool { if key == "aws:TagKeys" { diff --git a/weed/s3api/s3vectors/tag_conditions_test.go b/weed/s3api/s3vectors/tag_conditions_test.go index 78f2786db9..944f50e27d 100644 --- a/weed/s3api/s3vectors/tag_conditions_test.go +++ b/weed/s3api/s3vectors/tag_conditions_test.go @@ -282,6 +282,9 @@ func TestValidateVectorPolicyConditions(t *testing.T) { `{"StringEquals":{"aws:ResourceTag/env":[null]}}`, `{"StringEquals":{"aws:ResourceTag/env":{"a":"b"}}}`, `{"StringEquals":{"aws:ResourceTag/env":["prod",["dev"]]}}`, + `{"NumericLessThan":{"aws:RequestTag/priority":"low"}}`, + `{"StringEquals":{}}`, + `null`, } { if err := validateVectorPolicy(withCondition(bad)); err == nil { t.Errorf("%s accepted", bad) @@ -291,6 +294,29 @@ func TestValidateVectorPolicyConditions(t *testing.T) { if err := validateVectorPolicy(lower); err == nil { t.Error("a lower-case condition field was accepted") } + for _, misspelled := range []string{ + `{"Version":"2012-10-17","Statement":[{"effect":"Allow","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null}]}`, + `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","effect":"Allow","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null}]}`, + `{"Version":"2012-10-17","statement":[{"Effect":"Allow","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null}]}`, + } { + if err := validateVectorPolicy(misspelled); err == nil { + t.Errorf("%s accepted", misspelled) + } + } + for _, repeated := range []string{ + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null,"Condition":{}}]}`, + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"Condition":{"StringEquals":{"aws:ResourceTag/team":"x"}}}]}`, + `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}},"Condition":{"StringEquals":{"aws:ResourceTag/team":"x"}}}]}`, + `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null,"Condition":{}}]}`, + } { + if err := validateVectorPolicy(repeated); err == nil { + t.Errorf("%s accepted", repeated) + } + } + denyNull := `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":"*","Action":"s3vectors:GetIndex","Resource":"*","Condition":null}]}` + if err := validateVectorPolicy(denyNull); err != nil { + t.Errorf("a Deny with a null Condition was rejected: %v", err) + } } func TestPutVectorBucketPolicyWithTagConditions(t *testing.T) {