diff --git a/packages/@aws-cdk/aws-iam/lib/grant.ts b/packages/@aws-cdk/aws-iam/lib/grant.ts index ec460e325f13d..cfcc8d0f2008e 100644 --- a/packages/@aws-cdk/aws-iam/lib/grant.ts +++ b/packages/@aws-cdk/aws-iam/lib/grant.ts @@ -119,7 +119,10 @@ export class Grant { scope: options.resource }); - if (result.success) { return result; } + // If we added to the principal AND we're in the same account, then we're done. + // If not, it's a different account and we must also add a trust policy on the resource. + const definitelySameAccount = options.grantee.grantPrincipal.sameAccount(options.resource) === true; + if (result.success && definitelySameAccount) { return result; } const statement = new PolicyStatement({ actions: options.actions, diff --git a/packages/@aws-cdk/aws-iam/lib/group.ts b/packages/@aws-cdk/aws-iam/lib/group.ts index 7a755b85cf26f..0e21ab5b8555f 100644 --- a/packages/@aws-cdk/aws-iam/lib/group.ts +++ b/packages/@aws-cdk/aws-iam/lib/group.ts @@ -1,10 +1,11 @@ -import { Construct, Lazy, Resource, Stack } from '@aws-cdk/core'; +import { Construct, IConstruct, Lazy, Resource, Stack } from '@aws-cdk/core'; import { CfnGroup } from './iam.generated'; import { IIdentity } from './identity-base'; import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { sameAccount } from './private/accounts'; import { IUser } from './user'; import { AttachedPolicies } from './util'; @@ -105,6 +106,10 @@ abstract class GroupBase extends Resource implements IGroup { this.defaultPolicy.addStatements(statement); return true; } + + public sameAccount(scope: IConstruct): boolean | undefined { + return sameAccount(Stack.of(this).account, Stack.of(scope).account); + } } export class Group extends GroupBase { diff --git a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts index 03ffdd29d282a..056cf88a6727b 100644 --- a/packages/@aws-cdk/aws-iam/lib/lazy-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/lazy-role.ts @@ -4,6 +4,7 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { IPrincipal, PrincipalPolicyFragment } from './principals'; +import { sameAccount } from './private/accounts'; import { IRole, Role, RoleProps } from './role'; // tslint:disable-next-line:no-empty-interface @@ -107,6 +108,10 @@ export class LazyRole extends cdk.Resource implements IRole { return this.instantiate().grantPassRole(identity); } + public sameAccount(scope: cdk.IConstruct): boolean | undefined { + return sameAccount(cdk.Stack.of(this).account, cdk.Stack.of(scope).account); + } + private instantiate(): Role { if (!this.role) { const role = new Role(this, 'Default', this.props); diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts index 2049e910c4bb0..ab4fcd263aecd 100644 --- a/packages/@aws-cdk/aws-iam/lib/principals.ts +++ b/packages/@aws-cdk/aws-iam/lib/principals.ts @@ -48,6 +48,17 @@ export interface IPrincipal extends IGrantable { * question does not have a policy document to add the statement to. */ addToPolicy(statement: PolicyStatement): boolean; + + /** + * Return whether the principal and the given construct are in the same AWS account. + * + * This is used by `Grant` to determine whether a trust policy needs + * to be added to the resource as well as a permissions policy needs to be + * added to the identity. + * + * Returns `undefined` if it is unknown whether the accounts are the same. + */ + sameAccount(scope: cdk.IConstruct): boolean | undefined; } /** @@ -78,6 +89,10 @@ export abstract class PrincipalBase implements IPrincipal { return JSON.stringify(this.policyFragment.principalJson); } + public sameAccount(_scope: cdk.Construct): boolean | undefined { + return false; + } + public toJSON() { // Have to implement toJSON() because the default will lead to infinite recursion. return this.policyFragment.principalJson; diff --git a/packages/@aws-cdk/aws-iam/lib/private/accounts.ts b/packages/@aws-cdk/aws-iam/lib/private/accounts.ts new file mode 100644 index 0000000000000..bd6f469e8ad1e --- /dev/null +++ b/packages/@aws-cdk/aws-iam/lib/private/accounts.ts @@ -0,0 +1,26 @@ +import { Token } from '@aws-cdk/core'; + +/** + * Return whether the given accounts are definitely different. + * + * If one or both of them are agnostic, return false (we don't know). + */ +export function accountsAreDefinitelyDifferent(account1: string | undefined, + account2: string | undefined): boolean { + return !Token.isUnresolved(account1) && !Token.isUnresolved(account2) && account1 !== account2; +} + +/** + * Return whether two accounts are the same account + * + * Returns undefined if one is agnostic and the other one isn't. + */ +export function sameAccount(account1: string | undefined, account2: string | undefined): boolean | undefined { + // Both agnostic in 99% of cases means they will be deployed to the same environment, + // so treat as the same. + if (Token.isUnresolved(account1) && Token.isUnresolved(account2)) { return true; } + + // One agnostic and the other one not means "shug". + if (Token.isUnresolved(account1) || Token.isUnresolved(account2)) { return undefined; } + return account1 === account2; +} diff --git a/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts b/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts index d894cc8dbc722..2f99fcf6186cf 100644 --- a/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts +++ b/packages/@aws-cdk/aws-iam/lib/private/immutable-role.ts @@ -1,4 +1,4 @@ -import { DependableTrait } from '@aws-cdk/core'; +import { DependableTrait, IConstruct } from '@aws-cdk/core'; import { Grant } from '../grant'; import { IManagedPolicy } from '../managed-policy'; import { Policy } from '../policy'; @@ -22,7 +22,7 @@ import { IRole } from '../role'; export class ImmutableRole implements IRole { public readonly assumeRoleAction = this.role.assumeRoleAction; public readonly policyFragment = this.role.policyFragment; - public readonly grantPrincipal = this.role.grantPrincipal; + public readonly grantPrincipal = this; public readonly roleArn = this.role.roleArn; public readonly roleName = this.role.roleName; public readonly node = this.role.node; @@ -55,4 +55,8 @@ export class ImmutableRole implements IRole { public grantPassRole(grantee: IPrincipal): Grant { return this.role.grantPassRole(grantee); } + + public sameAccount(scope: IConstruct): boolean | undefined { + return this.role.sameAccount(scope); + } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/lib/role.ts b/packages/@aws-cdk/aws-iam/lib/role.ts index 7d3398da3a197..82c0a46a0238d 100644 --- a/packages/@aws-cdk/aws-iam/lib/role.ts +++ b/packages/@aws-cdk/aws-iam/lib/role.ts @@ -1,4 +1,4 @@ -import { Construct, Duration, Lazy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Construct, Duration, IConstruct, Lazy, Resource, Stack } from '@aws-cdk/core'; import { Grant } from './grant'; import { CfnRole } from './iam.generated'; import { IIdentity } from './identity-base'; @@ -7,6 +7,7 @@ import { Policy } from './policy'; import { PolicyDocument } from './policy-document'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { accountsAreDefinitelyDifferent, sameAccount } from './private/accounts'; import { ImmutableRole } from './private/immutable-role'; import { AttachedPolicies } from './util'; @@ -184,7 +185,7 @@ export class Role extends Resource implements IRole { public attachInlinePolicy(policy: Policy): void { const policyAccount = Stack.of(policy).account; - if (accountsAreEqualOrOneIsUnresolved(policyAccount, roleAccount)) { + if (!accountsAreDefinitelyDifferent(policyAccount, roleAccount)) { this.attachedPolicies.attach(policy); policy.attachToRole(this); } @@ -212,21 +213,17 @@ export class Role extends Resource implements IRole { scope: this, }); } + + public sameAccount(scp: IConstruct): boolean | undefined { + return sameAccount(parsedArn.account, Stack.of(scp).account); + } } const roleAccount = parsedArn.account; - const scopeAccount = scopeStack.account; - return options.mutable !== false && accountsAreEqualOrOneIsUnresolved(scopeAccount, roleAccount) - ? new Import(scope, id) - : new ImmutableRole(new Import(scope, id)); - - function accountsAreEqualOrOneIsUnresolved(account1: string | undefined, - account2: string | undefined): boolean { - return Token.isUnresolved(account1) || Token.isUnresolved(account2) || - account1 === account2; - } + const mutable = options.mutable !== false && !accountsAreDefinitelyDifferent(scopeAccount, roleAccount); + return mutable ? new Import(scope, id) : new ImmutableRole(new Import(scope, id)); } public readonly grantPrincipal: IPrincipal = this; @@ -382,6 +379,10 @@ export class Role extends Resource implements IRole { public withoutPolicyUpdates(): IRole { return new ImmutableRole(this); } + + public sameAccount(scope: Construct): boolean | undefined { + return sameAccount(Stack.of(this).account, Stack.of(scope).account); + } } /** diff --git a/packages/@aws-cdk/aws-iam/lib/unknown-principal.ts b/packages/@aws-cdk/aws-iam/lib/unknown-principal.ts index 181d2780bc513..0cdfcc4af8784 100644 --- a/packages/@aws-cdk/aws-iam/lib/unknown-principal.ts +++ b/packages/@aws-cdk/aws-iam/lib/unknown-principal.ts @@ -43,4 +43,8 @@ export class UnknownPrincipal implements IPrincipal { this.resource.node.addWarning(`Add statement to this resource's role: ${repr}`); return true; // Pretend we did the work. The human will do it for us, eventually. } + + public sameAccount(_scope: IConstruct): boolean | undefined { + return undefined; + } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/lib/user.ts b/packages/@aws-cdk/aws-iam/lib/user.ts index 977209f33252c..2a7d91ef03e34 100644 --- a/packages/@aws-cdk/aws-iam/lib/user.ts +++ b/packages/@aws-cdk/aws-iam/lib/user.ts @@ -1,4 +1,4 @@ -import { Construct, Lazy, Resource, SecretValue, Stack } from '@aws-cdk/core'; +import { Construct, IConstruct, Lazy, Resource, SecretValue, Stack } from '@aws-cdk/core'; import { IGroup } from './group'; import { CfnUser } from './iam.generated'; import { IIdentity } from './identity-base'; @@ -6,6 +6,7 @@ import { IManagedPolicy } from './managed-policy'; import { Policy } from './policy'; import { PolicyStatement } from './policy-statement'; import { ArnPrincipal, IPrincipal, PrincipalPolicyFragment } from './principals'; +import { sameAccount } from './private/accounts'; import { AttachedPolicies, undefinedIfEmpty } from './util'; export interface IUser extends IIdentity { @@ -153,6 +154,10 @@ export class User extends Resource implements IIdentity, IUser { public addManagedPolicy(_policy: IManagedPolicy): void { throw new Error('Cannot add managed policy to imported User'); } + + public sameAccount(_scope: IConstruct): boolean | undefined { + return true; + } } return new Import(scope, id); @@ -256,6 +261,10 @@ export class User extends Resource implements IIdentity, IUser { return true; } + public sameAccount(scope: IConstruct): boolean | undefined { + return sameAccount(Stack.of(this).account, Stack.of(scope).account); + } + private parseLoginProfile(props: UserProps): CfnUser.LoginProfileProperty | undefined { if (props.password) { return { diff --git a/packages/@aws-cdk/aws-iam/test/cross-account.test.ts b/packages/@aws-cdk/aws-iam/test/cross-account.test.ts new file mode 100644 index 0000000000000..ff3c64c065d21 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/test/cross-account.test.ts @@ -0,0 +1,221 @@ +import '@aws-cdk/assert/jest'; +import * as cdk from '@aws-cdk/core'; +import * as iam from '../lib'; + +// Test cross-account grant scenario's for principals +// +// When doing a grant on a resource with a resource policy: +// +// - Permissions are added to the identity if possible. +// - Trust is added to the resource if necessary (identity is in +// a different account than the resource). + +let app: cdk.App; +let stack1: cdk.Stack; +let stack2: cdk.Stack; + +beforeEach(() => { + app = new cdk.App(); + stack1 = new cdk.Stack(app, 'Stack1', { env: { account: '1234', region: 'us-bla-5' }}); + stack2 = new cdk.Stack(app, 'Stack2', { env: { account: '5678', region: 'us-bla-5' }}); +}); + +test('cross-account Role grant creates permissions AND trust', () => { + // GIVEN + const role = new iam.Role(stack1, 'Role', { + roleName: cdk.PhysicalName.GENERATE_IF_NEEDED, + assumedBy: new iam.ServicePrincipal('some.service'), + }); + const resource = new FakeResource(stack2, 'Resource'); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack1); + assertTrustCreated(stack2, { AWS: { + "Fn::Join": [ "", [ + "arn:", + { Ref: "AWS::Partition" }, + ":iam::1234:role/stack1stack1rolef3c14260253562f428b7" + ] ] + }}); +}); + +test('Service Principal grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + + // WHEN + doGrant(resource, new iam.ServicePrincipal('service.amazonaws.com')); + + // THEN + assertTrustCreated(stack2, { Service: 'service.amazonaws.com' }); +}); + +test('Imported Role with definitely different account grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', 'arn:aws:iam::123456789012:role/S3Access', { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + noPolicyCreated(stack2); + assertTrustCreated(stack2, { AWS: 'arn:aws:iam::123456789012:role/S3Access' }); +}); + +test('Imported Role with parition token in ARN (definitely different account) grant creates trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:${stack2.partition}:iam::123456789012:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + noPolicyCreated(stack2); + assertTrustCreated(stack2, { AWS: { "Fn::Join": [ "", [ + "arn:", + { Ref: "AWS::Partition" }, + ":iam::123456789012:role/S3Access" + ] ] } }); +}); + +test('Imported Role with definitely same account grant does not create trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', 'arn:aws:iam::5678:role/S3Access', { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack2); + noTrustCreated(stack2); +}); + +test('Imported Role with partition token and definitely same account grant does not create trust', () => { + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:${stack2.partition}:iam::5678:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertPolicyCreated(stack2); + noTrustCreated(stack2); +}); + +test('Agnostic stack with concrete imported role adds trust', () => { + // GIVEN + const stack = new cdk.Stack(app, 'AgStack'); + const resource = new FakeResource(stack, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', 'arn:aws:iam::5678:role/S3Access', { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertTrustCreated(stack, { AWS: 'arn:aws:iam::5678:role/S3Access' }); + noPolicyCreated(stack); +}); + +test('Agnostic stack with agnostic imported role does not add trust', () => { + // GIVEN + const stack = new cdk.Stack(app, 'AgStack'); + const resource = new FakeResource(stack, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', `arn:aws:iam::${cdk.Aws.ACCOUNT_ID}:role/S3Access`, { mutable: true }); + + // WHEN + doGrant(resource, role); + + // THEN + assertTrustCreated(stack2, { AWS: 'arn:aws:iam::123456789012:role/S3Access' }); + assertPolicyCreated(stack2); +}); + +test('Immutable role in same account adds no policy and no trust', () => { + // GIVEN + const resource = new FakeResource(stack2, 'Resource'); + const role = iam.Role.fromRoleArn(stack2, 'Role', 'arn:aws:iam::5678:role/S3Access', { mutable: false }); + + require('util').inspect.defaultOptions.customInspect = false; + + // WHEN + doGrant(resource, role); + + // THEN + noTrustCreated(stack2); + noPolicyCreated(stack2); + +}); + +class FakeResource extends cdk.CfnResource implements iam.IResourceWithPolicy { + public readonly arn = 'arn:aws:resource'; + private readonly policy = new iam.PolicyDocument(); + + constructor(scope: cdk.Construct, id: string) { + super(scope, id, { + type: 'Test::Fake::Resource', + properties: { + ResourcePolicy: cdk.Lazy.anyValue({ produce: () => this.policy }), + } + }); + } + + public addToResourcePolicy(statement: iam.PolicyStatement): void { + this.policy.addStatements(statement); + } +} + +function doGrant(resource: FakeResource, principal: iam.IPrincipal) { + iam.Grant.addToPrincipalOrResource({ + actions: ['some:action'], + grantee: principal, + resourceArns: [resource.arn], + resource, + }); +} + +function assertTrustCreated(stack: cdk.Stack, principal: any) { + expect(stack).toHaveResource('Test::Fake::Resource', { + ResourcePolicy: { + Statement: [ + { + Action: "some:action", + Effect: "Allow", + Resource: "arn:aws:resource", + Principal: principal, + } + ], + Version: "2012-10-17" + }, +}); +} + +function noTrustCreated(stack: cdk.Stack) { + expect(stack).not.toHaveResourceLike('Test::Fake::Resource', { + ResourcePolicy: { + Statement: [ + {}, + ] + } + }); +} + +function assertPolicyCreated(stack: cdk.Stack) { + expect(stack).toHaveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: "some:action", + Effect: "Allow", + Resource: "arn:aws:resource" + } + ], + Version: "2012-10-17" + }, + }); +} + +function noPolicyCreated(stack: cdk.Stack) { + expect(stack).not.toHaveResource('AWS::IAM::Policy'); +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts index 3f5388e6d75de..58fd5a8187386 100644 --- a/packages/@aws-cdk/aws-iam/test/policy-document.test.ts +++ b/packages/@aws-cdk/aws-iam/test/policy-document.test.ts @@ -341,7 +341,8 @@ describe('IAM polocy document', () => { get grantPrincipal() { return this; }, assumeRoleAction: 'sts:AssumeRole', policyFragment: new PrincipalPolicyFragment({ AWS: ['foo', 'bar'] }), - addToPolicy() { return false; } + addToPolicy() { return false; }, + sameAccount() { return false; } }; const s = new PolicyStatement(); s.addAccountRootPrincipal(); diff --git a/packages/@aws-cdk/core/lib/arn.ts b/packages/@aws-cdk/core/lib/arn.ts index 31a6a22106d65..863a6bcd939f1 100644 --- a/packages/@aws-cdk/core/lib/arn.ts +++ b/packages/@aws-cdk/core/lib/arn.ts @@ -135,12 +135,16 @@ export class Arn { * components of the ARN. */ public static parse(arn: string, sepIfToken: string = '/', hasName: boolean = true): ArnComponents { - if (Token.isUnresolved(arn)) { + const components = arn.split(':') as Array<string | undefined>; + const looksLikeArn = arn.startsWith('arn:') && components.length >= 6 && components.length <= 7; + + // If the ARN merely contains Tokens, but otherwise *looks* mostly like an ARN, + // it's a string of the form 'arn:${partition}:service:us-west-1:${account}:abc/xyz'. Parse fields + // out to the best of our ability. Tokens won't contain ":" so this won't break them. + if (Token.isUnresolved(arn) && !looksLikeArn) { return parseToken(arn, sepIfToken, hasName); } - const components = arn.split(':') as Array<string | undefined>; - if (components.length < 6) { throw new Error('ARNs must have at least 6 components: ' + arn); } diff --git a/packages/@aws-cdk/core/lib/cfn-pseudo.ts b/packages/@aws-cdk/core/lib/cfn-pseudo.ts index 4326a1ab7eec5..4c3f96d7c3c71 100644 --- a/packages/@aws-cdk/core/lib/cfn-pseudo.ts +++ b/packages/@aws-cdk/core/lib/cfn-pseudo.ts @@ -77,5 +77,5 @@ export class ScopedAws { } function pseudoString(name: string): string { - return Token.asString({ Ref: name }, { displayHint: name }); + return Token.asString({ Ref: name }, { displayHint: name.replace('::', '.') }); } diff --git a/packages/@aws-cdk/core/test/test.arn.ts b/packages/@aws-cdk/core/test/test.arn.ts index d8eb7d48ee6e9..8321d6a33c719 100644 --- a/packages/@aws-cdk/core/test/test.arn.ts +++ b/packages/@aws-cdk/core/test/test.arn.ts @@ -1,5 +1,5 @@ import { Test } from 'nodeunit'; -import { ArnComponents, CfnOutput, ScopedAws, Stack } from '../lib'; +import { ArnComponents, Aws, CfnOutput, ScopedAws, Stack } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { toCloudFormation } from './util'; @@ -233,4 +233,23 @@ export = { test.done(); }, + + 'parse other fields if only some are tokens'(test: Test) { + // GIVEN + const stack = new Stack(); + + // WHEN + const parsed = stack.parseArn(`arn:${Aws.PARTITION}:iam::123456789012:role/S3Access`); + + // THEN + test.deepEqual(stack.resolve(parsed.partition), { Ref: 'AWS::Partition' }); + test.deepEqual(stack.resolve(parsed.service), 'iam'); + test.equal(stack.resolve(parsed.region), undefined); // Note: This is wrong! It should be '', but parseArn() is incorrect. + test.deepEqual(stack.resolve(parsed.account), '123456789012'); + test.deepEqual(stack.resolve(parsed.resource), 'role'); + test.deepEqual(stack.resolve(parsed.resourceName), 'S3Access'); + test.equal(parsed.sep, '/'); + + test.done(); + }, };