Skip to content

Commit

Permalink
fix(pipelines): cross-region/cross-account key permissions are wrong
Browse files Browse the repository at this point in the history
In #8280 we made a resource's account/region distinct
from the stack in which the construct was defined, to account for
accounts and regions from imported resources.

The pipelines module used to define imported roles in a separate
in-memory Stack so that the old, broken "cross-environment" logic
would do the right thing. That crutch was removed as part of #8280.

The new logic hasn't been carried through everywhere though. For
example, the logic in the grants of KMS keys had not been updated
to match, leading to cross-account/cross-region deployments
being broken (as reported in #10166) because the cross-region support
stack's KMS key had the wrong permissions.

In fact, it switched from:

```
{
    "Action": [ "kms:Decrypt", "kms:DescribeKey" ],
    "Principal": {
        "AWS": {
            "Fn::Sub": "arn:${AWS::Partition}:iam::561462023695:role/cdk-hnb659fds-deploy-role-561462023695-us-east-2"
        }
    },
    "Resource": "*"
}
```

to

```
{
    "Action": [ "kms:Decrypt", "kms:DescribeKey" ],
    "Principal": {
        "AWS": {
            "Fn::Join": [ "", [
                "arn:", { "Ref": "AWS::Partition" }, ":iam::355421412380:root"
            ] ]
        }
    },
    "Resource": "*"
}
```

Ignoring the switch from `Fn::Sub` to `Fn::Join`, it switched from the
`deploy-role` in a DIFFERENT account to the root principal of the SAME
account.
  • Loading branch information
rix0rrr committed Sep 7, 2020
1 parent ffb84c6 commit 118dbfb
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 8 deletions.
9 changes: 2 additions & 7 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, IResource, RemovalPolicy, Resource, Stack, Token, TokenComparison } from '@aws-cdk/core';
import { Alias } from './alias';
import { CfnKey } from './kms.generated';

Expand Down Expand Up @@ -230,12 +230,7 @@ abstract class KeyBase extends Resource implements IKey {
}

private isGranteeFromAnotherAccount(grantee: iam.IGrantable): boolean {
if (!(Construct.isConstruct(grantee))) {
return false;
}
const bucketStack = Stack.of(this);
const identityStack = Stack.of(grantee);
return bucketStack.account !== identityStack.account;
return [TokenComparison.DIFFERENT, TokenComparison.ONE_UNRESOLVED].includes(Token.compareStrings(this.env.account, grantee.grantPrincipal.principalAccount ?? ''));
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/private/refs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function resolveValue(consumer: Stack, reference: CfnReference): IResolvable {
// unsupported: stacks are not in the same environment
if (producer.environment !== consumer.environment) {
throw new Error(
`Stack "${consumer.node.path}" cannot consume a cross reference from stack "${producer.node.path}". ` +
`Stack "${consumer.node.path}" (environment '${consumer.environment}') cannot consume a cross reference from stack "${producer.node.path} (environment '${producer.environment}')". ` +
'Cross stack references are only supported for stacks deployed to the same environment or between nested stacks and their parent stack');
}

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 118dbfb

Please sign in to comment.