[aws-kms] CMK keys grant root user too much privileges (GenerateDataKey) vs what is recommended to administer the key

[matdumsa]

During issue #3458 and commit #3459 it seems we introduced a permission for CDK-created CMK keys that deviates from the best practice and the spirit of the CMK official documentation.

I call for that commit to be reverted and the documentation updated instead. The issue lays in adding GenerateDataKey to the permissions for the root user on these keys to allow S3.PUT for administrators of the account.

Reproduction Steps

new s3.Bucket(this, 'MyBucket', {
encryption: s3.BucketEncryption.KMS,
removalPolicy: cdk.RemovalPolicy.DESTROY
});

What did you expect to happen?

As documented in the code and in https://aws.amazon.com/premiumsupport/knowledge-center/update-key-policy-future/ we want the admin users of our AWS account to be able to administer the key but we don't necessarily want them to be able to use it for security reasons. Imagine for example an HR system with payroll information where we only want a given role to encrypt/decrypt using the key and not the administrator of the account.

What actually happened?

Admins get the permission to GenerateDataKey but not to Decrypt it, the asymmetry introduced means that an admin would be able to do .PUT in a properly configured S3 bucket but not .GET and can create confusion.

Environment

• CLI Version : 1.65.0
• Framework Version: monocdk: 1.69.0
• Node.js Version: v12.16.0
• OS : OS X 10.15.7
• Language (Version): TypeScript 3.9.7

Other

when a CMK key is present s3.grantWrite and s3.grantRead are doing the right thing (they grant access to the bucket and to the underlying key). We simply need to modify the documentation on the Key object to say that by default administrators can administer but not use the key for encrypt/decrypt operations and that it should be explicitly granted as described here https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam

If you are an amazon employee you can refer to said confusion created by this bug at https://tiny.amazon.com/i1glis6y/tcorpamazV269

---

This is 🐛 Bug Report

[njlynch]

See #8977 for a closely related issue.

The KMS default key permissions as-is need a bit of love; as mentioned, GenerateDataKey probably shouldn't be there by default. Then again, the whole allowAccountToAdmin method doesn't quite do what people think it does, and isn't a terribly useful permissions boundary. (It allows PutKeyPolicy, which means an admin can grant themselves any other permissions they want, so artificially limited permissions on top of that isn't adding a lot of security.)

A bit of thought is needed here to find the right new default permissions boundaries, how to provide power users a bit more control of the policy, and how to gracefully deprecate the existing behavior. This is something I've already given some thought, but I confess I don't have a complete design yet.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.