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

Tracking: Policy-generation creates oversized templates #19276

Closed
NGL321 opened this issue Mar 8, 2022 · 4 comments · Fixed by #19114 or #20400
Closed

Tracking: Policy-generation creates oversized templates #19276

NGL321 opened this issue Mar 8, 2022 · 4 comments · Fixed by #19114 or #20400
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management management/tracking Issues that track a subject or multiple issues p1

Comments

@NGL321
Copy link
Contributor

NGL321 commented Mar 8, 2022

Overview

A number of customers in separate issues (as well as internal tickets) have reported problems with automatic policy generation executed by the CDK for various services. The most recurrent and problematic of those are the oversized policies generated that exceed maximum size.

This issue is to aggregate and track those separate issues and resolve once system has been revamped/revised.

Link to the service’s CDK Construct Library API reference page.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam-readme.html

Maturity: CloudFormation Resources Only

Stable

Implementation

PRs

Issue list

Reports

Potential Workarounds/Suggestions

@NGL321 NGL321 added management/tracking Issues that track a subject or multiple issues p1 @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Mar 8, 2022
@mergify mergify bot closed this as completed in #19114 Mar 18, 2022
mergify bot pushed a commit that referenced this issue Mar 18, 2022
The policies we generate sometimes have a lot of duplication between
statements. This duplication can lead to the policy going over the size
limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource
type).

This change combines multiple statements together, as long as it
doesn't change the meaning of the final policy.

Because doing so for all existing stacks will probably provoke minor
heart attacks in operators everywhere, the new behavior is gated
behind a feature flag. It can be retroactively switched on by
people currently being bit by the size issues:

```
@aws-cdk/aws-iam:minimizePolicies
```

We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their `Action`, `Resource` or `Principal` declarations. We will not merge `NotXxx` statements, because doing so will change the meaning of the statement (`not A or not B ≠ not (A or B)`). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like `*`-subsumption. This is a starting point that should help out in the common case.

Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@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.

iliapolo pushed a commit that referenced this issue Mar 20, 2022
The policies we generate sometimes have a lot of duplication between
statements. This duplication can lead to the policy going over the size
limit an IAM policy (either 2k, 6k or 10k bytes, depending on the resource
type).

This change combines multiple statements together, as long as it
doesn't change the meaning of the final policy.

Because doing so for all existing stacks will probably provoke minor
heart attacks in operators everywhere, the new behavior is gated
behind a feature flag. It can be retroactively switched on by
people currently being bit by the size issues:

```
@aws-cdk/aws-iam:minimizePolicies
```

We will merge 2 statements if their effects are the same, and they are otherwise exactly the same apart from their `Action`, `Resource` or `Principal` declarations. We will not merge `NotXxx` statements, because doing so will change the meaning of the statement (`not A or not B ≠ not (A or B)`). There may be multiple possible merges that apply and we are not guaranteed to find the smallest merging, nor do we take effort to find all possible merges and do simplifications like `*`-subsumption. This is a starting point that should help out in the common case.

Fixes #18774, fixes #16350, fixes #18457, fixes #18564, fixes #19276.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@peterwoodworth
Copy link
Contributor

Reopening due to a number of customers still experiencing this issue

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 20, 2022

SPecifically for CDK Pipelines:

#19939
#19835

rix0rrr added a commit that referenced this issue May 3, 2022
Two changes:

- Collapse CodeBuild action Roles: each CodeBuild step used to create a
  fresh Role to run the CodeBuild action. Change to use one Role for all
  CodeBuild actions. This saves a lot of resources and policy space when
  using a lot of CodeBuild steps, and doesn't appreciably change the
  security posture of the Pipeline (note: this is *not* about the
  Execution Role of the CodeBuild projects, this is about the Role
  assumed by the Pipeline to initiate execution of the Project).
- If inline policies grow bigger than 10k, split additional statements
  off into ManagedPolicies.

Since we want to do the splitting post-merging (to get the most bang for
our buck), we now need to do statement merging during the `prepare`
phase (that is, pre-rendering, instead of post-rendering). That means it
had to be modified to work on `PolicyStatement` objects, instead of on
raw IAM JSON documents.

Closes #19276, closes #19939, closes #19835.
rix0rrr added a commit that referenced this issue May 3, 2022
Two changes:

- Collapse CodeBuild action Roles: each CodeBuild step used to create a
  fresh Role to run the CodeBuild action. Change to use one Role for all
  CodeBuild actions. This saves a lot of resources and policy space when
  using a lot of CodeBuild steps, and doesn't appreciably change the
  security posture of the Pipeline (note: this is *not* about the
  Execution Role of the CodeBuild projects, this is about the Role
  assumed by the Pipeline to initiate execution of the Project).
- If inline policies grow bigger than 10k, split additional statements
  off into ManagedPolicies.

Since we want to do the splitting post-merging (to get the most bang for
our buck), we now need to do statement merging during the `prepare`
phase (that is, pre-rendering, instead of post-rendering). That means it
had to be modified to work on `PolicyStatement` objects, instead of on
raw IAM JSON documents.

Closes #19276, closes #19939, closes #19835.
rix0rrr added a commit that referenced this issue May 18, 2022
(This change has been split off from #20189 because that PR was growing
too big)

Collapse CodeBuild action Roles: each CodeBuild step used to create a
fresh Role to run the CodeBuild action. Change to use one Role for all
CodeBuild actions. This saves a lot of resources and policy space when
using many CodeBuild steps, and doesn't appreciably change the
security posture of the Pipeline (note: this is not about the
Execution Role of the CodeBuild projects, this is about the Role
assumed by the Pipeline to initiate execution of the Project).

Relates to #19276, #19939, #19835.
rix0rrr added a commit that referenced this issue May 18, 2022
We add all Role policy statements to the Inline policy, which
has a maximums size of 10k. Especially when creating CDK Pipelines
that deploy to a lot of environments, the list of Role ARNs
the Pipeline should be allowed to assume exceeds this size.

Roles also have the ability to have Managed Policies attached
(10 by default, 20 with a quota increase), each of them can be 6k
in size. By spilling over from inline policies into Managed Policies
we can get a total of 70k of statements attached to reach Role.

This PR introduces `IComparablePrincipal` to be able to value-compare
two principals: since we want to merge first before we split (to get the
most bang for our buck), we now need to do statement merging during the
prepare phase, while we are still working on the object graph (instead
of the rendered CloudFormation template).
* That means statement merging had to be modified to work on
  PolicyStatement objects, which requires being able to compare
  Principal objects.

Closes #19276, closes #19939, closes #19835.
mergify bot pushed a commit that referenced this issue May 24, 2022
(This change has been split off from #20189 because that PR was growing
too big)

Collapse CodeBuild action Roles: each CodeBuild step used to create a
fresh Role to run the CodeBuild action. Change to use one Role for all
CodeBuild actions. This saves a lot of resources and policy space when
using many CodeBuild steps, and doesn't appreciably change the
security posture of the Pipeline (note: this is not about the
Execution Role of the CodeBuild projects, this is about the Role
assumed by the Pipeline to initiate execution of the Project).

Relates to #19276, #19939, #19835.


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot closed this as completed in #20400 May 25, 2022
@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 management/tracking Issues that track a subject or multiple issues p1
Projects
None yet
3 participants