From fe524be3263b4580de0ff3cc81f4208746ffa960 Mon Sep 17 00:00:00 2001 From: Nick Lynch Date: Mon, 7 Dec 2020 13:44:53 +0000 Subject: [PATCH] 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 --- packages/@aws-cdk/aws-kms/README.md | 16 +- packages/@aws-cdk/aws-kms/lib/key.ts | 102 +-- packages/@aws-cdk/aws-kms/lib/perms.ts | 28 + packages/@aws-cdk/aws-kms/package.json | 3 + .../test/integ.key-sharing.lit.expected.json | 8 +- .../aws-kms/test/integ.key.expected.json | 8 +- packages/@aws-cdk/aws-kms/test/key.test.ts | 640 ++++++++++-------- packages/@aws-cdk/cx-api/lib/features.ts | 17 + 8 files changed, 494 insertions(+), 328 deletions(-) create mode 100644 packages/@aws-cdk/aws-kms/lib/perms.ts diff --git a/packages/@aws-cdk/aws-kms/README.md b/packages/@aws-cdk/aws-kms/README.md index 0f9c4c6df67cc..3ad3ba626651a 100644 --- 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 index db13ee4d761ea..3bdd86005ec32 100644 --- 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 new file mode 100644 index 0000000000000..fe510359d7e89 --- /dev/null +++ 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 index 65c05ca946fbf..e8e7502e9805e 100644 --- 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 index 1a2290cc9bce4..21d14d2f40e1b 100644 --- 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 @@ } } } -] \ No newline at end of file +] diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.expected.json b/packages/@aws-cdk/aws-kms/test/integ.key.expected.json index a11ff1abc7e94..a104eca0f6e67 100644 --- 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 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-kms/test/key.test.ts b/packages/@aws-cdk/aws-kms/test/key.test.ts index 93b5d1bc67e31..ad33974b3a6bb 100644 --- a/packages/@aws-cdk/aws-kms/test/key.test.ts +++ b/packages/@aws-cdk/aws-kms/test/key.test.ts @@ -1,10 +1,11 @@ -import { ResourcePart } from '@aws-cdk/assert'; +import { arrayWith, ResourcePart } from '@aws-cdk/assert'; import '@aws-cdk/assert/jest'; import * as iam from '@aws-cdk/aws-iam'; -import { App, CfnOutput, RemovalPolicy, Stack, Tags } from '@aws-cdk/core'; -import { Key } from '../lib'; +import * as cxschema from '@aws-cdk/cloud-assembly-schema'; +import * as cdk from '@aws-cdk/core'; +import * as kms from '../lib'; -const ACTIONS: string[] = [ +const LEGACY_ADMIN_ACTIONS: string[] = [ 'kms:Create*', 'kms:Describe*', 'kms:Enable*', @@ -15,167 +16,227 @@ const ACTIONS: string[] = [ 'kms:Disable*', 'kms:Get*', 'kms:Delete*', + 'kms:TagResource', + 'kms:UntagResource', 'kms:ScheduleKeyDeletion', 'kms:CancelKeyDeletion', 'kms:GenerateDataKey', - 'kms:TagResource', - 'kms:UntagResource', ]; +let app: cdk.App; +let stack: cdk.Stack; +beforeEach(() => { + app = new cdk.App({ + context: { + // By default, enable the correct key policy behavior. Specific tests will test the disabled behavior. + '@aws-cdk/aws-kms:defaultKeyPolicies': true, + }, + }); + stack = new cdk.Stack(app); +}); + test('default key', () => { - const stack = new Stack(); - - new Key(stack, 'MyKey'); - - expect(stack).toMatchTemplate({ - Resources: { - MyKey6AB29FA6: { - Type: 'AWS::KMS::Key', - Properties: { - KeyPolicy: { - Statement: [ - { - Action: ACTIONS, - Effect: 'Allow', - Principal: { - AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, - }, - Resource: '*', - }, - ], - Version: '2012-10-17', + new kms.Key(stack, 'MyKey'); + + expect(stack).toHaveResource('AWS::KMS::Key', { + Properties: { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', }, - }, - DeletionPolicy: 'Retain', - UpdateReplacePolicy: 'Retain', + ], + Version: '2012-10-17', }, }, - }); + DeletionPolicy: 'Retain', + UpdateReplacePolicy: 'Retain', + }, ResourcePart.CompleteDefinition); }); test('default with no retention', () => { - const app = new App(); - const stack = new Stack(app, 'TestStack'); - - new Key(stack, 'MyKey', { removalPolicy: RemovalPolicy.DESTROY }); + new kms.Key(stack, 'MyKey', { removalPolicy: cdk.RemovalPolicy.DESTROY }); expect(stack).toHaveResource('AWS::KMS::Key', { DeletionPolicy: 'Delete', UpdateReplacePolicy: 'Delete' }, ResourcePart.CompleteDefinition); }); -test('default with some permission', () => { - const app = new App(); - const stack = new Stack(app, 'Test'); - - const key = new Key(stack, 'MyKey'); - const p = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:encrypt'] }); - p.addArnPrincipal('arn'); - key.addToResourcePolicy(p); - - expect(stack).toMatchTemplate({ - Resources: { - MyKey6AB29FA6: { - Type: 'AWS::KMS::Key', - Properties: { - KeyPolicy: { - Statement: [ - { - Action: ACTIONS, - Effect: 'Allow', - Principal: { - AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, - }, - Resource: '*', - }, - { - Action: 'kms:encrypt', - Effect: 'Allow', - Principal: { - AWS: 'arn', - }, - Resource: '*', - }, - ], - Version: '2012-10-17', +describe('key policies', () => { + test('can specify a default key policy', () => { + const policy = new iam.PolicyDocument(); + const statement = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:Put*'] }); + statement.addArnPrincipal('arn:aws:iam::111122223333:root'); + policy.addStatements(statement); + + new kms.Key(stack, 'MyKey', { policy }); + + expect(stack).toHaveResource('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + { + Action: 'kms:Put*', + Effect: 'Allow', + Principal: { + AWS: 'arn:aws:iam::111122223333:root', + }, + Resource: '*', }, - }, - DeletionPolicy: 'Retain', - UpdateReplacePolicy: 'Retain', + ], + Version: '2012-10-17', }, - }, + }); + }); + + test('can append to the default key policy', () => { + const statement = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:Put*'] }); + statement.addArnPrincipal('arn:aws:iam::111122223333:root'); + + const key = new kms.Key(stack, 'MyKey'); + key.addToResourcePolicy(statement); + + expect(stack).toHaveResource('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', + }, + { + Action: 'kms:Put*', + Effect: 'Allow', + Principal: { + AWS: 'arn:aws:iam::111122223333:root', + }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); }); + test.each([ + ['decrypt', (key: kms.Key, user: iam.IGrantable) => key.grantDecrypt(user), 'kms:Decrypt'], + ['encrypt', (key: kms.Key, user: iam.IGrantable) => key.grantEncrypt(user), ['kms:Encrypt', 'kms:ReEncrypt*', 'kms:GenerateDataKey*']], + ])('grant %s', (_, grantFn, actions) => { + // GIVEN + const key = new kms.Key(stack, 'Key'); + const user = new iam.User(stack, 'User'); + + // WHEN + grantFn(key, user); + + // THEN + // Key policy should be unmodified by the grant. + expect(stack).toHaveResource('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] } }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: actions, + Effect: 'Allow', + Resource: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] }, + }, + ], + Version: '2012-10-17', + }, + }); + }); + + test('grant for a principal in a dependent stack works correctly', () => { + const principalStack = new cdk.Stack(app, 'PrincipalStack'); + const principal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AnyPrincipal(), + }); + + const keyStack = new cdk.Stack(app, 'KeyStack'); + const key = new kms.Key(keyStack, 'Key'); + + principalStack.addDependency(keyStack); + + key.grantEncrypt(principal); + + expect(principalStack).toHaveResourceLike('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Resource: { + 'Fn::ImportValue': 'KeyStack:ExportsOutputFnGetAttKey961B73FDArn5A860C43', + }, + }, + ], + Version: '2012-10-17', + }, + }); + }); }); test('key with some options', () => { - const stack = new Stack(); - - const key = new Key(stack, 'MyKey', { + const key = new kms.Key(stack, 'MyKey', { enableKeyRotation: true, enabled: false, }); - const p = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:encrypt'] }); - p.addArnPrincipal('arn'); - key.addToResourcePolicy(p); - - Tags.of(key).add('tag1', 'value1'); - Tags.of(key).add('tag2', 'value2'); - Tags.of(key).add('tag3', ''); - - expect(stack).toMatchTemplate({ - Resources: { - MyKey6AB29FA6: { - Type: 'AWS::KMS::Key', - Properties: { - KeyPolicy: { - Statement: [ - { - Action: ACTIONS, - Effect: 'Allow', - Principal: { - AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, - }, - Resource: '*', - }, - { - Action: 'kms:encrypt', - Effect: 'Allow', - Principal: { - AWS: 'arn', - }, - Resource: '*', - }, - ], - Version: '2012-10-17', - }, - Enabled: false, - EnableKeyRotation: true, - Tags: [ - { - Key: 'tag1', - Value: 'value1', - }, - { - Key: 'tag2', - Value: 'value2', - }, - { - Key: 'tag3', - Value: '', - }, - ], - }, - DeletionPolicy: 'Retain', - UpdateReplacePolicy: 'Retain', + + cdk.Tags.of(key).add('tag1', 'value1'); + cdk.Tags.of(key).add('tag2', 'value2'); + cdk.Tags.of(key).add('tag3', ''); + + expect(stack).toHaveResourceLike('AWS::KMS::Key', { + Enabled: false, + EnableKeyRotation: true, + Tags: [ + { + Key: 'tag1', + Value: 'value1', }, - }, + { + Key: 'tag2', + Value: 'value2', + }, + { + Key: 'tag3', + Value: '', + }, + ], }); }); -test('addAlias creates an alias', () => { - const app = new App(); - const stack = new Stack(app, 'Test'); +test('trustAccountIdentities will warn if set to false (when the defaultKeyPolicies feature flag is enabled)', () => { + const key = new kms.Key(stack, 'MyKey', { trustAccountIdentities: false }); + + expect(key.node.metadata[0].type).toEqual(cxschema.ArtifactMetadataEntryType.WARN); + expect(key.node.metadata[0].data).toEqual('`trustAccountIdentities` has no impact if the @aws-cdk/aws-kms:defaultKeyPolicies feature flag is set'); +}); - const key = new Key(stack, 'MyKey', { +test('addAlias creates an alias', () => { + const key = new kms.Key(stack, 'MyKey', { enableKeyRotation: true, enabled: false, }); @@ -196,10 +257,7 @@ test('addAlias creates an alias', () => { }); test('can run multiple addAlias', () => { - const app = new App(); - const stack = new Stack(app, 'Test'); - - const key = new Key(stack, 'MyKey', { + const key = new kms.Key(stack, 'MyKey', { enableKeyRotation: true, enabled: false, }); @@ -230,97 +288,10 @@ test('can run multiple addAlias', () => { }); }); -test('grant decrypt on a key', () => { - // GIVEN - const stack = new Stack(); - const key = new Key(stack, 'Key'); - const user = new iam.User(stack, 'User'); - - // WHEN - key.grantDecrypt(user); - - // THEN - expect(stack).toHaveResource('AWS::KMS::Key', { - KeyPolicy: { - Statement: [ - // This one is there by default - { - Action: ACTIONS, - Effect: 'Allow', - Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] } }, - Resource: '*', - }, - // This is the interesting one - { - Action: 'kms:Decrypt', - Effect: 'Allow', - Principal: { AWS: { 'Fn::GetAtt': ['User00B015A1', 'Arn'] } }, - Resource: '*', - }, - ], - Version: '2012-10-17', - }, - }); - - expect(stack).toHaveResource('AWS::IAM::Policy', { - PolicyDocument: { - Statement: [ - { - Action: 'kms:Decrypt', - Effect: 'Allow', - Resource: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] }, - }, - ], - Version: '2012-10-17', - }, - }); - -}); - -test('grant for a principal in a dependent stack works correctly', () => { - const app = new App(); - - const principalStack = new Stack(app, 'PrincipalStack'); - const principal = new iam.Role(principalStack, 'Role', { - assumedBy: new iam.AnyPrincipal(), - }); - - const keyStack = new Stack(app, 'KeyStack'); - const key = new Key(keyStack, 'Key'); - - principalStack.addDependency(keyStack); - - key.grantEncrypt(principal); - - expect(keyStack).toHaveResourceLike('AWS::KMS::Key', { - KeyPolicy: { - Statement: [ - { - // owning account management permissions - we don't care about them in this test - }, - { - Action: [ - 'kms:Encrypt', - 'kms:ReEncrypt*', - 'kms:GenerateDataKey*', - ], - Effect: 'Allow', - Principal: { - AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, - }, - Resource: '*', - }, - ], - }, - }); - -}); - test('keyId resolves to a Ref', () => { - const stack = new Stack(); - const key = new Key(stack, 'MyKey'); + const key = new kms.Key(stack, 'MyKey'); - new CfnOutput(stack, 'Out', { + new cdk.CfnOutput(stack, 'Out', { value: key.keyId, }); @@ -330,29 +301,8 @@ test('keyId resolves to a Ref', () => { }); }); -test('enablePolicyControl changes key policy to allow IAM control', () => { - const stack = new Stack(); - new Key(stack, 'MyKey', { trustAccountIdentities: true }); - expect(stack).toHaveResourceLike('AWS::KMS::Key', { - KeyPolicy: { - Statement: [ - { - Action: 'kms:*', - Effect: 'Allow', - Principal: { - AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, - }, - Resource: '*', - }, - ], - }, - }); -}); - test('fails if key policy has no actions', () => { - const app = new App(); - const stack = new Stack(app, 'my-stack'); - const key = new Key(stack, 'MyKey'); + const key = new kms.Key(stack, 'MyKey'); key.addToResourcePolicy(new iam.PolicyStatement({ resources: ['*'], @@ -363,9 +313,7 @@ test('fails if key policy has no actions', () => { }); test('fails if key policy has no IAM principals', () => { - const app = new App(); - const stack = new Stack(app, 'my-stack'); - const key = new Key(stack, 'MyKey'); + const key = new kms.Key(stack, 'MyKey'); key.addToResourcePolicy(new iam.PolicyStatement({ resources: ['*'], @@ -377,17 +325,15 @@ test('fails if key policy has no IAM principals', () => { describe('imported keys', () => { test('throw an error when providing something that is not a valid key ARN', () => { - const stack = new Stack(); - expect(() => { - Key.fromKeyArn(stack, 'Imported', 'arn:aws:kms:us-east-1:123456789012:key'); + kms.Key.fromKeyArn(stack, 'Imported', 'arn:aws:kms:us-east-1:123456789012:key'); }).toThrow(/KMS key ARN must be in the format 'arn:aws:kms:::key\/', got: 'arn:aws:kms:us-east-1:123456789012:key'/); }); test('can have aliases added to them', () => { - const stack2 = new Stack(); - const myKeyImported = Key.fromKeyArn(stack2, 'MyKeyImported', + const stack2 = new cdk.Stack(app, 'Stack2'); + const myKeyImported = kms.Key.fromKeyArn(stack2, 'MyKeyImported', 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); // addAlias can be called on imported keys. @@ -407,14 +353,11 @@ describe('imported keys', () => { }, }); }); - }); describe('addToResourcePolicy allowNoOp and there is no policy', () => { test('succeed if set to true (default)', () => { - const stack = new Stack(); - - const key = Key.fromKeyArn(stack, 'Imported', + const key = kms.Key.fromKeyArn(stack, 'Imported', 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); key.addToResourcePolicy(new iam.PolicyStatement({ resources: ['*'], actions: ['*'] })); @@ -422,9 +365,7 @@ describe('addToResourcePolicy allowNoOp and there is no policy', () => { }); test('fails if set to false', () => { - const stack = new Stack(); - - const key = Key.fromKeyArn(stack, 'Imported', + const key = kms.Key.fromKeyArn(stack, 'Imported', 'arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012'); expect(() => { @@ -433,3 +374,174 @@ describe('addToResourcePolicy allowNoOp and there is no policy', () => { }); }); + +describe('when the defaultKeyPolicies feature flag is disabled', () => { + beforeEach(() => { + app = new cdk.App({ + context: { + '@aws-cdk/aws-kms:defaultKeyPolicies': false, + }, + }); + stack = new cdk.Stack(app); + }); + + test('default key policy', () => { + new kms.Key(stack, 'MyKey'); + + expect(stack).toHaveResource('AWS::KMS::Key', { + Properties: { + KeyPolicy: { + Statement: [ + { + Action: LEGACY_ADMIN_ACTIONS, + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }, + DeletionPolicy: 'Retain', + UpdateReplacePolicy: 'Retain', + }, ResourcePart.CompleteDefinition); + }); + + test('policy if specified appends to the default key policy', () => { + const key = new kms.Key(stack, 'MyKey'); + const p = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:Encrypt'] }); + p.addArnPrincipal('arn:aws:iam::111122223333:root'); + key.addToResourcePolicy(p); + + expect(stack).toMatchTemplate({ + Resources: { + MyKey6AB29FA6: { + Type: 'AWS::KMS::Key', + Properties: { + KeyPolicy: { + Statement: [ + { + Action: LEGACY_ADMIN_ACTIONS, + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', + }, + { + Action: 'kms:Encrypt', + Effect: 'Allow', + Principal: { + AWS: 'arn:aws:iam::111122223333:root', + }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }, + DeletionPolicy: 'Retain', + UpdateReplacePolicy: 'Retain', + }, + }, + }); + }); + + test('trustAccountIdentities changes key policy to allow IAM control', () => { + new kms.Key(stack, 'MyKey', { trustAccountIdentities: true }); + expect(stack).toHaveResourceLike('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + { + Action: 'kms:*', + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', + }, + ], + }, + }); + }); + + describe('grants', () => { + test('grant decrypt on a key', () => { + // GIVEN + const key = new kms.Key(stack, 'Key'); + const user = new iam.User(stack, 'User'); + + // WHEN + key.grantDecrypt(user); + + // THEN + expect(stack).toHaveResource('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + // This one is there by default + { + Action: LEGACY_ADMIN_ACTIONS, + Effect: 'Allow', + Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] } }, + Resource: '*', + }, + // This is the interesting one + { + Action: 'kms:Decrypt', + Effect: 'Allow', + Principal: { AWS: { 'Fn::GetAtt': ['User00B015A1', 'Arn'] } }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: 'kms:Decrypt', + Effect: 'Allow', + Resource: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] }, + }, + ], + Version: '2012-10-17', + }, + }); + }); + + test('grant for a principal in a dependent stack works correctly', () => { + const principalStack = new cdk.Stack(app, 'PrincipalStack'); + const principal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AnyPrincipal(), + }); + + const keyStack = new cdk.Stack(app, 'KeyStack'); + const key = new kms.Key(keyStack, 'Key'); + + principalStack.addDependency(keyStack); + + key.grantEncrypt(principal); + + expect(keyStack).toHaveResourceLike('AWS::KMS::Key', { + KeyPolicy: { + Statement: arrayWith({ + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Principal: { + AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':root']] }, + }, + Resource: '*', + }), + }, + }); + }); + }); +}); diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index 84485ddf3c416..499c8e3438fe8 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -65,6 +65,21 @@ export const DOCKER_IGNORE_SUPPORT = '@aws-cdk/aws-ecr-assets:dockerIgnoreSuppor */ export const SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME = '@aws-cdk/aws-secretsmanager:parseOwnedSecretName'; +/** + * KMS Keys start with a default key policy that grants the account access to administer the key, + * mirroring the behavior of the KMS SDK/CLI/Console experience. Users may override the default key + * policy by specifying their own. + * + * If this flag is not set, the default key policy depends on the setting of the `trustAccountIdentities` + * flag. If false (the default, for backwards-compatibility reasons), the default key policy somewhat + * resemebles the default admin key policy, but with the addition of 'GenerateDataKey' permissions. If + * true, the policy matches what happens when this feature flag is set. + * + * Additionally, if this flag is not set and the user supplies a custom key policy, this will be appended + * to the key's default policy (rather than replacing it). + */ +export const KMS_DEFAULT_KEY_POLICIES = '@aws-cdk/aws-kms:defaultKeyPolicies'; + /** * This map includes context keys and values for feature flags that enable * capabilities "from the future", which we could not introduce as the default @@ -84,6 +99,7 @@ export const FUTURE_FLAGS = { [STACK_RELATIVE_EXPORTS_CONTEXT]: 'true', [DOCKER_IGNORE_SUPPORT]: true, [SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME]: true, + [KMS_DEFAULT_KEY_POLICIES]: true, // We will advertise this flag when the feature is complete // [NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: 'true', @@ -100,6 +116,7 @@ const FUTURE_FLAGS_DEFAULTS: { [key: string]: boolean } = { [NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false, [DOCKER_IGNORE_SUPPORT]: false, [SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME]: false, + [KMS_DEFAULT_KEY_POLICIES]: false, }; export function futureFlagDefault(flag: string): boolean {