From b6ca39ea48282d307b8954de00f446dbda7cbe4c Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 27 Apr 2018 15:02:54 -0700 Subject: [PATCH] Support migrating inconsistent bucket policies (#5855) Previously we used allow bucket policies without `Version` field to be set to any given value, but this behavior is inconsistent with AWS S3. PR #5790 addressed this by making bucket policies stricter and cleaner, but this causes a breaking change causing any existing policies perhaps without `Version` field or the field to be empty to fail upon server startup. This PR brings a code to migrate under these scenarios as a one time operation. --- cmd/bucket-policy-handlers.go | 6 ++++++ cmd/bucket-policy-handlers_test.go | 19 +++++++++++++++++-- cmd/policy.go | 22 ++++++++++++++++------ pkg/policy/policy.go | 2 +- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index 8279944e0..41e3863ea 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -79,6 +79,12 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht return } + // Version in policy must not be empty + if bucketPolicy.Version == "" { + writeErrorResponse(w, ErrMalformedPolicy, r.URL) + return + } + if err = objAPI.SetBucketPolicy(ctx, bucket, bucketPolicy); err != nil { writeErrorResponse(w, toAPIErrorCode(err), r.URL) return diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go index 210e1a222..f9b4719b2 100644 --- a/cmd/bucket-policy-handlers_test.go +++ b/cmd/bucket-policy-handlers_test.go @@ -110,6 +110,8 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string // template for constructing HTTP request body for PUT bucket policy. bucketPolicyTemplate := `{"Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::%s"]},{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/this*"]}]}` + bucketPolicyTemplateWithoutVersion := `{"Version":"","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::%s"]},{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/this*"]}]}` + // test cases with sample input and expected output. testCases := []struct { bucketName string @@ -207,7 +209,7 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string // Test case - 8. // non-existent bucket is used. // writing BucketPolicy should fail. - // should result is 404 StatusNotFound + // should result in 404 StatusNotFound { bucketName: "non-existent-bucket", bucketPolicyReader: bytes.NewReader([]byte(fmt.Sprintf(bucketPolicyTemplate, "non-existent-bucket", "non-existent-bucket"))), @@ -220,7 +222,7 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string // Test case - 9. // non-existent bucket is used (with invalid bucket name) // writing BucketPolicy should fail. - // should result is 404 StatusNotFound + // should result in 404 StatusNotFound { bucketName: ".invalid-bucket", bucketPolicyReader: bytes.NewReader([]byte(fmt.Sprintf(bucketPolicyTemplate, ".invalid-bucket", ".invalid-bucket"))), @@ -230,6 +232,19 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string secretKey: credentials.SecretKey, expectedRespStatus: http.StatusNotFound, }, + // Test case - 10. + // Existent bucket with policy with Version field empty. + // writing BucketPolicy should fail. + // should result in 400 StatusBadRequest. + { + bucketName: bucketName, + bucketPolicyReader: bytes.NewReader([]byte(fmt.Sprintf(bucketPolicyTemplateWithoutVersion, bucketName, bucketName))), + + policyLen: len(fmt.Sprintf(bucketPolicyTemplateWithoutVersion, bucketName, bucketName)), + accessKey: credentials.AccessKey, + secretKey: credentials.SecretKey, + expectedRespStatus: http.StatusBadRequest, + }, } // Iterating over the test cases, calling the function under test and asserting the response. diff --git a/cmd/policy.go b/cmd/policy.go index afb0fa27f..d4a917871 100644 --- a/cmd/policy.go +++ b/cmd/policy.go @@ -24,6 +24,7 @@ import ( "sync" miniogopolicy "github.com/minio/minio-go/pkg/policy" + "github.com/minio/minio/cmd/logger" "github.com/minio/minio/pkg/handlers" "github.com/minio/minio/pkg/policy" ) @@ -87,6 +88,20 @@ func (sys *PolicySys) Init(objAPI ObjectLayer) error { return err } } else { + // This part is specifically written to handle migration + // when the Version string is empty, this was allowed + // in all previous minio releases but we need to migrate + // those policies by properly setting the Version string + // from now on. + if config.Version == "" { + logger.Info("Found in-consistent bucket policies, Migrating them for Bucket: (%s)", bucket.Name) + config.Version = policy.DefaultVersion + + if err = savePolicyConfig(objAPI, bucket.Name, config); err != nil { + return err + } + } + sys.Set(bucket.Name, *config) } } @@ -143,12 +158,7 @@ func GetPolicyConfig(objAPI ObjectLayer, bucketName string) (*policy.Policy, err return nil, err } - bucketPolicy, err := policy.ParseConfig(reader, bucketName) - if err != nil { - return nil, err - } - - return bucketPolicy, nil + return policy.ParseConfig(reader, bucketName) } func savePolicyConfig(objAPI ObjectLayer, bucketName string, bucketPolicy *policy.Policy) error { diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go index cac6853a1..77a712190 100644 --- a/pkg/policy/policy.go +++ b/pkg/policy/policy.go @@ -77,7 +77,7 @@ func (policy Policy) IsEmpty() bool { // isValid - checks if Policy is valid or not. func (policy Policy) isValid() error { - if policy.Version != DefaultVersion { + if policy.Version != DefaultVersion && policy.Version != "" { return fmt.Errorf("invalid version '%v'", policy.Version) }