feat(codepipeline/cfn): Use fewer statements for pipeline permissions #1009

RomainMuller · 2018-10-25T08:34:02Z

When trying to make minimal-permission IAM policies, it can be necessary
to ensure the policy remains as compact as possible. In certain cases,
the same permissions will be extended to multiple resources separately,
and those can be represented using a single statement, instead of one
per each resource. This feature uses a role-local singleton construct
to ensure only one statement is created for a given permission template,
so as to minimize the size of the resulting policy.

The feature is being used in order to avoid creating extremely large
policy documents when adding CodePipeline actions to deploy a number of
CloudFormation stacks using the same ChangeSet name (using a single
statement instead of one per stack).

When trying to make minimal-permission IAM policies, it can be necessary to ensure the policy remains as compact as possible. In certain cases, the same permissions will be extended to multiple resources separately, and those can be represented using a single statement, instead of one per each resource. This feature allows code to select a "singleton" SID for their permissions, and look it up from an existing PolicyDocument so resources can be added to it instead of new statements being created. The feature is being used in order to avoid creating extremely large policy documents when adding CodePipeline actions to deploy a number of CloudFormation stacks using the same ChangeSet name (using a single statement instead of one per stack). BREAKING CHANGE: The `sid` attribute of the `PolicyStatement` class is now read-only and must be set at construction time if an `sid` is to be used. This also enables guaranteeing `sid` uniqueness in palces where it is required, such as SNS Topic policies.

eladb · 2018-10-25T10:03:54Z

packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts

-      .addAction('cloudformation:ExecuteChangeSet')
-      .addResource(stackArnFromName(props.stackName))
-      .addCondition('StringEquals', { 'cloudformation:ChangeSetName': props.changeSetName }));
+    const sid = uuid(`PipelineExecuteChangeSetAction-${props.changeSetName}`);


Why is it so important that this will be globally unique instead of just a well known SID that this construct uses and will not likely be defined by other users on the same policy? Feels like over doing it a little.

I need to have an SID for each ChangeSetName because of that being presented in the Condition. And I don't know that I can guarantee the change set names will always be "Sid-friendly" (either in length or content). I just felt using an opaque hash would be easy.

I also considered using a well-known sting prefix, then a hash of the changeSet name... that brings legibility improvements to the policy, but makes it bigger (which is precisely what we want to avoid there).

eladb · 2018-10-25T10:06:10Z

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

@@ -257,11 +265,6 @@ export class PolicyStatement extends Token {
    return this.resource && this.resource.length > 0;
  }

-  public describe(sid: string): PolicyStatement {


I hate the name of this method, but why did you decide that this must be passed in the ctor? Why not just rename this to addSid(sid)

I kinda wanted it to be immutable... There's a bunch of places that need to guarantee uniqueness (and presence) of SIDs (like SNS Topic policies), and it felt wrong to not be able to offer this guarantee safely at construction time.

How about just protecting against double-setting the SID. I think that's what you are trying to prevent. You can see the downside of this change in the SNS code, where now users have to artificially supply a SID, whereas we can do that for them quite safely.

Yeah - well the alternative approach I mentioned is actually much much nicer and thus I won't touch this at all anymore... Yay.

packages/@aws-cdk/aws-iam/lib/role.ts

eladb · 2018-10-25T10:08:32Z

packages/@aws-cdk/aws-iam/lib/role.ts

@@ -137,6 +159,16 @@ export class Role extends Construct implements IIdentityResource, IPrincipal, ID
    this.defaultPolicy.addStatement(statement);
  }

+  public findOrAddStatement(statement: PolicyStatement): PolicyStatement {


This feels like a tricky one. What if the statement I want to add is different than the one already in the policy? It's not clear what this is doing in that case? Some people may expect those to be merged somehow.

I rather we just have tryFindStatement(sid) and then users can decide how to deal with the result.

I thought about this. And it felt like a race condition waiting to happen. I guess I can do that, though... Found it makes the usage a bit cumbersome (but fine, it's not like you need to do this often anyway).

I thought about providing stronger guarantees (aka the corresponding statement to match all non-undefined attributes of the "default" installment), but the PolicyStatement class is currently designed to be write-only...

Keep it simple. We are single threaded. I don't see any potential race conditions if the user writes:

s = tryFind() if (!s) { add(new S()) } else { s.update() }

We are single threaded

Right now we are. We don't have to always be. Ruby has been "single threaded" (by virtue of a GIL), wants to get rid of the GIL and is basically unable to because everything assumes (either knowingly or unknowingly) there is one. I don't want to be that guy. Aaaaanyway, as I said, I'm not touching this part anymore, so win win.

packages/@aws-cdk/aws-sns/lib/topic-ref.ts

eladb · 2018-10-25T10:09:35Z

packages/@aws-cdk/aws-sns/lib/topic-ref.ts

+   * the topic is improted (``Topic.import``), then this is a no-op.
+   *
+   * @param statement the statement to add to the policy. It must have an Sid
+   *                  that has not yet been used in this resource policy.
   */
  public addToResourcePolicy(statement: iam.PolicyStatement) {


Why do we require that the user supplied an SID. If there is no SID, why not allocate one for the user? What's the value in this?

Immutability of the SID - I can guarantee that an SID is not used twice in a given Topic policy (that'd be illegal), and I can be suer it doesn't change after me. Of course, I could also do the validation at synthesis time in a validate function, but fail fast, right?

RomainMuller · 2018-10-25T12:36:13Z

I've had an idea to create a separate PolicyResource instance and use this instead of digging into the role-managed one... I'm going to try this out and see how it feels. Might offer something that's a lot cleaner.

…truct on the policy editor side

packages/@aws-cdk/aws-sns/test/test.sns.ts

packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts

…m actions and conditions instead

RomainMuller · 2018-10-26T09:02:48Z

Okay I changed so one doesn't need to come up with a statement template "id" by hand anymore, one is created based on the template's content - this should be more robust.

eladb · 2018-10-28T07:35:40Z

@RomainMuller better

Bug Fixes ========= * **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1)) * **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb)) * **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9)) * **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be)) * Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5)) Features ========= * **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291) * **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c)) * **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051) * **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062) * **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442)) * **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e)) * don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989) * **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab)) * add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb)) * **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643) * **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf)) BREAKING CHANGES ========= * The ec2.Connections object has been changed to be able to manage multiple security groups. The relevant property has been changed from `securityGroup` to `securityGroups` (an array of security group objects). * **aws-codecommit:** This modifies the default behavior of the CodeCommit Action. It also changes the internal API contract between the aws-codepipeline-api module and the CodePipeline Actions in the service packages. * **applets:** The applet schema has changed to allow Multiple applets can be define in one file by structuring the files like this: * **applets:** The applet schema has changed to allow definition of multiple applets in the same file. The schema now looks like this: applets: MyApplet: type: ./my-applet-file properties: property1: value ... By starting an applet specifier with npm://, applet modules can directly be referenced in NPM. You can include a version specifier (@1.2.3) to reference specific versions. * **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely that this would be sufficient to interact with a queue. Alternatively you can use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if there's a need to only grant this action.

Bug Fixes ======== * **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1)) * **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb)) * **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9)) * **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be)) * Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5)) Features ======== * don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989) * **app-delivery:** CI/CD for CDK Stacks ([#1022](#1022)) ([f2fe4e9](f2fe4e9)) * add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb)) * **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291) * **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c)) * **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051) * **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062) * **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442)) * **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e)) * **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab)) * **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643) * **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf)) * Update to CloudFormation resource specification v2.11.0 BREAKING CHANGES ======== * The ec2.Connections object has been changed to be able to manage multiple security groups. The relevant property has been changed from `securityGroup` to `securityGroups` (an array of security group objects). * **aws-codecommit:** this modifies the default behavior of the CodeCommit Action. It also changes the internal API contract between the aws-codepipeline-api module and the CodePipeline Actions in the service packages. * **applets:** The applet schema has changed to allow Multiple applets can be define in one file by structuring the files like this: * **applets:** The applet schema has changed to allow definition of multiple applets in the same file. The schema now looks like this: applets: MyApplet: type: ./my-applet-file properties: property1: value ... By starting an applet specifier with npm://, applet modules can directly be referenced in NPM. You can include a version specifier (@1.2.3) to reference specific versions. * **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely that this would be sufficient to interact with a queue. Alternatively you can use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if there's a need to only grant this action.

RomainMuller requested review from skinny85, rix0rrr and eladb October 25, 2018 08:34

eladb suggested changes Oct 25, 2018

View reviewed changes

Fix tyos

ff0c324

Stop messing with the statements, instead use a singleton-ifying cons…

46d1b65

…truct on the policy editor side

eladb reviewed Oct 25, 2018

View reviewed changes

packages/@aws-cdk/aws-sns/test/test.sns.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-cloudformation/lib/pipeline-actions.ts Outdated Show resolved Hide resolved

RomainMuller changed the title ~~feat(iam): Allow looking up a statement by sid~~ feat(codepipeline/cfn): Use fewer statements for pipeline permissions Oct 26, 2018

RomainMuller added 2 commits October 26, 2018 11:00

Stop specifying the statement template key manually, determine it fro…

adec992

…m actions and conditions instead

Revert spurious whitespace change

81ff065

eladb approved these changes Oct 28, 2018

View reviewed changes

RomainMuller merged commit 8f4c2ab into master Oct 29, 2018

RomainMuller deleted the rmuller/iam-lookup-statement-by-sid branch October 29, 2018 13:36

RomainMuller mentioned this pull request Nov 2, 2018

codepipeline: DescribeChangeSet permissions too restrictive for PipelineCreateReplaceChangeSetAction #1034

Closed

rix0rrr mentioned this pull request Nov 6, 2018

v0.15.0 #1094

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codepipeline/cfn): Use fewer statements for pipeline permissions #1009

feat(codepipeline/cfn): Use fewer statements for pipeline permissions #1009

RomainMuller commented Oct 25, 2018 •

edited

Loading

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

eladb Oct 25, 2018

RomainMuller Oct 25, 2018

RomainMuller commented Oct 25, 2018

RomainMuller commented Oct 26, 2018

eladb commented Oct 28, 2018

feat(codepipeline/cfn): Use fewer statements for pipeline permissions #1009

feat(codepipeline/cfn): Use fewer statements for pipeline permissions #1009

Conversation

RomainMuller commented Oct 25, 2018 • edited Loading

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

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

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 commented Oct 25, 2018

RomainMuller commented Oct 26, 2018

eladb commented Oct 28, 2018

RomainMuller commented Oct 25, 2018 •

edited

Loading