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

rix0rrr · 2020-01-15T16:18:04Z

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, #2807.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

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.

rix0rrr · 2020-01-15T16:20:28Z

This change might be contentious, in the addition of sameAccount()? and the behavior change of parseArn() (both of which I feel are defensible but also a reasonable person might disagree with the choices made).

Would appreciate some advice here.

Also see the set of test cases for coverage.

aws-cdk-automation · 2020-01-15T16:24:54Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

skinny85

I am OK with this.

Adding the 'do-not-merge' label so that others can chime in as well.

skinny85 · 2020-01-15T21:15:38Z

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


Trailing comma, to avoid the diff problem in the future? :)

skinny85

I've actually changed my mind on this, and I want to explore an alternative solution. I think we should develop something here that's usable across all construct libraries, not specific to IAM. We have a related issue about this around the necessity of having access to account and region on a resource level, instead of just stack level, but I can't find it :/.

I didn't have time yet to dive deep into this issue, but I plan on doing it.

@rix0rrr are you OK not merging this for the time being, and giving me some time to work on it?

rix0rrr · 2020-04-09T11:40:38Z

I've actually changed my mind on this, and I want to explore an alternative solution. I think we should develop something here that's usable across all construct libraries, not specific to IAM.

I'm always in favor of better solutions for anything. As long as we can solve the same issue as mentioned here, go ahead.

We have a related issue about this around the necessity of having access to account and region on a resource level, instead of just stack level, but I can't find it :/.

I guess because of the nature of our deployment mechanism:

Environment of "owned" resources == environment of containing Stack
Environment of "imported" resources == ¯\(ツ)/¯ lol can be anything look at the ARN

That seems to be the crux of it, and you're right it holds for all imported resources, not just roles.

@rix0rrr are you OK not merging this for the time being, and giving me some time to work on it?

As long as you incorporate the tests of this PR into whatever you come up with, sure!

eladb · 2020-04-16T07:15:14Z

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('::', '.') });


why is this needed?

eladb · 2020-04-16T07:16:39Z

packages/@aws-cdk/aws-iam/lib/private/accounts.ts

+ * 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,


Mmmmmokay...

eladb · 2020-04-16T07:17:17Z

packages/@aws-cdk/aws-iam/lib/private/accounts.ts

+  // so treat as the same.
+  if (Token.isUnresolved(account1) && Token.isUnresolved(account2)) { return true; }
+
+  // One agnostic and the other one not means "shug".


Suggested change

// One agnostic and the other one not means "shug".

// One agnostic and the other one not means "shrug".

eladb · 2020-04-16T07:19:42Z

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 {


Maybe return an enum instead of using undefined as a tri-state so this will be easier to reason about and avoid mistakes like (!sameAccount())).

eladb · 2020-04-16T07:22:54Z

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

+   *
+   * Returns `undefined` if it is unknown whether the accounts are the same.
+   */
+  sameAccount(scope: cdk.IConstruct): boolean | undefined;


I feel that this API should accept a cdk.Stack and not a cdk.IConstruct. It will make this API much clearer.

Would it make sense to make this _internal? It's used only inside this module and TBH it feels like it's polluting the interface.

skinny85 · 2020-05-29T22:54:02Z

I've actually changed my mind on this, and I want to explore an alternative solution. I think we should develop something here that's usable across all construct libraries, not specific to IAM.

I'm always in favor of better solutions for anything. As long as we can solve the same issue as mentioned here, go ahead.

We have a related issue about this around the necessity of having access to account and region on a resource level, instead of just stack level, but I can't find it :/.

I guess because of the nature of our deployment mechanism:

Environment of "owned" resources == environment of containing Stack

Environment of "imported" resources == ¯_(ツ)_/¯ lol can be anything look at the ARN

That seems to be the crux of it, and you're right it holds for all imported resources, not just roles.

@rix0rrr are you OK not merging this for the time being, and giving me some time to work on it?

As long as you incorporate the tests of this PR into whatever you come up with, sure!

@rix0rrr sorry this took so long, but this was a tricky one! I've opened #8280 with my proposed solution. It includes your tests from this PR.

I would appreciate if you could take a look, and let me know what you think!

rix0rrr requested review from RomainMuller, skinny85, eladb and iliapolo January 15, 2020 16:18

rix0rrr self-assigned this Jan 15, 2020

rix0rrr requested a review from nija-at January 15, 2020 16:20

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 15, 2020

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Jan 15, 2020

skinny85 approved these changes Jan 15, 2020

View reviewed changes

skinny85 suggested changes Apr 9, 2020

View reviewed changes

eladb suggested changes Apr 16, 2020

View reviewed changes

rix0rrr closed this Jul 9, 2020

rix0rrr deleted the huijbers/import-role-policy branch July 9, 2020 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rix0rrr commented Jan 15, 2020 •

edited

Loading

rix0rrr commented Jan 15, 2020

aws-cdk-automation commented Jan 15, 2020

skinny85 left a comment

skinny85 Jan 15, 2020

skinny85 left a comment

rix0rrr commented Apr 9, 2020 •

edited

Loading

eladb Apr 16, 2020

eladb Apr 16, 2020

eladb Apr 16, 2020

eladb Apr 16, 2020

eladb Apr 16, 2020

skinny85 commented May 29, 2020

	// One agnostic and the other one not means "shug".
	// One agnostic and the other one not means "shrug".

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

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

Conversation

rix0rrr commented Jan 15, 2020 • edited Loading

rix0rrr commented Jan 15, 2020

aws-cdk-automation commented Jan 15, 2020

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Jan 15, 2020

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

rix0rrr commented Apr 9, 2020 • edited Loading

eladb Apr 16, 2020

Choose a reason for hiding this comment

eladb Apr 16, 2020

Choose a reason for hiding this comment

eladb Apr 16, 2020

Choose a reason for hiding this comment

eladb Apr 16, 2020

Choose a reason for hiding this comment

eladb Apr 16, 2020

Choose a reason for hiding this comment

skinny85 commented May 29, 2020

rix0rrr commented Jan 15, 2020 •

edited

Loading

rix0rrr commented Apr 9, 2020 •

edited

Loading