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

fix(iam): IAM Policies are too large to deploy #19114

Merged
merged 37 commits into from Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
f8208e9
fix(iam): IAM Policies are too large to deploy
rix0rrr Feb 23, 2022
777c24d
Add unit tests
rix0rrr Feb 24, 2022
e9b6cdc
Only merge objects for principals
rix0rrr Feb 25, 2022
68d3940
Add test for condition
rix0rrr Feb 25, 2022
243f9c0
Update snapshots
rix0rrr Feb 25, 2022
1f40baa
Also merge Deny statements, add Alloy model
rix0rrr Feb 26, 2022
ef260c2
Revert "Update snapshots"
rix0rrr Feb 26, 2022
8b896d5
Add reference to Alloy model in source
rix0rrr Feb 26, 2022
281df8b
Correct docstring
rix0rrr Feb 26, 2022
a00582a
Don't unnecessarily update expectations
rix0rrr Feb 26, 2022
aeb902f
Update packages/@aws-cdk/aws-iam/lib/policy-document.ts
rix0rrr Feb 26, 2022
adf50cb
Add review comments
rix0rrr Feb 26, 2022
ccb192b
Redid merging logic
rix0rrr Feb 27, 2022
79f17d1
Update Alloy comment
rix0rrr Feb 27, 2022
09cdce1
Not subsets, equal sets!
rix0rrr Feb 27, 2022
02274c3
Update snapshots
rix0rrr Mar 2, 2022
ef354b4
Update model some more
rix0rrr Mar 4, 2022
00d0266
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 4, 2022
cc54755
Review comments
rix0rrr Mar 8, 2022
e4c3a2a
Update packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-docum…
rix0rrr Mar 9, 2022
8fbee24
Remove unused helpers
rix0rrr Mar 9, 2022
625b2a7
Merge remote-tracking branch 'origin/master' into huijbers/minimize-p…
rix0rrr Mar 9, 2022
8875096
Update packages/@aws-cdk/aws-iam/lib/private/postprocess-policy-docum…
rix0rrr Mar 9, 2022
3e570f0
Do better comparisons
rix0rrr Mar 10, 2022
fe4be84
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 10, 2022
812d4fb
Some implementation details
rix0rrr Mar 10, 2022
fac4952
Update snapshots
rix0rrr Mar 10, 2022
bdb113f
More snapshot updates
rix0rrr Mar 10, 2022
1f0b3c4
Merge branch 'master' into huijbers/minimize-policies
rix0rrr Mar 10, 2022
4988a90
Silly copy/paste bug 😓
rix0rrr Mar 14, 2022
2d2f16b
Reformat, find another weakening
rix0rrr Mar 14, 2022
e1f701c
Merge remote-tracking branch 'origin/master' into huijbers/minimize-p…
rix0rrr Mar 14, 2022
8eb3cfa
Also handle untyped principals, review comments
rix0rrr Mar 17, 2022
80d7a07
Merge branch 'huijbers/minimize-policies' of github.com:aws/aws-cdk i…
rix0rrr Mar 17, 2022
793da77
Merge branch 'master' into huijbers/minimize-policies
rix0rrr Mar 17, 2022
4fb02ef
Update new integ
rix0rrr Mar 18, 2022
37eaa56
Merge branch 'master' into huijbers/minimize-policies
mergify[bot] Mar 18, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/@aws-cdk/aws-iam/.npmignore
Expand Up @@ -21,8 +21,10 @@ tsconfig.json
.eslintrc.js
jest.config.js

docs/

# exclude cdk artifacts
**/cdk.out
junit.xml
test/
!*.lit.ts
!*.lit.ts
116 changes: 116 additions & 0 deletions packages/@aws-cdk/aws-iam/docs/policy-merging.als
@@ -0,0 +1,116 @@
/*
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
Alloy model to confirm the logic behind merging IAM Statements.

This proves that merging two statements based on the following conditions:

- Effects are the same, and one of:
- Both have some Actions and the rest of the statements is the same
- Both have some Resources and the rest of the statements is the same
- Both have some Principals and the rest of the statements is the same

Is sound, as the model doesn't find any examples of where the meaning
of statements is changed by merging.

Find Alloy at https://alloytools.org/.
*/

---------------------------------------------------------
-- Base Statement definitions
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
enum Effect { Allow, Deny }
enum Resource { ResourceA, ResourceB }
enum Action { ActionA, ActionB }
enum Principal { PrincipalA, PrincipalB }
Comment on lines +21 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to add three kinds of each since there are some checks at the end of the file with 3+ distinct statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes a difference. Wouldn't 2 of each be enough to represent "different from the other one"? We don't need 3 to show that.


sig Statement {
effect: Effect,
principal: set Principal,
notPrincipal: set Principal,
action: set Action,
notAction: set Action,
resource: set Resource,
notResource: set Resource,
} {
// Exactly one of Xxx and notXxx is non-empty
(some principal) iff not (some notPrincipal)
(some action) iff not (some notAction)
(some resource) iff not (some notResource)
}

---------------------------------------------------------
-- Requests and evaluations
sig Request {
principal: Principal,
resource: Resource,
action: Action,
}

// Whether the statement applies to the given request
pred applies[s: Statement, req: Request] {
req.principal in s.principal or req.principal not in s.notPrincipal
req.action in s.action or req.action not in s.notAction
req.resource in s.resource or req.resource not in s.notResource
}

// Whether or not to allow the given request according to the given statements
//
// A request is allowed if there's at least one statement allowing it and
// no statements denying it.
pred allow[req: Request, ss: some Statement] {
some s: ss | applies[s, req] and s.effect = Allow
no s: ss | applies[s, req] and s.effect = Deny
}

---------------------------------------------------------
-- Statement merging

// Helpers to assert that certain fields are the same
let sameAction[a, b] = { a.action = b.action and a.notAction = b.notAction }
let sameResource[a, b] = { a.resource = b.resource and a.notResource = b.notResource }
let samePrincipal[a, b] = { a.principal = b.principal and a.notPrincipal = b.notPrincipal }

// Assert that m is the merged version of a and b
//
// This encodes the important logic: the rules of merging.
pred merged[a: Statement, b: Statement, m: Statement] {
m.effect = a.effect and m.effect = b.effect
{
// Writing 'some a.action', 'some b.action' here is critical
// This asserts we are dealing with a POSITIVE action and not
// a notAction (that's excluded by the fact on Statement that only
// one of the two can be set.
//
// We can show that we have a bug if you uncomment the next line.
some a.action and some b.action // Excludes notAction

// The constraint on notAction here is not necessary, because
// in practice it must always the empty set, but it is necessary to
// correctly show the bug in action if the previous line is commented out.
m.action = a.action + b.action and m.notAction = a.notAction + b.notAction
sameResource[a, b] and sameResource[b, m]
samePrincipal[a, b] and samePrincipal[b, m]
} or {
some a.resource and some b.resource // Excludes notResource
m.resource = a.resource + b.resource and m.notResource = a.notResource + b.notResource
sameAction[a, b] and sameAction[b, m]
samePrincipal[a, b] and samePrincipal[b, m]
} or {
some a.principal and some b.principal // Excludes notPrincipal
m.principal = a.principal + b.principal and m.notPrincipal = a.notPrincipal + b.notPrincipal
sameAction[a, b] and sameAction[b, m]
sameResource[a, b] and sameResource[b, m]
}
}

// Maximum merging of all statements in orig
pred maxMerged[orig: some Statement, merged: some Statement] {
}

// There are no statements so that if they are merged,
// the evaluation of the separate statements is different than the evaluation
// of the merged statement
check merging_does_not_change_evaluation {
no a: Statement, b: Statement, m: Statement, r: Request {
merged[a, b, m]
allow[r, a + b] iff not allow[r, m]
}
} for 5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is very cool 🙂 What's assuring that the algorithm described here is the same as the one in the TS code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that always the problem with modeling languages?

Careful review, I suppose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll be happy to explain this in detail btw

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe good topic for a team time even?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could ensure they are side-by-side able... Like if we could annotate the TypeScript with the Alloy model pieces, and then have a tool that extracts the alloy model from the code.

This way would reduce the likelihood that one side is changed without the other side being changed to match; and would hopefully make it easier to validate that the code is expressing the same thing as the model.

Now - this is obviously work for a different PR, that is already more than enough things to look at in this one...

66 changes: 26 additions & 40 deletions packages/@aws-cdk/aws-iam/lib/policy-document.ts
@@ -1,5 +1,7 @@
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { PolicyStatement } from './policy-statement';
import { PostProcessPolicyDocument } from './private/postprocess-policy-document';

/**
* Properties for a new PolicyDocument
Expand All @@ -18,6 +20,24 @@ export interface PolicyDocumentProps {
* @default - No statements
*/
readonly statements?: PolicyStatement[];

/**
* Try to minimize the policy by merging statements
*
* To avoid overrunning the maximum policy size, combine statements if they produce
* the same result. Merging happens according to the following rules:
*
* - The Effect of both statements is the same
* - Neither of the statements have a 'Sid'
* - Combine Principals if the rest of the statement is exactly the same.
* - Combine Resources if the rest of the statement is exactly the same.
* - Combine Actions if the rest of the statement is exactly the same.
* - We will never combine NotPrincipals, NotResources or NotActions, because doing
* so would change the meaning of the policy document.
*
* @default - false, unless the feature flag `@aws-cdk/aws-iam:minimizePolicies` is set
*/
readonly minimize?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this property? Is the feature flag not enough?

I would get rid of it for now. We can always come back later, and add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better on principle. The boolean toggles the feature, and the default value for the boolean is taken from the flag for cases where you don't control the Policy initialization.

}

/**
Expand All @@ -43,16 +63,21 @@ export class PolicyDocument implements cdk.IResolvable {
public readonly creationStack: string[];
private readonly statements = new Array<PolicyStatement>();
private readonly autoAssignSids: boolean;
private readonly minimize?: boolean;

constructor(props: PolicyDocumentProps = {}) {
this.creationStack = cdk.captureStackTrace();
this.autoAssignSids = !!props.assignSids;
this.minimize = props.minimize;

this.addStatements(...props.statements || []);
}

public resolve(context: cdk.IResolveContext): any {
context.registerPostProcessor(new RemoveDuplicateStatements(this.autoAssignSids));
context.registerPostProcessor(new PostProcessPolicyDocument(
this.autoAssignSids,
this.minimize ?? cdk.FeatureFlags.of(context.scope).isEnabled(cxapi.IAM_MINIMIZE_POLICIES) ?? false,
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
));
return this.render();
}

Expand Down Expand Up @@ -153,42 +178,3 @@ export class PolicyDocument implements cdk.IResolvable {
return doc;
}
}

/**
* Removes duplicate statements and assign Sids if necessary
*/
class RemoveDuplicateStatements implements cdk.IPostProcessor {
constructor(private readonly autoAssignSids: boolean) {
}

public postProcess(input: any, _context: cdk.IResolveContext): any {
if (!input || !input.Statement) {
return input;
}

const jsonStatements = new Set<string>();
const uniqueStatements: any[] = [];

for (const statement of input.Statement) {
const jsonStatement = JSON.stringify(statement);
if (!jsonStatements.has(jsonStatement)) {
uniqueStatements.push(statement);
jsonStatements.add(jsonStatement);
}
}

// assign unique SIDs (the statement index) if `autoAssignSids` is enabled
const statements = uniqueStatements.map((s, i) => {
if (this.autoAssignSids && !s.Sid) {
s.Sid = i.toString();
}

return s;
});

return {
...input,
Statement: statements,
};
}
}