Skip to content

Commit

Permalink
fix(kms): imported key ignores environment from arn (aws#21519)
Browse files Browse the repository at this point in the history
Fixes aws#21464. KMS keys imported using `fromKeyArn()` currently take the environment of the stack, not the environment from the arn.

This PR follows the precedent set in aws#19026 and aws#18255. It is essentially the same code change and tests. Ideally, we would have a mechanism for testing all `fromXxxArn` APIs to ensure they have the correct behavior. There are still many places where it does not. However, given the significant overhead of creating such a mechanism, I'm creating this one-off PR to unblock users in KMS.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
kaizencc authored and josephedward committed Aug 30, 2022
1 parent 253d32e commit 76bcae2
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 6 deletions.
14 changes: 8 additions & 6 deletions packages/@aws-cdk/aws-kms/lib/key.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as iam from '@aws-cdk/aws-iam';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Duration, Token, ContextProvider, Arn, ArnFormat } from '@aws-cdk/core';
import { FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, Stack, Duration, Token, ContextProvider, Arn, ArnFormat } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { Alias } from './alias';
Expand Down Expand Up @@ -94,8 +94,8 @@ abstract class KeyBase extends Resource implements IKey {
*/
private readonly aliases: Alias[] = [];

constructor(scope: Construct, id: string) {
super(scope, id);
constructor(scope: Construct, id: string, props: ResourceProps = {}) {
super(scope, id, props);

this.node.addValidation({ validate: () => this.policy?.validateForResourcePolicy() ?? [] });
}
Expand Down Expand Up @@ -464,8 +464,8 @@ export class Key extends KeyBase {
// policies is really the only option
protected readonly trustAccountIdentities: boolean = true;

constructor(keyId: string) {
super(scope, id);
constructor(keyId: string, props: ResourceProps = {}) {
super(scope, id, props);

this.keyId = keyId;
}
Expand All @@ -476,7 +476,9 @@ export class Key extends KeyBase {
throw new Error(`KMS key ARN must be in the format 'arn:aws:kms:<region>:<account>:key/<keyId>', got: '${keyArn}'`);
}

return new Import(keyResourceName);
return new Import(keyResourceName, {
environmentFromArn: keyArn,
});
}

/**
Expand Down
31 changes: 31 additions & 0 deletions packages/@aws-cdk/aws-kms/test/key.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1240,3 +1240,34 @@ describe('key specs and key usages', () => {
.toThrow('key rotation cannot be enabled on asymmetric keys');
});
});

describe('Key.fromKeyArn()', () => {
let stack: cdk.Stack;

beforeEach(() => {
const app = new cdk.App();
stack = new cdk.Stack(app, 'Base', {
env: { account: '111111111111', region: 'stack-region' },
});
});

describe('for a key in a different account and region', () => {
let key: kms.IKey;

beforeEach(() => {
key = kms.Key.fromKeyArn(
stack,
'iKey',
'arn:aws:kms:key-region:222222222222:key:key-name',
);
});

test("the key's region is taken from the ARN", () => {
expect(key.env.region).toBe('key-region');
});

test("the key's account is taken from the ARN", () => {
expect(key.env.account).toBe('222222222222');
});
});
});

0 comments on commit 76bcae2

Please sign in to comment.