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

@@ -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

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.

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 {

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)
 	}