feat(app-delivery) IAM policy for deploy stack #1165

moofish32 · 2018-11-13T19:17:15Z

The changeset and apply changeset may need specific IAM permissions
and the user can now customize them via deployStackAction.role
Document updates for proper build stage configuration
Fixes Correct docs for app-delivery & improve usability #1151

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

eladb · 2018-11-15T08:44:05Z

@skinny85 can you take a look please?

rix0rrr

In general, looks good and thanks, just want to hear from @skinny85 about design decisions w.r.t. CFN capabitilies.

rix0rrr · 2018-11-15T08:52:51Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

+  /**
+   * The CloudFormation Capabilities enabled for the ChangeSet.
+   *
+   * @default None


I would default this to "anonymous" IAM capabilities, that should make 95% of all CDK templates work out of the box and doesn't introduce much appreciable risk.

People will then be able to opt out of IAM completely or opt-in to strongly-named IAM objects as they desire.

Also, an enum seems more ergonomic. I know it's not robust against future extension, but I do care about ergonomics today more than a hypothetical future extension, and we can always deal with that if it ever occurs (which I don't think it will any time soon).

Also, fullPermissions implies named-IAM, I'm seeing in the source of the underlying construct:

https://github.com/awslabs/aws-cdk/blob/8e701adfc503d3338ae890c74d4903607c4130cd/packages/%40aws-cdk/aws-cloudformation/lib/pipeline-actions.ts#L201

Does named-IAM subsume anonymous IAM?

I guess this is a request for upstream change then.

@skinny85 ?

The CloudFormation documentation states that NamedIAM implies IAM, and that you need not specify both.

I would agree with defaulting to IAM in app-delivery, while letting the CFN L2 default be nothing. This is L3 so we can allow opinion in (and CDK applications will very often make use of IAM resources).

Now the fact that it's an array of CloudFormationCapabilities is because that is what CloudFormation accepts (even though there is no apparent use for it just yet). Maybe it's future-proofing? But I guess if the API we use is future-proofed, maybe there's a reason and we should "leak" the future-proofing out to avoid a bad surprise later?

That future-proofing is exactly what I'm opposing to.

I would default this to "anonymous" IAM capabilities, that should make 95% of all CDK templates work out of the box and doesn't introduce much appreciable risk.

People will then be able to opt out of IAM completely or opt-in to strongly-named IAM objects as they desire.

Also, an enum seems more ergonomic. I know it's not robust against future extension, but I do care about ergonomics today more than a hypothetical future extension, and we can always deal with that if it ever occurs (which I don't think it will any time soon).

@rix0rrr I'm confused here. Is anonymous an actual thing or are we saying just change the word none to anonymous because that's a better description? Right now if you don't create IAM entities the capabilities is empty, so I'm confused on the wording?

@moofish32, I believe @rix0rrr means "anonymous IAM permissions" (as opposed to "named IAM permissions").

I agree wholeheartedly with Rico and Romain here. In general, we don't do these kind of future proofing in the CDK - we try to give a nicer API for the common case.

So, I would proceed as follows here:

Introduce a new enum in this module, with 3 values: None, AnonymousIAM, NamedIAM.

Make capabilities in PipelineDeployStackActionProps be of this new enum type (not an array), with default AnonymousIAM.

After this PR, open a separate PR to move this enum to the aws-cloudformation package, and change the API of the CFN Actions themselves to use it.

Does this make sense?

If I can do it all in this PR should I? I haven't looked at how difficult this would be plumb.

Probably easier to leave it out of this PR, as it's a breaking change. But knock yourself out if you want - I don't think it will be super difficult :).

RomainMuller

That's pretty good! There's a couple of things that I would like to have fixed (nothing major, really). I would also plan to give the README.md example the .lit.ts treatment (but don't you worry about this right now - I'll do it once this PR is merged) so we can make sure it at least compiles going forward.

RomainMuller · 2018-11-15T15:24:16Z

packages/@aws-cdk/app-delivery/README.md

 ```

 #### `buildspec.yml`
+The repository should contain a file at the root level named `buildspec.yml`.


That's only true if you don't specify an inline buildspec with your CodeBuild project, so I wouldn't phrase it like so.

RomainMuller · 2018-11-15T15:24:48Z

packages/@aws-cdk/app-delivery/README.md

 The `PipelineDeployStackAction` expects it's `inputArtifact` to contain the result of synthesizing a CDK App using the
 `cdk synth -o <directory>` command.

 For example, a *TypeScript* or *Javascript* CDK App can add the following `buildspec.yml` at the root of the repository
 configured in the `Source` stage:

+Example contents of `buildspec.yml` (note: `buildspec.yaml` is not compatible)


The precision on .yaml is definitely a good one - I'd have moved it in the paragraph just above.

RomainMuller · 2018-11-15T15:26:10Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

@@ -41,6 +41,29 @@ export interface PipelineDeployStackActionProps {
   * @default ``createChangeSetRunOrder + 1``
   */
  executeChangeSetRunOrder?: number;
+
+  /**
+   * The role to use when creating and executing the ChangeSet.


I would copy the comment from the PipelineCreateReplaceChangeSetActionProps attribute (same for the other two). I think they're pretty detailed and they avoid insisting so much on the implementation detail here (customer is really just interesting at deploying a stack - the fact there is a ChangeSet being created, then executed, is mostly irrelevant).

RomainMuller · 2018-11-15T15:31:25Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

-    new cfn.PipelineCreateReplaceChangeSetAction(this, 'ChangeSet', {
+    this.stack = props.stack;
+
+    const fullPermissions = props.fullPermissions === true;


I don't think this is needed. fullPermissions defaults to false in PipelineCreateReplaceChangeSetAction (and I reckon we're quite unlikely to change this default, as it has pretty drastic security implications).

RomainMuller · 2018-11-15T15:32:03Z

packages/@aws-cdk/app-delivery/package.json

@@ -63,6 +65,8 @@
  ],
  "peerDependencies": {
    "@aws-cdk/aws-codepipeline-api": "^0.16.0",
+    "@aws-cdk/aws-cloudformation": "^0.16.0",


You'll want to make sure you merge in the 0.17.0 version bump & update those :)

RomainMuller · 2018-11-15T15:35:14Z

packages/@aws-cdk/app-delivery/README.md

@@ -52,9 +52,13 @@ const source = new codepipeline.GitHubSourceAction(pipelineStack, 'GitHub', {
  /* ... */
 });
 const project = new codebuild.PipelineProject(pipelineStack, 'CodeBuild', {
-  /* ... */
+  environment: {
+    buildImage: codebuild.LinuxBuildImage.UBUNTU_14_04_NODEJS_10_1_0,


I wouldn't specify this in the README.md, it could be distracting (or looking like it needs to be that).

Made a comment instead as an example.

skinny85 · 2018-11-15T18:54:45Z

packages/@aws-cdk/app-delivery/package.json

+    "@aws-cdk/aws-codepipeline": "^0.17.0",
+    "@aws-cdk/aws-s3": "^0.17.0",
+    "cdk-build-tools": "^0.17.0",
+    "cdk-integ-tools": "^0.17.0",


Some dependencies are duplicated here (cdk-build-tools, cdk-integ-tools, @aws-cdk/aws-s3, @aws-cdk/aws-codepipeline) - I would remove the extra ones so that the @aws-cdk namespace ones are grouped at the beginning.

copy and paste on resolution -- my bad fixing and will alphabetize too

skinny85 · 2018-11-15T18:55:25Z

packages/@aws-cdk/app-delivery/package.json

+    "@aws-cdk/aws-codebuild": "^0.17.0",
+    "@aws-cdk/aws-codepipeline-api": "^0.17.0",
+    "@aws-cdk/cdk": "^0.17.0",
+    "@aws-cdk/cx-api": "^0.17.0",


Nit: alphabetic order.

skinny85 · 2018-11-15T18:55:40Z

packages/@aws-cdk/app-delivery/package.json

@@ -63,6 +83,8 @@
  ],
  "peerDependencies": {
    "@aws-cdk/aws-codepipeline-api": "^0.17.0",
+    "@aws-cdk/aws-iam": "^0.17.0",
+    "@aws-cdk/aws-cloudformation": "^0.17.0",


Nit: alphabetic order.

skinny85 · 2018-11-15T19:04:29Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

+  /**
+   * The CloudFormation Capabilities enabled for the ChangeSet.
+   *
+   * @default None


@moofish32, I believe @rix0rrr means "anonymous IAM permissions" (as opposed to "named IAM permissions").

I agree wholeheartedly with Rico and Romain here. In general, we don't do these kind of future proofing in the CDK - we try to give a nicer API for the common case.

So, I would proceed as follows here:

Introduce a new enum in this module, with 3 values: None, AnonymousIAM, NamedIAM.

Make capabilities in PipelineDeployStackActionProps be of this new enum type (not an array), with default AnonymousIAM.

After this PR, open a separate PR to move this enum to the aws-cloudformation package, and change the API of the CFN Actions themselves to use it.

Does this make sense?

skinny85

Looks great. Other than the capabilities thing (which is not a problem in this PR, but something we actually want to change upstream), I only have minor nitpicks.

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

rix0rrr · 2018-11-16T10:17:48Z

I will leave it to @RomainMuller and @skinny85 to merge this PR if they're satisfied.

RomainMuller · 2018-11-16T10:44:28Z

I think we'll be good to merge once @skinny85's suggestion with respects to defining an enum locally is implemented.

moofish32 · 2018-11-16T19:44:05Z

I have updated the commit (via squash/rebase) and included this breaking change. If we are going to break let's just get on with it.

* The changeset and apply changeset can now apply role IAM permissions,
 and CloudFormation Capabilities
 * Updated CloudFormationCapabilities enum to include `None` and renamed `IAM` to `AnonymousIAM`.
 * Document updates for proper build stage configuration
 * Fixes #1151

BREAKING CHANGE: `CloudFormationCapabilities.IAM` renamed to
`CloudFormation.AnonymousIAM` and `PipelineCloudFormationDeployActionProps.capabilities?: CloudFormationCapabilities[]` has been changed to
`PipelineCloudFormationDeployActionProps.capabilities?:
CloudFormationCapabilities` no longer an array.

eladb

Related #286

packages/@aws-cdk/app-delivery/README.md

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

eladb · 2018-11-18T11:21:18Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

+   *
+   * @default false
+   */
+  fullPermissions?: boolean;


This should have the word admin in it in my mind to ensure the extent of this grant is clear. I would even just call this administrator. It's okay that this also implies "capabilities=NAMED_IAM"...

I went with adminPermissions because when I put in administrator I asked my self of what? I thought adminPermissions would be more clear, if you are set on administrator let me know.

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

moofish32 · 2018-11-18T15:00:13Z

I'm not sure who has the final merge rights, but I've attempted to incorporate the feedback. Let me know if any further changes are needed.

eladb

LGTM @skinny85 you get the final approval

RomainMuller · 2018-11-19T14:28:51Z

Resolved the conflicts in package.json (hopefully correctly - I've done it from the web flow).

skinny85

Sorry @moofish32 , I still have comments here :(.

packages/@aws-cdk/app-delivery/README.md

skinny85 · 2018-11-19T18:06:14Z

packages/@aws-cdk/app-delivery/README.md

+// Add the necessary permissions for you service deploy action. This role is
+// is passed to CloudFormation and needs the permissions necessary to deploy
+// stack. Alternatively you can enable [Administrator](https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_job-functions.html#jf_administrator
+) permissions above, 


Huh. This shows up as a new line without comments. Perhaps it's just GitHub's diff displaying it weirdly.

legit mistake on me, GitHub doesn't lie

skinny85 · 2018-11-19T18:07:57Z

packages/@aws-cdk/app-delivery/README.md

 The `PipelineDeployStackAction` expects it's `inputArtifact` to contain the result of synthesizing a CDK App using the
 `cdk synth -o <directory>` command.

 For example, a *TypeScript* or *Javascript* CDK App can add the following `buildspec.yml` at the root of the repository
 configured in the `Source` stage:

+Example contents of `buildspec.yml`.


Example contents of buildspec.yml:
// empty line here

version: 0.2 phases:

...

I think this now clashes with the sentence above? "For example, a TypeScript or Javascript CDK App can add the following buildspec.yml at the root of the repository configured in the Source stage:". Perhaps it always did, I just never noticed ;p.

skinny85 · 2018-11-19T18:08:51Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

+   * Acknowledge certain changes made as part of deployment
+   *
+   * For stacks that contain certain resources, explicit acknowledgement that AWS CloudFormation
+   * might create or update those resources. For example, you must specify CAPABILITY_IAM if your


I believe CAPABILITY_IAM is not the correct constant name anymore?

skinny85 · 2018-11-19T18:12:27Z

packages/@aws-cdk/app-delivery/test/test.pipeline-deploy-stack-action.ts

+      addAction('ec2:DescribeSecurityGroups').
+      addAction('ec2:CreateSecurityGroup').
+      addAction('ec2:RevokeSecurityGroupEgress').
+      addAction('ec2:RevokeSecurityGroupIngress').


Feel free to use the addActions method here, that takes a varargs argument, will be more concise.

OH... I didn't see there was one!

skinny85 · 2018-11-19T18:15:45Z

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

+    // else capabilities are defined use them
+    return capabilities;
+  }
+}


Sorry, maybe I wasn't clear before.

The capabilities should default to AnonymousIAM only in the app-delivery construct.

In the CFN Actions, the default should be undefined (that is, none), like it is now.

I wondered about this change. With adminPermissions being required it seems a bit odd this default is allow IAM, but it's a pretty heavy coin flip considering it's rare not to deploy a role with a service.

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

moofish32

@skinny85 -- implementing changes requested in next push, if build passes I think I've incorporated the changes, if not I'll be fixing shortly.

packages/@aws-cdk/app-delivery/README.md

moofish32 · 2018-11-20T06:03:49Z

packages/@aws-cdk/app-delivery/README.md

 The `PipelineDeployStackAction` expects it's `inputArtifact` to contain the result of synthesizing a CDK App using the
 `cdk synth -o <directory>` command.

 For example, a *TypeScript* or *Javascript* CDK App can add the following `buildspec.yml` at the root of the repository
 configured in the `Source` stage:

+Example contents of `buildspec.yml`.


moofish32 · 2018-11-20T06:04:34Z

packages/@aws-cdk/app-delivery/README.md

+// Add the necessary permissions for you service deploy action. This role is
+// is passed to CloudFormation and needs the permissions necessary to deploy
+// stack. Alternatively you can enable [Administrator](https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_job-functions.html#jf_administrator
+) permissions above, 


legit mistake on me, GitHub doesn't lie

moofish32 · 2018-11-20T06:05:59Z

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts

+   * Acknowledge certain changes made as part of deployment
+   *
+   * For stacks that contain certain resources, explicit acknowledgement that AWS CloudFormation
+   * might create or update those resources. For example, you must specify CAPABILITY_IAM if your


moofish32 · 2018-11-20T07:06:51Z

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

+    // else capabilities are defined use them
+    return capabilities;
+  }
+}


I wondered about this change. With adminPermissions being required it seems a bit odd this default is allow IAM, but it's a pretty heavy coin flip considering it's rare not to deploy a role with a service.

moofish32 · 2018-11-21T16:15:27Z

Thanks for the conflict resolution @RomainMuller. @skinny85 did I get all the fixes in here?

skinny85

Can you revert the changes to the expected JSON files, and see if npm run test still passes for the packages? I'm pretty sure none of the changes to those files are needed, and I'd rather not change them if there's no reason (they are our biggest line of defense against regressions).

Also, some optional minor comments.

skinny85 · 2018-11-21T21:41:03Z

packages/@aws-cdk/app-delivery/README.md

+ // new iam.PolicyStatement().
+ // ... addAction('actions that you need').
+ // add resource
+);


I wouldn't comment out the whole thing, but instead just the usecase-specific part:

deployServiceAAction.addToRolePolicy( new iam.PolicyStatement() .addAction('service:SomeAction') .addResource(myResource.myResourceArn) // add more Action(s) and/or Resource(s) here, as needed );

packages/@aws-cdk/app-delivery/README.md

skinny85 · 2018-11-21T21:42:57Z

packages/@aws-cdk/app-delivery/README.md

 The `PipelineDeployStackAction` expects it's `inputArtifact` to contain the result of synthesizing a CDK App using the
 `cdk synth -o <directory>` command.

 For example, a *TypeScript* or *Javascript* CDK App can add the following `buildspec.yml` at the root of the repository
 configured in the `Source` stage:

+Example contents of `buildspec.yml`.


I think this now clashes with the sentence above? "For example, a TypeScript or Javascript CDK App can add the following buildspec.yml at the root of the repository configured in the Source stage:". Perhaps it always did, I just never noticed ;p.

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

skinny85 · 2018-11-21T21:48:05Z

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

   */
-  NamedIAM = 'CAPABILITY_NAMED_IAM'
+  None = '',


I would actually make None the first enum value (I would sort them in the order of increasing capabilities).

Just so we all know ... that enum was alphabetized, in accordance with multiple other nits from @skinny85 😮

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit-build.expected.json

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-commit.expected.json

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-code-deploy.expected.json

packages/@aws-cdk/aws-codepipeline/test/integ.pipeline-events.expected.json

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

* The changeset and apply changeset can now apply role IAM permissions, and CloudFormation Capabilities * Updated CloudFormationCapabilities enum to include `None` * User must set adminPermissions boolean for pipeline action * app-delivery defaults pipelin-action capabilities to AnonymousIAM * Document updates for proper build stage configuration * Fixes aws#1151 BREAKING CHANGE: `CloudFormationCapabilities.IAM` renamed to `CloudFormation.AnonymousIAM` and `PipelineCloudFormationDeployActionProps.capabilities?: CloudFormationCapabilities[]` has been changed to `PipelineCloudFormationDeployActionProps.capabilities?: CloudFormationCapabilities` no longer an array. `PipelineCloudFormationDeployActionProps.fullPermissions?:` has been renamed to `PipelineCloudFormationDeployActionProps.adminPermissions:` and is required instead of optional.

moofish32 · 2018-11-22T01:29:45Z

alright @skinny85 one more pass when ever you are back from the holiday.

skinny85

Looks great. Thanks for the patience @moofish32 !

moofish32 force-pushed the b-roles-for-app-delivery branch from 478d1e8 to ea9aac3 Compare November 13, 2018 19:26

eladb requested a review from skinny85 November 15, 2018 08:43

rix0rrr approved these changes Nov 15, 2018

View reviewed changes

RomainMuller reviewed Nov 15, 2018

View reviewed changes

RomainMuller mentioned this pull request Nov 15, 2018

Correct docs for app-delivery & improve usability #1151

Closed

moofish32 force-pushed the b-roles-for-app-delivery branch from 9dcfa2f to 0d1fdb9 Compare November 15, 2018 16:52

skinny85 reviewed Nov 15, 2018

View reviewed changes

packages/@aws-cdk/app-delivery/lib/pipeline-deploy-stack-action.ts Show resolved Hide resolved

moofish32 force-pushed the b-roles-for-app-delivery branch from 0d1fdb9 to 471008e Compare November 15, 2018 19:27

moofish32 force-pushed the b-roles-for-app-delivery branch from 471008e to 2a60d58 Compare November 16, 2018 19:41

eladb suggested changes Nov 18, 2018

View reviewed changes

moofish32 force-pushed the b-roles-for-app-delivery branch 2 times, most recently from dfc4db7 to 9aa9eed Compare November 18, 2018 14:56

eladb approved these changes Nov 19, 2018

View reviewed changes

skinny85 suggested changes Nov 19, 2018

View reviewed changes

moofish32 commented Nov 20, 2018

View reviewed changes

moofish32 force-pushed the b-roles-for-app-delivery branch 2 times, most recently from f055eff to 51401d3 Compare November 20, 2018 08:35

skinny85 reviewed Nov 21, 2018

View reviewed changes

moofish32 force-pushed the b-roles-for-app-delivery branch from 5be9fa8 to 7754916 Compare November 22, 2018 01:24

skinny85 approved these changes Nov 28, 2018

View reviewed changes

eladb merged commit edc9a21 into aws:master Nov 28, 2018

feat(app-delivery) IAM policy for deploy stack #1165

feat(app-delivery) IAM policy for deploy stack #1165

Conversation

moofish32 commented Nov 13, 2018

eladb commented Nov 15, 2018

rix0rrr left a 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 Nov 15, 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

RomainMuller left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

rix0rrr commented Nov 16, 2018

RomainMuller commented Nov 16, 2018

moofish32 commented Nov 16, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Nov 18, 2018

eladb left a comment

Choose a reason for hiding this comment

RomainMuller commented Nov 19, 2018

skinny85 left a 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

Choose a reason for hiding this comment

moofish32 left a 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

moofish32 commented Nov 21, 2018

skinny85 left a 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

moofish32 commented Nov 22, 2018

skinny85 left a comment

Choose a reason for hiding this comment

RomainMuller Nov 15, 2018 •

edited

Loading