Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s3.Bucket.grantDelete on bucket with KMS SSE generates invalid policy #4380

Closed
Roman-L opened this issue Oct 4, 2019 · 4 comments · Fixed by #7528
Closed

s3.Bucket.grantDelete on bucket with KMS SSE generates invalid policy #4380

Roman-L opened this issue Oct 4, 2019 · 4 comments · Fixed by #7528
Assignees
Labels
@aws-cdk/aws-kms Related to AWS Key Management @aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. in-progress This issue is being actively worked on. p2

Comments

@Roman-L
Copy link

Roman-L commented Oct 4, 2019

Reproduction Steps

This simple stack

Key sseKmsKey = new Key(this, "lapinr-see-kms-key");

BucketProps bucketProperties = BucketProps.builder()
                                          .bucketName("lapinr-test-bucket")
                                          .encryption(BucketEncryption.KMS)
                                          .encryptionKey(sseKmsKey)
                                          .build();
Bucket bucket = new Bucket(this,
                           "LapinrTestBucket",
                           bucketProperties);

RoleProps clientRoleProps = RoleProps.builder()
                                     .roleName("lapinr-access-role")
                                     .assumedBy(new ArnPrincipal("arn:aws:iam::1234567890:role/external-role"))
                                     .build();
Role clientRole = new Role(this,
                           "LapinrAccessRole",
                           clientRoleProps);

bucket.grantDelete(clientRole);

Generates the following policy

"LapinrAccessRoleDefaultPolicy0F01FF42": {
      "Type": "AWS::IAM::Policy",
      "Properties": {
        "PolicyDocument": {
          "Statement": [
            {
              "Action": "s3:DeleteObject*",
              "Effect": "Allow",
              "Resource": {
                "Fn::Join": [
                  "",
                  [
                    {
                      "Fn::GetAtt": [
                        "LapinrTestBucketDB437746",
                        "Arn"
                      ]
                    },
                    "/*"
                  ]
                ]
              }
            },
            {
              "Effect": "Allow",
              "Resource": {
                "Fn::GetAtt": [
                  "lapinrseekmskey4CB31ADE",
                  "Arn"
                ]
              }
            }
          ],
          "Version": "2012-10-17"
        },
        "PolicyName": "LapinrAccessRoleDefaultPolicy0F01FF42",
        "Roles": [
          {
            "Ref": "LapinrAccessRole015FF5A4"
          }
        ]
      }

The problematic part is below, note lack of 'Action' block

{
    "Effect": "Allow",
    "Resource": {
         "Fn::GetAtt": [
            "lapinrseekmskey4CB31ADE",
            "Arn"
         ]
    }
}

Error Log

During CloudFormation stack update, the update fails with a Malformed Policy error due to missing Action block.

Environment

  • CLI Version : 1.9.0
  • Framework Version: 1.9.0
  • OS : Linux
  • Language : Java

This is 🐛 Bug Report

@Roman-L Roman-L added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 4, 2019
@Roman-L
Copy link
Author

Roman-L commented Oct 4, 2019

Taking a look at the code

public grantDelete(identity: iam.IGrantable, objectsKeyPattern: any = '*') {
return this.grant(identity, perms.BUCKET_DELETE_ACTIONS, [],
this.arnForObjects(objectsKeyPattern));
}

Which leads to

if (this.encryptionKey) {
this.encryptionKey.grant(grantee, ...keyActions);
}

Eventually (via aws-kms grant) leads to this, without any checks on actions being empty

const statement = new PolicyStatement({
actions: options.actions,
resources: (options.resourceSelfArns || options.resourceArns),
principals: [options.resourcePolicyPrincipal || options.grantee!.grantPrincipal]
});

@NGL321 NGL321 added @aws-cdk/aws-kms Related to AWS Key Management @aws-cdk/aws-s3 Related to Amazon S3 needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 4, 2019
@NGL321
Copy link
Contributor

NGL321 commented Oct 4, 2019

Might be related to #4381?
@skinny85

@Roman-L
Copy link
Author

Roman-L commented Oct 5, 2019

At first glance, doesn't seem like it.

Based on the code snippets I provided above, I believe this issue is due to an empty 'actions' parameter that translates to an empty 'Actions' block in the template.
If I'm not mistaken, this makes s3.Bucket.grantDelete un-useable for buckets with SSE KMS.

#4381 has to do with a perceived difference between read/write and encrypt/decrypt. (I don't know enough to tell whether it's actually an issue)

@rix0rrr rix0rrr removed the needs-reproduction This issue needs reproduction. label Oct 10, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 10, 2019

Yep, this should have had an "empty array" check in there.

@rix0rrr rix0rrr added the p2 label Oct 24, 2019
@rix0rrr rix0rrr assigned skinny85 and unassigned rix0rrr Jan 23, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Apr 23, 2020
@mergify mergify bot closed this as completed in #7528 May 6, 2020
mergify bot pushed a commit that referenced this issue May 6, 2020
Added empty array check for keyActions. This will make sure that `grantDelete` will not create malformed policy when used with `KMS` key.

Added a new integ test to check CloudFormation will not error out during the deployment.

Fixes #4380
karupanerura pushed a commit to karupanerura/aws-cdk that referenced this issue May 7, 2020
Added empty array check for keyActions. This will make sure that `grantDelete` will not create malformed policy when used with `KMS` key.

Added a new integ test to check CloudFormation will not error out during the deployment.

Fixes aws#4380
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-kms Related to AWS Key Management @aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. in-progress This issue is being actively worked on. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants