fix(iam): no trust policy added for cross-account roles

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,

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 {

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);

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;

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;
+}

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);
+  }
 }

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);
+  }
 }
 
 /**

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;
+  }
 }

packages/@aws-cdk/aws-iam/lib/user.ts

@@ -1,11 +1,12 @@
-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';
 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 {