From 2665aba55525f47faf8a362d21b2aa1da10e09fa Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Mon, 30 Jan 2017 22:50:16 +0530 Subject: [PATCH] Fail PutBucketPolicy if conditions are incompatible with actions. (#3659) --- cmd/bucket-policy-handlers.go | 15 +++++++++----- cmd/bucket-policy-parser.go | 31 ++++++++++++++++++++-------- cmd/bucket-policy-parser_test.go | 35 +++++++++++++++++--------------- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index 6425c2df9..00b3b5d2b 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -97,6 +97,9 @@ func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement p // - s3:max-keys // - s3:aws-Referer + // The following loop evaluates the logical AND of all the + // conditions in the statement. Note: we can break out of the + // loop if and only if a condition evaluates to false. for condition, conditionKeyVal := range statement.Conditions { prefixConditon := conditionKeyVal["s3:prefix"] maxKeyCondition := conditionKeyVal["s3:max-keys"] @@ -126,13 +129,17 @@ func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement p } // wildcard match of referer in statement was not empty. // StringLike has a match, i.e, condition evaluates to true. + refererFound := false for referer := range conditions["referer"] { if !awsReferers.FuncMatch(refererMatch, referer).IsEmpty() { - return true + refererFound = true + break } } // No matching referer found, so the condition is false. - return false + if !refererFound { + return false + } } else if condition == "StringNotLike" { awsReferers := conditionKeyVal["aws:Referer"] // Skip empty condition, it is trivially satisfied. @@ -146,11 +153,9 @@ func bucketPolicyConditionMatch(conditions map[string]set.StringSet, statement p return false } } - // No matching referer found, so the condition is true. - return true } } - // No conditions were present in the statement, so trivially true (always). + return true } diff --git a/cmd/bucket-policy-parser.go b/cmd/bucket-policy-parser.go index f9324846c..7af5e5bf4 100644 --- a/cmd/bucket-policy-parser.go +++ b/cmd/bucket-policy-parser.go @@ -29,6 +29,11 @@ import ( "github.com/minio/minio-go/pkg/set" ) +var conditionKeyActionMap = map[string]set.StringSet{ + "s3:prefix": set.CreateStringSet("s3:ListBucket"), + "s3:max-keys": set.CreateStringSet("s3:ListBucket"), +} + // supportedActionMap - lists all the actions supported by minio. var supportedActionMap = set.CreateStringSet("*", "s3:*", "s3:GetObject", "s3:ListBucket", "s3:PutObject", "s3:GetBucketLocation", "s3:DeleteObject", @@ -178,11 +183,12 @@ func isValidPrincipals(principal interface{}) (err error) { return nil } -// isValidConditions - are valid conditions. -func isValidConditions(conditions map[string]map[string]set.StringSet) (err error) { - // Verify conditions should be valid. - // Validate if stringEquals, stringNotEquals are present - // if not throw an error. +// isValidConditions - returns nil if the given conditions valid and +// corresponding error otherwise. +func isValidConditions(actions set.StringSet, conditions map[string]map[string]set.StringSet) (err error) { + // Verify conditions should be valid. Validate if only + // supported condition keys are present and return error + // otherwise. conditionKeyVal := make(map[string]set.StringSet) for conditionType := range conditions { if !supportedConditionsType.Contains(conditionType) { @@ -194,6 +200,15 @@ func isValidConditions(conditions map[string]map[string]set.StringSet) (err erro err = fmt.Errorf("Unsupported condition key '%s', please validate your policy document", conditionType) return err } + + compatibleActions := conditionKeyActionMap[key] + if !compatibleActions.IsEmpty() && + compatibleActions.Intersection(actions).IsEmpty() { + err = fmt.Errorf("Unsupported condition key %s for the given actions %s, "+ + "please validate your policy document", key, actions) + return err + } + conditionVal, ok := conditionKeyVal[key] if ok && !value.Intersection(conditionVal).IsEmpty() { err = fmt.Errorf("Ambigious condition values for key '%s', please validate your policy document", key) @@ -222,8 +237,8 @@ func resourcePrefix(resource string) string { } // checkBucketPolicyResources validates Resources in unmarshalled bucket policy structure. -// First valation of Resources done for given set of Actions. -// Later its validated for recursive Resources. +// - Resources are validated against the given set of Actions. +// - func checkBucketPolicyResources(bucket string, bucketPolicy *bucketPolicy) APIErrorCode { // Validate statements for special actions and collect resources // for others to validate nesting. @@ -317,7 +332,7 @@ func parseBucketPolicy(bucketPolicyReader io.Reader, policy *bucketPolicy) (err return err } // Statement conditions should be valid. - if err := isValidConditions(statement.Conditions); err != nil { + if err := isValidConditions(statement.Actions, statement.Conditions); err != nil { return err } } diff --git a/cmd/bucket-policy-parser_test.go b/cmd/bucket-policy-parser_test.go index a9418db1f..a712a30c4 100644 --- a/cmd/bucket-policy-parser_test.go +++ b/cmd/bucket-policy-parser_test.go @@ -433,7 +433,12 @@ func TestIsValidConditions(t *testing.T) { generateConditions("StringNotEquals", "s3:max-keys", "100"), } + getObjectActionSet := set.CreateStringSet("s3:GetObject") + roBucketActionSet := set.CreateStringSet(readOnlyBucketActions...) + maxKeysConditionErr := fmt.Errorf("Unsupported condition key %s for the given actions %s, "+ + "please validate your policy document", "s3:max-keys", getObjectActionSet) testCases := []struct { + inputActions set.StringSet inputCondition map[string]map[string]set.StringSet // expected result. expectedErr error @@ -443,46 +448,44 @@ func TestIsValidConditions(t *testing.T) { // Malformed conditions. // Test case - 1. // "StringValues" is an invalid type. - {testConditions[0], fmt.Errorf("Unsupported condition type 'StringValues', " + + {roBucketActionSet, testConditions[0], fmt.Errorf("Unsupported condition type 'StringValues', " + "please validate your policy document"), false}, // Test case - 2. // "s3:Object" is an invalid key. - {testConditions[1], fmt.Errorf("Unsupported condition key " + + {roBucketActionSet, testConditions[1], fmt.Errorf("Unsupported condition key " + "'StringEquals', please validate your policy document"), false}, // Test case - 3. // Test case with Ambigious conditions set. - {testConditions[2], fmt.Errorf("Ambigious condition values for key 's3:prefix', " + + {roBucketActionSet, testConditions[2], fmt.Errorf("Ambigious condition values for key 's3:prefix', " + "please validate your policy document"), false}, // Test case - 4. // Test case with valid and invalid condition types. - {testConditions[3], fmt.Errorf("Unsupported condition type 'InvalidType', " + + {roBucketActionSet, testConditions[3], fmt.Errorf("Unsupported condition type 'InvalidType', " + "please validate your policy document"), false}, // Test case - 5. // Test case with valid and invalid condition keys. - {testConditions[4], fmt.Errorf("Unsupported condition key 'StringEquals', " + + {roBucketActionSet, testConditions[4], fmt.Errorf("Unsupported condition key 'StringEquals', " + "please validate your policy document"), false}, // Test cases with valid conditions. // Test case - 6. - {testConditions[5], nil, true}, + {roBucketActionSet, testConditions[5], nil, true}, // Test case - 7. - {testConditions[6], nil, true}, + {roBucketActionSet, testConditions[6], nil, true}, // Test case - 8. - {testConditions[7], nil, true}, + {roBucketActionSet, testConditions[7], nil, true}, // Test case - 9. - {testConditions[8], nil, true}, + {roBucketActionSet, testConditions[8], nil, true}, // Test case - 10. - {testConditions[9], nil, true}, + {roBucketActionSet, testConditions[9], nil, true}, // Test case - 11. - {testConditions[10], nil, true}, + {roBucketActionSet, testConditions[10], nil, true}, // Test case - 12. - {testConditions[11], nil, true}, + {roBucketActionSet, testConditions[11], nil, true}, // Test case - 13. - {testConditions[11], nil, true}, - // Test case - 14. - {testConditions[11], nil, true}, + {getObjectActionSet, testConditions[11], maxKeysConditionErr, false}, } for i, testCase := range testCases { - actualErr := isValidConditions(testCase.inputCondition) + actualErr := isValidConditions(testCase.inputActions, testCase.inputCondition) if actualErr != nil && testCase.shouldPass { t.Errorf("Test %d: Expected to pass, but failed with: %s", i+1, actualErr.Error()) }