Merge pull request #1233 from harshavardhana/bucket-policy

bucketpolicy: Improve bucket policy validation, avoid nested rules.

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)