Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-iam): policy document optimization #14713

Closed
1 of 2 tasks
andreialecu opened this issue May 15, 2021 · 6 comments
Closed
1 of 2 tasks

(aws-iam): policy document optimization #14713

andreialecu opened this issue May 15, 2021 · 6 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2

Comments

@andreialecu
Copy link
Contributor

andreialecu commented May 15, 2021

I wonder if this is an acceptable feature request. If so, I could try submitting a PR myself.

aws-iam currently creates sub-optimal policies that could be pretty easily optimized. For example consider a helper method that runs this on an ECR repository:

    policy.addToPolicy(
      new iam.PolicyStatement({
        resources: [repository.repositoryArn],
        actions: [
          "ecr:InitiateLayerUpload",
          "ecr:UploadLayerPart",
          "ecr:CompleteLayerUpload",
          "ecr:BatchCheckLayerAvailability",
          "ecr:PutImage",
        ],
      })
    );

If policy is a "shared" policy, and the stack adds new ECR repositories over time, this construct will very quickly run into #11562.

The optimization would be that instead of creating duplicate statements with the same actions for every separate resource, it could just create one policy statement and merge all the resources together.

For example, it would be the equivalent of adding just one policy of:

    policy.addToPolicy(
      new iam.PolicyStatement({
        resources: [repository.repositoryArn, repository2.repositoryArn, ...etc],
        actions: [
          "ecr:InitiateLayerUpload",
          "ecr:UploadLayerPart",
          "ecr:CompleteLayerUpload",
          "ecr:BatchCheckLayerAvailability",
          "ecr:PutImage",
        ],
      })
    );

Note that I'm aware that new resources could be added to the PolicyStatement instead of adding new statements to the policy, but this results in a lot of extra complexity in the stack's implementation, and I feel that cdk could do an optimization here instead.

Additionally, there are places like:

return Grant.addToPrincipal({
grantee,
actions,
resourceArns: [this.roleArn],
scope: this,
});
which are outside of the user's control, which suffer from the same problem (this very quickly runs into #11562 (comment)) but which could be optimized by this step.

Use Case

The main issue is that the resulting CloudFormation template ends up huge, and runs over some internal limits as mentioned in #11562 (comment) and #11562 (comment)

Proposed Solution

My initial idea is to add a post-processor similar to the one in:

context.registerPostProcessor(new RemoveDuplicateStatements(this.autoAssignSids));

It would detect if the generated policy can be optimized by merging similar resources or actions together.

Other

I think this shouldn't be a breaking change if implemented correctly. Also I'm not sure if it requires any changes, or if it affects the IAM Statement Changes interactive prompt.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@andreialecu andreialecu added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 15, 2021
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label May 15, 2021
@andreialecu andreialecu changed the title (aws-iam): policy statement optimization (aws-iam): policy document optimization May 15, 2021
andreialecu added a commit to andreialecu/aws-cdk that referenced this issue May 15, 2021
@andreialecu
Copy link
Contributor Author

I implemented this in #14714

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 4, 2021

Leaving this open for future upvotes which may increase the weight, but we consciously decided not to do anything about this.

IAM policy optimization is attractive but also dangerous, and we're super scared of messing up.

If you want to implement this for now, I would suggest you implement an Aspect to twiddle the construct tree out-of-band.

@rix0rrr rix0rrr added effort/large Large work item – several weeks of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 4, 2021
@rix0rrr rix0rrr removed their assignment Jun 4, 2021
@andreialecu
Copy link
Contributor Author

@rix0rrr wondering if you saw my comment just above you? The PR received no comments.

@bilalq
Copy link

bilalq commented Dec 7, 2021

The absence of this results in AWS CodePipelines being unusable for cross-region deployments in many cases. See #16244 for more details.

@comcalvi
Copy link
Contributor

I think this has been resolved by #19114

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants