Fail PutBucketPolicy if conditions are incompatible with actions. (#3659)

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.
+			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
 }
 

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
 		}
 	}

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: <ERROR> %s", i+1, actualErr.Error())
 		}