From ca5ca8332bdef662bc6bcb3e421013139c42954e Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 22 Sep 2016 23:47:48 -0700 Subject: [PATCH] bucket: refactor policies and fix bugs related to enforcing policies. (#2766) This patch also addresses the problem of double caching at object layer once at XL and another at handler layer. --- cmd/bucket-handlers.go | 24 +++++-- cmd/bucket-handlers_test.go | 6 -- cmd/bucket-policy-handlers.go | 6 -- cmd/bucket-policy-handlers_test.go | 6 -- cmd/bucket-policy.go | 104 +---------------------------- cmd/controller-handlers.go | 15 +++-- cmd/routers.go | 4 -- cmd/storage-errors.go | 3 - cmd/test-utils_test.go | 8 ++- cmd/web-handlers.go | 18 ++--- 10 files changed, 46 insertions(+), 148 deletions(-) diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 920775c80..17cd94763 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -31,13 +31,25 @@ import ( // http://docs.aws.amazon.com/AmazonS3/latest/dev/using-with-s3-actions.html // Enforces bucket policies for a bucket for a given tatusaction. func enforceBucketPolicy(bucket string, action string, reqURL *url.URL) (s3Error APIErrorCode) { - if !IsValidBucketName(bucket) { - return ErrInvalidBucketName - } // Fetch bucket policy, if policy is not set return access denied. - policy := globalBucketPolicies.GetBucketPolicy(bucket) - if policy == nil { - return ErrAccessDenied + policy, err := readBucketPolicy(bucket, newObjectLayerFn()) + if err != nil { + err = errorCause(err) + switch err.(type) { + case BucketNameInvalid: + // Return error for invalid bucket name. + return ErrInvalidBucketName + case BucketNotFound: + // For no bucket found we return NoSuchBucket instead. + return ErrNoSuchBucket + case BucketPolicyNotFound: + // For no bucket policy found, return AccessDenied, since + // anonymous requests are not allowed without bucket policies. + return ErrAccessDenied + } + errorIf(err, "Unable to read bucket policy.") + // Return internal error for any other errors so that we can investigate. + return ErrInternalError } // Construct resource in 'arn:aws:s3:::examplebucket/object' format. diff --git a/cmd/bucket-handlers_test.go b/cmd/bucket-handlers_test.go index 60b693ac8..bc993bac2 100644 --- a/cmd/bucket-handlers_test.go +++ b/cmd/bucket-handlers_test.go @@ -30,8 +30,6 @@ func TestGetBucketLocationHandler(t *testing.T) { } func testGetBucketLocationHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() // Create bucket. @@ -130,8 +128,6 @@ func TestHeadBucketHandler(t *testing.T) { } func testHeadBucketHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() // Create bucket. @@ -207,8 +203,6 @@ func TestListMultipartUploadsHandler(t *testing.T) { // testListMultipartUploadsHandler - Tests validate listing of multipart uploads. func testListMultipartUploadsHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index e6a5b826e..65207c66d 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -196,9 +196,6 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht return } - // Set the bucket policy in memory. - globalBucketPolicies.SetBucketPolicy(bucket, policy) - // Success. writeSuccessNoContent(w) } @@ -242,9 +239,6 @@ func (api objectAPIHandlers) DeleteBucketPolicyHandler(w http.ResponseWriter, r return } - // Remove bucket policy. - globalBucketPolicies.RemoveBucketPolicy(bucket) - // Success. writeSuccessNoContent(w) } diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go index cc2b75c45..27e5517de 100644 --- a/cmd/bucket-policy-handlers_test.go +++ b/cmd/bucket-policy-handlers_test.go @@ -247,8 +247,6 @@ func TestPutBucketPolicyHandler(t *testing.T) { // testPutBucketPolicyHandler - Test for Bucket policy end point. // TODO: Add exhaustive cases with various combination of statement fields. func testPutBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() // Create bucket. @@ -313,8 +311,6 @@ func TestGetBucketPolicyHandler(t *testing.T) { // testGetBucketPolicyHandler - Test for end point which fetches the access policy json of the given bucket. // TODO: Add exhaustive cases with various combination of statement fields. func testGetBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() // Create bucket. @@ -423,8 +419,6 @@ func TestDeleteBucketPolicyHandler(t *testing.T) { // testDeleteBucketPolicyHandler - Test for Delete bucket policy end point. // TODO: Add exhaustive cases with various combination of statement fields. func testDeleteBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestErrHandler) { - initBucketPolicies(obj) - // get random bucket name. bucketName := getRandomBucketName() // Create bucket. diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 7f62dd9d5..96fb7ea00 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -18,110 +18,10 @@ package cmd import ( "bytes" - "errors" "io" "path" - "sync" ) -// Variable represents bucket policies in memory. -var globalBucketPolicies *bucketPolicies - -// Global bucket policies list, policies are enforced on each bucket looking -// through the policies here. -type bucketPolicies struct { - rwMutex *sync.RWMutex - - // Collection of 'bucket' policies. - bucketPolicyConfigs map[string]*bucketPolicy -} - -// Fetch bucket policy for a given bucket. -func (bp bucketPolicies) GetBucketPolicy(bucket string) *bucketPolicy { - bp.rwMutex.RLock() - defer bp.rwMutex.RUnlock() - return bp.bucketPolicyConfigs[bucket] -} - -// Set a new bucket policy for a bucket, this operation will overwrite -// any previous bucketpolicies for the bucket. -func (bp *bucketPolicies) SetBucketPolicy(bucket string, policy *bucketPolicy) error { - bp.rwMutex.Lock() - defer bp.rwMutex.Unlock() - if policy == nil { - return errors.New("invalid argument") - } - bp.bucketPolicyConfigs[bucket] = policy - return nil -} - -// Remove bucket policy for a bucket, from in-memory map. -func (bp *bucketPolicies) RemoveBucketPolicy(bucket string) { - bp.rwMutex.Lock() - defer bp.rwMutex.Unlock() - delete(bp.bucketPolicyConfigs, bucket) -} - -// Loads all bucket policies from persistent layer. -func loadAllBucketPolicies(objAPI ObjectLayer) (policies map[string]*bucketPolicy, err error) { - // List buckets to proceed loading all notification configuration. - buckets, err := objAPI.ListBuckets() - errorIf(err, "Unable to list buckets.") - err = errorCause(err) - if err != nil { - return nil, err - } - - policies = make(map[string]*bucketPolicy) - var pErrs []error - // Loads bucket policy. - for _, bucket := range buckets { - policy, pErr := readBucketPolicy(bucket.Name, objAPI) - if pErr != nil { - switch pErr.(type) { - case BucketPolicyNotFound: - continue - } - pErrs = append(pErrs, pErr) - // Continue to load other bucket policies if possible. - continue - } - policies[bucket.Name] = policy - } - - // Look for any errors occurred while reading bucket policies. - for _, pErr := range pErrs { - if pErr != nil { - return policies, pErr - } - } - - // Success. - return policies, nil -} - -// Intialize all bucket policies. -func initBucketPolicies(objAPI ObjectLayer) error { - if objAPI == nil { - return errInvalidArgument - } - - // Read all bucket policies. - policies, err := loadAllBucketPolicies(objAPI) - if err != nil { - return err - } - - // Populate global bucket collection. - globalBucketPolicies = &bucketPolicies{ - rwMutex: &sync.RWMutex{}, - bucketPolicyConfigs: policies, - } - - // Success. - return nil -} - // getOldBucketsConfigPath - get old buckets config path. (Only used for migrating old bucket policies) func getOldBucketsConfigPath() (string, error) { configPath, err := getConfigPath() @@ -135,8 +35,8 @@ func getOldBucketsConfigPath() (string, error) { // if bucket policy is not found. func readBucketPolicyJSON(bucket string, objAPI ObjectLayer) (bucketPolicyReader io.Reader, err error) { // Verify if bucket actually exists - if e := isBucketExist(bucket, objAPI); e != nil { - return nil, e + if err = isBucketExist(bucket, objAPI); err != nil { + return nil, err } policyPath := pathJoin(bucketConfigPrefix, bucket, policyJSON) diff --git a/cmd/controller-handlers.go b/cmd/controller-handlers.go index 3ba72b797..c19f9c12e 100644 --- a/cmd/controller-handlers.go +++ b/cmd/controller-handlers.go @@ -16,6 +16,11 @@ package cmd +import "errors" + +// errServerNotInitialized - server not initialized. +var errServerNotInitialized = errors.New("Server not initialized, please try again.") + /// Auth operations // Login - login handler. @@ -59,7 +64,7 @@ type HealListReply struct { func (c *controllerAPIHandlers) ListObjectsHealHandler(args *HealListArgs, reply *HealListReply) error { objAPI := c.ObjectAPI() if objAPI == nil { - return errVolumeBusy + return errServerNotInitialized } if !isRPCTokenValid(args.Token) { return errInvalidToken @@ -91,11 +96,11 @@ type HealObjectArgs struct { // HealObjectReply - reply by HealObject RPC. type HealObjectReply struct{} -// HealObject - heal the object. +// HealObject - heals an object, returns nil error upon success. func (c *controllerAPIHandlers) HealObjectHandler(args *HealObjectArgs, reply *GenericReply) error { objAPI := c.ObjectAPI() if objAPI == nil { - return errVolumeBusy + return errServerNotInitialized } if !isRPCTokenValid(args.Token) { return errInvalidToken @@ -103,11 +108,11 @@ func (c *controllerAPIHandlers) HealObjectHandler(args *HealObjectArgs, reply *G return objAPI.HealObject(args.Bucket, args.Object) } -// HealObject - heal the object. +// HealDiskMetadataHandler - heals disks metadata, returns nil error upon success. func (c *controllerAPIHandlers) HealDiskMetadataHandler(args *GenericArgs, reply *GenericReply) error { objAPI := c.ObjectAPI() if objAPI == nil { - return errVolumeBusy + return errServerNotInitialized } if !isRPCTokenValid(args.Token) { return errInvalidToken diff --git a/cmd/routers.go b/cmd/routers.go index 43b1a9478..f55dfde2e 100644 --- a/cmd/routers.go +++ b/cmd/routers.go @@ -73,10 +73,6 @@ func newObjectLayer(disks, ignoredDisks []string) (ObjectLayer, error) { err = initEventNotifier(objAPI) fatalIf(err, "Unable to initialize event notification.") - // Initialize and load bucket policies. - err = initBucketPolicies(objAPI) - fatalIf(err, "Unable to load all bucket policies.") - // Success. return objAPI, nil } diff --git a/cmd/storage-errors.go b/cmd/storage-errors.go index 8e6fb65f0..787febf25 100644 --- a/cmd/storage-errors.go +++ b/cmd/storage-errors.go @@ -59,6 +59,3 @@ var errVolumeAccessDenied = errors.New("volume access denied") // errVolumeAccessDenied - cannot access file, insufficient permissions. var errFileAccessDenied = errors.New("file access denied") - -// errVolumeBusy - remote disk is not connected to yet. -var errVolumeBusy = errors.New("volume is busy") diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index 432b20221..d3ed4f5b6 100644 --- a/cmd/test-utils_test.go +++ b/cmd/test-utils_test.go @@ -1298,11 +1298,17 @@ func initTestAPIEndPoints(objLayer ObjectLayer, apiFunctions []string) http.Hand // initialize a new mux router. // goriilla/mux is the library used to register all the routes and handle them. muxRouter := router.NewRouter() + // All object storage operations are registered as HTTP handlers on `objectAPIHandlers`. // When the handlers get a HTTP request they use the underlyting ObjectLayer to perform operations. + objLayerMutex.Lock() + globalObjectAPI = objLayer + objLayerMutex.Unlock() + api := objectAPIHandlers{ - ObjectAPI: func() ObjectLayer { return objLayer }, + ObjectAPI: newObjectLayerFn, } + // API Router. apiRouter := muxRouter.NewRoute().PathPrefix("/").Subrouter() // Bucket router. diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index 0aed38594..36cbea2f9 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -127,7 +127,7 @@ func (web *webAPIHandlers) StorageInfo(r *http.Request, args *GenericArgs, reply reply.UIVersion = miniobrowser.UIVersion objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again., please try again."} } reply.StorageInfo = objectAPI.StorageInfo() return nil @@ -146,7 +146,7 @@ func (web *webAPIHandlers) MakeBucket(r *http.Request, args *MakeBucketArgs, rep reply.UIVersion = miniobrowser.UIVersion objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } if err := objectAPI.MakeBucket(args.BucketName); err != nil { return &json2.Error{Message: err.Error()} @@ -175,7 +175,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re } objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } buckets, err := objectAPI.ListBuckets() if err != nil { @@ -227,7 +227,7 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r for { objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } lo, err := objectAPI.ListObjects(args.BucketName, args.Prefix, marker, "/", 1000) if err != nil { @@ -269,7 +269,7 @@ func (web *webAPIHandlers) RemoveObject(r *http.Request, args *RemoveObjectArgs, reply.UIVersion = miniobrowser.UIVersion objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } if err := objectAPI.DeleteObject(args.BucketName, args.ObjectName); err != nil { return &json2.Error{Message: err.Error()} @@ -412,7 +412,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { objectAPI := web.ObjectAPI() if objectAPI == nil { - writeWebErrorResponse(w, errors.New("Server not initialized")) + writeWebErrorResponse(w, errors.New("Server not initialized, please try again.")) return } if _, err := objectAPI.PutObject(bucket, object, -1, r.Body, metadata); err != nil { @@ -468,7 +468,7 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) { objectAPI := web.ObjectAPI() if objectAPI == nil { - writeWebErrorResponse(w, errors.New("Server not initialized")) + writeWebErrorResponse(w, errors.New("Server not initialized, please try again.")) return } objInfo, err := objectAPI.GetObjectInfo(bucket, object) @@ -567,7 +567,7 @@ func (web *webAPIHandlers) GetBucketPolicy(r *http.Request, args *GetBucketPolic objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } policyInfo, err := readBucketAccessPolicy(objectAPI, args.BucketName) if err != nil { @@ -630,7 +630,7 @@ func (web *webAPIHandlers) SetBucketPolicy(r *http.Request, args *SetBucketPolic } objectAPI := web.ObjectAPI() if objectAPI == nil { - return &json2.Error{Message: "Server not initialized"} + return &json2.Error{Message: "Server not initialized, please try again."} } bucketP := policy.BucketPolicy(args.Policy)