bucketPolicy: Do not use regexes, just do prefix matches. (#1497)

AWS arn supports wildcards and this is flat namespace, simple
prefix matching is fine.

Fixes #1481
Fixes #1482

bucket-policy-handlers.go

@@ -75,41 +75,41 @@ func bucketPolicyActionMatch(action string, statement policyStatement) bool {
 	return false
 }
 
-// Verify if given resource matches with policy statement.
-func bucketPolicyResourceMatch(resource string, statement policyStatement) bool {
-
-	match := func(pattern, subj string) bool {
-		if pattern == "" {
-			return subj == pattern
-		}
-		if pattern == "*" {
-			return true
-		}
-		parts := strings.Split(pattern, "*")
-		if len(parts) == 1 {
-			return subj == pattern
-		}
-		tGlob := strings.HasSuffix(pattern, "*")
-		end := len(parts) - 1
-		if !strings.HasPrefix(subj, parts[0]) {
+// Match function matches wild cards in 'pattern' for resource.
+func resourceMatch(pattern, resource string) bool {
+	if pattern == "" {
+		return resource == pattern
+	}
+	if pattern == "*" {
+		return true
+	}
+	parts := strings.Split(pattern, "*")
+	if len(parts) == 1 {
+		return resource == pattern
+	}
+	tGlob := strings.HasSuffix(pattern, "*")
+	end := len(parts) - 1
+	if !strings.HasPrefix(resource, parts[0]) {
+		return false
+	}
+	for i := 1; i < end; i++ {
+		if !strings.Contains(resource, parts[i]) {
 			return false
 		}
-		for i := 1; i < end; i++ {
-			if !strings.Contains(subj, parts[i]) {
-				return false
-			}
-			idx := strings.Index(subj, parts[i]) + len(parts[i])
-			subj = subj[idx:]
-		}
-		return tGlob || strings.HasSuffix(subj, parts[end])
+		idx := strings.Index(resource, parts[i]) + len(parts[i])
+		resource = resource[idx:]
 	}
+	return tGlob || strings.HasSuffix(resource, parts[end])
+}
 
-	for _, presource := range statement.Resources {
+// Verify if given resource matches with policy statement.
+func bucketPolicyResourceMatch(resource string, statement policyStatement) bool {
+	for _, resourcep := range statement.Resources {
 		// the resource rule for object could contain "*" wild card.
 		// the requested object can be given access based on the already set bucket policy if
 		// the match is successful.
 		// More info: http://docs.aws.amazon.com/AmazonS3/latest/dev/s3-arn-format.html .
-		if matched := match(presource, resource); !matched {
+		if matched := resourceMatch(resourcep, resource); !matched {
 			return false
 		}
 	}

bucket-policy-parser.go

@@ -22,7 +22,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"regexp"
+	"path"
 	"sort"
 	"strings"
 )
@@ -200,6 +200,14 @@ var invalidPrefixActions = map[string]struct{}{
 	// Add actions which do not honor prefixes.
 }
 
+// resourcePrefix - provides the prefix removing any wildcards.
+func resourcePrefix(resource string) string {
+	if strings.HasSuffix(resource, "*") {
+		resource = strings.TrimSuffix(resource, "*")
+	}
+	return path.Clean(resource)
+}
+
 // checkBucketPolicyResources validates Resources in unmarshalled bucket policy structure.
 // First valation of Resources done for given set of Actions.
 // Later its validated for recursive Resources.
@@ -232,7 +240,7 @@ func checkBucketPolicyResources(bucket string, bucketPolicy BucketPolicy) APIErr
 
 	var resources []string
 	for resource := range resourceMap {
-		resources = append(resources, resource)
+		resources = append(resources, resourcePrefix(resource))
 	}
 
 	// Sort strings as shorter first.
@@ -241,12 +249,12 @@ func checkBucketPolicyResources(bucket string, bucketPolicy BucketPolicy) APIErr
 	for len(resources) > 1 {
 		var resource string
 		resource, resources = resources[0], resources[1:]
-		resourceRegex := regexp.MustCompile(resource)
 		// Loop through all resources, if one of them matches with
 		// previous shorter one, it means we have detected
 		// nesting. Reject such rules.
 		for _, otherResource := range resources {
-			if resourceRegex.MatchString(otherResource) {
+			// Common prefix reject such rules.
+			if strings.HasPrefix(otherResource, resource) {
 				return ErrMalformedPolicy
 			}
 		}

bucket-policy-parser_test.go

@@ -453,31 +453,42 @@ func TestCheckBucketPolicyResources(t *testing.T) {
 		return statements
 	}
 
+	// constructing policy statement with lexically close characters.
+	// should not result in ErrMalformedPolicy
+	setResourceLexical := func(statements []policyStatement) []policyStatement {
+		statements[0].Resources = []string{"arn:aws:s3:::minio-bucket/op*", "arn:aws:s3:::minio-bucket/oo*"}
+		return statements
+	}
+
 	// List of BucketPolicy used for tests.
 	bucketAccessPolicies := []BucketPolicy{
-		// BucketPolicy - 0.
+		// BucketPolicy - 1.
 		// Contains valid read only policy statement.
 		{Version: "1.0", Statements: setReadOnlyStatement("minio-bucket", "")},
-		// BucketPolicy - 1.
+		// BucketPolicy - 2.
 		// Contains valid read-write only policy statement.
 		{Version: "1.0", Statements: setReadWriteStatement("minio-bucket", "Asia/")},
-		// BucketPolicy - 2.
+		// BucketPolicy - 3.
 		// Contains valid write only policy statement.
 		{Version: "1.0", Statements: setWriteOnlyStatement("minio-bucket", "Asia/India/")},
-		// BucketPolicy - 3.
+		// BucketPolicy - 4.
 		// Contains invalidPrefixActions.
 		// Since resourcePrefix is not to the bucket-name, it return ErrMalformedPolicy.
 		{Version: "1.0", Statements: setReadOnlyStatement("minio-bucket-fail", "Asia/India/")},
-		// BucketPolicy - 4.
+		// BucketPolicy - 5.
 		// constructing policy statement without invalidPrefixActions (check bucket-policy-parser.go).
 		// but bucket part of the resource is not equal to the bucket name.
 		// this results in return of ErrMalformedPolicy.
 		{Version: "1.0", Statements: setValidPrefixActions(setWriteOnlyStatement("minio-bucket-fail", "Asia/India/"))},
-		// BucketPolicy - 5.
-		// constructing policy statement without invalidPrefixActions (check bucket-policy-parser.go).
+		// BucketPolicy - 6.
 		// contructing policy statement with recursive resources.
 		// should result in ErrMalformedPolicy
 		{Version: "1.0", Statements: setRecurseResource(setValidPrefixActions(setWriteOnlyStatement("minio-bucket", "")))},
+		// BucketPolciy - 7.
+		// constructing policy statment with non recursive but
+		// lexically close resources.
+		// should result in ErrNone.
+		{Version: "1.0", Statements: setResourceLexical(setValidPrefixActions(setWriteOnlyStatement("minio-bucket", "oo")))},
 	}
 
 	testCases := []struct {
@@ -505,6 +516,11 @@ func TestCheckBucketPolicyResources(t *testing.T) {
 		// contructing policy statement with recursive resources.
 		// should result in ErrMalformedPolicy.
 		{bucketAccessPolicies[5], ErrMalformedPolicy, false},
+		// Test case - 7.
+		// constructing policy statement with lexically close
+		// characters.
+		// should result in ErrNone.
+		{bucketAccessPolicies[6], ErrNone, true},
 	}
 	for i, testCase := range testCases {
 		apiErrCode := checkBucketPolicyResources("minio-bucket", testCase.inputPolicy)