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

Trust policies should be added to resources when trying to grant
permissions to a principal in a different account.  Typically this is a
Service Principal, but it could also be an imported Role from a
different account (and the same should not be done for a Role in the
same account).

Our previous API (`addToPolicy` returning `true|false`) was designed for
in-account Roles and cross-account ServicePrincipals, but was not rich
enough to make the distinction that Roles could be cross-account.

Add a function to `IPrincipal` to ask it whether it is in the same or a
different account from a given scope, and have `Grant` respect that.

Also in this commit: a bugfix for `ImmutableRole` to make it work
correctly with `Grant`

Also in this commit: `parseArn()` used to behave the same for
`parseArn(TOKEN)` and `parseArn("arn:{TOKEN}:svc:1234:...")`.
Make it (opportunistically) do the right thing in the latter case,
parsing out the concrete components and the TOKEN components
separately. This makes it possible to parse out the concrete
account number from an ARN that contains tokenized parts we don't
care about.

Fixes #5740.

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 {