feat(kms): change default key policy to align with KMS best practices…

… (under feature flag) 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
aws · Dec 7, 2020 · fe524be · fe524be
1 parent e8c8d77
commit fe524be
Show file tree

Hide file tree

Showing 8 changed files with 494 additions and 328 deletions.
diff --git a/packages/@aws-cdk/aws-kms/README.md b/packages/@aws-cdk/aws-kms/README.md
@@ -74,17 +74,19 @@ 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:
+KMS keys can be created to trust IAM policies. This is the default behavior for both the KMS APIs and in
+the console and is described [here](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html).
+
+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:
 
 ```ts
 new Key(stack, 'MyKey', { trustAccountIdentities: true });
 ```
 
-Using `trustAccountIdentities` solves many issues around cyclic dependencies
-between stacks. The most common use case is creating an S3 Bucket with CMK
+Adopting the default KMS key policy (and so trusting account identities)
+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.
 
 stack-1 (bucket and key created)
@@ -138,5 +140,3 @@ 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.
-
-
diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/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 { Annotations, 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 './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;
 
@@ -171,32 +174,21 @@ abstract class KeyBase extends Resource implements IKey {
    * Grant decryption permisisons 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
    */
   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
    */
   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,6 +285,10 @@ 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.
    */
@@ -311,9 +307,13 @@ 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
    */
   readonly trustAccountIdentities?: boolean;
@@ -365,12 +365,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 defaultKeyPoliciesEnabled = FeatureFlags.of(this).isEnabled(cxapi.KMS_DEFAULT_KEY_POLICIES);
+
+    this.policy = props.policy ?? new iam.PolicyDocument();
+    if (defaultKeyPoliciesEnabled) {
+      if (props.trustAccountIdentities === false) {
+        Annotations.of(this).addWarning('`trustAccountIdentities` has no impact 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.grantDefaultKeyPolicy();
+      }
     } else {
-      this.allowAccountToAdmin();
+      this.trustAccountIdentities = props.trustAccountIdentities || false;
+      if (this.trustAccountIdentities) {
+        this.grantDefaultKeyPolicy();
+      } else {
+        this.grantAccountAdminPrivilegesPlusGenerateDataKey();
+      }
     }
 
     const resource = new CfnKey(this, 'Resource', {
@@ -389,41 +403,33 @@ export class Key extends KeyBase {
     }
   }
 
-  private allowAccountIdentitiesToControl() {
+  /**
+   * 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 grantDefaultKeyPolicy() {
     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() {
-    const actions = [
-      'kms:Create*',
-      'kms:Describe*',
-      'kms:Enable*',
-      'kms:List*',
-      'kms:Put*',
-      'kms:Update*',
-      'kms:Revoke*',
-      'kms:Disable*',
-      'kms:Get*',
-      'kms:Delete*',
-      'kms:ScheduleKeyDeletion',
-      'kms:CancelKeyDeletion',
-      'kms:GenerateDataKey',
-      'kms:TagResource',
-      'kms:UntagResource',
-    ];
-
+  private grantAccountAdminPrivilegesPlusGenerateDataKey() {
     this.addToResourcePolicy(new iam.PolicyStatement({
       resources: ['*'],
-      actions,
+      actions: [...perms.ADMIN_ACTIONS, 'kms:GenerateDataKey'],
       principals: [new iam.AccountRootPrincipal()],
     }));
   }

diff --git a/packages/@aws-cdk/aws-kms/lib/perms.ts b/packages/@aws-cdk/aws-kms/lib/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',
+];
diff --git a/packages/@aws-cdk/aws-kms/package.json b/packages/@aws-cdk/aws-kms/package.json
@@ -74,6 +74,7 @@
   "license": "Apache-2.0",
   "devDependencies": {
     "@aws-cdk/assert": "0.0.0",
+    "@aws-cdk/cloud-assembly-schema": "0.0.0",
     "cdk-build-tools": "0.0.0",
     "cdk-integ-tools": "0.0.0",
     "cfn2ts": "0.0.0",
@@ -82,12 +83,14 @@
   "dependencies": {
     "@aws-cdk/aws-iam": "0.0.0",
     "@aws-cdk/core": "0.0.0",
+    "@aws-cdk/cx-api": "0.0.0",
     "constructs": "^3.2.0"
   },
   "homepage": "https://github.com/aws/aws-cdk",
   "peerDependencies": {
     "@aws-cdk/aws-iam": "0.0.0",
     "@aws-cdk/core": "0.0.0",
+    "@aws-cdk/cx-api": "0.0.0",
     "constructs": "^3.2.0"
   },
   "engines": {

diff --git a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.expected.json
@@ -18,11 +18,11 @@
                   "kms:Disable*",
                   "kms:Get*",
                   "kms:Delete*",
+                  "kms:TagResource",
+                  "kms:UntagResource",
                   "kms:ScheduleKeyDeletion",
                   "kms:CancelKeyDeletion",
-                  "kms:GenerateDataKey",
-                  "kms:TagResource",
-                  "kms:UntagResource"
+                  "kms:GenerateDataKey"
                 ],
                 "Effect": "Allow",
                 "Principal": {
@@ -80,4 +80,4 @@
       }
     }
   }
-]
+]
diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json
@@ -17,11 +17,11 @@
                 "kms:Disable*",
                 "kms:Get*",
                 "kms:Delete*",
+                "kms:TagResource",
+                "kms:UntagResource",
                 "kms:ScheduleKeyDeletion",
                 "kms:CancelKeyDeletion",
-                "kms:GenerateDataKey",
-                "kms:TagResource",
-                "kms:UntagResource"
+                "kms:GenerateDataKey"
               ],
               "Effect": "Allow",
               "Principal": {
@@ -74,4 +74,4 @@
       }
     }
   }
-}
+}