feat(kms): change default key policy to align with KMS best practices (under feature flag)

[njlynch]

In #5575, a new flag (trustAccountIdentities) was introduced which -- when set
-- changes the default key policy from a custom key admin policy to one that
grants all access to the key to the root account user. This key policy matches
the default policy when a key is created via the KMS APIs or console.

For backwards-compatibility reasons, the default for trustAccountIdentities
had to be set to false. Without the flag explicitly set, the default key
policy is one that (a) doesn't match the KMS-recommended admin policy and (b)
doesn't explicitly enable IAM principal policies to acccess the key. This means
that all usage operations (e.g., Encrypt, GenerateDataKey) must be added to both
the key policy and to the principal policy.

This change introduces a new feature flag to flip the default behavior of the
trustAccountIdentities flag, so new keys created will have the sane defaults
matching the KMS recommended best practices.

As a related change, this feature flag also changes the behavior when a user
passes in policy when creating a Key. Without the feature flag set, the policy
is always appended to the default key policy. With the feature flag set, the
policy will override the default key policy, enabling users to opt-out of the
default key policy to introduce a more restrictive policy if desired. This also
matches the KMS API behavior, where a policy provided by the user will override
the defaults.

Marking this PR as requires-two-approvers to ensure this PR gets an
appropriately-critical review.

BREAKING CHANGE: change the default value of trustAccountIdentities to true,
which will result in the key getting the KMS-recommended default key
policy. This is enabled through the '@aws-cdk/aws-kms:defaultKeyPolicies'
feature flag.

fixes #8977
fixes #10575
fixes #11309

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[njlynch]

Note that the PR is broken up into 2 commits: the first is the set of changes to KMS; the second is all of the (integ) tests that were updated due to the change in ordering of the KMS actions for the (old) default admin policy. I could have coded around this, but I really wanted perms.ts to have the ordering of actions exactly as they appear in documentation, for the sake of easy comparisons in the future. I am open to arguments to revert this part of the PR for the sake of a cleaner diff though.

[skinny85]

I still feel like I would avoid the feature flag here, and change the default policy (I wouldn't change the append/override behavior though), since it seems like it's so broken. I'm not holding the PR on changing that though.

A few comments, mainly around naming (things which I didn't "grok" when looking at the code on the first pass).

Also, since we're in the area already, is this a good time to handle #4381 ?

[njlynch]

Also, since we're in the area already, is this a good time to handle #4381 ?

@skinny85 - The only difference I'm seeing is that S3/DynamoDB KEY_READ_ACTIONS includes kms:DescribeKey, whereas grantDecrypt doesn't. I can just add that to grantDecrypt/DECRYPT_ACTIONS, but (a) it's not clear if it's actually needed in most cases, and (b) it would cause a resource update for all existing usages. Not sure if it's worth it, to be honest, especially because none of the existing usages in S3/DynamoDB would be a simple replacement of the existing code with key.grantDecrypt; there's a lot of custom stuff in there that I'm not sure I want to couple into this change.

That being said, if you (and others) think it's okay to make that change without a feature flag, I'm happy enough to do it as a separate PR right after this one. Just don't want to unnecessarily muddy the waters here.

[skinny85]

Looks great! A few minor comments.

[mergify]

Thank you for contributing! Your pull request will be automatically updated and merged (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: bf36a36
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be automatically updated and merged (do not update manually, and be sure to allow changes to be pushed to your fork).