Skip to content

Commit

Permalink
fix(kms): kms key grant methods misidentify region when enclosing sta…
Browse files Browse the repository at this point in the history
…ck is different region (#29315)

### Issue # (if applicable)

Closes #29308

### Reason for this change

This problem is grant() determines the region of a Key using
Stack.of(key).region, however the enclosing Stack's region may differ to
that of the actual resource. When this happens, the IAM policy generated
allows a `*` resource which is against the least privilege rule.

### Description of changes

KMS key already has `env` value on account and region, use this first.
If not exist, use stack account and region.

### Description of how you validated changes

New unit test

### Checklist
- [X] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

---------

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
  • Loading branch information
GavinZZ and paulhcsun authored Mar 14, 2024
1 parent fac4a9c commit 9076d6e
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 1 deletion.
12 changes: 12 additions & 0 deletions packages/aws-cdk-lib/aws-kms/lib/key.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,12 @@ abstract class KeyBase extends Resource implements IKey {
}
const bucketStack = Stack.of(this);
const identityStack = Stack.of(grantee.grantPrincipal);

if (FeatureFlags.of(this).isEnabled(cxapi.KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE)) {
// if two compared stacks have the same region, this should return 'false' since it's from the
// same region; if two stacks have different region, then compare env.region
return bucketStack.region !== identityStack.region && this.env.region !== identityStack.region;
}
return bucketStack.region !== identityStack.region;
}

Expand All @@ -271,6 +277,12 @@ abstract class KeyBase extends Resource implements IKey {
}
const bucketStack = Stack.of(this);
const identityStack = Stack.of(grantee.grantPrincipal);

if (FeatureFlags.of(this).isEnabled(cxapi.KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE)) {
// if two compared stacks have the same region, this should return 'false' since it's from the
// same region; if two stacks have different region, then compare env.account
return bucketStack.account !== identityStack.account && this.env.account !== identityStack.account;
}
return bucketStack.account !== identityStack.account;
}
}
Expand Down
61 changes: 61 additions & 0 deletions packages/aws-cdk-lib/aws-kms/test/key.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describeDeprecated } from '@aws-cdk/cdk-build-tools';
import { Match, Template } from '../../assertions';
import * as iam from '../../aws-iam';
import * as cdk from '../../core';
import * as cxapi from '../../cx-api';
import * as kms from '../lib';
import { KeySpec, KeyUsage } from '../lib';

Expand Down Expand Up @@ -81,6 +82,66 @@ describe('key policies', () => {
});
});

test('cross region key with iam role grant', () => {
const app = new cdk.App({ context: { [cxapi.KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE]: true } });
const stack = new cdk.Stack(app, 'test-stack', { env: { account: '000000000000', region: 'us-west-2' } });
const key = kms.Key.fromKeyArn(
stack,
'Key',
'arn:aws:kms:eu-north-1:000000000000:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287',
);

const roleStack = new cdk.Stack(app, 'RoleStack', {
env: { account: '000000000000', region: 'eu-north-1' },
});
const role = new iam.Role(roleStack, 'Role', {
assumedBy: new iam.AccountPrincipal('000000000000'),
});
key.grantEncryptDecrypt(role);

Template.fromStack(roleStack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Effect: 'Allow',
Resource: 'arn:aws:kms:eu-north-1:000000000000:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287',
},
],
Version: '2012-10-17',
},
});
});

test('cross region key with iam role grant when feature flag is disabled', () => {
const app = new cdk.App({ context: { [cxapi.KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE]: false } });
const stack = new cdk.Stack(app, 'test-stack', { env: { account: '000000000000', region: 'us-west-2' } });
const key = kms.Key.fromKeyArn(
stack,
'Key',
'arn:aws:kms:eu-north-1:000000000000:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287',
);

const roleStack = new cdk.Stack(app, 'RoleStack', {
env: { account: '000000000000', region: 'eu-north-1' },
});
const role = new iam.Role(roleStack, 'Role', {
assumedBy: new iam.AccountPrincipal('000000000000'),
});
key.grantEncryptDecrypt(role);

Template.fromStack(roleStack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Effect: 'Allow',
Resource: '*',
},
],
Version: '2012-10-17',
},
});
});

test('can append to the default key policy', () => {
const stack = new cdk.Stack();
const statement = new iam.PolicyStatement({ resources: ['*'], actions: ['kms:Put*'] });
Expand Down
18 changes: 17 additions & 1 deletion packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ Flags come in three types:
| [@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction](#aws-cdkaws-cloudwatch-actionschangelambdapermissionlogicalidforlambdaaction) | When enabled, the logical ID of a Lambda permission for a Lambda action includes an alarm ID. | 2.124.0 | (fix) |
| [@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse](#aws-cdkaws-codepipelinecrossaccountkeysdefaultvaluetofalse) | Enables Pipeline to set the default value for crossAccountKeys to false. | 2.127.0 | (default) |
| [@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2](#aws-cdkaws-codepipelinedefaultpipelinetypetov2) | Enables Pipeline to set the default pipeline type to V2. | V2NEXT | (default) |
| [@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope](#aws-cdkaws-kmsreducecrossaccountregionpolicyscope) | When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only. | V2NEXT | (fix) |

<!-- END table -->

Expand Down Expand Up @@ -122,7 +123,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-codepipeline-actions:useNewDefaultBranchForCodeCommitSource": true,
"@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction": true,
"@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse": true,
"@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2": true
"@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2": true,
"@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope": true
}
}
```
Expand Down Expand Up @@ -1249,4 +1251,18 @@ construct, the construct automatically defaults the value of this property to `P
**Compatibility with old behavior:** Pass `pipelineType: PipelineType.V1` to `Pipeline` construct to restore the previous behavior.


### @aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope

*When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only.* (fix)

When this feature flag is enabled and calling KMS key grant method, the created IAM policy will reduce the resource scope from
'*' to this specific granting KMS key.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `true` |


<!-- END details -->
17 changes: 17 additions & 0 deletions packages/aws-cdk-lib/cx-api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -292,3 +292,20 @@ _cdk.json_
}
}
```

* `@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope`

Reduce resource scope of the IAM Policy created from KMS key grant to granting key only.

When this feature flag is enabled and calling KMS key grant method, the created IAM policy will reduce the resource scope from
'*' to this specific granting KMS key.

_cdk.json_

```json
{
"context": {
"@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope": true
}
}
```
13 changes: 13 additions & 0 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export const CODECOMMIT_SOURCE_ACTION_DEFAULT_BRANCH_NAME = '@aws-cdk/aws-codepi
export const LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION = '@aws-cdk/aws-cloudwatch-actions:changeLambdaPermissionLogicalIdForLambdaAction';
export const CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE = '@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse';
export const CODEPIPELINE_DEFAULT_PIPELINE_TYPE_TO_V2 = '@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2';
export const KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE = '@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope';

export const FLAGS: Record<string, FlagInfo> = {
//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -1021,6 +1022,18 @@ export const FLAGS: Record<string, FlagInfo> = {
recommendedValue: true,
compatibilityWithOldBehaviorMd: 'Pass `pipelineType: PipelineType.V1` to `Pipeline` construct to restore the previous behavior.',
},

//////////////////////////////////////////////////////////////////////
[KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE]: {
type: FlagType.BugFix,
summary: 'When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only.',
detailsMd: `
When this feature flag is enabled and calling KMS key grant method, the created IAM policy will reduce the resource scope from
'*' to this specific granting KMS key.
`,
introducedIn: { v2: 'V2NEXT' },
recommendedValue: true,
},
};

const CURRENT_MV = 'v2';
Expand Down

0 comments on commit 9076d6e

Please sign in to comment.