First round of PR feedback

njlynch · njlynch · commit 996b6e1985a0 · 2020-12-08T12:54:24.000Z
diff --git a/packages/@aws-cdk/aws-kms/README.md b/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,96 @@ 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
+## Key Policies
 
-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).
+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
 
-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:
+* https://docs.aws.amazon.com/kms/latest/developerguide/control-access-overview.html
+* https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html
+
+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:
 
 ```ts
-new Key(stack, 'MyKey', { trustAccountIdentities: true });
+new kms.Key(stack, 'MyKey', { trustAccountIdentities: true });
 ```
 
-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)
-
-```ts
-// ... snip
-const myKmsKey = new kms.Key(this, 'MyKey', { trustAccountIdentities: true });
+With either the `defaultKeyPolicies` feature flag set, or `trustAccountIdentities` set,
+the Key will be given the following default key policy:
 
-const bucket = new Bucket(this, 'MyEncryptedBucket', {
-    bucketName: 'myEncryptedBucket',
-    encryption: BucketEncryption.KMS,
-    encryptionKey: myKmsKey
-});
+```json
+{
+  "Effect": "Allow",
+  "Principal": {"AWS": "arn:aws:iam::111122223333:root"},
+  "Action": "kms:*",
+  "Resource": "*"
+}
 ```
 
-stack-2 (lambda that operates on bucket and key)
+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
-// ... snip
+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.
+```
 
-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')),
-});
+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).
 
-const bucket = s3.Bucket.fromBucketName(this, 'BucketId', 'myEncryptedBucket');
+### Appending to or replacing the default key policy
 
-const key = kms.Key.fromKeyArn(this, 'KeyId', 'arn:aws:...'); // key ARN passed via stack props
+The default key policy can be ammended 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 `addAdmin` method.
 
-bucket.grantReadWrite(fn);
-key.grantEncryptDecrypt(fn);
+```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.addAdmin(myTrustedAdminRole);
 ```
 
-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:
+Alternatively, a custom key policy can be specified, which will replace the default key policy.
 
-```json
-{
-  "Sid": "Enable IAM User Permissions",
-  "Effect": "Allow",
-  "Principal": {"AWS": "arn:aws:iam::111122223333:root"},
-  "Action": "kms:*",
-  "Resource": "*"
-}
+> **Note**: In applications without the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag set,
+or `trustedAccountIdentities` set to false, 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,
+});
 ```
 
-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.
+> **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.
diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts
@@ -1,10 +1,10 @@
 import * as iam from '@aws-cdk/aws-iam';
-import { Annotations, FeatureFlags, 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 './perms';
+import * as perms from './private/perms';
 
 /**
  * A KMS Key, either managed by this CDK app, or imported.
@@ -294,6 +294,18 @@ export interface KeyProps {
    */
   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.
@@ -315,6 +327,7 @@ export interface KeyProps {
    *
    * @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,28 +378,30 @@ export class Key extends KeyBase {
   constructor(scope: Construct, id: string, props: KeyProps = {}) {
     super(scope, id);
 
-    const defaultKeyPoliciesEnabled = FeatureFlags.of(this).isEnabled(cxapi.KMS_DEFAULT_KEY_POLICIES);
+    const defaultKeyPoliciesFeatureEnabled = FeatureFlags.of(this).isEnabled(cxapi.KMS_DEFAULT_KEY_POLICIES);
 
     this.policy = props.policy ?? new iam.PolicyDocument();
-    if (defaultKeyPoliciesEnabled) {
+    if (defaultKeyPoliciesFeatureEnabled) {
       if (props.trustAccountIdentities === false) {
-        Annotations.of(this).addWarning('`trustAccountIdentities` has no impact if the @aws-cdk/aws-kms:defaultKeyPolicies feature flag is set');
+        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.grantDefaultKeyPolicy();
+        this.addDefaultAdminPolicy();
       }
     } else {
-      this.trustAccountIdentities = props.trustAccountIdentities || false;
+      this.trustAccountIdentities = props.trustAccountIdentities ?? false;
       if (this.trustAccountIdentities) {
-        this.grantDefaultKeyPolicy();
+        this.addDefaultAdminPolicy();
       } else {
-        this.grantAccountAdminPrivilegesPlusGenerateDataKey();
+        this.addLegacyAdminPolicy();
       }
     }
 
+    (props.admins ?? []).forEach((p) => this.addAdmin(p));
+
     const resource = new CfnKey(this, 'Resource', {
       description: props.description,
       enableKeyRotation: props.enableKeyRotation,
@@ -403,13 +418,27 @@ export class Key extends KeyBase {
     }
   }
 
+  /**
+   * Adds the specified principal to the key policy as a key administrator.
+   *
+   * 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 addAdmin(principal: iam.IPrincipal) {
+    this.addToResourcePolicy(new iam.PolicyStatement({
+      resources: ['*'],
+      actions: perms.ADMIN_ACTIONS,
+      principals: [principal],
+    }));
+  }
+
   /**
    * 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() {
+  private addDefaultAdminPolicy() {
     this.addToResourcePolicy(new iam.PolicyStatement({
       resources: ['*'],
       actions: ['kms:*'],
@@ -426,10 +455,30 @@ export class Key extends KeyBase {
    * @link https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default
    * @deprecated
    */
-  private grantAccountAdminPrivilegesPlusGenerateDataKey() {
+  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*',
+      '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',
+    ];
+
     this.addToResourcePolicy(new iam.PolicyStatement({
       resources: ['*'],
-      actions: [...perms.ADMIN_ACTIONS, 'kms:GenerateDataKey'],
+      actions,
       principals: [new iam.AccountRootPrincipal()],
     }));
   }
diff --git a/packages/@aws-cdk/aws-kms/lib/private/perms.ts b/packages/@aws-cdk/aws-kms/lib/private/perms.ts
diff --git a/packages/@aws-cdk/aws-kms/package.json b/packages/@aws-cdk/aws-kms/package.json
@@ -74,7 +74,6 @@
   "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",
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:GenerateDataKey",
+                  "kms:TagResource",
+                  "kms:UntagResource"
                 ],
                 "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:GenerateDataKey",
+                "kms:TagResource",
+                "kms:UntagResource"
               ],
               "Effect": "Allow",
               "Principal": {
@@ -74,4 +74,4 @@
       }
     }
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-kms/test/key.test.ts b/packages/@aws-cdk/aws-kms/test/key.test.ts
