bucketpolicy: Improve bucket policy validation, avoid nested rules.

Bucket policy validation is more stricter now, to avoid nested
rules. The reason to do this is keep the rules simpler and more
meaningful avoiding conflicts.

This patch implements stricter checks.

Example policy to be generally avoided.
```
{
	"Version": "2012-10-17",
	"Statement": [
		{
			"Action": [
				"s3:GetObject",
				"s3:DeleteObject"
			],
			"Effect": "Allow",
			"Principal": {
				"AWS": [
					"*"
				]
			},
			"Resource": [
				"arn:aws:s3:::jarjarbing/*"
			]
		},
		{
			"Action": [
				"s3:GetObject",
				"s3:DeleteObject"
			],
			"Effect": "Deny",
			"Principal": {
				"AWS": [
					"*"
				]
			},
			"Resource": [
				"arn:aws:s3:::jarjarbing/restic/key/*"
			]
		}
	]
}
```

Bucket-Policy.md

@@ -9,14 +9,10 @@ This package implements parsing and validating bucket access policies based on A
 
 ### Supports following set of operations.
 
-    *
-    s3:*
     s3:GetObject
     s3:ListBucket
     s3:PutObject
-    s3:CreateBucket
     s3:GetBucketLocation
-    s3:DeleteBucket
     s3:DeleteObject
     s3:AbortMultipartUpload
     s3:ListBucketMultipartUploads
@@ -31,3 +27,7 @@ Supported applicable condition keys for each conditions.
 
     s3:prefix
     s3:max-keys
+
+### Nested policy support.
+
+Nested policies are not allowed.

bucket-handlers.go

@@ -33,7 +33,6 @@ import (
 	"github.com/minio/minio/pkg/crypto/sha256"
 	"github.com/minio/minio/pkg/fs"
 	"github.com/minio/minio/pkg/probe"
-	"github.com/minio/minio/pkg/s3/access"
 	"github.com/minio/minio/pkg/s3/signature4"
 )
 
@@ -54,14 +53,14 @@ func enforceBucketPolicy(action string, bucket string, reqURL *url.URL) (s3Error
 		}
 	}
 	// Parse the saved policy.
-	accessPolicy, e := accesspolicy.Validate(policy)
+	bucketPolicy, e := parseBucketPolicy(policy)
 	if e != nil {
 		errorIf(probe.NewError(e), "Parse policy failed.", nil)
 		return ErrAccessDenied
 	}
 
 	// Construct resource in 'arn:aws:s3:::examplebucket' format.
-	resource := accesspolicy.AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/")
+	resource := AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/")
 
 	// Get conditions for policy verification.
 	conditions := make(map[string]string)
@@ -70,7 +69,7 @@ func enforceBucketPolicy(action string, bucket string, reqURL *url.URL) (s3Error
 	}
 
 	// Validate action, resource and conditions with current policy statements.
-	if !bucketPolicyEvalStatements(action, resource, conditions, accessPolicy.Statements) {
+	if !bucketPolicyEvalStatements(action, resource, conditions, bucketPolicy.Statements) {
 		return ErrAccessDenied
 	}
 	return ErrNone
@@ -436,12 +435,6 @@ func (api storageAPI) PutBucketHandler(w http.ResponseWriter, r *http.Request) {
 		// For all unknown auth types return error.
 		writeErrorResponse(w, r, ErrAccessDenied, r.URL.Path)
 		return
-	case authTypeAnonymous:
-		// http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuAndPermissions.html
-		if s3Error := enforceBucketPolicy("s3:CreateBucket", bucket, r.URL); s3Error != ErrNone {
-			writeErrorResponse(w, r, s3Error, r.URL.Path)
-			return
-		}
 	case authTypePresigned:
 		ok, err := auth.DoesPresignedSignatureMatch()
 		if err != nil {
@@ -639,12 +632,6 @@ func (api storageAPI) DeleteBucketHandler(w http.ResponseWriter, r *http.Request
 		// For all unknown auth types return error.
 		writeErrorResponse(w, r, ErrAccessDenied, r.URL.Path)
 		return
-	case authTypeAnonymous:
-		// http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuAndPermissions.html
-		if s3Error := enforceBucketPolicy("s3:DeleteBucket", bucket, r.URL); s3Error != ErrNone {
-			writeErrorResponse(w, r, s3Error, r.URL.Path)
-			return
-		}
 	case authTypePresigned, authTypeSigned:
 		if s3Error := isReqAuthenticated(api.Signature, r); s3Error != ErrNone {
 			writeErrorResponse(w, r, s3Error, r.URL.Path)

bucket-policy-handlers.go

@@ -29,7 +29,6 @@ import (
 	mux "github.com/gorilla/mux"
 	"github.com/minio/minio/pkg/fs"
 	"github.com/minio/minio/pkg/probe"
-	"github.com/minio/minio/pkg/s3/access"
 )
 
 // maximum supported access policy size.
@@ -37,12 +36,13 @@ const maxAccessPolicySize = 20 * 1024 * 1024 // 20KiB.
 
 // Verify if a given action is valid for the url path based on the
 // existing bucket access policy.
-func bucketPolicyEvalStatements(action string, resource string, conditions map[string]string, statements []accesspolicy.Statement) bool {
+func bucketPolicyEvalStatements(action string, resource string, conditions map[string]string, statements []policyStatement) bool {
 	for _, statement := range statements {
 		if bucketPolicyMatchStatement(action, resource, conditions, statement) {
 			if statement.Effect == "Allow" {
 				return true
 			}
+			// Do not uncomment kept here for readability.
 			// else statement.Effect == "Deny"
 			return false
 		}
@@ -52,7 +52,7 @@ func bucketPolicyEvalStatements(action string, resource string, conditions map[s
 }
 
 // Verify if action, resource and conditions match input policy statement.
-func bucketPolicyMatchStatement(action string, resource string, conditions map[string]string, statement accesspolicy.Statement) bool {
+func bucketPolicyMatchStatement(action string, resource string, conditions map[string]string, statement policyStatement) bool {
 	// Verify if action matches.
 	if bucketPolicyActionMatch(action, statement) {
 		// Verify if resource matches.
@@ -67,7 +67,7 @@ func bucketPolicyMatchStatement(action string, resource string, conditions map[s
 }
 
 // Verify if given action matches with policy statement.
-func bucketPolicyActionMatch(action string, statement accesspolicy.Statement) bool {
+func bucketPolicyActionMatch(action string, statement policyStatement) bool {
 	for _, policyAction := range statement.Actions {
 		// Policy action can be a regex, validate the action with matching string.
 		matched, e := regexp.MatchString(policyAction, action)
@@ -80,7 +80,7 @@ func bucketPolicyActionMatch(action string, statement accesspolicy.Statement) bo
 }
 
 // Verify if given resource matches with policy statement.
-func bucketPolicyResourceMatch(resource string, statement accesspolicy.Statement) bool {
+func bucketPolicyResourceMatch(resource string, statement policyStatement) bool {
 	for _, presource := range statement.Resources {
 		matched, e := regexp.MatchString(presource, strings.TrimPrefix(resource, "/"))
 		fatalIf(probe.NewError(e), "Invalid pattern, please verify the pattern string.", nil)
@@ -93,7 +93,7 @@ func bucketPolicyResourceMatch(resource string, statement accesspolicy.Statement
 }
 
 // Verify if given condition matches with policy statement.
-func bucketPolicyConditionMatch(conditions map[string]string, statement accesspolicy.Statement) bool {
+func bucketPolicyConditionMatch(conditions map[string]string, statement policyStatement) bool {
 	// Supports following conditions.
 	// - StringEquals
 	// - StringNotEquals
@@ -152,29 +152,25 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ
 	// Read access policy up to maxAccessPolicySize.
 	// http://docs.aws.amazon.com/AmazonS3/latest/dev/access-policy-language-overview.html
 	// bucket policies are limited to 20KB in size, using a limit reader.
-	accessPolicyBytes, e := ioutil.ReadAll(io.LimitReader(r.Body, maxAccessPolicySize))
+	bucketPolicyBuf, e := ioutil.ReadAll(io.LimitReader(r.Body, maxAccessPolicySize))
 	if e != nil {
 		errorIf(probe.NewError(e).Trace(bucket), "Reading policy failed.", nil)
 		writeErrorResponse(w, r, ErrInternalError, r.URL.Path)
 		return
 	}
 
-	// Parse access access.
-	accessPolicy, e := accesspolicy.Validate(accessPolicyBytes)
+	// Parse bucket policy.
+	bucketPolicy, e := parseBucketPolicy(bucketPolicyBuf)
 	if e != nil {
+		errorIf(probe.NewError(e), "Unable to parse bucket policy.", nil)
 		writeErrorResponse(w, r, ErrInvalidPolicyDocument, r.URL.Path)
 		return
 	}
 
-	// If the policy resource has different bucket name, reject it.
-	for _, statement := range accessPolicy.Statements {
-		for _, resource := range statement.Resources {
-			resourcePrefix := strings.SplitAfter(resource, accesspolicy.AWSResourcePrefix)[1]
-			if !strings.HasPrefix(resourcePrefix, bucket) {
-				writeErrorResponse(w, r, ErrMalformedPolicy, r.URL.Path)
-				return
-			}
-		}
+	// Parse check bucket policy.
+	if s3Error := checkBucketPolicy(bucket, bucketPolicy); s3Error != ErrNone {
+		writeErrorResponse(w, r, s3Error, r.URL.Path)
+		return
 	}
 
 	// Set http request for signature verification.
@@ -192,10 +188,10 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ
 		}
 	} else if isRequestSignatureV4(r) {
 		sh := sha256.New()
-		sh.Write(accessPolicyBytes)
+		sh.Write(bucketPolicyBuf)
 		ok, err := api.Signature.DoesSignatureMatch(hex.EncodeToString(sh.Sum(nil)))
 		if err != nil {
-			errorIf(err.Trace(string(accessPolicyBytes)), "SaveBucketPolicy failed.", nil)
+			errorIf(err.Trace(string(bucketPolicyBuf)), "SaveBucketPolicy failed.", nil)
 			writeErrorResponse(w, r, ErrSignatureDoesNotMatch, r.URL.Path)
 			return
 		}
@@ -206,9 +202,9 @@ func (api storageAPI) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Requ
 	}
 
 	// Save bucket policy.
-	err := writeBucketPolicy(bucket, accessPolicyBytes)
+	err := writeBucketPolicy(bucket, bucketPolicyBuf)
 	if err != nil {
-		errorIf(err.Trace(bucket), "SaveBucketPolicy failed.", nil)
+		errorIf(err.Trace(bucket, string(bucketPolicyBuf)), "SaveBucketPolicy failed.", nil)
 		switch err.ToGoError().(type) {
 		case fs.BucketNameInvalid:
 			writeErrorResponse(w, r, ErrInvalidBucketName, r.URL.Path)

bucket-policy-parser.go

@@ -14,14 +14,16 @@
  * limitations under the License.
  */
 
-// Package accesspolicy implements AWS Access Policy Language parser in
+// This file implements AWS Access Policy Language parser in
 // accordance with http://docs.aws.amazon.com/AmazonS3/latest/dev/access-policy-language-overview.html
-package accesspolicy
+package main
 
 import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"regexp"
+	"sort"
 	"strings"
 )
 
@@ -32,14 +34,10 @@ const (
 
 // supportedActionMap - lists all the actions supported by minio.
 var supportedActionMap = map[string]struct{}{
-	"*":                             {},
-	"s3:*":                          {},
 	"s3:GetObject":                  {},
 	"s3:ListBucket":                 {},
 	"s3:PutObject":                  {},
-	"s3:CreateBucket":               {},
 	"s3:GetBucketLocation":          {},
-	"s3:DeleteBucket":               {},
 	"s3:DeleteObject":               {},
 	"s3:AbortMultipartUpload":       {},
 	"s3:ListBucketMultipartUploads": {},
@@ -47,15 +45,15 @@ var supportedActionMap = map[string]struct{}{
 }
 
 // User - canonical users list.
-type User struct {
+type policyUser struct {
 	AWS []string
 }
 
 // Statement - minio policy statement
-type Statement struct {
+type policyStatement struct {
 	Sid        string
 	Effect     string
-	Principal  User
+	Principal  policyUser                   `json:"Principal"`
 	Actions    []string                     `json:"Action"`
 	Resources  []string                     `json:"Resource"`
 	Conditions map[string]map[string]string `json:"Condition"`
@@ -63,8 +61,8 @@ type Statement struct {
 
 // BucketPolicy - minio policy collection
 type BucketPolicy struct {
-	Version    string      // date in 0000-00-00 format
-	Statements []Statement `json:"Statement"`
+	Version    string            // date in 0000-00-00 format
+	Statements []policyStatement `json:"Statement"`
 }
 
 // supportedEffectMap - supported effects.
@@ -183,9 +181,66 @@ func isValidConditions(conditions map[string]map[string]string) (err error) {
 	return nil
 }
 
-// Validate - validate if request body is of proper JSON and in
-// accordance with policy standards.
-func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) {
+// List of actions for which prefixes are not allowed.
+var invalidPrefixActions = map[string]struct{}{
+	"s3:GetBucketLocation":          {},
+	"s3:ListBucket":                 {},
+	"s3:ListBucketMultipartUploads": {},
+	// Add actions which do not honor prefixes.
+}
+
+// checkBucketPolicy validates unmarshalled bucket policy structure.
+func checkBucketPolicy(bucket string, bucketPolicy BucketPolicy) APIErrorCode {
+	// Validate statements for special actions and collect resources
+	// for others to validate nesting.
+	var resources []string
+	for _, statement := range bucketPolicy.Statements {
+		for _, action := range statement.Actions {
+			for _, resource := range statement.Resources {
+				resourcePrefix := strings.SplitAfter(resource, AWSResourcePrefix)[1]
+				if _, ok := invalidPrefixActions[action]; ok {
+					// Resource prefix is not equal to bucket for
+					// prefix invalid actions, reject them.
+					if resourcePrefix != bucket {
+						return ErrMalformedPolicy
+					}
+				} else {
+					// For all other actions validate if prefix begins
+					// with bucket, if not reject them.
+					if !strings.HasPrefix(resourcePrefix, bucket) {
+						return ErrMalformedPolicy
+					}
+					// All valid resources collect them separately to verify nesting.
+					resources = append(resources, resourcePrefix)
+				}
+			}
+		}
+	}
+
+	// Sort strings as shorter first.
+	sort.Strings(resources)
+
+	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) {
+				return ErrMalformedPolicy
+			}
+		}
+	}
+
+	// No errors found.
+	return ErrNone
+}
+
+// parseBucketPolicy - parses and validates if bucket policy is of
+// proper JSON and follows allowed restrictions with policy standards.
+func parseBucketPolicy(bucketPolicyBuf []byte) (policy BucketPolicy, err error) {
 	if err = json.Unmarshal(bucketPolicyBuf, &policy); err != nil {
 		return BucketPolicy{}, err
 	}
@@ -216,7 +271,7 @@ func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) {
 		if err := isValidActions(statement.Actions); err != nil {
 			return BucketPolicy{}, err
 		}
-		// Statment resources should be valid.
+		// Statement resources should be valid.
 		if err := isValidResources(statement.Resources); err != nil {
 			return BucketPolicy{}, err
 		}
@@ -225,6 +280,23 @@ func Validate(bucketPolicyBuf []byte) (policy BucketPolicy, err error) {
 			return BucketPolicy{}, err
 		}
 	}
+
+	// Separate deny and allow statements, so that we can apply deny
+	// statements in the beginning followed by Allow statements.
+	var denyStatements []policyStatement
+	var allowStatements []policyStatement
+	for _, statement := range policy.Statements {
+		if statement.Effect == "Deny" {
+			denyStatements = append(denyStatements, statement)
+			continue
+		}
+		// else if statement.Effect == "Allow"
+		allowStatements = append(allowStatements, statement)
+	}
+
+	// Deny statements are enforced first once matched.
+	policy.Statements = append(denyStatements, allowStatements...)
+
 	// Return successfully parsed policy structure.
 	return policy, nil
 }

bucket-policy.go

@@ -138,6 +138,11 @@ func writeBucketPolicy(bucket string, accessPolicyBytes []byte) *probe.Error {
 		}
 	}
 
+	// Create top level directory.
+	if e := os.MkdirAll(filepath.Dir(bucketPolicyFile), 0700); e != nil {
+		return probe.NewError(e)
+	}
+
 	// Write bucket policy.
 	if e := ioutil.WriteFile(bucketPolicyFile, accessPolicyBytes, 0600); e != nil {
 		return probe.NewError(e)

server_fs_test.go

@@ -279,6 +279,76 @@ func (s *MyAPIFSCacheSuite) TestAuth(c *C) {
 	c.Assert(len(accessID), Equals, minioAccessID)
 }
 
+func (s *MyAPIFSCacheSuite) TestBucketPolicy(c *C) {
+	// Sample bucket policy.
+	bucketPolicyBuf := `{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Action": [
+                "s3:GetBucketLocation",
+                "s3:ListBucket"
+            ],
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": [
+                    "*"
+                ]
+            },
+            "Resource": [
+                "arn:aws:s3:::policybucket"
+            ]
+        },
+        {
+            "Action": [
+                "s3:GetObject"
+            ],
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": [
+                    "*"
+                ]
+            },
+            "Resource": [
+                "arn:aws:s3:::policybucket/this*"
+            ]
+        }
+    ]
+}`
+
+	// Put a new bucket policy.
+	request, err := s.newRequest("PUT", testAPIFSCacheServer.URL+"/policybucket?policy", int64(len(bucketPolicyBuf)), bytes.NewReader([]byte(bucketPolicyBuf)))
+	c.Assert(err, IsNil)
+
+	client := http.Client{}
+	response, err := client.Do(request)
+	c.Assert(err, IsNil)
+	c.Assert(response.StatusCode, Equals, http.StatusNoContent)
+
+	// Fetch the uploaded policy.
+	request, err = s.newRequest("GET", testAPIFSCacheServer.URL+"/policybucket?policy", 0, nil)
+	c.Assert(err, IsNil)
+
+	client = http.Client{}
+	response, err = client.Do(request)
+	c.Assert(err, IsNil)
+	c.Assert(response.StatusCode, Equals, http.StatusOK)
+
+	bucketPolicyReadBuf, err := ioutil.ReadAll(response.Body)
+	c.Assert(err, IsNil)
+	// Verify if downloaded policy matches with previousy uploaded.
+	c.Assert(bytes.Equal([]byte(bucketPolicyBuf), bucketPolicyReadBuf), Equals, true)
+
+	// Delete policy.
+	request, err = s.newRequest("DELETE", testAPIFSCacheServer.URL+"/policybucket?policy", 0, nil)
+	c.Assert(err, IsNil)
+
+	client = http.Client{}
+	response, err = client.Do(request)
+	c.Assert(err, IsNil)
+	c.Assert(response.StatusCode, Equals, http.StatusNoContent)
+}
+
 func (s *MyAPIFSCacheSuite) TestDeleteBucket(c *C) {
 	request, err := s.newRequest("PUT", testAPIFSCacheServer.URL+"/deletebucket", 0, nil)
 	c.Assert(err, IsNil)