Add more context aware error for policy parsing errors (#8726)

In existing functionality we simply return a generic
error such as "MalformedPolicy" which indicates just
a generic string "invalid resource" which is not very
meaningful when there might be multiple types of errors
during policy parsing. This PR ensures that we send
these errors back to client to indicate the actual
error, brings in two concrete types such as

 - iampolicy.Error
 - policy.Error

Refer #8202

cmd/admin-handlers-users.go

@@ -484,7 +484,7 @@ func (a adminAPIHandlers) AddCannedPolicy(w http.ResponseWriter, r *http.Request
 
 	iamPolicy, err := iampolicy.ParseConfig(io.LimitReader(r.Body, r.ContentLength))
 	if err != nil {
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrMalformedPolicy), r.URL)
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
 	}
 

cmd/admin-handlers.go

@@ -979,6 +979,12 @@ func toAdminAPIErr(ctx context.Context, err error) APIError {
 
 	var apiErr APIError
 	switch e := err.(type) {
+	case iampolicy.Error:
+		apiErr = APIError{
+			Code:           "XMinioMalformedIAMPolicy",
+			Description:    e.Error(),
+			HTTPStatusCode: http.StatusBadRequest,
+		}
 	case config.Error:
 		apiErr = APIError{
 			Code:           "XMinioConfigError",

cmd/api-errors.go

@@ -34,6 +34,7 @@ import (
 	"github.com/minio/minio/pkg/auth"
 	"github.com/minio/minio/pkg/event"
 	"github.com/minio/minio/pkg/hash"
+	"github.com/minio/minio/pkg/policy"
 )
 
 // APIError structure
@@ -1772,6 +1773,12 @@ func toAPIError(ctx context.Context, err error) APIError {
 		// their internal error types. This code is only
 		// useful with gateway implementations.
 		switch e := err.(type) {
+		case policy.Error:
+			apiErr = APIError{
+				Code:           "MalformedPolicy",
+				Description:    e.Error(),
+				HTTPStatusCode: http.StatusBadRequest,
+			}
 		case crypto.Error:
 			apiErr = APIError{
 				Code:           "XKMSInternalError",

cmd/bucket-policy-handlers.go

@@ -77,7 +77,7 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht
 
 	bucketPolicy, err := policy.ParseConfig(io.LimitReader(r.Body, r.ContentLength), bucket)
 	if err != nil {
-		writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrMalformedPolicy), r.URL, guessIsBrowserReq(r))
+		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
 		return
 	}
 

cmd/web-handlers.go

@@ -2106,11 +2106,7 @@ func toWebAPIError(ctx context.Context, err error) APIError {
 
 	// Log unexpected and unhandled errors.
 	logger.LogIf(ctx, err)
-	return APIError{
+	return toAPIError(ctx, err)
-		Code:           "InternalError",
-		HTTPStatusCode: http.StatusInternalServerError,
-		Description:    err.Error(),
-	}
 }
 
 // writeWebErrorResponse - set HTTP status code and write error description to the body.

pkg/iam/policy/action.go

@@ -18,7 +18,6 @@ package iampolicy
 
 import (
 	"encoding/json"
-	"fmt"
 
 	"github.com/minio/minio/pkg/policy/condition"
 	"github.com/minio/minio/pkg/wildcard"
@@ -177,7 +176,7 @@ func (action Action) MarshalJSON() ([]byte, error) {
 		return json.Marshal(string(action))
 	}
 
-	return nil, fmt.Errorf("invalid action '%v'", action)
+	return nil, Errorf("invalid action '%v'", action)
 }
 
 // UnmarshalJSON - decodes JSON data to Action.
@@ -190,7 +189,7 @@ func (action *Action) UnmarshalJSON(data []byte) error {
 
 	a := Action(s)
 	if !a.IsValid() {
-		return fmt.Errorf("invalid action '%v'", s)
+		return Errorf("invalid action '%v'", s)
 	}
 
 	*action = a
@@ -209,7 +208,7 @@ func parseAction(s string) (Action, error) {
 		return action, nil
 	}
 
-	return action, fmt.Errorf("unsupported action '%v'", s)
+	return action, Errorf("unsupported action '%v'", s)
 }
 
 // actionConditionKeyMap - holds mapping of supported condition key for an action.

pkg/iam/policy/actionset.go

@@ -58,7 +58,7 @@ func (actionSet ActionSet) Intersection(sset ActionSet) ActionSet {
 // MarshalJSON - encodes ActionSet to JSON data.
 func (actionSet ActionSet) MarshalJSON() ([]byte, error) {
 	if len(actionSet) == 0 {
-		return nil, fmt.Errorf("empty action set")
+		return nil, Errorf("empty action set")
 	}
 
 	return json.Marshal(actionSet.ToSlice())
@@ -92,7 +92,7 @@ func (actionSet *ActionSet) UnmarshalJSON(data []byte) error {
 	}
 
 	if len(sset) == 0 {
-		return fmt.Errorf("empty action set")
+		return Errorf("empty action set")
 	}
 
 	*actionSet = make(ActionSet)

pkg/iam/policy/admin-action.go

@@ -17,8 +17,6 @@
 package iampolicy
 
 import (
-	"fmt"
-
 	"github.com/minio/minio/pkg/policy/condition"
 )
 
@@ -119,7 +117,7 @@ func parseAdminAction(s string) (AdminAction, error) {
 		return action, nil
 	}
 
-	return action, fmt.Errorf("unsupported action '%v'", s)
+	return action, Errorf("unsupported action '%v'", s)
 }
 
 // IsValid - checks if action is valid or not.

pkg/iam/policy/error.go

@@ -0,0 +1,39 @@
+/*
+ * MinIO Cloud Storage, (C) 2019 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 iampolicy
+
+import "fmt"
+
+// Error generic iam policy error type
+type Error struct {
+	Err string
+}
+
+// Errorf - formats according to a format specifier and returns
+// the string as a value that satisfies error of type iampolicy.Error
+func Errorf(format string, a ...interface{}) error {
+	return Error{Err: fmt.Sprintf(format, a...)}
+}
+
+// New initializes a new Error
+func New(err string) error {
+	return Error{Err: err}
+}
+
+func (e Error) Error() string {
+	return e.Err
+}

pkg/iam/policy/policy.go

@@ -18,7 +18,6 @@ package iampolicy
 
 import (
 	"encoding/json"
-	"fmt"
 	"io"
 
 	"github.com/minio/minio/pkg/policy"
@@ -81,7 +80,7 @@ func (iamp Policy) IsEmpty() bool {
 // isValid - checks if Policy is valid or not.
 func (iamp Policy) isValid() error {
 	if iamp.Version != DefaultVersion && iamp.Version != "" {
-		return fmt.Errorf("invalid version '%v'", iamp.Version)
+		return Errorf("invalid version '%v'", iamp.Version)
 	}
 
 	for _, statement := range iamp.Statements {
@@ -106,7 +105,7 @@ func (iamp Policy) isValid() error {
 				continue
 			}
 
-			return fmt.Errorf("duplicate actions %v, resources %v found in statements %v, %v",
+			return Errorf("duplicate actions %v, resources %v found in statements %v, %v",
 				actions, resources, iamp.Statements[i], statement)
 		}
 	}

pkg/iam/policy/resource.go

@@ -18,7 +18,6 @@ package iampolicy
 
 import (
 	"encoding/json"
-	"fmt"
 	"path"
 	"strings"
 
@@ -66,7 +65,7 @@ func (r Resource) Match(resource string, conditionValues map[string][]string) bo
 // MarshalJSON - encodes Resource to JSON data.
 func (r Resource) MarshalJSON() ([]byte, error) {
 	if !r.IsValid() {
-		return nil, fmt.Errorf("invalid resource %v", r)
+		return nil, Errorf("invalid resource %v", r)
 	}
 
 	return json.Marshal(r.String())
@@ -96,7 +95,7 @@ func (r *Resource) UnmarshalJSON(data []byte) error {
 // Validate - validates Resource is for given bucket or not.
 func (r Resource) Validate() error {
 	if !r.IsValid() {
-		return fmt.Errorf("invalid resource")
+		return Errorf("invalid resource")
 	}
 	return nil
 }
@@ -104,14 +103,14 @@ func (r Resource) Validate() error {
 // parseResource - parses string to Resource.
 func parseResource(s string) (Resource, error) {
 	if !strings.HasPrefix(s, ResourceARNPrefix) {
-		return Resource{}, fmt.Errorf("invalid resource '%v'", s)
+		return Resource{}, Errorf("invalid resource '%v'", s)
 	}
 
 	pattern := strings.TrimPrefix(s, ResourceARNPrefix)
 	tokens := strings.SplitN(pattern, "/", 2)
 	bucketName := tokens[0]
 	if bucketName == "" {
-		return Resource{}, fmt.Errorf("invalid resource format '%v'", s)
+		return Resource{}, Errorf("invalid resource format '%v'", s)
 	}
 
 	return Resource{

pkg/iam/policy/resourceset.go

@@ -69,7 +69,7 @@ func (resourceSet ResourceSet) Intersection(sset ResourceSet) ResourceSet {
 // MarshalJSON - encodes ResourceSet to JSON data.
 func (resourceSet ResourceSet) MarshalJSON() ([]byte, error) {
 	if len(resourceSet) == 0 {
-		return nil, fmt.Errorf("empty resource set")
+		return nil, Errorf("empty resource set")
 	}
 
 	resources := []Resource{}
@@ -116,7 +116,7 @@ func (resourceSet *ResourceSet) UnmarshalJSON(data []byte) error {
 		}
 
 		if _, found := (*resourceSet)[resource]; found {
-			return fmt.Errorf("duplicate resource '%v' found", s)
+			return Errorf("duplicate resource '%v' found", s)
 		}
 
 		resourceSet.Add(resource)

pkg/iam/policy/statement.go

@@ -18,7 +18,6 @@ package iampolicy
 
 import (
 	"encoding/json"
-	"fmt"
 	"strings"
 
 	"github.com/minio/minio/pkg/policy"
@@ -74,11 +73,11 @@ func (statement Statement) isAdmin() bool {
 // isValid - checks whether statement is valid or not.
 func (statement Statement) isValid() error {
 	if !statement.Effect.IsValid() {
-		return fmt.Errorf("invalid Effect %v", statement.Effect)
+		return Errorf("invalid Effect %v", statement.Effect)
 	}
 
 	if len(statement.Actions) == 0 {
-		return fmt.Errorf("Action must not be empty")
+		return Errorf("Action must not be empty")
 	}
 
 	if statement.isAdmin() {
@@ -86,14 +85,14 @@ func (statement Statement) isValid() error {
 			keys := statement.Conditions.Keys()
 			keyDiff := keys.Difference(adminActionConditionKeyMap[action])
 			if !keyDiff.IsEmpty() {
-				return fmt.Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
+				return Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
 			}
 		}
 		return nil
 	}
 
 	if len(statement.Resources) == 0 {
-		return fmt.Errorf("Resource must not be empty")
+		return Errorf("Resource must not be empty")
 	}
 
 	if err := statement.Resources.Validate(); err != nil {
@@ -102,13 +101,13 @@ func (statement Statement) isValid() error {
 
 	for action := range statement.Actions {
 		if !statement.Resources.objectResourceExists() && !statement.Resources.bucketResourceExists() {
-			return fmt.Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
+			return Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
 		}
 
 		keys := statement.Conditions.Keys()
 		keyDiff := keys.Difference(actionConditionKeyMap[action])
 		if !keyDiff.IsEmpty() {
-			return fmt.Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
+			return Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
 		}
 	}
 

pkg/policy/action.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 
 	"github.com/minio/minio/pkg/policy/condition"
 )
@@ -157,7 +156,7 @@ func (action Action) MarshalJSON() ([]byte, error) {
 		return json.Marshal(string(action))
 	}
 
-	return nil, fmt.Errorf("invalid action '%v'", action)
+	return nil, Errorf("invalid action '%v'", action)
 }
 
 // UnmarshalJSON - decodes JSON data to Action.
@@ -170,7 +169,7 @@ func (action *Action) UnmarshalJSON(data []byte) error {
 
 	a := Action(s)
 	if !a.IsValid() {
-		return fmt.Errorf("invalid action '%v'", s)
+		return Errorf("invalid action '%v'", s)
 	}
 
 	*action = a
@@ -185,7 +184,7 @@ func parseAction(s string) (Action, error) {
 		return action, nil
 	}
 
-	return action, fmt.Errorf("unsupported action '%v'", s)
+	return action, Errorf("unsupported action '%v'", s)
 }
 
 // actionConditionKeyMap - holds mapping of supported condition key for an action.

pkg/policy/actionset.go

@@ -53,7 +53,7 @@ func (actionSet ActionSet) Intersection(sset ActionSet) ActionSet {
 // MarshalJSON - encodes ActionSet to JSON data.
 func (actionSet ActionSet) MarshalJSON() ([]byte, error) {
 	if len(actionSet) == 0 {
-		return nil, fmt.Errorf("empty action set")
+		return nil, Errorf("empty actions not allowed")
 	}
 
 	return json.Marshal(actionSet.ToSlice())
@@ -87,7 +87,7 @@ func (actionSet *ActionSet) UnmarshalJSON(data []byte) error {
 	}
 
 	if len(sset) == 0 {
-		return fmt.Errorf("empty action set")
+		return Errorf("empty actions not allowed")
 	}
 
 	*actionSet = make(ActionSet)

pkg/policy/effect.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 )
 
 // Effect - policy statement effect Allow or Deny.
@@ -54,7 +53,7 @@ func (effect Effect) IsValid() bool {
 // MarshalJSON - encodes Effect to JSON data.
 func (effect Effect) MarshalJSON() ([]byte, error) {
 	if !effect.IsValid() {
-		return nil, fmt.Errorf("invalid effect '%v'", effect)
+		return nil, Errorf("invalid effect '%v'", effect)
 	}
 
 	return json.Marshal(string(effect))
@@ -69,7 +68,7 @@ func (effect *Effect) UnmarshalJSON(data []byte) error {
 
 	e := Effect(s)
 	if !e.IsValid() {
-		return fmt.Errorf("invalid effect '%v'", s)
+		return Errorf("invalid effect '%v'", s)
 	}
 
 	*effect = e

pkg/policy/error.go

@@ -0,0 +1,39 @@
+/*
+ * MinIO Cloud Storage, (C) 2019 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 policy
+
+import "fmt"
+
+// Error generic policy parse error type
+type Error struct {
+	Err string
+}
+
+// Errorf - formats according to a format specifier and returns
+// the string as a value that satisfies error of type policy.Error
+func Errorf(format string, a ...interface{}) error {
+	return Error{Err: fmt.Sprintf(format, a...)}
+}
+
+// New initializes a new Error
+func New(err string) error {
+	return Error{Err: err}
+}
+
+func (e Error) Error() string {
+	return e.Err
+}

pkg/policy/id.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 	"unicode/utf8"
 )
 
@@ -33,7 +32,7 @@ func (id ID) IsValid() bool {
 // MarshalJSON - encodes ID to JSON data.
 func (id ID) MarshalJSON() ([]byte, error) {
 	if !id.IsValid() {
-		return nil, fmt.Errorf("invalid ID %v", id)
+		return nil, Errorf("invalid ID %v", id)
 	}
 
 	return json.Marshal(string(id))
@@ -48,7 +47,7 @@ func (id *ID) UnmarshalJSON(data []byte) error {
 
 	i := ID(s)
 	if !i.IsValid() {
-		return fmt.Errorf("invalid ID %v", s)
+		return Errorf("invalid ID %v", s)
 	}
 
 	*id = i

pkg/policy/policy.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 	"io"
 )
 
@@ -78,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 && policy.Version != "" {
-		return fmt.Errorf("invalid version '%v'", policy.Version)
+		return Errorf("invalid version '%v'", policy.Version)
 	}
 
 	for _, statement := range policy.Statements {
@@ -108,7 +107,7 @@ func (policy Policy) isValid() error {
 				continue
 			}
 
-			return fmt.Errorf("duplicate principal %v, actions %v, resouces %v found in statements %v, %v",
+			return Errorf("duplicate principal %v, actions %v, resouces %v found in statements %v, %v",
 				principals, actions, resources, policy.Statements[i], statement)
 		}
 	}

pkg/policy/principal.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 
 	"github.com/minio/minio-go/v6/pkg/set"
 	"github.com/minio/minio/pkg/wildcard"
@@ -42,7 +41,7 @@ func (p Principal) Intersection(principal Principal) set.StringSet {
 // MarshalJSON - encodes Principal to JSON data.
 func (p Principal) MarshalJSON() ([]byte, error) {
 	if !p.IsValid() {
-		return nil, fmt.Errorf("invalid principal %v", p)
+		return nil, Errorf("invalid principal %v", p)
 	}
 
 	// subtype to avoid recursive call to MarshalJSON()
@@ -75,7 +74,7 @@ func (p *Principal) UnmarshalJSON(data []byte) error {
 		}
 
 		if s != "*" {
-			return fmt.Errorf("invalid principal '%v'", s)
+			return Errorf("invalid principal '%v'", s)
 		}
 
 		sp.AWS = set.CreateStringSet("*")

pkg/policy/resource.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 	"strings"
 
 	"github.com/minio/minio/pkg/policy/condition"
@@ -63,7 +62,7 @@ func (r Resource) Match(resource string, conditionValues map[string][]string) bo
 // MarshalJSON - encodes Resource to JSON data.
 func (r Resource) MarshalJSON() ([]byte, error) {
 	if !r.IsValid() {
-		return nil, fmt.Errorf("invalid resource %v", r)
+		return nil, Errorf("invalid resource %v", r)
 	}
 
 	return json.Marshal(r.String())
@@ -93,11 +92,11 @@ func (r *Resource) UnmarshalJSON(data []byte) error {
 // Validate - validates Resource is for given bucket or not.
 func (r Resource) Validate(bucketName string) error {
 	if !r.IsValid() {
-		return fmt.Errorf("invalid resource")
+		return Errorf("invalid resource")
 	}
 
 	if !wildcard.Match(r.BucketName, bucketName) {
-		return fmt.Errorf("bucket name does not match")
+		return Errorf("bucket name does not match")
 	}
 
 	return nil
@@ -106,14 +105,14 @@ func (r Resource) Validate(bucketName string) error {
 // parseResource - parses string to Resource.
 func parseResource(s string) (Resource, error) {
 	if !strings.HasPrefix(s, ResourceARNPrefix) {
-		return Resource{}, fmt.Errorf("invalid resource '%v'", s)
+		return Resource{}, Errorf("invalid resource '%v'", s)
 	}
 
 	pattern := strings.TrimPrefix(s, ResourceARNPrefix)
 	tokens := strings.SplitN(pattern, "/", 2)
 	bucketName := tokens[0]
 	if bucketName == "" {
-		return Resource{}, fmt.Errorf("invalid resource format '%v'", s)
+		return Resource{}, Errorf("invalid resource format '%v'", s)
 	}
 
 	return Resource{

pkg/policy/resourceset.go

@@ -69,7 +69,7 @@ func (resourceSet ResourceSet) Intersection(sset ResourceSet) ResourceSet {
 // MarshalJSON - encodes ResourceSet to JSON data.
 func (resourceSet ResourceSet) MarshalJSON() ([]byte, error) {
 	if len(resourceSet) == 0 {
-		return nil, fmt.Errorf("empty resource set")
+		return nil, Errorf("empty resources not allowed")
 	}
 
 	resources := []Resource{}
@@ -116,7 +116,7 @@ func (resourceSet *ResourceSet) UnmarshalJSON(data []byte) error {
 		}
 
 		if _, found := (*resourceSet)[resource]; found {
-			return fmt.Errorf("duplicate resource '%v' found", s)
+			return Errorf("duplicate resource '%v' found", s)
 		}
 
 		resourceSet.Add(resource)

pkg/policy/statement.go

@@ -18,7 +18,6 @@ package policy
 
 import (
 	"encoding/json"
-	"fmt"
 	"strings"
 
 	"github.com/minio/minio/pkg/policy/condition"
@@ -67,36 +66,36 @@ func (statement Statement) IsAllowed(args Args) bool {
 // isValid - checks whether statement is valid or not.
 func (statement Statement) isValid() error {
 	if !statement.Effect.IsValid() {
-		return fmt.Errorf("invalid Effect %v", statement.Effect)
+		return Errorf("invalid Effect %v", statement.Effect)
 	}
 
 	if !statement.Principal.IsValid() {
-		return fmt.Errorf("invalid Principal %v", statement.Principal)
+		return Errorf("invalid Principal %v", statement.Principal)
 	}
 
 	if len(statement.Actions) == 0 {
-		return fmt.Errorf("Action must not be empty")
+		return Errorf("Action must not be empty")
 	}
 
 	if len(statement.Resources) == 0 {
-		return fmt.Errorf("Resource must not be empty")
+		return Errorf("Resource must not be empty")
 	}
 
 	for action := range statement.Actions {
 		if action.isObjectAction() {
 			if !statement.Resources.objectResourceExists() {
-				return fmt.Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
+				return Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
 			}
 		} else {
 			if !statement.Resources.bucketResourceExists() {
-				return fmt.Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
+				return Errorf("unsupported Resource found %v for action %v", statement.Resources, action)
 			}
 		}
 
 		keys := statement.Conditions.Keys()
 		keyDiff := keys.Difference(actionConditionKeyMap[action])
 		if !keyDiff.IsEmpty() {
-			return fmt.Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
+			return Errorf("unsupported condition keys '%v' used for action '%v'", keyDiff, action)
 		}
 	}