improve iam error handling (#6446)

* improve iam error handling

* Delete docker/test.py

4 files changed, 142 insertions, 61 deletions

diff --git a/weed/iamapi/iamapi_handlers.go b/weed/iamapi/iamapi_handlers.go
index 5007c34c6..c8eac8ef6 100644
--- a/weed/iamapi/iamapi_handlers.go
+++ b/weed/iamapi/iamapi_handlers.go
@@ -1,31 +1,45 @@
 package iamapi
 
 import (
-	"fmt"
+	"net/http"
+
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
-	"net/http"
 )
 
-func writeIamErrorResponse(w http.ResponseWriter, r *http.Request, err error, object string, value string, msg error) {
-	errCode := err.Error()
+func newErrorResponse(errCode string, errMsg string) ErrorResponse {
 	errorResp := ErrorResponse{}
 	errorResp.Error.Type = "Sender"
 	errorResp.Error.Code = &errCode
-	if msg != nil {
-		errMsg := msg.Error()
-		errorResp.Error.Message = &errMsg
+	errorResp.Error.Message = &errMsg
+	return errorResp
+}
+
+func writeIamErrorResponse(w http.ResponseWriter, r *http.Request, iamError *IamError) {
+
+	if iamError == nil {
+		// Do nothing if there is no error
+		glog.Errorf("No error found")
+		return
 	}
-	glog.Errorf("Response %+v", err)
+
+	errCode := iamError.Code
+	errMsg := iamError.Error.Error()
+	glog.Errorf("Response %+v", errMsg)
+
+	errorResp := newErrorResponse(errCode, errMsg)
+	internalErrorResponse := newErrorResponse(iam.ErrCodeServiceFailureException, "Internal server error")
+
 	switch errCode {
 	case iam.ErrCodeNoSuchEntityException:
-		msg := fmt.Sprintf("The %s with name %s cannot be found.", object, value)
-		errorResp.Error.Message = &msg
 		s3err.WriteXMLResponse(w, r, http.StatusNotFound, errorResp)
+	case iam.ErrCodeMalformedPolicyDocumentException:
+		s3err.WriteXMLResponse(w, r, http.StatusBadRequest, errorResp)
 	case iam.ErrCodeServiceFailureException:
-		s3err.WriteXMLResponse(w, r, http.StatusInternalServerError, errorResp)
+		// We do not want to expose internal server error to the client
+		s3err.WriteXMLResponse(w, r, http.StatusInternalServerError, internalErrorResponse)
 	default:
-		s3err.WriteXMLResponse(w, r, http.StatusInternalServerError, errorResp)
+		s3err.WriteXMLResponse(w, r, http.StatusInternalServerError, internalErrorResponse)
 	}
 }
diff --git a/weed/iamapi/iamapi_management_handlers.go b/weed/iamapi/iamapi_management_handlers.go
index 6b0f9bbfc..e5c533e27 100644
--- a/weed/iamapi/iamapi_management_handlers.go
+++ b/weed/iamapi/iamapi_management_handlers.go
@@ -89,6 +89,10 @@ func MapToIdentitiesAction(action string) string {
 	}
 }
 
+const (
+	USER_DOES_NOT_EXIST = "the user with name %s cannot be found."
+)
+
 type Statement struct {
 	Effect   string   `json:"Effect"`
 	Action   []string `json:"Action"`
@@ -153,27 +157,27 @@ func (iama *IamApiServer) CreateUser(s3cfg *iam_pb.S3ApiConfiguration, values ur
 	return resp
 }
 
-func (iama *IamApiServer) DeleteUser(s3cfg *iam_pb.S3ApiConfiguration, userName string) (resp DeleteUserResponse, err error) {
+func (iama *IamApiServer) DeleteUser(s3cfg *iam_pb.S3ApiConfiguration, userName string) (resp DeleteUserResponse, err *IamError) {
 	for i, ident := range s3cfg.Identities {
 		if userName == ident.Name {
 			s3cfg.Identities = append(s3cfg.Identities[:i], s3cfg.Identities[i+1:]...)
 			return resp, nil
 		}
 	}
-	return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+	return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(USER_DOES_NOT_EXIST, userName)}
 }
 
-func (iama *IamApiServer) GetUser(s3cfg *iam_pb.S3ApiConfiguration, userName string) (resp GetUserResponse, err error) {
+func (iama *IamApiServer) GetUser(s3cfg *iam_pb.S3ApiConfiguration, userName string) (resp GetUserResponse, err *IamError) {
 	for _, ident := range s3cfg.Identities {
 		if userName == ident.Name {
 			resp.GetUserResult.User = iam.User{UserName: &ident.Name}
 			return resp, nil
 		}
 	}
-	return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+	return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(USER_DOES_NOT_EXIST, userName)}
 }
 
-func (iama *IamApiServer) UpdateUser(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp UpdateUserResponse, err error) {
+func (iama *IamApiServer) UpdateUser(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp UpdateUserResponse, err *IamError) {
 	userName := values.Get("UserName")
 	newUserName := values.Get("NewUserName")
 	if newUserName != "" {
@@ -186,7 +190,7 @@ func (iama *IamApiServer) UpdateUser(s3cfg *iam_pb.S3ApiConfiguration, values ur
 	} else {
 		return resp, nil
 	}
-	return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+	return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(USER_DOES_NOT_EXIST, userName)}
 }
 
 func GetPolicyDocument(policy *string) (policyDocument PolicyDocument, err error) {
@@ -196,12 +200,12 @@ func GetPolicyDocument(policy *string) (policyDocument PolicyDocument, err error
 	return policyDocument, err
 }
 
-func (iama *IamApiServer) CreatePolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp CreatePolicyResponse, err error) {
+func (iama *IamApiServer) CreatePolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp CreatePolicyResponse, iamError *IamError) {
 	policyName := values.Get("PolicyName")
 	policyDocumentString := values.Get("PolicyDocument")
 	policyDocument, err := GetPolicyDocument(&policyDocumentString)
 	if err != nil {
-		return CreatePolicyResponse{}, err
+		return CreatePolicyResponse{}, &IamError{Code: iam.ErrCodeMalformedPolicyDocumentException, Error: err}
 	}
 	policyId := Hash(&policyDocumentString)
 	arn := fmt.Sprintf("arn:aws:iam:::policy/%s", policyName)
@@ -212,26 +216,36 @@ func (iama *IamApiServer) CreatePolicy(s3cfg *iam_pb.S3ApiConfiguration, values
 	policyLock.Lock()
 	defer policyLock.Unlock()
 	if err = iama.s3ApiConfig.GetPolicies(&policies); err != nil {
-		return resp, err
+		return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: err}
 	}
 	policies.Policies[policyName] = policyDocument
 	if err = iama.s3ApiConfig.PutPolicies(&policies); err != nil {
-		return resp, err
+		return resp, &IamError{Code: iam.ErrCodeServiceFailureException, Error: err}
 	}
 	return resp, nil
 }
 
+type IamError struct {
+	Code  string
+	Error error
+}
+
 // https://docs.aws.amazon.com/IAM/latest/APIReference/API_PutUserPolicy.html
-func (iama *IamApiServer) PutUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp PutUserPolicyResponse, err error) {
+func (iama *IamApiServer) PutUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp PutUserPolicyResponse, iamError *IamError) {
 	userName := values.Get("UserName")
 	policyName := values.Get("PolicyName")
 	policyDocumentString := values.Get("PolicyDocument")
 	policyDocument, err := GetPolicyDocument(&policyDocumentString)
 	if err != nil {
-		return PutUserPolicyResponse{}, err
+		return PutUserPolicyResponse{}, &IamError{Code: iam.ErrCodeMalformedPolicyDocumentException, Error: err}
 	}
 	policyDocuments[policyName] = &policyDocument
-	actions := GetActions(&policyDocument)
+	actions, err := GetActions(&policyDocument)
+	if err != nil {
+		return PutUserPolicyResponse{}, &IamError{Code: iam.ErrCodeMalformedPolicyDocumentException, Error: err}
+	}
+	// Log the actions
+	glog.V(3).Infof("PutUserPolicy: actions=%v", actions)
 	for _, ident := range s3cfg.Identities {
 		if userName != ident.Name {
 			continue
@@ -239,10 +253,10 @@ func (iama *IamApiServer) PutUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
 		ident.Actions = actions
 		return resp, nil
 	}
-	return resp, fmt.Errorf("%s: the user with name %s cannot be found", iam.ErrCodeNoSuchEntityException, userName)
+	return PutUserPolicyResponse{}, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf("the user with name %s cannot be found", userName)}
 }
 
-func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp GetUserPolicyResponse, err error) {
+func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp GetUserPolicyResponse, err *IamError) {
 	userName := values.Get("UserName")
 	policyName := values.Get("PolicyName")
 	for _, ident := range s3cfg.Identities {
@@ -253,7 +267,7 @@ func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
 		resp.GetUserPolicyResult.UserName = userName
 		resp.GetUserPolicyResult.PolicyName = policyName
 		if len(ident.Actions) == 0 {
-			return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+			return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: errors.New("no actions found")}
 		}
 
 		policyDocument := PolicyDocument{Version: policyDocumentVersion}
@@ -293,10 +307,10 @@ func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
 		resp.GetUserPolicyResult.PolicyDocument = policyDocument.String()
 		return resp, nil
 	}
-	return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+	return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(USER_DOES_NOT_EXIST, userName)}
 }
 
-func (iama *IamApiServer) DeleteUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp PutUserPolicyResponse, err error) {
+func (iama *IamApiServer) DeleteUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp PutUserPolicyResponse, err *IamError) {
 	userName := values.Get("UserName")
 	for i, ident := range s3cfg.Identities {
 		if ident.Name == userName {
@@ -304,27 +318,27 @@ func (iama *IamApiServer) DeleteUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, val
 			return resp, nil
 		}
 	}
-	return resp, fmt.Errorf(iam.ErrCodeNoSuchEntityException)
+	return resp, &IamError{Code: iam.ErrCodeNoSuchEntityException, Error: fmt.Errorf(USER_DOES_NOT_EXIST, userName)}
 }
 
-func GetActions(policy *PolicyDocument) (actions []string) {
+func GetActions(policy *PolicyDocument) ([]string, error) {
+	var actions []string
+
 	for _, statement := range policy.Statement {
 		if statement.Effect != "Allow" {
-			continue
+			return nil, fmt.Errorf("not a valid effect: '%s'. Only 'Allow' is possible", statement.Effect)
 		}
 		for _, resource := range statement.Resource {
 			// Parse "arn:aws:s3:::my-bucket/shared/*"
 			res := strings.Split(resource, ":")
 			if len(res) != 6 || res[0] != "arn" || res[1] != "aws" || res[2] != "s3" {
-				glog.Infof("not match resource: %s", res)
-				continue
+				return nil, fmt.Errorf("not a valid resource: '%s'. Expected prefix 'arn:aws:s3'", res)
 			}
 			for _, action := range statement.Action {
 				// Parse "s3:Get*"
 				act := strings.Split(action, ":")
 				if len(act) != 2 || act[0] != "s3" {
-					glog.Infof("not match action: %s", act)
-					continue
+					return nil, fmt.Errorf("not a valid action: '%s'. Expected prefix 's3:'", act)
 				}
 				statementAction := MapToStatementAction(act[1])
 				if res[5] == "*" {
@@ -341,7 +355,7 @@ func GetActions(policy *PolicyDocument) (actions []string) {
 			}
 		}
 	}
-	return actions
+	return actions, nil
 }
 
 func (iama *IamApiServer) CreateAccessKey(s3cfg *iam_pb.S3ApiConfiguration, values url.Values) (resp CreateAccessKeyResponse) {
@@ -438,7 +452,7 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) {
 
 	glog.V(4).Infof("DoActions: %+v", values)
 	var response interface{}
-	var err error
+	var iamError *IamError
 	changed := true
 	switch r.Form.Get("Action") {
 	case "ListUsers":
@@ -452,24 +466,24 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) {
 		response = iama.CreateUser(s3cfg, values)
 	case "GetUser":
 		userName := values.Get("UserName")
-		response, err = iama.GetUser(s3cfg, userName)
-		if err != nil {
-			writeIamErrorResponse(w, r, err, "user", userName, nil)
+		response, iamError = iama.GetUser(s3cfg, userName)
+		if iamError != nil {
+			writeIamErrorResponse(w, r, iamError)
 			return
 		}
 		changed = false
 	case "UpdateUser":
-		response, err = iama.UpdateUser(s3cfg, values)
-		if err != nil {
-			glog.Errorf("UpdateUser: %+v", err)
+		response, iamError = iama.UpdateUser(s3cfg, values)
+		if iamError != nil {
+			glog.Errorf("UpdateUser: %+v", iamError.Error)
 			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
 			return
 		}
 	case "DeleteUser":
 		userName := values.Get("UserName")
-		response, err = iama.DeleteUser(s3cfg, userName)
-		if err != nil {
-			writeIamErrorResponse(w, r, err, "user", userName, nil)
+		response, iamError = iama.DeleteUser(s3cfg, userName)
+		if iamError != nil {
+			writeIamErrorResponse(w, r, iamError)
 			return
 		}
 	case "CreateAccessKey":
@@ -479,29 +493,31 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) {
 		handleImplicitUsername(r, values)
 		response = iama.DeleteAccessKey(s3cfg, values)
 	case "CreatePolicy":
-		response, err = iama.CreatePolicy(s3cfg, values)
-		if err != nil {
-			glog.Errorf("CreatePolicy:  %+v", err)
+		response, iamError = iama.CreatePolicy(s3cfg, values)
+		if iamError != nil {
+			glog.Errorf("CreatePolicy:  %+v", iamError.Error)
 			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
 			return
 		}
 	case "PutUserPolicy":
-		response, err = iama.PutUserPolicy(s3cfg, values)
-		if err != nil {
-			glog.Errorf("PutUserPolicy:  %+v", err)
-			s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
+		var iamError *IamError
+		response, iamError = iama.PutUserPolicy(s3cfg, values)
+		if iamError != nil {
+			glog.Errorf("PutUserPolicy:  %+v", iamError.Error)
+
+			writeIamErrorResponse(w, r, iamError)
 			return
 		}
 	case "GetUserPolicy":
-		response, err = iama.GetUserPolicy(s3cfg, values)
-		if err != nil {
-			writeIamErrorResponse(w, r, err, "user", values.Get("UserName"), nil)
+		response, iamError = iama.GetUserPolicy(s3cfg, values)
+		if iamError != nil {
+			writeIamErrorResponse(w, r, iamError)
 			return
 		}
 		changed = false
 	case "DeleteUserPolicy":
-		if response, err = iama.DeleteUserPolicy(s3cfg, values); err != nil {
-			writeIamErrorResponse(w, r, err, "user", values.Get("UserName"), nil)
+		if response, iamError = iama.DeleteUserPolicy(s3cfg, values); iamError != nil {
+			writeIamErrorResponse(w, r, iamError)
 			return
 		}
 	default:
@@ -515,7 +531,8 @@ func (iama *IamApiServer) DoActions(w http.ResponseWriter, r *http.Request) {
 	if changed {
 		err := iama.s3ApiConfig.PutS3ApiConfiguration(s3cfg)
 		if err != nil {
-			writeIamErrorResponse(w, r, fmt.Errorf(iam.ErrCodeServiceFailureException), "", "", err)
+			var iamError = IamError{Code: iam.ErrCodeServiceFailureException, Error: err}
+			writeIamErrorResponse(w, r, &iamError)
 			return
 		}
 	}
diff --git a/weed/iamapi/iamapi_test.go b/weed/iamapi/iamapi_test.go
index f32e1ac51..067209eb5 100644
--- a/weed/iamapi/iamapi_test.go
+++ b/weed/iamapi/iamapi_test.go
@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"regexp"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -152,6 +153,53 @@ func TestPutUserPolicy(t *testing.T) {
 	assert.Equal(t, http.StatusOK, response.Code)
 }
 
+func TestPutUserPolicyError(t *testing.T) {
+	userName := aws.String("InvalidUser")
+	params := &iam.PutUserPolicyInput{
+		UserName:   userName,
+		PolicyName: aws.String("S3-read-only-example-bucket"),
+		PolicyDocument: aws.String(
+			`{
+				  "Version": "2012-10-17",
+				  "Statement": [
+					{
+					  "Effect": "Allow",
+					  "Action": [
+						"s3:Get*",
+						"s3:List*"
+					  ],
+					  "Resource": [
+						"arn:aws:s3:::EXAMPLE-BUCKET",
+						"arn:aws:s3:::EXAMPLE-BUCKET/*"
+					  ]
+					}
+				  ]
+			}`),
+	}
+	req, _ := iam.New(session.New()).PutUserPolicyRequest(params)
+	_ = req.Build()
+	response, err := executeRequest(req.HTTPRequest, nil)
+	assert.Equal(t, nil, err)
+	assert.Equal(t, http.StatusNotFound, response.Code)
+
+	expectedMessage := "the user with name InvalidUser cannot be found"
+	expectedCode := "NoSuchEntity"
+
+	code, message := extractErrorCodeAndMessage(response)
+
+	assert.Equal(t, expectedMessage, message)
+	assert.Equal(t, expectedCode, code)
+}
+
+func extractErrorCodeAndMessage(response *httptest.ResponseRecorder) (string, string) {
+	pattern := `<Error><Code>(.*)</Code><Message>(.*)</Message><Type>(.*)</Type></Error>`
+	re := regexp.MustCompile(pattern)
+
+	code := re.FindStringSubmatch(response.Body.String())[1]
+	message := re.FindStringSubmatch(response.Body.String())[2]
+	return code, message
+}
+
 func TestGetUserPolicy(t *testing.T) {
 	userName := aws.String("Test")
 	params := &iam.GetUserPolicyInput{UserName: userName, PolicyName: aws.String("S3-read-only-example-bucket")}
diff --git a/weed/s3api/auth_credentials_test.go b/weed/s3api/auth_credentials_test.go
index 1d9f1a95f..dbc431332 100644
--- a/weed/s3api/auth_credentials_test.go
+++ b/weed/s3api/auth_credentials_test.go
@@ -80,8 +80,10 @@ func TestCanDo(t *testing.T) {
 	}
 	// object specific
 	assert.Equal(t, true, ident1.canDo(ACTION_WRITE, "bucket1", "/a/b/c/d.txt"))
+	assert.Equal(t, true, ident1.canDo(ACTION_WRITE, "bucket1", "/a/b/c/d/e.txt"))
 	assert.Equal(t, false, ident1.canDo(ACTION_DELETE_BUCKET, "bucket1", ""))
 	assert.Equal(t, false, ident1.canDo(ACTION_WRITE, "bucket1", "/a/b/other/some"), "action without *")
+	assert.Equal(t, false, ident1.canDo(ACTION_WRITE, "bucket1", "/a/b/*"), "action on parent directory")
 
 	// bucket specific
 	ident2 := &Identity{