From 329fae5203d35923a5a975847f48b2b6c8a32ef4 Mon Sep 17 00:00:00 2001 From: jonaustin09 Date: Mon, 24 Jun 2024 13:14:28 -0400 Subject: [PATCH 1/2] fix: Changed bucket policy validation error messages --- auth/bucket_policy.go | 28 ++++++++++--- auth/bucket_policy_actions.go | 27 +++++++----- auth/bucket_policy_effect.go | 3 +- auth/bucket_policy_principals.go | 13 +++--- auth/bucket_policy_resources.go | 14 ++----- tests/integration/group-tests.go | 2 + tests/integration/tests.go | 72 +++++++++++++++++++------------- 7 files changed, 98 insertions(+), 61 deletions(-) diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go index 67fc7f3f..ff0cebbb 100644 --- a/auth/bucket_policy.go +++ b/auth/bucket_policy.go @@ -16,12 +16,22 @@ package auth import ( "encoding/json" - "fmt" + "errors" "net/http" "github.com/versity/versitygw/s3err" ) +var ( + errResourceMismatch = errors.New("Action does not apply to any resource(s) in statement") + //lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations + errInvalidResource = errors.New("Policy has invalid resource") + //lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations + errInvalidPrincipal = errors.New("Invalid principal in policy") + //lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations + errInvalidAction = errors.New("Policy has invalid action") +) + type BucketPolicy struct { Statement []BucketPolicyItem `json:"Statement"` } @@ -75,11 +85,14 @@ func (bpi *BucketPolicyItem) Validate(bucket string, iam IAMService) error { for action := range bpi.Actions { isObjectAction := action.IsObjectAction() - if isObjectAction && !containsObjectAction { - return fmt.Errorf("unsupported object action '%v' on the specified resources", action) + if isObjectAction == nil { + break } - if !isObjectAction && !containsBucketAction { - return fmt.Errorf("unsupported bucket action '%v' on the specified resources", action) + if *isObjectAction && !containsObjectAction { + return errResourceMismatch + } + if !*isObjectAction && !containsBucketAction { + return errResourceMismatch } } @@ -108,6 +121,11 @@ func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) err return getMalformedPolicyError(err) } + if len(policy.Statement) == 0 { + //lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations + return getMalformedPolicyError(errors.New("Could not parse the policy: Statement is empty!")) + } + if err := policy.Validate(bucket, iam); err != nil { return getMalformedPolicyError(err) } diff --git a/auth/bucket_policy_actions.go b/auth/bucket_policy_actions.go index ea7988df..10ffd9e4 100644 --- a/auth/bucket_policy_actions.go +++ b/auth/bucket_policy_actions.go @@ -16,7 +16,6 @@ package auth import ( "encoding/json" - "fmt" "strings" ) @@ -119,7 +118,7 @@ var supportedObjectActionList = map[Action]struct{}{ // Validates Action: it should either wildcard match with supported actions list or be in it func (a Action) IsValid() error { if !strings.HasPrefix(string(a), "s3:") { - return fmt.Errorf("invalid action: %v", a) + return errInvalidAction } if a == AllActions { @@ -134,31 +133,39 @@ func (a Action) IsValid() error { } } - return fmt.Errorf("invalid wildcard usage: %v prefix is not in the supported actions list", pattern) + return errInvalidAction } _, found := supportedActionList[a] if !found { - return fmt.Errorf("unsupported action: %v", a) + return errInvalidAction } return nil } +func getBoolPtr(bl bool) *bool { + return &bl +} + // Checks if the action is object action -func (a Action) IsObjectAction() bool { +// nil points to 's3:*' +func (a Action) IsObjectAction() *bool { + if a == AllActions { + return nil + } if a[len(a)-1] == '*' { pattern := strings.TrimSuffix(string(a), "*") for act := range supportedObjectActionList { if strings.HasPrefix(string(act), pattern) { - return true + return getBoolPtr(true) } } - return false + return getBoolPtr(false) } _, found := supportedObjectActionList[a] - return found + return &found } func (a Action) WildCardMatch(act Action) bool { @@ -177,7 +184,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error { var err error if err = json.Unmarshal(data, &ss); err == nil { if len(ss) == 0 { - return fmt.Errorf("actions can't be empty") + return errInvalidAction } *a = make(Actions) for _, s := range ss { @@ -190,7 +197,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error { var s string if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return fmt.Errorf("actions can't be empty") + return errInvalidAction } *a = make(Actions) err = a.Add(s) diff --git a/auth/bucket_policy_effect.go b/auth/bucket_policy_effect.go index c2ccc31d..74217895 100644 --- a/auth/bucket_policy_effect.go +++ b/auth/bucket_policy_effect.go @@ -30,5 +30,6 @@ func (bpat BucketPolicyAccessType) Validate() error { return nil } - return fmt.Errorf("invalid effect: %v", bpat) + //lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations + return fmt.Errorf("Invalid effect: %v", bpat) } diff --git a/auth/bucket_policy_principals.go b/auth/bucket_policy_principals.go index 11508f2f..dd2ed9ed 100644 --- a/auth/bucket_policy_principals.go +++ b/auth/bucket_policy_principals.go @@ -16,7 +16,6 @@ package auth import ( "encoding/json" - "fmt" ) type Principals map[string]struct{} @@ -37,7 +36,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { if err = json.Unmarshal(data, &ss); err == nil { if len(ss) == 0 { - return fmt.Errorf("principals can't be empty") + return errInvalidPrincipal } *p = make(Principals) for _, s := range ss { @@ -46,7 +45,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { return nil } else if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return fmt.Errorf("principals can't be empty") + return errInvalidPrincipal } *p = make(Principals) p.Add(s) @@ -54,7 +53,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { return nil } else if err = json.Unmarshal(data, &k); err == nil { if k.AWS == "" { - return fmt.Errorf("principals can't be empty") + return errInvalidPrincipal } *p = make(Principals) p.Add(k.AWS) @@ -66,7 +65,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { } if err = json.Unmarshal(data, &sk); err == nil { if len(sk.AWS) == 0 { - return fmt.Errorf("principals can't be empty") + return errInvalidPrincipal } *p = make(Principals) for _, s := range sk.AWS { @@ -98,7 +97,7 @@ func (p Principals) Validate(iam IAMService) error { if len(p) == 1 { return nil } - return fmt.Errorf("principals should either contain * or user access keys") + return errInvalidPrincipal } accs, err := CheckIfAccountsExist(p.ToSlice(), iam) @@ -106,7 +105,7 @@ func (p Principals) Validate(iam IAMService) error { return err } if len(accs) > 0 { - return fmt.Errorf("user accounts don't exist: %v", accs) + return errInvalidPrincipal } return nil diff --git a/auth/bucket_policy_resources.go b/auth/bucket_policy_resources.go index 3c05552b..99c5635c 100644 --- a/auth/bucket_policy_resources.go +++ b/auth/bucket_policy_resources.go @@ -16,7 +16,6 @@ package auth import ( "encoding/json" - "fmt" "strings" ) @@ -30,7 +29,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error { var err error if err = json.Unmarshal(data, &ss); err == nil { if len(ss) == 0 { - return fmt.Errorf("resources can't be empty") + return errInvalidResource } *r = make(Resources) for _, s := range ss { @@ -43,7 +42,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error { var s string if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return fmt.Errorf("resources can't be empty") + return errInvalidResource } *r = make(Resources) err = r.Add(s) @@ -60,12 +59,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error { func (r Resources) Add(rc string) error { ok, pattern := isValidResource(rc) if !ok { - return fmt.Errorf("invalid resource: %v", rc) - } - - _, found := r[pattern] - if found { - return fmt.Errorf("duplicate resource: %v", rc) + return errInvalidResource } r[pattern] = struct{}{} @@ -99,7 +93,7 @@ func (r Resources) ContainsBucketPattern() bool { func (r Resources) Validate(bucket string) error { for resource := range r { if !strings.HasPrefix(resource, bucket) { - return fmt.Errorf("incorrect bucket name in %v", resource) + return errInvalidResource } } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 88ec4f2d..d12aa49b 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -286,6 +286,7 @@ func TestGetBucketAcl(s *S3Conf) { func TestPutBucketPolicy(s *S3Conf) { PutBucketPolicy_non_existing_bucket(s) + PutBucketPolicy_empty_statement(s) PutBucketPolicy_invalid_effect(s) PutBucketPolicy_empty_actions_string(s) PutBucketPolicy_empty_actions_array(s) @@ -638,6 +639,7 @@ func GetIntTests() IntTests { "GetBucketAcl_access_denied": GetBucketAcl_access_denied, "GetBucketAcl_success": GetBucketAcl_success, "PutBucketPolicy_non_existing_bucket": PutBucketPolicy_non_existing_bucket, + "PutBucketPolicy_empty_statement": PutBucketPolicy_empty_statement, "PutBucketPolicy_invalid_effect": PutBucketPolicy_invalid_effect, "PutBucketPolicy_empty_actions_string": PutBucketPolicy_empty_actions_string, "PutBucketPolicy_empty_actions_array": PutBucketPolicy_empty_actions_array, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 4b8dc588..28649739 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -6320,6 +6320,28 @@ func PutBucketPolicy_non_existing_bucket(s *S3Conf) error { }) } +func PutBucketPolicy_empty_statement(s *S3Conf) error { + testName := "PutBucketPolicy_empty_statement" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + doc := ` + { + "Statement": [] + } + ` + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ + Bucket: &bucket, + Policy: &doc, + }) + cancel() + if err := checkApiErr(err, getMalformedPolicyError("Could not parse the policy: Statement is empty!")); err != nil { + return err + } + return nil + }) +} + func PutBucketPolicy_invalid_effect(s *S3Conf) error { testName := "PutBucketPolicy_invalid_effect" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -6332,7 +6354,7 @@ func PutBucketPolicy_invalid_effect(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("invalid effect: invalid_effect")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid effect: invalid_effect")); err != nil { return err } return nil @@ -6351,7 +6373,7 @@ func PutBucketPolicy_empty_actions_string(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("actions can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid action")); err != nil { return err } return nil @@ -6370,7 +6392,7 @@ func PutBucketPolicy_empty_actions_array(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("actions can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid action")); err != nil { return err } return nil @@ -6389,7 +6411,7 @@ func PutBucketPolicy_invalid_action(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("invalid action: ListObjects")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid action")); err != nil { return err } return nil @@ -6408,7 +6430,7 @@ func PutBucketPolicy_unsupported_action(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("unsupported action: s3:PutLifecycleConfiguration")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid action")); err != nil { return err } return nil @@ -6427,7 +6449,7 @@ func PutBucketPolicy_incorrect_action_wildcard_usage(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("invalid wildcard usage: s3:hello prefix is not in the supported actions list")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid action")); err != nil { return err } return nil @@ -6446,7 +6468,7 @@ func PutBucketPolicy_empty_principals_string(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { return err } return nil @@ -6465,7 +6487,7 @@ func PutBucketPolicy_empty_principals_array(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { return err } return nil @@ -6484,7 +6506,7 @@ func PutBucketPolicy_principals_aws_struct_empty_string(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { return err } return nil @@ -6503,7 +6525,7 @@ func PutBucketPolicy_principals_aws_struct_empty_string_slice(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { return err } return nil @@ -6522,7 +6544,7 @@ func PutBucketPolicy_principals_incorrect_wildcard_usage(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("principals should either contain * or user access keys")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { return err } return nil @@ -6541,14 +6563,8 @@ func PutBucketPolicy_non_existing_principals(s *S3Conf) error { }) cancel() - apiErr1 := getMalformedPolicyError(fmt.Sprintf("user accounts don't exist: %v", []string{"a_rarely_existing_user_account_1", "a_rarely_existing_user_account_2"})) - apiErr2 := getMalformedPolicyError(fmt.Sprintf("user accounts don't exist: %v", []string{"a_rarely_existing_user_account_2", "a_rarely_existing_user_account_1"})) - - err1 := checkApiErr(err, apiErr1) - err2 := checkApiErr(err, apiErr2) - - if err1 != nil && err2 != nil { - return err1 + if err := checkApiErr(err, getMalformedPolicyError("Invalid principal in policy")); err != nil { + return err } return nil @@ -6567,7 +6583,7 @@ func PutBucketPolicy_empty_resources_string(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("resources can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid resource")); err != nil { return err } return nil @@ -6586,7 +6602,7 @@ func PutBucketPolicy_empty_resources_array(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("resources can't be empty")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid resource")); err != nil { return err } return nil @@ -6606,7 +6622,7 @@ func PutBucketPolicy_invalid_resource_prefix(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError(fmt.Sprintf("invalid resource: %v", resource[1:len(resource)-1]))); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid resource")); err != nil { return err } return nil @@ -6626,7 +6642,7 @@ func PutBucketPolicy_invalid_resource_with_starting_slash(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError(fmt.Sprintf("invalid resource: %v", resource[1:len(resource)-1]))); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid resource")); err != nil { return err } return nil @@ -6645,10 +6661,10 @@ func PutBucketPolicy_duplicate_resource(s *S3Conf) error { Policy: &doc, }) cancel() - - if err := checkApiErr(err, getMalformedPolicyError(fmt.Sprintf("duplicate resource: %v", resource[1:len(resource)-1]))); err != nil { + if err != nil { return err } + return nil }) } @@ -6666,7 +6682,7 @@ func PutBucketPolicy_incorrect_bucket_name(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError(fmt.Sprintf("incorrect bucket name in prefix-%v", bucket))); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Policy has invalid resource")); err != nil { return err } return nil @@ -6686,7 +6702,7 @@ func PutBucketPolicy_object_action_on_bucket_resource(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("unsupported object action 's3:PutObjectTagging' on the specified resources")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Action does not apply to any resource(s) in statement")); err != nil { return err } return nil @@ -6706,7 +6722,7 @@ func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error { }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("unsupported bucket action 's3:DeleteBucket' on the specified resources")); err != nil { + if err := checkApiErr(err, getMalformedPolicyError("Action does not apply to any resource(s) in statement")); err != nil { return err } return nil From 10e22e8bef26323de728e0cf405e798003602c29 Mon Sep 17 00:00:00 2001 From: Luke McCrone Date: Mon, 24 Jun 2024 16:25:03 -0300 Subject: [PATCH 2/2] test: updated test to match error change --- tests/test_aws.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_aws.sh b/tests/test_aws.sh index 6d7e0d42..a8ae22d4 100755 --- a/tests/test_aws.sh +++ b/tests/test_aws.sh @@ -782,7 +782,7 @@ EOF fail "put succeeded despite malformed policy" fi # shellcheck disable=SC2154 - [[ "$put_bucket_policy_error" == *"MalformedPolicy"*"unsupported action"* ]] || fail "invalid policy error: $put_bucket_policy_error" + [[ "$put_bucket_policy_error" == *"MalformedPolicy"*"invalid action"* ]] || fail "invalid policy error: $put_bucket_policy_error" delete_bucket_or_contents "aws" "$BUCKET_ONE_NAME" delete_test_files "$policy_file" }