diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go index a29e6fa..9cd7991 100644 --- a/auth/bucket_policy.go +++ b/auth/bucket_policy.go @@ -22,14 +22,20 @@ import ( "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 policyErr string + +func (p policyErr) Error() string { + return string(p) +} + +const ( + policyErrResourceMismatch = policyErr("Action does not apply to any resource(s) in statement") + policyErrInvalidResource = policyErr("Policy has invalid resource") + policyErrInvalidPrincipal = policyErr("Invalid principal in policy") + policyErrInvalidAction = policyErr("Policy has invalid action") + policyErrInvalidPolicy = policyErr("This policy contains invalid Json") + policyErrInvalidFirstChar = policyErr("Policies must be valid JSON and the first byte must be '{'") + policyErrEmptyStatement = policyErr("Could not parse the policy: Statement is empty!") ) type BucketPolicy struct { @@ -90,10 +96,10 @@ func (bpi *BucketPolicyItem) Validate(bucket string, iam IAMService) error { break } if *isObjectAction && !containsObjectAction { - return errResourceMismatch + return policyErrResourceMismatch } if !*isObjectAction && !containsBucketAction { - return errResourceMismatch + return policyErrResourceMismatch } } @@ -117,14 +123,20 @@ func getMalformedPolicyError(err error) error { } func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) error { + if len(policyBin) == 0 || policyBin[0] != '{' { + return getMalformedPolicyError(policyErrInvalidFirstChar) + } var policy BucketPolicy if err := json.Unmarshal(policyBin, &policy); err != nil { - return getMalformedPolicyError(err) + var pe policyErr + if errors.As(err, &pe) { + return getMalformedPolicyError(err) + } + return getMalformedPolicyError(policyErrInvalidPolicy) } 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!")) + return getMalformedPolicyError(policyErrEmptyStatement) } if err := policy.Validate(bucket, iam); err != nil { diff --git a/auth/bucket_policy_actions.go b/auth/bucket_policy_actions.go index 7e468fd..3e7d7d8 100644 --- a/auth/bucket_policy_actions.go +++ b/auth/bucket_policy_actions.go @@ -125,7 +125,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 errInvalidAction + return policyErrInvalidAction } if a == AllActions { @@ -140,12 +140,12 @@ func (a Action) IsValid() error { } } - return errInvalidAction + return policyErrInvalidAction } _, found := supportedActionList[a] if !found { - return errInvalidAction + return policyErrInvalidAction } return nil } @@ -191,7 +191,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error { var err error if err = json.Unmarshal(data, &ss); err == nil { if len(ss) == 0 { - return errInvalidAction + return policyErrInvalidAction } *a = make(Actions) for _, s := range ss { @@ -204,7 +204,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error { var s string if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return errInvalidAction + return policyErrInvalidAction } *a = make(Actions) err = a.Add(s) diff --git a/auth/bucket_policy_principals.go b/auth/bucket_policy_principals.go index dd2ed9e..828db29 100644 --- a/auth/bucket_policy_principals.go +++ b/auth/bucket_policy_principals.go @@ -36,7 +36,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { if err = json.Unmarshal(data, &ss); err == nil { if len(ss) == 0 { - return errInvalidPrincipal + return policyErrInvalidPrincipal } *p = make(Principals) for _, s := range ss { @@ -45,7 +45,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { return nil } else if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return errInvalidPrincipal + return policyErrInvalidPrincipal } *p = make(Principals) p.Add(s) @@ -53,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 errInvalidPrincipal + return policyErrInvalidPrincipal } *p = make(Principals) p.Add(k.AWS) @@ -65,7 +65,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error { } if err = json.Unmarshal(data, &sk); err == nil { if len(sk.AWS) == 0 { - return errInvalidPrincipal + return policyErrInvalidPrincipal } *p = make(Principals) for _, s := range sk.AWS { @@ -97,7 +97,7 @@ func (p Principals) Validate(iam IAMService) error { if len(p) == 1 { return nil } - return errInvalidPrincipal + return policyErrInvalidPrincipal } accs, err := CheckIfAccountsExist(p.ToSlice(), iam) @@ -105,7 +105,7 @@ func (p Principals) Validate(iam IAMService) error { return err } if len(accs) > 0 { - return errInvalidPrincipal + return policyErrInvalidPrincipal } return nil diff --git a/auth/bucket_policy_resources.go b/auth/bucket_policy_resources.go index e105e95..cde9c36 100644 --- a/auth/bucket_policy_resources.go +++ b/auth/bucket_policy_resources.go @@ -29,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 errInvalidResource + return policyErrInvalidResource } *r = make(Resources) for _, s := range ss { @@ -42,7 +42,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error { var s string if err = json.Unmarshal(data, &s); err == nil { if s == "" { - return errInvalidResource + return policyErrInvalidResource } *r = make(Resources) err = r.Add(s) @@ -59,7 +59,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error { func (r Resources) Add(rc string) error { ok, pattern := isValidResource(rc) if !ok { - return errInvalidResource + return policyErrInvalidResource } r[pattern] = struct{}{} @@ -93,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 errInvalidResource + return policyErrInvalidResource } } diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go index 65b5cd4..dbc7128 100644 --- a/s3api/controllers/base_test.go +++ b/s3api/controllers/base_test.go @@ -634,8 +634,7 @@ func TestS3ApiController_PutBucketActions(t *testing.T) { ` - policyBody := ` - { + policyBody := `{ "Statement": [ { "Effect": "Allow", diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 4625637..6b2f0e5 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -466,6 +466,7 @@ func TestGetBucketAcl(s *S3Conf) { func TestPutBucketPolicy(s *S3Conf) { PutBucketPolicy_non_existing_bucket(s) + PutBucketPolicy_invalid_json(s) PutBucketPolicy_empty_statement(s) PutBucketPolicy_invalid_effect(s) PutBucketPolicy_empty_actions_string(s) @@ -1055,6 +1056,7 @@ func GetIntTests() IntTests { "GetBucketAcl_access_denied": GetBucketAcl_access_denied, "GetBucketAcl_success": GetBucketAcl_success, "PutBucketPolicy_non_existing_bucket": PutBucketPolicy_non_existing_bucket, + "PutBucketPolicy_invalid_json": PutBucketPolicy_invalid_json, "PutBucketPolicy_empty_statement": PutBucketPolicy_empty_statement, "PutBucketPolicy_invalid_effect": PutBucketPolicy_invalid_effect, "PutBucketPolicy_empty_actions_string": PutBucketPolicy_empty_actions_string, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index b260d19..e8ace20 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -11402,14 +11402,50 @@ func PutBucketPolicy_non_existing_bucket(s *S3Conf) error { }) } +func PutBucketPolicy_invalid_json(s *S3Conf) error { + testName := "PutBucketPolicy_invalid_json" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + for _, doc := range []string{ + "{true}", + "{asdfsdaf", + `{"Principal": "*" `, + } { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ + Bucket: &bucket, + Policy: &doc, + }) + cancel() + if err := checkApiErr(err, getMalformedPolicyError("This policy contains invalid Json")); err != nil { + return err + } + } + + for _, doc := range []string{ + "false", + "invalid_json", + "bucketPolicy", + `"Statement": []}`, + } { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ + Bucket: &bucket, + Policy: &doc, + }) + cancel() + if err := checkApiErr(err, getMalformedPolicyError("Policies must be valid JSON and the first byte must be '{'")); err != nil { + return err + } + } + + return nil + }) +} + func PutBucketPolicy_empty_statement(s *S3Conf) error { testName := "PutBucketPolicy_empty_statement" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - doc := ` - { - "Statement": [] - } - ` + doc := `{"Statement": []}` ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ @@ -11783,16 +11819,16 @@ func PutBucketPolicy_object_action_on_bucket_resource(s *S3Conf) error { Policy: &doc, }) cancel() - if err := checkApiErr(err, getMalformedPolicyError("Action does not apply to any resource(s) in statement")); err != nil { return err } + return nil }) } func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error { - testName := "PutBucketPolicy_object_action_on_bucket_resource" + testName := "PutBucketPolicy_bucket_action_on_object_resource" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { resource := fmt.Sprintf(`"arn:aws:s3:::%v/*"`, bucket) doc := genPolicyDoc("Allow", `["*"]`, `"s3:DeleteBucket"`, resource) @@ -11811,7 +11847,7 @@ func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error { }) } func PutBucketPolicy_explicit_deny(s *S3Conf) error { - testName := "PutBucketPolicy_object_action_on_bucket_resource" + testName := "PutBucketPolicy_explicit_deny" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { user2 := user{"grt2", "grt2secret", "user"} err := createUsers(s, []user{ @@ -11826,8 +11862,7 @@ func PutBucketPolicy_explicit_deny(s *S3Conf) error { resourceWildCard := fmt.Sprintf("%v/*", resource) resourcePrefix := fmt.Sprintf("%v/someprefix/*", resource) - policy := fmt.Sprintf(` - { + policy := fmt.Sprintf(`{ "Statement": [ { "Action": [ @@ -11866,8 +11901,7 @@ func PutBucketPolicy_explicit_deny(s *S3Conf) error { "Resource": "%v" } ] - } - `, resourcePrefix, resource, resourceWildCard, resource, resourcePrefix) + }`, resourcePrefix, resource, resourceWildCard, resource, resourcePrefix) ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ @@ -14032,8 +14066,7 @@ func AccessControl_single_object_resource_actions(s *S3Conf) error { func AccessControl_multi_statement_policy(s *S3Conf) error { testName := "AccessControl_multi_statement_policy" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - policy := fmt.Sprintf(` - { + policy := fmt.Sprintf(`{ "Statement": [ { "Effect": "Deny", @@ -14048,8 +14081,7 @@ func AccessControl_multi_statement_policy(s *S3Conf) error { "Resource": ["arn:aws:s3:::%s", "arn:aws:s3:::%s/*"] } ] - } - `, bucket, bucket, bucket) + }`, bucket, bucket, bucket) usr := user{ access: "grt1", diff --git a/tests/integration/utils.go b/tests/integration/utils.go index ebb51b0..c16ba68 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -973,8 +973,7 @@ func changeAuthCred(uri, newVal string, index int) (string, error) { } func genPolicyDoc(effect, principal, action, resource string) string { - jsonTemplate := ` - { + jsonTemplate := `{ "Statement": [ { "Effect": "%s",