feat(aws-iam): grants support non-identity principals #1623

rix0rrr · 2019-01-28T12:07:56Z

feat(aws-iam): grants support non-identity principals …
Add support for non-identity Principals in grants (for example,
principals that represent accounts or organization IDs). For resources
that support them, the required IAM statements will be added to the
resource policy. For resources that don't support them (because they
don't have resource policies) an error will be thrown.

Add a new OrganizationPrincipal principal which represents all
identities in the given AWS Organization.

Fixes #236.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Add support for non-identity Principals in grants (for example, principals that represent accounts or organization IDs). For resources that support them, the required IAM statements will be added to the resource policy. For resources that don't support them (because they don't have resource policies) an error will be thrown. Add a new `OrganizationPrincipal` principal which represents all identities in the given AWS Organization. Fixes #236.

rix0rrr · 2019-01-28T12:08:05Z

Pre-emptive implementation FAQ

Why does `IIdentity extends IPrincipal`?

So that we can do:

// What we can do today
bucket.grantRead(role);

// As well as (can't do this today)
bucket.grantRead(new AccountPrincipal('12345678'));
bucket.grantRead(new OrganizationPrincipal('o-asdf'));

But now we have to implement `assumeRoleAction` and `policyFragment` in all Identity objects!

Because of this inheritance we now have to do:

class Role implements IPrincipal {
    public readonly assumeRoleAction = 'sts:AssumeRole';
    public readonly policyFragment: PolicyFragment;

    constructor(...) {
        // ...
        this.policyFragment = new ArnPrincipal(this.roleArn).policyFragment;
    }
}

Can't we use inheritance? Not really, the ARN comes from a different source
every time. The only thing we could share is the assumeRoleAction, and
declaring 4 abstact methods. Doesn't seem worth it. Repeating the implementation is simpler.

The alternative is the "Curiously Recurring Interface Accessor" pattern, but it's
slightly more complex. I think I actually prefer it, but we keep it simple
unless there is consensus that people don't like simple:

// And also we'd need to find a good name for this.
interface IPrincipalable {
    principal: IPrincipal;
}

interface IPrincipal implements IPrincipalable {
    principal: IPrincipal;
    assumeRoleAction: string;
    policyPrincipal: PolicyPrincipal;
    addToPolicy(...);
}

interface IIdentity extends IPrincipalable {
    principal: IPrincipal;
    addManagedPolicy(...);
}

So that we can do:

class ArnPrincipal implements IPrincipal {
    public readonly principal: IPrincipal = this;
}

class Role implements IPrincipalable {
    public readonly principal: IPrincipal;

    constructor(...) {
        // Slightly better because we get to reuse the whole implementation
        // of ArnPrincipal here.
        this.principal = new ArnPrincipal(this.roleArn);
    }
}

The advantage of doing this is to that we can also do:

class Function implements IPrincipalable {
    public readonly principal: IPrincipal;

    constructor(...) {
        this.principal = this.role;
    }
}

// Instead of
bucket.grantRead(lambda.role);

// We can now do
bucket.grantRead(lambda);

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

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

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

packages/@aws-cdk/aws-cloudwatch/lib/metric.ts

packages/@aws-cdk/aws-dynamodb/lib/table.ts

packages/@aws-cdk/aws-iam/README.md

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

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

RomainMuller · 2019-01-29T15:28:35Z

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

+  if (!props.principal) { return true; }
+
+  const addedToPrincipal = props.principal.addToPolicy(new PolicyStatement()
+    .addActions(...props.actions)


If there are not props.actions, then the statement can (should? must?) be skipped. Same goes for resourceArns

The alternative is that that's an error. I see what you're saying, but feels like the chance for this to be a mistake is bigger than for it to be an expected use case?

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

eladb · 2019-02-04T08:12:09Z

@rix0rrr let us know when you want a re-review

rix0rrr · 2019-02-08T18:51:01Z

I need to get around to it again. But nothing majorly blocking in the reviews so far it seems.

rix0rrr · 2019-02-13T20:12:41Z

Ready for that re-review @eladb @RomainMuller

RomainMuller · 2019-02-18T12:22:43Z

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

+      throw new Error(`Either 'scope' or 'resource' must be supplied.`);
+    }
+
+    // One-iteration loop to be able to skip to end of function easily


What's wrong with using an inner function then? 🤷🏻‍♂️ You can return from that whenever you please.

eladb · 2019-02-18T10:19:50Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+      actions,
+      resourceArns: [
+        this.tableArn,
+        new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString()


eladb · 2019-02-18T10:21:35Z

packages/@aws-cdk/aws-iam/README.md

+When we say *Principal*, we mean an entity you grant permissions to. This
+entity can be an AWS Service, a Role, or something more abstract such as "all
+users in this account" or even "all users in this organization". An
+*Identity* is an IAM representing a single IAM entity that can have


"is an IAM resource representing"

eladb · 2019-02-18T10:22:43Z

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

+  resourceArns: string[];
+
+  /**
+   * Adder to the resource policy


Rephrase, it's not an "adder" anymore

eladb · 2019-02-18T10:23:07Z

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

+   * Either 'scope' or 'resource' must be supplied.
+   *
+   * An error will be thrown if the policy could not be added to the principal,
+   * no resource is supplied given and `skipResourcePolicy` is false.


"supplied given"

Kinda feels like it should be possible to supply only resource without resourceArns somehow. Maybe IResourceWithPolicy can have a property resourceArn which will be the canonic resource ARN to be used?

eladb · 2019-02-18T10:24:25Z

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

+   *
+   * @default Same as regular resource ARNs
+   */
+  resourceSelfArns?: string[];


Wouldn't this make more sense as an enum or boolean?

eladb · 2019-02-18T10:26:37Z

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

+    do {
+      if (!options.principal) {
+        // tslint:disable-next-line:max-line-length
+        scope.node.addWarning(`Could not add grant for '${options.actions}' on '${options.resourceArns}' because the principal was not available. Add the permissions by hand.`);


Not sure this should be a warning, but okay to start

eladb · 2019-02-18T12:29:06Z

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

+      throw new Error(`Either 'scope' or 'resource' must be supplied.`);
+    }
+
+    // One-iteration loop to be able to skip to end of function easily


Although this is creative and nice, I rather we keep all our IAM code very simple and straightforward, so it will be dead easy to maintain and reason about.

eladb · 2019-02-18T12:30:16Z

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

+  scope?: cdk.IConstruct;
+}
+
+export class Permissions {


Something feels wrong with this and I am not sure why. Can we talk? 📲

rix0rrr · 2019-02-18T14:52:21Z

We agreed during a chat to make grant methods output an opaque type which will be produced by our grant method.

Also split out grant method into 2 flavours for the different code paths.

eladb · 2019-02-27T15:02:22Z

packages/@aws-cdk/aws-cloudwatch/lib/metric.ts

-    identity.addToPolicy(new iam.PolicyStatement()
-      .addAllResources()
-      .addAction("cloudwatch:PutMetricData"));
+  public static grantPutMetricData(principal?: iam.IPrincipal) {


Shouldn't this return a Grant?

eladb · 2019-02-27T15:02:48Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

@@ -180,11 +181,11 @@ export class Table extends Construct {
   */
  public static grantListStreams(principal?: iam.IPrincipal): void {


should the awslint rule also be applied to static methods?

Probably. One problem with the statics is that we won't have a scope to attach a warning to.

Probably not the end of the world

eladb · 2019-02-27T15:03:21Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

-      .addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())
-      .addActions(...actions));
+  public grant(principal?: iam.IPrincipal, ...actions: string[]): iam.GrantResult {
+    return iam.Permissions.grant({


I was under the impression that we wanted a specific API for resources with resource policies, no?

…to huijbers/iam-refactor

…errror

…to huijbers/iam-refactor

eladb · 2019-04-03T11:40:30Z

packages/@aws-cdk/assets-docker/test/test.image-asset.ts

                ]
              ]
            }
          },
          {
-            "Action": [


Does it make sense that we lost these permissions?

Yes, the user pulling from ECR doesn't need to create logs (they should have never have been part of that grant in the first place).

packages/@aws-cdk/aws-cloudwatch/lib/metric.ts

packages/@aws-cdk/aws-codebuild/lib/project.ts

packages/@aws-cdk/aws-dynamodb-global/lib/aws-dynamodb-global.d.ts

packages/@aws-cdk/aws-dynamodb-global/lib/dynamodb.generated.js

….onPonPrincipalOrResource

rix0rrr · 2019-04-03T12:59:12Z

NuGet build error again. Seems to be getting worse?

Rico Huijbers added 2 commits January 27, 2019 11:33

Introduce Role -> IIdentity -> IPrincipal

19ad316

rix0rrr requested a review from a team as a code owner January 28, 2019 12:07

Forgot to add new file

fcce210

eladb reviewed Jan 29, 2019

View reviewed changes

RomainMuller reviewed Jan 29, 2019

View reviewed changes

rix0rrr self-assigned this Feb 4, 2019

rix0rrr added 2 commits February 11, 2019 13:02

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

f3c7827

Undo move of principals to their own file

d53d794

fulghum added the effort/medium Medium work item – several days of effort label Feb 11, 2019

rix0rrr added 7 commits February 12, 2019 15:24

Change grants API

a445780

Make key interface work with jsii

a43bf24

Splat in the consumer

2a75620

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

dd03daf

Can't have the same function in 2 interfaces in C#

09cffbd

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

295d698

Respect the refactoring

554816d

RomainMuller approved these changes Feb 18, 2019

View reviewed changes

eladb reviewed Feb 18, 2019

View reviewed changes

Add awslint rule to force grant() methods to use helpers

cf68f7d

eladb reviewed Feb 27, 2019

View reviewed changes

rix0rrr added 3 commits February 27, 2019 16:29

WIP

df46221

Update API

8428937

Update ECS expectations

81de979

rix0rrr requested a review from skinny85 as a code owner February 28, 2019 15:44

rix0rrr added 6 commits March 1, 2019 15:21

Merge branch 'huijbers/iam-refactor' of github.com:awslabs/aws-cdk in…

53f103b

…to huijbers/iam-refactor

Review comments

2006891

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

3323f08

IRole implementing both IConstruct and IIdentity leads to a C# build …

b195ac3

…errror

Fix unused import

49f996e

awslint should also find indirect interface extensions

a65d805

rix0rrr requested a review from eladb March 4, 2019 14:06

rix0rrr and others added 9 commits March 20, 2019 16:00

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

f2eab4e

Fixes

77865f5

Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor

0192f0f

Make it build

c9349f6

Fix failing tests

929c6fb

Make principal in grant methods nonoptional

753eb84

Adding IGrantable (WIP)

c2ca705

Adding IGrantable (WIP)

e53474b

Merge branch 'huijbers/iam-refactor' of github.com:awslabs/aws-cdk in…

1b707b1

…to huijbers/iam-refactor

eladb approved these changes Apr 3, 2019

View reviewed changes

rix0rrr added 6 commits April 3, 2019 13:59

Finish introduction of IGrantable, rename Grant.withResource -> Grant…

74bbf35

….onPonPrincipalOrResource

Rename grant methods to be more explicit

71964e7

Remove dynamodb global

361a92e

Update IParameter

3772ba4

Fix stray unrenamed call

0d24421

Make sure JSON.stringify(principal) doesn't recurse indefinitely

b51c76d

rix0rrr merged commit 1bb8ca9 into master Apr 3, 2019

rix0rrr deleted the huijbers/iam-refactor branch April 3, 2019 14:08

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-iam): grants support non-identity principals #1623

feat(aws-iam): grants support non-identity principals #1623

rix0rrr commented Jan 28, 2019

rix0rrr commented Jan 28, 2019 •

edited

Loading

RomainMuller Jan 29, 2019

rix0rrr Feb 11, 2019

eladb commented Feb 4, 2019

rix0rrr commented Feb 8, 2019

rix0rrr commented Feb 13, 2019

RomainMuller Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

eladb Feb 18, 2019

rix0rrr commented Feb 18, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

rix0rrr Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb Apr 3, 2019

rix0rrr Apr 3, 2019 •

edited

Loading

rix0rrr commented Apr 3, 2019

		@@ -180,11 +181,11 @@ export class Table extends Construct {
		*/
		public static grantListStreams(principal?: iam.IPrincipal): void {

feat(aws-iam): grants support non-identity principals #1623

feat(aws-iam): grants support non-identity principals #1623

Conversation

rix0rrr commented Jan 28, 2019

Pull Request Checklist

rix0rrr commented Jan 28, 2019 • edited Loading

Why does IIdentity extends IPrincipal?

But now we have to implement assumeRoleAction and policyFragment in all Identity objects!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Feb 4, 2019

rix0rrr commented Feb 8, 2019

rix0rrr commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

rix0rrr commented Apr 3, 2019

rix0rrr commented Jan 28, 2019 •

edited

Loading

Why does `IIdentity extends IPrincipal`?

But now we have to implement `assumeRoleAction` and `policyFragment` in all Identity objects!

rix0rrr Apr 3, 2019 •

edited

Loading