fix(iam): Role policies cannot grow beyond 10k #20400
Conversation
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.
|
|
github-actions
bot
added
bug
rix0rrr
added
effort/large
There was a problem hiding this comment.
Nice work on this @rix0rrr! This PR is enormous, so this is a first pass, mainly style comments. My only concern with this PR is the estimation of ARN lengths. The estimates are probably safe but in the event a user does something we didn't forsee, they should have a way of specifying the ARN length.
| let ret = 0; | ||
|
|
||
| const actionEstimate = 20; | ||
| const arnEstimate = 120; // A safe (over)estimate on how long ARNs typically are |
There was a problem hiding this comment.
Per this doc: https://docs.aws.amazon.com/IAM/latest/APIReference/API_Policy.html
ARNs can be up to 2048 characters in length. The only way the estimate would be (dangerously) inaccurate is if the user imports an ARN, in which case they likely know the length of it themselves. Can we add an optional argument to ARN import methods, arnLength?: number, which we could use here? This way if users run into a deploy time issue they can specify the ARN length and _estimateSize() can correct for that here.
packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-document.ts
Outdated
Show resolved
Hide resolved
| let ret = 0; | ||
|
|
||
| const actionEstimate = 20; | ||
| const arnEstimate = 120; // A safe (over)estimate on how long ARNs typically are |
There was a problem hiding this comment.
https://docs.aws.amazon.com/IAM/latest/APIReference/API_Role.html mentions 2048 as the upper boundary.
Does the "typically" here imply computations based on average?
|
The build fails because the test success depends on #20396 being merged first. |
| * Because we often can't know the length of an ARN (it may be a token and only | ||
| * available at deployment time) we'll have to estimate it. | ||
| * | ||
| * The estimate can be overridden by setting the `@aws-cdk/aws-iam.arnSizeEstimate` context key. |
There was a problem hiding this comment.
I like the option of a value that can be set.
| if (!this.defaultPolicy || this._didSplit) { | ||
| return; | ||
| } | ||
| this._didSplit = true; |
There was a problem hiding this comment.
Am I understanding correctly that this is a marker for an (attempted) split? The true holds whether or not the split actually happens.
|
|
||
| // Validate sizes | ||
| // | ||
| // Not entirely accurate, because our "Ref"s and "Fn::GetAtt"s actually need to be evaluated |
There was a problem hiding this comment.
How likely is that with enough time (code changes) this test will start failing?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
(This change has been split off from #20189 because that PR was growing
too big)
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
IComparablePrincipalto be able to value-comparetwo 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).
PolicyStatement objects, which requires being able to compare
Principal objects.
Closes #19276, closes #19939, closes #19835.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license