diff --git a/pkg/s3/access/README.md b/Bucket-Policy.md similarity index 91% rename from pkg/s3/access/README.md rename to Bucket-Policy.md index b1ff17075..b642de294 100644 --- a/pkg/s3/access/README.md +++ b/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. diff --git a/bucket-handlers.go b/bucket-handlers.go index 3d2c6d4e1..210c6f60c 100644 --- a/bucket-handlers.go +++ b/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) diff --git a/bucket-policy-handlers.go b/bucket-policy-handlers.go index d75968831..f59244d20 100644 --- a/bucket-policy-handlers.go +++ b/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) diff --git a/pkg/s3/access/policy.go b/bucket-policy-parser.go similarity index 68% rename from pkg/s3/access/policy.go rename to bucket-policy-parser.go index a659720ee..695cd97f7 100644 --- a/pkg/s3/access/policy.go +++ b/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 } diff --git a/bucket-policy.go b/bucket-policy.go index fb995bfd9..14f8c0ae0 100644 --- a/bucket-policy.go +++ b/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) diff --git a/server_fs_test.go b/server_fs_test.go index 3c6aff822..2ba7c8e68 100644 --- a/server_fs_test.go +++ b/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)