feat(iam): avoid duplicate statements in policy documents #2254

jogold · 2019-04-12T07:56:57Z

Avoid duplicate statements in IAM policy documents by comparing statements with JSON.stringify.

Only identical duplicate statements are removed.

Fixes #1777
Fixes #2168

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- 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.

Fixes aws#1777 Fixes aws#2168

jogold · 2019-04-12T08:24:41Z

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

+  /**
+   * The hash of this statement. Used to avoid duplicates.
+   */
+  public get hash(): string {


I like the idea of having a hash for a statement, but this is actually not necessary for comparison purposes (JSON.stringify(this.toJson()) is sufficient). Opinions?

This is IAM, which is sensitive, so I am definitely not comfortable with the idea of making a hash-comparison, because in the improbable cause of a collision, it'll have awkward consequences.

I am however not sure that JSON.stringify will work though, as there can be any amount of unresolved Tokens here that may or may not be represented by the same string.

I am however not sure that JSON.stringify will work though, as there can be any amount of unresolved Tokens here that may or may not be represented by the same string.

If this is the case then the comparison will fail and it will leave the statements as is, which is suboptimal but safe?

IIRC, toJson will throw in this case...

RomainMuller

I'm not super favorable to this. We have attempted policy size optimization in the past (it all started with getting rid of duplicated statements, and it escalated quickly) and introduced horrible permission-widening bugs in the process. I don't want that again.

I personally would prefer devising a strategy that prevents inserting a duplicate statement in the policy document, rather than pruning them after the fact. This way, if I can assert that the policy generated by some construct is acceptable to me, I know none of those statements will be removed by the framework at a later point...

RomainMuller · 2019-04-12T08:30:50Z

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

+  /**
+   * The hash of this statement. Used to avoid duplicates.
+   */
+  public get hash(): string {


This is IAM, which is sensitive, so I am definitely not comfortable with the idea of making a hash-comparison, because in the improbable cause of a collision, it'll have awkward consequences.

I am however not sure that JSON.stringify will work though, as there can be any amount of unresolved Tokens here that may or may not be represented by the same string.

jogold · 2019-04-12T10:23:02Z

@RomainMuller how about this 0baa26a? It's indeed suboptimal but on the safe side and still fixes #1777.

rix0rrr · 2019-04-12T11:50:43Z

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

  public addStatement(statement: PolicyStatement): PolicyDocument {
-    this.statements.push(statement);


Instead of doing this, implement IResolvedValuePostProcessor and do it in the postProcess method.

This is guaranteed to receive the post-resolution Statement, and you can do safe pairwise comparisons based on JSON.stringify().

RomainMuller · 2019-04-15T07:18:43Z

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

-    doc.Statement = doc.Statement.concat(this.statements);
+    const doc = {
+      ...this.baseDocument,
+      Statement: (this.baseDocument.Statement || []).concat(this.statements),


This concatenation happens at construction time - when this.statements is guaranteed to be empty. You need to wrap this in a Token so it is done at synthesis time.

It happens in resolve() not constructor(), it was like that before.

https://github.com/awslabs/aws-cdk/blob/b61707896582373c4526129945ddaac20055f313/packages/%40aws-cdk/aws-iam/lib/policy-document.ts#L23-L27

RomainMuller · 2019-04-15T07:20:05Z

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

+
+    for (const statement of input.Statement) {
+      const jsonStatement = JSON.stringify(statement);
+      if (!jsonStatements.includes(jsonStatement)) {


Make jsonStatements a new Set<string>(), so you get faster existence check with has.

I think it's more a matter of preferences here, what you gain in has you lose in add, no? Will adapt.

I think the O(N^2) check is fine, N will be small. But I'm not opposed to using sets either. The difference will probably be a wash.

eladb

looks nice and clean

eladb · 2019-04-15T10:03:59Z

packages/@aws-cdk/aws-lambda/lib/log-retention.ts

-          .addAllResources()
-      );
-    }
+    provider.addToRolePolicy(


add a note here that duplicate statements will be deduplicated by PolicyDocument

…ws-lambda-python-alpha/test/lambda-handler-poetry (#27381) Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.0.3 to 2.0.6. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/releases">urllib3's releases</a>.</em></p> <blockquote> <h2>2.0.6</h2> <ul> <li>Added the <code>Cookie</code> header to the list of headers to strip from requests when redirecting to a different host. As before, different headers can be set via <code>Retry.remove_headers_on_redirect</code>. (GHSA-v845-jxx5-vc9f)</li> </ul> <h2>2.0.5</h2> <ul> <li>Allowed pyOpenSSL third-party module without any deprecation warning. <a href="https://github.com/urllib3/urllib3/issues/3126">#3126</a></li> <li>Fixed default <code>blocksize</code> of <code>HTTPConnection</code> classes to match high-level classes. Previously was 8KiB, now 16KiB. <a href="https://github.com/urllib3/urllib3/issues/3066%3E">#3066</a></li> </ul> <h2>2.0.4</h2> <ul> <li>Added support for union operators to <code>HTTPHeaderDict</code> (<a href="https://github.com/urllib3/urllib3/issues/2254">#2254</a>)</li> <li>Added <code>BaseHTTPResponse</code> to <code>urllib3.__all__</code> (<a href="https://github.com/urllib3/urllib3/issues/3078">#3078</a>)</li> <li>Fixed <code>urllib3.connection.HTTPConnection</code> to raise the <code>http.client.connect</code> audit event to have the same behavior as the standard library HTTP client (<a href="https://github.com/urllib3/urllib3/issues/2757">#2757</a>)</li> <li>Relied on the standard library for checking hostnames in supported PyPy releases (<a href="https://github.com/urllib3/urllib3/issues/3087">#3087</a>)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/blob/main/CHANGES.rst">urllib3's changelog</a>.</em></p> <blockquote> <h1>2.0.6 (2023-10-02)</h1> <ul> <li>Added the <code>Cookie</code> header to the list of headers to strip from requests when redirecting to a different host. As before, different headers can be set via <code>Retry.remove_headers_on_redirect</code>.</li> </ul> <h1>2.0.5 (2023-09-20)</h1> <ul> <li>Allowed pyOpenSSL third-party module without any deprecation warning. (<code>[#3126](urllib3/urllib3#3126) <https://github.com/urllib3/urllib3/issues/3126></code>__)</li> <li>Fixed default <code>blocksize</code> of <code>HTTPConnection</code> classes to match high-level classes. Previously was 8KiB, now 16KiB. (<code>[#3066](urllib3/urllib3#3066) <https://github.com/urllib3/urllib3/issues/3066></code>__)</li> </ul> <h1>2.0.4 (2023-07-19)</h1> <ul> <li>Added support for union operators to <code>HTTPHeaderDict</code> (<code>[#2254](urllib3/urllib3#2254) <https://github.com/urllib3/urllib3/issues/2254></code>__)</li> <li>Added <code>BaseHTTPResponse</code> to <code>urllib3.__all__</code> (<code>[#3078](urllib3/urllib3#3078) <https://github.com/urllib3/urllib3/issues/3078></code>__)</li> <li>Fixed <code>urllib3.connection.HTTPConnection</code> to raise the <code>http.client.connect</code> audit event to have the same behavior as the standard library HTTP client (<code>[#2757](urllib3/urllib3#2757) <https://github.com/urllib3/urllib3/issues/2757></code>__)</li> <li>Relied on the standard library for checking hostnames in supported PyPy releases (<code>[#3087](urllib3/urllib3#3087) <https://github.com/urllib3/urllib3/issues/3087></code>__)</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/urllib3/urllib3/commit/262e3e332209ee93ff70e2b13502c8f20c105ac8"><code>262e3e3</code></a> Release 2.0.6</li> <li><a href="https://github.com/urllib3/urllib3/commit/644124ecd0b6e417c527191f866daa05a5a2056d"><code>644124e</code></a> Merge pull request from GHSA-v845-jxx5-vc9f</li> <li><a href="https://github.com/urllib3/urllib3/commit/740380c59ca2a7c2dceca19e5dba99f6b7060e62"><code>740380c</code></a> Bump cryptography from 41.0.3 to 41.0.4 (<a href="https://github.com/urllib3/urllib3/issues/3131">#3131</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/d9f85a749488188c286cd50606d159874db94d5f"><code>d9f85a7</code></a> Release 2.0.5</li> <li><a href="https://github.com/urllib3/urllib3/commit/d41f4122966f7f4f5f92001ad518e5d9dafcc886"><code>d41f412</code></a> Undeprecate pyOpenSSL module (<a href="https://github.com/urllib3/urllib3/issues/3127">#3127</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/b6c04cb3e62ef5a0e4947d037c12fb3ca79e024a"><code>b6c04cb</code></a> Fix a link to "absolute URI" definition (<a href="https://github.com/urllib3/urllib3/issues/3128">#3128</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/af7c78fa30f5a4e265911371d0c59b6baeddca0f"><code>af7c78f</code></a> refactor: change double conditional to one (<a href="https://github.com/urllib3/urllib3/issues/3118">#3118</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/34c13c8e68df6f89890ba08b9fc4fbf87ed21669"><code>34c13c8</code></a> Refer to current internet standards in docs on proxies (<a href="https://github.com/urllib3/urllib3/issues/3124">#3124</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/a3e94f218cd8297db73302eadae235f0c832a809"><code>a3e94f2</code></a> Fix a name of an attribute in docs (<a href="https://github.com/urllib3/urllib3/issues/3125">#3125</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/da69d4f4f95bc7ef9307fc8e0499c2121f1e4791"><code>da69d4f</code></a> Fix docs build (<a href="https://github.com/urllib3/urllib3/issues/3123">#3123</a>)</li> <li>Additional commits viewable in <a href="https://github.com/urllib3/urllib3/compare/2.0.3...2.0.6">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=urllib3&package-manager=pip&previous-version=2.0.3&new-version=2.0.6)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/aws/aws-cdk/network/alerts). </details>

jogold added 3 commits April 11, 2019 22:46

feat(iam): remove duplicate statements from policy documents

4c24fb3

Fixes aws#1777 Fixes aws#2168

remove duplicate check in log retention

dec38e4

fix codepipeline-actions integ test

fb71270

jogold requested review from RomainMuller, skinny85 and a team as code owners April 12, 2019 07:56

jogold commented Apr 12, 2019

View reviewed changes

RomainMuller reviewed Apr 12, 2019

View reviewed changes

prevent adding duplicate statement

0baa26a

rix0rrr reviewed Apr 12, 2019

View reviewed changes

jogold added 3 commits April 12, 2019 14:51

implement IResolvedValuePostProcessor

8a0c36c

jsdoc

185edd0

add unit test with base document

2afd5d3

RomainMuller reviewed Apr 15, 2019

View reviewed changes

use a set for jsonStatements

94028c1

eladb approved these changes Apr 15, 2019

View reviewed changes

add note

c421907

rix0rrr merged commit a89758f into aws:master Apr 16, 2019

jogold deleted the iam-duplicates branch June 13, 2019 13:20

njlynch mentioned this pull request Sep 3, 2021

fix(codepipeline): large cross-region pipelines exceed IAM policy size limit #16350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iam): avoid duplicate statements in policy documents #2254

feat(iam): avoid duplicate statements in policy documents #2254

jogold commented Apr 12, 2019 •

edited

Loading

jogold Apr 12, 2019

RomainMuller Apr 12, 2019

jogold Apr 12, 2019

RomainMuller Apr 15, 2019

RomainMuller left a comment

RomainMuller Apr 12, 2019

jogold commented Apr 12, 2019

rix0rrr Apr 12, 2019

RomainMuller Apr 15, 2019

jogold Apr 15, 2019 •

edited

Loading

RomainMuller Apr 15, 2019

jogold Apr 15, 2019 •

edited

Loading

rix0rrr Apr 15, 2019

eladb left a comment

eladb Apr 15, 2019

		public addStatement(statement: PolicyStatement): PolicyDocument {
		this.statements.push(statement);

feat(iam): avoid duplicate statements in policy documents #2254

feat(iam): avoid duplicate statements in policy documents #2254

Conversation

jogold commented Apr 12, 2019 • edited Loading

Pull Request Checklist

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

RomainMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Apr 15, 2019 • edited Loading

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

Choose a reason for hiding this comment

jogold commented Apr 12, 2019 •

edited

Loading

jogold Apr 15, 2019 •

edited

Loading

jogold Apr 15, 2019 •

edited

Loading