feat(iam): `Role.withoutPolicyUpdates()` #5569

rix0rrr · 2019-12-27T15:03:51Z

Add a new method to Role called withoutPolicyUpdates(), which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK.

Needs only be used with roles that are created in the same CDK application; for imported roles, supplying mutable=false when calling Role.fromRoleArn() is sufficient.

Fixes #2985.
Fixes #4465.
Closes #4501.

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

Commit Message

feat(iam): Role.withoutPolicyUpdates()

Add a new method to Role called withoutPolicyUpdates(), which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK.

Needs only be used with roles that are created in the same CDK application; for imported roles, supplying mutable=false when calling Role.fromRoleArn() is sufficient.

Fixes #2985.
Fixes #4465.
Closes #4501.

aws-cdk-automation · 2019-12-27T15:11:07Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-27T15:34:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-27T16:15:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85

Sweet. Small comments / suggestions on wording.

skinny85 · 2019-12-27T17:14:52Z

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

+   *
+   * @default false
+   */
+  readonly mustCreate?: boolean;


I'm not crazy about this name... How about lazy?: boolean?

skinny85 · 2019-12-27T17:22:36Z

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

+        result.push('Policy created with mustCreate=true is empty. You must add statements to the policy');
+      }
+      if (!this.mustCreate && this.referenceTaken) {
+        result.push('Policy name has been read of empty policy. You must add statements to the policy so it can exist.');


I think we can word this a little more clearly... how about: This Policy has been referenced by another resource, and so it must contain at least one statement.

skinny85 · 2019-12-27T17:23:39Z

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

+        result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`);
+      }
+      if (!this.mustCreate && this.referenceTaken) {
+        result.push('Policy name has been read of unattached policy. Attach to at least one principal: user, group or role.');


Same here: This Policy has been referenced by another resource, and so it must be attached to at least one principal: user, group or role.

eladb

I am not sure I fully understand the use case for this feature, and I couldn't dig those up from the referenced issues. #2985 is about immutable imported roles, #4465 is about maximum policy size limitations and I am not sure I fully understand how immutable roles help here.

Also, missing README, which might be a good place to provide a useful example that demonstrates when I'd want to use these immutable roles.

I also think that the name ImmutableRole is a bit misleading, because I'd expect an immutable "thing" to fail if I try to mutate it, and this is basically ignoring mutations. But perhaps this ship has sailed since we already have immutable: true when we import roles, so whateveeeee.

skinny85 · 2019-12-30T16:39:53Z

I am not sure I fully understand the use case for this feature, and I couldn't dig those up from the referenced issues. #2985 is about immutable imported roles, #4465 is about maximum policy size limitations and I am not sure I fully understand how immutable roles help here.

The maximum policy size is related to the immutable roles because immutability decreases the size of the statements added to the role when it's passed into constructs that add statements to it. An example from #4465:

           const myRole = new iam.Role(this, 'Role', { ... });

           // use myRole in a bunch of places...

           // then:
           new codepipeline_actions.CodeBuildAction({
                // ...
                role: myRole,
           });

Now, it might happen that passing myRole to CodeBuildAction will add so many statements that it will exceed the limit. In that case, the customer could do:

           new codepipeline_actions.CodeBuildAction({
                // ...
                role: new iam.ImmutableRole(myRole),
           });

And this way, all statement-adding expressions in CodeBuildAction would be effectively ignored, and the customer could work around the "maximum policy size exceeded" error they get.

Hope the motivation for this is a little more clear now!

"Changes requested" is not a reasonable way to say "I don't understand the motivation for this change"

eladb · 2019-12-31T12:57:59Z

README missing is "changes requested" and also I wanted to make sure the motivation for this, and the use case is clear before it's merged.

To be honest, the motivation @skinny85 provided above doesn't sounds like a "proper" use case. Maybe it's some kind of an escape hatch to allow people to work around bugs or issues we have in the system, but then I'd argue that a whole new type of Role is not the right API.

Let's discuss this before we introduce additional public APIs that don't have a clear use case.

I think trying to write a README entry for this will be a good exercise as it will require you to articulate a reasonable situation where someone would want to use this feature.

rix0rrr · 2020-01-02T10:05:48Z

I think we can chunk this PR smaller. There are some useful changes in there which are separate from the ImmutableRole class.

rix0rrr · 2020-01-02T11:28:00Z

Let me try this again as #5608. Although that doesn't fix #4465.

…role

aws-cdk-automation · 2020-01-07T11:01:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-01-07T13:46:48Z

Added a README explaining why you might want to use this class.

Pre-emptive FAQ:

Can we not do anything else if the policy size is growing too big?

We need to apply judicious use of wildcards then, and we decide not to do IAM policy manipulation because it's scary. Therefore, we have to punt to the user in some way. This is that way.

We could make it slightly easier for the user to get an ImmutableRole, maybe instead of

new codepipeline.Pipeline(this, 'Pipeline', {
  role: new iam.ImmutableRole(role),
});

they could do:

new codepipeline.Pipeline(this, 'Pipeline', {
  role: role.immutableCopy(),
});

But that's about the extent of it, and that's not appreciably different imo.

aws-cdk-automation · 2020-01-07T13:56:22Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-07T14:11:38Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-08T09:34:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb · 2020-01-09T12:00:26Z

Thanks for adding the README and the FAQ, they are very helpful.

I think this API is much better:

new codepipeline.Pipeline(this, 'Pipeline', {
  role: role.immutableCopy(),
});

As it doesn’t introduce a whole new type and is more discoverable. You just return an IRole this way!. It can also be applied both to owned and imported roles, so there won’t be a difference (maybe even deprecate “mutable: false”).

I am still not sure about the terminology of immutable. Perhaps something more around “role.withoutPolicyUpdates()” or something around “disable automatic policy” which is more in line with the README narrative.

rix0rrr · 2020-01-09T12:22:25Z

Mmyeah. I agree that a role.withoutPolicyUpdates() looks nicer at the usage site.

I don't buy adding it to the IRole:

For one, it makes the interface unnecessarily heavy to implement. In Java-like languages (which we are roughly emulating using jsii) there is no way to say "classes that implement IRole automatically also have these operations available". We're left taking them as an argument, either showing a visible wrapper class (new Immutable(role: IRole), or hiding the name of the wrapper class behind a factory function like public static makeImmutable(role: IRole): IRole, which is effectively the same thing).
For two, I believe that for imports our default of mutable: true is probably wrong, and should be mutable: false. If you're using existing roles, you're probably looking to use them as-is (99% certainly because they're provisioned to you by your Ops team and you don't have permissions to create new roles), otherwise why wouldn't you be creating new roles? So if we were to put this withoutPolicyUpdates() on the IRole and rely on people calling that for imports, almost all people doing imports would be writing this:

const role = Role.fromRoleArn(....).withoutPolicyUpdates();

Which is a little silly.

My proposal is:

Hide the ImmutableRole type and put it behind a method withoutPolicyUpdates() on the Role class.
In a different PR, switch the mutable default of fromRoleArn() to false.

eladb · 2020-01-09T13:30:55Z

@rix0rrr wrote:

Hide the ImmutableRole type and put it behind a method withoutPolicyUpdates() on the Role class.

Sounds good.

In a different PR, switch the mutable default of fromRoleArn() to false.

Yeah I agree that this makes more sense to be honest. And then I would deprecate the name immutable and rename this to use similar terminology (something like policyUpdates: true|false).

skinny85 · 2020-01-09T17:50:32Z

For two, I believe that for imports our default of mutable: true is probably wrong, and should be mutable: false. If you're using existing roles, you're probably looking to use them as-is (99% certainly because they're provisioned to you by your Ops team and you don't have permissions to create new roles), otherwise why wouldn't you be creating new roles? So if we were to put this withoutPolicyUpdates() on the IRole and rely on people calling that for imports, almost all people doing imports would be writing this:

Are you sure? This (mutable: false - it wasn't called that way, but that's what it effectively was) was the behavior we had originally, and it was changed (to be mutable: true by default). I'm pretty sure we had a reason to make that change. Also, I'm not sure reverting to the previous behavior now is backwards-compatible...

skinny85 · 2020-01-09T17:53:01Z

I personally think withoutPolicyUpdates() is just a roundabout way of saying "I want the role to be immutable", so I'd rather be direct than roundabout, but I don't feel super strongly about this.

rix0rrr · 2020-01-10T15:51:10Z

Are you sure? This (mutable: false - it wasn't called that way, but that's what it effectively was) was the behavior we had originally, and it was changed (to be mutable: true by default)

This is where we introduced the mutable keyword: #3716 and we went from a mutable imported Role to introducing the mutable keyword with default true.

We added support for adding policies to imported roles here: #2805. This is where I think we went wrong though.

Motivating issues for that:
#2651 - CodeBuild doesn't work with imported role: solved by not rendering unattached policy.
#2381 - Unable to use SQS event Source with imported role: actual root cause was using the wrong IAM action (but that only became visible when using imported roles)
(2 other issues are referenced that don't actually have anything to do with imported roles)

So it seems like the correct choice to me.

You're right the change is not backwards compatible but I think we'll do everyone a favor in the long run if we rip off the band-aid now. The flag is @experimental anyway.

…role

aws-cdk-automation · 2020-01-10T16:14:39Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-10T17:15:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85 · 2020-01-10T18:40:34Z

#2651 - CodeBuild doesn't work with imported role: solved by not rendering unattached policy.

I actually don't agree that this was the error in the general case, as #2651 was a way to workaround an actual bug in CodeBuild (missing DependsOn between Policy and Project resources). If you switch this over now, everyone who has a working setup with CodeBuild in a VPC with an imported role will most likely be broken (as we will remove the required permissions CodeBuild needs to connect to the VPC, which is validated at deploy time).

eladb

Update PR description

eladb · 2020-01-12T11:46:36Z

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

@@ -301,4 +301,34 @@ export = {

    test.done();
  },
+
+  'can use an ImmutableRole for a Project within a VPC'(test: Test) {


Suggested change

'can use an ImmutableRole for a Project within a VPC'(test: Test) {

'can use role.withoutPolicyUpdates() for a Project within a VPC'(test: Test) {

eladb · 2020-01-12T11:48:36Z

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

-      ? new MutableImport(scope, id)
-      : new ImmutableImport(scope, id);
+      ? new Import(scope, id)
+      : new ImmutableRole(new Import(scope, id));


should this be new Import().withoutPolicyUpdates()?

eladb · 2020-01-12T11:49:26Z

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

+   * If you do, you are responsible for adding the correct statements to the
+   * Role's policies yourself.
+   */
+  public withoutPolicyUpdates(): IRole {


Going back to our conversation, why can't this be in IRole and works both for Role and imported roles?

#5569 (comment)

The interface is already starting to get onerous to implement and I don't want to make it worse. There are 2 different parts to it: "what makes an IRole a role", and "operations that can be done to any IRole, which are the same for every role anyway". This is part of the 2nd, but I'd argue those shouldn't actually be on IRole at all.

Even if we put it on IRole, my gut says the 99% case will be immutable, hence following up the import by calling this method. We want to optimize for the common case, and adding more verbiage seems the opposite. Either we (ALSO?) add a withPolicyUpdates() to switch it back on, or we just leave it as import properties for the fromRoleArn().

So either we make IRole even heavier or it doesn't need to be there at all.

Sure. It’s not that critical.

claabs · 2020-01-23T20:06:42Z

It looks like this PR actually broke {mutable: false} for imported roles. Upgrading from 1.20.0 to 1.21.1 causes my imported roles to start taking on policies. I can create an issue if needed.

skinny85 · 2020-01-23T20:27:25Z

It looks like this PR actually broke {mutable: false} for imported roles. Upgrading from 1.20.0 to 1.21.1 causes my imported roles to start taking on policies. I can create an issue if needed.

Yes please!

claabs · 2020-01-23T22:35:02Z

Created issue: #5943

In the refactoring done in #5569, we introduced a bug. The `ImmutableRole` class correctly ignored policies directly added to it, but did not ignore policies added via `Grant.addToPrincipal()`. That's because its `IGrantable#grantPrincipal` field was being used as the principal to grant to, which was pointing to the wrapped role instead of the `ImmutableRole` itself. Fix this oversight and add a test to cement it in. Fixes #5943.

In the refactoring done in #5569, we introduced a bug. The `ImmutableRole` class correctly ignored policies directly added to it, but did not ignore policies added via `Grant.addToPrincipal()`. That's because its `IGrantable#grantPrincipal` field was being used as the principal to grant to, which was pointing to the wrapped role instead of the `ImmutableRole` itself. Fix this oversight and add a test to cement it in. Fixes #5943. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

rix0rrr added 3 commits December 27, 2019 15:38

Make it possible for Policy not to materialize

c571295

Add immutable role

e017fb6

Fix Project for use with immutable roles

f81c312

rix0rrr requested a review from skinny85 December 27, 2019 15:03

rix0rrr requested a review from eladb as a code owner December 27, 2019 15:03

rix0rrr self-assigned this Dec 27, 2019

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 27, 2019

attachInlinePolicy takes a 'Policy' not an 'IPolicy'

7349f2a

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Dec 27, 2019

Don't allow require() style import

6e0d1e3

skinny85 reviewed Dec 27, 2019

View reviewed changes

eladb previously requested changes Dec 30, 2019

View reviewed changes

skinny85 mentioned this pull request Jan 3, 2020

fix(codebuild): cannot use immutable roles for Project #5608

Merged

rix0rrr assigned skinny85 and unassigned rix0rrr Jan 7, 2020

Merge remote-tracking branch 'origin/master' into huijbers/immutable-…

fe49f05

…role

rix0rrr changed the title ~~feat(iam): new ImmutableRole class~~ feat(iam): ImmutableRole class Jan 7, 2020

rix0rrr changed the title ~~feat(iam): ImmutableRole class~~ feat(iam): ImmutableRole wrapper class Jan 7, 2020

Rewrote imported Role using ImmutableRole, add README

10c3eb0

rix0rrr requested a review from eladb January 7, 2020 13:47

Make ImmutableRole implement DependableTrait

1973409

Fix import ordering

334cb8c

rix0rrr added 2 commits January 10, 2020 16:55

Merge remote-tracking branch 'origin/master' into huijbers/immutable-…

39de5a1

…role

Hide 'ImmutableRole' behind factory function on 'Role'

717d052

rix0rrr changed the title ~~feat(iam): ImmutableRole wrapper class~~ feat(iam): Role.withoutPolicyUpdates() allows manual policy management Jan 10, 2020

rix0rrr changed the title ~~feat(iam): Role.withoutPolicyUpdates() allows manual policy management~~ feat(iam): Role.withoutPolicyUpdates() prevents automatic policies Jan 10, 2020

rix0rrr changed the title ~~feat(iam): Role.withoutPolicyUpdates() prevents automatic policies~~ feat(iam): Role.withoutPolicyUpdates() Jan 10, 2020

Also update test.project.ts

8ef7ab5

eladb approved these changes Jan 12, 2020

View reviewed changes

rix0rrr merged commit ea4ca3e into master Jan 15, 2020

rix0rrr deleted the huijbers/immutable-role branch January 15, 2020 09:44

rix0rrr mentioned this pull request Feb 4, 2020

fix(iam): policies added to immutably imported role #6090

Merged

brentryan mentioned this pull request May 13, 2021

feat(ecs-service-extensions): allow taskRole to be passed in on creation of an ECS service #13834

Merged

	'can use an ImmutableRole for a Project within a VPC'(test: Test) {
	'can use role.withoutPolicyUpdates() for a Project within a VPC'(test: Test) {

feat(iam): Role.withoutPolicyUpdates() #5569

feat(iam): Role.withoutPolicyUpdates() #5569

Conversation

rix0rrr commented Dec 27, 2019 • edited Loading

Commit Message

aws-cdk-automation commented Dec 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 27, 2019

AWS CodeBuild CI Report

skinny85 left a 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

eladb left a comment

Choose a reason for hiding this comment

skinny85 commented Dec 30, 2019

eladb commented Dec 31, 2019

rix0rrr commented Jan 2, 2020

rix0rrr commented Jan 2, 2020

aws-cdk-automation commented Jan 7, 2020

AWS CodeBuild CI Report

rix0rrr commented Jan 7, 2020

Can we not do anything else if the policy size is growing too big?

aws-cdk-automation commented Jan 7, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 7, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 8, 2020

AWS CodeBuild CI Report

eladb commented Jan 9, 2020

rix0rrr commented Jan 9, 2020

eladb commented Jan 9, 2020

skinny85 commented Jan 9, 2020

skinny85 commented Jan 9, 2020

rix0rrr commented Jan 10, 2020

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 10, 2020

AWS CodeBuild CI Report

skinny85 commented Jan 10, 2020

eladb left a 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

claabs commented Jan 23, 2020

skinny85 commented Jan 23, 2020

claabs commented Jan 23, 2020

feat(iam): `Role.withoutPolicyUpdates()` #5569

feat(iam): `Role.withoutPolicyUpdates()` #5569

rix0rrr commented Dec 27, 2019 •

edited

Loading