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

feat(aws-cloudformation): add permission management to CreateUpdate and Delete Stack CodePipeline Actions #880

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Oct 9, 2018

Follow-up from #843.


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

@skinny85
Copy link
Contributor Author

skinny85 commented Oct 9, 2018

My refactoring changed the order of some permissions.

@skinny85 skinny85 force-pushed the feature/cfn-stack-action-perms branch from 0af0279 to 0213e6b Compare October 9, 2018 20:58
@skinny85
Copy link
Contributor Author

skinny85 commented Oct 9, 2018

My refactoring changed the order of some permissions.

In 2 integ tests, as it turns out.

@eladb
Copy link
Contributor

eladb commented Oct 10, 2018

@skinny85 can you please add a PR description and make sure your PR title adheres to conventionalcommits

@@ -210,6 +210,10 @@ export abstract class PipelineCloudFormationDeployAction extends PipelineCloudFo
this.role.addToPolicy(new iam.PolicyStatement().addAction('*').addAllResources());
}
}

props.stage.pipelineRole.addToPolicy(new iam.PolicyStatement()
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining why this is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't reckon that's needed for all actions (you need this only to CreateReplaceChangeSet, but not to ExecuteChangeSet, for example). Factoring this here violates the principle of least privilege to me. Since this allows passing a role with permissions to do "everything" (via CloudFormation), I would argue we need this to be as tight as possible.

Copy link
Contributor Author

@skinny85 skinny85 Oct 10, 2018

Choose a reason for hiding this comment

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

It's not needed for all Actions. This code has been moved to PipelineCloudFormationDeployAction, of which all subclasses need it. ExecuteChangeSet extends a different class (PipelineCloudFormationAction), and thus does not have this permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a comment explaining why this is needed

Done.

@skinny85 skinny85 changed the title Add permission management to CreateUpdate and Delete Stack CFN CodePipeline Actions feat(aws-cloudformation): add permission management to CreateUpdate and Delete Stack CodePipeline Actions Oct 10, 2018
@skinny85 skinny85 force-pushed the feature/cfn-stack-action-perms branch from 0213e6b to 64810de Compare October 10, 2018 19:10
@skinny85
Copy link
Contributor Author

Updated the comment according to Elad's feedback.

@skinny85 skinny85 force-pushed the feature/cfn-stack-action-perms branch from 64810de to 3ace300 Compare October 11, 2018 21:25
@skinny85
Copy link
Contributor Author

Rebased to solve a conflict.

@skinny85 skinny85 merged commit 8b3ae43 into aws:master Oct 11, 2018
@skinny85 skinny85 deleted the feature/cfn-stack-action-perms branch October 11, 2018 21:55
RomainMuller added a commit that referenced this pull request Oct 12, 2018
__IMPORTANT NOTE__: This release includes a [breaking change](#845)
in the AWS CodePipeline construct library:
* The `inputArtifacts` and `outputArtifacts` properties of `Action` were intended for internal usage
  only, and have consequently been renamed to `_inputArtifacts` and `_outputArtifacts` respectively.
* The `artifact` property of `Action` classes was renamed to `outputArtifact`.
* The `artifactName` property of `Action` classes was renamed to `outputArtifactName`.
* It is no longer possible to add output artifacts to `Actions` by instantiating `Artifact`.

This release also includes a [fix](#911) for a bug
that would make the toolkit unusable for multi-stack applications. In order to benefit from
this fix, a globally installed CDK toolkit must also be updated:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.12.0 (build ...)
```

Like always, you will also need to update your project's library versions:

|Language|Update?|
|--------|-------|
|JavaScript/TypeScript (npm)|[`npx npm-check-updates -u`](https://www.npmjs.com/package/npm-check-updates)|
|Java (maven)|[`mvn versions:use-latest-versions`](https://www.mojohaus.org/versions-maven-plugin/use-latest-versions-mojo.html)
|.NET (NuGet)|[`nuget update`](https://docs.microsoft.com/en-us/nuget/tools/cli-ref-update)

* **aws-cdk:** multi-stack apps can be synthesized or deployed [#911](#911).
* **@aws-cdk/aws-codebuild:** allow passing oauth token to GitHubEnterpriseSource [#908](#908)

* **@aws-cdk/aws-codepipeline:** make input and output artifact names optional when creating Actions. [#845](#945)

* **@aws-cdk/aws-cloudformation:** add permission management to CreateUpdate and Delete Stack CodePipeline Actions. [#880](#880)
RomainMuller added a commit that referenced this pull request Oct 12, 2018
* **aws-codebuild:** allow passing oauth token to GitHubEnterpriseSource ([#908](#908)) ([c23da91](c23da91))
* **toolkit:** multi-stack apps cannot be synthesized or deployed ([#911](#911)) ([5511076](5511076)), closes [#868](#868) [#294](#294) [#910](#910)

* **aws-cloudformation:** add permission management to CreateUpdate and Delete Stack CodePipeline Actions. ([#880](#880)) ([8b3ae43](8b3ae43))
* **aws-codepipeline:** make input and output artifact names optional when creating Actions. ([#845](#845)) ([3d91c93](3d91c93))

* **aws-codepipeline:** this commit contains the following breaking changes:
* Rename 'artifactName' in Action construction properties to 'outputArtifactName'
* Rename the 'artifact' property of Actions to 'outputArtifact'
* No longer allow adding output artifacts to Actions by instantiating the Artifact class
* Rename Action#input/outputArtifacts properties to _input/_outputArtifacts

Previously, we always required customers to explicitly name the output artifacts the Actions used in the Pipeline,
and to explicitly "wire together" the outputs of one Action as inputs to another.
With this change, the CodePipeline Construct generates artifact names,
if the customer didn't provide one explicitly,
and tries to find the first available output artifact to use as input to a newly created Action that needs it,
thus turning both the input and output artifacts from required to optional properties.
@RomainMuller RomainMuller mentioned this pull request Oct 12, 2018
RomainMuller added a commit that referenced this pull request Oct 12, 2018
* **aws-codebuild:** allow passing oauth token to GitHubEnterpriseSource ([#908](#908)) ([c23da91](c23da91))
* **toolkit:** multi-stack apps cannot be synthesized or deployed ([#911](#911)) ([5511076](5511076)), closes [#868](#868) [#294](#294) [#910](#910)

* **aws-cloudformation:** add permission management to CreateUpdate and Delete Stack CodePipeline Actions. ([#880](#880)) ([8b3ae43](8b3ae43))
* **aws-codepipeline:** make input and output artifact names optional when creating Actions. ([#845](#845)) ([3d91c93](3d91c93))

* **aws-codepipeline:** this commit contains the following breaking changes:
* Rename 'artifactName' in Action construction properties to 'outputArtifactName'
* Rename the 'artifact' property of Actions to 'outputArtifact'
* No longer allow adding output artifacts to Actions by instantiating the Artifact class
* Rename Action#input/outputArtifacts properties to _input/_outputArtifacts

Previously, we always required customers to explicitly name the output artifacts the Actions used in the Pipeline,
and to explicitly "wire together" the outputs of one Action as inputs to another.
With this change, the CodePipeline Construct generates artifact names,
if the customer didn't provide one explicitly,
and tries to find the first available output artifact to use as input to a newly created Action that needs it,
thus turning both the input and output artifacts from required to optional properties.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants