From 82113b747c17bb786048ad14c78c7e019941f517 Mon Sep 17 00:00:00 2001 From: karthic rao Date: Thu, 5 May 2016 05:26:57 +0530 Subject: [PATCH] Resource matching fix to overcome issues with regular expression based match (#1476) --- bucket-handlers.go | 2 +- bucket-policy-handlers.go | 40 ++++++++++++++--- bucket-policy-handlers_test.go | 78 ++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 bucket-policy-handlers_test.go diff --git a/bucket-handlers.go b/bucket-handlers.go index e1f9e04a4..7cf1f66fd 100644 --- a/bucket-handlers.go +++ b/bucket-handlers.go @@ -54,7 +54,7 @@ func enforceBucketPolicy(action string, bucket string, reqURL *url.URL) (s3Error return ErrAccessDenied } - // Construct resource in 'arn:aws:s3:::examplebucket' format. + // Construct resource in 'arn:aws:s3:::examplebucket/object' format. resource := AWSResourcePrefix + strings.TrimPrefix(reqURL.Path, "/") // Get conditions for policy verification. diff --git a/bucket-policy-handlers.go b/bucket-policy-handlers.go index cd9302f06..6827dd66d 100644 --- a/bucket-policy-handlers.go +++ b/bucket-policy-handlers.go @@ -77,15 +77,43 @@ func bucketPolicyActionMatch(action string, statement policyStatement) bool { // Verify if given resource matches with policy statement. func bucketPolicyResourceMatch(resource string, statement policyStatement) bool { - for _, presource := range statement.Resources { - matched, err := regexp.MatchString(presource, strings.TrimPrefix(resource, "/")) - fatalIf(err, "Invalid pattern, please verify the pattern string.", nil) - // For any path matches, we return quickly and the let the caller continue. - if matched { + + 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]) { + 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]) } - return false + + for _, presource := 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 { + return false + } + } + return true } // Verify if given condition matches with policy statement. diff --git a/bucket-policy-handlers_test.go b/bucket-policy-handlers_test.go new file mode 100644 index 000000000..3847100f3 --- /dev/null +++ b/bucket-policy-handlers_test.go @@ -0,0 +1,78 @@ +/* + * Minio Cloud Storage, (C) 2015, 2016 Minio, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package main + +import ( + "fmt" + "testing" +) + +// Tests validate Bucket policy resource matcher. +func TestBucketPolicyResourceMatch(t *testing.T) { + + // generates\ statement with given resource.. + generateStatement := func(resource string) policyStatement { + statement := policyStatement{} + statement.Resources = []string{resource} + return statement + } + + // generates resource prefix. + generateResource := func(bucketName, objectName string) string { + return AWSResourcePrefix + bucketName + "/" + objectName + } + + testCases := []struct { + resourceToMatch string + statement policyStatement + expectedResourceMatch bool + }{ + // Test case 1-4. + // Policy with resource ending with bucket/* allows access to all objects inside the given bucket. + {generateResource("minio-bucket", ""), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/*")), true}, + {generateResource("minio-bucket", ""), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/*")), true}, + {generateResource("minio-bucket", ""), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/*")), true}, + {generateResource("minio-bucket", ""), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/*")), true}, + // Test case - 5. + // Policy with resource ending with bucket/oo* should not allow access to bucket/output.txt. + {generateResource("minio-bucket", "output.txt"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/oo*")), false}, + // Test case - 6. + // Policy with resource ending with bucket/oo* should allow access to bucket/ootput.txt. + {generateResource("minio-bucket", "ootput.txt"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/oo*")), true}, + // Test case - 7. + // Policy with resource ending with bucket/oo* allows access to all subfolders starting with "oo" inside given bucket. + {generateResource("minio-bucket", "oop-bucket/my-file"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/oo*")), true}, + // Test case - 8. + {generateResource("minio-bucket", "Asia/India/1.pjg"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/Asia/Japan/*")), false}, + // Test case - 9. + {generateResource("minio-bucket", "Asia/India/1.pjg"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, "minio-bucket"+"/Asia/Japan/*")), false}, + // Test case - 10. + // Proves that the name space is flat. + {generateResource("minio-bucket", "Africa/Bihar/India/design_info.doc/Bihar"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, + "minio-bucket"+"/*/India/*/Bihar")), true}, + // Test case - 11. + // Proves that the name space is flat. + {generateResource("minio-bucket", "Asia/China/India/States/Bihar/output.txt"), generateStatement(fmt.Sprintf("%s%s", AWSResourcePrefix, + "minio-bucket"+"/*/India/*/Bihar/*")), true}, + } + for i, testCase := range testCases { + actualResourceMatch := bucketPolicyResourceMatch(testCase.resourceToMatch, testCase.statement) + if testCase.expectedResourceMatch != actualResourceMatch { + t.Errorf("Test %d: Expected Resource match to be `%v`, but instead found it to be `%v`", i+1, testCase.expectedResourceMatch, actualResourceMatch) + } + } +}