Skip to content

Commit

Permalink
fix(kms): do not change the principal to root for imported resources …
Browse files Browse the repository at this point in the history
…in dependent Stacks (#10299)

We have logic in KMS Key that checks whether the grantee is from a Stack that depends on the Key's Stack.
It's required because KMS validated that the principals contained in its Key policy actually exist,
and fails if they don't, so in that case, we switch to using the root principal instead.
However, that logic only makes sense for newly created resources;
for imported resources, like those with `Role.fromRoleArn`,
they already exist, so no need to make this switch.

Fixes  #10166.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
skinny85 authored Sep 11, 2020
1 parent 698e5ef commit 54dfe83
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 3 deletions.
21 changes: 18 additions & 3 deletions packages/@aws-cdk/aws-kms/lib/key.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as iam from '@aws-cdk/aws-iam';
import { Construct, IResource, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
import { Construct, IConstruct, IResource, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
import { Alias } from './alias';
import { CfnKey } from './kms.generated';

Expand Down Expand Up @@ -207,11 +207,18 @@ abstract class KeyBase extends Resource implements IKey {
* undefined otherwise
*/
private granteeStackDependsOnKeyStack(grantee: iam.IGrantable): string | undefined {
if (!(Construct.isConstruct(grantee))) {
const grantPrincipal = grantee.grantPrincipal;
if (!(Construct.isConstruct(grantPrincipal))) {
return undefined;
}
// this logic should only apply to newly created
// (= not imported) resources
if (!this.principalIsANewlyCreatedResource(grantPrincipal)) {
return undefined;
}
// return undefined;
const keyStack = Stack.of(this);
const granteeStack = Stack.of(grantee);
const granteeStack = Stack.of(grantPrincipal);
if (keyStack === granteeStack) {
return undefined;
}
Expand All @@ -220,6 +227,14 @@ abstract class KeyBase extends Resource implements IKey {
: undefined;
}

private principalIsANewlyCreatedResource(principal: IConstruct): boolean {
// yes, this sucks
// this is just a temporary stopgap to stem the bleeding while we work on a proper fix
return principal instanceof iam.Role ||
principal instanceof iam.User ||
principal instanceof iam.Group;
}

private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean {
if (!(Construct.isConstruct(grantee))) {
return false;
Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/pipelines/test/cross-environment-infra.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,24 @@ test('in a cross-account/cross-region setup, artifact bucket can be read by depl
})),
},
});

// And the key to go along with it
expect(supportStack).toHaveResourceLike('AWS::KMS::Key', {
KeyPolicy: {
Statement: arrayWith(objectLike({
Action: arrayWith('kms:Decrypt', 'kms:DescribeKey'),
Principal: {
AWS: {
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
stringLike('*-deploy-role-*'),
]],
},
},
})),
},
});
});

test('in a cross-account/same-region setup, artifact bucket can be read by deploy role', () => {
Expand Down

0 comments on commit 54dfe83

Please sign in to comment.