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

In aws#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 aws#8977
fixes aws#10575
fixes aws#11309

----

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

Author: njlynch

packages/@aws-cdk/aws-kms/README.md

@@ -31,8 +31,6 @@ key.addAlias('alias/bar');
 
 ## Sharing keys between stacks
 
-> see Trust Account Identities for additional details
-
 To use a KMS key in a different stack in the same CDK application,
 pass the construct to the other stack:
 
@@ -41,8 +39,6 @@ pass the construct to the other stack:
 
 ## Importing existing keys
 
-> see Trust Account Identities for additional details
-
 To use a KMS key that is not defined in this CDK app, but is created through other means, use
 `Key.fromKeyArn(parent, name, ref)`:
 
@@ -72,71 +68,97 @@ Note that calls to `addToResourcePolicy` and `grant*` methods on `myKeyAlias` wi
 no-ops, and `addAlias` and `aliasTargetKey` will fail, as the imported alias does not
 have a reference to the underlying KMS Key.
 
-## Trust Account Identities
-
-KMS keys can be created to trust IAM policies. This is the default behavior in
-the console and is described
-[here](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html).
-This same behavior can be enabled by:
+## Key Policies
 
-```ts
-new Key(stack, 'MyKey', { trustAccountIdentities: true });
-```
+Controlling access and usage of KMS Keys requires the use of key policies (resource-based policies attached to the key);
+this is in contrast to most other AWS resources where access can be entirely controlled with IAM policies,
+and optionally complemented with resource policies. For more in-depth understanding of KMS key access and policies, see
 
-Using `trustAccountIdentities` solves many issues around cyclic dependencies
-between stacks. The most common use case is creating an S3 Bucket with CMK
-default encryption which is later accessed by IAM roles in other stacks.
+* https://docs.aws.amazon.com/kms/latest/developerguide/control-access-overview.html
+* https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html
 
-stack-1 (bucket and key created)
+KMS keys can be created to trust IAM policies. This is the default behavior for both the KMS APIs and in
+the console. This behavior is enabled by the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag,
+which is set for all new projects; for existing projects, this same behavior can be enabled by
+passing the `trustAccountIdentities` property as `true` when creating the key:
 
 ```ts
-// ... snip
-const myKmsKey = new kms.Key(this, 'MyKey', { trustAccountIdentities: true });
-
-const bucket = new Bucket(this, 'MyEncryptedBucket', {
-    bucketName: 'myEncryptedBucket',
-    encryption: BucketEncryption.KMS,
-    encryptionKey: myKmsKey
-});
+new kms.Key(stack, 'MyKey', { trustAccountIdentities: true });
 ```
 
-stack-2 (lambda that operates on bucket and key)
-
-```ts
-// ... snip
-
-const fn = new lambda.Function(this, 'MyFunction', {
-  runtime: lambda.Runtime.NODEJS_10_X,
-  handler: 'index.handler',
-  code: lambda.Code.fromAsset(path.join(__dirname, 'lambda-handler')),
-});
-
-const bucket = s3.Bucket.fromBucketName(this, 'BucketId', 'myEncryptedBucket');
-
-const key = kms.Key.fromKeyArn(this, 'KeyId', 'arn:aws:...'); // key ARN passed via stack props
-
-bucket.grantReadWrite(fn);
-key.grantEncryptDecrypt(fn);
-```
-
-The challenge in this scenario is the KMS key policy behavior. The simple way to understand
-this, is IAM policies for account entities can only grant the permissions granted to the
-account root principle in the key policy. When `trustAccountIdentities` is true,
-the following policy statement is added:
+With either the `@aws-cdk/aws-kms:defaultKeyPolicies` feature flag set,
+or the `trustAccountIdentities` prop set, the Key will be given the following default key policy:
 
 ```json
 {
-  "Sid": "Enable IAM User Permissions",
   "Effect": "Allow",
   "Principal": {"AWS": "arn:aws:iam::111122223333:root"},
   "Action": "kms:*",
   "Resource": "*"
 }
 ```
 
-As the name suggests this trusts IAM policies to control access to the key.
-If account root does not have permissions to the specific actions, then the key
-policy and the IAM policy for the entity (e.g. Lambda) both need to grant
-permission.
+This policy grants full access to the key to the root account user.
+This enables the root account user -- via IAM policies -- to grant access to other IAM principals.
+With the above default policy, future permissions can be added to either the key policy or IAM principal policy.
 
+```ts
+const key = new kms.Key(stack, 'MyKey');
+const user = new iam.User(stack, 'MyUser');
+key.grantEncrypt(user); // Adds encrypt permissions to user policy; key policy is unmodified.
+```
+
+Adopting the default KMS key policy (and so trusting account identities)
+solves many issues around cyclic dependencies between stacks.
+Without this default key policy, future permissions must be added to both the key policy and IAM principal policy,
+which can cause cyclic dependencies if the permissions cross stack boundaries.
+(For example, an encrypted bucket in one stack, and Lambda function that accesses it in another.)
+
+### Appending to or replacing the default key policy
+
+The default key policy can be amended or replaced entirely, depending on your use case and requirements.
+A common addition to the key policy would be to add other key admins that are allowed to administer the key
+(e.g., change permissions, revoke, delete). Additional key admins can be specified at key creation or after
+via the `grantAdmin` method.
+
+```ts
+const myTrustedAdminRole = iam.Role.fromRoleArn(stack, 'TrustedRole', 'arn:aws:iam:....');
+const key = new kms.Key(stack, 'MyKey', {
+  admins: [myTrustedAdminRole],
+});
+
+const secondKey = new kms.Key(stack, 'MyKey2');
+secondKey.grantAdmin(myTrustedAdminRole);
+```
+
+Alternatively, a custom key policy can be specified, which will replace the default key policy.
+
+> **Note**: In applications without the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag set
+and with `trustedAccountIdentities` set to false (the default), specifying a policy at key creation _appends_ the
+provided policy to the default key policy, rather than _replacing_ the default policy.
+
+```ts
+const myTrustedAdminRole = iam.Role.fromRoleArn(stack, 'TrustedRole', 'arn:aws:iam:....');
+// Creates a limited admin policy and assigns to the account root.
+const myCustomPolicy = new iam.PolicyDocument({
+  statements: [new iam.PolicyStatement({
+    actions: [
+      'kms:Create*',
+      'kms:Describe*',
+      'kms:Enable*',
+      'kms:List*',
+      'kms:Put*',
+    ],
+    principals: [new iam.AccountRootPrincipal()],
+    resources: ['*'],
+  })],
+});
+const key = new kms.Key(stack, 'MyKey', {
+  policy: myCustomPolicy,
+});
+```
 
+> **Warning:** Replacing the default key policy with one that only grants access to a specific user or role
+runs the risk of the key becoming unmanageable if that user or role is deleted.
+It is highly recommended that the key policy grants access to the account root, rather than specific principals.
+See https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html for more information.

packages/@aws-cdk/aws-kms/lib/key.ts

@@ -1,8 +1,10 @@
 import * as iam from '@aws-cdk/aws-iam';
-import { IResource, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
+import { FeatureFlags, IResource, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
+import * as cxapi from '@aws-cdk/cx-api';
 import { IConstruct, Construct } from 'constructs';
 import { Alias } from './alias';
 import { CfnKey } from './kms.generated';
+import * as perms from './private/perms';
 
 /**
  * A KMS Key, either managed by this CDK app, or imported.
@@ -77,8 +79,9 @@ abstract class KeyBase extends Resource implements IKey {
   /**
    * Optional property to control trusting account identities.
    *
-   * If specified grants will default identity policies instead of to both
-   * resource and identity policies.
+   * If specified, grants will default identity policies instead of to both
+   * resource and identity policies. This matches the default behavior when creating
+   * KMS keys via the API or console.
    */
   protected abstract readonly trustAccountIdentities: boolean;
 
@@ -168,35 +171,24 @@ abstract class KeyBase extends Resource implements IKey {
   }
 
   /**
-   * Grant decryption permisisons using this key to the given principal
+   * Grant decryption permissions using this key to the given principal
    */
   public grantDecrypt(grantee: iam.IGrantable): iam.Grant {
-    return this.grant(grantee,
-      'kms:Decrypt',
-    );
+    return this.grant(grantee, ...perms.DECRYPT_ACTIONS);
   }
 
   /**
-   * Grant encryption permisisons using this key to the given principal
+   * Grant encryption permissions using this key to the given principal
    */
   public grantEncrypt(grantee: iam.IGrantable): iam.Grant {
-    return this.grant(grantee,
-      'kms:Encrypt',
-      'kms:ReEncrypt*',
-      'kms:GenerateDataKey*',
-    );
+    return this.grant(grantee, ...perms.ENCRYPT_ACTIONS);
   }
 
   /**
-   * Grant encryption and decryption permisisons using this key to the given principal
+   * Grant encryption and decryption permissions using this key to the given principal
    */
   public grantEncryptDecrypt(grantee: iam.IGrantable): iam.Grant {
-    return this.grant(grantee,
-      'kms:Decrypt',
-      'kms:Encrypt',
-      'kms:ReEncrypt*',
-      'kms:GenerateDataKey*',
-    );
+    return this.grant(grantee, ...[...perms.DECRYPT_ACTIONS, ...perms.ENCRYPT_ACTIONS]);
   }
 
   /**
@@ -293,11 +285,27 @@ export interface KeyProps {
   /**
    * Custom policy document to attach to the KMS key.
    *
+   * NOTE - If the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag is set (the default for new projects),
+   * this policy will *override* the default key policy and become the only key policy for the key. If the
+   * feature flag is not set, this policy will be appended to the default key policy.
+   *
    * @default - A policy document with permissions for the account root to
    * administer the key will be created.
    */
   readonly policy?: iam.PolicyDocument;
 
+  /**
+   * A list of principals to add as key administrators to the key policy.
+   *
+   * Key administrators have permissions to manage the key (e.g., change permissions, revoke), but do not have permissions
+   * to use the key in cryptographic operations (e.g., encrypt, decrypt).
+   *
+   * These principals will be added to the default key policy (if none specified), or to the specified policy (if provided).
+   *
+   * @default []
+   */
+  readonly admins?: iam.IPrincipal[];
+
   /**
    * Whether the encryption key should be retained when it is removed from the Stack. This is useful when one wants to
    * retain access to data that was encrypted with a key that is being retired.
@@ -311,10 +319,15 @@ export interface KeyProps {
    *
    * Setting this to true adds a default statement which delegates key
    * access control completely to the identity's IAM policy (similar
-   * to how it works for other AWS resources).
+   * to how it works for other AWS resources). This matches the default behavior
+   * when creating KMS keys via the API or console.
    *
-   * @default false
+   * If the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag is set (the default for new projects),
+   * this flag will always be treated as 'true' and does not need to be explicitly set.
+   *
+   * @default - false, unless the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag is set.
    * @see https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam
+   * @deprecated redundant with the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag
    */
   readonly trustAccountIdentities?: boolean;
 }
@@ -365,12 +378,26 @@ export class Key extends KeyBase {
   constructor(scope: Construct, id: string, props: KeyProps = {}) {
     super(scope, id);
 
-    this.policy = props.policy || new iam.PolicyDocument();
-    this.trustAccountIdentities = props.trustAccountIdentities || false;
-    if (this.trustAccountIdentities) {
-      this.allowAccountIdentitiesToControl();
+    const defaultKeyPoliciesFeatureEnabled = FeatureFlags.of(this).isEnabled(cxapi.KMS_DEFAULT_KEY_POLICIES);
+
+    this.policy = props.policy ?? new iam.PolicyDocument();
+    if (defaultKeyPoliciesFeatureEnabled) {
+      if (props.trustAccountIdentities === false) {
+        throw new Error('`trustAccountIdentities` cannot be false if the @aws-cdk/aws-kms:defaultKeyPolicies feature flag is set');
+      }
+
+      this.trustAccountIdentities = true;
+      // Set the default key policy if one hasn't been provided by the user.
+      if (!props.policy) {
+        this.addDefaultAdminPolicy();
+      }
     } else {
-      this.allowAccountToAdmin();
+      this.trustAccountIdentities = props.trustAccountIdentities ?? false;
+      if (this.trustAccountIdentities) {
+        this.addDefaultAdminPolicy();
+      } else {
+        this.addLegacyAdminPolicy();
+      }
     }
 
     const resource = new CfnKey(this, 'Resource', {
@@ -384,25 +411,49 @@ export class Key extends KeyBase {
     this.keyId = resource.ref;
     resource.applyRemovalPolicy(props.removalPolicy);
 
+    (props.admins ?? []).forEach((p) => this.grantAdmin(p));
+
     if (props.alias !== undefined) {
       this.addAlias(props.alias);
     }
   }
 
-  private allowAccountIdentitiesToControl() {
+  /**
+   * Grant admins permissions using this key to the given principal
+   *
+   * Key administrators have permissions to manage the key (e.g., change permissions, revoke), but do not have permissions
+   * to use the key in cryptographic operations (e.g., encrypt, decrypt).
+   */
+  public grantAdmin(grantee: iam.IGrantable): iam.Grant {
+    return this.grant(grantee, ...perms.ADMIN_ACTIONS);
+  }
+
+  /**
+   * Adds the default key policy to the key. This policy gives the AWS account (root user) full access to the CMK,
+   * which reduces the risk of the CMK becoming unmanageable and enables IAM policies to allow access to the CMK.
+   * This is the same policy that is default when creating a Key via the KMS API or Console.
+   * @see https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default
+   */
+  private addDefaultAdminPolicy() {
     this.addToResourcePolicy(new iam.PolicyStatement({
       resources: ['*'],
       actions: ['kms:*'],
       principals: [new iam.AccountRootPrincipal()],
     }));
-
   }
+
   /**
-   * Let users or IAM policies from this account admin this key.
+   * Grants the account admin privileges -- not full account access -- plus the GenerateDataKey action.
+   * The GenerateDataKey action was added for interop with S3 in https://github.com/aws/aws-cdk/issues/3458.
+   *
+   * This policy is discouraged and deprecated by the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag.
+   *
    * @link https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default
-   * @link https://aws.amazon.com/premiumsupport/knowledge-center/update-key-policy-future/
+   * @deprecated
    */
-  private allowAccountToAdmin() {
+  private addLegacyAdminPolicy() {
+    // This is equivalent to `[...perms.ADMIN_ACTIONS, 'kms:GenerateDataKey']`,
+    // but keeping this explicit ordering for backwards-compatibility (changing the ordering causes resource updates)
     const actions = [
       'kms:Create*',
       'kms:Describe*',

packages/@aws-cdk/aws-kms/lib/private/perms.ts

@@ -0,0 +1,28 @@
+// https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html
+
+export const ADMIN_ACTIONS = [
+  'kms:Create*',
+  'kms:Describe*',
+  'kms:Enable*',
+  'kms:List*',
+  'kms:Put*',
+  'kms:Update*',
+  'kms:Revoke*',
+  'kms:Disable*',
+  'kms:Get*',
+  'kms:Delete*',
+  'kms:TagResource',
+  'kms:UntagResource',
+  'kms:ScheduleKeyDeletion',
+  'kms:CancelKeyDeletion',
+];
+
+export const ENCRYPT_ACTIONS = [
+  'kms:Encrypt',
+  'kms:ReEncrypt*',
+  'kms:GenerateDataKey*',
+];
+
+export const DECRYPT_ACTIONS = [
+  'kms:Decrypt',
+];